Skip to content

feat(studio): trimming a clip shows the snap guide and the frame at the dragged edge - #4163

Merged
miguel-heygen merged 5 commits into
mainfrom
fix/timeline-snap-lines
Sep 19, 2026
Merged

miguel-heygen merged 5 commits into
mainfrom
fix/timeline-snap-lines

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

What a user sees

Dragging a clip's edge (trim) snapped to neighbouring clip edges, beats and the playhead, but never showed where it snapped: moving a clip draws a vertical guide at the snap target, trimming drew nothing. The preview also stayed on whatever frame the playhead was on, so a trim was done blind.

Now:

  • A trim draws the same guide as a move, at the target the edge snapped to, including when the edge is already exactly on it. A beat snap also lights the beat highlight.
  • While the edge is dragged, the preview shows the frame at that edge (the last visible frame for an end trim). When the gesture ends while paused, the playhead returns to where it was before the trim. A clip move keeps the preview still, as before.
  • A trim never changes play state: playback that was running keeps running, and releasing the trim while playing leaves the playhead where playback is.
  • The playhead is not a snap target for a trim (it follows the dragged edge, so snapping to it would be circular). Neighbouring clip edges and beats still snap.
  • Group trim: when another selected clip clamps the shared delta, the dragged edge is drawn where it actually renders, and no guide is drawn if that is off the snap target.

Playhead continuity across any commit during playback (the rewind visible after releasing a trim mid-playback) is fixed in #4151, not here.

How it works

  • computeResizePreview publishes the snap target it used (snapTime, snapType); previewGroupResize keeps it only while the rendered edge is still on that target.
  • One resolveSnapGuide(moving, trimming) picks the guide; TimelineCanvas draws it and passes it to TimelineLanes for the beat highlight.
  • useTimelineClipDrag seeks the preview through Timeline's existing onSeek (with keepPlaying: true) to trimPreviewTime(edge, start, duration) and restores the pre-gesture playhead from a ref on release, only when paused.
  • collectTimelineSnapTargets gained includePlayhead; the trim passes false.

Tests

  • useTimelineClipDrag.resize.test.tsx: guide on end and start trim, guide kept when on target, group clamp, playhead never a target, preview seek at the dragged edge, restore when paused, no restore when playing, every seek passes keepPlaying.
  • timelineSnapping.test.ts: resolveSnapGuide, includePlayhead: false.
  • TimelineLanes.test.tsx: the beat highlight follows snapGuide, not the stale dragged-clip prop.
  • One mutation per behaviour (guide publish, group clamp, beat highlight source, keep-playing on the preview and restore seeks, playhead exclusion in the trim and in the collector, paused-only restore): each turns the matching test red.

Before

Trimming a clip's edge next to a neighbour: no guide line, and the preview stays on the old frame while the edge is dragged.

trim-before.mp4

After

Plain trim: the guide appears at the neighbour's edge, the preview follows the dragged edge, and the playhead returns on release.

01-plain-trim.mp4

Group trim: the dragged edge is drawn where it renders when another selected clip clamps it, with no guide off the target.

02-group-clamp.mp4

Beat snap: the edge snaps to a beat and the beat highlight lights up.

03-beat-snap.mp4

The playhead is not a snap target: dragging an edge across it shows no guide.

04-playhead-not-target.mp4

Independent review

Two adversarial passes by reviewers who did not write the change; blocking findings fixed before this PR.

Audited consumers of the new snapTime/snapType fields, every gesture-teardown path, seek/play-state semantics, one fresh mutation per fix
Trusting the seek stub in the hook tests (call shape, not a real player)
Not exercised a real player in unit tests, reverse shuttle and end-of-media mid-gesture; the commit-during-playback rewind is #4151

… snaps

Independent review found: the group-trim guide and preview seek read the
raw single-clip snap even when a member clamp moved the rendered edge; a
beat-snapped trim never drew a guide because the beat highlight still read
the old move-only draggedClip prop; the 1/30 frame lead duplicated
STUDIO_PREVIEW_FPS.
…layhead

Both are regressions from the guide/preview seek this branch adds: the
preview and restore seeks now carry keepPlaying, so seek() only resumes
playback if it was already running; the playhead is dropped from the trim
snap set since the dragged edge drives its own preview seek, so snapping
to it was circular. Neighbour clip edges and beats remain valid targets.

@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 at eadcae6b — trim behavior matches PR body's claims, all pins hold

Read the diff against the six behavioral claims in the PR body and pinned each against the code.

1. Guide publish — snapTime/snapType flow, single source of truth

computeResizePreview now returns snapTime/snapType when the trim edge lands on a target within the pixel-radius, else null:

return {
  ...
  snapTime: snap?.time ?? null,
  snapType: snap?.type ?? null,
};

previewGroupResize republishes those only when the rendered edge (after member-clamp) still sits on the raw snap target (1ms tolerance):

const edgeTime = session.edge === "end" ? previewStart + previewDuration : previewStart;
const stillSnapped = next.snapTime != null && Math.abs(edgeTime - next.snapTime) < 1e-3;
setResizeState({
  ...
  snapTime: stillSnapped ? next.snapTime : null,
  snapType: stillSnapped ? next.snapType : null,
  ...
});

Matches the "no guide is drawn if that is off the snap target" claim exactly.

resolveSnapGuide(moving, trimming) picks move-when-started, else trim — one function, canvas + lane both consume it. No competing snap-guide paths.

2. Trim excludes the playhead as a snap target

collectTimelineSnapTargets gained includePlayhead (default true); the trim call site passes false:

const trimTargets = buildSnapTargets(
  resize.element.key ?? resize.element.id,
  !isMusicTrack(resize.element),
  false,          // <-- includePlayhead
);

collectTimelineSnapTargets:

if (input.playheadTime != null && input.includePlayhead !== false)
  add(input.playheadTime, "playhead");

buildSnapTargets cache key now includes the includePlayhead bit, so a mid-gesture toggle can't hand back stale (playhead-inclusive) targets. Circular snap prevented.

3. Preview follows the dragged edge — keepPlaying: true on every seek

useTimelineClipDrag wires onSeek and seeks to trimPreviewTime(edge, start, duration) on every resize preview update:

const setResizeState = (v: ResizePreviewResult) => {
  onSeekRef.current?.(trimPreviewTime(resize.edge, v.previewStart, v.previewDuration), {
    keepPlaying: true,
  });
  publishResizingClip(...);
};

Comment: "A trim never changes the play state: keepPlaying lets seek() decide, and it only resumes playback if it was already playing." Verified — every seek in the resize path carries the flag.

4. Playhead returns only when paused

trimSeekOriginRef.current ??= usePlayerStore.getState().currentTime; captures the pre-gesture playhead lazily on the first preview. On teardown:

if (trimSeekOriginRef.current != null) {
  if (!usePlayerStore.getState().isPlaying) {
    onSeekRef.current?.(trimSeekOriginRef.current, { keepPlaying: true });
  }
  trimSeekOriginRef.current = null;
}

Paused → restore. Playing → leave. Comment names the reason: "a backward jump would rewind live playback." That's exactly the case the PR body scopes to #4151.

5. Scope containment + #4151 relationship

12 files, all packages/studio/src/player/components/*. Base is main (not #4151's branch), so this doesn't stack. PR body is transparent: "Playhead continuity across any commit during playback (the rewind visible after releasing a trim mid-playback) is fixed in #4151, not here." This PR's own claim (trim never changes play state) is bounded to the play/pause state bit, which is preserved by keepPlaying: true — verified above. The visible-rewind-during-playback case is out of scope. Landing order between #4163 and #4151 is independent; if #4163 merges first, users see a mid-playback rewind on release-during-playing until #4151 lands, but the play/pause state itself is preserved.

6. Test coverage — hits the mutation surface

  • useTimelineClipDrag.resize.test.tsx +136 lines: guide on end and start trim, guide kept when on target, group clamp, playhead never a target, preview seek at the dragged edge, restore when paused, no restore when playing, every seek passes keepPlaying.
  • timelineSnapping.test.ts +27 lines: resolveSnapGuide precedence, includePlayhead: false behavior.
  • TimelineLanes.test.tsx +32 lines: beat highlight follows snapGuide, not the stale dragged-clip prop.

Author claims one mutation per behavior turns the matching test red. The test file names each target the specific invariant (guide-publish, group-clamp, beat-highlight-source, keep-playing on preview+restore, playhead-exclusion in trim and collector, paused-only restore). Structurally these are the right pins for the fixes shipped.

CI

Older 09:49/09:51 runs failed on captures (PR body needed the correct video embed shape) and Test (flaky). Author fixed the body; fresh runs at head all green: PR captures, regression, CI (Test included), Windows render verification, CodeQL, Player perf, preview-regression. Last-run-per-workflow all success / completed.

Stamp mechanics

hf-oss require_last_push_approval=true → this APPROVE binds to eadcae6b only.

— Review by tai (pr-review)

@miguel-heygen
miguel-heygen merged commit 8666720 into main Sep 19, 2026
97 of 121 checks passed
@miguel-heygen
miguel-heygen deleted the fix/timeline-snap-lines branch September 19, 2026 10:08
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.

2 participants