fix(studio): publish keyframe cache refresh atomically

[miguel-heygen]

What

Publish full-file keyframe cache refreshes as one atomic state update.

Why

Incrementally publishing a multi-animation refresh exposes transient partial state to the timeline and inspector. That can render stale or mismatched diamonds while an edit is committing.

How

• Build the complete refreshed cache first.
• Publish it once after all affected animations are resolved.
• Keep the cache update owner and side-effect count explicit.

This is C3 of the independent Family C draft Graphite stack.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (not applicable)

Validated on the exact Family C tip with 94 focused tests, the full Studio suite (2,885 passed; 18 todos), Studio Server files tests (67 passed), both package typechecks, oxfmt, oxlint, diff checks, file-size gates, and Fallow with zero introduced findings.

[miguel-heygen]

• fix(studio): switch keyframe ease modes optimistically #2695
• fix(studio): publish keyframe cache refresh atomically #2694 👈 (View in Graphite)
• feat(studio): bulk-edit easing for merged keyframes #2693
• fix(studio): target colliding keyframes exactly #2692
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[vanceingalls]

R1 review — via

Grade: A-
Overall: CORRECT

Thesis check: The PR replaces the multi-step clear-then-populate write path in populateKeyframeCacheFromAst with a single atomic Zustand setState. OLD (pre-PR keyframeCacheAstLoad.ts:83-113): clearKeyframeCacheForFile(sf) iterated cached ids and made N × 2-key setKeyframeCache/setGsapAnimations calls, then the merged-by-element loop emitted another setKeyframeCache per alias key plus a writeGsapAnimationsForElement per element — every intermediate emission was a live re-render surface for timeline diamonds and inspector. NEW (gsapKeyframeCacheHelpers.ts:189-215 + keyframeCacheAstLoad.ts:106): build nextKeyframeCache and nextGsapAnimations off one getState() snapshot, then usePlayerStore.setState({ keyframeCache: nextKeyframeCache, gsapAnimations: nextGsapAnimations }) publishes both maps in one Zustand notification round. Since Zustand setState is synchronous and both slice references land on the state atomically before any subscriber fires, cross-map coherence is preserved for every consumer. The new test at gsapKeyframeCacheHelpers.test.ts:158-209 asserts snapshots.length === 1 — a direct verification of the atomicity claim, not just the final state.

P0/P1 findings:

• None.

P2/P3 findings:

• [P2] pruneKeyframeCacheToFiles (packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:148-166) still calls clearKeyframeCacheForElement per stale element, which emits 2-4 per-key setStates per element. Called synchronously by useGsapTweenCache.ts:384 immediately before the atomic replacements. On composition switch that means the store passes through many intermediate partial states before any atomic file-replace lands.
  • Failure scenario: mid-switch, timeline diamonds and inspector observe the "prune half done" store; the atomic guarantee this PR introduces only covers the refresh window, not the switch window that precedes it. Sibling of the same anti-pattern (feedback_sweep_fix_grep_breadth_lens). Follow-up: fold prune into an atomic replaceKeyframeCacheAtomically(nextCache, nextAnimations) batch call, or piggyback the prune deletes onto the first file's atomic replace.
• [P2] updateKeyframeCacheFromParsed (packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:14-83) has the same clear-then-populate shape on the post-commit path: per-key setKeyframeCache + writeGsapAnimationsForElement per merged element, then per-target clearKeyframeCacheForElement for ids that lost their tween. Different trigger (single mutation instead of full AST refresh) but same non-atomic transient exposure. Not this PR's scope, but the fix ships with the anti-pattern documented alongside the fixed sibling — grep breadth follow-up.
• [P3] Dead export — clearKeyframeCacheForFile (gsapKeyframeCacheHelpers.ts:118-123) is now used only by its own test file. All production callers migrated to replaceKeyframeCacheForFile. Recommend deletion in a follow-up (or @deprecated marker) so the next reader can't reintroduce the non-atomic path by grepping for it.
• [P3] No-op still emits a Map clone — when cachedElementIdsForFile(sourceFile, ...) is empty and entries is empty (e.g. AST fetch returns zero animations for a file that also had no prior entries), the function still does new Map(keyframeCache) / new Map(gsapAnimations) and calls setState with fresh Map identities. Any subscriber using Object.is on the slice references re-renders for no diff. Cheap guard restores the old zero-emission behavior:

  const ids = cachedElementIdsForFile(...);
  if (ids.size === 0 && entries.size === 0) return;

  Pre-PR the analogous no-op was silent (clearKeyframeCacheForFile looped over zero ids; the merged-by-element loop iterated zero times → zero setState calls).

Nits:

• Test hygiene at gsapKeyframeCacheHelpers.test.ts:171-179 — the "other.html#other" / "other" seeds carry staleAnimation too, which reads as copy-paste from the stale block. otherAnimation = animWithKeyframes("other") communicates the "other file is untouched" invariant explicitly (currently only the key-set equality catches it).
• The KDoc on clearKeyframeCacheForFile (lines 141-149) still lives despite that helper being production-dead. If deletion is deferred, add a @deprecated Use replaceKeyframeCacheForFile marker so it's grep-visible.

Positive callouts:

• Extracted cachedElementIdsForFile (gsapKeyframeCacheHelpers.ts:126-138) — clean DRY between the pruning clear path and the atomic replace path, single source of truth for prefix-owned id enumeration.
• Alias-symmetric delete-then-set: the clear loop iterates elementCacheKeys(sourceFile, id) (three variants for non-index.html, two for index.html) — exactly the pattern the writer uses, so no stale bare-id / index.html#id alias survives the swap. Matches the invariant already asserted by clearKeyframeCacheForElement's KDoc at lines 84-95.
• Cloned maps built off one getState() snapshot with zero awaits between snapshot and setState — the atomicity claim is not just per-emission but also per-transaction (no reader interleaving possible under JS single-threaded semantics).
• Test asserts snapshots.length === 1 via a live subscribe — direct verification of the notification-count claim, not indirect final-state coverage. This is the right shape for an atomicity regression test.

Cache-coherence audit:

• Old pattern site: packages/studio/src/hooks/keyframeCacheAstLoad.ts:84-112 (pre-PR) — clearKeyframeCacheForFile(sf) + per-key setKeyframeCache loop + per-element writeGsapAnimationsForElement.
• New pattern site: packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:189-215 + call site packages/studio/src/hooks/keyframeCacheAstLoad.ts:106.
• Read-during-write closed? Yes. No await between the getState() snapshot at line 197 and setState at line 212; JS single-threaded semantics guarantee no interleaving. Two concurrent populateKeyframeCacheFromAst calls for different files (via Promise.all at useGsapTweenCache.ts:385) serialize their replaceKeyframeCacheForFile invocations, and each snapshots the post-prior-file state.
• Stale-read via useMemo / useState avoided? Yes for AST refresh path — both slice references update in the same setState, so any selector reading { keyframeCache, gsapAnimations } sees them coherent on the next render. Prune path remains non-coherent (see P2 above).
• Peer cache surfaces (grep breadth): updateKeyframeCacheFromParsed (post-commit refresh, same file), pruneKeyframeCacheToFiles (composition switch), setKeyframeCache / setGsapAnimations slice setters (per-key writers still callable by any importer). Two of those (prune, updateKeyframeCacheFromParsed) share the shape this PR fixed; the slice setters remain the granular per-key API and are still correct in that role.

Standards-lens mechanical:

• Named exports only: pass (all new exports named).
• import type used for type-only imports: pass (KeyframeCacheEntry uses type at line 6, GsapAnimation uses type at line 5).
• No barrel: pass (direct file import at keyframeCacheAstLoad.ts:9).
• No default export: pass.
• No inline style: n/a.
• No hardcoded colors: n/a.
• File size gates: gsapKeyframeCacheHelpers.ts at 248 lines, under the 600-line threshold documented in the module KDoc.

CI note (non-blocking for this PR's content): Preflight (lint + format) is red on packages/studio/src/hooks/useDomEditSession.ts — a file not touched by this PR (git diff <base>..2694 -- packages/studio/src/hooks/useDomEditSession.ts is empty). Downstack contamination from the codex/studio-timeline-c-bulk-easing-v2 base. The stack needs an oxfmt sweep before merge, but the content of #2694 is not the source of the format failure.

— Review by Via

[james-russo-rames-d-jusso]

Reviewed at 23a8103f.

The atomic-publish contract lands cleanly and the new replaceKeyframeCacheForFile at packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:189-216 composes correctly with the existing alias semantics:

• Delete loop reuses cachedElementIdsForFile (extracted here from the old clearKeyframeCacheForFile body) and drops all three elementCacheKeys aliases per id, then the write loop restores the same three keys with fresh entries. Behavioral parity with the old clear-then-write path — verified against elementCacheKeys("index.html", …) (two-key path) and non-index.html files (three-key path) both handle deletes and writes symmetrically.
• The test at gsapKeyframeCacheHelpers.test.ts:158 pins the whole invariant with one subscribe-and-count on the store, which is exactly the assertion this refactor exists to make: one notification, not N.
• Missing-in-entries elements are correctly evicted (delete loop sees them via cachedElementIdsForFile; write loop only restores what entries names), so an element whose keyframes are removed leaves no stale entry behind. Confirmed with a manual trace of {elemA, elemB} cached → scan returns only elemA.

The call-site simplification in packages/studio/src/hooks/keyframeCacheAstLoad.ts:104 (clearKeyframeCacheForFile(sf) + per-id writes → single replaceKeyframeCacheForFile(sf, mergedByElement, sourceByElement)) is the payoff and is well-scoped.

Concerns

• pruneKeyframeCacheToFiles (packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:148) is still non-atomic — it iterates clearKeyframeCacheForElement per (file, id) pair, each of which does two setState calls. It runs immediately before populateKeyframeCacheFromAst in useGsapTweenCache.ts:384 on every composition switch, so subscribers still see a partial-eviction window right before the atomic populate lands. Not a regression from this PR, but the "publish once" claim in the description is only true per-file per-populate; the surrounding scan path still has the smaller-scale version of the exact race this fix targets. Extending the atomic-publish pattern to pruneKeyframeCacheToFiles would complete the story.

What I didn't verify

• Concurrent multi-file scans: Promise.all(files.map(populateKeyframeCacheFromAst)) fires each publish in parallel, and each publish reads the store then writes a whole new pair of Maps. Because replaceKeyframeCacheForFile itself is synchronous, the read-then-write is atomic within one microtask, and each publish only touches its own file's keys — so I believe cross-file races are safe, but I didn't exhaustively work through interleavings with useGsapKeyframeOps writers that might land in between.

— Review by Rames D Jusso

[miguel-heygen]

Both follow-ups are fixed here rather than deferred.

Every multi-key cache writer now publishes through one helper:

export function publishKeyframeCache(edit: (draft: KeyframeCacheDraft) => void): void

It clones both maps once, hands the caller the drafts, and writes the store once. pruneKeyframeCacheToFiles and updateKeyframeCacheFromParsed both route through it, as do the two per-element write sites in useGsapTweenCache.ts (the selected-element populate and the runtime-scan loop, which previously emitted up to three notifications per scanned element). The prune now drops every stale file in a single publish immediately before the atomic repopulate, so the "publish once" story holds across the whole composition switch, not just inside populateKeyframeCacheFromAst.

The no-op identity leak is fixed in the same place: the helper compares both maps entry by entry after the edit and skips the store write when nothing changed, which is the guard the per-key setters used to carry individually. A prune that finds nothing stale hands subscribers the same Map back.

clearKeyframeCacheForFile is deleted along with its tests, production-dead as you found.

Two tests added: prune drops every stale file in exactly one notification, and a prune with nothing stale produces zero notifications and the identical Map instance.

The oxfmt --check failure is fixed in #2693, where the file it flagged is changed.

[james-russo-rames-d-jusso]

Delta-reviewed 23a8103f..0165b3a4.

Prior concern fully addressed and the fix is broader than what I asked for. The new publishKeyframeCache(edit) primitive at packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:36-46 centralises the atomic-publish pattern so every cache mutator goes through it, and the identity check (sameEntries) means an edit that changed nothing does not re-render every subscriber — the guard the per-key setters used to carry individually is now shared:

• pruneKeyframeCacheToFiles (line 205): the raison d'être of my prior concern. One publish for the whole prune. New test at gsapKeyframeCacheHelpers.test.ts:117 pins it (subscribes and counts, expects notifications === 1), and the no-publish-when-unchanged case gets its own test at line 138.
• clearKeyframeCacheForElement (line 165): now one publish per element instead of two setStates (keyframeCache + gsapAnimations).
• updateKeyframeCacheFromParsed (line 61): writes + clears batched into one publish.
• replaceKeyframeCacheForFile (line 250): re-expressed in terms of publishKeyframeCache with no behavior change (verified by matching the delete-then-set loop against the prior imperative version).
• writeGsapAnimationsForElement (line 264): now routes through the primitive.
• Bonus race Miguel found while unifying the paths: the prefixed + bare double-write in useGsapAnimationsForElement (useGsapTweenCache.ts:317) and the runtime-scan-all publish (useGsapTweenCache.ts:407) — both were separate setKeyframeCache calls that a reader could interleave between. Both now land in one publish.
• clearKeyframeCacheForFile removed entirely, along with its tests, which is the right call — no callers left after the C3 unification.

The sameEntries short-circuit is worth naming: it does reference-equality on values, so a caller who replaces an entry with a semantically-equal-but-different-reference object still counts as a change (safe direction — no missed updates, at the cost of an occasional needless publish). Behavior matches the per-key setter's prior data === state.keyframeCache.get(elementId) check exactly.

What I didn't verify

• Concurrent-writer interleavings between publishKeyframeCache calls that share a microtask window: because the read-then-mutate-then-setState is synchronous inside one publish, I don't see a race window, but I didn't exhaustively work through the case where two useEffect callbacks land in the same commit and both publish.

— Review by Rames D Jusso

[vanceingalls]

R2 review — via

HEAD verified: 0165b3a4 (R1 target 23a8103f was rewritten via --force-with-lease; the one commit at HEAD carries every change).
Grade: A
Overall: APPROVE

R1 findings delta:

• [P2 pruneKeyframeCacheToFiles per-key setState] — RESOLVED. packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:193-213 now routes through the new publishKeyframeCache helper (line 208): stale ids are collected across every non-kept file in one pass, then a single edit callback drops each (file, id) via deleteElementFromDraft on the draft maps, and the outer helper publishes once. The composition-switch window is now covered by the same atomic invariant the AST refresh introduced. Test at packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts:118-133 subscribes-and-counts across a 4-key stale set with 3 stale files → notifications === 1 (was N per stale id pre-fix).
• [P2 updateKeyframeCacheFromParsed per-key setState] — RESOLVED. packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:55-125 builds merged/sourceAnimations off one getState() snapshot, then one publishKeyframeCache (line 117) writes every merged element and evicts stale target ids in a single edit callback via writeElementIntoDraft / deleteElementFromDraft. The post-commit path now has the same atomic shape as the AST refresh.
• [P3 dead export clearKeyframeCacheForFile] — RESOLVED. The function and its three-case test block are fully deleted. grep -n clearKeyframeCacheForFile across the four PR files returns nothing. keyframeCacheAstLoad.ts:9 now imports only replaceKeyframeCacheForFile.
• [P3 no-op emission flips Map identity] — RESOLVED. The publishKeyframeCache helper at packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:44-48 walks both slice pairs with sameEntries (gsapKeyframeCacheHelpers.ts:20-26 — size + per-key identity) and returns before setState when nothing changed. This becomes the standing no-op guard for every call site — pruneKeyframeCacheToFiles with no stale, updateKeyframeCacheFromParsed with nothing to write, clearKeyframeCacheForElement on an absent element — none of them touch store identity. Test at gsapKeyframeCacheHelpers.test.ts:137-150 pins both signals: notifications === 0 and cache() === before (Map identity preserved), which was the exact regression I couldn't have caught with a keys-only check.

Rames' concerns (PRR_kwDORi26-c8AAAABHkDXrg):

• pruneKeyframeCacheToFiles non-atomic follow-up — aligned with my P2#1; RESOLVED as above.
• Concurrent multi-file scans (Rames flagged as "not verified") — verified here. publishKeyframeCache snapshots the store at edit time, mutates a draft synchronously, and setStates in one microtask; two concurrent populateKeyframeCacheFromAst calls via Promise.all at useGsapTweenCache.ts:390 each go through replaceKeyframeCacheForFile which serializes on the JS event loop. Because cachedElementIdsForFile (gsapKeyframeCacheHelpers.ts:169-181) only enumerates keys with the file's own prefix and elementCacheKeys only writes the file's prefix + fallback + bare aliases, cross-file publishes touch disjoint key sets and each snapshots the post-prior-file state — no lost writes, no cross-file interleaving. useGsapKeyframeOps-writer interleavings land on their own file's prefix and stay coherent for the same reason.

Fresh-pass findings (12-lens editor-UI set on publishKeyframeCache + reworked call sites):

• None P0/P1/P2. The helper is clean: single getState() read, synchronous edit callback (no awaits possible in the caller between snapshot and publish because edit is a sync function), identity-based sameEntries guard (correctly detects new-object writes as diff via reference inequality), disjoint delete+write ordering inside replaceKeyframeCacheForFile (delete first, then write — so a re-appearing key survives).
• P3 nit — two-publish window on selected-element effect. useGsapTweenCache.ts:259 (writeGsapAnimationsForElement) and useGsapTweenCache.ts:322-325 (the publishKeyframeCache for keyframeCache) fire back-to-back synchronously in the same effect body. Both go through publishKeyframeCache now, so each is atomic within itself, and the net notification count dropped from ~5 (per pre-PR shape) to 2 — but a strict single-publish reading of "publish once" is 1, not 2. A subscriber that reads {keyframeCache, gsapAnimations} observes gsapAnimations updated with keyframeCache one publish behind between them. Not a regression (pre-PR had the same split shape at higher granularity) and not this PR's headline claim (which is per-file-refresh atomicity), so it's a follow-up tighten-up: fold the gsapAnimations write into the same publishKeyframeCache edit callback as the keyframeCache writes and issue one publish per selection.

Positive callouts:

• publishKeyframeCache (gsapKeyframeCacheHelpers.ts:37-53) is a strong primitive: draft-mutation API keeps call sites readable, the identity-guard lives in one place, and every multi-key writer routes through it. The new KeyframeCacheDraft interface makes the "both maps mid-edit" contract explicit at the type level.
• Both new tests target the invariant, not the state: pruneKeyframeCacheToFiles gets both a "one notification for N stale files" test and a "zero notifications + same Map identity" test. That's the shape that catches future regressions where someone re-splits the publish.
• pruneKeyframeCacheToFiles doc at gsapKeyframeCacheHelpers.ts:183-192 explicitly names the "empty-cache flash that repopulate was made to avoid" — the fix and its rationale are grep-visible for the next reader.
• updateKeyframeCacheFromParsed refactor (gsapKeyframeCacheHelpers.ts:55-125) tightened the delete-of-orphan-targets path: targetIds now derives from mutation selector or selectionId and only entries missing from idsWithKeyframes are evicted — inside the same draft as the writes, so the mutation-that-removes-a-keyframe path is atomic too.

— Review by Via

The base branch was changed.