fix(studio): preserve active Player ref during cross-fade unmount

[virang-nimrobo]

Summary

NLEPreview renders two <Player> instances during a refresh cross-fade — a "retiring" one and a new active one — sharing the same forwarded iframeRef. The retiring Player's cleanup unconditionally nulled the ref, even though the active Player had already claimed it. Result: iframeRef.current === null for the rest of the session.

That null ref breaks the source filter in useTimelinePlayer's message handler:

```ts
if (e.source && ourIframe && e.source !== ourIframe.contentWindow) return;
```

When ourIframe is null the guard short-circuits, so every sibling iframe's hf-preview postMessage flows into processTimelineMessage. The CompositionsTab sidebar mounts an <iframe> per composition, and each one's runtime broadcasts its own clip set every ~333 ms (postTimeline() on a 20-frame transport tick). The studio then oscillates setElements between every visible thumbnail's manifest, remounting CompositionThumbnail on every cycle and visibly flickering the timeline.

Page refresh hides the bug because there's no retiring Player to clobber the ref.

Fix: only null the forwarded ref when it still points at our own iframe (object-ref path). Function-ref path falls through harmlessly — there is no in-tree consumer of <Player> that uses a function ref today.

Repro

1. Open a project with several timed compositions visible in the sidebar.
2. Trigger any refreshKey++ while the studio is mounted (e.g. cancel an in-flight timeline-comment agent run, save a file via the editor, etc.).
3. Observe: timeline elements oscillate, every CompositionThumbnail re-fetches, network tab fills with hundreds of requests per minute.

Test plan

• Reproduce the flicker on main, confirm the fix on this branch eliminates it.
• Verify normal Player remount cases still work: composition switch, manual file save (refreshKey bump), preview refresh.
• Verify iframeRef.current stays valid after the 160 ms cross-fade window.

🤖 Generated with Claude Code

[virang-nimrobo]

@miguel-heygen found this bug while building the comment feature. I will raise the PR for the feature one post this is merged into main.

[jrusso1020]

Verdict

Approve. The bug analysis is correct, the fix is the right shape (object-ref ownership check before clearing), and the test reproduces the exact lifecycle that broke. One non-blocking issue with the dropped function-ref branch and one minor durability concern worth flagging.

Verifying the diagnosis

The PR description nails it. Confirming from the source at this SHA:

• NLEPreview.tsx renders the retiring Player without ref and the active Player with ref={iframeRef}. When refreshKey bumps, the previously-active Player keeps its React identity (same key) but is now re-rendered without a ref prop — meanwhile the new Player mounts with the shared iframeRef and claims it via its async import("@hyperframes/player").then(...).
• Player's setup/cleanup lives inside useMountEffect, so the cleanup function closes over the ref value at initial mount, not at unmount. The retiring Player's cleanup therefore holds a reference to iframeRef from when it was active, even though the JSX no longer passes that ref to it.
• 160 ms later the retiring Player unmounts → old cleanup runs → old (unconditional) code did iframeRef.current = null → flooded processTimelineMessage because the e.source !== ourIframe.contentWindow guard short-circuits on ourIframe === null.

The downstream consequence (sidebar CompositionThumbnail iframes broadcasting hf-preview timeline messages every ~333 ms, oscillating setElements, remount loops) is consistent with what I'd expect from that null-ref short-circuit in useTimelinePlayer's message handler.

Why the fix is right

• Check ownership before clearing. if (objectRef.current === iframe) objectRef.current = null; is the standard pattern for shared refs across overlapping component lifetimes. It's also self-documenting — only the owner of the current ref value gets to null it.
• Refresh path still works. When there's no overlap (no retiring Player), objectRef.current === iframe is true at unmount → ref is cleared as before. Test 2 covers this explicitly.
• Test 1 reproduces the exact failure mode. Mount-with-ref → re-render-with-retiring-plus-new-claiming-ref → unmount-retiring → assert ref still points at the new iframe. Without the fix this would fail; with the fix it passes. Good regression coverage.

Key Concerns (non-blocking)

1. Function-ref branch is silently dropped

Old code preserved the function-ref path:

if (typeof ref === "function") {
  ref(null);
} else if (ref) { ... }

New code drops it entirely:

if (typeof ref !== "function" && ref) { ... }

The PR description says "Function-ref path falls through harmlessly — there is no in-tree consumer of <Player> that uses a function ref today." That's true today, but:

• The forwardRef<HTMLIFrameElement, PlayerProps> signature accepts function refs, so external consumers (or future internal ones) could pass one and silently get a stale value on unmount.
• The same ownership-tracking is possible for function refs by holding const lastForwardedIframe = useRef<HTMLIFrameElement | null>(null); and only calling ref(null) if lastForwardedIframe.current === iframe. Or — simpler — preserve the unconditional ref(null) for the function-ref path and only guard the object-ref path; the function-ref consumer was already responsible for its own ownership logic.

Suggest one of:

• Preserve if (typeof ref === "function") ref(null); (status-quo for function refs).
• Or add a tightening to a React.RefObject type so function refs are a compile-time error: forwardRef<HTMLIFrameElement, PlayerProps>(...) already permits both, so a type-narrowing wrapper would be needed.
• At minimum, a code comment noting the deliberate omission, since the PR description rationale won't survive in tree.

2. Root cause vs. symptom

The underlying issue is that Player's setup/cleanup uses useMountEffect (mount-once) and therefore captures stale closures over props that can change. This PR patches the specific symptom for ref, but the pattern remains for any future prop that's read in cleanup. If onLoad, onCompositionLoadingChange, or directUrl ever grow cleanup-time behavior, the same class of bug recurs.

Not blocking — refactoring to a useEffect([ref]) setup would force remount listeners on every parent re-render, which is heavier than the bug warrants. Worth a comment near the cleanup explaining "this closure captures props from initial mount; check ownership before destructive cleanup."

Test Coverage

Solid.

• Test 1 is a tight reproduction of the production bug — mount, retire+reclaim, unmount-retired, assert ref preserved.
• Test 2 is the inverse — confirms the default unmount-clears-ref path still works, so we haven't silently broken normal lifecycle.
• The installHyperframesPlayerStub helper (mocking document.createElement for <hyperframes-player>) is heavyweight but necessary given Player creates the web component imperatively. It would be worth extracting into a shared test helper if more Player tests follow.
• No test for the function-ref path — see concern #1. If you preserve the function-ref ref(null), a third test asserting "function ref is called with null on unmount" would round it out.

Nits / Future

• globalThis.IS_REACT_ACT_ENVIRONMENT = true; at module load is fine for this file but should ideally live in a vitest setup file if more tests start needing act() for React 19.
• flushMountedPlayer two-Promise.resolve() pattern works because vi.mock("@hyperframes/player", () => ({})) makes the dynamic import synchronously resolvable. If the mocked module gains async exports later this could become flaky.
• getPreviewPlayerKey is exported but only consumed internally by NLEPreview per the file I read. Worth checking if anything else references it; if not, drop the export.

Good catch on the root cause and good fix. Recommend merging.

[virang-nimrobo]

@jrusso1020 CI failed during bun install because Puppeteer hit a transient 500 downloading Chrome 148.0.7778.97. Could you rerun the workflow job?