fix(studio): harden keyframe editing semantics

[miguel-heygen]

What

Hardens keyframe selection, authoring time, deletion, clip movement, and easing interactions.

Why

The reviewed timeline surfaced correctness bugs around stale selection, clip-relative versus global time, keyboard/button parity, pointer capture, bulk deletion, failed retimes, and moving clips with keyframes.

How

Preserves explicit identity and captured playhead time through the authoring transaction, makes selection gestures deterministic, keeps accessible ease controls mounted, and adds regression coverage for the critical paths. The cohesive 1,722-line delta is documented as an R9 correctness exception so fixes remain with their proof. This is B7 of the Family B draft Graphite stack.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (not applicable)

Exact Family B tip validation: 2,910 Studio tests, 398 Studio Server tests, both package typechecks, formatting, lint, diff check, file-size gate, and Fallow audit passed.

Deferred review findings

Every blocker and high finding raised on this PR is fixed in the stack. The 3 remaining low/nit findings are parked, verbatim, in .scratch/studio-timeline-family-b/issues/07-pr-2689-deferred-review-findings.md:

• 🟡 packages/studio/src/hooks/gsapDragPositionCommit.ts:278 — activeKeyframePct not cleared when arc drag falls through to temporal-keyframe branch
• 🟢 packages/studio/src/components/TimelineToolbar.test.tsx:61 — TimelineToolbar keyboard/button parity: only the 'endpoint disabled' path asserted; 4 new motion-path label branches untested
• 🟢 packages/studio-server/src/routes/files.ts:1056 — Server-side resolveReplacementEaseEach re-parses the entire script on every request that omits easeEach

---

Supersedes #2689, which was closed when main was rewritten to unwind an early landing of this stack. Same head commit, same review history.

R1 review follow-ups

Fixed in this PR:

• isPlayheadWithinTween takes the selection, so the toolbar stops promising "extends animation" on a duration-less tween whose click actually toggles an interior keyframe.
• The arc drag commit resolves its replacement duration the same way its click-path sibling does, instead of authoring duration: 0.5 and collapsing the arc window.
• The flat text section drops its two marker-clearing effects. The autofocus marker is a ref now, read and cleared by the render that consumes it.
• timelineKeyframeSelectionKey documents the percentage / tweenPercentage asymmetry its callers have to respect.

Checked, no action needed: the #qa-keyframe-box to #qa-zone-keyframe rename applies to the clip wrapper only. The inner box keeps its id and no spec outside the fixture references either.

R2 review follow-ups

Fixed in this PR:

• A diamond had two identities. keyframeTarget dropped the property group and animation id for collapsed clip rows, so the same keyframe hashed to a different selection key collapsed than expanded (selected in one view, unselected in the other) and the retime and delete mutations lost the id they use to pick between two animations colliding at one percentage. The target now always carries the full identity the cache already holds, and the callbacks take that target instead of flattened positional fields. Covered by a regression test asserting one shared key across both views.
• Timeline.tsx carried its own copy of the three keyframe handlers alongside the existing useTimelineKeyframeHandlers hook. The inline copy is deleted and the hook is wired in, so there is one implementation.
• Diamond geometry (centre, hit width, visual size) is computed once per diamond into one record, which removes the index lookups and the non-null assertions the connector pass used to need.

[miguel-heygen]

• feat(studio): wire expanded keyframe timeline lanes #2791
• fix(studio): harden keyframe editing semantics #2787 👈 (View in Graphite)
• feat(studio): consolidate timeline editor callbacks #2786
• feat(studio): add keyframe track headers #2785
• feat(studio): add timeline property lanes #2784
• feat(studio): add timeline keyframe retiming interactions #2783
• feat(studio): add variable timeline timing and layout #2782
• feat(studio): add keyframe timeline state #2781
• main

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

[vanceingalls]

Adversarial R1 pass at 92c23fda3e2a3fd5a2d639d5918d65d4d93bd8ad. The hardening thesis (identity through the transaction, deterministic gestures, duration-less fallback, always-mounted ease control) is largely well-executed and the test surface actively violates the invariants (path endpoint block, colon-id round-trip, empty-writer settlement returning false). The stack-tip claim of green suites reads as accurate. Two callers of the new "clip-duration fallback" primitive were missed, and one small block adopts a useEffect derived-state pattern the house style forbids. All P2 — nothing here blocks merge on its own.

Verdict: COMMENT (approve with follow-ups tracked; recommend fixing the two duration-fallback drifts before this cascade lands on top of #2791).

Adversarial lenses

Lens A — Hardened edges vs displaced edges

The new resolveEditableTweenDuration(anim, sel) correctly makes applyKeyframeAtPlayhead treat a duration-less keyframe tween as spanning the owning clip. Two sibling call paths were not migrated, so the edge shifted rather than closed:

• packages/studio/src/components/TimelineToolbar.tsx — resolveKeyframeToggleState still calls isPlayheadWithinTween(animation, currentTime) (line inside the new helper), which delegates to resolveTweenDuration(anim) with the 0.5 s default. For a duration-less non-arc keyframe tween on, e.g., a 16 s clip at playhead=5 s, the toolbar surfaces willExtend=true and the tooltip "Add keyframe at playhead, extends animation (K)", but clicking dispatches through applyKeyframeAtPlayhead, which now sees the playhead inside the tween (0..16.26) and toggles an interior keyframe instead of extending. Silent UX/action mismatch. P2.
• packages/studio/src/hooks/gsapDragPositionCommit.ts (arc branch, right after arcPath?.enabled guard) — the new temporal-arc drop uses const tweenDuration = resolveTweenDuration(anim); the sibling applyArcKeyframeAtPlayhead in this same PR was migrated to resolveEditableTweenDuration. A duration-less arc dragged instead of clicked would authoring a replace-with-keyframes mutation with duration: 0.5, silently collapsing the arc window. Symmetric drift with the toolbar case. P2.

Recommend both call sites take a DomEditSelection parameter and route through resolveEditableTweenDuration — otherwise the "clip is the fallback" invariant only holds on the toggle path.

Lens B — New failure modes

The changed propagation via onResult is well-scoped: the store never sees the result until after the writer resolves, and both moveKeyframe and resizeKeyframedTween swallow the rejection into trackGsapSaveFailure (verified in useGsapKeyframeOps.test.tsx:113-155). No new bubbled throw. onDeleteAllKeyframes now awaits buildDomSelectionForTimelineElement before firing; if that resolves to null (element unmounted mid-context-menu), the delete is a silent no-op — the same behavior the caller had before, so no regression.

Lens C — Invariant enforcement

• "Motion path endpoints cannot be removed" is enforced at the button (disabled + relabeled aria) and at applyArcKeyframeAtPlayhead (interior-only via timedNodeIndex > 0 && timedNodeIndex < nodes.length - 1). The toolbar test at TimelineToolbar.test.tsx:60-100 violates the invariant and confirms disable. Enforced at both entry and internal call site — good.
• timelineKeyframeSelectionKey round-trip. JSON envelope + colon-id fallback is validated at timelineKeyframeIdentity.test.ts:17-33, including the colon-in-id case that the previous format silently mis-parsed. Encoding is asymmetric with tweenPercentage: the writer defaults it to percentage when absent (line just after the JSON stringify), but the reader treats it as an equally-authoritative dimension. If a call site passes only percentage, the two keys are interchangeable; if any caller mutates tweenPercentage without percentage, two logically-equal selections may hash to different keys. Not a bug today, but worth a code-adjacent comment.

Lens D — Backwards compat / anti-patterns

• packages/studio/src/components/editor/propertyPanelFlatTextSection.tsx:260-279 introduces two useEffect calls whose body only calls setAutoFocusFieldKey(null). The second — if (autoFocusFieldKey && autoFocusFieldKey === activeFieldKey) setAutoFocusFieldKey(null) — is a textbook derived-state effect (clear the "just added" marker once the added field becomes active). Under the repo's Golden Rule "No useEffect for state syncing" this belongs either in the same .then((nextKey) => …) callback that armed it, or as derived-during-render (const autoFocus = pendingAutoFocusKey === activeField.key). P2.

Lens E — Stack interaction (#2786, #2791)

Signatures of moveKeyframe and resizeKeyframedTween moved from void to Promise<boolean>; useDomEditWiring.ts typings updated in-PR. If #2791 (expanded lanes) authors any call to these that expects the void shape, it will type-fail on rebase — but the stack is serial (#2786 → #2787 → #2791), so this is a scheduling constraint, not a live risk. onDeleteAllKeyframes / onMoveKeyframeToPlayhead also swapped their elementId: string arg for a full TimelineElement; I enumerated the three constructors (Timeline.tsx, useTimelineKeyframeHandlers.ts, MotionPathOverlay.tsx) — all three pass the element explicitly, so blast radius is bounded to this PR.

Peer-lens checks

• Facade blast-radius for KeyframeDiamondContextMenuState.element — three constructors, all migrated in-PR ✓.
• Extract-to-helper resolveKeyframeToggleState — single caller in the same file ✓, but the extracted helper introduced the layer-N/N+1 gap flagged in Lens A.
• Deleted packages/studio/src/utils/keyframeSelection.ts — sole consumer deleteSelectedKeyframes.ts swapped to timelineKeyframeTargetFromSelectionKey in this PR; helper + test both go together ✓.
• Fixture rename #qa-keyframe-box → #qa-zone-keyframe in packages/studio/tests/e2e/fixtures/design-panel-qa/index.html — cross-check any e2e spec still targeting the old id; not in this file list, but worth a rg qa-keyframe-box before merge.

— Via

[james-russo-rames-d-jusso]

Reviewed at 92c23fda3e2a.

R2 adversarial pass on the correctness bundle. R9 verdict: split_path_exists — the keyframe-core clusters (identity+delete rewrite, duration-less basis, motion-path temporal keyframes, callback element-identity/settlement) form a coherent thread, but ~6 unrelated cleanups ride along (TimelineClipDiamonds hit-region math, ease-button hover-visibility rewrite, propertyPanelFlatTextSection multi-field auto-focus, useStudioTestHooks typing, keyframeRetime.test refactor, e2e fixture retarget). One blocker: isPlayheadWithinTween (useEnableKeyframes.ts:84) still uses resolveTweenDuration(anim) with the 0.5s fallback while the PR standardizes the rest of the surface on resolveEditableTweenDuration (clip-duration fallback). This is called by BOTH TimelineToolbar toggle-state AND applyArcKeyframeAtPlayhead, so toolbar and click-handler now disagree on whether the playhead sits inside a duration-less tween. Same class of bug the PR claims to have closed. Convergent with Via's flag on the same displaced-edge pattern; also caught buildTemporalArcKeyframes append-without-dedup and motion-path 0.05% vs 1% asymmetry.

R9 exception verdict — split_path_exists

Keyframe-core clusters (identity+delete rewrite; duration-less basis; motion-path temporal keyframes; callback element-identity/settlement contract) form one coherent 'harden keyframe editing semantics' thread. But the PR bundles at least five independent clusters that touch no shared authority:

• (a) TimelineClipDiamonds hit-region math (markerMetrics, non-overlapping widths) + test — pure UI math
• (b) Ease-button hover-visibility rewrite (setState → opacity-0 group-hover) + TimelinePropertyLanes.test rewrite
• (c) propertyPanelFlatTextSection multi-field auto-focus fix + tests
• (d) useStudioTestHooks typing-only hardening
• (e) keyframeRetime.test refactor (dedupe expectLeftResize helper)
• (f) e2e fixture retarget from #qa-keyframe-box to #qa-zone-keyframe with no in-diff rationale

None of (a)–(f) shares mutable authority with keyframe-core files; each could land as separate small PR. This is the largest PR in the stack (+1320/-382) precisely because unrelated cleanups were folded into an R9 bundle.

Bug-test coverage audit

• ✅ Cross-element stale selection percentages must not be applied to active element on bulk delete: timelineEditingHelpers.test.ts:346-393 covers per-animation dispatch and coalesceKey; specific 'other element's keyframes dropped' case is implicit through element-id gate
• ✅ Motion-path drag on implicitly selected point adds temporal keyframe not spatial waypoint: gsapRuntimeBridge.test.ts:311-360 covers activeKeyframePct=null (temporal) and =50 (spatial). Near-miss case (0.5% off authored waypoint) not covered — see 0.05 tolerance finding
• ✅ 'Add keyframe at playhead' on arc deletes existing interior stop instead of redistributing path: useEnableKeyframes.test.ts:238-262 asserts interior removal preserves endpoint timing; endpoints protected (line 266-269)
• ✅ Duration-less tween editors fall back to OWNING clip duration not 0.5s: gsapShared.test.ts:11-23 and globalTimeCompiler.test.ts:115-118 for the unit; useTimelineEditCallbacks.test.tsx:369-393 for retime flow. But isPlayheadWithinTween (useEnableKeyframes.ts:84) — SAME class of decision — was NOT converted; no test covers duration-less tween through toolbar toggle path
• ✅ Delete-all / move-to-playhead on diamond in non-selected element commits against clicked element: useTimelineEditCallbacks.test.tsx:268-304 asserts handleGsapRemoveAllKeyframes / handleGsapMoveKeyframeToPlayhead receive CLICKED element's selection
• ✅ Move-keyframe / resize-tween outcomes settle to Promise so callers react to no-op/failure: useGsapKeyframeOps.test.tsx:126-166 covers no writer result, changed:false, and rejected commits
• ✅ replace-with-keyframes on arc preserves per-segment easing: files.test.ts:1833-1872 asserts easeEach: 'power1.inOut' after replacement. See outer-ease-vs-easeEach semantics drift note
• ⚠️ Scrubbing on timeline must not swallow clicks on interactive controls: No test exercises Timeline.tsx:439's closest('button, input, select, a') bailout
• ✅ Multi-field text panels don't steal canvas focus when element is selected: propertyPanelFlatTextSection.test.tsx:406-422 asserts activeElement stays on focus owner
• ✅ Adjacent keyframe diamonds don't overlap hit regions or visual squares in dense timelines: TimelineClipDiamonds.test.tsx:48-79 asserts three keyframes at 30/60/90% across 36px produce non-overlapping hit widths ~10.8px and visual widths ~8.8px

🟢 Verified clean

• keyframeRetime.test.ts refactor (dedup expectLeftResize helper) is behaviour-preserving
• TimelinePropertyLanes.test.tsx switch from 'reveal on hover' to 'always-rendered opacity-0 group-hover:opacity-100' matches CSS in TimelineClipDiamonds.tsx
• KeyframeDiamondContextMenu now requires state.element: TimelineElement; every producer in the diff provides it — no stranded call site
• Deletion of packages/studio/src/utils/keyframeSelection.ts: only importer (deleteSelectedKeyframes.ts) is rewritten and no other in-repo references remain
• useDomEditWiring.ts return-type widening to Promise for moveKeyframe / resizeKeyframedTween matches useGsapKeyframeOps.ts's rewrite and useGsapSelectionHandlers.ts's forwarding — signatures reconcile
• onMoveKeyframe: TimelineEditCallbacks returns Promise; useTimelineEditCallbacks.ts:254 always returns a Promise; TimelineClipDiamonds.tsx:518 handles via .then((committed) => ...)

Complements Via's parallel adversarial pass (Family B rollup). Where Via found 0 P1 / 17 P2 / 18 P3, the adversarial subagent pass here surfaced additional depth on the mutation-authority thread and the Promise chain.

— Review by Rames D Jusso

[vanceingalls]

Reviewed at 8c0d9a07ddbec632e191e55baf5a0e99b9aded6f (code-review max, R2 delta over my R1 at 92c23fda3e2a3fd5a2d639d5918d65d4d93bd8ad, +1570/-593 across 43 files).

Verdict

APPROVE. The three new commits since R1 (14c10d9c clip-spanning fallback, b8406e6d unified diamond identity, 8c0d9a07 diamond types split) close every P2 I raised in R1 and Rames R1's single blocker. The residual findings are either explicitly deferred in the PR body's .scratch/…/07-pr-2689-deferred-review-findings.md block, addressed at the stack tip 6ee750fee, or 🟢 nits.

R1 findings — status at head

Verified per-finding at 8c0d9a07:

• 🟢 R1 Lens A #1 (TimelineToolbar.tsx — isPlayheadWithinTween still on 0.5s fallback) — FIXED in 14c10d9c. resolveKeyframeToggleState now passes session.domEditSelection through, and isPlayheadWithinTween(anim, t, sel?) resolves duration via resolveEditableTweenDuration when a selection is present. useEnableKeyframes.test.ts:108-121 locks the invariant with an explicit "answers false without the selection, true with it, false past clip end" case.
• 🟢 R1 Lens A #2 (gsapDragPositionCommit.ts arc branch — resolveTweenDuration(anim) 0.5s default) — FIXED in 14c10d9c. Line 302: const tweenDuration = resolveEditableTweenDuration(anim, selection); with the invariant recited in the code comment.
• 🟢 R1 Lens C (timelineKeyframeSelectionKey — percentage/tweenPercentage asymmetry) — DOCUMENTED in 14c10d9c. timelineKeyframeIdentity.ts:8-14 now carries the JSDoc naming the "callers must keep the pair consistent" contract verbatim.
• 🟢 R1 Lens D (propertyPanelFlatTextSection.tsx — two useEffects doing derived-state clearing) — FIXED in 14c10d9c. Both effects deleted; autoFocusFieldKeyRef is a ref, read and cleared during the render that consumes it.

Rames R1 findings — status

Cross-verified at head and at 6ee750fee where Miguel claimed deferral to #2791:

• 🔴 Rames #1 (useEnableKeyframes.ts:109 — isPlayheadWithinTween 0.5s fallback) — FIXED at head in 14c10d9c (convergent with my Lens A #1). Miguel's "addressed at #2791" is technically correct but understates it: closure lives in this PR.
• 🟠 Rames #4 (useEnableKeyframes.ts:303 — applyKeyframeAtPlayhead uses clip-relative pct when start === null) — INVARIANT-HOLDS under audit. The start === null branch only runs when the tween has no resolvable anchor; resolveEditableTweenDuration(kfAnim, sel) then returns clip-duration, so tween-effective-range collapses to the clip. Under that construction clip-% and tween-% are the same axis, and comparison against k.percentage (<= 1) is well-typed. If any writer later authors tweenPercentage on a null-start tween without normalising to clip origin, this breaks — worth an assertion long-term but not live-bug today.
• 🟠 Rames #5 (gsapDragPositionCommit.ts:282 — 0.05% vs 1% tolerance mismatch) — ADDRESSED at 6ee750fee. Verified: KEYFRAME_PCT_MATCH constant is imported in packages/studio/src/hooks/gsapDragPositionCommit.ts at the tip and both call-sites collapse to it.
• 🟡 Rames #6 (buildTemporalArcKeyframes no dedup) — ADDRESSED at 6ee750fee. Verified: buildTemporalArcKeyframes now .filter((keyframe) => Math.abs(keyframe.percentage - percentage) > KEYFRAME_PCT_MATCH) before appending, so dedup moves to source.
• 🟠 Rames #2 (TimelineToolbar.test.tsx:61 — motion-path 4-branch tooltip coverage) — EXPLICITLY DEFERRED in the PR body's deferred-findings block (line-for-line verbatim). Same coverage gap at 6ee750fee — Miguel's claim overstates that one.
• 🟡 Rames #3 (Timeline.tsx:451 — interactive-controls scrubbing bailout has no test) — STILL UNCOVERED at head and at 6ee750fee. Timeline.test.ts exists but does not exercise pointerdown on a button/input inside the timeline does not scrub. Miguel's claim that this is addressed at the stack tip does not hold up under grep. Non-blocking (behaviour is verified manually per the PR body's manual-testing checkbox) but worth a dedicated regression at some point.
• 🟢 Rames #7 (timelineKeyframeIdentity.test.ts — collapsed-fallback + colon-in-element-id case) — PARTIALLY-COVERED. retains the collapsed key fallback test covers comp#a:30 but not the specific a:b:40 shape (element id itself carries a colon). Non-blocker; the JSON envelope covers colon-bearing ids via the expanded-lane test.
• 🟢 Rames #8, #9, #10, #11 — nits at head and at 6ee750fee. #10 in particular (server-side resolveReplacementEaseEach falls back from easeEach to outer ease) is unchanged at the tip — Miguel's blanket "addressed" claim overreaches here; the PR body's own deferred block acknowledges only the perf concern on this function, not the semantic drift. That semantic call — falling back to outer ease when easeEach is absent — is arguably intentional (retain author-set ease under conversion) but deserves an explicit contract note before it turns into a mystery.

Hardening scope audit

The PR's thesis is "harden keyframe editing semantics." At head the bug classes it names are actually eliminated, not just narrowed:

• Duration-less tween → 0.5s default — eliminated at both edit paths (toolbar toggle, arc drag) and at the toolbar's willExtend answer. The three sites now share resolveEditableTweenDuration(anim, sel) as the single source of truth. useEnableKeyframes.test.ts and gsapShared.test.ts (+78 lines) actively violate the invariant to prove disable.
• Diamond identity split (collapsed vs expanded lanes) — eliminated. keyframeTarget(kf) unconditionally carries {percentage, tweenPercentage, propertyGroup, animationId}; the collapsed row's key must equal the expanded row's key. TimelineClipDiamonds.test.tsx:96-131 asserts that a grouped keyframe hashes identically across views and highlights from the shared key alone (playhead off-clip, no other match).
• Retime settlement (Promise<boolean> result) — propagated through useDomEditWiring → useGsapKeyframeOps → useTimelineEditCallbacks.onMoveKeyframe. useGsapKeyframeOps.test.tsx:113-155 locks the empty-writer-settlement false return.
• Keyboard/button parity for keyframe toggle — motion-path endpoint block asserted; the four label branches (Add motion path waypoint / Remove motion path waypoint / Extend motion path to playhead / Motion path endpoint) remain deferred as one 🟢.

Per feedback_adversarial_audit_fix_internal_boundaries, I looked for asymmetric shapes of each failure mode. The identity split's asymmetric shape (writer defaults tweenPercentage to percentage, reader treats both as authoritative) is now called out in JSDoc and enforced by the round-trip test — the class is closed, not just narrowed.

Fix-internal remaining-silent-X audit

Six internal boundary checks:

• State discard: handleGsapMoveKeyframeToPlayhead still fires void moveKeyframe(...); the Promise settlement never reaches the UI. Present pre-PR; 🟢 not new here.
• Return invariant: onDeleteAllKeyframes builds the selection asynchronously and no-ops silently if the DOM node is gone. Same as pre-PR shape; documented in the deferred block.
• Library defaults: resolveEditableTweenDuration uses 0.5 only when both the tween's duration AND the element's data-duration attribute are missing. Reasonable floor.
• Precedence: resolveReplacementEaseEach on the server side prefers request → keyframes.easeEach → outer ease. The "outer → easeEach on conversion" is a semantic call — worth a contract comment when a reviewer next touches this file.
• Session ownership: onDeleteKeyframe correctly resolves through the clicked element's own selection, not domEditSelection, so cross-file deletes commit against the right element. Comment at useTimelineEditCallbacks.ts:224-227 locks the reasoning.
• Discovery completeness: the three constructors of KeyframeDiamondContextMenuState.element (Timeline.tsx, useTimelineKeyframeHandlers.ts, MotionPathOverlay.tsx) all migrated to the full TimelineElement. No stale caller.

Standards lens re-run

Mechanical grep across the three new commits (14c10d9c, b8406e6d, 8c0d9a07) on added lines only:

• Bare as T casts: 1 (useEnableKeyframes.test.ts:120 — as unknown as DomEditSelection in a test-file mock; double-cast through unknown preserves narrowing discipline. Acceptable per HF test-shim convention.)
• Non-null !.: 0 on new lines. b8406e6d actively removed markerMetrics[i]! non-nulls by restructuring to per-marker records.
• .message without instanceof Error: 0
• Angle-bracket casts <T>: 0

Independent findings

No new independent findings on the three post-R1 commits. b8406e6d's diamond-identity unification and 8c0d9a07's types split are both clean refactors preserving public shape (TimelineDiamondKeyframe is re-exported from the split file, so no downstream import churn). The useTimelineKeyframeHandlers hook wire-in at Timeline.tsx:266-276 deduplicates the three inline handlers exactly as R2's follow-up section promised.

Peer state

• Rames R1 at 92c23fda (11 findings, split_path_exists R9 verdict). Rames has not re-reviewed since the three new commits landed — the R1 blocker (#1) is nevertheless closed by them.
• Miguel dropped 11 identical "Addressed at the stack tip in 6ee750fee (PR #2791)" replies to each of Rames's findings. Under per-finding verification the claim holds for #5 and #6, and is redundant-with-this-PR for #1 (closed here); it overstates the state for #2, #3, and #10 (the semantic-drift arm), all of which remain 🟡/🟢 non-blockers.
• No stale reviewer state from #2786 leaks — that stack's own R3 chain converged separately.

— Review by Via

[james-russo-rames-d-jusso]

Amended 2026-07-27 22:12 UTC — HEAD moved to f5d3c7a3d mid-post; a new commit 9fc001170 fix(studio): give every keyframe callback one mutation frame landed and closes two of my carry-over concerns (onChangeKeyframeEase + onToggleKeyframeAtPlayhead — both now resolveElementAnimations(elId) + buildDomSelectionForTimelineElement(el) + selection threaded to downstream, and handleGsapUpdateMeta at useGsapSelectionHandlers.ts:170 gained a selectionOverride param). The blocker below and the other findings still stand.

Original review (unchanged below): Reviewed at 8c0d9a07d.

Family B tip (B7). Comparing against #2786 base codex/studio-timeline-b-editor-callbacks-v2, so this delta is the 4-commit hardening slice (~1,570/-593 across 43 files). CI green (Preflight, regression, preview-regression, player-perf all SUCCESS at head; Graphite mergeability still in progress).

TL;DR — updated status

• ✅ Two big correctness wins from #2786 R2/R3 still hold: (a) diamond-identity refactor is real, and (b) three of the four #2786-R3 mutation-authority findings landed cleanly. My R3 blocker on onMoveKeyframeToPlayhead:222 closed.
• ✅ onChangeKeyframeEase + onToggleKeyframeAtPlayhead now on the same "clicked-element-own-frame" pattern as the other handlers (from the new 9fc001170 commit). The for anim of selectedGsapAnimations reads are gone. Well done.
• 🔴 Blocker still stands on TimelineOverlays.tsx:109-115 context-menu adapters (unchanged in the restack — same identity-drop shape).
• 🟠 Remaining concerns unchanged.

🔴 Blocker — unchanged at f5d3c7a3d

TimelineOverlays.tsx:109-115 context-menu adapters flatten the identity KeyframeDiamondContextMenu now supplies — R2 identity refactor is defeated on this code path.

KeyframeDiamondContextMenu at head calls its props with the FULL identity:

// KeyframeDiamondContextMenu.tsx:88-94
onDelete(state.elementId, state.percentage, state.propertyGroup, state.tweenPercentage, state.animationId);
// :74-80
onMoveToPlayhead(state.element, state.percentage, state.propertyGroup, state.tweenPercentage, state.animationId);

But the adapters in TimelineOverlays.tsx propagate only the first two args:

// TimelineOverlays.tsx:109-115  (unchanged in the f5d3c7a3d restack)
onDelete={(elId, pct) => onDeleteKeyframe?.(elId, { percentage: pct })}
onChangeEase={(elId, pct, ease) => onChangeKeyframeEase?.(elId, pct, ease)}
onMoveToPlayhead={
  onMoveKeyframeToPlayhead
    ? (elId, pct) => onMoveKeyframeToPlayhead(elId, { percentage: pct })
    : undefined
}

The adapters silently drop propertyGroup, tweenPercentage, animationId. resolveKeyframeTarget.carriesIdentity at useTimelineEditCallbacks.ts:157-160 then goes false, falls back to keyframes.find((item) => Math.abs(item.percentage - pct) < 0.2), which picks the FIRST match. When two anims collide at one percentage on the same property group — the exact case the PR body's own R2 motivation calls out ("the retime and delete mutations lost the id they use to pick between two animations colliding at one percentage") — the context-menu delete/retime/change-ease path picks the wrong one.

The row → menu segment of the identity flow is fixed; the menu → mutator segment still drops it. Fix shape: change the three adapter arrows in TimelineOverlays.tsx:109-115 to pass through the full TimelineKeyframeTarget (menu can construct it inline and pass the whole object, or the adapters can just forward all 5 args). Sibling onDeleteAll={(elId) => onDeleteAllKeyframes?.(elId)} at line 110 is also stale-named — elId is now a TimelineElement (the callback contract changed in this PR) — behaviour is right by luck, but naming is misleading.

✅ Closed by 9fc001170 restack

Both handlers I flagged in the original review as carry-overs from #2786-R3 are now on the same clicked-element-own-frame pattern as the other mutation handlers:

useTimelineEditCallbacks.ts:304 — onChangeKeyframeEase (now at f5d3c7a3d):

onChangeKeyframeEase: (elId: string, _pct: number, ease: string) => {
  // The edited element's own animations + selection, not the selection's:
  const animations = resolveElementAnimations(elId);
  const element = usePlayerStore.getState().elements.find((el) => (el.key ?? el.id) === elId);
  if (!element) return;
  void buildDomSelectionForTimelineElement(element).then((selection) => {
    if (!selection) return;
    for (const anim of animations) {
      if (anim.keyframes) handleGsapUpdateMeta(anim.id, { ease }, selection);
    }
  });
},

useTimelineEditCallbacks.ts:319 — onToggleKeyframeAtPlayhead:

onToggleKeyframeAtPlayhead: (el: TimelineElement) => {
  ...
  const animations = resolveElementAnimations(el.key ?? el.id);
  void buildDomSelectionForTimelineElement(el).then((selection) => {
    if (!selection) return;
    const anim = animations.find((a) => a.keyframes);
    ...

handleGsapUpdateMeta at useGsapSelectionHandlers.ts:170 gained a selectionOverride param + a shared resolveWriteSelection(selectionOverride) helper — the pattern is systematized across every mutation handler. Nice.

Small residual on onToggleKeyframeAtPlayhead: it still compares clip-relative pct against tween-relative k.percentage at line 332-334 (anim.keyframes.keyframes.find((k) => Math.abs(k.percentage - pct) <= 1)). Only reproduces when the tween doesn't span the whole clip — narrower than my original claim, but worth checking on any tween-in-clip case.

🟠 Concerns — still open at f5d3c7a3d

deleteSelectedKeyframes.ts:23 — keyframedAnimations[0] fallback fires on ungrouped-anim collapsed keys

Unchanged in the restack.

const fallbackAnimation = keyframedAnimations[0];
// ...
const animation = target.animationId
  ? animationsById.get(target.animationId)
  : fallbackAnimation;

timelineKeyframeSelectionKey at timelineKeyframeIdentity.ts:17 still emits the collapsed form when propertyGroup is absent:

if (!target.propertyGroup) return `${elementId}:${target.percentage}`;

And keyframeTarget(kf) at timelineDiamondTypes.ts:113 passes through whatever propertyGroup the underlying TimelineDiamondKeyframe carries — undefined for ungrouped animations. So real UI produces collapsed keys for ungrouped keyframes on multi-anim elements, and this fallback silently deletes from whichever anim happens to be first in selectedGsapAnimations.

Edge-case (needs multi-anim + at-least-one-ungrouped), but the coupling means any element migrating from grouped-only to mixed-group anims silently breaks bulk delete. Fix shape: skip the fallbackAnimation fallback and require target.animationId; if the key form doesn't carry it, refuse to delete rather than deleting from an arbitrary anim.

propertyPanelFlatTextSection.tsx:278-279 — ref read-and-clear during render is a Strict Mode / concurrent footgun

Unchanged in the restack.

const autoFocusActiveField = autoFocusFieldKeyRef.current === activeField.key;
if (autoFocusActiveField) autoFocusFieldKeyRef.current = null;

In React 18's concurrent renderer + Strict Mode, a render can be discarded and re-run. If the first render consumes the marker (clears the ref) but never commits, the retained/committed render sees null at the ref and no autofocus fires — the whole point of the marker.

The docblock at line 260-262 acknowledges the pattern ("read and cleared by the render that first shows the new field") but doesn't address the discarded-render case. A useLayoutEffect that reads-and-clears on activeField.key change, or a useEffect that focuses the newly-added field's DOM node directly, is strict-mode-safe.

useTimelineEditCallbacks.test.tsx:515-520 — watered-down assertion masks the invariant the test title claims to lock

Unchanged in the restack (assertion is now at line 515 after the +14-line grow, still expect.any(Number) at line 518).

The test title is "uses the clip timing basis when retiming a duration-less tween". The whole point is that the computed to-percentage uses the CLIP range (elStart=10.94, elDuration=16.26) rather than a 0.5s default — with dropAbsTime = 10.94 + 0.40 * 16.26 = 17.444 and tweenStart=3.2, tweenDuration=elDuration=16.26, the correct to-percentage is (17.444 - 3.2) / 16.26 * 100 ≈ 87.6.

expect.any(Number) would still pass if the SUT regressed to 50 (the from-percentage) or 0 (the collapsed default). Use expect.closeTo(87.6, 1) instead — or follow the pattern at gsapRuntimeBridge.test.ts:270, which asserts a literal 23.233 from real math for the sibling waypoint case.

Timeline.tsx:267-275 — onSelectSegment returned by hook but not destructured

Unchanged.

const { onClickKeyframe, onShiftClickKeyframe, onContextMenuKeyframe } =
  useTimelineKeyframeHandlers({ ... });

useTimelineKeyframeHandlers exports four handlers; only three are consumed here. onSelectSegment is exercised in useTimelineKeyframeHandlers.test.ts but nothing in the production wire-up consumes it. Either the hook return is dead surface, or the segment-ease wire-up is missing from Timeline.tsx. Worth a note either way.

🟡 Nits — still open at f5d3c7a3d

• gsapDragPositionCommit.ts:318 — parked-but-real: the arc-drag temporal-keyframe branch returns without calling setActiveKeyframePct(null) (the sibling waypoint branch at line 295 clears it). Next arc drag on the same element reuses the stale value. Miguel parked this in the PR body; flagging so it doesn't stay parked forever.
• useTimelineEditCallbacks.test.tsx:381 — docblock lies about what the assertion verifies. buildDomSelectionForTimelineElement is stubbed to always resolve to mocks.selection (box), and this test doesn't override it for the circle case. So toHaveBeenCalledWith(otherFlatAnimation.id, mocks.selection) at :379-382 checks "some selection was propagated" — it CANNOT distinguish "the CLICKED element's own selection" from "the active/box selection", which is what the docblock claims. Either strengthen the stub or trim the docblock.
• files.ts:1056 — parked; server resolveReplacementEaseEach re-parses the full script on every request that omits easeEach, called from both the Acorn and Recast executor paths. The test at files.test.ts:1831 covers the omit-easeEach + arc-path branch but not the omit-easeEach + non-arc branch.
• Test-file bare as T — six occurrences in gsapShared.test.ts:40,43,49 (bare) and useEnableKeyframes.test.ts:173-174 + gsapRuntimeBridge.test.ts (double-cast but no justification comment). CONTRIBUTING.md wants as unknown as T with a comment.
• PR body inaccuracy — "the flat text section drops its two marker-clearing effects" — the base had exactly ONE useEffect and no marker-clearing effects.
• MotionPathOverlay.tsx:525 — onMoveToPlayhead={(_elId, pct) => ...} — first arg is now a TimelineElement after the KeyframeDiamondContextMenu signature change; behaviour unchanged but the variable name _elId is stale.

R9 exception verdict — MIXED (weaker than "strong")

Six of eight clusters hard-couple through shared type / helper add-then-consume:

• (a) diamond identity → emits TimelineKeyframeTarget shape consumed by (b) retime + (e) deletion
• (b) retime → adds resolveEditableTweenDuration in gsapShared.ts consumed by (c) arc drag + (d) toolbar
• (a) → adds the JSON-encoded selection key format decoded by (e)'s timelineKeyframeTargetFromSelectionKey
• (a) + (e) + (f) → all thread the new element: TimelineElement callback contract

But TWO clusters are independently splittable:

• (g) packages/studio-server/src/routes/files.ts + files.test.ts — resolveReplacementEaseEach is a graceful additive fallback; server compiles and its tests pass without any client change. Split path exists.
• (h) propertyPanelFlatTextSection.tsx + .test.tsx — useState-with-two-effects → useRef-with-render-clear refactor touches only this one component + its own test. No cross-cluster type or helper add-then-consume.

The R9 argument is stronger than my default assumption, but two side-quests hitchhike in what could have been the main correctness slice + two small siblings.

PR body promise audit

Delivered (verified at 8c0d9a07d, unchanged in the restack):

• R1: isPlayheadWithinTween takes the selection ✓
• R1: arc drag resolves duration same as click-path sibling ✓
• R1: timelineKeyframeSelectionKey docblock ✓
• R1: #qa-keyframe-box → #qa-zone-keyframe scoped to clip wrapper ✓
• R2: keyframeTarget carries full identity ✓
• R2: Timeline.tsx inline handlers deleted, hook wired ✓
• R2: diamond geometry computed once per diamond into one record ✓

Overstated:

• "Identity flows row → menu → mutator across all views." Row → menu is fixed. Menu → mutator is broken by the TimelineOverlays.tsx:109-115 adapters — see blocker above.
• "Drops its two marker-clearing effects." Base had one useEffect; no marker-clearing effects.

Not asked for but shipped (small scope creep, likely fine but worth flagging):

• Timeline.tsx:450 — new pointerdown early-return over button, input, select, a.

What I didn't verify

• Runtime behaviour of the identity flow through the context-menu path (TimelineOverlays.tsx:109-115 adapters). The blocker is a code-read.
• Whether TimelineOverlays.tsx was intentionally left out of this PR's scope (the file is unchanged in the diff).
• E2E fixtures haven't been walked by hand.

— Review by Rames D Jusso

The base branch was changed.