fix(studio): harden keyframe editing semantics#2689
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 2b4a520fbf3f.
Correctness bundle. R9 verdict: split_path_exists — the keyframe-core fixes (~1500 lines) are genuinely interdependent, but 3 files (propertyPanelFlatTextSection, useStudioTestHooks typing refactor, e2e fixture) are unrelated to keyframe editing and could split. Two orange items on the fix set itself: arc-drag temporal-keyframe path still uses the old un-scoped resolveTweenDuration while the rest of the PR standardizes on resolveEditableTweenDuration (mixed-basis stranded), and pointer-capture 'closest-interactive-control' bailout ships without regression coverage.
R9 exception verdict — split_path_exists
Core keyframe-editing fixes (identity/selection, retime settlement, bulk delete, motion-path temporal keyframes, toolbar parity, easeEach server-side, useTimelineEditCallbacks element identity) are genuinely interdependent — Promise return type ripples through 5 files, identity model requires simultaneous updates in delete/context-menu/callbacks, motion-path drag temporal-keyframe requires paired server easeEach preservation. ~1500 lines pass 'fixes remain with their proof'. However 3 files in the delta are unrelated to keyframe editing and cleanly separable: (1) propertyPanelFlatTextSection.tsx (+28/-3) + test — text-field auto-focus fix so FlatTextSection doesn't steal canvas focus on element selection; (2) useStudioTestHooks.ts (+13/-3) — pure declare global typing refactor, zero runtime effect; (3) tests/e2e/fixtures/design-panel-qa/index.html (1-line selector fix). Removing them would drop ~120 lines without weakening the coherent argument. R9 is honest for keyframe core but has ridealong scope-creep.
Bug-test coverage audit
- ✅ stale selection: timelineKeyframeIdentity.test.ts + useTimelineEditCallbacks.test.tsx
- ✅ clip-relative vs global time: globalTimeCompiler.test.ts + gsapShared.test.ts + useTimelineEditCallbacks.test.tsx
- ✅ keyboard/button parity: TimelineToolbar.test.tsx + TimelinePropertyLanes.test.tsx
⚠️ pointer capture: Only ease-segment fix covered (TimelineClipDiamonds.test.tsx); Timeline.tsx onPointerDown 'closest-interactive-control' bailout at :477 has NO regression test- ✅ bulk deletion: timelineEditingHelpers.test.ts — but see F4
- ✅ failed retimes: useGsapKeyframeOps.test.tsx + useGsapSelectionHandlers.test.tsx + useGsapScriptCommits.test.tsx + useTimelineEditCallbacks.test.tsx
- ✅ moving clips with keyframes: useTimelineEditCallbacks.test.tsx + gsapRuntimeBridge.test.ts
Findings not anchored inline
🟠 'closest-interactive-control' pointer-capture bailout claimed as a bug fix but ships with zero regression coverage
packages/studio/src/player/components/Timeline.tsx:477
// Let interactive controls (keyframe nav/toggle, caret, inputs) handle
// their own clicks — scrubbing here would preventDefault and eat them.
if (e.target instanceof Element && e.target.closest("button, input, select, a")) return;Searched every Timeline*.test.* in packages/studio — no test dispatches a pointerdown whose target is a nested button/input to assert the scrub path is skipped. If a future refactor removes this line (or narrows the selector), no CI signal fires. PR body lists 'pointer capture' as one of seven correctness bugs; the other pointer fix (pointerEvents: 'none' on the ease segment) is covered by TimelineClipDiamonds.test.tsx but this branch is not.
Fix: Add a Timeline.test.ts case that fires pointerdown on a nested button and asserts scrub isn't initiated.
🟡 Two files bundled here are unrelated to keyframe editing — clean split candidates that weaken R9
packages/studio/src/components/editor/propertyPanelFlatTextSection.tsx:1
// propertyPanelFlatTextSection.tsx (+28/-3):
// adds autoFocusFieldKey state — 'does not steal canvas focus when a multi-field element is selected'
// useStudioTestHooks.ts (+13/-3):
// pure `declare global { interface Window { __studioTest?: StudioTestApi; } }` refactorpropertyPanelFlatTextSection: new test 'does not steal canvas focus when a multi-field element is selected' explicitly asserts focus behavior that has nothing to do with keyframe editing, timeline, or the seven listed bugs. useStudioTestHooks: pure typing refactor, zero runtime effect. Both land under a commit titled 'harden keyframe editing semantics' and are counted in the 1,722-line R9 exception, but neither is defended by that framing.
Fix: Either lift them out into their own follow-up PR, or update the PR body to acknowledge them as ridealong non-keyframe fixes.
🟢 parkPlayheadOnKeyframe still uses un-scoped resolveTweenDuration while surrounding drag branch uses selection-scoped duration
packages/studio/src/hooks/gsapDragCommit.ts:59
export function parkPlayheadOnKeyframe(anim: GsapAnimation, pct: number): void {
const ts = resolveTweenStart(anim);
const td = resolveTweenDuration(anim);
if (ts == null || !td || td <= 0) return;
usePlayerStore.getState().requestSeek(roundTo3(ts + (pct / 100) * td));
}Called from gsapDragPositionCommit.ts:296 (arc waypoint branch, new code in this PR). For a duration-less animation the drag's percentage is computed against clip duration but the playhead is parked against 0.5s — the two never agree. Pre-existing, but now more visible because the PR introduced the mixed-basis pattern within the same function.
Fix: Thread the selection through parkPlayheadOnKeyframe and use resolveEditableTweenDuration.
🟢 Verified clean
- deleteSelectedKeyframes.ts Map-based dedupe uses
${animation.id}\0${percentage}— unique (anim, pct) pairs guaranteed;!selectedElementIdearly bail replaces the deleted null-guard cleanly - timelineKeyframeIdentity.ts JSON-array encoding unambiguous vs legacy
elementId:group[:animId]:percentage(which could collide when elementId contains ':'); fallback branch preserves backward compat for collapsed keys - keyframeSelection.ts deletion — confirmed no remaining importers in packages/; sole consumer migrated to timelineKeyframeTargetFromSelectionKey
- useGsapKeyframeOps.ts moveKeyframe/resizeKeyframedTween Promise return correctly forwards
result.changed !== false; catch branch always returns false + routes to trackGsapSaveFailure; three settlement outcomes covered by test - files.ts easeEach threading correctly derives outer envelope's easeEach from parsed original when client sends
ease: 'none'— preserves per-segment easing across conversion; test coverage 'replace-with-keyframes preserves per-segment easing for exact temporal keyframes' - useGsapSelectionHandlers.ts handleGsapMoveKeyframeToPlayhead animationOverride param correctly threads CLICKED element's anim snapshot
- TimelineClipDiamonds.tsx replaces useState with CSS group-hover/focus-visible — net reduction in state updates on hover; ease button is now tabbable, satisfying 'accessible ease controls mounted'
Cross-stack notes
R1 from Family A #2674 flagged that Hold ease renders draggable handles in the ease editor — this PR touches ease-related code only at TimelineClipDiamonds.tsx (mid-segment ease button opacity/mount lifecycle), does NOT touch the ease editor. R1 concern does not land here. B7 designation is honest: sits on B6 (#2688 editor-callbacks-v2) whose TimelineEditCallbacks typed surface is what forced the Promise ripple across five files — those files could not have been split without breaking B6's compile in an intermediate state. HF flicker lens: arc-drag temporal-keyframe branch commits softReload: true — mirrors existing waypoint-edit behavior, no re-render cascade added. Combined with #2688 R9 verdict: #2689 IS what completes #2688's stated stale-target fix — but the story would be cleaner if the two shipped together.
| return; | ||
| } | ||
|
|
||
| const tweenStart = resolveTweenStart(anim); |
There was a problem hiding this comment.
🟠 Arc-drag temporal-keyframe path uses resolveTweenDuration (0.5s fallback) — stranded from PR's mixed-basis fix
const tweenStart = resolveTweenStart(anim);
const tweenDuration = resolveTweenDuration(anim);
if (tweenStart === null || tweenDuration <= 0 || keyframes.length < 2) return;
const temporalKeyframes = buildTemporalArcKeyframes(anim, pct, { x: newX, y: newY });
...
position: roundTo3(tweenStart),
duration: roundTo3(tweenDuration),Contrast with applyArcKeyframeAtPlayhead in useEnableKeyframes.ts (added in this PR): const duration = resolveEditableTweenDuration(arcAnim, sel);. And computeElementPercentage (updated in this PR) — called two lines above via computeCurrentPercentage(selection, anim) — routes through resolveEditableTweenDuration(animation, selection). Result: for a duration-less arc anim inside a clip with data-duration=16, pct is computed against 16s (correct) but mutation's duration is written as 0.5s (wrong) — truncates the tween window. gsapRuntimeBridge.test.ts fixture hardcodes duration: 16.055 so this asymmetry is not exercised. Since the whole rest of the PR standardizes on resolveEditableTweenDuration, this call site is stranded.
Fix: Switch to resolveEditableTweenDuration(anim, selection) to match the rest of the fix.
| expect(options[1]).not.toHaveProperty("softReload"); | ||
| expect(options[2]).not.toHaveProperty("skipReload"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟠 Bulk-delete regression coverage narrowed vs deleted keyframeSelection.test.ts
// Deleted test asserted the specific bug:
// 'drops keyframes that belong to other elements'
// New suite only has:
// - 'coalesces all removals and reloads only after the last one' (collapsed keys, single element)
// - 'deletes two expanded lanes through their own animation and tween percentages' (expanded keys, all matching element)Deleted test asserted: bulk delete on comp#a with a stale comp#b selection must not delete anything from comp#a. Neither new test asserts that a mixed Set([keyForA, keyForB]) on active element A produces exactly one call (dropping B). Cross-element behavior is now only tested at the identity primitive (timelineKeyframeIdentity.test.ts returns null for wrong element), which proves the primitive but not that deleteSelectedKeyframes composes it correctly — exactly the kind of composition bug the deleted test was designed to catch.
Fix: Port the deleted 'drops keyframes that belong to other elements' assertion into timelineEditingHelpers.test.ts.
| const backfillDefaults: Record<string, number> = { x: baseGsapX, y: baseGsapY }; | ||
| const ct = usePlayerStore.getState().currentTime; | ||
| if (anim.arcPath?.enabled) { | ||
| const { activeKeyframePct, setActiveKeyframePct } = usePlayerStore.getState(); |
There was a problem hiding this comment.
🟡 activeKeyframePct not cleared when arc drag falls through to temporal-keyframe branch
if (pointIndex >= 0) {
await callbacks.commitMutation(...);
setActiveKeyframePct(null); // cleared
parkPlayheadOnKeyframe(anim, pct);
return;
}
...
const temporalKeyframes = buildTemporalArcKeyframes(anim, pct, { x: newX, y: newY });
await callbacks.commitMutation(...);
return; // activeKeyframePct still setOther keyframed-tween drag helpers in this file (:100, :232, :294, :438) uniformly clear activeKeyframePct after using it. If user selects waypoint at pct 50 but drags to a spot whose computed pct is 50.06 (outside 0.05% tolerance), pointIndex check misses, temporal keyframe committed, activeKeyframePct=50 lingers into next drag — reused as pct = activeKeyframePct. Soft reload usually refreshes state, but invariant asymmetry is new in this PR.
Fix: Add setActiveKeyframePct(null); before the temporal-branch return.
| act(() => root.unmount()); | ||
| }); | ||
| }); | ||
| describe("TimelineToolbar — motion path endpoints", () => { |
There was a problem hiding this comment.
🟢 TimelineToolbar keyboard/button parity: only the 'endpoint disabled' path asserted; 4 new motion-path label branches untested
const button = host.querySelector<HTMLButtonElement>(
'button[aria-label="Motion path endpoint"]',
);
expect(button?.disabled).toBe(true);The diff introduces five distinct tooltip/aria-label branches ('Motion path endpoints cannot be removed', 'Extend motion path to playhead (K)', 'Remove waypoint from motion path (K)', 'Add waypoint to motion path (K)', plus the pre-existing non-motion-path branches). The only new test asserts the endpoint case. If a future refactor swaps the arcAnimation-vs-keyframedAnimation resolution order, the wrong label ships silently.
Fix: Add render assertions for each of the four new label branches.
|
|
||
| type GsapMutationResult = string | { script: string; skippedSelectors: string[] }; | ||
|
|
||
| function resolveReplacementEaseEach( |
There was a problem hiding this comment.
🟢 Server-side resolveReplacementEaseEach re-parses the entire script on every request that omits easeEach
function resolveReplacementEaseEach(
scriptText: string,
request: { animationId: string; easeEach?: string },
): string | undefined {
if (request.easeEach !== undefined) return request.easeEach;
const original = parseGsapScriptAcorn(scriptText).animations.find(
(animation) => animation.id === request.animationId,
);
...
}Called from both executeGsapMutationAcorn (:1520) and executeGsapMutationRecast (:1891). During a bulk keyframe edit sequence over an arc with N stops, each request re-parses the entire GSAP script AST just to look up source easeEach. Correctness fine but parse cost scales with N. The new client callers (useEnableKeyframes/useGsapKeyframeOps) send ease: 'none' only — do NOT send easeEach — so this fallback path fires for every real motion-path temporal-keyframe write.
Fix: Client-side: derive easeEach from the local anim record before the fetch, or pass through gsapAnimations lookup. Or server-side: cache the parse across a single request batch.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 2b4a520fbf3f.
Correctness bundle (R9-flagged). Rames's R1 landed split_path_exists — ~1500 lines of keyframe-editor correctness are genuinely coupled, but 3 files (text-field auto-focus, useStudioTestHooks typing, e2e fixture) split cleanly. He also flagged parkPlayheadOnKeyframe still using un-scoped resolveTweenDuration while surrounding code standardizes on resolveEditableTweenDuration, plus a coverage gap on the pointer-capture bailout. My additive read verified the pre-existing bugs each fix targets — that was the discipline the task asked for.
Prior review
Rames D Jusso R1: split_path_exists R9 verdict, bug-test coverage matrix (all ✅ except pointer-capture parkPlayheadOnKeyframe mixed-basis, 3-file ridealong split. Overlap: I confirmed the pre-existing bug for the arc-waypoint→temporal-keyframe rewrite and the resolveEditableTweenDuration fix.
🟢 Pre-existing bug verification (per Miguel-flagged R9 for #2689)
For each claimed correctness fix I traced the pre-fix path at the base ref (da9f172254f0, the #2688 head):
- Arc-waypoint → temporal-keyframe (
applyArcWaypointAtPlayhead→applyArcKeyframeAtPlayhead). Pre-fix path insertedtype: "add-motion-path-point"at a spatial location. GSAP MotionPath distributes progress evenly across N+1 segments post-insertion, so adding a waypoint at t=1.0s on a 2s tween with two authored waypoints (0%, 100%) makes it a 3-node arc whose old-t=1.0s waypoint is now at 50% progress — silent time compression. Toolbar's "Add keyframe at playhead" implies temporal preservation. New logic converts to temporal x/y keyframes (type: "replace-with-keyframes") with the drop position preservingduration+position. Bug is real; fix is sound. resolveEditableTweenDuration. Pre-fixresolveTweenDuration(animation)returned GSAP's 0.5s default for duration-less tweens; the timeline renders such tweens across the clip duration, so mutations targeted a phantom 0.5s window while the UI showed the clip's actual span. Fix threadsselection.dataAttributes?.durationin as the fallback. Bug is real.computeElementPercentage. Old formula:Math.max(0, Math.min(100, Math.round(((currentTime - elStart) / elDuration) * 1000) / 10)). New:absoluteToPercentage(currentTime, elStart, elDuration). The old clamp was silently wrapping — playhead past the clip returned 100%, playhead before returned 0%. The new unclamped path returns real percentages, letting callers see out-of-range and route throughisTimeWithinTween/ extend-duration logic instead of being lied to. Consistent with the rest of this PR's move toward "let callers see truth." Fine; downstream callers do handle out-of-range.- Bulk keyframe deletion.
deleteSelectedKeyframes.tsMap-based dedupe on${animation.id}\0${percentage}is unique per (anim, pct) — a duplicate selection key can't cause double-delete. Verified. - Selection identity encoding.
timelineKeyframeIdentity.tsuses JSON-array encoding vs the legacyelementId:group[:animId]:percentage— the legacy scheme could collide whenelementIdcontains:. Fallback branch preserves backward compat. Sound. - easeEach threading in
files.ts. Server derives outer envelope'seaseEachfrom the parsed original when client sendsease: 'none'. Preserves per-segment easing across convert. Test casereplace-with-keyframes preserves per-segment easing for exact temporal keyframesconfirms. - Clicked-element selection through
useTimelineEditCallbacks.onDeleteKeyframe,onMoveKeyframeToPlayhead,onMoveKeyframe,onDeleteAllKeyframesall now route throughbuildDomSelectionForTimelineElement(element)before committing — a delete on a non-selected element commits against that element's own selection, notdomEditSelection. Patches the exact gap Rames flagged in #2688.
Every claimed bug reproduces at base. No speculative fixes.
🟢 Verified clean
- Toolbar keyframe-toggle logic:
resolveKeyframeToggleStatecovers active/inactive/none across motion-path vs keyframe tweens;isMotionPathEndpointcorrectly skips endpoint deletion; aria-label branches on every state. Lens 4 (ARIA coherence) clean. applyArcKeyframeAtPlayhead's middle-node deletion (timedNodeIndex > 0 && timedNodeIndex < nodes.length - 1) skips endpoints — cannot delete arc start/end. Reasonable semantic; matches the toolbar's "cannot remove endpoints" tooltip.useGsapKeyframeOps.ts moveKeyframe/resizeKeyframedTweenPromise<boolean>return threadsresult.changed !== falseup correctly; catch branch returnsfalseand routes totrackGsapSaveFailure. Lens 1 (silent-catch) clean — every catch surfaces to telemetry.
Cross-stack
Rames's coverage-gap finding on Timeline.tsx:477 closest("button, input, select, a") bailout is real — no test dispatches a pointerdown on a nested button to assert scrub is not initiated. Follow-up-able.
Verdict
Approving. Correctness fixes are sound and reproducibly targeted at real pre-existing bugs. R9 verdict is split_path_exists (Rames) — I concur; the three ridealong files could split but the keyframe core is genuinely coupled.
— Review by Via
da9f172 to
69da4b0
Compare
2b4a520 to
67d2ef1
Compare
69da4b0 to
dd77b83
Compare
67d2ef1 to
50b6822
Compare
The base branch was changed.
3 participants
What
Hardens keyframe selection, authoring time, deletion, clip movement, and easing interactions.
Why
The reviewed timeline surfaced correctness bugs around stale selection, clip-relative versus global time, keyboard/button parity, pointer capture, bulk deletion, failed retimes, and moving clips with keyframes.
How
Preserves explicit identity and captured playhead time through the authoring transaction, makes selection gestures deterministic, keeps accessible ease controls mounted, and adds regression coverage for the critical paths. The cohesive 1,722-line delta is documented as an R9 correctness exception so fixes remain with their proof. This is B7 of the Family B draft Graphite stack.
Test plan
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 3 remaining low/nit findings are parked, verbatim, in
.scratch/studio-timeline-family-b/issues/07-pr-2689-deferred-review-findings.md:packages/studio/src/hooks/gsapDragPositionCommit.ts:278—activeKeyframePctnot cleared when arc drag falls through to temporal-keyframe branchpackages/studio/src/components/TimelineToolbar.test.tsx:61— TimelineToolbar keyboard/button parity: only the 'endpoint disabled' path asserted; 4 new motion-path label branches untestedpackages/studio-server/src/routes/files.ts:1056— Server-side resolveReplacementEaseEach re-parses the entire script on every request that omits easeEach