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]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• 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