feat(studio): consolidate timeline editor callbacks#2688
Conversation
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at da9f172254f0.
Editor-callback consolidation. R9 verdict: split_path_exists — ~40% of the diff has a genuine R9 story (mutation-boundary core), but ~60% (telemetry-wrapper DRY, Add-effect UI dedupe, shouldShowMotionPath prep for #2690) rides along under the same banner. One orange correctness item: onDeleteKeyframe's clicked-element path is missing an if (selection) guard, silently falling back to domEditSelection — the exact misdirect this PR claims to fix. #2689 patches it.
R9 exception verdict — split_path_exists
R9 'dual mutation authority' rationale only defends the timeline-mutation core (useTimelineEditCallbacks + TimelineEditContext memo-dep). Three of six clusters have zero mutation-authority interaction and could split cleanly: (1) withTrackedGsapAnimationCallbacks extraction in gsapAnimationCallbacks.ts is a pure telemetry-wrapper DRY refactor — swaps ~140 lines of inline arrow chains for a shared helper. Does not touch add/remove/retime/select mutation paths. (2) GsapAddAnimationControl.tsx extraction is a pure UI dedupe — two panels shared the same 'Add effect' menu inline; now share one component; no state authority. (3) shouldShowMotionPath added to useStudioContextValue.ts:73/99 is DEAD CODE at this SHA — App.tsx destructures only shouldShowSelectedDomBounds and does not forward shouldShowMotionPath; only the follow-up commit 0ac47c0 consumes it. Strongest indicator of gratuitous bundling — code lands here explicitly to prepare a different PR. The mutation-boundary core IS genuinely atomic; ~40% has a real R9 story. Other ~60% is bundling under the exception's banner.
🟢 Verified clean
- resolveTimelineKeyframeTarget correctly handles both new identified-animation path and legacy single-candidate group path — three new unit tests exercise collision, stale-identity, unresolved cases
- withTrackedGsapAnimationCallbacks preserves undefined optional callbacks (does not promote them to defined pass-throughs) and preserves identity for pass-through preview callbacks
- PropertyPanelFlat's stale-successor guard uses
${element.sourceFile}#${element.id}— the panel's rendered identity, matching propertyPanelFlatMotionSection.tsx:146-150 - onTogglePropertyGroupKeyframe (:307) DOES guard null-selection with
if (!selection) return;at :309 — unlike onDeleteKeyframe - No silent try/catch swallowing in the new callback owner or gsapAnimationCallbacks helpers
- TimelineEditContext memo dep list correctly includes onTogglePropertyGroupKeyframe (:47)
Cross-stack notes
This PR is B6, on top of #2687. Two follow-up commits are visible locally: 2b4a520 (#2689) and 0ac47c0 (#2690). #2689 patches the null-selection guard (finding 1), threads elementKey into resolveKeyframeTarget (finding 2), and routes onMoveKeyframe/onMoveKeyframeToPlayhead/onDeleteAllKeyframes through the clicked element (finding 3) — EVERY substantive correctness issue is on the pending fix list. Evidence that this PR was landed knowing the mutation-authority story was incomplete. #2690 consumes shouldShowMotionPath (finding 5), confirming that cluster belongs downstream. Recommend either (a) squash #2689's guards into this PR before merge, or (b) explicitly note in the body that stale-target fixes for the retime path are deferred to the immediately-following commit — the current body's 'edits to stale or incorrect targets' framing implies the fix is complete, which it is not at head da9f172.
| // Persist through the CLICKED element's own selection so a deletion on a | ||
| // non-selected element (especially one in a different source file) commits | ||
| // against the right element instead of the current domEditSelection. | ||
| void buildDomSelectionForTimelineElement(element).then((selection) => { |
There was a problem hiding this comment.
🟠 onDeleteKeyframe's clicked-element persist path is missing an if (selection) guard
void buildDomSelectionForTimelineElement(element).then((selection) => {
removeKeyframeTarget(target.animId, target.tweenPct, animations, selection);
});Calls removeKeyframeTarget with selection = null when resolution fails. Because handleGsapDeleteAnimation and handleGsapRemoveKeyframe both use selectionOverride ?? domEditSelection ?? lastSelectionRef.current (useGsapSelectionHandlers.ts:177,320), a null override silently falls back to the CURRENT domEditSelection — exactly the misdirect the PR body claims to have fixed ('commits against the right element instead of the current domEditSelection'). The new onTogglePropertyGroupKeyframe at :309 IS guarded with if (!selection) return;, so guarding is inconsistent within the same PR. Follow-up commit 2b4a520 (#2689) 'fix(studio): harden keyframe editing semantics' explicitly adds if (selection) here — confirms this is a bug at PR head.
Fix: Add if (!selection) return; before the removeKeyframeTarget call, matching the pattern on :309.
| const explicitTarget = | ||
| propertyGroup !== undefined || tweenPercentage !== undefined || animationId !== undefined | ||
| ? [{ percentage: pct, propertyGroup, tweenPercentage, animationId }] | ||
| : undefined; |
There was a problem hiding this comment.
🟡 resolveKeyframeTarget fallback still uses domEditSelection?.id for keyframe-cache lookup
const cached = usePlayerStore.getState().keyframeCache.get(domEditSelection?.id ?? "");Even though the caller (onDeleteKeyframe :200) already resolves animations from the explicit elId, the fallback path here reads keyframes for the currently-selected element — a within-call inconsistency. Callers currently always pass explicit target args (rendered keyframes carry animationId), so this is a latent bug rather than active; but the fallback path is dead defense that quietly misbehaves. Fix in #2689 threads elementKey into resolveKeyframeTarget and drops domEditSelection?.id.
Fix: Thread elementKey through resolveKeyframeTarget so the cache lookup uses the clicked element.
| // 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) => { |
There was a problem hiding this comment.
🟡 onMoveKeyframe + onMoveKeyframeToPlayhead discard passed _elId and use global selection
// onMoveKeyframe (:228): const sel = domEditSelection;
// onMoveKeyframeToPlayhead (:216): reads global selectedGsapAnimationsNeither builds a clicked-element selection via buildDomSelectionForTimelineElement. PR body claims cross-element mutation risk was fixed, but only onDeleteKeyframe and the new onTogglePropertyGroupKeyframe route through the clicked element — retime paths (drag-to-retime and playhead retime) still misdirect to the current selection. Follow-up #2689 threads element identity into both. PR title 'consolidate' and body claim 'passes explicit element/keyframe identity through callers' overstate what was done at this SHA.
Fix: Route both through buildDomSelectionForTimelineElement so retime paths use clicked-element identity.
| onFocusSegmentConsumed?.(); | ||
| }, [focusedSegment, onFocusSegmentConsumed]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🟡 Scroll-into-view effect queries a data attribute that no code writes
cardRef.current?.querySelector<HTMLElement>(
`[data-ease-segment-pct="${expandedKfPct}"]`
)?.scrollIntoView(...)No code in the repo sets a data-ease-segment-pct attribute (git log --all -S 'data-ease-segment-pct' returns only this PR; KeyframeEaseList.tsx:87-124 renders segments without that attribute). segment is always null; scrollIntoView never fires. Card expands and segment opens, but auto-scroll half of the feature is dead code.
Fix: Either add the attribute to KeyframeEaseList's segment render (data-ease-segment-pct={segment.percentage}) or change the selector to match what's actually rendered.
| designPanelActive: boolean; | ||
| inspectorPanelActive: boolean; | ||
| inspectorButtonActive: boolean; | ||
| shouldShowMotionPath: boolean; |
There was a problem hiding this comment.
🟡 shouldShowMotionPath added to InspectorState but no consumer reads it at this SHA
// Added to InspectorState type (:73) + computed in useInspectorState (:99):
shouldShowMotionPath: !!domEditSelection && !isPlaying && !isGestureRecordinggit show da9f17225:packages/studio/src/App.tsx | grep shouldShowMotionPath returns only the destructure — value is discarded, never passed to EditorShell/PreviewOverlays. Follow-up commit 0ac47c0e feat(studio): wire expanded keyframe timeline lanes is what threads it through. InspectorState change (plus its 93-line useStudioContextValue.test.ts) is orthogonal preparation for a downstream PR; should not live under an R9 'consolidate timeline callbacks' exception.
Fix: Move shouldShowMotionPath addition into #2690 where it's actually consumed, or note in this PR body that it's a pre-wire for the next slice.
| trackAnimationMetaUpdate(track, updates); | ||
| onUpdateMeta(animationId, updates); | ||
| }; | ||
| const trackedCallbacks = withTrackedGsapAnimationCallbacks(callbacks, track); |
There was a problem hiding this comment.
🟢 withTrackedGsapAnimationCallbacks recomputed every render — defeats AnimationCard memoization
const trackedCallbacks = withTrackedGsapAnimationCallbacks(callbacks, track);
// ...
<AnimationCard {...trackedCallbacks} />Each call returns a fresh object of fresh arrow functions; spreading hands memo(AnimationCard) new callback identities every parent render. Pre-PR inline-arrow pattern had the same issue (not a regression), but 'consolidation behind one mutation boundary' misses the free memoization win.
Fix: Wrap withTrackedGsapAnimationCallbacks(callbacks, track) in useMemo with [callbacks, track] deps.
| const hashIndex = elementKey.lastIndexOf("#"); | ||
| const elementId = hashIndex === -1 ? elementKey : elementKey.slice(hashIndex + 1); | ||
| const sourceFile = | ||
| hashIndex === -1 ? (activeCompPath ?? "index.html") : elementKey.slice(0, hashIndex); |
There was a problem hiding this comment.
🟢 resolveElementAnimations hardcodes 'index.html' twice as default composition path
sourceFile = hashIndex === -1 ? (activeCompPath ?? "index.html") : ...
// and:
gsapAnimations.get(`index.html#${elementId}`)For any project whose active composition is not index.html AND whose elementKey lacks a # prefix, the second fallback becomes stale. Not a regression, but the ad-hoc fallback chain (three .get() calls in one return) belongs in a helper with tests. Same magic string in onMoveKeyframe :240.
Fix: Extract a resolveGsapAnimationsForElement(gsapAnimations, elementKey, activeCompPath) helper with explicit tests.
| const cardRef = useRef<HTMLDivElement>(null); | ||
| const pendingAutoScrollRef = useRef(false); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🟢 onFocusSegmentConsumed inline-arrow deps make scroll effect fire every render
// deps: [focusedSegment, onFocusSegmentConsumed]
// but consumers pass:
() => setFocusedEaseSegment(null) // inline every renderonFocusSegmentConsumed passed as inline arrow from both consumers (GsapAnimationSection.tsx:60 and propertyPanelFlatMotionSection.tsx:185), so identity changes every parent render. Effect at :60 lists it in deps, so effect fires every render — the !focusedSegment guard short-circuits after the first consumption, but dependency shape is misleading.
Fix: Extract with useCallback in each consumer to make the intent honest.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at da9f172254f0.
Callback consolidation (R9-flagged cohesive). Rames's R1 assigned split_path_exists — ~40% is a genuine R9 mutation-boundary story (useTimelineEditCallbacks + TimelineEditContext memo dep), ~60% (telemetry-wrapper DRY, GsapAddAnimationControl UI dedupe, shouldShowMotionPath prep for #2690) rides along under the banner. He also caught the missed onDeleteKeyframe null-selection guard (patched by #2689). I concur on the R9 verdict and did the missed-caller audit Miguel called out.
Prior review
Rames D Jusso R1: split_path_exists verdict, onDeleteKeyframe missing if (selection) guard (patched in #2689), resolveTimelineKeyframeTarget correctly handles new-identity vs legacy paths, withTrackedGsapAnimationCallbacks preserves undefined optional callbacks. Overlap: I confirm the R9 split rationale (particularly that shouldShowMotionPath is dead code at this SHA and only consumed by #2690's PreviewOverlays.tsx).
🟢 Missed-caller audit (Miguel-flagged R9)
For every callback signature that widened in this PR, I grepped for stale call sites:
onDeleteKeyframe(elId, pct)→(elId, pct, group?, tweenPct?, animationId?)— new params are all optional; existing 2-arg callers keep working (fall back toresolveKeyframeTarget(pct)). Rames anchored the missed disambiguation call in #2685'sTimelineOverlays.tsx:109— same wiring gap surfaces the identity args touseTimelineEditCallbacksbut drops them at the menu handler. Cross-references verified.onMoveKeyframeToPlayhead(elId, pct)→ same optional widening; same behavior.onMoveKeyframe(elId, fromClipPct, toClipPct)→ addsgroup?,tweenPct?,animationId?; #2690'sTimelineLanes.tsxpasses all three from the property-lane path. Compact-row path uses 3-arg call (fine — no lane ambiguity there).handleGsapDeleteAnimation(animId)→(animId, selectionOverride?)— new optional arg; every existing 1-arg caller in the diff-touched files still compiles.- Export removal audit:
trackAnimationMetaUpdatewasexport functionat base and becomes internal-only here. Repo grep at #2688 head returned three files (gsapAnimationCallbacks.tswhere it lives,GsapAnimationSection.tsx,propertyPanelFlatMotionSection.tsx). Both external callers had their imports removed in this same PR — no stale caller left behind. Consolidation is complete.
No live orphan caller.
🟢 Verified
resolveTimelineKeyframeTarget's new identity path — ifkf.animationIdis set, it takes precedence over the property-group heuristic; if the id doesn't resolve to a candidate, returnsnull(safe) instead of picking arbitrarily. Three unit tests cover collision, stale-identity, and unresolved cases.PropertyPanelFlat's stale-successor guard uses${element.sourceFile}#${element.id}— matchespropertyPanelFlatMotionSection.tsxrender identity. Rames confirmed.TimelineEditContextmemo dep list correctly includesonTogglePropertyGroupKeyframe— no dependency omission.useStudioContextValue.ts'sshouldShowMotionPathis added toInspectorStateand theuseMemoreturn but no consumer in this SHA — Rames anchored this as evidence of ridealong bundling. Confirmed: only #2690'sPreviewOverlays.tsxconsumes it.withTrackedGsapAnimationCallbackspreserves identity of pass-through preview callbacks (onLivePreview,onLivePreviewEnd) — no unnecessary wrapping.
Cross-stack note
Every substantive correctness gap Rames flagged (null-selection guard, elementKey threading, clicked-element vs domEditSelection for onMoveKeyframe/onMoveKeyframeToPlayhead/onDeleteAllKeyframes) is patched by the immediately-following #2689. On a squash-merge stack this is fine; the honest wording would be to note in the PR body that "stale target fix" completes at #2689.
Verdict
Approving. R9 rationale is partial-honest (Rames's split_path_exists), and the missed-caller audit turned up no live orphan. Correctness gaps identified here are all repaired by #2689 in the same series.
— Review by Via
3b9d73a to
f5a2098
Compare
da9f172 to
69da4b0
Compare
f5a2098 to
7f86018
Compare
69da4b0 to
dd77b83
Compare
The base branch was changed.
3 participants
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
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 lookuppackages/studio/src/components/editor/GsapAnimationSection.tsx:30— withTrackedGsapAnimationCallbacks recomputed every render — defeats AnimationCard memoizationpackages/studio/src/components/nle/useTimelineEditCallbacks.ts:123— resolveElementAnimations hardcodes 'index.html' twice as default composition pathpackages/studio/src/components/editor/AnimationCard.tsx:60— onFocusSegmentConsumed inline-arrow deps make scroll effect fire every render