perf(studio): stabilize virtualized keyframe retiming

[miguel-heygen]

Summary

Makes keyframe retiming independent from the mounted diamond that started the drag. A keyframe can move while its clip or row enters and leaves the virtualized window without losing its identity or leaking preview state into another gesture.

Changes

• Move the active retime lifecycle to viewport/window-scoped pointer handling.
• Scope preview and commit state to one gesture session and exact keyframe identity.
• Add horizontal autoscroll while retiming.
• Clear pending state on every terminal path and cover remount, cancellation, and dense-keyframe cases.

Stack

Family E, 2 of 7. Base: #2704. Next: #2706.

Validation

• Full Family E tip: 301 Studio test files passed; 3,271 tests passed; 18 todos.
• Studio typecheck, build, oxlint, oxfmt, diff checks, and Fallow audit pass with no new issues.
• Manual QA retimed a keyframe with virtualization enabled and verified the exact value changed.

[miguel-heygen]

• fix(studio): scope timeline ease focus lifecycle #2710
• perf(studio): stabilize virtualized beat gestures #2709
• fix(studio): scope timeline context targets #2708
• perf(studio): virtualize timeline marquee selection #2707
• perf(studio): stabilize virtualized timeline drops #2706
• perf(studio): stabilize virtualized keyframe retiming #2705 👈 (View in Graphite)
• perf(studio): stabilize virtualized clip gestures #2704
• main

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

[vanceingalls]

Verdict: APPROVE at 79752fdb53.

Consistent design language with #2704 (state machine active → committing → cancelled → complete, snapshot-before-clear on commit, element.key ?? element.id identity, session-epoch invalidation). The retime lifecycle is imperative (called from event handlers) rather than effect-mounted, which fits the "one retime session per viewport" model better than a global window mount. No blockers.

What I verified

Coordinator-per-viewport scoping (useTimelineKeyframeHandlers.ts:96-112)

keyframeRetimeCoordinators = new WeakMap<EventTarget, TimelineKeyframeRetimeCoordinator>(). The owner is target.closest("[data-timeline-scroll-viewport]") ?? target.ownerDocument. Each viewport gets its own {actor, pending, latest} coordinator; WeakMap keying means viewport unmount → coordinator + pending map GC together. Correct scope: a keyframe drag inside viewport A doesn't leak into viewport B.

Session-epoch invalidation is wired to store subscribe (line 413-425)

usePlayerStore.subscribe((state) => { if (state.timelineSessionEpoch !== actor.sessionEpoch || (actor.sourceWasPresent && !sourceStillPresent)) { coordinator.pending.clear(); coordinator.latest = null; cancel(actor); } }). Any store mutation that bumps timelineSessionEpoch OR removes the source element cancels the actor AND clears ALL pending entries (not just this one). This addresses the "row scrolled out AND deleted" case: if the element is gone, the pending retimes it fed into the coordinator are stale and must not survive.

Chained drag preservation via pending entries (line 212-231)

fromClipPercentage: pending?.clipPercentage ?? input.target.percentage. If a keyframe was retimed but the cache hasn't caught up, a chained retime starts from the pending destination, not the stale cache position. This is the "second drag can cross a neighbour that already moved past it" case the doc comment at line 118-123 describes. Correct.

Pending-entry retirement on tolerance match (line 197-210)

Math.abs(keyframe.percentage - pending.clipPercentage) < 0.2

Tolerance-based check (not equality) because cache writers round percentages. Retires the pending entry when the cache reflects a keyframe near the pending percentage AND the identity keys match. The identity constraint (input.keyframeKeyOf(keyframe) === key) prevents an unrelated sibling at that percentage from retiring the entry — direct answer to the doc comment at line 47-53. Well-designed.

Revert-race protection (line 338-346)

isLatest = coordinator.latest === nextPending; clearPending(); if (isLatest) actor.onSelect(fromTarget, false);

If drag A's onMove rejects AFTER drag B is committed, A's revert doesn't touch the selection (only clears its own pending entry). Selection stays on B's target. This is the specific "rejected first drag whose commit settles after a second one started" case in the doc comment at line 339-342. Excellent handling of the async-race edge.

Selection identity keys use key ?? id consistently — line 226, 415, 459, 488, 506 all follow the same pattern. No inconsistency across the file.

Scroll compensation (line 235-236, 383)

pointerXWithScroll = () => actor.lastClientX + (viewport?.scrollLeft ?? 0) - actor.originScrollLeft

Folds current scroll delta into pointer x — same mirror as the drag preview code in timelineClipDragPreview.ts. Both publishPreview (line 237) and the pointerMove threshold check (line 383) use pointerXWithScroll so movement detection and preview stay coherent while autoscroll pulls the viewport under a stationary pointer.

Teardown covers all four listeners + capture + RAF + store subscribe (line 406-412)

teardownListeners removes: pointermove, pointerup, pointercancel, keydown, and viewport's lostpointercapture. teardown() also stops autoscroll RAF and unsubscribes from the store. Called from both claimActorForCommit (commit path) and cancel (all cancellation paths). No listener leak paths identified.

Actor race: beginTimelineKeyframeRetime called while another actor exists (line 191)

if (coordinator.actor) cancel(coordinator.actor); — cancels the prior actor synchronously before setting up the new one. Between cancel and coordinator.actor = actor at line 231, coordinator.actor is null, but the code is synchronous so no re-entrancy window. Correct.

Viewport-null fallback — viewport = owner instanceof HTMLElement ? owner : null. When no [data-timeline-scroll-viewport] ancestor is found, viewport is null; all pointer-capture / scrollLeft / addEventListener calls null-check and fall back to window listeners only. Correct defensive path.

Non-blocker observations

1. sourceWasPresent gate at line 419 doesn't fire cancel if source was NEVER present. Edge case where a keyframe drag starts on an element that isn't in the elements list at gesture start. Unlikely to happen in practice (would require a race between mount and drag-start), but worth calling out — the check actor.sourceWasPresent && !sourceStillPresent treats never-present as "don't cancel". If it were ever hit, the drag would continue on an invalid target. Non-blocker because the entry-point predicate input.event.currentTarget implies the element is in the DOM when drag starts.

2. Store subscribe fires on every state mutation. The predicate is cheap (elements.some for identity match + one scalar comparison), but subscribe fires unconditionally. On a busy composition, this adds ~one predicate eval per store dispatch. Not a hot-path concern; probably fine.

3. actor.suppressNextClick() fires ONLY in claimActorForCommit, not in cancel. If a gesture is cancelled mid-drag AND the user releases the pointer immediately, the resulting click could still fire. In practice, cancel paths (Esc, pointer cancel, source unmount) don't correlate with a pointerup, so this is likely benign. Worth confirming with TimelineClipDiamonds.test.tsx if it covers this edge.

4. pending entries older than the current session epoch are only cleared on the NEXT beginTimelineKeyframeRetime call (line 193-195). If no next drag ever happens after a session change, stale pending entries stay in the coordinator until the viewport unmounts (WeakMap GC). Bounded but non-zero staleness. Non-blocker — the store subscribe at line 413-425 also clears them on session-epoch change while an actor is active, and no consumer reads pending entries for a non-active actor.

Family E stack ack

Family E, 2 of 7. Base is #2704 (approved above). Design language consistent — state machine, phase transitions, identity keying, autoscroll cleanup all mirror the base PR's pattern. The imperative-per-gesture entry (vs #2704's effect-mounted global lifecycle) is the correct choice for keyframe retiming since there's typically only ONE retime in flight per viewport at a time.

CI: 12 passing, 1 running, 0 failing at time of review.

— Review by Via

[vanceingalls]

Verdict: CHANGES_REQUESTED at 79752fdb53 — reversing my earlier APPROVE at the same head. My adversarial pass converged on the seams I traced from the viewport-level actor and missed a preview-channel gap on the source-lane side. Cross-checking with a divergent-lens re-scan surfaced it.

P1 — Source-lane mountedRef freezes preview updates for the rest of the gesture when the row unmounts

Location: TimelineClipDiamonds.tsx:271-274.

onPreview: (clipPercentage) => {
  if (!mountedRef.current) return;
  setPreview(clipPercentage === null ? null : { kfKey, clipPct: clipPercentage });
},

mountedRef is per-lane (TimelineClipDiamonds.tsx:91-98, set false in the effect cleanup). The actor stored in the viewport-level WeakMap coordinator holds THIS closure — captured at drag start. If the source clip's lane unmounts mid-gesture (row scrolled out of the virtualized window), mountedRef.current flips to false, and every subsequent onPreview call returns without updating any state.

Consequence: the actor keeps running (correctly — it's viewport-owned) and the commit still lands (verified commitMove uses usePlayerStore.getState().updateElement, no lane dependency). But from the user's perspective, the diamond preview freezes at the last position drawn before the row unmounted. The visible drag indicator disconnects from the pointer for the rest of the gesture.

Directly contradicts the PR body's stated invariant ("A keyframe can move while its clip or row enters and leaves the virtualized window without losing its identity or leaking preview state into another gesture"). The existing test at TimelineClipDiamonds.test.tsx asserts commit-side correctness on source unmount but doesn't observe the preview channel during the unmount window.

Suggested fixes (any of):

1. Route preview through the viewport-level channel: coordinator holds a previewSubscribers: Set<(state) => void>, lanes subscribe on mount and unsubscribe on unmount. The actor calls coordinator.publishPreview(state) which fans out to whichever lane is currently mounted for that kfKey.

2. Store preview state on the actor itself and have TimelineClipDiamonds read it from the coordinator during render, not via a captured setter closure. This removes the closure-over-mounted-ref problem entirely.

3. If (1) and (2) are too disruptive, at minimum: check mountedRef.current in a way that doesn't drop the update — e.g., cache the last preview value and let the diamond consume it when it remounts.

A regression test that scrolls the source row out mid-drag AND asserts the preview stays coherent (e.g., via a second lane instance that mounts as the first unmounts) would pin the fix.

Non-blocker observations (would not block on their own)

1. Stale pending entries only cleared on next retime, not on epoch change (useTimelineKeyframeHandlers.ts:193-195). The store subscription cancels the active actor when timelineSessionEpoch changes but leaves coordinator.pending entries behind. Bounded by next-retime sweep, but the invariant is asymmetric vs. #2704's more eager epoch handling. Consider clearing coordinator.pending in the store subscription alongside coordinator.latest = null.

2. usePlayerStore.subscribe at line 413 fires unfiltered on every store update. During an active retime the callback runs the elements.some(...) predicate per store dispatch (including per-frame currentTime ticks). Not a correctness bug; consider subscribeWithSelector on [timelineSessionEpoch, sourceElementPresent].

3. Escape keydown listener attaches with default (bubble) phase, no preventDefault / stopPropagation (useTimelineKeyframeHandlers.ts:397-399). The clip-drag lifecycle in #2704 attaches keydown with capture=true AND calls preventDefault/stopPropagation. If a drag + retime were ever simultaneously armed (which the per-coordinator replacement contract should prevent within a single lifecycle, but is not enforced across the two lifecycles), Esc handling would diverge. Minor.

Cross-stack observations (context for Family E, not gating this PR)

• sessionEpoch is threaded three different ways across #2704 (prop), #2705 (reads usePlayerStore.getState().timelineSessionEpoch), #2706 (prop again). Verify the Timeline.tsx caller passes the SAME source of truth into both prop sites so a session change invalidates all three lifecycles simultaneously.
• Three independent autoscroll RAF handles across the stack (clipDragScrollRaf, actor.scrollRaf, autoScrollRafRef). No shared coordinator. A future refactor might unify these.
• data-timeline-scroll-viewport is now referenced implicitly by #2705 (WeakMap key lookup via closest(...)) and #2706 (autoscroll target). Worth a JSDoc on the attribute assignment noting its cross-consumer contract so a future refactor doesn't strip it.

Reflection

This is a "shared blind-spot between converging approvers" pattern per my recently-codified feedback_shared_blind_spot_between_converging_approvers memory. My earlier APPROVE converged on the seams I traced from the viewport-level actor code — I verified the coordinator lifecycle, session-epoch handling, revert-race protection, and identity keying, all correct. But I did not audit the source-lane's callback closures for refs that would go stale after unmount. The class of gap: "callbacks captured by the actor at gesture start close over refs from a component that can unmount mid-gesture." A divergent-lens re-scan surfaced it. I've re-audited #2704 and #2706 for the same class and did not find comparable sites (they're architected top-level, not per-clip). Codifying the audit-callbacks-for-stale-refs technique in memory.

Once the preview channel is fixed, happy to re-review at the new head.

— Review by Via

[james-russo-rames-d-jusso]

The viewport-scoped retime coordinator (WeakMap keyed on [data-timeline-scroll-viewport]) with sessionEpoch + source-presence guards is a solid answer to the diamond-unmounts-mid-drag hazard. Window listeners in capture phase + pointer-capture on the viewport, plus a store subscribe that cancels on session-flip or source removal, cover the terminal paths well. Two correctness observations + two design nits inline. All four surviving because the concrete anchor is unambiguous.

— Review by Rames D Jusso

[vanceingalls]

Verdict: APPROVE at 0c2b929411 — clearing my prior CHANGES_REQUESTED.

P1 preview channel — resolved

mountedRef is gone from TimelineClipDiamonds.tsx (verified — no references remain). Preview now lives on the viewport-level coordinator:

• TimelineKeyframeRetimeCoordinator.preview + previewListeners: Set (state stored on coordinator, not in the actor's callback closure).
• publishRetimePreview(coordinator, preview) at useTimelineKeyframeHandlers.ts:145-151 writes to coordinator.preview AND fans out to every subscriber.
• subscribeTimelineKeyframeRetimePreview(source, listener) at :153-161 — lanes subscribe/unsubscribe via useEffect. Line 159 immediately calls the listener with coordinator.preview on subscribe, so a lane that mounts MID-drag (row scrolls in from off-screen) picks up the current preview state on its first render. That's the specific case the mountedRef closure previously silently dropped.
• Lane consumer at TimelineClipDiamonds.tsx:104-110: subscribeTimelineKeyframeRetimePreview(source, (nextPreview) => setPreview(...)), unsubscribe returned as effect cleanup. Correct lifecycle shape.
• renderPct = preview?.kfKey === kfKey ? preview.clipPct : kf.percentage at :246 — kfKey mismatch means the lane ignores previews for OTHER diamonds. Cross-lane preview channel is isolated by kfKey.

This is exactly option (1) from my earlier finding — viewport-level channel with per-lane subscribe/unsubscribe. Cleanest of the three fix options I suggested.

Other observations also addressed

• Element-scoped pending retirement by destination identity (useTimelineKeyframeHandlers.ts:224-235). Pending retirement predicate now requires input.keyframeKeyOf(keyframe) === pending.destinationKeyframeKey — matched by the destination keyframe's identity, not just percentage-in-tolerance-of-any-keyframe. Directly addresses my earlier P2 about "unrelated sibling at that percentage retiring the entry" without needing to know which pending entry the retire was for.
• Stale pending swept on session-epoch change (:220-222). Loop runs at the top of every beginTimelineKeyframeRetime; pending entries with mismatched sessionEpoch get deleted. Combined with the store subscription's cancel, this closes the earlier asymmetry.

Sanity-check remaining

• Coordinator identity is stable across lane remounts because it's stored in a WeakMap keyed by the scroll viewport (which doesn't unmount when rows scroll).
• No mountedRef re-introduction anywhere in the file (verified via grep).
• New previewListeners set uses reference equality — adding same listener twice is a no-op (Set semantics). Unsubscribe closure captures the listener + coordinator refs, safe post-unmount.
• Immediate-publish-on-subscribe (:159) means the visual preview snaps to the current gesture state even if the mount happens 200ms into a drag — the previously frozen-at-last-drawn-position visual is now correct.

CI: verifying on this head — all three PRs claim 3,278 tests + typecheck + lint + Fallow green at the rebased tip.

Nice fix — the coordinator-owned preview + immediate-publish-on-subscribe pattern is cleaner than the original per-lane closure, and it addresses the entire class of "callback captures a ref from a component that can unmount mid-gesture" I flagged in my memory addendum.

— Review by Via

[james-russo-rames-d-jusso]

Reviewed at 0c2b92941.

Substantive R2 — Vance's option 1 (viewport-owned pub/sub preview channel) is adopted cleanly, and the F1/F2/F3 refactors are all correctly landed. But one blocker on the test scaffolding, plus a re-render-cascade concern that shows up under the HF-flicker lens.

Blocker

• Missing module ./timelineTestViewport — TimelineClipDiamonds.test.tsx:14 and :990 reference configureTimelineTestViewport from ./timelineTestViewport, but the module does not exist in the tree at 0c2b92941 (verified via git ls-tree; also not on origin/main). Vitest can't resolve the import, so the entire TimelineClipDiamonds.test.tsx file fails to load — which takes down every R2 regression test added alongside the refactor: preserves another element's pending retime when the active source is removed (F2), retires a pending retime once the cache exposes its destination identity (F3), keeps the retime preview coherent when the source lane remounts (V1), plus the new auto-scrolls horizontally without virtualizing the source row away (F1). Either the file was forgotten in the commit, or the symbol lives elsewhere and the import path is wrong. Landing this as-is means the regression coverage silently doesn't run — please push the missing file or fix the import.

Fixes verified

• F1 (horizontal-only autoscroll) — FIXED. useTimelineKeyframeHandlers.ts:17-20 imports the horizontal-only helper; :286 calls applyTimelineHorizontalAutoScrollStep(viewport, actor.lastClientX) with no clientY; timelineEditing.ts:501-514 zeroes delta.y. Y-edge cannot virtualize the source row away.
• F2 (element-scoped pending cleanup) — FIXED. useTimelineKeyframeHandlers.ts:451-458 filters the pending-map deletion by pending.elementId === actor.elementId. Coordinator remains viewport-wide (WeakMap<EventTarget, ...>), but the deletion is scoped correctly.
• F3 (destination-identity retirement) — FIXED. PendingTimelineKeyframeRetime gains a destinationKeyframeKey: string at :60; commitMove precomputes it at :351-355 via timelineKeyframeSelectionKey with the destination percentage; the drag-start sweep at :230 compares against pending.destinationKeyframeKey instead of the stale drag-start source key.
• Vance P1 (lane-remount preview coherence) — FIXED. TimelineClipDiamonds.tsx:91-111 drops mountedRef and swaps the per-lane setter closure for subscribeTimelineKeyframeRetimePreview(source, ...). useTimelineKeyframeHandlers.ts:144-160 defines viewport-owned publishRetimePreview fanning out to coordinator.previewListeners: Set<(preview) => void>, and coordinator.preview caches last-published state so a lane subscribing mid-drag receives current preview.
• Vance N1 (epoch cleanup broadens to full clear) — FIXED. :447-450 clears coordinator.pending + resets coordinator.latest on epoch change (was only cancelling the actor in R1).

Follow-ups Vance labeled non-blocker (still open, non-blocking)

• Vance N3 (subscribeWithSelector on store subscribe) — Not adopted. :445 still runs state.elements.some(...) on every store dispatch (including per-frame currentTime ticks during retime). Vance's own gating language said "would not block on its own"; leaving it open. Follow-up.
• Vance N4 (Esc key capture-phase parity with #2704) — Not adopted. :428 still window.addEventListener("keydown", onKeyDown) at bubble phase without preventDefault/stopPropagation. Vance's own gating language same as N3; follow-up.
• My F4 (shared TimelineGestureLifecycle extraction) — Not adopted; the retime coordinator stays a bespoke state machine. Was tagged as a family-level refactor suggestion, not a hard blocker. Follow-up.

Concern + nit (inline)

Two inline items on the pub/sub broadcast and the sweep-invariant.

Once the missing test module lands, Vance's change-request naturally clears (P1 + N1 are the load-bearing items; the rest are his own non-blockers).

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🔴 BLOCKER — module doesn't exist. configureTimelineTestViewport is imported from ./timelineTestViewport, but there is no timelineTestViewport.{ts,tsx} file anywhere in the tree at 0c2b92941 (verified with git ls-tree -r --name-only, and it's not on origin/main either). The symbol is only referenced by this file (:14 import, :990 call site). Vitest will fail to resolve the module on load, which takes down the entire test file — including the F1/F2/F3/V1 regression tests added in R2. Please either commit the missing timelineTestViewport.ts or fix the import path. Blocking. — Rames D Jusso

[james-russo-rames-d-jusso]

🟡 (flicker lens — non-blocking) Every mounted TimelineDiamondLane subscribes to subscribeTimelineKeyframeRetimePreview and calls setPreview({ kfKey: nextPreview.keyframeKey, clipPct: nextPreview.clipPercentage }) — a fresh object literal on every publish. React 18 useState bails only on Object.is, so each publish re-renders every non-source lane too (N lanes × publish rate during retime). Correctness holds — renderPct = preview?.kfKey === kfKey ? preview.clipPct : kf.percentage filters at render — but the pre-R2 shape re-rendered only the source lane. Consider filtering at the listener boundary (skip when nextPreview?.keyframeKey isn't in this lane's keyframesData), or dedup by memoizing the object when semantically unchanged. Non-blocking; documenting under the HF re-render-cascade / flicker lens. — Rames D Jusso

[james-russo-rames-d-jusso]

🟢 (nit — invariant tightening) The source-removal branch iterates for (const [key, pending] of coordinator.pending) and deletes matches by pending.elementId === actor.elementId, but does not check pending.sessionEpoch === actor.sessionEpoch. A pending entry from a prior session that shares the current actor's elementId would be deleted too. Harmless (stale entries deserve removal), and the epoch-change branch above at :447-450 already clears the whole map on any epoch bump — so cross-session pollution is bounded in practice. Still, worth either a ponytail: comment noting the invariant or an explicit epoch gate here for symmetry. — Rames D Jusso

[james-russo-rames-d-jusso]

Re-reviewed at e12dba81.

Blocker closed — timelineTestViewport.ts now committed (+13 lines, single-file surgical push landed within a second of the prior review). Full delta 0c2b929..e12dba81 is only that one file. Regression tests for F1/F2/F3/V1 (the four R2 additions in TimelineClipDiamonds.test.tsx) can now load.

R2 non-blocker follow-ups from the prior pass remain open (all Vance's-own-labeled-non-blocker + my family-refactor suggestion, plus the two inline nits on the pub/sub cascade and the epoch gate). None of those block; leaving as follow-up.

Clean pass from where I sit.

— Review by Rames D Jusso

The base branch was changed.

[vanceingalls]

Verdict: APPROVE at c2f827427 — byte-clean re-stamp of my R3 approve at 0c2b929411.

Confirmed via GitHub contents API: TimelineClipDiamonds.tsx and useTimelineKeyframeHandlers.ts both have identical blob SHAs at the old and new heads:

• TimelineClipDiamonds.tsx: 4117e7ac4ec41799a9d786c9e8eea7b6919a37b3 (both heads)
• useTimelineKeyframeHandlers.ts: 7632bad2216add374c6c263a7802b747d34f8ecb (both heads)

Rebase moved the shared test helper one commit earlier to satisfy the exact-PR CI shape, but the final tree state is byte-identical. All findings verified at R3 (viewport-owned preview channel + subscribe-with-immediate-publish + destination-identity pending retirement) hold at this head.

Base is now main (post-#2704-merge). CI in progress at time of review.

— Review by Via

[james-russo-rames-d-jusso]

Re-reviewed at c2f827427 (rebased onto main post-#2704-merge; timelineTestViewport.ts moved from #2706 into #2705 as noted).

Byte-identical to my R3 head at e12dba81:

• git diff --stat e12dba81 c2f827427 — empty (identical trees).
• Cumulative patch-ID git diff origin/main..c2f827427 | git patch-id --stable matches git diff origin/main..e12dba81 | git patch-id --stable at b47ea7c32d59cd41b2e8c951c1c0264e223f95a6.

No content delta — the rebase carried the fix into #2705 verbatim. All prior R3 conclusions stand: F1/F2/F3 fixed, Vance's P1 (viewport pub/sub) + N1 (epoch full-clear) fixed (Vance's CR already dismissed). Follow-up non-blockers remain (Vance's N3 / N4 + my family-refactor suggestion + the two inline nits on the pub/sub cascade and epoch gate on the source-removal sweep).

Ready from where I sit.

— Review by Rames D Jusso