fix(studio): harden keyframe editing semantics#2787
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
50b6822 to
92c23fd
Compare
dd77b83 to
041ead6
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Adversarial R1 pass at 92c23fda3e2a3fd5a2d639d5918d65d4d93bd8ad. The hardening thesis (identity through the transaction, deterministic gestures, duration-less fallback, always-mounted ease control) is largely well-executed and the test surface actively violates the invariants (path endpoint block, colon-id round-trip, empty-writer settlement returning false). The stack-tip claim of green suites reads as accurate. Two callers of the new "clip-duration fallback" primitive were missed, and one small block adopts a useEffect derived-state pattern the house style forbids. All P2 — nothing here blocks merge on its own.
Verdict: COMMENT (approve with follow-ups tracked; recommend fixing the two duration-fallback drifts before this cascade lands on top of #2791).
Adversarial lenses
Lens A — Hardened edges vs displaced edges
The new resolveEditableTweenDuration(anim, sel) correctly makes applyKeyframeAtPlayhead treat a duration-less keyframe tween as spanning the owning clip. Two sibling call paths were not migrated, so the edge shifted rather than closed:
packages/studio/src/components/TimelineToolbar.tsx—resolveKeyframeToggleStatestill callsisPlayheadWithinTween(animation, currentTime)(line inside the new helper), which delegates toresolveTweenDuration(anim)with the 0.5 s default. For a duration-less non-arc keyframe tween on, e.g., a 16 s clip at playhead=5 s, the toolbar surfaceswillExtend=trueand the tooltip "Add keyframe at playhead, extends animation (K)", but clicking dispatches throughapplyKeyframeAtPlayhead, which now sees the playhead inside the tween (0..16.26) and toggles an interior keyframe instead of extending. Silent UX/action mismatch. P2.packages/studio/src/hooks/gsapDragPositionCommit.ts(arc branch, right afterarcPath?.enabledguard) — the new temporal-arc drop usesconst tweenDuration = resolveTweenDuration(anim); the siblingapplyArcKeyframeAtPlayheadin this same PR was migrated toresolveEditableTweenDuration. A duration-less arc dragged instead of clicked would authoring areplace-with-keyframesmutation withduration: 0.5, silently collapsing the arc window. Symmetric drift with the toolbar case. P2.
Recommend both call sites take a DomEditSelection parameter and route through resolveEditableTweenDuration — otherwise the "clip is the fallback" invariant only holds on the toggle path.
Lens B — New failure modes
The changed propagation via onResult is well-scoped: the store never sees the result until after the writer resolves, and both moveKeyframe and resizeKeyframedTween swallow the rejection into trackGsapSaveFailure (verified in useGsapKeyframeOps.test.tsx:113-155). No new bubbled throw. onDeleteAllKeyframes now awaits buildDomSelectionForTimelineElement before firing; if that resolves to null (element unmounted mid-context-menu), the delete is a silent no-op — the same behavior the caller had before, so no regression.
Lens C — Invariant enforcement
- "Motion path endpoints cannot be removed" is enforced at the button (disabled + relabeled aria) and at
applyArcKeyframeAtPlayhead(interior-only viatimedNodeIndex > 0 && timedNodeIndex < nodes.length - 1). The toolbar test atTimelineToolbar.test.tsx:60-100violates the invariant and confirms disable. Enforced at both entry and internal call site — good. timelineKeyframeSelectionKeyround-trip. JSON envelope + colon-id fallback is validated attimelineKeyframeIdentity.test.ts:17-33, including the colon-in-id case that the previous format silently mis-parsed. Encoding is asymmetric withtweenPercentage: the writer defaults it topercentagewhen absent (line just after the JSON stringify), but the reader treats it as an equally-authoritative dimension. If a call site passes onlypercentage, the two keys are interchangeable; if any caller mutatestweenPercentagewithoutpercentage, two logically-equal selections may hash to different keys. Not a bug today, but worth a code-adjacent comment.
Lens D — Backwards compat / anti-patterns
packages/studio/src/components/editor/propertyPanelFlatTextSection.tsx:260-279introduces twouseEffectcalls whose body only callssetAutoFocusFieldKey(null). The second —if (autoFocusFieldKey && autoFocusFieldKey === activeFieldKey) setAutoFocusFieldKey(null)— is a textbook derived-state effect (clear the "just added" marker once the added field becomes active). Under the repo's Golden Rule "No useEffect for state syncing" this belongs either in the same.then((nextKey) => …)callback that armed it, or as derived-during-render (const autoFocus = pendingAutoFocusKey === activeField.key). P2.
Lens E — Stack interaction (#2786, #2791)
Signatures of moveKeyframe and resizeKeyframedTween moved from void to Promise<boolean>; useDomEditWiring.ts typings updated in-PR. If #2791 (expanded lanes) authors any call to these that expects the void shape, it will type-fail on rebase — but the stack is serial (#2786 → #2787 → #2791), so this is a scheduling constraint, not a live risk. onDeleteAllKeyframes / onMoveKeyframeToPlayhead also swapped their elementId: string arg for a full TimelineElement; I enumerated the three constructors (Timeline.tsx, useTimelineKeyframeHandlers.ts, MotionPathOverlay.tsx) — all three pass the element explicitly, so blast radius is bounded to this PR.
Peer-lens checks
- Facade blast-radius for
KeyframeDiamondContextMenuState.element— three constructors, all migrated in-PR ✓. - Extract-to-helper
resolveKeyframeToggleState— single caller in the same file ✓, but the extracted helper introduced the layer-N/N+1 gap flagged in Lens A. - Deleted
packages/studio/src/utils/keyframeSelection.ts— sole consumerdeleteSelectedKeyframes.tsswapped totimelineKeyframeTargetFromSelectionKeyin this PR; helper + test both go together ✓. - Fixture rename
#qa-keyframe-box→#qa-zone-keyframeinpackages/studio/tests/e2e/fixtures/design-panel-qa/index.html— cross-check any e2e spec still targeting the old id; not in this file list, but worth arg qa-keyframe-boxbefore merge.
— Via
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 92c23fda3e2a.
R2 adversarial pass on the correctness bundle. R9 verdict: split_path_exists — the keyframe-core clusters (identity+delete rewrite, duration-less basis, motion-path temporal keyframes, callback element-identity/settlement) form a coherent thread, but ~6 unrelated cleanups ride along (TimelineClipDiamonds hit-region math, ease-button hover-visibility rewrite, propertyPanelFlatTextSection multi-field auto-focus, useStudioTestHooks typing, keyframeRetime.test refactor, e2e fixture retarget). One blocker: isPlayheadWithinTween (useEnableKeyframes.ts:84) still uses resolveTweenDuration(anim) with the 0.5s fallback while the PR standardizes the rest of the surface on resolveEditableTweenDuration (clip-duration fallback). This is called by BOTH TimelineToolbar toggle-state AND applyArcKeyframeAtPlayhead, so toolbar and click-handler now disagree on whether the playhead sits inside a duration-less tween. Same class of bug the PR claims to have closed. Convergent with Via's flag on the same displaced-edge pattern; also caught buildTemporalArcKeyframes append-without-dedup and motion-path 0.05% vs 1% asymmetry.
R9 exception verdict — split_path_exists
Keyframe-core clusters (identity+delete rewrite; duration-less basis; motion-path temporal keyframes; callback element-identity/settlement contract) form one coherent 'harden keyframe editing semantics' thread. But the PR bundles at least five independent clusters that touch no shared authority:
- (a) TimelineClipDiamonds hit-region math (markerMetrics, non-overlapping widths) + test — pure UI math
- (b) Ease-button hover-visibility rewrite (setState → opacity-0 group-hover) + TimelinePropertyLanes.test rewrite
- (c) propertyPanelFlatTextSection multi-field auto-focus fix + tests
- (d) useStudioTestHooks typing-only hardening
- (e) keyframeRetime.test refactor (dedupe expectLeftResize helper)
- (f) e2e fixture retarget from #qa-keyframe-box to #qa-zone-keyframe with no in-diff rationale
None of (a)–(f) shares mutable authority with keyframe-core files; each could land as separate small PR. This is the largest PR in the stack (+1320/-382) precisely because unrelated cleanups were folded into an R9 bundle.
Bug-test coverage audit
- ✅ Cross-element stale selection percentages must not be applied to active element on bulk delete: timelineEditingHelpers.test.ts:346-393 covers per-animation dispatch and coalesceKey; specific 'other element's keyframes dropped' case is implicit through element-id gate
- ✅ Motion-path drag on implicitly selected point adds temporal keyframe not spatial waypoint: gsapRuntimeBridge.test.ts:311-360 covers activeKeyframePct=null (temporal) and =50 (spatial). Near-miss case (0.5% off authored waypoint) not covered — see 0.05 tolerance finding
- ✅ 'Add keyframe at playhead' on arc deletes existing interior stop instead of redistributing path: useEnableKeyframes.test.ts:238-262 asserts interior removal preserves endpoint timing; endpoints protected (line 266-269)
- ✅ Duration-less tween editors fall back to OWNING clip duration not 0.5s: gsapShared.test.ts:11-23 and globalTimeCompiler.test.ts:115-118 for the unit; useTimelineEditCallbacks.test.tsx:369-393 for retime flow. But isPlayheadWithinTween (useEnableKeyframes.ts:84) — SAME class of decision — was NOT converted; no test covers duration-less tween through toolbar toggle path
- ✅ Delete-all / move-to-playhead on diamond in non-selected element commits against clicked element: useTimelineEditCallbacks.test.tsx:268-304 asserts handleGsapRemoveAllKeyframes / handleGsapMoveKeyframeToPlayhead receive CLICKED element's selection
- ✅ Move-keyframe / resize-tween outcomes settle to Promise so callers react to no-op/failure: useGsapKeyframeOps.test.tsx:126-166 covers no writer result, changed:false, and rejected commits
- ✅ replace-with-keyframes on arc preserves per-segment easing: files.test.ts:1833-1872 asserts easeEach: 'power1.inOut' after replacement. See outer-ease-vs-easeEach semantics drift note
⚠️ Scrubbing on timeline must not swallow clicks on interactive controls: No test exercises Timeline.tsx:439's closest('button, input, select, a') bailout- ✅ Multi-field text panels don't steal canvas focus when element is selected: propertyPanelFlatTextSection.test.tsx:406-422 asserts activeElement stays on focus owner
- ✅ Adjacent keyframe diamonds don't overlap hit regions or visual squares in dense timelines: TimelineClipDiamonds.test.tsx:48-79 asserts three keyframes at 30/60/90% across 36px produce non-overlapping hit widths ~10.8px and visual widths ~8.8px
🟢 Verified clean
- keyframeRetime.test.ts refactor (dedup expectLeftResize helper) is behaviour-preserving
- TimelinePropertyLanes.test.tsx switch from 'reveal on hover' to 'always-rendered opacity-0 group-hover:opacity-100' matches CSS in TimelineClipDiamonds.tsx
- KeyframeDiamondContextMenu now requires state.element: TimelineElement; every producer in the diff provides it — no stranded call site
- Deletion of packages/studio/src/utils/keyframeSelection.ts: only importer (deleteSelectedKeyframes.ts) is rewritten and no other in-repo references remain
- useDomEditWiring.ts return-type widening to Promise for moveKeyframe / resizeKeyframedTween matches useGsapKeyframeOps.ts's rewrite and useGsapSelectionHandlers.ts's forwarding — signatures reconcile
- onMoveKeyframe: TimelineEditCallbacks returns Promise; useTimelineEditCallbacks.ts:254 always returns a Promise; TimelineClipDiamonds.tsx:518 handles via .then((committed) => ...)
Complements Via's parallel adversarial pass (Family B rollup). Where Via found 0 P1 / 17 P2 / 18 P3, the adversarial subagent pass here surfaced additional depth on the mutation-authority thread and the Promise chain.
| anim: GsapAnimation, | ||
| currentTime: number, | ||
| position: Record<string, number>, | ||
| sourceDuration = resolveTweenDuration(anim), |
There was a problem hiding this comment.
🔴 isPlayheadWithinTween still uses 0.5s fallback — toolbar and applyKeyframeAtPlayhead disagree for a duration-less keyframed tween
return isTimeWithinTween(currentTime, start, resolveTweenDuration(anim));PR's stated fix is that keyframe editors must use the OWNING clip duration when tween omits an outer duration (resolveEditableTweenDuration). computeElementPercentage (gsapShared.ts:119) and applyArcKeyframeAtPlayhead (useEnableKeyframes.ts:378) were switched to the editable basis, but isPlayheadWithinTween — called by BOTH TimelineToolbar.resolveKeyframeToggleState (TimelineToolbar.tsx:76) AND the arc branch here — still resolves duration via raw 0.5s fallback. For a duration-less tween on a 16.26s clip, toolbar reports 'outside tween' → willExtend=true and shows 'Extend to playhead' tooltip, while click handler will compute the editable duration (16.26s) and treat the same time as inside the tween, executing a plain add/remove. Same class of bug the PR body claims to have fixed.
Fix: Route isPlayheadWithinTween through resolveEditableTweenDuration (or accept an explicit duration argument, forwarded from the toolbar which already has the selection).
| act(() => root.unmount()); | ||
| }); | ||
| }); | ||
| describe("TimelineToolbar — motion path endpoints", () => { |
There was a problem hiding this comment.
🟠 TimelineToolbar motion-path tooltip states have no regression test except the endpoint case
describe("TimelineToolbar — motion path endpoints", () => {
it("does not advertise a destructive keyframe toggle for a required endpoint"resolveKeyframeToggleState (TimelineToolbar.tsx:63-90) now branches into isMotionPath, pathEndpoint, willExtend, and toggles four distinct aria-labels/tooltips ('Add motion path waypoint', 'Remove motion path waypoint', 'Extend motion path to playhead', 'Motion path endpoint'). Only the pathEndpoint state is covered by the sole new test. The willExtend interaction with the isPlayheadWithinTween-vs-editable-duration bug above is exactly the sort of thing this suite should have caught.
Fix: Add tests for at least (a) isMotionPath && active → 'Remove waypoint', (b) isMotionPath && inactive && !willExtend → 'Add waypoint', (c) isMotionPath && willExtend → 'Extend motion path to playhead'.
| onDrop={handleAssetDrop} | ||
| onPointerDown={(e) => { | ||
| // Let interactive controls (keyframe nav/toggle, caret, inputs) handle | ||
| // their own clicks — scrubbing here would preventDefault and eat them. |
There was a problem hiding this comment.
🟡 Timeline scrubbing bailout for interactive controls has no regression test
if (e.target instanceof Element && e.target.closest("button, input, select, a")) return;This is a behavioral bug fix (previously scrubbing preventDefault'd inside the timeline could eat clicks on nav/toggle buttons, inputs, and the ruler caret). Comment 'Let interactive controls (keyframe nav/toggle, caret, inputs) handle their own clicks' names concrete regressions the PR claims to fix, but no test in the diff exercises 'pointerdown on a button inside the timeline does not trigger scrub or razor'. A silent regression here (e.g., future refactor changing the selector) will not be caught.
Fix: Add a Timeline.test that dispatches pointerdown on a nested <button> and asserts handlePointerDown / onRazorSplitAll are not invoked; likewise for shift+click while activeTool='razor'.
| return; | ||
| } | ||
| const pct = computeElementPercentage(t, sel, kfAnim); | ||
| const pct = |
There was a problem hiding this comment.
🟠 applyKeyframeAtPlayhead compares clip-relative percentage against tween-relative keyframe percentages when tween start is unresolved
const pct =
start === null ? computeElementPercentage(t, sel) : absoluteToPercentage(t, start, duration);
const existing = kfAnim.keyframes?.keyframes.find((k) => Math.abs(k.percentage - pct) <= 1);When resolveTweenStart returns null the branch falls back to computeElementPercentage(t, sel) with no animation argument, which now returns a clip-range percentage (0..100 across data-start/data-duration on the DOM element). It then compares that clip-basis percentage against kfAnim.keyframes.keyframes[].percentage, which is authored tween-relative. For a keyframed tween whose start is unresolved (e.g., a labelled position on a nested timeline) this either matches the wrong existing keyframe or fails to find one that should have been toggled off — exactly the class of 'wrong-basis' bug this PR aims to eliminate.
Fix: Either short-circuit (bail out with a warning when start === null and kfAnim.keyframes exists), or route the fallback through the clip's timing basis and convert to tween-relative before comparing.
| const { activeKeyframePct, setActiveKeyframePct } = usePlayerStore.getState(); | ||
| const pct = activeKeyframePct ?? computeCurrentPercentage(selection, anim); | ||
| const keyframes = anim.keyframes?.keyframes ?? []; | ||
| const pointIndex = keyframes.findIndex((kf) => Math.abs(kf.percentage - pct) < 0.05); |
There was a problem hiding this comment.
🟠 Motion-path drag intercept uses a 0.05% waypoint-match tolerance while every other arc handler uses 1% — dragging near an existing waypoint silently adds a duplicate
const pointIndex = keyframes.findIndex((kf) => Math.abs(kf.percentage - pct) < 0.05);applyArcKeyframeAtPlayhead (useEnableKeyframes.ts:397) and isMotionPathEndpoint (TimelineToolbar.tsx:55) both use |Δpct| <= 1 for 'this playhead is on an existing keyframe'. The drag path uses < 0.05. Consequence: if the user has NOT click-selected the diamond (so activeKeyframePct is null and pct falls back to computeCurrentPercentage), a drag whose playhead percentage rounds to 0.1..0.9 away from an authored keyframe will (a) skip the update-motion-path-point branch, (b) call buildTemporalArcKeyframes which appends without dedup, and produce two keyframes ~0.5% apart on the arc. New test only covers the two clean cases (activeKeyframePct=null with a novel percentage, or activeKeyframePct=50 exactly on a waypoint) — never the near-miss.
Fix: Use the same 1% tolerance as applyArcKeyframeAtPlayhead, or (better) make the tolerance derive from tween duration / clip resolution and share the check with useEnableKeyframes.
|
|
||
| const separator = key.lastIndexOf(":"); | ||
| if (separator < 0 || key.slice(0, separator) !== elementId) return null; | ||
| const percentage = Number(key.slice(separator + 1)); |
There was a problem hiding this comment.
🟢 Regression coverage for collapsed selection-key form with a colon-bearing element id was deleted and not re-added elsewhere
// timelineKeyframeTargetFromSelectionKey preserves lastIndexOf(':') semantics
// for the collapsed fallback used by Timeline.tsx:505/513The deleted test asserted selectedKeyframePercentagesForElement(new Set(['a:b:40']), 'a:b') === [40] — explicit reason the old helper used lastIndexOf(':'). timelineKeyframeTargetFromSelectionKey preserves that shape for the collapsed fallback but its new test suite only exercises JSON-encoded (expanded lane) round-trip for a colon-bearing id and collapsed form for 'comp#a' (no colon). Timeline.tsx:505/513 still writes the collapsed form for clip-lane shift-clicks — the exact code path that would break on a colon-bearing element id.
Fix: Add a case to timelineKeyframeIdentity.test.ts: timelineKeyframeTargetFromSelectionKey('a:b', 'a:b:40') → { percentage: 40 } (plus the negative: 'a:b:40' with active id 'a' returns null).
| tl.to("#qa-tween-box", { x: 300, rotation: 90, duration: 2, ease: "power1.inOut" }, 0); | ||
| tl.to( | ||
| "#qa-keyframe-box", | ||
| "#qa-zone-keyframe", |
There was a problem hiding this comment.
🟢 e2e fixture retargets the keyframed tween from the inner box to the outer zone with no diff-visible rationale
- "#qa-keyframe-box",
+ "#qa-zone-keyframe",Only change to any e2e fixture in the PR; target element goes from innermost animated element (#qa-keyframe-box) to its wrapping zone (#qa-zone-keyframe, parent). Commit body ('harden keyframe editing semantics') gives no reason for the target selector to move up one layer. If chasing a real bug (parent-vs-child selector resolution when timeline element identity switched from string to TimelineElement) deserves its own commit + assertion; if flake-driven should be documented. Either way, retargeting a shared QA fixture inside a 41-file 'correctness' bundle is a review hazard.
Fix: Split into its own commit with a note explaining which failure mode motivates the change, or restore the original selector and document why current tests still pass.
| const toPercentage = computeCurrentPercentage(sel, anim); | ||
| trackStudioEvent("keyframe", { action: "move_to_playhead" }); | ||
| moveKeyframe(sel, animId, fromPercentage, toPercentage); | ||
| void moveKeyframe(sel, animId, fromPercentage, toPercentage); |
There was a problem hiding this comment.
🟢 handleGsapMoveKeyframeToPlayhead's write outcome is discarded — 'move to playhead' UI paths cannot react to a failed retime
void moveKeyframe(sel, animId, fromPercentage, toPercentage);Rest of the PR standardizes retime settlement so callers can distinguish 'committed', 'no-op' (changed === false), and 'failed' via CommitMutationOptions.onResult → useGsapKeyframeOps.moveKeyframe/resizeKeyframedTween → useDomEditWiring signatures → useTimelineEditCallbacks.onMoveKeyframe. Only handleGsapMoveKeyframeToPlayhead swallows the Promise with void. KeyframeDiamondContextMenu 'Move to Playhead' button (primary consumer) and toolbar cannot show 'no-op / failure' state — exactly the loss of information that motivated the settlement plumbing.
Fix: Return the Promise from moveKeyframe (and update TimelineEditCallbacks.onMoveKeyframeToPlayhead to Promise<boolean>) so the ContextMenu can leave menu open or surface a toast when write is no-op / failure.
| (animation) => animation.id === request.animationId, | ||
| ); | ||
| if (!original?.arcPath?.enabled) return undefined; | ||
| return original?.keyframes?.easeEach ?? original?.ease; |
There was a problem hiding this comment.
🟡 resolveReplacementEaseEach silently rewrites 'outer ease' as per-segment easeEach — visual behaviour of arc converted to temporal keyframes may drift
if (!original?.arcPath?.enabled) return undefined;
return original?.keyframes?.easeEach ?? original?.ease;For an arc authored with only outer ease: 'power1.inOut' and no easeEach, client now issues { ease: 'none' } (no easeEach) and server backfills easeEach: 'power1.inOut'. GSAP applies outer ease across the whole tween timeline but applies easeEach per keyframe segment — so 'power1.inOut applied once to the entire arc' becomes 'power1.inOut applied independently to every segment'. For a 2-segment arc the second-segment restart of the eased curve is visible. New server test only asserts the string 'easeEach: "power1.inOut"' appears, not that visual motion is preserved.
Fix: Either preserve only easeEach (drop the fallback to original.ease), or, when translating outer ease → easeEach, emit the equivalent piecewise curve; alternatively, document explicitly that arc→temporal-keyframe conversion changes ease semantics.
| // than deleting the whole animation — deleting strands a stale GSAP base | ||
| // that the next drag adds to, flinging the element off-screen. | ||
| const anim = selectedGsapAnimations.find((a) => a.keyframes); | ||
| const elementKey = getTimelineElementIdentity(element); |
There was a problem hiding this comment.
🟢 onDeleteAllKeyframes on the timeline toolbar bag ignores handleGsapRemoveAllKeyframes' return, defeating error surfacing when selection lookup fails
void buildDomSelectionForTimelineElement(element).then((selection) => {
if (selection) handleGsapRemoveAllKeyframes(anim.id, selection);
});buildDomSelectionForTimelineElement returning null (deleted DOM node, cross-file element without accessible selection) results in silent no-op — no toast, no telemetry, no console warn. Behaviour change vs. pre-PR path which called handleGsapRemoveAllKeyframes(anim.id) directly against domEditSelection so store's guardrails ran even when passed selection was stale. Because handleGsapRemoveAllKeyframes now accepts a selectionOverride but useGsapSelectionHandlers.ts:412 still bails on if (!selection) return;, both layers silently no-op. No test for the null-selection branch.
Fix: In the else branch of the .then, either fall back to handleGsapRemoveAllKeyframes(anim.id) (no override → use domEditSelection ?? lastSelectionRef.current) or emit a warn/toast so user knows action was dropped.
041ead6 to
bcc22d6
Compare
1f73607 to
5f9725e
Compare
Two sibling call paths still answered from GSAP's 0.5s default while the toggle path had moved to the clip-wide fallback. isPlayheadWithinTween now takes the selection, so the toolbar stops promising "extends animation" on a duration-less tween whose click actually toggles an interior keyframe. The arc drag commit resolves its replacement duration the same way its click-path sibling does, instead of authoring duration 0.5 and collapsing the arc window. The flat text section drops its two marker-clearing effects: the autofocus marker is a ref now, read and cleared by the render that consumes it.
The collapsed clip row dropped a keyframe's property group and animation id before handing it to a callback, so the same keyframe hashed to a different selection key than the expanded property lane did. Selecting a diamond in one view left it unselected in the other, and retime/delete on the collapsed row lost the animation id they use to pick between two animations that collide at one percentage. Diamonds now always carry their full identity, the collapsed shim just curries the element id, and Timeline reuses useTimelineKeyframeHandlers instead of its own inline copy of the same three handlers. Neighbour geometry moves into one marker record per diamond, which drops the index-lookup non-null assertions the connector pass needed.
5f9725e to
e5c162e
Compare
89eeddb to
783fa4a
Compare
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 easeEachSupersedes #2689, which was closed when
mainwas rewritten to unwind an early landing of this stack. Same head commit, same review history.R1 review follow-ups
Fixed in this PR:
isPlayheadWithinTweentakes the selection, so the toolbar stops promising "extends animation" on a duration-less tween whose click actually toggles an interior keyframe.duration: 0.5and collapsing the arc window.timelineKeyframeSelectionKeydocuments thepercentage/tweenPercentageasymmetry its callers have to respect.Checked, no action needed: the
#qa-keyframe-boxto#qa-zone-keyframerename applies to the clip wrapper only. The inner box keeps its id and no spec outside the fixture references either.R2 review follow-ups
Fixed in this PR:
keyframeTargetdropped the property group and animation id for collapsed clip rows, so the same keyframe hashed to a different selection key collapsed than expanded (selected in one view, unselected in the other) and the retime and delete mutations lost the id they use to pick between two animations colliding at one percentage. The target now always carries the full identity the cache already holds, and the callbacks take that target instead of flattened positional fields. Covered by a regression test asserting one shared key across both views.Timeline.tsxcarried its own copy of the three keyframe handlers alongside the existinguseTimelineKeyframeHandlershook. The inline copy is deleted and the hook is wired in, so there is one implementation.