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

[miguel-heygen]

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

• 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.

[james-russo-rames-d-jusso]

Reviewed at 8a4211506.

Three-fix crash-boundary PR with careful diagnosis on each. Fixes 1 and 2 are proper root fixes (Node.remove() on cleanup that can race a reparent, and routing through safeLocalStorage() where property-read itself throws) — I'd land those unchanged. Fix 3's shape is right too (coerce at the write gate + emit cache_key_non_string telemetry so the next occurrence names its producer). One blocker on Fix 3, one concern on Fix 2's scope. Neither is about the mechanism; both are about the "gate" claim not holding when I grep the workspace.

Blocker

elementCacheKeys is NOT the one gate every cache write passes through. The PR body claims "elementCacheKeys is 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" — and the docblock on the function repeats this. grep -rn "keyframeCache\.set\|gsapAnimations\.set" packages/studio/src --include='*.ts*' | grep -v test (verified locally) surfaces two sibling writers in packages/studio/src/hooks/useGsapTweenCache.ts that go straight to draft.keyframeCache.set(...) without routing through elementCacheKeys, and both of them repeat the exact bare-id anti-pattern the coercion was added to close:

• useGsapTweenCache.ts:323-324 —

  draft.keyframeCache.set(`${sourceFile}#${elementId}`, merged);   // template literal — coerces
  draft.keyframeCache.set(elementId, merged);                       // BARE elementId — no coercion

  Comment at :319-321 names the same motivation as elementCacheKeys ("PropertyPanel reads the cache by bare elementId … the same entry is written under the bare key for cross-component lookups"). Same pattern, no coerce.
• useGsapTweenCache.ts:448-450 —

  draft.keyframeCache.set(cacheKey, entry);
  if (sf !== "index.html") draft.keyframeCache.set(fallbackKey, entry);
  draft.keyframeCache.set(id, entry);                               // BARE id from `scanAllRuntimeKeyframes` — no coercion

  id comes from a for (const [id, data] of scanned) where scanned is the return of scanAllRuntimeKeyframes(iframe, clipById) — same trust-the-declared-type situation the fix is repairing at elementCacheKeys. If a non-string can reach elementCacheKeys (which the fix asserts is possible), it can reach these too along a parallel producer chain.

pruneKeyframeCacheToFiles at gsapKeyframeCacheHelpers.ts:198-206 iterates [...keyframeCache.keys(), ...gsapAnimations.keys()] flatly and calls key.indexOf("#") — so a non-string key written by either of those two sibling sites still throws TypeError: s.indexOf is not a function and takes the editor down. The Fix 3 tests (gsapKeyframeCacheHelpers.test.ts:53,71 — "keeps every written key a string" / "survives a prune after a write with a non-string id") verify the invariant for writes routed through elementCacheKeys; they don't verify it for the two sibling paths, so a green test suite here doesn't tell you the crash is closed.

Two shapes that would close this:

1. Route the sibling sites through the same helper. Extract coerceCacheKeyId (already private at gsapKeyframeCacheHelpers.ts:255) as a named export, or expose an elementCacheKeys-like helper that just coerces a single id, and call it at the two useGsapTweenCache.ts sites so the bare-id write becomes draft.keyframeCache.set(coerceCacheKeyId(elementId, sourceFile), merged). Preserves the "next occurrence names its producer" telemetry across all writers — which is arguably the more valuable part of Fix 3 than the coercion itself.
2. Belt-and-suspenders defense at pruneKeyframeCacheToFiles:198. Filter keys to typeof key === "string" before the indexOf call. Cheaper diff, but silently sweeps whatever non-string writer exists under the rug — losing the telemetry-driven producer hunt that Fix 3 goes to some length to preserve.

Option 1 is the honest fix for the "one owner" claim the PR body makes. If Option 2 is the pragmatic choice, the PR body claim needs to be softened — "the write gate we know about" rather than "the one gate every cache write passes through" — because the current wording is asserting an invariant that the code doesn't hold.

Concern

Sibling localStorage sites outside Fix 2's scope. safeLocalStorage() is the right pattern and lifting getPersistedTab + selectTab onto it is the right fix. But the PR body's "routed through the existing safeLocalStorage() helper with the access guarded too, matching the pattern telemetry/config.ts documents" undersells that there are five other unguarded direct-localStorage sites in packages/studio/src that hit the same Chrome property-read throw for the same class of users:

• components/StudioFeedbackBar.tsx:38,39,53,54,62,63 (6 accesses)
• components/renders/useRenderQueue.ts:49,60
• components/renders/renderSettings.ts:11,34
• telemetry/policy.ts:32
• utils/resizeDebug.ts:11

The specific reason getPersistedTab broke Studio hard is that it runs as a useState initializer — the throw takes React's initial render, no error boundary catches it above the tree. Some of the other sites are lazier (behind user gestures, effects, or opt-in flags), so their failure mode is less catastrophic, but each is a latent instance of the same pattern.

Not a blocker for THIS PR — you've named the specific useState-initializer position that caused the crash boundary. But if the scope becomes "close the class, not the specific report", these five call sites are the sibling sweep. Worth landing in the "Not covered" section either way so a future crash report against StudioFeedbackBar's session-count read doesn't get diagnosed cold.

What lands cleanly

• Fix 1 (player.remove()). Node.remove() is a no-op on a detached node per WHATWG DOM §4.4; container.removeChild(player) requires the parent-child relationship still hold at cleanup time, which is fundamentally an invariant a React cleanup effect can't guarantee. The two call sites (Player.tsx:301, clipboard.ts:29) are the only two removeChild sites in non-vendor source (verified), so the sibling audit is done. clipboard.ts's is inside a finally, which means it fires even when the try body has already succeeded and browser handling may have moved the textarea — same idiom violation as Player.tsx, same right fix. Test at Player.test.ts:108-115 detaches the player then unmounts and asserts .not.toThrow(), which is a direct behaviour check on the fix.
• Fix 2 (safeLocalStorage). The double-guard shape (safeLocalStorage internally handles the property-read throw; the outer try/catch handles a getItem throw after the property read succeeded) is deliberate and matches the "same case telemetry/config.ts documents" reference — that file's :25-33 comment explicitly names the "guards the REFERENCE, not the access" distinction. Test at LeftSidebar.storage.test.ts:16-27 defines localStorage as a getter that throws, which reproduces the exact SecurityError Chrome emits on the property access. Not the method call — the property access, which the pre-PR code was unguarded against.
• Fix 3 shape, once the blocker is closed. The choice to coerce at a write gate rather than guess at a producer is the right trade — memory [[feedback_consolidation_fix_must_reach_handler_layer]] and [[feedback_verify_pr_body_semantic_invariants_by_grep]] say roughly the opposite in general ("verify the fix reaches downstream", "prove enumeration claims by grep") but they're deferring to the case where the producer is knowable and enumerable; here the diagnosis names that traced writers all produce strings, which makes the gate the right layer for the invariant repair rather than "punt the fix downstream". The telemetry shape {value_type, constructor_name, is_array, source_file} — no value content, just type-shape — is the right balance of producer-observability vs user-content-in-telemetry. Once the two sibling sites are also routed through the same helper, the maps' Map<string, ...> type will actually hold at runtime.
• v0.7.90 tag rebuild for stack-decode. "Rebuilding the tag reproduces the same asset filename hash byte-for-byte" is a proof (not a claim) that the rebuild is the code the crash came from — same discipline as #3096's A/B measurement table. The "follow-up worth its own PR: ship sourcemaps for Studio" note in the PR body is the right kind of "this was expensive, next time we shouldn't have to" acknowledgment. The bundled crash-decode workaround should never become a permanent process.
• Every test verified to fail without its fix. The PR body claim is checkable: I confirmed the four gsapKeyframeCacheHelpers.test.ts cases exercise the specific paths described (bare-id coercion via elementCacheKeys(sourceFile, badId), telemetry emission with matching value-shape, silence on the string path, prune-after-non-string-write). Same for LeftSidebar.storage.test.ts and Player.test.ts:108.

Series note: this is Miguel's seventh open HF PR from today's arc (3091, 3092, 3094, 3096, 3097, 3098 — this is a different theme, defensive-cleanup rather than parity or geometry). CI is green through this SHA except for Windows tests still in progress. LGTM on Fixes 1 and 2 unchanged; Fix 3 needs the sibling-site sweep before I'd call it "the maps' declared contract is guaranteed".

— Review by Rames D Jusso

[vanceingalls]

APPROVE @ 8a42115068fd4228b47f25bd384b12104264d521 (R1) — Fixes the three named boundary trips: (1) NotFoundError on Player teardown / clipboard textarea removal, (2) SecurityError reading localStorage in getPersistedTab (a useState initializer), (3) TypeError: s.indexOf is not a function in pruneKeyframeCacheToFiles on a non-string cache key. Each fix has a regression test that fails without the fix, and every required check I can see is green.

Per crash

• #1 removeChild NotFoundError — packages/studio/src/player/components/Player.tsx:301 and packages/studio/src/utils/clipboard.ts:29. container.removeChild(player) / document.body.removeChild(textarea) → .remove(). Correct: Element.remove() is a no-op when the node has no parent, so a container re-render, a crossfade swap, or a translation-extension reparent between mount and cleanup can no longer throw. Confirmed via rg 'removeChild' packages/studio/src that these are the only two removeChild call sites in non-vendor source. Cleanup-fn timing is what makes this a class fix rather than a two-site symptom guard. Player.test.ts:103 detaches the element, then unmounts — I traced this reproduces the reported DOMException without the fix.

• #2 localStorage SecurityError — packages/studio/src/components/sidebar/LeftSidebar.tsx:27 (getPersistedTab) and :118 (the setItem in selectTab). Routes through the existing safeLocalStorage() helper (packages/studio/src/utils/safeStorage.ts:6, already the pattern in telemetry/config.ts) and adds an outer try/catch for the getItem throw case. Correct: safeLocalStorage()'s internal try only catches the property read; a getItem throw needs the outer catch. getPersistedTab really is the useState<SidebarTab>(getPersistedTab) initializer at LeftSidebar.tsx:102, so an unguarded throw did drop the whole editor. Test triggers the throw at the property getter, which is the exact shape the crash reports carry.

• #3 s.indexOf is not a function in prune — packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:183-217. Coerces the bare-id key inside elementCacheKeys and emits studio:cache_key_non_string (value_type / constructor_name / is_array / source_file — no user content). Correct diagnosis: pruneKeyframeCacheToFiles:153 (key.indexOf("#")) reads keys straight from both maps, and the bare-id branch of elementCacheKeys was the only unwrapped write path there (the two template-literal branches self-coerce). Rebuilding v0.7.90 to decode the minified frame is documented in the PR body; the four-case unit test covers key shape, telemetry emission, silence on the string path, and prune-survival after a non-string write.

Cross-cutting notes

1. [NOTE] "One gate" is a slight overstatement — two bare-id writers bypass elementCacheKeys. useGsapTweenCache.ts:320 (setKeyframeCache(elementId, merged)) and :440 (setKeyframeCache(id, entry)) write the bare-id key directly, not via elementCacheKeys. If the still-unknown non-string producer routes through either, pruneKeyframeCacheToFiles still throws — the crash surface is narrower than "closed." The PR body already acknowledges the producer is unknown, and the telemetry event will finger it on next occurrence, so I don't think this blocks. Cheapest belt-and-suspenders: coerce inside the two setters in packages/studio/src/player/store/keyframeSlice.ts:103,118 (if (typeof elementId !== "string") elementId = String(elementId)) — that's the only true single gate.

2. [NOTE] String(elementId) on an HTMLElement collapses to "[object HTMLDivElement]". If the mystery producer is passing a DOM element (which the constructor_name field is designed to catch), every element collides on one key. That would surface as visible keyframe corruption in the editor rather than a crash, so it's a strict improvement over throwing at prune — worth naming here so the next PR that reads the telemetry knows to look for it and pick a better coerce (el.id ?? String(el) or similar) once the producer is identified.

3. [NIT] getPersistedTab now exports a non-component from a .tsx file. Fast-Refresh's react-refresh/only-export-components warns on that; Lint CI is green, so the studio ESLint config either has allowConstantExport broader, or scopes the rule per-file. If future churn adds friction, splitting into LeftSidebar.storage.ts next to the new LeftSidebar.storage.test.ts is the tidier landing. Not blocking here.

4. [NOTE — good pattern] Emitting cache_key_non_string with value shape but no content is the right instinct: the offending id could carry user data if the producer is e.g. an author-supplied selector, and this keeps telemetry PII-safe.

CI

All required-suite checks I can see are green: Build, Test, Lint, Format, Typecheck, Producer unit/integration, Preflight, Studio load smoke, Studio timeline viewport gate, CLI smoke, Fallow audit, Semantic PR title, CodeQL, File size check. Tests on windows-latest was still pending at review time — happy to re-check if it flips red. No red required checks.

— Via

[miguel-heygen]

Thanks both. Blocker confirmed and fixed; one concern I'm pushing back on with evidence; nit declined.

Blocker — confirmed, fixed in 9aaa6bd5b

You're right, and the PR body was wrong. useGsapTweenCache built the key-variant list by hand at two sites, so a non-string id there reached both maps uncoerced and prune could still throw the same TypeError.

I took neither suggested close, because a better one exists. Rather than export coerceCacheKeyId and call it at the sibling sites (which leaves three places that must each remember to coerce), both sites now loop elementCacheKeys itself, and their matching reads use the same list instead of a second hand-rolled copy. That makes it a real single gate rather than three gated places, and it deletes the duplication instead of adding to it.

Worth noting that the alternative floated in the second review — coercing inside keyframeSlice.setKeyframeCache / setGsapAnimations — would not have closed this. publishKeyframeCache writes the drafts through usePlayerStore.setState directly and never touches those setters, so every path in this PR bypasses them.

Two things fell out of it:

• The per-element writer omitted the index.html#<id> fallback key that every sibling writer sets. That is exactly the drift elementCacheKeys' own doc comment was written to end, so a reader falling back to that key could see a stale entry. Routing through the helper fixes it.
• The only remaining direct writers are in the timeline performance fixture, which is dev-gated and generates its own string ids.

PR body updated: the "one gate every cache write passes through" claim is gone.

On the String(HTMLElement) note — agreed, that degrades a crash into a key collision rather than eliminating the problem. That is the intended trade for now, and constructor_name is what actually closes it.

Concern on Fix 2 — I think this one is a false positive

I checked all five sites before scoping Fix 2, and every one of them is already inside a try/catch that covers the property-read throw:

• StudioFeedbackBar.tsx — shouldShowFeedback (36-44), incrementSessionCount (49-57), markPrompted (60-66)
• useRenderQueue.ts — readHiddenIds (47-54), writeHiddenIds (56-63)
• renderSettings.ts — getPersistedRenderSettings (9-26), persistRenderSettings (28-38)
• telemetry/policy.ts — isLegacyOptedOut (30-36)
• resizeDebug.ts — isEnabled (8-16)

getPersistedTab was the only unguarded storage access in packages/studio/src, which is consistent with it being the only one that ever produced this crash signature. Adding guarded sites to "Not covered" would point a future investigation at code that already handles the case, so I'd rather not. Happy to add a note if you still read one of those as reachable — tell me which and I'll re-check it.

Nit — declining

No react-refresh / only-export-components rule is configured for this package, and non-component exports from .tsx are the existing convention in Studio, including both sibling files in the same directory (CompositionsTab.tsx, AssetsTab.tsx, GlobalAssetsView.tsx, plus StudioHeader.tsx and others). Splitting this one file would make it the odd one out. If Fast Refresh becomes a real problem it's worth a sweep across all of them, not a one-off here.

Full suite still green after the change: 3559 passed, 335 files, 0 failures; oxlint, oxfmt, tsc --noEmit clean.