fix(studio): the timeline follows a paused playhead and zoom keeps it in place - #4712
miguel-heygen wants to merge 7 commits into
Conversation
…e playhead in place
jrusso1020
left a comment
There was a problem hiding this comment.
Review at bcaa4311d587721901dee7f89922f95a9ebce53d.
Verdict: REQUEST_CHANGES
The zoom half is solid and the paused follow is the right shape. One path breaks the promise that a person's own scroll while paused is never undone.
Findings
Blocking
1. The first preview reload after a keyboard pause scrolls the timeline back to the playhead.
useTimelinePlayhead.ts decides "the playhead moved" by comparing each liveTime value with lastLiveTimeRef, and that ref is only written by the liveTime subscriber. pause() in useTimelinePlayer.ts (the path Space and togglePlay take) stops the rAF ticks, then calls setCurrentTime(adapter.getTime()) and setIsPlaying(false). It never calls liveTime.notify. So after a pause the ref holds the last rAF tick time, while the store and the adapter hold a slightly later time.
The next reload (an edit that refreshes the preview) saves adapter.getTime() into pendingSeekRef, and the reload commit publishes it with liveTime.notify(startTime) (useTimelineSyncCallbacks.ts:249). That value differs from the ref, so moved is true. If the person scrolled away from the playhead meanwhile, revealPlayheadScrollLeft jumps the view back to it. The geometry hook's scroll restore then keeps the jumped position, because the scroll event updates lastScrollLeftRef first.
Repro at the hook level (drop into useTimelinePlayhead.test.tsx, it uses the existing harness):
it("keeps a person's scroll when a reload follows a keyboard pause", () => {
const scroll = scrollBox(0);
mount({ pps: 100, scroll });
act(() => usePlayerStore.setState({ isPlaying: true }));
act(() => liveTime.notify(30)); // last rAF tick
act(() => {
// pause(): store gets adapter.getTime(), no liveTime notify
usePlayerStore.getState().setCurrentTime(30.012);
usePlayerStore.setState({ isPlaying: false });
});
scroll.scrollLeft = 0; // the person scrolls back to the start
act(() => liveTime.notify(30.012)); // reload commit republishes the restored time
expect(scroll.scrollLeft).toBe(0);
});At this head it fails with expected 2425.2 to be +0.
A fix that passes this test and the 13 existing tests in the file: sync the ref when playback stops, in the existing unsubPlaying subscriber.
if (prev.isPlaying && !state.isPlaying) {
lastLiveTimeRef.current = state.currentTime;
place(lastLiveTimeRef.current, true);
}pause() sets currentTime before isPlaying, so state.currentTime is the adapter time there. The preview-click pause and the end-of-playback stop both publish through liveTime anyway. The same drift also feeds the zoom anchor's time, so this fixes that too. The gap there is sub-frame, so it does not matter on its own.
Non-blocking
2. userZoomCount in the anchor effect's dependency list is not covered by a test. If I drop it from the list, all 26 tests still pass. It matters when a toolbar zoom hits the zoom clamp. The counter moves but pps does not, so only that dependency re-runs the effect and moves previousZoomCountRef forward. Without it, the next window resize would look like a person's zoom and anchor on the playhead. A test for "zoom in at the max clamp, then resize a scrolled view, centre is kept" would pin it.
Claims checked
- "A person's zoom is told apart ... by a counter." True.
useTimelineZoom'ssetManualZoomPercentbumpsuserZoomCountbefore the store write, and both land in one React commit from the toolbar click and slideronChangehandlers. The edit pin (pinTimelineZoom) and the length re-pin inuseTimelineGeometry.tswritemanualZoomPercentdirectly, so they keep the centre rule. Pinch also goes through the wrapper, butskipCenterAnchorRefstill returns early first, so it keeps its cursor anchor. - "No scroll while the playhead is being dragged." True.
beginScrubsetsisDragging, andfinishScrubpublishes its last seek before it clears the flag, so a release past the edge does not reveal. - "A person's own timeline scroll while paused is never undone." Not true in the case in finding 1. It does hold when the same time is published again.
- "Four of the new tests fail on main's code." Checked by putting the three source files from
2195db5eback in place: 4 tests inuseTimelinePlayhead.test.tsxfail, plus the new toolbar test, since the field does not exist there. - The follow while playing is unchanged. The
playingbranch still callsgetTimelinePlaybackFollowScrollLeftwith the same inputs, and the drag and Fit guards are the same. - Scroll clamps: the anchor result is clamped to
[0, scrollWidth - clientWidth], and the reveal goes through the follow helper, which clamps to the same range. - Cleanup: the mount effect still unsubscribes both listeners. No new rAF or timer was added.
- New store field:
userZoomCountdefaults to 0. Its only readers are the playhead hook and the tests.
Tests run
- Mutation test, one change at a time, on the two touched test files. These changes each failed at least one test: removing the time-moved check, turning the paused follow off, using the playback follow line while paused, always anchoring at the centre, applying the 00:00 skip to zooms, dropping the reveal after the anchor, treating every scale change as a zoom, and removing the counter bump. Dropping
userZoomCountfrom the effect dependencies failed nothing (finding 2). Every change was reverted. - Repro test above: fails at this head. Passes with the suggested fix. The file was removed afterwards.
useTimelinePlayhead.test.tsxandTimelineToolbar.test.tsx: 26 passed.- Full Studio suite: 557 files passed, 1 skipped; 6013 tests passed, 16 todo.
tsc --noEmitinpackages/studio: clean.oxlintandoxfmt --checkon the 5 changed files: clean.
Checks
38 pass, 17 skipping, none failing or pending at this head.
Gate
reviewDecision REVIEW_REQUIRED, mergeStateStatus BLOCKED. No reviews exist on this PR yet.
— Rames
…fter a keyboard pause
What
The Studio timeline now keeps the playhead in view while the film is paused:
Unchanged: no scroll while the playhead is being dragged, none in Fit, and a person's own timeline scroll while paused is never undone. The view follows only when the playhead time changes. A window resize still keeps the view's middle time, or 00:00 when the view is at the start (#4411).
Why
Paused, the timeline only followed during playback, so a seek from the preview or the keyboard could leave the playhead off screen with no way to see where it went. Zooming in to look closely at the playhead moved it away instead.
Related work
Keeps #4411's rule for window resizes. A zoom set with the playhead at 00:00, such as a zoom restored when a project opens, still stays at 00:00.
How
useTimelinePlayhead.ts:useTimelineZoom'ssetManualZoomPercent(the toolbar buttons and slider) bumpsuserZoomCountin the store, while the zoom pin on a first edit and the re-pin after a length change write the percent directly. The anchor effect treats a scale change as a person's zoom only when the counter moved in the same commit. A person's zoom anchors on the playhead when it is on screen, then brings it into view if needed; a window resize, a pin or a re-pin keep the old centre anchor and the start-of-view rule.Test plan
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug
useTimelinePlayhead.test.tsxcovers: a paused seek off screen scrolls it into view; a paused seek on screen (even past the playback follow line) does not; a republished paused time does not undo a manual scroll; no scroll while dragging or in Fit; a toolbar zoom keeps the playhead's screen position, brings an off-screen playhead into view, and still anchors when it lands on the stored percent; a zoom with the playhead at 0 stays at 00:00; a resize keeps 00:00 (also after an edit pinned the zoom) and keeps the centre when scrolled; a length re-pin does not jump to an off-screen playhead.TimelineToolbar.test.tsx: a Zoom in click counts as a person's zoom. Four of the new tests fail on main's code.Nine targeted breaks (time-moved check, drag check, Fit check, paused reveal rule, playhead anchor, the 00:00 skip for zooms, resize treated as zoom, the counter bump, the resize 00:00 rule) each fail a test.
Touched test files pass together and one by one (149 tests);
tsc,oxlint,oxfmt, the comment checks and the dead-code audit pass.A repeat seek to the time the playhead already has (Home when it is at 0) does not scroll, because the view follows only a playhead change.
Before
Studio on main (2195db5), paused, on the pr-to-video film from the public hyperframes-launches repo: zoom in three times, step the playhead past the right edge with Shift+Right, then zoom in once more. The timeline stays at 00:00 while the playhead leaves the view, and the last zoom keeps 00:00 with the playhead far off screen.
follow-main-2195db5e1.mp4
After
The same steps on this branch (bcaa431): the timeline scrolls to keep the stepped playhead in view, and the last zoom keeps the playhead where it was on screen.
follow-fix-bcaa4311d.mp4