Skip to content

fix(studio): trimming keeps the playhead still; clips snap to the ruler grid - #4986

Merged
miguel-heygen merged 7 commits into
mainfrom
fix/studio-trim-playhead-ruler-snap
Oct 4, 2026
Merged

miguel-heygen merged 7 commits into
mainfrom
fix/studio-trim-playhead-ruler-snap

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

What

Two timeline editing fixes in Studio:

  1. Trimming a clip's edge no longer moves the playhead. The preview still shows the frame at the dragged edge, but the playhead, the time readout and the current time stay where they were paused. Releasing or cancelling the trim puts the playhead's own frame back.
  2. Dragging or trimming a clip also snaps to the ruler's visible tick lines at the current zoom, with the same guide the clip-edge snap draws. The magnet toggle (N) turns it off together with the other snap targets.

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

  • Preview frame without a seek: store/previewFrameStore.ts owns the one way to show a frame without moving time. useTimelinePlayer subscribes and puts the frame on the adapter only (no liveTime publish, no setCurrentTime), 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. useTimelineClipDrag no longer takes onSeek.
  • The edge frame never becomes the transport time: while a paused preview frame is shown, getAdapter() (the one accessor every transport reader uses) hands out a view whose getTime() is the playhead's time and whose play() 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.
  • Trims snap to the playhead now: the old exclusion existed only because the edge drove the playhead; with the playhead fixed, includePlayhead had no other caller and is removed.
  • Ruler grid: getTimelineGridStep in timelineRulerGeometry.ts returns the spacing of the drawn lines (minor ticks when drawn, else major) from the same helpers generateTicks uses, 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.
  • A guide only where the clip lands: clip files store the local start (and, for a resize, the duration) to the centisecond. savedClipEdges in timelineElement.ts, beside toAuthoredStart, says where a clip's edges sit once saved, including clips nested in a sub-composition; guideIfSaved drops 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.
  • Priority: the grid is a fallback. A clip edge, playhead or beat within the snap radius always wins, on either edge of a moved clip.
  • There was no held snap-off modifier for clip drags (Alt unlinks linked clips), so none was added; the magnet toggle is the off switch.
  • Walk: tests/e2e/timeline-trim-snap.mjs with its own fixture, run in the existing Studio browser-check step of ci.yml.

Before

Mid-trim: the playhead has jumped to the dragged edge (readout 00:02).

Before: playhead follows the trimmed edge

A clip dropped 6 px past a ruler line stays 6.5 px off it.

Before: dropped clip sits off the ruler line

After

Mid-trim: the playhead stays at 1.2 s (readout 00:01); the guide marks the snapped edge.

After: playhead stays while the edge is trimmed

The same drop lands on the line (0.9 px, the centisecond save).

After: dropped clip lands on the ruler line

What I measured

  • Walk timeline-trim-snap.mjs in 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).
  • Unit tests (vitest), each file 3 runs, all exit 0: timelineSnapping.test.ts 20, timelineClipDragPreview.test.ts 48, timelineLayout.test.ts 46, useTimelineClipDrag.resize.test.tsx 27, useTimelinePlayer.seek.test.ts 19, useTimelinePlayer.shadowReload.test.ts 36, useTimelinePlayer.test.ts 34, Timeline.test.ts 83. Full packages/studio suite: 663 files, 7301 tests passed, exit 0. tsc --noEmit, oxlint and oxfmt clean; every touched production file is under 600 lines.
  • Each new test fails when its code is broken (17 deliberate breaks, only the targeted tests went red), among them: no grid line, grid not demoted across edges, preview calling seek(), preview applied while playing, frame not cleared at gesture end, no getAdapter view, 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

  • macOS or a GPU: the walk ran headless on Linux, so the captures prove behaviour, not pixels.
  • HyperFrames Desktop: it picks this up on its next Studio version bump; not walked there yet.
  • Group trims and group moves against the grid in a browser (covered only by unit tests).
  • Frame display mode in a browser; extreme zoom where lines fall between centiseconds (unit-tested only).
  • A clip whose authored duration runs past the composition end: the timeline shows it cut at the end, and moving it snaps that displayed tail (as on main). The saved tail sits further out. Fixing that needs the authored duration carried from the runtime manifest into timeline rows; left for a follow-up.

Test plan

  • Unit tests added/updated
  • Manual testing performed (browser walk above)
  • 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

@github-actions

github-actions Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Edit accuracy: accurate 1556 (base branch 1556), smooth 1433 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@miguel-heygen
miguel-heygen marked this pull request as ready for review October 4, 2026 07:47

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 setPreviewFrame and 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() updates currentTime, so the next view reads it.
    • play() uses a single getAdapter() view, so its rewind to the in-point sticks (useTimelinePlayer.ts:207-214).
  • Snapping:
    • The grid step comes from the same helpers generateTicks uses.
    • 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 +thresholdSecs rank).
    • Magnet off gives step 0.
  • Resize commit writes the preview's start/duration rounded to centiseconds (useTimelineEditing.ts:273-274), which matches savedClipEdges.

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: guideIfSaved with a 10 px tolerance; savedClipEdges without the host offset.
    • Preview frame: applied while playing; not restored on clear; view play() skips its seek; the rewind's seeked flag never set; transportAdapter never wraps; teardown keeps the frame.
  • Survived: computeResizePreview without || gridStep > 0 (:395). It is effectively equivalent: the playhead is always a target now, so trimTargets is 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 terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.mjs against a Studio dev server at the merge base and at this head (headless Chrome 1440x900, repo fixture). On the base it fails with playhead moved 306.0px during the trim and clip 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 whose getTime() is the playhead's time and whose play() 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. clampToDuration is identical to the old inline clamp.
  • Snapping. nearestGridLine takes 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 +thresholdSecs rank. The grid step comes from the helpers generateTicks uses, 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 with NODE_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

  1. 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 gives previewStart 2.217 with snapType null, 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.
  2. Contract (Somu's #2). Timeline's onSeek no longer receives trim previews. previewFrameStore is module-global and not exported, so a host with its own player loses the edge-frame preview without an error. The only in-repo useTimelinePlayer host (NLEContext) is fine. A line in the PR body would help, since Desktop has not been walked.
  3. Grid step vs ruler duration (Somu's #3). snapGridStep reads durationRef while the ruler reads displayDuration; they can differ only at the 14400 s cut.
  4. Test gap (new). The !isPlaying clause in transportAdapter (previewFrameStore.ts:35) is not pinned: with it removed, all 89 tests in the three useTimelinePlayer files 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.
  5. 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 first waitForFunction. 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)

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 4, 2026
Merged via the queue into main with commit 7859c0f Oct 4, 2026
94 checks passed
@miguel-heygen
miguel-heygen deleted the fix/studio-trim-playhead-ruler-snap branch October 4, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants