feat(studio): add keyframe timeline state#2683
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 915a4e440226.
Foundation slice for Family B. Publishes 4 contracts (keyframeCache, gsapAnimations, focusedEaseSegment, expandedClipIds) that all 7 downstream PRs build on. Math + type surface read clean; the concerns cluster around silent invariants the type system doesn't enforce, which will propagate through B2-B8 consumers.
🟢 Verified clean
- keyframeSlice.ts setClipExpanded/expandClips have short-circuit
return stateguards; toggleClipExpanded copy-then-mutate for immutability - setGsapAnimations/setKeyframeCache use if(data) next.set else next.delete — undefined correctly removes rather than storing
undefined - KeyframeCacheEntry re-exported via playerStore.ts so external consumers keep working
- useStudioTestHooks gates on import.meta.env.DEV inside try/catch — Vite DCE strips in prod
- idSelector regex SAFE_HASH_ID rejects digit-leading / dotted / spaced ids — conservative bias toward safe attribute selector
- synthesizeFlatTweenKeyframes numeric-only guard correctly filters mixed-type flat tweens
- gsapDragCommit and useGestureCommit switch from #${sel.id} to idSelector(sel.id) — strict superset
- collectAnimatableKeyframeProperties refactor is pure extraction; both call sites' behaviour unchanged
- playerStore.test.ts new tests exercise expandedClipIds semantics (toggle, idempotent set)
- useStudioTestHooks import unconditional but useEffect fully guarded — prod bundle keeps a few dozen bytes, no PII, no window pollution outside DEV
Cross-stack notes
KeyframeSlice publishes 4 contracts B2-B8 build on: (1) keyframeCache = merged collapsed-diamond view; (2) gsapAnimations = unmerged per-tween view for expanded lanes; (3) focusedEaseSegment = scoped ease-editor target; (4) expandedClipIds = per-clip lane visibility. Two invariants NOT enforced by types and will silently propagate: (a) gsapAnimations and keyframeCache must be populated by the same filter (violated — see finding 1); (b) focusedEaseSegment.elementId is project-local but survives reset() (finding 2). selectedKeyframes stores two disjoint shapes with no discriminator (finding 6) — first B2/B3 write fixes convention for all future consumers. easeAmbiguous flag is inlined in two writers (finding 5) — a shared helper avoids drift. idSelector is a good defensive move but the cache-update regex (finding 3) doesn't consume its attribute-selector output — safety stops at parser boundary; B7/B8 mutation-response consumers should be aware.
| // fallow-ignore-next-line complexity | ||
| useEffect(() => { | ||
| if (!elementId) return; | ||
| const sourceAnimations = animations.filter( |
There was a problem hiding this comment.
🟠 useGsapAnimationsForElement writes sourceAnimations BEFORE the position-only zero-duration filter — gsapAnimations diverges from keyframeCache
useEffect(() => {
if (!elementId) return;
const sourceAnimations = animations.filter(
(animation) =>
animation.propertyGroup && (animation.keyframes || synthesizeFlatTweenKeyframes(animation)),
);
if (sourceAnimations.length > 0)
writeGsapAnimationsForElement(sourceFile, elementId, sourceAnimations);The sourceAnimations filter here is intentionally the same gate as the parallel path in usePopulateKeyframeCacheForFile (:494-500). BUT in this hook the write runs BEFORE the per-animation position-hold filter used to build allKeyframes (:365-371 — skips !anim.keyframes && all x/y-only && duration===0). The parallel path in usePopulateKeyframeCacheForFile places sourceByElement.set(...) INSIDE the loop, AFTER that same filter (:495-500). Result: a zero-duration tl.to('#el', { x: 0, duration: 0 }) (no immediateRender) is included in gsapAnimations here but produces no keyframeCache diamonds. Given the KeyframeSlice comment 'Unmerged source tweens per element; expanded property lanes read this, never keyframeCache' — B3's lane renderer will show a phantom lane for an animation the collapsed row has no diamond for. Foundation contract that B2-B8 consumers build on.
Fix: Move the sourceAnimations write to run only for anims that survive the same position-hold filter as the main loop — either inline the filter into the sourceAnimations gate or accumulate sourceAnimations inside the for (const anim of animations) loop after the continues, mirroring sourceByElement.set in usePopulateKeyframeCacheForFile.
| selectedElementIds: new Set(), | ||
| clipRevealRequest: null, | ||
| keyframeCache: new Map(), | ||
| gsapAnimations: new Map(), |
There was a problem hiding this comment.
🟠 reset() clears every keyframe slice member except focusedEaseSegment
reset: () =>
set({
...
activeTool: "select",
selectedKeyframes: new Set(),
expandedClipIds: new Set(),
selectedElementIds: new Set(),
clipRevealRequest: null,
keyframeCache: new Map(),
gsapAnimations: new Map(),
...
}),reset() is used on composition/project switch. It nulls every other keyframe-related slice member but leaves focusedEaseSegment set to whatever the prior project pointed at. The KeyframeSlice comment says elementId scopes the request to one element so a shared (class-selector) animation id can't open the ease editor on the wrong element — but that guarantee is per-project. If the new project happens to have an element with the same id (very common: hero, title, box) and an animation whose id starts with that prefix, the stale focusedEaseSegment would re-open the ease editor on the wrong element the instant B4-B6's consumer mounts. State-contract foot-gun for the 7 downstream PRs.
Fix: Add focusedEaseSegment: null, to the reset() object literal.
| 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.
🟡 id extraction regex /^#([\w-]+)/ isn't paired with the new idSelector attribute-selector output
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;This PR introduces idSelector (gsapShared.ts:81) which emits [id="01-hook-hero-word"] for digit-leading / dotted / spaced ids and is now used by gsapDragCommit.materializeIfDynamic, gsapScriptCommitHelpers.ensureElementAddressable, useGestureCommit.commit, and selectorFromSelection. When such a selector is written into the source file, the next parse returns anim.targetSelector = '[id="01-hook-hero-word"]'. The ^#([\w-]+) regex on :21 (and again on :91 for the mutation target) returns undefined for that shape, so the animation is silently skipped and the keyframe cache is never updated for that element. Same regex is unchanged, but the new idSelector primitive is a fresh source of selectors it can't parse — the two changes together create a latent bug for exactly the case idSelector is meant to make safe.
Fix: Extract id via a helper handling both shapes: sel.match(/^#([\w-]+)/)?.[1] ?? sel.match(/^\[id="((?:[^"\\]|\\.)+)"\]/)?.[1]?.replace(/\\(["\\])/g, '$1'). Cover the mutation-target site (:91) too.
| existing.keyframes = Array.from(byPct.values()).sort((a, b) => a.percentage - b.percentage); | ||
| } else { | ||
| merged.set(id, { ...anim.keyframes, keyframes: clipKeyframes }); | ||
| merged.set(id, { |
There was a problem hiding this comment.
🟡 Synthesized flat-tween's top-level ease is dropped when written into keyframeCache
} else {
merged.set(id, {
...anim.keyframes,
format: anim.keyframes?.format ?? "percentage",
keyframes: clipKeyframes,
});
}For a flat tween, anim.keyframes is undefined, so ...anim.keyframes is a no-op and only format + keyframes are set — the flat tween's ease (which synthesizeFlatTweenKeyframes returns at BOTH top level AND on the 100% keyframe, gsapTweenSynth.ts:88-92) is dropped from the entry-level ease. useGsapAnimationsForElement's parallel write (:400-405) DOES carry ease at the entry level. Contract mismatch: two writers, one produces { ease: 'power2.out', keyframes: [...] } for a flat tween, the other produces { keyframes: [...] } with the ease only on the 100% kf. B2-B8 consumers reading entry.ease will see it sometimes and not others.
Fix: Compute the source shape once: const src = anim.keyframes ?? synthesizeFlatTweenKeyframes(anim) above the kfSource extraction, then merged.set(id, { ...src, format: src?.format ?? 'percentage', keyframes: clipKeyframes }) for consistency with the useGsapTweenCache write.
| const prev = byPct.get(kf.percentage); | ||
| if (prev) { | ||
| prev.properties = { ...prev.properties, ...kf.properties }; | ||
| // Mirror deduplicateKeyframes: a same-% collision across different |
There was a problem hiding this comment.
🟡 Ambiguity-flag logic is inlined identically in two places — contract drift risk across foundation slice
if (
prev.animationId !== undefined &&
kf.animationId !== undefined &&
prev.animationId !== kf.animationId
) {
prev.easeAmbiguous = true;
}
if (kf.ease) prev.ease = kf.ease;Same 6-line ambiguity check appears in deduplicateKeyframes (gsapTweenSynth.ts:14-22). B2-B8 will add more consumers that merge cross-lane keyframes; implementation drift (tightening in one place, not the other) would silently show/hide the inline ease button inconsistently. Invariant is once ambiguous within a batch, always ambiguous — good, but it should be encoded once, not twice.
Fix: Extract a mergeKeyframeIntoExisting(existing, incoming) helper in gsapTweenSynth.ts that owns both the property-spread AND the ambiguity check, then reuse from both call sites. Also worth adding to the KeyframeCacheEntry TSDoc that easeAmbiguous is monotonic within a merge pass.
| } | ||
|
|
||
| export interface KeyframeSlice { | ||
| /** Selected collapsed (`element:pct`) or expanded (`element:group:animation:clipPct`) diamonds. */ |
There was a problem hiding this comment.
🔵 selectedKeyframes stores two disjoint key shapes with no discriminator or helper
/** Selected collapsed (`element:pct`) or expanded (`element:group:animation:clipPct`) diamonds. */
selectedKeyframes: Set<string>;
toggleSelectedKeyframe: (key: string) => void;
clearSelectedKeyframes: () => void;Set stores keys in TWO different string shapes (element:pct vs element:group:animation:clipPct) with only a colon-count discriminator. Nothing in the slice validates which shape is used at what site, and toggling between collapsed/expanded views of the same clip can leak stale keys of the other shape into the Set (a stale expanded key silently persists after a collapse). B2/B3 are the ones actually writing keys; without a helper or a typed union, they will imitate whichever shape happens to be in the reviewer's example.
Fix: Introduce a small buildSelectedKeyframeKey({ elementId, propertyGroup?, animationId?, percentage }) helper (with a matching parseSelectedKeyframeKey) exported from this slice, and have the collapsed vs expanded diamonds call it — the encoding then lives in one place instead of being re-inlined at every call site.
| toggleClipExpanded: (id: string) => void; | ||
| setClipExpanded: (id: string, expanded: boolean) => void; | ||
| /** Union-expand clips (keyframed clips are expanded by default on load). */ | ||
| expandClips: (ids: readonly string[]) => void; |
There was a problem hiding this comment.
🟢 expandClips has no bulk-collapse counterpart
/** Union-expand clips (keyframed clips are expanded by default on load). */
expandClips: (ids: readonly string[]) => void;Slice offers toggleClipExpanded, setClipExpanded, expandClips (bulk union), but no bulk collapse. B3/B4 UI adding a 'collapse all' button would iterate and call setClipExpanded N times, causing N re-renders (each writing a new Set). API asymmetry that will invite one of the downstream PRs to inline the wrong pattern.
Fix: Add collapseClips: (ids: readonly string[]) => void that removes ids in one setState with the same short-circuit as expandClips.
| applyDomSelection(selection, { revealPanel: true }); | ||
| return true; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🟢 window.__studioTest is reassigned every effect re-run and stomps on any prior handle
(window as unknown as { __studioTest?: typeof api }).__studioTest = api;
return () => {
(window as unknown as { __studioTest?: typeof api }).__studioTest = undefined;
};
}, [applyDomSelection, buildDomSelectionFromTarget, previewIframeRef]);Effect deps include applyDomSelection and buildDomSelectionFromTarget, which are useCallback'd. If any of their own deps flip, the effect re-runs, cleaning up (undefined-ing) and re-installing the api. A headless driver that resolved window.__studioTest.selectByDomId and called it after such a flip would observe undefined is not a function. Also unconditional = undefined on cleanup would clobber a co-mounted api.
Fix: Guard cleanup with if (window.__studioTest === api) window.__studioTest = undefined;. Consider stabilising via useMemo/useRef so the effect only runs once.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 915a4e440226.
Foundation slice — 4 store contracts (keyframeCache, gsapAnimations, focusedEaseSegment, expandedClipIds) that every downstream Family B slice consumes. Rames landed the cross-stack invariant audit; I verified the individual primitives read clean and want to record the ones I re-checked plus one small semantic worth naming.
Prior review
Rames D Jusso's R1 covered the four contracts and the cross-stack invariants (gsapAnimations vs keyframeCache filter alignment, focusedEaseSegment survival across reset, easeAmbiguous inlined in two writers, selectedKeyframes shape disjunction). Overlap: I re-verified idSelector, the numeric-only filter in synthesizeFlatTweenKeyframes, and the .set / .delete on if(data). All match.
🟢 Verified
idSelector'sSAFE_HASH_IDregex conservatively rejects digit-leading / dotted / whitespace ids and escapes quotes+backslashes in the attribute-selector value — call sites (gsapDragCommit,gsapScriptCommitHelpers,useGestureCommit) migrated consistently.synthesizeFlatTweenKeyframesnumeric-only filter (typeof rawStart[k] === "number" && typeof rawEnd[k] === "number") correctly returnsnullon pure non-numeric flat tweens (e.g.backgroundColor: "#fff"), matching the new test ingsapKeyframeCacheHelpers.test.ts.deduplicateKeyframes<T extends …>correctly flagseaseAmbiguousonly when two source animations collide at the same clip-% — same-animation collisions preserved (covered by test).keyframeSlice.tsextraction —playerStore.ts.resetcorrectly re-initializesexpandedClipIds,keyframeCache,gsapAnimations,selectedKeyframes. Live consumer contract preserved viaexport type { KeyframeCacheEntry } from "./keyframeSlice".useStudioTestHooksDEV gate is insidetry/catchfor theimport.meta.env.DEVread; cleanup zeros the window field. Vite DCE strips in prod.
🟢 Small semantic worth naming
writeGsapAnimationsForElement(sf, id, sourceAnimations.get(id)) at gsapKeyframeCacheHelpers.ts:108 is invoked for every id that produced a merged cache entry. If an id had no source animation with a propertyGroup — e.g. a re-parse that yields a keyframed tween whose parser hasn't classified the group yet — sourceAnimations.get(id) is undefined, and writeGsapAnimationsForElement then calls setGsapAnimations(key, undefined) which DELETES the existing entry. That's likely the intended semantic (an element that no longer contributes property-lane tweens should drop its gsapAnimations entry), but it means a transient parser state where propertyGroup is absent from an anim briefly empties the store slot until the next scan repopulates it. Not a bug; worth a code comment naming the invariant that "no propertyGroup ⇒ drop the entry" so a future refactor doesn't lose it.
Verdict
Foundation contracts read clean. Rames's cross-stack invariants are the load-bearing concerns — deferring blocker-tier judgment to his write-up.
— Review by Via
915a4e4 to
f499121
Compare
503372f to
3b3d4f5
Compare
The base branch was changed.
f499121 to
5dd41e9
Compare
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 helper