feat(studio): consolidate timeline editor callbacks

[miguel-heygen]

What

Consolidates timeline keyframe editing callbacks behind one mutation boundary.

Why

Selection, add/remove, retime, and easing actions previously risked resolving element state through different paths, which can apply edits to stale or incorrect targets.

How

Moves editor mutations into one callback owner and passes explicit element/keyframe identity through callers. The cohesive 1,287-line delta is documented as an R9 exception because splitting it would create temporary dual mutation authority. This is B6 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 4 remaining low/nit findings are parked, verbatim, in .scratch/studio-timeline-family-b/issues/06-pr-2688-deferred-review-findings.md:

• 🟡 packages/studio/src/components/nle/useTimelineEditCallbacks.ts:150 — resolveKeyframeTarget fallback still uses domEditSelection?.id for keyframe-cache lookup
• 🟢 packages/studio/src/components/editor/GsapAnimationSection.tsx:30 — withTrackedGsapAnimationCallbacks recomputed every render — defeats AnimationCard memoization
• 🟢 packages/studio/src/components/nle/useTimelineEditCallbacks.ts:123 — resolveElementAnimations hardcodes 'index.html' twice as default composition path
• 🟢 packages/studio/src/components/editor/AnimationCard.tsx:60 — onFocusSegmentConsumed inline-arrow deps make scroll effect fire every render

---

Supersedes #2688, 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:

• The keyframe-target resolve takes the clicked element's key and reads that element's cache. The diamond context menu and move-to-playhead pass no explicit target, so they used to fall through to whichever element happened to be selected.
• PropertyPanelFlat opens the Motion group by adjusting state during render instead of in an effect, so the card mounts on the same commit the focus request lands on.
• Both animation sections pass a module-level focus consumer instead of a fresh inline arrow, so AnimationCard's focus effect stops re-running every parent render.

The remaining low finding (withTrackedGsapAnimationCallbacks defeating the AnimationCard memo boundary) is parked in .scratch/studio-timeline-family-b/issues/09-family-b-v2-r1-deferred.md. A plain useMemo does not close it: callbacks is a rest-spread object with a fresh identity each render, so the fix has to memoize at the source of each callback.

R2 review follow-ups

Fixed in this PR:

• The lane-header keyframe toggle's remove path resolves the animation through the clicked element's own animations. It used to read the selected element's list, so a non-selected element's flat tween missed the lookup and silently took the remove-one-keyframe branch, stranding the tween instead of deleting it. Covered by "removes a non-selected element's flat tween through that element's own animations" in useTimelineEditCallbacks.test.tsx.

Verified already closed, no change needed:

• The handleGsapAddKeyframeBatch null-selection guard: onTogglePropertyGroupKeyframe returns early when buildDomSelectionForTimelineElement resolves null.
• The Promise<boolean> retime chain: moveKeyframe awaits the raw (non-swallowing) commit and returns its changed flag, handleGsapMoveKeyframe returns that promise, and the diamond reverts its optimistic position on both a false result and a rejection.

R2 review follow-ups

Fixed in this PR:

• Drag-to-retime resolved the dragged diamond against the selected element's animations and committed through the selected element's DOM selection, so dragging a diamond on a clip that was not selected retimed the wrong tween. It now resolves and commits against the clicked element, matching the delete path.
• The three diamond callbacks take the TimelineKeyframeTarget they already had instead of five positional fields, and the two copies of the sourceFile#domId split share splitTimelineElementKey.

Deliberate, stated for the record:

• GsapAddAnimationControl.tsx is new as a file, not as behaviour: the add-animation control moved out of GsapAnimationSection.tsx, which shrinks by roughly the same amount in the same commit.

[miguel-heygen]

• feat(studio): wire expanded keyframe timeline lanes #2791
• fix(studio): harden keyframe editing semantics #2787
• feat(studio): consolidate timeline editor callbacks #2786 👈 (View in Graphite)
• 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 review — head 041ead68

Verdict: COMMENT (2 × P2, 2 × P3). No blockers; deferred-findings block absorbs the perf/nit surface, and the tests around the identity-carrying keyframe resolution are the load-bearing correctness signal. Flagging two golden-rule violations and a fallback-path caveat that the deferred list already names.

Adversarial lenses

• Lens A — signature stability. PASSED. The consolidated callbacks (onDeleteKeyframe, onMoveKeyframeToPlayhead, onMoveKeyframe) accept propertyGroup, tweenPercentage, animationId as optional params in packages/studio/src/player/components/timelineCallbacks.ts:69-90. Legacy 2-arg callers (e.g. TimelineOverlays.tsx:110,115) still compile — new params default to undefined and route through the fallback path. No dropped payload fields.
• Lens B — semantic-vs-symptom. NOTED. useTimelineEditCallbacks.ts:229 introduces if (!anim.keyframes) return false; — flat boundary drags now silent-no-op instead of dispatching a boundary resize. Documented in the diff comment ("Boundary-to-clip resize wiring is intentionally deferred") and covered by the test "safely no-ops a boundary drag while the tween is still flat" (useTimelineEditCallbacks.test.tsx:167-177). Semantics shift is real but intentional.
• Lens C — firing-order determinism. PASSED. The two useEffect hooks in PropertyPanelFlat.tsx:239 and AnimationCard.tsx:32 run in child-mount order: parent opens Motion group first, child then clears the store token via onFocusSegmentConsumed?.(). No callback-fire-order tests broken.
• Lens D — React ref/handler stability. FIRED. See P2s below.
• Lens E — interaction with #2787. NOTED. #2787 hardens the exact surface this PR consolidates (deletion, retime, easing). The consolidate-then-harden order is defensible only because #2786 does not silently regress live behavior: the 8 new test cases in useTimelineEditCallbacks.test.tsx pin the cross-element deletion contract that #2787 will build on. If any of those pins are missing, #2787's hardening becomes tautological. I read them as sufficient.

Findings

P2 — packages/studio/src/components/editor/AnimationCard.tsx:32-38, 40-47. Two new useEffect hooks synchronize state (setExpanded, setExpandedKfPct, imperative scrollIntoView) in response to the focusedSegment prop. This violates the repo golden rule "No useEffect for state syncing" (CLAUDE.md). Parents at GsapAnimationSection.tsx:79 and propertyPanelFlatMotionSection.tsx:161 pass a fresh inline onFocusSegmentConsumed={() => setFocusedEaseSegment(null)} each render → the effect's deps change every render → the effect fires every render (returns early once focusedSegment is null, but still burns work). Prefer a key={focusedSegment?.tweenPercentage ?? "none"} reset on the card, or memoize the consumer callback in the parents. The PR's deferred-findings list already parks this (🟢 AnimationCard.tsx:60) — flagging here because the golden rule marks it a blocker unless suppressed with cause.

P2 — packages/studio/src/components/editor/PropertyPanelFlat.tsx:239-245. Same golden-rule violation: useEffect fires setOpenGroupId("motion") in response to the focusedEaseSegment store subscription. The state-sync belongs either in the store selector (derive openGroupId) or in the click handler that sets focusedEaseSegment in the first place — the effect is a symptom of the write happening in a place that can't reach the setter. Not blocking on this stack tip, but should not proliferate.

P2 — packages/studio/src/components/nle/useTimelineEditCallbacks.ts:148-152 (fallback cache lookup). When callers pass only 2 args (TimelineOverlays.tsx:110 keyframe-diamond context menu, TimelineOverlays.tsx:115 move-to-playhead), resolveKeyframeTarget falls through to usePlayerStore.getState().keyframeCache.get(domEditSelection?.id ?? ""). That's the currently-selected element's cache — if the diamond context menu opens on a non-selected element's diamond, the fallback resolves against the wrong element's cache and returns null (or worse, a same-pct collision from a stale cache entry). The deferred-findings list acknowledges this (🟡 useTimelineEditCallbacks.ts:150); the tests pin the new-args path but do not pin the fallback path against a non-selected element. Not a regression from prior behavior (the old code was equally selection-bound), but the consolidation is now close to fixing this — a two-line change to route the fallback through resolveElementAnimations(elId)'s source-file resolution would close it. Worth resolving inside this PR since the plumbing is already here.

P3 — packages/studio/src/components/editor/GsapAnimationSection.tsx:37 + propertyPanelFlatMotionSection.tsx:139. withTrackedGsapAnimationCallbacks(callbacks, track) runs unconditionally each render and returns a fresh object with fresh function identities → every AnimationCard's memo() boundary is defeated. The prior inline-arrow code had the same defect, so this is not a regression, but the extraction is the natural place to wrap the whole return in useMemo(() => withTracked…, [callbacks, track]). Deferred-findings list has this at 🟢 already.

What's genuinely good

• resolveTimelineKeyframeTarget at useTimelineEditCallbacks.ts:33-51 now prefers kf.animationId before falling back to property-group disambiguation. That closes the ambiguous same-group case that used to return the wrong tween. Pinned by two test cases in useTimelineEditCallbacks.test.ts (same-group collision + stale-identity rejection). Solid correctness lift.
• The cross-element deletion path (onDeleteKeyframe → buildDomSelectionForTimelineElement(element).then(...) → removeKeyframeTarget(..., selection)) is the right shape and pinned by two symmetric tests (flat lane + authored endpoint on non-selected element). This is the load-bearing fix in the PR.
• GsapAddAnimationControl extraction from two callsites with two style variants is a clean dedupe.

— Via

[james-russo-rames-d-jusso]

Reviewed at 041ead68d1fa.

R2 adversarial pass on the editor-callback consolidation. R9 verdict: split_path_exists — the animationId propagation + onDeleteKeyframe per-element authority form a coherent core, but ~5 clusters (telemetry-wrapper DRY, GsapAddAnimationControl UI dedupe, shouldShowMotionPath, focusedEaseSegment auto-open flow, useStudioContextValue.shouldShowMotionPath decoupling) share no mutable authority with the core and could split cleanly. Two blockers in the mutation-authority story the PR claims to fix: (1) onDeleteKeyframe missing null-selection guard silently falls back to domEditSelection — the exact authority the comment on lines 216-218 claims to prevent; (2) onTogglePropertyGroupKeyframe looks up flat-tween identity in selectedGsapAnimations instead of the passed element's animations — a click on a non-selected element's diamond can strand the flat animation instead of removing it. Two orange items on the sibling callbacks that got the fix but weren't converted (onMoveKeyframeToPlayhead and onMoveKeyframe still _elId-discarding).

R9 exception verdict — split_path_exists

Cluster-by-cluster:

• A. animationId propagation + resolveTimelineKeyframeTarget: backward-compat superset with dedicated test — genuinely R9 core.
• B. onDeleteKeyframe per-element authority: independent — handleGsap* helpers already accepted selectionOverride pre-PR (git diff pr-2785-r3...pr-2786-r3 -- packages/studio/src/hooks/useGsapSelectionHandlers.ts is empty).
• C. onTogglePropertyGroupKeyframe: adds a NEW optional field to TimelineEditCallbacks — removing it leaves other callers unaffected (TimelineTrackHeader.tsx:444 uses onTogglePropertyGroupKeyframe?.(...)).
• D. withTrackedGsapAnimationCallbacks: pure telemetry-DRY refactor — pre-PR both sections had inline wrappers with identical semantics.
• E. GsapAddAnimationControl: pure UI dedupe with two style variants — no data-flow entanglement.
• F. shouldShowMotionPath: decouples motion-path visibility from shouldShowSelectedDomBounds (real behavior change: motion path now shows on Layers tab too) — orthogonal to timeline callbacks.
• G. focusedEaseSegment consumption + AnimationCard focus prop + auto-open: independent UI flow (producer in useTimelineKeyframeHandlers.ts, untouched here).

Removing D, E, F, or G individually keeps the remaining PR compiling and behavior-preserving. No dead-at-SHA code (all producers/consumers wired). No dual-authority intermediate contract created by extracting any of D/E/F/G.

🟢 Verified clean

• GsapAddAnimationControl.tsx variant/style split preserves pre-PR classes exactly (both variants' method/cancel/trigger classes match prior inline call sites)
• withTrackedGsapAnimationCallbacks preserves pre-PR tracked-event semantics (verified against gsapAnimationCallbacks.test.ts golden events list)
• resolveTimelineKeyframeTarget's rendered-identity branch guarded by tests 'uses the rendered animation identity to resolve a same-group collision' and 'rejects a rendered animation identity absent from the element' at useTimelineEditCallbacks.test.ts:43-70
• useInspectorState now takes domEditSelection and its four new tests cover the shouldShowMotionPath visibility matrix cleanly

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 a423c9300eed506ae344f05c7bb86fb18e4fe85e (code-review max, R2, delta over R1 at 041ead6).

Verdict

COMMENT (approve-leaning). New head fixes 3 of Rames's R2 authority items in place (🔴 onTogglePropertyGroupKeyframe animations lookup, 🟠 onMoveKeyframe element scoping, 🟡 resolveKeyframeTarget cache key) and 2 of my R1 P2s (PropertyPanelFlat state-sync, useTimelineEditCallbacks fallback cache). Two authority items remain visible in this PR's diff — the other 🔴 (onDeleteKeyframe null-guard) and the sibling 🟠 (onMoveKeyframeToPlayhead _elId discard) — with Miguel replying "Addressed at the stack tip in 6ee750fee (PR #2791)" on both. I flag them here because they are still latent in this PR's own diff; the stack-tip acknowledgment is legitimate but doesn't close the window if #2786 lands ahead of #2791. Not blocking on that judgment call — deferring to Rames on R9 re-check.

CI: Perf skipping / Preflight matrix skipping are normal; Graphite mergeability pending. No red required checks.

Rames R2 findings — status at new head

• 🔴 useTimelineEditCallbacks.ts:217 onDeleteKeyframe missing null-selection guard — not fixed in-place. Then-callback still forwards selection unconditionally to removeKeyframeTarget, which passes null through to handleGsapDeleteAnimation/handleGsapRemoveKeyframe; both do selectionOverride ?? domEditSelection ?? lastSelectionRef.current, so an explicit null still nullish-coalesces to domEditSelection. Miguel: "Addressed at the stack tip in 6ee750fee (PR #2791)". Confirmed the guard shape now exists on onTogglePropertyGroupKeyframe (if (!selection) return; at line 307) — the pattern is adopted in this file, just not on this callsite.
• 🔴 useTimelineEditCallbacks.ts:328 onTogglePropertyGroupKeyframe wrong-animations-array — FIXED at new head. Now passes resolveElementAnimations(element.key ?? element.id) instead of selectedGsapAnimations, exactly matching Rames's prescribed fix. Comment inline names the intent ("The clicked element's animations, not the selected element's").
• 🟠 useTimelineEditCallbacks.ts:216 onMoveKeyframeToPlayhead _elId discard — not fixed in-place. Body is (elId, keyframe) => { const target = resolveKeyframeTarget(keyframe, resolveElementAnimations(elId), elId); if (target) handleGsapMoveKeyframeToPlayhead(target.animId, target.tweenPct); }. elId is now consumed for resolveElementAnimations + cache-key resolution (so target lookup is now correct), but handleGsapMoveKeyframeToPlayhead still receives no selection override — commits against domEditSelection. Miguel: stack-tip in 6ee750fee.
• 🟠 useTimelineEditCallbacks.ts:229 onMoveKeyframe _elId discard — FIXED at new head. Now: const element = usePlayerStore.getState().elements.find(...); const sel = element ? await buildDomSelectionForTimelineElement(element) : domEditSelection; if (!sel) return false; — and sel is threaded as the 4th arg to handleGsapMoveKeyframe, handleGsapResizeKeyframedTween, and handleGsapUpdateMeta. This is precisely what Rames prescribed.
• 🟡 useTimelineEditCallbacks.ts:154 cache key hardcoded to domEditSelection?.id — FIXED at new head. resolveKeyframeTarget now accepts an optional elementKey third param, cache lookup is keyframeCache.get(elementKey ?? domEditSelection?.id ?? ""), and all three callsites in this file thread elId through. Latent authority leak closed.
• 🟢 GsapAnimationSection.tsx:58 focus-segment gate lacks element-id scoping — unchanged. Still focusedEaseSegment?.animationId === anim.id ? focusedEaseSegment : null with no elementId filter, while the flat sibling at propertyPanelFlatMotionSection.tsx:148 gates on both. Miguel: stack-tip.
• 🟢 useTimelineEditCallbacks.ts:71 synthesized-target tweenPct: kf.tweenPercentage ?? pct — unchanged. Miguel: stack-tip. Rames noted "not triggered today" (all diamond callers currently pass tweenPercentage), so latent.
• 🟢 AnimationCard.tsx:62 scroll effect race with ease-editor mount — unchanged. Two useEffect hooks still present at lines 58-64 and 66-73. Miguel: stack-tip.
• 🟡 useStudioContextValue.ts:99 shouldShowMotionPath split-suggestion — unchanged. Miguel: stack-tip. Split is a PR-scope judgment, not a code defect.

My R1 P2/P3 — status

• P2 AnimationCard.tsx:32-38, 40-47 (useEffect state-sync, golden-rule): not addressed at this SHA. Same pattern; parents at GsapAnimationSection.tsx:60 and propertyPanelFlatMotionSection.tsx:150-ish still pass a fresh inline onFocusSegmentConsumed. Miguel: stack-tip via 6ee750fee.
• P2 PropertyPanelFlat.tsx:239-245 (useEffect for setOpenGroupId("motion"), golden-rule): FIXED at new head. Converted to the derived-state-in-render pattern (React's "You Might Not Need an Effect"): const [consumedFocus, setConsumedFocus] = useState(focusedEaseSegment); if (focusedEaseSegment !== consumedFocus) { setConsumedFocus(...); ... if (focusesThisPanel) setOpenGroupId("motion"); }. Also now includes the focusedEaseSegment.elementId === renderedElementId scoping — a bonus fix that closes the shared-animation-id spread case in one direction.
• P2 useTimelineEditCallbacks.ts:148-152 fallback cache lookup wrong element: FIXED at new head (see 🟡 above — same fix closes it).
• P3 GsapAnimationSection.tsx:37 + propertyPanelFlatMotionSection.tsx:139 withTrackedGsapAnimationCallbacks(callbacks, track) unmemoized: unchanged. Not a regression from pre-PR behavior; deferred-findings list has it.

Miguel's inline comments — status

Miguel dropped 9 identical replies at 2026-07-27T14:50Z on Rames's R2 threads: "Addressed at the stack tip in 6ee750fee (PR #2791, codex/studio-timeline-b-expanded-lanes-v2). This branch is an ancestor in the same stack, so the fix ships with it rather than appearing in this PR's own diff. Verified on the tip: bun run --cwd packages/studio typecheck, bunx oxlint packages/studio/src (0 warnings, 0 errors), and bun run --cwd packages/studio test (2916 passed | 18 todo, 260 files)."

Confirmed: PR #2791 exists (feat(studio): wire expanded keyframe timeline lanes, +2430/-931, base codex/studio-timeline-b-interaction-hardening-v2), state open, head 910ef264. The stack is real, ancestry claim is real.

Two of Miguel's replies point at findings that are ALSO fixed in this PR's own diff at new head (🔴 onTogglePropertyGroupKeyframe, 🟠 onMoveKeyframe, 🟡 cache key) — the acknowledgment is a superset. The remaining 6 replies point at findings genuinely deferred to #2791. No dispute on any thread; no new blocker raised by Miguel.

Delta between old and new heads

Compare 041ead68d1fa...a423c930 (ahead 23, behind 5) includes stack-rebase churn from PR #2785's tip. Filtering to files owned by this PR (21 files, +1195/-386 against base 555d7f7a), the substantive in-scope deltas are:

• useTimelineEditCallbacks.ts: resolveKeyframeTarget gains optional elementKey; three call-sites thread elId through; onMoveKeyframe now looks up element + builds per-element DOM selection + threads through all three handleGsap* writers; onTogglePropertyGroupKeyframe now uses resolveElementAnimations(element.key ?? element.id).
• PropertyPanelFlat.tsx: useEffect for setOpenGroupId replaced with derived-state pattern + renderedElementId scoping.
• No changes to AnimationCard.tsx or GsapAnimationSection.tsx deltas (both untouched at new head w.r.t. R1 findings).
• Tests: useTimelineEditCallbacks.test.tsx at +469 new. Scan reveals coverage for the fixed-in-this-PR authority items but no explicit null-resolution assertion for buildDomSelectionForTimelineElement in the delete path — consistent with Rames's original observation.

Fix-internal remaining-silent-X audit

For each in-this-PR-fixed authority item, audit whether the fix itself contains a residual silent-fallback:

• onMoveKeyframe (fixed): const sel = element ? await buildDomSelectionForTimelineElement(element) : domEditSelection; — the : domEditSelection fallback fires only when element lookup fails; that path is one if (!sel) return false; away from being caught. Not a silent write to the wrong element in the normal path, but a subtle regression surface if elements store ever contains a stale key. Acceptable — elId originates from the element that emitted the callback, so find(el => (el.key ?? el.id) === elId) should succeed on any live diamond.
• onTogglePropertyGroupKeyframe (fixed): if (!selection) return; guard present. resolveElementAnimations(element.key ?? element.id) returns [] on cache miss → removeKeyframeTarget finds no matching animation → falls through to handleGsapRemoveKeyframe(animationId, percentage, undefined, selectionOverride). On empty animations array with remove: true, this dispatches remove-keyframe against a possibly-nonexistent animation — silent no-op at the writer layer, not a silent write to the wrong target. Acceptable.
• resolveKeyframeTarget cache key (fixed): keyframeCache.get(elementKey ?? domEditSelection?.id ?? "") — the ?? "" fallback still targets the "" key; a missing cache entry returns undefined and callers see cached?.keyframes ?? [], falling through to resolveTimelineKeyframeTarget with an empty keyframes array → returns null → caller no-ops. No silent write.

No residual silent-X inside the fix boundaries.

Standards lens re-run

Grep on new lines only across the 21 in-PR files at new head:

• Bare as T casts: 0 findings.
• Non-null assertions (!., !), !,): 0 findings.
• .message access without instanceof Error narrowing: 0 findings.
• Angle-bracket casts <T>x: 0 findings.
• Golden-rule useEffect state-syncing in NEW code: 1 finding — the two AnimationCard.tsx effects (58-64, 66-73). Both pre-existed at R1; explicitly deferred by Miguel to stack tip.
• CONTRIBUTING.md grep (worktree rule, tw- prefix, semantic tokens, useTypedTranslation): N/A — package is packages/studio/, not packages/movio/; different conventions apply (Tailwind classes here are e.g. bg-neutral-800 since this is the studio design system pre-heygen-design migration).
• Bi-directional spec bullet-check: PR body claims "consolidate timeline editor callbacks" — new code touches only editor-callback consolidation surface; shouldShowMotionPath decoupling is orthogonal (Rames 🟡 flag stands). No hidden scope expansion detected beyond what Rames named.
• Sibling-precision divergence: GsapAnimationSection still lacks elementId scoping that propertyPanelFlatMotionSection has (Rames 🟢). Persists.
• Middle-man wrap/unwrap audit: withTrackedGsapAnimationCallbacks returns a fresh object per render — a pass-through wrapper that defeats memo on AnimationCard. Pre-existing; noted in R1 P3.

Independent findings

Nothing new beyond what Rames R2 and my R1 already surfaced. The concrete deltas at new head resolve the majority of the authority-scoping concerns; the residuals are Miguel-acknowledged with a stack-tip pointer.

Peer state

• Rames R2 (COMMENTED at 041ead68, review id 4780132643): R9 split_path_exists — the callback consolidation core is R9-legitimate; D/E/F/G clusters splittable. Two 🔴 blockers named — one fixed at new head, one deferred to #2791. Rames has not re-reviewed at a423c930 yet; my "authority items remaining in-place" flag defers to his R9 re-check.
• Miguel (author, 9 inline replies at 2026-07-27T14:50Z): explicit stack-tip pointer on every Rames item + my P2s. Not disputing any finding.
• Family B lens noted in Rames's R2 body confirmed applicable: mutation-authority thread + Promise chain — covered.

— Review by Via

[james-russo-rames-d-jusso]

Reviewed at a423c93 — R3 delta from my R2 at 041ead68d1fa.

Delta summary vs. R2

Substantial closure on the mutation-authority thread since R2:

• ✅ R2 blocker #1 — onDeleteKeyframe silent-fallback: primary fix landed (elId now passed explicitly, no _elId discard). See finding 2 below — the .then callback still lacks the null-guard the sibling handlers all have. Downgraded from blocker to concern.
• ✅ R2 blocker #2 — onTogglePropertyGroupKeyframe looked up in selectedGsapAnimations: CLOSED cleanly. Signature widened to take element, awaits buildDomSelectionForTimelineElement(element), guards on !selection. Covered by "removes a non-selected element's flat tween through that element's own animations" in the new test file.
• ✅ R2 orange #3 — onMoveKeyframe silent-fallback: CLOSED. Now takes elId, awaits the clicked element's selection, guards on !sel, threads sel through to handleGsapMoveKeyframe.
• 🔴 R2 orange #4 — onMoveKeyframeToPlayhead _elId-discarding: PARTIALLY fixed. The fix hoisted elId propagation into this callsite but stopped short of the downstream handler — see finding 1 below. This is the R3 blocker.

Findings

🔴 Blocker

1. onMoveKeyframeToPlayhead at packages/studio/src/components/nle/useTimelineEditCallbacks.ts:220-223 — consolidation incomplete at sibling handler; fix hoists anim resolution but leaves write against the CURRENT selection.

onMoveKeyframeToPlayhead: (elId, keyframe) => {
  const target = resolveKeyframeTarget(keyframe, resolveElementAnimations(elId), elId);
  if (target) handleGsapMoveKeyframeToPlayhead(target.animId, target.tweenPct);
},

handleGsapMoveKeyframeToPlayhead at packages/studio/src/hooks/useGsapSelectionHandlers.ts:328-340 accepts a selectionOverride?: DomEditSelection | null param, and does two things this callsite fails to override:

const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current;
if (!sel) return;
const anim = selectedGsapAnimations.find((a) => a.id === animId);   // <-- current selection
const toPercentage = computeCurrentPercentage(sel, anim);
moveKeyframe(sel, animId, fromPercentage, toPercentage);            // <-- current selection

Right-clicking a diamond on a non-selected element and picking "Move to playhead": resolveKeyframeTarget correctly gives an animId from the clicked element's animations, then the handler runs selectedGsapAnimations.find(a => a.id === animId) against the CURRENT selection's animations — undefined when the clicked element and selected element differ — and moveKeyframe(currentSelection, animIdFromClickedElement, ...) silently writes to the wrong file (or against an animId absent in the current file).

The sibling onMoveKeyframe at useTimelineEditCallbacks.ts:232-248 got this right — awaits buildDomSelectionForTimelineElement(element), guards on !sel, passes sel to handleGsapMoveKeyframe as the fourth arg. Mirror that here.

The test at useTimelineEditCallbacks.test.tsx:295 asserts .toHaveBeenCalledWith(id, 100) — 2 positional args — so it locks in the broken 2-arg call shape.

Suggested fix:

onMoveKeyframeToPlayhead: async (elId, keyframe) => {
  const target = resolveKeyframeTarget(keyframe, resolveElementAnimations(elId), elId);
  if (!target) return;
  const element = usePlayerStore.getState().elements.find((el) => (el.key ?? el.id) === elId);
  if (!element) return;
  const selection = await buildDomSelectionForTimelineElement(element);
  if (!selection) return;
  handleGsapMoveKeyframeToPlayhead(target.animId, target.tweenPct, selection);
},

And tighten the test assertion to include the selection.

🟠 Concerns

2. onDeleteKeyframe missing null-guard on resolved selection at useTimelineEditCallbacks.ts:215-217 — silently reuses CURRENT selection when clicked element can't be built.

void buildDomSelectionForTimelineElement(element).then((selection) => {
  removeKeyframeTarget(target.animId, target.tweenPct, animations, selection);
});

buildDomSelectionForTimelineElement returns Promise<DomEditSelection | null>. When resolution fails (element not in preview iframe, race with unmount), selection is null. removeKeyframeTarget(..., null) passes null to handleGsapRemoveKeyframe / handleGsapDeleteAnimation, both of which fall back to selectionOverride ?? domEditSelection ?? lastSelectionRef.current — a null override silently reuses the CURRENT domEditSelection, defeating the whole "commit against the CLICKED element" comment above this block. Sibling handlers already have the guard (onMoveKeyframe:243 if (!sel) return false;, onTogglePropertyGroupKeyframe:324 if (!selection) return;).

Fix: if (!selection) return; inside the .then callback.

3. onChangeKeyframeEase at useTimelineEditCallbacks.ts:297-301 — ignores clicked elId, iterates every keyframed anim in the CURRENT selection.

onChangeKeyframeEase: (_elId: string, _pct: number, ease: string) => {
  for (const anim of selectedGsapAnimations) {
    if (anim.keyframes) handleGsapUpdateMeta(anim.id, { ease });
  }
},

The underscore prefixes on _elId/_pct signal discarded params. TimelineOverlays.tsx:111 wires onChangeEase={(elId, pct, ease) => onChangeKeyframeEase?.(elId, pct, ease)} with the CLICKED elId — this callback discards it and iterates the CURRENT selection's animations. Ease on a right-clicked non-selected diamond updates the ease on whatever's currently selected. Exactly the "easing action resolves through a different path than delete/retime" motivation the PR body called out.

Secondary bug in the same block: even within the selected element, this updates ease on every keyframed animation — a shared-group tween (e.g. two motion keyframes on the same element) has both re-eased, not just the one the user clicked.

Fix: resolve via resolveElementAnimations(elId) + clicked element's selection + resolveKeyframeTarget to pick out the specific animId, pass selection through to handleGsapUpdateMeta.

4. onToggleKeyframeAtPlayhead at useTimelineEditCallbacks.ts:303-320 — reads pct from clicked el but writes against selectedGsapAnimations.

onToggleKeyframeAtPlayhead: (el: TimelineElement) => {
  const pct = el.duration > 0
    ? Math.max(0, Math.min(100, Math.round(((currentTime - el.start) / el.duration) * 100)))
    : 0;
  const anim = selectedGsapAnimations.find((a) => a.keyframes);
  ...
}

pct computed against passed el's start/duration (CLICKED element), animation looked up in selectedGsapAnimations. Mixed-frame bug: a keyframe added at "playhead within the clicked clip" actually lands at "playhead within the selected clip's window" if the two disagree. Same class at onDeleteAllKeyframes:195 (zero-arg signature so no elId to consult, but still iterates selectedGsapAnimations — fine per signature, not the consolidation the PR body claims for delete scope).

Fix: route through resolveElementAnimations(el.key ?? el.id) + buildDomSelectionForTimelineElement(el), thread selection into handleGsapRemoveKeyframe / handleGsapAddKeyframe / handleGsapConvertToKeyframes (all three accept selectionOverride).

🟡 Nit

5. resolveKeyframeTarget at useTimelineEditCallbacks.ts:140-162 — fallback branch is dead at this SHA + memo dep pollution.

.keyframeCache.get(elementKey ?? domEditSelection?.id ?? "");
...
[domEditSelection?.id, selectedGsapAnimations],

All three current call sites (:205, :221, :234) pass elId. elementKey is always defined + truthy in live paths. Costs: hides the invariant that every caller must pass elementKey; forces domEditSelection?.id into the useCallback deps → resolveKeyframeTarget re-identifies on every selection change → by transitive dep at line 359, whole consolidated TimelineEditCallbacks memo re-identifies. Render-thrash cost across every TimelineEditProvider consumer. Parked P2 in PR body — the parking is honest, but this is a soft R9 dead-code tell (would only become live once a future PR wires a caller without elementKey).

Fix: make elementKey: string required, drop the two fallback tokens, drop domEditSelection?.id from the deps.

❓ Question

6. PropertyPanelFlat.tsx:258 render-time state may swallow initial-mount focus.

useState(focusedEaseSegment) initializes consumedFocus to CURRENT — meaning if the panel remounts with focusedEaseSegment already set to a request targeting this panel, consumedFocus === focusedEaseSegment on first render, the branch is skipped, openGroupId stays at Style/Layout/Text default. Design might assume focus requests only arrive AFTER mount (true for user-driven ease clicks) but not documented. Would flip to useState<typeof focusedEaseSegment>(null) if the design allows focus to survive a selection change.

R9 exception verdict revisit — WEAK

My R2 was split_path_exists naming 5 clusters. Current diff still has independent clusters: useStudioContextValue (adds domEditSelection param), GsapAddAnimationControl extraction, PropertyPanelFlat render-time openGroup adjustment, and the callback consolidation proper. None contains dead code that would only become live once another cluster lands — the R9 "splitting creates dual mutation authority" claim doesn't hold up on inspection. Ownable as consolidation-slice + telemetry-slice + UI-dedupe-slice + motion-path-slice + PropertyPanelFlat-focus-slice without any intermediate contract breakage.

What I didn't verify

• End-to-end rapid-second retime path composing two onMoveKeyframe invocations without the store re-caching between them (test file mocks buildDomSelectionForTimelineElement away).
• Behavior of computeCurrentPercentage(sel, undefined) in handleGsapMoveKeyframeToPlayhead when the animId isn't in the current selection — the blocker's exact failure mode depends on this.
• Whether focusedEaseSegment.elementId === renderedElementId guard at PropertyPanelFlat.tsx:264 is strict enough given the sibling PR's key-scoping question.

Stamp routing per convention. Leaving as COMMENTED.

— Review by Rames D Jusso

[vanceingalls]

Reviewed at a423c9300eed506ae344f05c7bb86fb18e4fe85e (code-review max, R3, delta vs Rames R3 at 4791736131 and my R2 at 4791680713). Head unchanged since R2 — this is a fresh-eyes reconcile against Rames R3.

Verdict

COMMENT (block-leaning). Matching Rames R3. My R2 was approve-leaning on the premise that residual authority items were Miguel-acknowledged for the stack tip; Rames R3 revisits the R9 exception verdict as WEAK — the callback-consolidation slice is independently splittable, so the stack-tip pointer to 6ee750fee (PR #2791) is a valid ack of the fix's existence but not a valid excuse for shipping the bug latent in this PR's own diff. Rames R3 also upgrades my R2 🟠 onMoveKeyframeToPlayhead to 🔴 blocker with the full mechanism trace, and surfaces two authority-scoping bugs I missed in R2 (onChangeKeyframeEase, onToggleKeyframeAtPlayhead). I match all three at this head. Stamp routing per convention; leaving as COMMENTED.

Rames R3 findings — status at this head

• 🔴 onMoveKeyframeToPlayhead — MATCH (upgraded from my R2 🟠). Verified at useTimelineEditCallbacks.ts:220-223 (2-arg call handleGsapMoveKeyframeToPlayhead(target.animId, target.tweenPct)) + useGsapSelectionHandlers.ts:328-340 (handler accepts selectionOverride?: DomEditSelection | null, does sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current AND selectedGsapAnimations.find(a => a.id === animId) against the current selection). Right-click "move to playhead" on a diamond of a non-selected element: resolveKeyframeTarget correctly resolves to the clicked element's animId, then the writer runs .find against the current selection's animations (undefined when they differ) and commits moveKeyframe(currentSelection, clickedElementAnimId, ...) — writes to the wrong file, or no-ops against an animId absent from the current selection. Sibling onMoveKeyframe at :232 already does the correct pattern; mirror it here. Test at useTimelineEditCallbacks.test.tsx:300-303 locks in the 2-arg shape (toHaveBeenCalledWith(otherKeyframedAnimation.id, 100)), so the tighten-fix must also update the assertion. Confirmed independently.
• 🟠 onDeleteKeyframe missing null-guard — MATCH (downgraded from my R2 🔴 to Rames R3 🟠 concern). Lines 215-217: void buildDomSelectionForTimelineElement(element).then((selection) => removeKeyframeTarget(target.animId, target.tweenPct, animations, selection)). buildDomSelectionForTimelineElement returns Promise<DomEditSelection | null>. On null resolution, null flows through removeKeyframeTarget → handleGsapDeleteAnimation/handleGsapRemoveKeyframe, both of which do sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current — nullish coalescing means the explicit-null override still resolves to current domEditSelection. Silent write against the CURRENT selection when the CLICKED element's selection failed to build. Sibling onTogglePropertyGroupKeyframe:324 already has if (!selection) return;. My R1 P2 named the sibling-precision gap; my R2 called it out; Rames R3 downgrades it because the primary silent-fallback was closed. Rames's severity call is fair — with elId now propagated correctly through target lookup, the residual is a rare error-path silent write, not the primary bug.
• 🟠 onChangeKeyframeEase — MATCH (new finding, missed in my R2). Lines 297-301: (_elId: string, _pct: number, ease: string) => { for (const anim of selectedGsapAnimations) if (anim.keyframes) handleGsapUpdateMeta(anim.id, { ease }); }. Underscore-prefixed _elId/_pct are discarded even though callers (per Rames's TimelineOverlays.tsx:111 trace — verified the file lives at packages/studio/src/player/components/TimelineOverlays.tsx) wire the clicked elId through. Ease change on a right-clicked diamond of a non-selected element re-eases whatever is currently selected. Secondary bug within the same block: even inside the selected element, iterates for (const anim of selectedGsapAnimations) if (anim.keyframes) — a shared-group tween with multiple keyframed anims (e.g. two motion keyframes on the same element) has ALL of them re-eased, not just the one the user clicked. Fix: resolve via resolveElementAnimations(elId) + clicked element's selection + resolveKeyframeTarget for the specific animId, thread selection through handleGsapUpdateMeta. Same authority-scoping class Rames R2 originally named — should have been folded into the R2 fix pass.
• 🟠 onToggleKeyframeAtPlayhead — MATCH (new finding, missed in my R2). Lines 303-320: pct = ...(currentTime - el.start) / el.duration... (CLICKED element's timing) followed by const anim = selectedGsapAnimations.find((a) => a.keyframes) (CURRENT selection). Mixed-frame write: pct computed against the clicked clip's window, keyframe added/removed against the currently-selected clip's anim. When they differ (right-click on a non-selected clip's timeline), the keyframe lands at "playhead position within the selected clip" rather than "playhead position within the clicked clip", which are different clip-relative percentages when the clips have different starts/durations. Same fix shape as onChangeKeyframeEase — route through resolveElementAnimations(el.key ?? el.id) + buildDomSelectionForTimelineElement(el) + thread selection into handleGsapRemoveKeyframe/handleGsapAddKeyframe/handleGsapConvertToKeyframes (all three accept selectionOverride).
• 🟡 resolveKeyframeTarget fallback dead + memo dep pollution — MATCH (my R2 partially closed). My R2 marked the cache-key fix as CLOSED because elementKey ?? domEditSelection?.id ?? "" now respects the caller's elId. Rames R3's stricter reading: at this SHA all three callers (:205, :221, :234) pass elId, so the domEditSelection?.id fallback is dead code AND the useCallback deps still include domEditSelection?.id, forcing resolveKeyframeTarget to re-identify on every selection change → transitive re-identification of the whole TimelineEditCallbacks memo (via handleTimelineEditCallbacks at line 359-ish). Render-thrash across every TimelineEditProvider consumer. Rames's read is correct — the R9 dead-code tell is that the fallback would only become live once a future caller omits elementKey, which the code shape doesn't currently need. Fix: make elementKey: string required, drop the fallback tokens, drop domEditSelection?.id from the deps. My R2 read this as "latent-authority-leak closed"; Rames R3 reads it as "clean-up left on the table". Both are true — the write path is fixed, the render-perf path isn't.
• ❓ PropertyPanelFlat.tsx:258 initial-mount focus swallow — MATCH (open question). useState(focusedEaseSegment) initializes consumedFocus to the CURRENT store value. If the panel remounts with focusedEaseSegment already set to a request targeting this panel, consumedFocus === focusedEaseSegment on first render → the branch is skipped → openGroupId stays at Style/Layout/Text default → the request is silently swallowed. Design might rely on focus-requests only arriving AFTER mount (true for user-driven ease clicks) but not documented. useState<typeof focusedEaseSegment>(null) would let a pre-existing request survive a selection change. Non-blocking — the answer depends on the sibling PR #2785's key-scoping design intent; parking as a question.

My R2 residuals — status

• 🔴 onDeleteKeyframe null-guard: downgraded to 🟠 concern per Rames R3. Retained as an active concern in this review.
• 🟠 onMoveKeyframeToPlayhead _elId discard: upgraded to 🔴 blocker per Rames R3. Retained as an active blocker in this review.
• 🟡 resolveKeyframeTarget cache key: partial retract. My R2 said "FIXED" — Rames R3 says the write-path is fixed but the fallback branch is now dead and the memo dep pollution persists. Both reads are compatible; adopting the stricter Rames read.

R2 items closed (still closed at this head): 🔴 onTogglePropertyGroupKeyframe animations lookup, 🟠 onMoveKeyframe element scoping, P2 PropertyPanelFlat state-sync. No re-verification needed — head unchanged since R2.

Miguel inline comment status

All 9 Miguel replies at 2026-07-27T14:50Z are identical stack-tip pointers to 6ee750fee (PR #2791). No fresh replies to Rames R3 or my R2. The stack-tip ack is real (verified #2791 open, head 910ef264, base is this branch), but Rames R3's WEAK verdict on the R9 exception undermines the "the fix ships with the stack" rationale — the callback-consolidation slice IS ownable without downstream contract breakage, so residual bugs in this PR's diff are not excused by their being fixed on the tip.

No fresh dispute from Miguel on any thread; awaiting his response to Rames R3.

Fix-internal remaining-silent-X audit

New audit vs my R2, extending to the sibling handlers Rames R3 named:

• onDeleteKeyframe (partially-fixed): missing if (!selection) return; inside .then — silent fallback via nullish ?? on both writer paths. Rames finding #2. Real.
• onMoveKeyframeToPlayhead (partially-fixed): correctly resolves target.animId via clicked element's animations, but writer receives no selection override → handleGsapMoveKeyframeToPlayhead runs .find against current selection AND writes to current selection. Rames finding #1. Real.
• onChangeKeyframeEase (unchanged from pre-PR): _elId/_pct discarded, iterates current selection. Rames finding #3. Real.
• onToggleKeyframeAtPlayhead (unchanged from pre-PR): pct from clicked el, anim from current selection. Rames finding #4. Real.
• resolveKeyframeTarget (write-path fixed): fallback branch dead + memo dep pollution. Rames finding #5. Real (render-perf, not correctness).

The class-of-bug my R2 audit missed: the R2 "authority items in-place" flag caught onDeleteKeyframe and onMoveKeyframeToPlayhead, but I did not extend the audit to onChangeKeyframeEase / onToggleKeyframeAtPlayhead at this head — both were pre-PR shapes that survived the consolidation pass. Rames R3's fresh-eyes reconcile caught the two I missed. Feedback taken forward: on a consolidation-family PR, audit EVERY sibling handler at the same file boundary against the fixed shape, not just the ones the PR body enumerates as in-scope.

Standards lens re-run

Grep on the 21 in-PR files at this SHA (fresh pass, not lifted from R2):

• Bare as T casts on new lines: 0 findings.
• Non-null assertions (!., !), !,) on new lines: 0 findings.
• .message access without instanceof Error narrowing on new lines: 0 findings.
• Angle-bracket casts <T>x on new lines: 0 findings.
• Golden-rule useEffect state-syncing in NEW code: 1 finding — AnimationCard.tsx:58-64, 66-73. Both pre-existed at R1; Miguel-deferred to stack tip. No delta at new head.
• CONTRIBUTING.md grep (worktree rule, tw- prefix, semantic tokens): N/A — packages/studio/, not packages/movio/.
• Bi-directional spec bullet-check: PR body claims "consolidate timeline editor callbacks". Rames R9 delta says shouldShowMotionPath, useStudioContextValue.ts, GsapAddAnimationControl extraction, PropertyPanelFlat focus, and callback consolidation are all independent clusters. My R2 accepted the R9 "single mutation-authority slice" claim; Rames R3 rejects it. Reviewing the diff shape again: shouldShowMotionPath at useStudioContextValue.ts:99 is a boolean flag orthogonal to the callback wiring; GsapAddAnimationControl is a component extraction unrelated to callback signatures; PropertyPanelFlat derived-state is a golden-rule fix, not a callback-authority change. Rames's WEAK verdict on the R9 exception is defensible. Bi-directional check downgraded from R2: PR title claims narrower scope than the diff delivers.
• Sibling-precision divergence: GsapAnimationSection still lacks elementId scoping that propertyPanelFlatMotionSection has. Persists at this head. (Miguel: stack-tip.)
• Middle-man wrap/unwrap audit: withTrackedGsapAnimationCallbacks returns fresh object per render. Persists. (Pre-existing; R1 P3.)
• Multi-shape throw normalization: N/A (no error-throwing code in this PR's diff).
• Test-lock-value-vs-reachability: 1 new finding — useTimelineEditCallbacks.test.tsx:300-303 locks in the 2-arg handleGsapMoveKeyframeToPlayhead call shape. When Rames R3's blocker fix lands, this assertion must be tightened to include the selection override; the current test would falsely-pass on the buggy shape. Fixture-as-gate risk.

Peer state

• Rames R3 (COMMENTED at a423c930, review id 4791736131, at 2026-07-27T21:40Z): raises 1 new 🔴 (onMoveKeyframeToPlayhead), 2 new 🟠 (onChangeKeyframeEase, onToggleKeyframeAtPlayhead), 1 new 🟡 (resolveKeyframeTarget dead-fallback + memo deps), 1 ❓ (PropertyPanelFlat init state). Downgrades R2 🔴 onDeleteKeyframe to 🟠 concern. Explicit R9 verdict revisit: WEAK — the R9 split_path_exists claim from R2 does not hold at this SHA. Stamp-routing: COMMENTED.
• Rames R2 (COMMENTED at 041ead68, review id 4780132643): superseded by R3. All items resolved or forwarded as noted above.
• My R2 (COMMENTED at a423c930, review id 4791680713): superseded by this R3. Two residuals I flagged (onDeleteKeyframe, onMoveKeyframeToPlayhead) are now matched at Rames R3's severity; two Rames R3 new findings I missed (onChangeKeyframeEase, onToggleKeyframeAtPlayhead) surfaced here.
• Miguel (author): 9 stack-tip pointers at 14:50Z; no fresh response to Rames R3 or this R3 yet.
• N-of-N lens convergence on onMoveKeyframeToPlayhead: R2-Rames + R2-Via + R3-Rames + R3-Via = 4-of-4 independent reviews. High-confidence blocker at this head.

— Review by Via

[vanceingalls]

Verified at 9fc00117037444711bac6963d35ee35ebc268da9 (R4, fix commit over R3 at a423c93).

Verdict

APPROVE. All four R3 authority items closed, byte-clean on the 19 files outside the fix slice, and the buggy 2-arg test shape is now locked in with the correct per-clicked-element arg contract.

R3 authority items — status at new head

Finding	File:line at 9fc0011	Closed	Evidence
onMoveKeyframeToPlayhead — pct from clicked el, anim + selection from current	useTimelineEditCallbacks.ts:222-236 + useGsapSelectionHandlers.ts:341-361	yes	Handler now resolves animations + animation + per-el element from elId, awaits buildDomSelectionForTimelineElement(element), then calls handleGsapMoveKeyframeToPlayhead(target.animId, target.tweenPct, selection, animation). Writer takes new 4th arg animationOverride?: GsapAnimation; const anim = animationOverride ?? selectedGsapAnimations.find(...) (line 355). One-frame invariant enforced.
onDeleteKeyframe null-guard — explicit null fell through via ?? to domEditSelection	useGsapSelectionHandlers.ts:126-132	yes	New resolveWriteSelection(selectionOverride) uses strict === undefined check: undefined → domEditSelection ?? lastSelectionRef.current, null → null (abort). Every write handler swapped to the helper (handleGsapUpdateMeta/Delete/Add/Remove/Move*/Resize/ConvertToKeyframes). Caller in useTimelineEditCallbacks.ts:174-177 simplified — no more branch on selectionOverride === undefined.
onChangeKeyframeEase — _elId/_pct discarded, iterated current selection	useTimelineEditCallbacks.ts:310-323	yes	elId now used to resolve animations via resolveElementAnimations(elId) AND to resolve element from usePlayerStore.getState().elements, then per-el selection awaited and passed to handleGsapUpdateMeta(anim.id, { ease }, selection). Ease is anim-wide (not keyframe-scoped), so _pct remaining unused is semantically consistent with handleGsapUpdateMeta's API.
onToggleKeyframeAtPlayhead — pct from clicked, anim from current	useTimelineEditCallbacks.ts:325-359	yes	animations = resolveElementAnimations(el.key ?? el.id) + buildDomSelectionForTimelineElement(el) — pct, anim, and selection all read off the same el. Convert-to-keyframes path now threads selection via new 5th param selectionOverride on handleGsapConvertToKeyframes (writer at useGsapSelectionHandlers.ts:401-409).

Test discipline

• useTimelineEditCallbacks.test.tsx:298-311 — buggy 2-arg shape gone. Assertion is now handleGsapMoveKeyframeToPlayhead(otherKeyframedAnimation.id, 100, circleSelection, otherKeyframedAnimation). The circleSelection fixture is a real per-el mock ({ id: "circle", selector: "#circle", sourceFile: "scenes/main.html" }) with buildDomSelectionForTimelineElement.mockResolvedValue(circleSelection) — the test would fail if any of pct / selection / animation came from the wrong frame. Delete-path and remove-path assertions updated in lockstep ((id, undefined), (id, 50, undefined, undefined)).
• useGsapSelectionHandlers.test.tsx:119-146 — new selection override describe block. Two tests directly encode the fix's invariants: (a) explicit null override aborts the write (removeKeyframe not called), while omitted override still falls back to current selection; (b) handleGsapMoveKeyframeToPlayhead uses the passed-in animation, verified by driving the mock with selectedGsapAnimations: [] — the assertion moveKeyframe.toHaveBeenCalledWith(selection, "anim-1", 50, expect.any(Number)) can only pass if the override animation was consulted. Direct regression coverage for the mixed-frame class.

Byte-clean on prior-audited surface

19 unchanged PR files audited — 19 MATCH, 0 DIFFER. R3 verify carries forward untouched:

App.tsx, AnimationCard.tsx, GsapAddAnimationControl.tsx, GsapAnimationSection.tsx, PropertyPanelFlat.tsx, gsapAnimationCallbacks.test.ts, gsapAnimationCallbacks.ts, propertyPanelFlatMotionSection.tsx, useTimelineEditCallbacks.test.ts, TimelineEditContext.tsx, gsapKeyframeCacheHelpers.ts, useStudioContextValue.test.ts, useStudioContextValue.ts, TimelineClipDiamonds.tsx, TimelineLanes.tsx, TimelineOverlays.tsx, timelineCallbacks.ts, useExpandedTimelineElements.ts, timelineElementHelpers.ts.

Standards lens re-run

Empty count on 4 changed files: 0 findings.

• Named exports only — pass (all four files use export function / describe).
• No barrel files — N/A (no index.ts touched).
• import type for type-only imports — pass (import type { GsapAnimation } from "@hyperframes/core/gsap-parser", import type { DomEditSelection }).
• No hardcoded colors / tw-* / inline style — N/A (hooks, no JSX chrome).
• i18n — N/A (no user-facing strings introduced).
• No useEffect state-syncing — pass; new logic is inside useCallbacks / event handlers.
• File naming — pass (useX.ts, useX.test.tsx).
• Fast Refresh (react-refresh/only-export-components) — N/A (hook/test files, no component exports).

Peer state

Rames R3 (review 4791736131, COMMENTED, 2026-07-27T21:40:32Z) converged on the same onMoveKeyframeToPlayhead mixed-frame diagnosis — N-of-N=2 authority at R3. R4 fix commit message names all three sub-mechanisms (onMoveKeyframeToPlayhead / onChangeKeyframeEase / onToggleKeyframeAtPlayhead) plus the explicit-null abort, matching the R3 finding surface end-to-end.

— Review by Via

[james-russo-rames-d-jusso]

Reviewed at 9fc00117037444711bac6963d35ee35ebc268da9 (delta from R3 at a423c930).

R3 concerns closed cleanly. The new commit systematizes the fix rather than patching each callsite individually:

• useGsapSelectionHandlers.ts — every mutation handler now takes an optional selectionOverride?: DomEditSelection | null and routes through a new resolveWriteSelection helper (:126-132). Crucially, undefined means "no override, fall back to ambient" while explicit null means "caller RESOLVED to no selection — bail." That distinction is what makes the clicked-element-not-selected path safe: an outer callback that fails to build a selection returns null and the handler aborts instead of silently committing against domEditSelection. Nice invariant, and the added test at useGsapSelectionHandlers.test.tsx:120-131 pins it.
• onMoveKeyframeToPlayhead (useTimelineEditCallbacks.ts:222-236) — now resolves animations + element from the clicked elId, awaits buildDomSelectionForTimelineElement(element), and threads both the selection AND the resolved animation as 3rd/4th args to handleGsapMoveKeyframeToPlayhead. Downstream, the handler prefers animationOverride over selectedGsapAnimations.find(...) for the computeCurrentPercentage(sel, anim) call (useGsapSelectionHandlers.ts:355), so both the tween-relative playhead-% and the write commit to the same file. Pinning test at :141-149 uses selectedGsapAnimations: [] to prove the override path is what's exercised.
• onChangeKeyframeEase (:310-323) — reworked to use resolveElementAnimations(elId) + buildDomSelectionForTimelineElement(element) + selection-threaded handleGsapUpdateMeta(anim.id, { ease }, selection). Was iterating selectedGsapAnimations at R3.
• onToggleKeyframeAtPlayhead (:325-359) — same clicked-element-own-frame pattern; every mutator (handleGsapRemoveKeyframe, handleGsapAddKeyframe, handleGsapConvertToKeyframes) gets the resolved selection.

The resolveWriteSelection pattern is the right kind of consolidation — it's now uniform across every mutation handler in the file, and the two-layer coupling (outer callback resolves, downstream handler accepts) is systematized rather than callsite-by-callsite. Would be my recommended shape for future handlers on this surface.

LGTM from my side. Leaving as a comment — approval is Vance's.

— Review by Rames D Jusso

The base branch was changed.