Skip to content

feat(studio): consolidate timeline editor callbacks#2786

Open
miguel-heygen wants to merge 4 commits into
codex/studio-timeline-b-track-headers-v2from
codex/studio-timeline-b-editor-callbacks-v2
Open

feat(studio): consolidate timeline editor callbacks#2786
miguel-heygen wants to merge 4 commits into
codex/studio-timeline-b-track-headers-v2from
codex/studio-timeline-b-editor-callbacks-v2

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

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,865 Studio tests, 67 Studio Server route 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 commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (onDeleteKeyframebuildDomSelectionForTimelineElement(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 james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

onMoveKeyframeToPlayhead: (_elId: string, pct: number) => {
const target = resolveKeyframeTarget(pct);
onMoveKeyframeToPlayhead: (_elId, pct, group, tweenPct, animationId) => {
const target = resolveKeyframeTarget(pct, group, tweenPct, animationId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 onDeleteKeyframe missing null-selection guard — silent-fallback deletes against the wrong element

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

buildDomSelectionForTimelineElement is typed Promise<DomEditSelection | null> (useDomSelection.ts:94-96). When it resolves null, removeKeyframeTarget forwards null → handleGsapDeleteAnimation(id, null) or handleGsapRemoveKeyframe(id, pct, undefined, null). Both do selectionOverride ?? domEditSelection ?? lastSelectionRef.current (useGsapSelectionHandlers.ts:177,318), so explicit null falls through to the CURRENTLY-SELECTED element's DOM selection — precisely the authority the comment on lines 216-218 claims to prevent. onDeleteAllKeyframes in the same PR block (line 203) does guard with if (selection); onDeleteKeyframe does not. New tests only mockResolvedValue(mocks.selection); no null-resolution assertion.

Fix: Guard with if (!selection) return; before invoking removeKeyframeTarget in the then-callback, mirroring onDeleteAllKeyframes at line 203-205.

Review by Rames D Jusso

target.properties,
undefined,
selection,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 onTogglePropertyGroupKeyframe looks up flat-tween identity in selectedGsapAnimations instead of the passed element's animations

if (target.remove) {
  removeKeyframeTarget(
    target.animationId,
    target.tweenPercentage,
    selectedGsapAnimations,
    selection,
  );
  return;
}

removeKeyframeTarget uses the passed animations array to decide flat-vs-keyframed: animations.find(candidate => candidate.id === animationId); if (animation && !animation.keyframes) handleGsapDeleteAnimation(...). When onTogglePropertyGroupKeyframe fires for a non-selected element (TimelineTrackHeader.tsx:444 invokes it on expandedElement, not the currently-selected one), target.animationId is unlikely to appear in selectedGsapAnimations — so animation is undefined, delete-animation branch is skipped, and it falls to handleGsapRemoveKeyframe for what is actually a flat tween. Strands the flat animation instead of removing it — the very footgun the flat-boundary comment at line 261-263 protects onMoveKeyframe against.

Fix: Pass resolveElementAnimations(getTimelineElementIdentity(element)) instead of selectedGsapAnimations, matching the pattern established by onDeleteKeyframe on line 208.

Review by Rames D Jusso

// Retime the keyframe to the playhead, preserving its value + ease.
onMoveKeyframeToPlayhead: (_elId: string, pct: number) => {
const target = resolveKeyframeTarget(pct);
onMoveKeyframeToPlayhead: (_elId, pct, group, tweenPct, animationId) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 onMoveKeyframeToPlayhead silent-fallback to domEditSelection (delete-authority fix not carried through)

onMoveKeyframeToPlayhead: (_elId, pct, group, tweenPct, animationId) => {
  const target = resolveKeyframeTarget(pct, group, tweenPct, animationId);
  if (target) handleGsapMoveKeyframeToPlayhead(target.animId, target.tweenPct);
},

First arg is _elId — explicitly discarded. handleGsapMoveKeyframeToPlayhead does selectionOverride ?? domEditSelection ?? lastSelectionRef.current (useGsapSelectionHandlers.ts:330). Retime-to-playhead on a diamond belonging to a non-selected element (or one in a different source file) commits against the current domEditSelection — the exact authority bug onDeleteKeyframe fixes in this same PR. Inconsistent authority within one consolidation.

Fix: Route through buildDomSelectionForTimelineElement(element) like onDeleteKeyframe does; take the TimelineElement as the first arg or find it in usePlayerStore.getState().elements from elId.

Review by Rames D Jusso

onMoveKeyframe: async (_elId: string, fromClipPct: number, toClipPct: number) => {
const target = resolveKeyframeTarget(fromClipPct);
onMoveKeyframe: async (_elId, fromClipPct, toClipPct, group, tweenPct, animationId) => {
const target = resolveKeyframeTarget(fromClipPct, group, tweenPct, animationId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 onMoveKeyframe silent-fallback to domEditSelection

onMoveKeyframe: async (_elId, fromClipPct, toClipPct, group, tweenPct, animationId) => {
  const target = resolveKeyframeTarget(fromClipPct, group, tweenPct, animationId);
  const sel = domEditSelection;

Drag-retime uses const sel = domEditSelection — same authority-bug class as previous finding. handleGsapMoveKeyframe accepts selectionOverride (useGsapSelectionHandlers.ts:342-346) and would honor a per-element override, but this call site never supplies one. A drag on a non-selected diamond routes through the wrong element's DOM. Tests either exercise selected element or hit flat-tween early-return.

Fix: Take TimelineElement (or look up via elId), await buildDomSelectionForTimelineElement, pass resolved selection as 4th arg of handleGsapMoveKeyframe (and handleGsapResizeKeyframedTween / handleGsapUpdateMeta which also accept overrides).

Review by Rames D Jusso

return resolveTimelineKeyframeTarget(pct, cached?.keyframes ?? [], selectedGsapAnimations);
return resolveTimelineKeyframeTarget(
pct,
explicitTarget ?? cached?.keyframes ?? [],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 resolveKeyframeTarget cache key still hardcoded to domEditSelection?.id

const cached = usePlayerStore.getState().keyframeCache.get(domEditSelection?.id ?? "");
return resolveTimelineKeyframeTarget(
  pct,
  explicitTarget ?? cached?.keyframes ?? [],
  animations,
);

Fallback branch when caller omits propertyGroup/tweenPercentage/animationId reads the SELECTED element's keyframe cache — not the diamond's element. Inconsistent with the new resolveElementAnimations (line 118) which correctly uses the passed elementKey. For a diamond click on a non-selected element arriving without explicit-target fields (allowed by the optional signature in timelineCallbacks.ts:74-80), resolution consults the wrong cache. Current callers (KeyframeDiamondContextMenu.tsx:89) do pass all fields, so bug isn't triggered today — latent authority leak.

Fix: Thread the elementKey into resolveKeyframeTarget (as with resolveElementAnimations) and read keyframeCache.get(elementKey) ?? keyframeCache.get(elementId).

Review by Rames D Jusso

}
: undefined
focusedSegment={
focusedEaseSegment?.animationId === anim.id ? focusedEaseSegment : null

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 GsapAnimationSection focus-segment gate lacks the element-id scoping the flat panel has

focusedEaseSegment?.animationId === anim.id ? focusedEaseSegment : null

propertyPanelFlatMotionSection.tsx:148 filters by BOTH animationId AND elementId (focusedEaseSegment.elementId === renderedElementId), with an explicit comment that a shared class-selector animation id could otherwise open the wrong element's editor. Classic GsapAnimationSection performs no element-id filter, so if two panels ever share an animation id (class-selector authoring) the classic path can auto-open the wrong card.

Fix: Add elementId filter to GsapAnimationSection matching the flat panel — accept element identity and gate focusedEaseSegment.elementId === renderedElementId before matching animationId.

Review by Rames D Jusso

? animations.find((animation) => animation.id === kf.animationId)
: undefined;
if (kf.animationId) {
return identifiedAnimation

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 resolveTimelineKeyframeTarget synthesized-target falls back tweenPct to clip-% when caller omits tweenPercentage

if (kf.animationId) {
  return identifiedAnimation
    ? { animId: identifiedAnimation.id, tweenPct: kf.tweenPercentage ?? pct }
    : null;
}

When resolveKeyframeTarget is called with explicit animationId but tweenPercentage === undefined (allowed by the optional-arg signature), synthesized explicitTarget has tweenPercentage undefined, and this line collapses tweenPct to clip-%. Flat tweens don't sit at 0-100 clip-% (they occupy a sub-range), so downstream handleGsapRemoveKeyframe/handleGsapMoveKeyframeToPlayhead receive wrong tween percentage. All current diamond callers (KeyframeDiamondContextMenu.tsx:89-93) do pass state.tweenPercentage, so not triggered today — type-level footgun asymmetric with R9's locked-down contract claim.

Fix: Require tweenPercentage when animationId is provided (in the callback signature or by rejecting the target when tweenPercentage is undefined in the identified-animation branch).

Review by Rames D Jusso


useEffect(() => {
if (!focusedSegment) return;
setExpanded(true);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 AnimationCard focus-segment auto-scroll races the ease editor's mount

useEffect(() => {
  if (!pendingAutoScrollRef.current || expandedKfPct === null) return;
  const segment = cardRef.current?.querySelector<HTMLElement>(
    `[data-ease-segment-pct="${expandedKfPct}"]`,
  );
  segment?.scrollIntoView({ block: "nearest", behavior: "smooth" });
  pendingAutoScrollRef.current = false;
}, [expandedKfPct]);

First effect sets both expanded=true and expandedKfPct. When focus arrives while card is collapsed, the [data-ease-segment-pct] DOM node only exists after ease editor renders — needs both flags to have committed AND child mounts to have run. This second effect fires on the expandedKfPct change; if ease editor renders in same commit, querySelector returns null and scrollIntoView silently no-ops, leaving pendingAutoScrollRef=false so no retry happens. Tests don't cover scroll path. onFocusSegmentConsumed is also a new function reference each parent render (inline arrow at GsapAnimationSection.tsx:60), so first effect re-runs on every parent re-render — guarded by if (!focusedSegment) return, so wasteful but not incorrect.

Fix: Defer scroll to a layoutEffect gated on the segment's mount (data-ease-segment-pct present) or use requestAnimationFrame + ref lookup with one retry. Memoize onFocusSegmentConsumed at the parent to avoid effect re-runs.

Review by Rames D Jusso

STUDIO_INSPECTOR_PANELS_ENABLED && !rightCollapsed && inspectorPanelActive,
// Keep the selection box + motion path drawn even when the Inspector is
// collapsed — closing the panel shouldn't visually deselect the element.
shouldShowMotionPath: !!domEditSelection && !isPlaying && !isGestureRecording,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 shouldShowMotionPath decoupling is a behavior change unrelated to 'consolidated timeline editor callbacks'

shouldShowMotionPath: !!domEditSelection && !isPlaying && !isGestureRecording,

Pre-PR the motion path was drawn under the same condition as shouldShowSelectedDomBounds (Design/Variables tab open with a selection). Post-PR, motion path draws for ANY selection regardless of inspector tab. Grouped under a PR titled 'consolidated timeline editor callbacks', this is an orthogonal visibility change and a canonical R9 split candidate — removing this cluster leaves remaining PR compiling and behavior-preserving; consumers (App.tsx:397, EditorShell.tsx:94, PreviewOverlays.tsx:279) form a self-contained thread.

Fix: Extract shouldShowMotionPath work into its own PR with focused description of the intended visibility contract; leave this PR to the callback consolidation it advertises.

Review by Rames D Jusso

@miguel-heygen
miguel-heygen force-pushed the codex/studio-timeline-b-editor-callbacks-v2 branch from 041ead6 to bcc22d6 Compare July 25, 2026 21:17
Three review follow-ups on the editor-callback consolidation.

The keyframe-target resolve now takes the clicked element's key and reads
that element's keyframe cache. The diamond context menu and move-to-playhead
pass no explicit target, so they fell through to the cache of whatever
element happened to be selected: opening the menu on a non-selected
element's diamond resolved against the wrong keyframes.

PropertyPanelFlat opens the Motion group by adjusting state during render
instead of in an effect, so the AnimationCard mounts on the same commit the
focus request arrives on rather than a frame later.

Both animation sections pass a module-level focus consumer instead of a
fresh inline arrow, so AnimationCard's focus effect stops re-running on
every parent render.
The lane-header keyframe toggle fires on whichever element owns the lane,
which need not be the selected one. The remove path looked the animation up
in the selected element's animations, so a non-selected element's flat tween
missed and silently took the remove-one-keyframe branch, stranding the tween
instead of deleting it.
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 non-selected clip retimed the wrong tween. It now
resolves against the clicked element's animations and commits through that
element's selection, matching the delete path.

The three diamond callbacks also take the TimelineKeyframeTarget they already
had instead of five positional fields, and the two copies of the
sourceFile#domId split share splitTimelineElementKey.
@miguel-heygen
miguel-heygen force-pushed the codex/studio-timeline-b-track-headers-v2 branch from 0b9eff8 to cf1cc8c Compare July 25, 2026 23:51
@miguel-heygen
miguel-heygen force-pushed the codex/studio-timeline-b-editor-callbacks-v2 branch from 89eeddb to 783fa4a Compare July 25, 2026 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants