fix(studio): improve preview loading reliability - #2837
Conversation
There was a problem hiding this comment.
Pull request overview
Improves Studio preview initialization reliability by ensuring timeline/player listeners are attached before iframe navigation, reducing sidebar idle resource usage via thumbnail previews, and surfacing initialization failures with a retry UI.
Changes:
- Reordered preview lifecycle wiring so listener attachment happens before
srcassignment/DOM connection, preventing “00:00/00:00” stuck states on warm-cache loads. - Replaced eager composition-card preview iframes with cached thumbnail images; mounts a single live iframe preview only after sustained hover, and removes it on leave.
- Added explicit preview initialization failure reporting + retry action, with regression tests covering listener ordering, hover-preview lifecycle, and init timeout behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/studio/src/player/hooks/useTimelineSyncCallbacks.ts | Adds timeout-based initialization failure reporting hook for the preview timeline adapter. |
| packages/studio/src/player/hooks/useTimelinePlayer.seek.test.ts | Adds a regression test ensuring init failure is reported after iframe load. |
| packages/studio/src/player/components/Player.tsx | Reorders listener attachment before src/append; adds preview error UI + retry mechanism; parses player error messages. |
| packages/studio/src/player/components/Player.test.ts | Adds tests for preview error message extraction/fallback behavior. |
| packages/studio/src/components/sidebar/CompositionsTab.tsx | Uses thumbnail images at rest; mounts/unmounts live hover-preview iframe; tracks live-preview load for crossfade. |
| packages/studio/src/components/sidebar/CompositionsTab.drag.test.tsx | Adds tests verifying no eager iframes, and hover mounts/unmounts the live preview. |
| packages/studio/src/components/nle/NLEPreview.tsx | Threads the new reportError callback through the preview load pipeline. |
| packages/studio/src/components/nle/NLEContext.tsx | Updates onIframeLoad signature to accept an optional reportError callback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| window.removeEventListener("message", onMessage); | ||
| // Never leave the preview stuck invisible if the runtime never settled | ||
| // (initializeAdapter reveals on success; this covers the give-up case). | ||
| revealIframe(iframeRef.current); | ||
| }, 5000) as unknown as ReturnType<typeof setInterval>; |
| const handleLeave = () => { | ||
| if (hoverTimer.current) { | ||
| clearTimeout(hoverTimer.current); | ||
| hoverTimer.current = null; | ||
| } |
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 3af9a7d.
Fix shape is right on all three prongs — listener-attach-before-src is the real race close, hover-mount iframes drop the idle-composition WebGL/decoding pressure to zero, and the surface-player-error + retry replaces the pre-existing timeout duplication. Cleanup handler correctly removes all 5 listeners and only nils retryPreviewRef.current under an identity check, matching the forwarded-ref pattern that was already there for crossfade refreshes.
Two things worth adding before merge — both non-blockers, both about tests catching regressions rather than the fix itself:
- The listener-ordering invariant is not pinned by a test. The PR body claims coverage for it but Player.test.ts only tests the message-parsing helper. If a future refactor moves
setAttribute("src", src)back above theaddEventListenercalls, CI is green. Inline suggestion at Player.test.ts. - The Retry preview button has no click test. Same file. Inline suggestion.
One consistency question:
- CompositionsTab
<img>at rest has noonErrorhandler, but the siblingFramePoster.tsxdoes (renders "Preview unavailable"). If the thumbnail service fails, the previously-visible iframe preview is now completely absent with no recovery UX. Inline detail.
CI at head is all green (all shards, Preflight, preview-regression, Windows render + tests, Producer integration all pass). The _hfStudioRetry URL-param cache-buster is a nice touch — deliberate monotonic counter, URL.searchParams.set replaces, no accumulation.
LGTM from my side once the two test gaps are considered.
| "The composition preview did not become ready.", | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 The PR body says this PR adds "focused regression coverage for listener ordering" — but the only new tests in this file cover readPreviewErrorMessage message parsing. There is no test that asserts the invariant the fix hinges on: that load / click / shadertransitionstate / ready / error listeners attach BEFORE player.setAttribute("src", src) and container.appendChild(player). The race the PR is closing ("a cached preview loads before listeners are attached") is inherently a code-ordering invariant, so ordering is what a regression test would need to observe.
A reasonable pin: mount the Player under jsdom / happy-dom, spy on player.addEventListener and player.setAttribute, then assert the recorded call order has all four addEventListener("ready" | "error" | ...) calls precede the setAttribute("src", ...) call. Reordering them back later would then fail CI instead of shipping.
Not a blocker — the fix looks correct at HEAD — but the marquee invariant this PR is buying isn't defended by a test that would fire when it regresses.
— Rames D Jusso
| className="mt-4 rounded-md bg-white px-3 py-1.5 text-xs font-semibold text-black transition-colors hover:bg-neutral-200" | ||
| onClick={() => retryPreviewRef.current?.()} | ||
| > | ||
| Retry preview |
There was a problem hiding this comment.
🟡 The Retry preview button (onClick={() => retryPreviewRef.current?.()}) is the user-visible affordance the whole readPreviewErrorMessage + previewError state machine exists to enable, and it has no test. The only new tests exercise the string-parsing helper. A test that mounts Player, dispatches an error CustomEvent on player, asserts the overlay renders, clicks Retry, and asserts player.setAttribute("src", ...) is re-called with the _hfStudioRetry param appended would pin the fix.
— Rames D Jusso
| className={`absolute inset-0 h-full w-full object-contain transition-opacity ${ | ||
| livePreviewLoaded ? "opacity-0" : "opacity-100" | ||
| }`} | ||
| /> |
There was a problem hiding this comment.
🟡 This at-rest thumbnail <img> has no onError handler. The sibling that already shipped for storyboard posters (packages/studio/src/components/storyboard/FramePoster.tsx:65) handles the same failure mode with onError={() => setFailed(true)} and renders "Preview unavailable" fallback text.
If the thumbnail 404s / times out (Chrome-pool failure on the studio server, transient thumbnailBrowser lease issue in packages/cli/src/server/studioServer.ts:171), the pre-PR code's iframe would at least still attempt to load the composition; this branch shows an empty box with alt="". Matching FramePoster's fallback (either "Preview unavailable" text or falling back to eagerly mounting the live iframe for that card only) would close the visible-nothing regression.
— Rames D Jusso
| const retryPreview = () => { | ||
| retryCountRef.current += 1; | ||
| const retryUrl = new URL(src, window.location.origin); | ||
| retryUrl.searchParams.set("_hfStudioRetry", String(retryCountRef.current)); |
There was a problem hiding this comment.
🟢 nit — _hfStudioRetry is cache-busting via URL param, so the retry does hit the server fresh even if the failure was cache-related. The choice to keep the counter monotonic (rather than reusing 1) is also correct because URL.searchParams.set replaces the existing value, so N-th retries don't accumulate params. Just calling out that the mechanism is deliberate; no action.
— Rames D Jusso
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
SSOT -- fix(studio): improve preview loading reliability
CI: all 29 checks pass on 3af9a7df. Mergeable.
Inventory of new surfaces
| What | File | Kind |
|---|---|---|
readPreviewErrorMessage() |
Player.tsx | exported pure fn |
DEFAULT_PREVIEW_ERROR |
Player.tsx | module constant |
retryPreviewRef, retryCountRef |
Player.tsx | component refs |
previewError state |
Player.tsx | component state |
| Error overlay + Retry button | Player.tsx | JSX |
livePreviewLoaded state |
CompositionsTab.tsx | component state |
thumbnailUrl derivation |
CompositionsTab.tsx | render-time computed |
buildCompositionThumbnailUrl import |
CompositionsTab.tsx | new import |
SMOKE_THUMBNAIL_SVG |
studio-runtime-smoke.mjs | fixture constant |
| thumbnail route | studio-runtime-smoke.mjs | GET_RESPONSES entry |
| 3 new test cases | Player.test.ts, CompositionsTab.drag.test.tsx, studio-runtime-smoke.test.mjs | tests |
Claim-vs-implementation audit
| PR claim | Verified? |
|---|---|
| Attach listeners before src/connect | Yes -- iframe.addEventListener("load", handleLoad) + player listeners now precede player.setAttribute("src", ...) and container.appendChild(player). Comment explains the race. |
| Replace idle iframes with cached thumbnails | Yes -- <img src={thumbnailUrl}> always renders; <iframe> conditionally renders only when hovered. |
| Mount one live preview after sustained hover, remove on leave | Yes -- 300ms setTimeout in handleEnter, setHovered(false) + iframe unmount in handleLeave. |
| Surface player probe errors with retry | Yes -- readPreviewErrorMessage extracts CustomEvent.detail.message; overlay shows it with a retry button that cache-busts via _hfStudioRetry query param. |
| Extend smoke fixture for thumbnails | Yes -- SMOKE_THUMBNAIL_SVG + GET_RESPONSES entry for thumbnail/index.html. |
| Regression tests for listener ordering, hover lifecycle, error messages | Partial -- error message and hover lifecycle are tested. Listener ordering is structural (code reorder in an imperative effect) and not directly unit-testable; the existing Studio: load smoke CI check exercises the fix end-to-end. Acceptable. |
Findings
1. Thumbnail URL semantic mismatch (nit, non-blocking)
const thumbnailUrl = buildCompositionThumbnailUrl({
previewUrl,
seekTime: 0,
duration: THUMBNAIL_SEEK_TIME_SECONDS * 2,
origin: window.location.origin,
});buildCompositionThumbnailUrl computes midTime = seekTime + duration / 2. The caller reverse-engineers this formula (0 + 6/2 = 3) to get t=3.00 instead of expressing intent directly. If the midpoint formula in buildCompositionThumbnailUrl ever changes, this silently drifts.
Two alternatives that would survive a formula change:
- Add a
timeparameter tobuildCompositionThumbnailUrlthat bypasses the midpoint math when provided. - Use
seekTime: THUMBNAIL_SEEK_TIME_SECONDS, duration: 0(clearer: "start at 3s, zero-length clip" => midpoint = 3s).
Not blocking -- the formula is stable and both call sites are in the same repo -- but worth a TODO or a follow-up.
2. Orphaned sync timer on hover leave (pre-existing, non-blocking)
When hover ends, the iframe unmounts, but syncTimer.current from the last requestIframePlaybackSync(true) call may still have pending retries. These fire harmlessly (access null iframeRef.current, fail, retry until exhausted -- max ~1s of no-ops). The old code had the same pattern except the iframe was always present so the timer would succeed immediately. Now that the iframe unmounts on leave, the timer ticks into the void a few more times.
Not new to this PR and self-terminates, but if you want to tighten it: have handleLeave clear syncTimer.current alongside the hoverTimer.
3. Redundant truthiness check in smoke test (nit)
assert.ok(response && response !== null);assert.ok(response) already rejects null. The && response !== null is redundant. Harmless but reads as double-checking.
What I looked for and found clean
-
Listener ordering: The reordered sequence (listeners -> ref bridge -> attributes -> src -> appendChild -> pasteboard style -> enableInteractive) is correct. The iframe element exists pre-connection (created in the player constructor), so
iframe.addEventListener("load", ...)works before the player enters the DOM. Cleanup mirrors attachment in reverse. The identity guard onretryPreviewRefcleanup (if (retryPreviewRef.current === retryPreview)) correctly prevents a stale effect from nulling a newer retry function. -
Error state machine:
previewErroris cleared in three places (retryPreview, handleReady, handleLoad) -- all three are valid "something positive happened" signals. The error overlay atz-40stacks above the loading overlay (z-30) and asset overlay (z-20), so the retry button is always reachable. The retry cache-buster (_hfStudioRetryquery param) is a query parameter, not a path segment, so servers that ignore unknown params serve the same composition. -
readPreviewErrorMessagetype narrowing: Uses the existingisRecordguard (notas), checksinstanceof CustomEvent, validatestypeof event.detail.message === "string", trims. Falls back toDEFAULT_PREVIEW_ERROR. Clean and exported for testing. -
livePreviewLoadedcrossfade: The<img>hasopacity-0whenlivePreviewLoadedis true, so it fades behind the iframe. On hover leave, bothhoveredandlivePreviewLoadedreset in the same handler (React batches the state updates), so the iframe unmounts and the thumbnail reappears in one render. No flash. -
Pre-existing
as React.MutableRefObject: The diff moves but does not introduce this assertion. It's guarded bytypeof ref === "function"andif (ref)narrowing. Standard forwardRef pattern; the remaining type is alwaysMutableRefObjectat runtime. -
Side-effect invariant on retry:
retryPreviewincrementsretryCountRef(monotonic), clears error, sets loading, and sets a new src. Each click produces exactly one state transition. TheretryPreviewRefidentity check in cleanup prevents double-null. No unbounded mutation path. -
Smoke fixture routing:
studioSmokeApiResponseextractspathnamefrom the URL (stripping query params) and looks it up inGET_RESPONSES. The new key${PROJECT_PATH}/thumbnail/index.htmlmatches the thumbnail URL generated bybuildCompositionThumbnailUrlfor the smoke project'sindex.htmlcomposition. The SVG placeholder (16x9,image/svg+xml) is aspect-correct.
Verdict
Ship it. The listener-ordering fix is the right structural change, the thumbnail swap is a clear performance win (N iframes down to 0 at rest, max 1 on hover), and the error surface is clean. The three nits above are all non-blocking.
-- Miga
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed at 3af9a7df2d687ec018f7d0705d25b5416c8dfa78.
The reliability fix is correct at the mechanism boundary:
Player.tsx:249-274attaches the iframe/player lifecycle listeners beforesrcassignment and DOM connection, closing the warm-cache load race. Cleanup at:290-315mirrors every attachment and identity-guards the retry ref.- Retry changes the observed
srcwith a monotonic_hfStudioRetryvalue;hyperframes-playerobservessrcand forwards it to the iframe, so the button performs a real reload rather than only clearing UI state. CompositionsTab.tsx:227-270leaves zero live preview iframes at rest, mounts one after sustained hover, and unmounts it on leave; the thumbnail endpoint is shared throughbuildCompositionThumbnailUrl.- Listener/timer audit found no leak: Player listeners, polling interval, overlay timers, and hover/sync timers all have unmount cleanup. The already-noted sync retry can run harmless no-ops for at most ~1s after hover leave and is non-blocking.
git diff --checkis clean, and the exact-head CI matrix is green across Typecheck, Test, Studio smoke, preview parity/regression, Producer, Windows, CLI smoke, and global install.
The missing direct ordering/retry-click regression pins and thumbnail-error fallback are worthwhile hardening follow-ups, but the implementation itself is correct and existing smoke coverage exercises the real Studio load surface.
Verdict: APPROVE
Reasoning: The race is closed before navigation, idle iframe pressure is eliminated, failure/retry behavior is real, lifecycle cleanup is sound, and no correctness blocker remains.
— Magi
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reasoning: Re-reviewed the delta from 3af9a7df to c62bd4c45. The prior hardening notes are closed at the real seams: Player.test.ts mounts the component and proves all five lifecycle listeners attach before the first src, then drives the emitted error → visible Retry action → fresh _hfStudioRetry=1 URL; composition cards now show a tested thumbnail failure fallback, express the 3-second thumbnail time directly, clear pending playback-sync retries on leave, and remount state safely when project/composition identity changes. The smoke-test cleanup is mechanical. git diff --check is clean. Exact-head build, lint, typecheck, unit/integration, Studio smoke, preview parity/regression, CodeQL, and global-install checks are green; Windows jobs remain in flight and should gate merge normally.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Delta-only re-review against the previously reviewed head. All four prior notes are addressed, and the two new tests pin the invariants rather than restate them.
- Listener ordering. The new test instruments a real custom element and asserts each of the five listener attachments indexes before
setAttribute("src"). That is the invariant this PR exists to enforce, now held against regression instead of described in the body. - Retry. Exercises the real path end to end: error event, surfaced error element, click on the retry action, then asserts the cache-busted parameter on the reloaded URL and the cleared error state.
- Thumbnail fallback and hover cleanup. Both land, and the hover test asserting a zero timer count on leave is the right shape for the orphaned-timer note.
On the thumbnail seek change. buildCompositionThumbnailUrl computes midTime = seekTime + duration / 2, so the previous (0, 2 * SEEK) and the new (SEEK, 0) resolve to the same value. The new form is the more robust of the two: zeroing duration makes the result independent of how the duration term is weighted, where the old form hard-coded knowledge of the exact /2. Worth noting the durable part is the new assertion pinning t to 3.00 — that is what actually catches a formula change; the call-site shape alone would still drift under a formula that doesn't vanish at duration = 0.
One undeclared change worth naming, since it isn't in the commit subject: the card key moves from the composition name to a project-scoped key. That reads as incidental but is load-bearing here. The thumbnail-failure state added in this same delta is sticky, so without the project in the key a card reused across a project switch could carry a stale "Preview unavailable" onto a healthy composition. Correctly paired with the change that created the need.
Verified at the exact head with no pending or failing contexts. Approving.
— Review by Rames Jusso
…w-reliability fix(studio): improve preview loading reliability
What
Improves Studio preview reliability and reduces idle composition-preview resource usage.
Why
Heavy projects can intermittently remain at 00:00/00:00 when a cached preview loads before listeners are attached. The Comps sidebar also eagerly creates one live iframe per composition, increasing CPU, memory, decoding, and WebGL pressure even when previews are not being inspected. Player-reported initialization failures currently leave the preview silently stuck.
How
Test plan