fix(studio): trimming keeps the playhead still; clips snap to the ruler grid - #4986
Conversation
Edit accuracy: accurate 1556 (base branch 1556), smooth 1433 of thoseThe gate passes. Quarantined, measured but not gated (0) |
…s where clips save
…iew frame gets its own store
somanshreddy
left a comment
There was a problem hiding this comment.
Review at cf04a14a (full PR).
This is a comment, not an approval. I found no blockers. The playhead fix holds on every trim path I traced, and grid snapping matches the ruler. Three small items:
1. Low: a move can still snap to a spot it won't save at, now with no guide. guideIfSaved (timelineClipDragPreview.ts:293-303) drops the guide, but the snapped previewStart is still applied (:262-289). Repro (probe with computeDragPreview, not committed): a 31/30 s clip at 1440 px/s, clip-edge target at 3.25 s, dragged +1.22 s. The end-edge snap puts the preview at 2.217, snapType is null, and it saves at 2.22. So the clip sits flush with the edge during the drag, then jumps 4.3 px on release and lands 4.8 px past the edge. Without the snap it would preview and save at 2.22 with no jump. The preview/save gap was there before this PR, but main at least drew a guide. nearestGridLine already refuses a line it can't save (timelineSnapping.ts:66-67). Doing the same for a snap whose guide gets dropped (keep the unsnapped start) would close the reverse case for claim (3).
2. Low (contract): Timeline's onSeek no longer receives trim previews. The edge frame now only reaches useTimelinePlayer through subscribePreviewFrame (useTimelinePlayer.ts:372). Timeline and useTimelinePlayer are both public exports (src/index.ts:24-27), but previewFrameStore is not. A host that renders Timeline over its own player loses the edge-frame preview with no error. The in-repo hosts are all fine: TimelinePane goes through NLEContext, which uses useTimelinePlayer. Worth a line in the PR body or an export, since Desktop hasn't been walked.
3. Nit: grid step and ruler read different durations. snapGridStep passes durationRef (effectiveDuration, useTimelineClipDrag.ts:185-190), while the ruler gets displayDuration (useTimelineProviderState.tsx:449-450). With pps > 0 that only matters at the isSupportedTickDuration 14400 s cut (timelineRulerGeometry.ts:64). A comp just under 4 h whose display width pads past 14400 s would snap to a grid the ruler doesn't draw.
Checked, fine:
- Playhead:
- Both trim edges go through
setPreviewFrameand no longer seek (useTimelineClipDrag.ts:354). - Every teardown clears the frame through
stopClipDragAutoScroll(:401): release (timelineClipDragGestureLifecycle.ts:348), Escape, pointercancel, release outside the window and lostpointercapture (:172,:392-418), and unmount (:426-427). - There is no keyboard trim. Keyboard pickup is move-only. Razor doesn't seek.
- A real
seek()updatescurrentTime, so the next view reads it. play()uses a singlegetAdapter()view, so its rewind to the in-point sticks (useTimelinePlayer.ts:207-214).
- Both trim edges go through
- Snapping:
- The grid step comes from the same helpers
generateTicksuses. - A grid snap returns the saved centisecond, so it saves on the line.
- Clip edge, playhead and beat targets beat the grid on the same edge (
??=) and across edges (the+thresholdSecsrank). - Magnet off gives step 0.
- The grid step comes from the same helpers
- Resize commit writes the preview's
start/durationrounded to centiseconds (useTimelineEditing.ts:273-274), which matchessavedClipEdges.
Mutation (14 mutants, 8 targeted vitest files, tree restored and verified clean after each): 13 caught, 1 survived.
- Caught:
- Grid: skip the unsavable-line check; snap to the raw line; no grid demotion across edges; grid beats a real target; step = major ticks only.
- Guides:
guideIfSavedwith a 10 px tolerance;savedClipEdgeswithout the host offset. - Preview frame: applied while playing; not restored on clear; view
play()skips its seek; the rewind'sseekedflag never set;transportAdapternever wraps; teardown keeps the frame.
- Survived:
computeResizePreviewwithout|| gridStep > 0(:395). It is effectively equivalent: the playhead is always a target now, sotrimTargetsis never empty while the magnet is on. Optional: simplify the branch or give it a test where the target list is empty.
Walk / CI: the walk runs in Studio: timeline viewport gate, which is not a required check. It has no fixed sleeps (double rAF plus 10 s waitForFunction), so I don't expect it to flake. It doesn't assert that the edge frame is actually shown (unit tests cover that), and it reads "after" as soon as clipEnd changes.
CI at head: 72 pass, 20 pending, 0 fail, 1 skipped. All 10 required checks pass. The Before/After captures weren't viewable (the asset URLs return a login page), so I didn't view them.
What I ran: read the full diff from the merge base. Ran the 8 PR-listed vitest files: 313/313 pass (after building core). Ran the 14 mutants above and one computeDragPreview probe for #1. No Codex pass.
— Somu
terencecho
left a comment
There was a problem hiding this comment.
Approving cf04a14a. Trimming no longer moves the playhead, and a dragged or trimmed clip lands on the ruler line it snaps to. I found no blockers. I independently reproduced the one real edge in Somu's review (#1 below) and found one test gap he did not list (#4); both are low.
What I verified
- The browser walk catches the bug and the fix. I ran
timeline-trim-snap.mjsagainst a Studio dev server at the merge base and at this head (headless Chrome 1440x900, repo fixture). On the base it fails withplayhead moved 306.0px during the trimandclip b landed 6.5px off the nearest ruler line. At the head, 3 of 3 runs on a fresh fixture pass: the playhead stays at 1.203 s before and during the trim, and the dropped clip lands 0.9 px from the line. - Playhead paths.
getAdapter()is the one accessor the transport reads:play,pause,seek, the RAF loop,initializeAdapter(reload hand-over) and the keyboard handlers all go through it. While a paused preview frame is shown they get a view whosegetTime()is the playhead's time and whoseplay()starts from it, unless the caller seeked through it first (previewFrameStore.ts:20–37). The subscriber puts the playhead's frame back when the preview clears and does nothing while playing.clampToDurationis identical to the old inline clamp. - Snapping.
nearestGridLinetakes the nearest multiple, saves it at its centisecond and refuses a line that saves a pixel or more away. A real target (clip edge, playhead, beat) beats the grid on the same edge, and across edges via the+thresholdSecsrank. The grid step comes from the helpersgenerateTicksuses, so the grid and the ruler draw the same lines. Magnet off gives step 0. - Tests. The seven PR-listed vitest files I ran (all except
Timeline.test.ts) pass 230/230 withNODE_ENV=test. Somu's 14 mutants plus the walk difference cover the targeted behaviour. - Captures. I viewed all four. Before: the playhead jumps to the trimmed edge (readout 00:02), and the dropped clip sits off the ruler line. After: the playhead stays at 00:01 with a guide at the trimmed edge, and the dropped clip sits on the line.
Non-blocking
- Preview/save gap now has no guide (Somu's #1). I reproduced it with
computeDragPreview: a 31/30 s clip, clip-edge target at 3.25 s, 1440 px/s, dragged +1.22 s givespreviewStart2.217 withsnapTypenull, against 2.22 without the target. It only shows at extreme zoom with a non-centisecond duration, and main had the same jump with a misleading guide. Refusing the snap when its guide is dropped would close it. - Contract (Somu's #2).
Timeline'sonSeekno longer receives trim previews.previewFrameStoreis module-global and not exported, so a host with its own player loses the edge-frame preview without an error. The only in-repouseTimelinePlayerhost (NLEContext) is fine. A line in the PR body would help, since Desktop has not been walked. - Grid step vs ruler duration (Somu's #3).
snapGridStepreadsdurationRefwhile the ruler readsdisplayDuration; they can differ only at the 14400 s cut. - Test gap (new). The
!isPlayingclause intransportAdapter(previewFrameStore.ts:35) is not pinned: with it removed, all 89 tests in the threeuseTimelinePlayerfiles still pass. Without it, a trim during playback would hand the RAF loop the stored playhead time instead of the runtime's, so the readout would stall. The code is right as written; a test with playback on and a preview frame set would pin it. - Walk not re-runnable on the same data dir. It saves edits into
data/projects/timeline-trim-snap, so a second run without re-copying the fixture times out at the firstwaitForFunction. CI copies it fresh each time, so this only affects local reruns.
I read the diff from the merge base and ran the tests, walk and mutants locally. At review time CI at this head had finished with 93 passing, 1 skipped and none failing, including Studio: timeline viewport gate, which runs the walk. I did not exercise macOS or GPU rendering, HyperFrames Desktop, or group trims in a browser. This approval is not authorization to merge or deploy beyond what the gate already does.
— Review by tai (pr-review)
What
Two timeline editing fixes in Studio:
Why
The playhead used to follow the dragged edge during a trim and jump back on release, so the paused time was lost while editing (and in hosts that route seeks through their own handler it did not always come back). Snapping only to clips, beats and the playhead made it hard to place a clip on a round time.
Related work
Refs #4163 (added the edge-frame preview this keeps, without the playhead move).
How
store/previewFrameStore.tsowns the one way to show a frame without moving time.useTimelinePlayersubscribes and puts the frame on the adapter only (noliveTimepublish, nosetCurrentTime), ignores it while playing, and puts the playhead's frame back when it clears. The trim sets it on each move; the gesture teardown (release, Escape, cancel) clears it.useTimelineClipDragno longer takesonSeek.getAdapter()(the one accessor every transport reader uses) hands out a view whosegetTime()is the playhead's time and whoseplay()starts from it, unless the caller seeked through the view first (play's rewind to the in-point). So pausing, playing, frame-stepping or a reload promoting mid-trim all keep the paused position.includePlayheadhad no other caller and is removed.getTimelineGridStepintimelineRulerGeometry.tsreturns the spacing of the drawn lines (minor ticks when drawn, else major) from the same helpersgenerateTicksuses, so the grid and the ruler cannot drift. Snapping to it is arithmetic (nearest multiple), not a target list: at frame zoom a long timeline has over 100k lines.savedClipEdgesintimelineElement.ts, besidetoAuthoredStart, says where a clip's edges sit once saved, including clips nested in a sub-composition;guideIfSaveddrops any snap guide (grid, playhead, clip edge, beat) whose saved edge would land a pixel or more away. A grid line between two centiseconds snaps at its saved time, or not at all at extreme zoom.tests/e2e/timeline-trim-snap.mjswith its own fixture, run in the existing Studio browser-check step ofci.yml.Before
Mid-trim: the playhead has jumped to the dragged edge (readout 00:02).
A clip dropped 6 px past a ruler line stays 6.5 px off it.
After
Mid-trim: the playhead stays at 1.2 s (readout 00:01); the guide marks the snapped edge.
The same drop lands on the line (0.9 px, the centisecond save).
What I measured
timeline-trim-snap.mjsin headless Chrome on Linux, 1440x900, repo fixture: on main the playhead moved 306 px during the trim and the clip landed 6.5 px off the line (exit 1); with this branch 3 of 3 runs passed (playhead at 1.203 s before, during and after the trim within 1 px; clip 0.9 px from the line).timelineSnapping.test.ts20,timelineClipDragPreview.test.ts48,timelineLayout.test.ts46,useTimelineClipDrag.resize.test.tsx27,useTimelinePlayer.seek.test.ts19,useTimelinePlayer.shadowReload.test.ts36,useTimelinePlayer.test.ts34,Timeline.test.ts83. Fullpackages/studiosuite: 663 files, 7301 tests passed, exit 0.tsc --noEmit, oxlint and oxfmt clean; every touched production file is under 600 lines.seek(), preview applied while playing, frame not cleared at gesture end, nogetAdapterview, play ignoring its own rewind, an unsavable grid line snapping, a trim or move guide kept where the saved edge misses, a nested clip's host offset ignored, a move judged by a rounded duration, a grid where the ruler draws no lines.What I did NOT exercise
Test plan