From d05ecb10910ec463370a3561d3b3868b5e94b287 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Sun, 26 Jul 2026 02:13:38 +0200 Subject: [PATCH] fix(studio): scope the per-file keyframe-cache clear to its own keys R3 review follow-ups on the keyframe cache: - clearKeyframeCacheForFile collected ids from the index.html alias prefix too, so a re-scan of one composition file wiped rows a sibling file had just written (several files re-scan concurrently). Only the file's own prefixed keys name the ids now; clearKeyframeCacheForElement still takes the alias and bare key with them. - toClipKeyframes fell back to a fixed 1s tween duration, which put a duration-less tween's keyframes at a percentage no edit path agreed with. It now spans the clip, matching resolveEditableTweenDuration. - collectAnimatableKeyframeProperties takes `object` so call sites drop their `as Record` casts. Regression tests cover both fixes. --- .../hooks/gsapKeyframeCacheHelpers.test.ts | 14 +++++++++++ .../src/hooks/gsapKeyframeCacheHelpers.ts | 20 ++++++++-------- packages/studio/src/hooks/gsapShared.test.ts | 24 +++++++++++++++++++ packages/studio/src/hooks/gsapShared.ts | 13 +++++----- 4 files changed, 55 insertions(+), 16 deletions(-) diff --git a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts index 7ac1ac2ce..9bc45eec2 100644 --- a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts +++ b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts @@ -84,6 +84,20 @@ describe("clearKeyframeCacheForFile", () => { } }); + // Several composition files re-scan concurrently, so a clear that walked the + // index.html alias would delete rows a sibling file had just written. + it("leaves an index.html-owned element alone when another file re-scans", () => { + seed("index.html#title"); + seed("title"); + seed("comp.html#a"); + + clearKeyframeCacheForFile("comp.html"); + + expect(cache().has("index.html#title")).toBe(true); + expect(cache().has("title")).toBe(true); + expect(cache().has("comp.html#a")).toBe(false); + }); + it("leaves entries that belong to a different source file", () => { seed("comp.html#a"); seed("a"); diff --git a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts index d6a1e3a82..f9953f9fc 100644 --- a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts +++ b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts @@ -92,22 +92,22 @@ export function clearKeyframeCacheForElement(sourceFile: string, elementId: stri /** * Clear every cached element of `sourceFile` before a full re-scan repopulates - * it. Collects the element ids that currently have a prefixed or index.html - * fallback key for the file and drops each through clearKeyframeCacheForElement - * so the bare key goes too — an element whose keyframes were removed (and so is - * absent from the re-scan) leaves no stale bare entry behind. + * it. Only the file's OWN prefixed keys name the ids to clear: every write sets + * the prefixed key (see elementCacheKeys), so the file's elements are all + * reachable that way, and clearKeyframeCacheForElement then takes the + * index.html alias and the bare key with them — an element whose keyframes were + * removed (and so is absent from the re-scan) leaves no stale bare entry + * behind. Reading the alias prefix here instead would collect ids owned by + * OTHER files, and several files re-scan concurrently, so this file's clear + * would wipe the entries a sibling file had just written. */ export function clearKeyframeCacheForFile(sourceFile: string): void { const { keyframeCache, gsapAnimations } = usePlayerStore.getState(); const sfPrefix = `${sourceFile}#`; - const fallbackPrefix = "index.html#"; const ids = new Set(); for (const key of [...keyframeCache.keys(), ...gsapAnimations.keys()]) { - const matchesFile = - key.startsWith(sfPrefix) || (sourceFile !== "index.html" && key.startsWith(fallbackPrefix)); - if (!matchesFile) continue; - const hashIdx = key.indexOf("#"); - if (hashIdx !== -1) ids.add(key.slice(hashIdx + 1)); + if (!key.startsWith(sfPrefix)) continue; + ids.add(key.slice(sfPrefix.length)); } for (const id of ids) { clearKeyframeCacheForElement(sourceFile, id); diff --git a/packages/studio/src/hooks/gsapShared.test.ts b/packages/studio/src/hooks/gsapShared.test.ts index 1d3fc8f36..144dcfb7c 100644 --- a/packages/studio/src/hooks/gsapShared.test.ts +++ b/packages/studio/src/hooks/gsapShared.test.ts @@ -4,6 +4,7 @@ import { idSelector, isInstantHold, parsePercentageKeyframes, + toClipKeyframes, toClipPercentage, } from "./gsapShared"; @@ -122,3 +123,26 @@ describe("toClipPercentage", () => { expect(toClipPercentage(5, 0, 0, 42)).toBe(42); }); }); + +describe("toClipKeyframes", () => { + const durationless: GsapAnimation = { + id: "a1", + method: "to", + targetSelector: "#box", + vars: {}, + resolvedStart: 0, + } as GsapAnimation; + + // A tween with no duration spans its clip everywhere else in Studio + // (resolveEditableTweenDuration), so the cache rows have to agree: a fixed 1s + // basis put the end keyframe at 25% of a 4s clip instead of 100%. + it("spans the clip when the tween has no duration", () => { + const rows = toClipKeyframes([{ percentage: 0 }, { percentage: 100 }], durationless, 0, 4); + expect(rows.map((row) => row.percentage)).toEqual([0, 100]); + }); + + it("keeps the tween percentage and the animation identity on every row", () => { + const rows = toClipKeyframes([{ percentage: 50 }], durationless, 0, 4); + expect(rows[0]).toMatchObject({ tweenPercentage: 50, animationId: "a1" }); + }); +}); diff --git a/packages/studio/src/hooks/gsapShared.ts b/packages/studio/src/hooks/gsapShared.ts index fdfcc0aae..4780d7407 100644 --- a/packages/studio/src/hooks/gsapShared.ts +++ b/packages/studio/src/hooks/gsapShared.ts @@ -145,9 +145,7 @@ export interface ParsedPercentageKeyframes { easeEach?: string; } -function collectAnimatableKeyframeProperties( - entry: Record, -): Record { +function collectAnimatableKeyframeProperties(entry: object): Record { const properties: Record = {}; for (const [property, value] of Object.entries(entry)) { if (property === "ease") continue; @@ -185,7 +183,7 @@ export function parsePercentageKeyframes( steps.forEach((entry, i) => { if (!entry || typeof entry !== "object") return; const percentage = steps.length > 1 ? Math.round((i / (steps.length - 1)) * 1000) / 10 : 0; - const properties = collectAnimatableKeyframeProperties(entry as Record); + const properties = collectAnimatableKeyframeProperties(entry); if (Object.keys(properties).length > 0) keyframes.push({ percentage, properties }); }); return keyframes.length > 0 ? { keyframes } : null; @@ -199,7 +197,7 @@ export function parsePercentageKeyframes( const pctMatch = key.match(/^(\d+(?:\.\d+)?)%$/); if (!pctMatch || !val || typeof val !== "object") continue; const percentage = parseFloat(pctMatch[1]); - const properties = collectAnimatableKeyframeProperties(val as Record); + const properties = collectAnimatableKeyframeProperties(val); if (Object.keys(properties).length > 0) { keyframes.push({ percentage, properties }); } @@ -253,7 +251,10 @@ export function toClipKeyframes( } > { const tweenStart = anim.resolvedStart ?? (typeof anim.position === "number" ? anim.position : 0); - const tweenDuration = anim.duration ?? 1; + // A duration-less tween spans the clip, the same rule the edit paths use + // (resolveEditableTweenDuration). A fixed 1s here put its keyframes at a + // percentage no editor agreed with. + const tweenDuration = anim.duration ?? clipDuration; return source.map((keyframe) => ({ ...keyframe, percentage: toClipPercentage(