diff --git a/packages/studio/src/components/sidebar/LeftSidebar.storage.test.ts b/packages/studio/src/components/sidebar/LeftSidebar.storage.test.ts new file mode 100644 index 000000000..7e4cf8501 --- /dev/null +++ b/packages/studio/src/components/sidebar/LeftSidebar.storage.test.ts @@ -0,0 +1,28 @@ +// @vitest-environment happy-dom + +import { afterEach, expect, it, vi } from "vitest"; +import { getPersistedTab } from "./LeftSidebar"; + +vi.mock("../../utils/studioTelemetry", () => ({ trackStudioEvent: vi.fn() })); + +const originalDescriptor = Object.getOwnPropertyDescriptor(globalThis, "localStorage"); + +afterEach(() => { + if (originalDescriptor) Object.defineProperty(globalThis, "localStorage", originalDescriptor); +}); + +it("falls back to the default tab when localStorage is blocked", () => { + // Chrome throws on the property read itself when site data is blocked for + // the document. This runs as a useState initializer, so throwing here takes + // Studio to the crash boundary. + Object.defineProperty(globalThis, "localStorage", { + configurable: true, + get() { + throw new Error( + "Failed to read the 'localStorage' property from 'Window': Access is denied for this document.", + ); + }, + }); + + expect(getPersistedTab()).toBe("compositions"); +}); diff --git a/packages/studio/src/components/sidebar/LeftSidebar.tsx b/packages/studio/src/components/sidebar/LeftSidebar.tsx index b27edfb20..cd2420808 100644 --- a/packages/studio/src/components/sidebar/LeftSidebar.tsx +++ b/packages/studio/src/components/sidebar/LeftSidebar.tsx @@ -10,6 +10,7 @@ import { import { CompositionsTab } from "./CompositionsTab"; import { AssetsTab } from "./AssetsTab"; import { trackStudioEvent } from "../../utils/studioTelemetry"; +import { safeLocalStorage } from "../../utils/safeStorage"; import { BlocksTab, type BlockPreviewInfo } from "./BlocksTab"; import { FileTree } from "../editor/FileTree"; import { Tooltip } from "../ui"; @@ -23,8 +24,18 @@ export interface LeftSidebarHandle { const STORAGE_KEY = "hf-studio-sidebar-tab"; -function getPersistedTab(): SidebarTab { - const stored = localStorage.getItem(STORAGE_KEY); +// Both the `localStorage` reference and `getItem` itself can throw when the +// browsing context is partitioned or site data is blocked — the same case +// telemetry/config.ts documents. This runs as a `useState` initializer, so an +// unguarded throw here takes the whole editor to the crash boundary rather +// than losing one remembered tab. +export function getPersistedTab(): SidebarTab { + let stored: string | null = null; + try { + stored = safeLocalStorage()?.getItem(STORAGE_KEY) ?? null; + } catch { + /* storage unavailable — fall back to the default tab */ + } if (stored === "assets") return "assets"; if (stored === "code") return "code"; if (stored === "blocks") return "blocks"; @@ -104,7 +115,11 @@ export const LeftSidebar = memo( const selectTab = useCallback((t: SidebarTab) => { setTab(t); - localStorage.setItem(STORAGE_KEY, t); + try { + safeLocalStorage()?.setItem(STORAGE_KEY, t); + } catch { + /* storage unavailable — the tab just won't be remembered */ + } trackStudioEvent("tab_switch", { panel: "left_sidebar", tab: t }); }, []); diff --git a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts index 10415f1f6..2c2ff4fc4 100644 --- a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts +++ b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts @@ -1,12 +1,16 @@ -import { describe, it, expect, beforeEach } from "vitest"; +import { describe, it, expect, beforeEach, vi } from "vitest"; import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; import { usePlayerStore, type KeyframeCacheEntry } from "../player/store/playerStore"; import { clearKeyframeCacheForElement, + elementCacheKeys, pruneKeyframeCacheToFiles, replaceKeyframeCacheForFile, updateKeyframeCacheFromParsed, } from "./gsapKeyframeCacheHelpers"; +import { trackStudioEvent } from "../utils/studioTelemetry"; + +vi.mock("../utils/studioTelemetry", () => ({ trackStudioEvent: vi.fn() })); const entry = (): KeyframeCacheEntry => ({ format: "percentage", @@ -30,6 +34,48 @@ const animWithKeyframes = (id: string): GsapAnimation => ({ beforeEach(() => { usePlayerStore.setState({ keyframeCache: new Map(), gsapAnimations: new Map(), elements: [] }); + vi.mocked(trackStudioEvent).mockClear(); +}); + +describe("non-string cache keys", () => { + // `s.indexOf is not a function` in pruneKeyframeCacheToFiles, decoded from the + // released 0.7.90 bundle. Some producer reaches elementCacheKeys with a + // non-string id; the bare-id key was written through raw, so both maps ended + // up holding a key that prune's `key.indexOf("#")` cannot handle. + const badId = 42 as unknown as string; + + it("keeps every written key a string", () => { + expect(elementCacheKeys("comp.html", badId).every((k) => typeof k === "string")).toBe(true); + }); + + it("reports the offending value instead of swallowing it", () => { + elementCacheKeys("comp.html", badId); + + expect(trackStudioEvent).toHaveBeenCalledWith( + "cache_key_non_string", + expect.objectContaining({ + value_type: "number", + constructor_name: "Number", + source_file: "comp.html", + }), + ); + }); + + it("stays silent on the normal string path", () => { + elementCacheKeys("comp.html", "box"); + + expect(trackStudioEvent).not.toHaveBeenCalled(); + }); + + it("survives a prune after a write with a non-string id", () => { + replaceKeyframeCacheForFile( + "stale.html", + new Map([[badId, entry()]]), + new Map([[badId, [animWithKeyframes("box")]]]), + ); + + expect(() => pruneKeyframeCacheToFiles(["kept.html"])).not.toThrow(); + }); }); describe("clearKeyframeCacheForElement", () => { diff --git a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts index b1d0db3cf..63a7c8506 100644 --- a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts +++ b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts @@ -4,6 +4,7 @@ */ import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; import { usePlayerStore, type KeyframeCacheEntry } from "../player/store/playerStore"; +import { trackStudioEvent } from "../utils/studioTelemetry"; import { resolveClipTimingBasis, resolveSelectorElementIds, toClipKeyframes } from "./gsapShared"; import { deduplicateKeyframes, @@ -226,11 +227,38 @@ export function scopedElementKey(element: { return `${element.sourceFile || "index.html"}#${element.id}`; } -/** Every cache key a write for this element sets, in read-preference order. */ +/** + * The one gate every cache write passes through, so it is also the one place + * that can guarantee `keyframeCache` / `gsapAnimations` really are keyed by + * string the way their types claim. + * + * Two of the three keys are template literals, which coerce on their own. The + * bare-id key was passed through raw, so a non-string `elementId` reaching here + * put a non-string key in both maps — and `pruneKeyframeCacheToFiles` then threw + * `s.indexOf is not a function` on it, taking Studio to the crash boundary. + * + * Which caller supplies a non-string id is still unknown: every writer traced + * from here produces a string. So this coerces rather than guesses, and reports + * the offending value's shape instead of swallowing it — the next occurrence + * names its own producer. + */ export function elementCacheKeys(sourceFile: string, elementId: string): string[] { + const id = typeof elementId === "string" ? elementId : coerceCacheKeyId(elementId, sourceFile); return sourceFile === "index.html" - ? [`index.html#${elementId}`, elementId] - : [`${sourceFile}#${elementId}`, `index.html#${elementId}`, elementId]; + ? [`index.html#${id}`, id] + : [`${sourceFile}#${id}`, `index.html#${id}`, id]; +} + +function coerceCacheKeyId(elementId: unknown, sourceFile: string): string { + trackStudioEvent("cache_key_non_string", { + value_type: typeof elementId, + // An Element lands here as "HTMLDivElement", a boxed id as "Number" — enough + // to name the producer without shipping user content to telemetry. + constructor_name: (elementId as { constructor?: { name?: string } })?.constructor?.name ?? null, + is_array: Array.isArray(elementId), + source_file: sourceFile, + }); + return String(elementId); } /** Replace one file's complete cache snapshot with one atomic store publish. */ diff --git a/packages/studio/src/hooks/useGsapTweenCache.ts b/packages/studio/src/hooks/useGsapTweenCache.ts index cc16648ef..9b3a8aa22 100644 --- a/packages/studio/src/hooks/useGsapTweenCache.ts +++ b/packages/studio/src/hooks/useGsapTweenCache.ts @@ -4,6 +4,7 @@ import { usePlayerStore } from "../player/store/playerStore"; import { readRuntimeKeyframes, scanAllRuntimeKeyframes } from "./gsapRuntimeBridge"; import { clearKeyframeCacheForElement, + elementCacheKeys, pruneKeyframeCacheToFiles, publishKeyframeCache, writeGsapAnimationsForElement, @@ -302,8 +303,9 @@ export function useGsapAnimationsForElement( // scan already cached. Only clear when no source cached this element — // otherwise selecting it would wipe its diamonds. const { keyframeCache } = usePlayerStore.getState(); - const hasCached = - keyframeCache.has(`${sourceFile}#${elementId}`) || keyframeCache.has(elementId); + const hasCached = elementCacheKeys(sourceFile, elementId).some((key) => + keyframeCache.has(key), + ); if (!hasCached) clearKeyframeCacheForElement(sourceFile, elementId); return; } @@ -314,14 +316,16 @@ export function useGsapAnimationsForElement( ...(ease ? { ease } : {}), ...(easeEach ? { easeEach } : {}), }; - // PropertyPanel reads the cache by bare elementId (without sourceFile - // prefix), so the same entry is written under the bare key for - // cross-component lookups. Both keys land in one publish: a reader that woke - // between two separate writes saw the prefixed key updated and the bare one - // still stale. + // elementCacheKeys owns the key-variant list every writer sets (prefixed, + // index.html fallback, bare id). Building it by hand here is what let this + // site drift: it omitted the fallback key, and it wrote the bare id without + // the string coercion that keeps prune from throwing on a non-string. All + // keys land in one publish: a reader that woke between two separate writes + // saw the prefixed key updated and the bare one still stale. publishKeyframeCache((draft) => { - draft.keyframeCache.set(`${sourceFile}#${elementId}`, merged); - draft.keyframeCache.set(elementId, merged); + for (const key of elementCacheKeys(sourceFile, elementId)) { + draft.keyframeCache.set(key, merged); + } }); // eslint-disable-next-line react-hooks/exhaustive-deps }, [elementId, sourceFile, animations, domClipChildrenKey]); @@ -426,13 +430,8 @@ export function usePopulateKeyframeCacheForFile( // in between re-rendered against a cache only partly filled in. publishKeyframeCache((draft) => { for (const [id, data] of scanned) { - const cacheKey = `${sf}#${id}`; - const fallbackKey = `index.html#${id}`; - const alreadyCached = - draft.keyframeCache.has(cacheKey) || - draft.keyframeCache.has(fallbackKey) || - draft.keyframeCache.has(id); - if (alreadyCached) continue; + const keys = elementCacheKeys(sf, id); + if (keys.some((key) => draft.keyframeCache.has(key))) continue; // Skip position-only set tweens from runtime too, same filter as AST path const isPosOnly = data.keyframes.length === 1 && @@ -445,9 +444,7 @@ export function usePopulateKeyframeCacheForFile( keyframes: data.keyframes, ...(data.easeEach ? { easeEach: data.easeEach } : {}), }; - draft.keyframeCache.set(cacheKey, entry); - if (sf !== "index.html") draft.keyframeCache.set(fallbackKey, entry); - draft.keyframeCache.set(id, entry); + for (const key of keys) draft.keyframeCache.set(key, entry); } }); runtimeScanDoneRef.current = `kf-cache:${projectId}:${sf}:${version}`; diff --git a/packages/studio/src/player/components/Player.test.ts b/packages/studio/src/player/components/Player.test.ts index 444f0199d..07e7df14e 100644 --- a/packages/studio/src/player/components/Player.test.ts +++ b/packages/studio/src/player/components/Player.test.ts @@ -100,6 +100,19 @@ describe("preview errors", () => { ); }); + it("unmounts cleanly when the player element is already detached", async () => { + const { player } = await mountPlayer(); + + // A container re-render, a crossfade swap, or a page-translation extension + // can detach the element before React tears the Player down. Cleanup must + // not throw NotFoundError — the error boundary turns that into a + // full-screen "Something went wrong". + player.remove(); + + expect(() => act(() => root?.unmount())).not.toThrow(); + root = null; + }); + it("attaches lifecycle listeners before navigating the player", async () => { await mountPlayer(); const srcIndex = lifecycleLog.indexOf("src"); diff --git a/packages/studio/src/player/components/Player.tsx b/packages/studio/src/player/components/Player.tsx index 15602f07b..c08fb4c33 100644 --- a/packages/studio/src/player/components/Player.tsx +++ b/packages/studio/src/player/components/Player.tsx @@ -295,7 +295,13 @@ export const Player = forwardRef( player.removeEventListener("error", handleError); if (assetPollRef.current) clearInterval(assetPollRef.current); assetPollRef.current = null; - container.removeChild(player); + // `remove()` rather than `container.removeChild(player)`: by the time + // this cleanup runs the element may already be detached — React can + // re-render the container, a crossfade refresh can swap it, or a + // translation/extension can reparent it. `removeChild` then throws + // NotFoundError, which the error boundary turns into a full-screen + // "Something went wrong". `remove()` is a no-op when already detached. + player.remove(); if (retryPreviewRef.current === retryPreview) retryPreviewRef.current = null; // Clear the forwarded ref only if it still points to THIS iframe. // During crossfade refreshes the retiring Player unmounts after the diff --git a/packages/studio/src/utils/clipboard.ts b/packages/studio/src/utils/clipboard.ts index 449603fd0..358a6e61d 100644 --- a/packages/studio/src/utils/clipboard.ts +++ b/packages/studio/src/utils/clipboard.ts @@ -26,7 +26,7 @@ function copyWithSelection(text: string): boolean { } catch { return false; } finally { - document.body.removeChild(textarea); + textarea.remove(); } }