feat(studio): trimming a clip shows the snap guide and the frame at the dragged edge - #4163
Conversation
… 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
left a comment
There was a problem hiding this comment.
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 passeskeepPlaying.timelineSnapping.test.ts+27 lines:resolveSnapGuideprecedence,includePlayhead: falsebehavior.TimelineLanes.test.tsx+32 lines: beat highlight followssnapGuide, 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)
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:
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
computeResizePreviewpublishes the snap target it used (snapTime,snapType);previewGroupResizekeeps it only while the rendered edge is still on that target.resolveSnapGuide(moving, trimming)picks the guide;TimelineCanvasdraws it and passes it toTimelineLanesfor the beat highlight.useTimelineClipDragseeks the preview through Timeline's existingonSeek(withkeepPlaying: true) totrimPreviewTime(edge, start, duration)and restores the pre-gesture playhead from a ref on release, only when paused.collectTimelineSnapTargetsgainedincludePlayhead; the trim passesfalse.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 passeskeepPlaying.timelineSnapping.test.ts:resolveSnapGuide,includePlayhead: false.TimelineLanes.test.tsx: the beat highlight followssnapGuide, not the stale dragged-clip prop.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.
snapTime/snapTypefields, every gesture-teardown path, seek/play-state semantics, one fresh mutation per fix