feat(studio): drag keyframes with live beat snapping

[vanceingalls]

Summary

Keyframe diamonds in the Studio timeline are now draggable with live preview and snap to the music beat grid (requires VITE_STUDIO_ENABLE_KEYFRAMES=1).

Drag model

• Start point → trims the front: the tween's position moves to the drop, the end stays fixed (duration adjusts).
• End point → resizes: duration changes, the start stays.
• Intermediate keyframe → moves only that keyframe within the tween (adjacent segments resize); other keyframes are untouched. Start/end moves on keyframe-array tweens remap the intermediates so their absolute times stay put.

Works for both flat from/to/set tweens (whose synthesized start/end points are the diamonds) and keyframes:[] tweens.

Beat snapping

While dragging, the keyframe snaps to the nearest beat within ~8px (clip-% → time → nearest beat → clip-%), centered exactly on the beat dot — the keyframe-cache clip-% precision was raised to 0.001% and commit rounding to 3 decimals so the marker lands precisely where dropped (beats sit on a 1ms grid).

Reliability

Keyframe edits were tightly coupled to the async DOM-edit selection session, which caused intermittent reverts. Now:

• The commit resolves the dragged element's selection + parsed animations on demand (awaited, via buildDomSelectionForTimelineElement + fetchParsedAnimations) and mutates through a selection-param override on the GSAP handlers — no dependency on the global session being loaded.
• Among same-group tweens (e.g. a fade-in and a fade-out on one element) it picks the one whose time window contains the dragged keyframe's original time, so the right tween is edited.
• An optimistic hold keeps the diamond at the dropped position until the cache round-trip lands, so it no longer flashes back to the old spot.

Structure

Pure match/plan logic (pickKeyframeTween, computeKeyframeMovePlan, remapKeyframes) lives in editor/keyframeMove.ts with unit tests; the timeline handler is a thin async orchestrator. Beat-snap math (snapKeyframePctToBeat) is in timelineEditing.ts.

🤖 Generated with Claude Code

[vanceingalls]

• feat(studio): drag keyframes with live beat snapping #1439 👈 (View in Graphite)
• feat(studio,cli): music beat detection with timeline guides + headless beats CLI #1424
• fix(core,player,studio): bound trimmed audio playback to the clip window #1430
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miguel-heygen]

Strengths

1. keyframeMove.ts:1–151 — clean separation of pure logic. Extracting pickKeyframeTween / computeKeyframeMovePlan / remapKeyframes into a side-effect-free module with its own unit tests is exactly the right call for this level of complexity. The test coverage in keyframeMove.test.ts covers the stale-cache no-op, same-group tween disambiguation, and the intermediate-remapping math — makes the behaviour reviewable without needing to spin up the full studio.

2. TimelineClipDiamonds.tsx:624–651 — mount-cleanup is thorough. Both the pending-hold timer and the in-flight drag listeners are covered by useEffect/dragCleanupRef: the unmount effect fires dragCleanupRef.current?.() (removes pointermove/pointerup from document) and clearTimeout(pendingTimerRef.current), so mid-drag unmounts (comp switch, clip delete, zoom-out collapse) don't leak.

3. timelineEditing.ts:796 — snap-range math is solid. Converting 8px to seconds via 8 / pixelsPerSecond makes the snap radius stay perceptually constant across zoom levels, which is the right UX. The Math.max(pixelsPerSecond, 1) guard avoids a division-by-zero on extreme zoom-out.

---

Blockers

None.

---

Important

1. StudioPreviewArea.tsx:37 — no cancellation guard on the async onMoveKeyframe. buildDomSelectionForTimelineElement is awaited inside a useCallback; if the component unmounts (or the active composition changes) while the Promise.all is in-flight, the continuation still runs and calls handleGsapRemoveKeyframe / handleGsapAddKeyframe on a stale selection. In practice the 2 s optimistic-hold timer is the only safeguard, but a silent stale-edit is possible. Consider an AbortController ref (or a mounted-ref early-exit) after the await:

   if (!mountedRef.current) return;

   Low probability but worth guarding, especially since rapid drags on slow machines can queue multiple concurrent invocations.

2. keyframeMove.ts:335 — isStartPoint heuristic for flat tweens (tweenOldPct <= 50). For a from/to tween the synthesised diamonds are always at 0 % and 100 %, so the current heuristic is never actually wrong. But the comment describing the contract ("start point → diamond at 0") is absent in the code, making the <= 50 look like an arbitrary threshold to future readers. A // synthesised start/end are always 0 and 100; this is just a fallback note would prevent a wrong refactor.

---

Nits

• keyframeMove.ts:246 — clampPct rounds to 0.01 % but round3 rounds to 0.001. Remapped intermediate keyframes use clampPct (2 dp) while the tween position/duration use round3 (3 dp). This is fine in practice since 0.01 % precision on a percentage is sub-millisecond, but it's worth a comment explaining why two different rounding levels exist.

• TimelineClipDiamonds.tsx:734 — effPct uses strict === on float.

  const effPct = (p: number): number => (drag && drag.origPct === p ? drag.pct : p);

  drag.origPct is set from kf.percentage in handlePointerDown and p is also kf.percentage, so they are always the same JS value (no intermediate math), making the === safe here. Worth a brief inline comment to forestall the "should this be an epsilon compare?" question from the next person.

• keyframeMove.test.ts:45 — el fixture is missing key/id. The pickKeyframeTween test uses a fixture that only has domId/selector, which is fine for coverage, but the KeyframeCacheEntry lookup in the real handler uses el.key ?? el.id — a test that exercises the el.key-only path would complete the coverage.

---

Verdict: APPROVE

Reasoning: The core beat-snap and keyframe-move logic is well-structured, pure, and unit-tested. The resource-cleanup audit passes. CI is fully green (UNSTABLE is just Graphite's mergeability check still pending, not a code signal). The async race noted above is the most meaningful gap but is low-probability and bounded by the 2 s timeout; it doesn't block shipping.

— Magi

[james-russo-rames-d-jusso]

Verdict: LGTM with concerns — keyframe drag-with-snap is built on the right primitives (pure plan logic + tested unit module), feature-flagged, deterministic, and the round-2/3 follow-ups in commits 2 + 3 already preempted the biggest holes I'd have flagged. Two UX concerns and a few coverage gaps below.

Verified at HEAD c99e9d9b

What ships

• editor/keyframeMove.ts — pure pickKeyframeTween + computeKeyframeMovePlan + remapKeyframes, with unit tests covering selector mismatch, group-aware pick, intermediate move, start-resize remap, and stale-cache no-op.
• timelineEditing.ts:snapKeyframePctToBeat — pure, ~8px window in time-domain (8 / max(pps, 1) seconds), returns input unchanged when no beat in range.
• TimelineClipDiamonds.tsx — pointer-down on diamond → document-level pointermove/pointerup, 4px dead-zone, optimistic hold until cache reflects commit, fallback 2s timeout, unmount-safe cleanup.
• StudioPreviewArea.onMoveKeyframe — async commit that awaits buildDomSelectionForTimelineElement + fetchParsedAnimations per drop, then dispatches handleGsapUpdateMeta/Remove/Add with a selectionOverride so the commit doesn't depend on the global DOM-edit session having caught up.
• useGsapAnimationsForElement + usePopulateKeyframeCacheForFile — clip-pct precision bumped 0.1% → 0.001% so beat-snapped keyframes center on the dot and the two caches agree.

Performance on drag-move hot path

• handleMove does: one bounds clamp → one call to snapKeyframePctToBeat → one setDrag. The snap is O(N) linear scan over beatTimes. Worst-case bound is small (10-min song @ 180 BPM ≈ 1800 beats), so ~1800 float subs/compares per move event — trivially fine at 60-120 events/sec.
• No DOM measurement, no layout reads, no allocation in the hot path beyond the setDrag state object.
• fetchParsedAnimations is at commit-time only (handleUp → onMoveKeyframe), NOT per move — one network round-trip per drop, which is correct.
• Nit: beatTimes is sorted (verified in beatDetection.ts), so binary-search would be O(log N). Not load-bearing at current N, but a 1-line win if you're polishing.
• Concern: handleMove closes over clipWidthPx captured at pointer-down. If the user zooms mid-drag the dx / clipWidthPx mapping skews. Edge case, but clipWidthPx is the only prop the closure depends on for math — consider a clipWidthPxRef if zoom-during-drag turns out to be a real workflow.

Wire contract from #1424

• Beat data flows store (useMusicBeatAnalysis → playerStore.beatAnalysis) → memoized remapBeatAnalysisToComposition in Timeline.tsx → passed to TimelineCanvas.beatAnalysis AND to snapKeyframePctToBeat via the onSnapKeyframePct closure. Memo deps are [beatAnalysis, musicElement, beatEdits] — stable across move events. Consumer is on the right side of the contract; no high-freq subscription on the hot path.
• Type MusicBeatAnalysis.beatTimes: number[] confirmed against @hyperframes/core/beats. Correct field, correct units (seconds, composition-absolute after remap).
• Beats are correctly treated as a global timeline reference — snap math uses el.start + (pct/100)*el.duration to get composition-abs time, then nearest beat, then back to clip-pct. So a non-audio clip's keyframes can still snap to the music's beat grid, which is the expected behavior.

Determinism + clip-relative interaction with #1448

• No Date.now, Math.random, performance.now, or fetch in any render-time code path in the new files (verified by grep).
• The snap function is pure: (el, pct, beatTimes, pps) → pct'. The commit math (computeKeyframeMovePlan) is pure.
• Clip-relative interaction looks right: the snap operates in clip-% (which is the persisted GSAP keyframe space post-#1448), maps to absolute time only to find the nearest beat, then maps back. The persisted value is clip-relative throughout. computeKeyframeMovePlan reconstructs absolute time from el.start + (newPct/100)*el.duration and writes back tween-relative position/duration — that round-trip stays in author-time and doesn't leak into runtime/render.

Blockers
None.

Concerns

• TimelineClipDiamonds.tsx:124-131 — Re-drag during the optimistic-hold window uses stale origPct. If the user drops keyframe A at position B, then clicks A again before the cache round-trip lands (the optimistic hold is rendering at B but kf.percentage in props is still A), handlePointerDown(e, kf.percentage) is invoked with pct = A, so dragRef.origPct = A. The diamond is visually at B but the new drag tracks from A. Result: tiny mouse motion at the visual B position re-commits with origPct=A, newPct=A+epsilon, which onMoveKeyframe then tries to look up at A (the now-removed keyframe) and either no-ops via the stale-cache guard or hits the wrong one. Probably rare in practice (sub-2-second double-drag) but a real footgun. Fix idea: during pendingRef.current === true, gate handlePointerDown (return early) until the hold releases, or read from pendingHeldPctRef when present.
• TimelineClipDiamonds.tsx:130-140 — No snap-disable modifier. Standard NLE convention (AE / Premiere / Resolve) is hold-Cmd or hold-Alt to bypass snap for precise placement. Right now once snap is on you can't place a keyframe between beats inside the ~8px window. Flag-gated feature so not blocking, but worth a follow-up — if (!me.altKey && snapPct) snapPct(rawPct) else rawPct reads natively.
• timelineEditing.ts:120 — snapKeyframePctToBeat has no direct unit test. Pure math, easy to pin: snaps within window, doesn't snap outside, returns unchanged on empty beats, picks nearest when between two. Matters because the snap window math (8 / max(pps, 1)) is zoom-dependent and a regression here would silently break "feel" without breaking any test in the move/plan module.
• StudioPreviewArea.tsx:onMoveKeyframe — No error surface if the network fetch fails. fetchParsedAnimations returns null on error and the commit silently no-ops. Drop with no change → optimistic hold renders the new position for 2s → diamond snaps back to original on hold-timeout. Worth at least a console.warn on the early-return for the diagnostics trail.

Nits

• timelineEditing.ts:131-137 — beatTimes is sorted by beatDetection.ts construction; O(log N) binary search would be cleaner than the linear scan. Not load-bearing at current N.
• TimelineClipDiamonds.tsx:158 — setDrag({ origPct: d.origPct, pct: d.pct }) in handleUp duplicates a setDrag from the last handleMove. Harmless re-render but redundant since d.pct is what's already in state.
• keyframeMove.ts:34 — clampPct rounds to 0.01 precision while the cache change bumps to 0.001 elsewhere — small precision asymmetry between the plan output and the cache. Worth aligning if you want exact round-trip parity.

Stack notes

• #1424 sibling under independent review; the wire contract this PR consumes (MusicBeatAnalysis.beatTimes) is the right shape.
• Three commits in this PR (initial + R2 follow-up + R3 beat-strip cosmetic). The R2 commit message lists exactly the issues a reviewer would have called out (selector-mismatch fallback removed, stale-cache no-op, optimistic-hold cache-match release, document-listener cleanup on unmount). Authors did the homework — appreciated.

CI
All required green at HEAD c99e9d9b: regression, preview-regression, player-perf, preflight. Graphite mergeability in_progress (expected — downstack #1424/#1430 open).

Review by Rames D Jusso

The base branch was changed.