fix(studio): stop three crash-boundary trips in the editor (#3102)

## What

Fixes three Studio crashes. All three throw into React and drop the user on the full-screen "Something went wrong" boundary.

**1. `NotFoundError: Failed to execute 'removeChild' on 'Node'`** — the highest-reach of the three. The `Player` mount effect appends a `<hyperframes-player>` into its container and tears it down with `container.removeChild(player)`. By the time that cleanup runs the element may already be detached: the container can re-render, a crossfade refresh can swap it, or a translation extension can reparent it. Switched to `player.remove()`, a no-op when the node has no parent. `utils/clipboard.ts` had the same unguarded `document.body.removeChild(textarea)` and is fixed with it — those are the only two `removeChild` call sites in non-vendor source.

**2. `SecurityError: Failed to read the 'localStorage' property from 'Window'`** — `getPersistedTab()` read `localStorage` unguarded and runs as a `useState` initializer. Chrome throws on the *property read itself* when site data is blocked for the document, so a profile with storage blocked lost the whole editor instead of one remembered tab. Routed through the existing `safeLocalStorage()` helper with the access guarded too, matching the pattern `telemetry/config.ts` documents. The `setItem` on tab switch was unguarded the same way and is fixed with it.

**3. `TypeError: s.indexOf is not a function`** — `pruneKeyframeCacheToFiles` calls `key.indexOf("#")` on a key that is not a string, though `keyframeCache` and `gsapAnimations` are both typed `Map<string, …>`.

## Why

None of the three loses real work — they are incidental teardown, persistence, and cache-pruning paths taking down the whole editor. The `removeChild` one reaches by far the most users.

## How

### Locating #3

The Studio build ships no sourcemaps, so the reported frame in a minified chunk was not traceable as-is. Checking out the `v0.7.90` tag and rebuilding it reproduces the same asset filename hash **byte-for-byte**, which confirms the rebuild is the same code the crash came from. Decoding the frame against that bundle lands on `gsapKeyframeCacheHelpers.ts:198`.

### Fixing #3

`elementCacheKeys` owns the key-variant list every cache write sets. Two of its three keys are template literals and coerce on their own; the bare-id key was passed through raw, so a non-string `elementId` reaching it put a non-string key into both maps, which prune then choked on. It now coerces that key.

Review caught that it was not yet the *only* write gate: `useGsapTweenCache` built the same key list by hand at two sites, so a non-string id there still reached the maps uncoerced. Both sites now loop `elementCacheKeys`, and their matching reads use the same list instead of a second hand-rolled copy. That also closes a drift the helper's own doc comment warns about — the per-element writer omitted the `index.html#<id>` fallback key its siblings all set, so a reader falling back to that key saw a stale entry. The only remaining direct writers are in the dev-only timeline performance fixture, which generates its own string ids.

The coercion **reports** the offending value's `typeof`, constructor name, and source file as `studio:cache_key_non_string` rather than swallowing it. This is deliberate: every writer that reaches `elementCacheKeys` was traced and each one produces a string, so **which caller supplies a non-string id is still unknown**. Rather than guess at a producer, this hardens the single gate that can guarantee the maps' declared contract, and makes the next occurrence name its own producer. Only the value's shape is reported, never its content.

Fixes 1 and 2 are both the smaller diff *and* the root fix: one guard where every caller routes through, rather than one per call site. No behaviour change on any happy path.

## Test plan

- [x] Unit tests added/updated
- [ ] Manual testing performed
- [ ] Documentation updated (if applicable)

Six regression tests, every one verified to fail without its fix:

- `Player.test.ts` — detaches the player element, then unmounts. Without the fix: `DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.`
- `LeftSidebar.storage.test.ts` — makes the `localStorage` property getter throw, then calls `getPersistedTab()`. Without the fix it fails with the same `SecurityError` the crash reports carry.
- `gsapKeyframeCacheHelpers.test.ts` — four cases: keys stay strings, the violation is reported, the normal string path stays silent, and a prune after a non-string write does not throw. Without the fix the last one fails with `TypeError: key.indexOf is not a function`.

Full Studio suite green: 3559 passed, 335 files, 0 failures. `oxlint`, `oxfmt` and `tsc --noEmit` clean.

Manual testing is unchecked deliberately: none of the three reproduces on a normal local profile, which is why they only surfaced in crash reports. The tests exercise the exact throwing boundaries instead.

## Not covered

Two other crash signatures reviewed alongside these are **not** fixed here: one occurs almost entirely on locally-built dev Studio rather than released builds, and the other has not appeared on any recent release.

**Follow-up worth its own PR:** ship sourcemaps for the Studio build. Rebuilding a tag to decode one frame worked, but it should not be the process, and it is the prerequisite for diagnosing the next minified crash.

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");
+});

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 });
     }, []);
 

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", () => {

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. */

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}`;

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");

packages/studio/src/player/components/Player.tsx

@@ -295,7 +295,13 @@ export const Player = forwardRef<HTMLIFrameElement, PlayerProps>(
           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

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();
   }
 }