feat(studio): timeline magnetic snapping#2196
Conversation
e0cc5a5 to
7312dd0
Compare
055fc12 to
eb1e603
Compare
7312dd0 to
7da8833
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed as part of the 25-PR NLE overhaul stack. Full stack review on #2205 (the keystone). No blockers on this PR. — Miga
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: LGTM (green) — units clean, tie-break deterministic
Snapping in miniature. The module keeps pixels and seconds strictly separated at the module boundary, resolves them once inside snapMoveToTargets, and never conflates the two again.
The pointed queries
- Pixel-vs-time unit consistency.
packages/studio/src/player/components/timelineSnapping.ts:11—TIMELINE_SNAP_PX = 8is documented as pixel radius; conversion to seconds happens exactly once at :80 (thresholdSecs = TIMELINE_SNAP_PX / Math.max(pixelsPerSecond, 1)). Every downstream comparison is in seconds.Math.max(pps, 1)guards divide-by-zero — commendable. - Deterministic tie-break.
snapTimelineTimeat :55–:60 chooses the winner among equidistant targets byTYPE_PRIORITY[type](playhead=0 < clip-edge=1 < beat=2) — the same ordering used by the collector's dedup at :30. Order-independent and consistent between the collector's dedup and the picker's tie-break; that symmetry is what makes the fn deterministic. - Negative deltas / snap-to-zero. The collector at :27 rejects
time < 0outright, so no snap target can pull a clip below zero.snapMoveToTargetsclamps the candidate to[0, timelineDuration − duration]at :99–:100 and drops the highlight if clamping moves the clip off-target (:101) — no phantom snap indicator when it can't actually resolve. Test at :86–:90 pins this explicitly. - Threshold scaling with zoom. The
pps=10vspps=1000case at :92–:96 exercises the two extremes of the pixel↔time projection. Both directions verified.
Nits (non-blocking)
- Dedup tie-break at :30 is tested for
playhead > beat(:41–:49) but notclip-edge > beat; the code path is identical (min priority wins) so the invariant is by construction, but a one-line case would give the test more surface area if you're feeling generous.
Nothing here that would keep a case awake at night.
R1 by Via
eb1e603 to
e21c151
Compare
85079ea to
f441a84
Compare
e21c151 to
ed27a2e
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review — snap indicator drops on frame-quantized durations
timelineSnapping.ts:100-101:
const clamped = Math.max(0, Math.min(maxStart, Math.round(candidate * 1000) / 1000));
if (target && Math.abs(clamped - candidate) > 1e-6) target = null;The 1e-6 tolerance is meant to detect "did the timeline-bounds clamp actually pull us?" — but line 100 also rounds candidate to 1ms, and for the end-snap path candidate = endSnap.time - duration inherits duration's sub-ms bits. For any frame-quantized duration (1/24, 1/30, 1/60s), the rounding drift is ~5e-4, well above 1e-6, so target is nulled every single time.
Reproduced with dur = 1/24, snap target playhead at t=5:
{"start":4.958, "snapTime":null, "snapType":null}
The clip still snaps correctly (4.948→4.958); the snap-line indicator just vanishes at the moment it should show. UX regression not caught by tests because all shipped tests use integer durations.
Fix: gate the target-nulling on whether the clamp actually moved past the timeline bounds (candidate < 0 || candidate > maxStart), not on rounding delta. OR round candidate before the compare so both sides share the same precision.
— Rames Jusso
ed27a2e to
7f9c812
Compare
a399c01 to
ff3ddb7
Compare
3916b0f to
e0ad57b
Compare
ff3ddb7 to
b3ae6f0
Compare
28f4251 to
ca5caa8
Compare
b3ae6f0 to
bf710eb
Compare
What: new pure module timelineSnapping — snap-target collection and pixel-threshold time snapping (collectTimelineSnapTargets, snapTimelineTime, snapMoveToTargets) with tests. Why: the magnet math for clip drags/trims, reviewable standalone. How: new files only; type-only playerStore imports; consumers land with the drag engine. Test plan: bunx vitest run timelineSnapping.test.ts; tsc --noEmit; fallow audit clean.
ca5caa8 to
b910ef1
Compare
bf710eb to
f1f90c1
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed current head and verified the addressed feedback and required behavior. Approval is for this exact head; Graphite mergeability may still be pending.

What
new pure module timelineSnapping — snap-target collection and pixel-threshold time snapping (collectTimelineSnapTargets, snapTimelineTime, snapMoveToTargets) with tests.
Why
the magnet math for clip drags/trims, reviewable standalone.
How
new files only; type-only playerStore imports; consumers land with the drag engine.
Test plan
bunx vitest run timelineSnapping.test.ts; tsc --noEmit; fallow audit clean.
Stack position 5/25 — studio NLE overhaul (CapCut-parity timeline + canvas). Graphite manages bases; merge from #2192 upward. Temporary
TEMP(studio-dnd)fallow entries (unwired-component windows) are all removed by #2213 (app-shell swap), whose tree is byte-identical to the fully-verified integration branch.