feat(studio): add keyframe timeline state#2781
Conversation
5dd41e9 to
e4d7bde
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R1 adversarial review — head e4d7bde
Verdict: COMMENT. State-shape primitive is sound: pure setters, no Date.now/Math.random/DOM in reducers, immutable Set/Map copies on write, and the ambiguity-flag semantics are captured in two tests. Author has parked six known low/nit findings verbatim in the scratch doc, so I'm surfacing only the ones that are stack-forward defects or drift risks the deferred list didn't already own. Head SHA verified e4d7bde64f3b3e500834231134bc9fdb3858ad41; pre-existing state on main confirms selectedKeyframes:Set and keyframeCache:Map shapes are inherited, so I only fault the new additions where scope worsens the invariant.
P2
1. Selection primitives are single-key only — later stack PRs will patch this slice. packages/studio/src/player/store/keyframeSlice.ts:31-35. toggleSelectedKeyframe(key) + clearSelectedKeyframes() covers add/remove/clear. Any downstream marquee/range/select-all lands with either (a) a loop of toggleSelectedKeyframe (N re-renders, each rebuilding the Set), or (b) a slice patch that adds replaceSelectedKeyframes(keys). Since this PR is explicitly the shared primitive, add replaceSelectedKeyframes(keys: readonly string[]) and selectRangeKeyframes(keys: readonly string[]) now — Lens B / Lens E.
2. easeAmbiguous guard is duplicated verbatim across the two writers — the exact drift risk the deferred 01-pr-2683 doc calls out. packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:60-72 and packages/studio/src/hooks/gsapTweenSynth.ts:15-30. Both blocks encode the same three-clause invariant on the merged keyframe. The invariant is the state contract downstream lanes read from (easeAmbiguous ⇒ suppress inline button), so a divergence between the two writers is a silent data-shape defect, not just a style nit. Extract flagAmbiguousEase(existing, incoming) and call it from both sites. Deferring this to a future PR is fine, but this is the correct PR to fix it — the slice is where the invariant becomes public API. Author flagged 🟡.
3. gsapAnimations silently diverges from keyframeCache on any anim missing propertyGroup. packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:22-25 gates the sourceAnimations write on if (anim.propertyGroup); keyframeCache writes regardless. useGsapTweenCache.ts:333-337 applies the same filter. Net effect: for an element whose only tween has no propertyGroup, keyframeCache has an entry, gsapAnimations is empty, and writeGsapAnimationsForElement(..., undefined) deletes whatever was there. The slice comment (keyframeSlice.ts:38) says "expanded property lanes read this, never keyframeCache" — so an expanded lane on that element renders nothing while the collapsed row still shows diamonds. Either (a) tighten the invariant ("no propertyGroup ⇒ don't cache") uniformly across both writers, or (b) drop the propertyGroup gate on gsapAnimations and let downstream lane renderers filter. Pick one; the current split is undefined behavior. Lens A / Lens E.
4. selectedKeyframes holds two disjoint key shapes with no discriminator — author flagged 🔵, upgrading to P2 because it's the slice's public API. packages/studio/src/player/store/keyframeSlice.ts:15. Collapsed diamonds key as element:pct, expanded as element:group:animation:clipPct. Callers passing a collapsed key against an expanded set won't error — they'll silently miss. This is a state-shape defect the stack will inherit N times. Fix now with a discriminator prefix (c: / e:) or split into two Sets before consumers land, so the top of the stack doesn't cement it.
P3
5. expandClips has no collapseClips counterpart. packages/studio/src/player/store/keyframeSlice.ts:36. Author flagged 🟢. Symmetric pair belongs in the primitive — Lens E.
6. window.__studioTest is reassigned every effect re-run. packages/studio/src/hooks/useStudioTestHooks.ts:25-46. Effect deps [applyDomSelection, buildDomSelectionFromTarget, previewIframeRef] will churn on any parent re-render that doesn't memoize the two callbacks; each churn stomps any handle a dev driver just captured. Guard with if (!window.__studioTest) or install once at module scope. Author flagged 🟢.
7. Set/Map shape is persistence-hostile; document that constraint on the slice. packages/studio/src/player/store/keyframeSlice.ts:31, 47, 49. Neither zustand/persist nor devtools middleware is currently wired (grep-verified), so this is deferred, not blocking. But two new Map/Set fields land in this PR, and any downstream PR that adds persist/devtools/SSR replay will round-trip them to {} with no warning. Add a top-of-file comment: "Set/Map by design — do not add persist middleware without a serializer." Lens D.
Adversarial lenses
- A (state-shape): #3, #4 fired — divergent cache invariants and mixed key shapes are the P2s.
- B (selection): #1 fired — no bulk/range primitive.
- C (reducer purity): clean; only #6 (window write in effect) is a side-effect boundary nit.
- D (serialization): #7 fired — deferred but worth pinning.
- E (stack-forward compat): #1, #3, #5 fired — all shape gaps the primitive should close before consumers land.
Approve after #2 (ambiguity-helper extraction) and #3 (propertyGroup gate convergence) land in-stack; #1 and #4 are worth attempting now while the primitive has zero consumers.
— 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 e4d7bde64f3b.
R2 adversarial pass on the foundation slice for Family B. Publishes 5 contracts (keyframeCache, gsapAnimations, focusedEaseSegment, selectedKeyframes, expandedClipIds) that all 7 downstream PRs build on. The concerns cluster around primitive-layer invariants the type system doesn't enforce — they'll propagate through B2-B8 consumers, so worth closing here. Convergent with Via's rollup on gsapAnimations/keyframeCache divergence.
🟢 Verified clean
- idSelector() correctly emits [id="..."] fallback for digit-leading/dotted/space-containing ids and escapes quotes/backslashes; SAFE_HASH_ID regex ^-?[A-Za-z_][\w-]*$ cannot match anything that would break querySelector
- deduplicateKeyframes' generic signature threads animationId/easeAmbiguous through without narrowing to GsapPercentageKeyframe
- trackStudioSegmentEaseEdit's discriminant action: 'open' | 'commit' threaded through AnimationCard + events test
- createKeyframeSlice's initial Set/Map values are fresh instances; toggle/set/expandClips setters return early with state unchanged on no-op transitions (proper Zustand shallow-eq)
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.
| outPoint: null, | ||
| activeTool: "select", | ||
| selectedKeyframes: new Set(), | ||
| expandedClipIds: new Set(), |
There was a problem hiding this comment.
🟠 playerStore.reset() does not clear focusedEaseSegment — stranded state across composition switches
reset: () =>
set({
isPlaying: false,
currentTime: 0,
duration: 0,
timelineReady: false,
beatDragging: false,
elements: [],
selectedElementId: null,
inPoint: null,
outPoint: null,
activeTool: "select",
selectedKeyframes: new Set(),
expandedClipIds: new Set(),
selectedElementIds: new Set(),
clipRevealRequest: null,
keyframeCache: new Map(),
gsapAnimations: new Map(),New KeyframeSlice adds focusedEaseSegment (initial null, keyframeSlice.ts:40,89) as project-scoped state referencing a specific animationId + elementId in the currently-loaded composition. reset() is described as "resets project-specific state when switching compositions" and enumerates every other slice field (selectedKeyframes, expandedClipIds, keyframeCache, gsapAnimations) but omits focusedEaseSegment. After composition switch, store holds a focus target for element/animation that no longer exists; PropertyPanelFlat / propertyPanelFlatMotionSection / GsapAnimationSection read this and try to open the ease editor for a stale id.
Fix: Add focusedEaseSegment: null to the reset() set(...) call alongside other keyframe-slice resets. Consider exposing a resetKeyframeSlice() from keyframeSlice.ts so future slice additions can't drift from the reset.
| for (const anim of animations) { | ||
| const id = anim.targetSelector.match(/^#([\w-]+)/)?.[1]; | ||
| if (!id || !anim.keyframes) continue; | ||
| const kfSource = |
There was a problem hiding this comment.
🟠 updateKeyframeCacheFromParsed strips [id="..."] selectors — cache stale for the very ids idSelector() was added to unblock
for (const anim of animations) {
const id = anim.targetSelector.match(/^#([\w-]+)/)?.[1];
const kfSource =
anim.keyframes?.keyframes ?? synthesizeFlatTweenKeyframes(anim)?.keyframes ?? [];
if (!id || kfSource.length === 0) continue;The whole point of new idSelector() (gsapShared.ts) is that ensureElementAddressable / useGestureCommit / gsapDragCommit.materializeIfDynamic now emit [id="01-hook-..."] for digit-leading, dotted, or CSS-unsafe ids. But post-commit cache updater still parses target with /^#([\w-]+)/, which does not match an attribute selector. id becomes undefined, branch continues, cache never refreshed after mutation. Second regex use at line 91 has same shape. After drag/keyframe mutation on digit-leading id, diamonds stay pinned to pre-commit positions until unrelated full file re-scan runs.
Fix: Parse ids from both selector shapes, e.g. anim.targetSelector.match(/^#([\w-]+)/)?.[1] ?? anim.targetSelector.match(/^\[id="((?:\\.|[^"\\])+)"\]/)?.[1] (unescaping captured value); mirror the fix in targetId computation.
| // fallow-ignore-next-line complexity | ||
| useEffect(() => { | ||
| if (!elementId) return; | ||
| const sourceAnimations = animations.filter( |
There was a problem hiding this comment.
🟡 useGsapAnimationsForElement never clears stale gsapAnimations when source animations become empty
useEffect(() => {
if (!elementId) return;
const sourceAnimations = animations.filter(
(animation) =>
animation.propertyGroup && (animation.keyframes || synthesizeFlatTweenKeyframes(animation)),
);
if (sourceAnimations.length > 0)
writeGsapAnimationsForElement(sourceFile, elementId, sourceAnimations);Only .length > 0 branch writes. When an element previously had propertyGroup-tagged animations and now has none (remove-all-keyframes, delete-animation), effect never invalidates the stale gsapAnimations entry — falls through to keyframeCache-only handling below. useAutoExpandKeyframedClips subscribes to gsapAnimations and would keep auto-expanding a clip whose keyframes have been removed; propertyPanelFlatMotionSection reads it as source list.
Fix: Symmetrically call writeGsapAnimationsForElement(sourceFile, elementId, undefined) when sourceAnimations.length === 0 (guarded on store already having an entry for the key so idle re-renders don't allocate a new Map).
| // Mirror deduplicateKeyframes: a same-% collision across different | ||
| // source animations is an ambiguous merged segment (the button can | ||
| // only target one arbitrary animation). Flag it so the collapsed row | ||
| // suppresses the inline ease button there. |
There was a problem hiding this comment.
🟡 updateKeyframeCacheFromParsed can wipe valid gsapAnimations when merged element has only propertyGroup-less animations
for (const [id, entry] of merged) {
setKeyframeCache(`${targetPath}#${id}`, entry);
setKeyframeCache(id, entry);
if (targetPath !== "index.html") setKeyframeCache(`index.html#${id}`, entry);
writeGsapAnimationsForElement(targetPath, id, sourceAnimations.get(id));
}sourceAnimations is populated only when anim.propertyGroup is truthy (line 26). GsapAnimation.propertyGroup is optional (propertyGroup?: PropertyGroupName in packages/parsers/src/gsapSerialize.ts:77). When an element is in merged because it has real keyframes but none of its parsed animations carry propertyGroup, sourceAnimations.get(id) is undefined. writeGsapAnimationsForElement(..., undefined) iterates every key variant and calls setGsapAnimations(key, undefined) — slice implements as Map delete. Any gsapAnimations previously written for this element (from useGsapAnimationsForElement / usePopulateKeyframeCacheForFile) are silently deleted — cache-vs-source drift where diamonds render but source list disappears.
Fix: Only call writeGsapAnimationsForElement when sourceAnimations.has(id), so a mutation that legitimately has no propertyGroup metadata for that id doesn't wipe unrelated writers' data. If wipe IS desired here, do it explicitly (and mirror in usePopulateKeyframeCacheForFile).
| const existing = byPct.get(kf.percentage); | ||
| if (existing) { | ||
| existing.properties = { ...existing.properties, ...kf.properties }; | ||
| // Two DIFFERENT source animations with a keyframe at the same clip %: a |
There was a problem hiding this comment.
🟡 deduplicateKeyframes leaves ease iteration-order-dependent after flagging easeAmbiguous
const existing = byPct.get(kf.percentage);
if (existing) {
existing.properties = { ...existing.properties, ...kf.properties };
// ...
if (
existing.animationId !== undefined &&
kf.animationId !== undefined &&
existing.animationId !== kf.animationId
) {
existing.easeAmbiguous = true;
}
if (kf.ease) existing.ease = kf.ease;Once easeAmbiguous is set, existing.ease still overwritten by whichever conflicting animation happens to iterate last — the ease field on the merged keyframe is nondeterministic (depends on parser order across animations, e.g. #a-position vs #a-visual). Inline ease button is suppressed via easeAmbiguous, but other consumers of KeyframeCacheEntry.keyframes[i].ease (drag readouts, per-lane rendering hints not checking the ambiguous flag) see an arbitrary ease value. Parallel merge path in updateKeyframeCacheFromParsed (gsapKeyframeCacheHelpers.ts:69) has same shape.
Fix: Once easeAmbiguous becomes true, either stop mutating existing.ease or explicitly clear it so downstream readers must go through the ambiguous flag. At minimum, document that ease is not stable under an ambiguous keyframe.
| applyDomSelection(selection, { revealPanel: true }); | ||
| return true; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🟡 useStudioTestHooks cleanup sets window.__studioTest = undefined instead of removing the key
(window as unknown as { __studioTest?: typeof api }).__studioTest = api;
return () => {
(window as unknown as { __studioTest?: typeof api }).__studioTest = undefined;
};Hook effect deps [applyDomSelection, buildDomSelectionFromTarget, previewIframeRef] include callbacks without stable identities across renders. Every re-render tears down and reinstalls the API. Assigning undefined also leaves __studioTest enumerable as own property with undefined value — automated tests doing window.__studioTest && ... handle it, but "__studioTest" in window remains true, which can defeat feature-detection.
Fix: Use delete (window as unknown as { __studioTest?: typeof api }).__studioTest; in cleanup, and consider guarding setup with if ((window as any).__studioTest === api) check before deletion so later mount doesn't wipe active handle installed by another instance.
| keyframeCache: new Map(), | ||
| setKeyframeCache: (elementId, data) => | ||
| set((state) => { | ||
| const next = new Map(state.keyframeCache); |
There was a problem hiding this comment.
🟢 KeyframeSlice setters allocate a new Map on every call, even for no-op deletes
setKeyframeCache: (elementId, data) =>
set((state) => {
const next = new Map(state.keyframeCache);
if (data) next.set(elementId, data);
else next.delete(elementId);
return { keyframeCache: next };
}),
gsapAnimations: new Map(),
setGsapAnimations: (elementId, animations) =>
set((state) => {
const next = new Map(state.gsapAnimations);
if (animations) next.set(elementId, animations);
else next.delete(elementId);
return { gsapAnimations: next };
}),Both setters always clone the Map and emit a new reference, even when data === undefined && !state.keyframeCache.has(elementId) (or incoming value is strictly equal to current). Every usePlayerStore(s => s.keyframeCache) subscriber re-renders on no-op. clearKeyframeCacheForElement already guards with has, but writeGsapAnimationsForElement does not, and every write path pays allocation cost. With slice now serving multiple hot writers (per-element effect, file populate, post-commit updater, delete), wasted renders add up.
Fix: Short-circuit inside setter: if data === state.keyframeCache.get(elementId) (identity) or, for undefined branch, if !state.keyframeCache.has(elementId), return state unchanged.
An ungrouped tween (mixed property groups classify to propertyGroup undefined) fed keyframeCache but was skipped by every gsapAnimations writer, so the collapsed row drew diamonds the expanded lanes had no source animation to render. Drop the property-group gate at all three writers; lane consumers already filter by group. Also route the same-percentage merge in updateKeyframeCacheFromParsed through deduplicateKeyframes so the easeAmbiguous rule has one owner.
3 participants
What
Adds the shared Studio state and cache primitives for keyframe-aware timeline authoring.
Why
Timeline lanes need one canonical source for discovered GSAP keyframes, selection identity, and edit state before rendering or interaction layers can safely consume them.
How
Introduces focused keyframe state/cache helpers and hooks, with identity and state transitions covered independently. This is B1 of the Family B draft Graphite stack and targets the immutable PR-less Family A review baseline.
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 6 remaining low/nit findings are parked, verbatim, in
.scratch/studio-timeline-family-b/issues/01-pr-2683-deferred-review-findings.md:packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:22— id extraction regex /^#([\w-]+)/ isn't paired with the new idSelector attribute-selector outputpackages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:77— Synthesized flat-tween's top-level ease is dropped when written into keyframeCachepackages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:59— Ambiguity-flag logic is inlined identically in two places — contract drift risk across foundation slicepackages/studio/src/player/store/keyframeSlice.ts:36— expandClips has no bulk-collapse counterpartpackages/studio/src/hooks/useStudioTestHooks.ts:45— window.__studioTest is reassigned every effect re-run and stomps on any prior handlepackages/studio/src/player/store/keyframeSlice.ts:26— selectedKeyframes stores two disjoint key shapes with no discriminator or helperSupersedes #2683, 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:
gsapAnimationsno longer disagree on which tweens they admit. The property-group gate is gone from both writers, so a mixed-group tween ({ x, opacity }classifies to no group) can no longer cache diamonds with no source animation behind them.Not changed, with rationale:
replaceSelectedKeyframes/selectRangeKeyframesare skipped as YAGNI. No consumer exists in the stack and the unused-exports gate would flag both.timelineKeyframeIdentity.ts(fix(studio): harden keyframe editing semantics #2787).The 3 remaining low findings are parked in
.scratch/studio-timeline-family-b/issues/09-family-b-v2-r1-deferred.md.