Repository navigation
fix(studio): a preview reload survives its pane unmounting mid-load - #7590
Conversation
When the preview pane unmounted while a shadow reload was loading, its Player removed the shadow frame but the reload kept timing it, so the edit was dropped with "Its page had got to: no-page" and the previous preview stayed. The reload now pauses while its frame is off the page and starts its budgets again when the pane mounts a new frame. The Player's comment on callback refs said the mutable-ref branch was the only one in use; the shadow reload passes a callback ref, so the comment now says why those are left alone.
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head ea2af6ba
- blocker — PR body: there's no
fails without the fix:line. The body says the two newuseTimelinePlayer.shadowReload.test.tscases "fail onmain", but this lane requires a line in exactly that form. Add one that names at least one test, for examplefails without the fix: useTimelinePlayer.shadowReload.test.ts › keeps a reload whose frame left the page, and promotes it once a new frame mounts.
I found no blocker in the code. The detach check runs only when a budget runs out. setShadowIframeNode restarts the budgets only for the current gen after the frame has left the page, and stopPendingShadow clears the new flag. Leaving callback refs alone in Player.tsx keeps promotion safe.
Scope
I read all 5 changed files end to end, plus the callers of setShadowIframeNode (NLEPreview.tsx:522, PreviewPane.tsx, NLEContext.tsx). There were no prior reviews. mergeStateStatus is BLOCKED, and this reviewer can't read check-runs.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at ea2af6ba63bf7e9b084b23a25ff0b44c01ff379d.
No code blocker found. The body now has the required fails without the fix: line for these two tests in useTimelinePlayer.shadowReload.test.ts:
- “keeps a reload whose frame left the page, and promotes it once a new frame mounts”
- “gives a frame that mounts again its full budgets, then drops it if it stays silent”
I verified the claim: reverting onlyuseShadowPreviewReload.tsto the merge-base version makes both named tests fail because the reload is dropped while its frame is detached. Restoring the head gives 85/85 passing tests across the changed shadow-reload and Player suites. The full workspace build also succeeds. All temporary edits were restored.
The pause/rearm state belongs to the current reload generation and is cleared by promotion, failure, composition reset and teardown. Hidden-tab handling remains in place; the passing suite includes “does not spend the budget while the tab is hidden, and promotes once it is visible” and the composition-switch refresh-reset case. Callback-ref cleanup is unchanged behavior: retiring Players do not null a frame their owner may have promoted. The new connected-frame harness and Player unmount test reflect that ownership. I read the synchronization, retry and preview-pane wiring; I did not independently repeat the author's full browser/layout capture or whole-package suite.
Pre-existing follow-up (useShadowPreviewReload.ts:338–345): a detach/remount before the next timeout tick is not observed, so the replacement can inherit the remaining old budget. I checked the merge-base setter/timer: a quick replacement also retains its budget there. This fix covers detachments seen by a timeout; it does not reset every iframe replacement. Resetting on a changed node identity could address that separately, with an early-remount boundary test.
Gate mechanics: Genesis's request for changes (review 5870197494) is still live at this exact head despite the body correction. This approval does not dismiss another reviewer's request for changes.
Need: yes, retain a reload while its pane is absent · Size: focused · Reuse: existing generation and visibility-budget lifecycle · Simpler: extend that lifecycle rather than changing callback-ref promotion ownership · Structure: fine (checked sync callbacks, retry owner and pane wiring; no split)
Risk: low — timeout bookkeeping changes; preview promotion and ref-cleanup behavior are retained.
Verdict: APPROVE
Reasoning: The stated detached-pane regression is fixed, and both named tests discriminate the reverted implementation from this head.
— Rames
What changes
A preview that never finishes reloading after an edit, when the preview pane unmounts while the reload is still loading.
useShadowPreviewReloadloads a full-reload edit in a hidden shadow frame and gives it three 15 s budgets. When the pane holding the preview unmounted mid-load (for example a layout switch), the shadow Player removed its frame, but the hook kept timing that detached frame. It could never load, so after 45 s, and again after the retry, the edit was dropped with "Its page had got to: no-page", and the previous preview stayed on screen.Now, when a budget runs out and the shadow's frame is no longer on the page, the reload pauses, the same way it already does for a hidden tab. When the pane mounts again and its new shadow Player hands over a fresh frame (
setShadowIframeNode), the reload starts its budgets again from the beginning.Why not clear the callback ref instead
Player.tsxnever calls a callback ref withnullon unmount, and its comment said the mutable-ref branch was "the only path used today". That is no longer true: both the shadow reload (setShadowIframeNode) and the stage's live ref pass callback refs. Calling them withnullwould break promotion. A promotion makes the shadow frame the live one directly, without calling the live Player's ref, so the retiring live Player'snullwould wipe the frame that was just promoted. The Player keeps leaving callback refs alone, the comment now says why, and the reload checksisConnectedinstead.Trade-off: if the pane never comes back, the reload now stays pending instead of being dropped after 45 s. Nothing is waiting to be shown in that case, and a composition switch or the hook's unmount clears it, as before.
Tests
useTimelinePlayer.shadowReload.test.ts: two new cases.main(the reload is dropped at 45 s).main, where the reload is dropped at 45 s.Player.test.ts: unmounting a Player never calls its callback ref withnull, and its frame is off the page afterwards.timelinePlayerTestHarness.ts:makeFakeIframenow attaches its frame to the page, as a mounted Player's is. All existing reload tests pass unchanged with it.tsc --noEmitfor studio is clean, with core's generated runtime built.oxfmt --checkandoxlinton the changed files are clean.LEFTHOOK_EXCLUDE), because this worktree has no local install. The same checks ran on a separate machine with a full install, except fallow, whose CI comment covers it.How the captures were made
Studio dev server with the
studio-opentest fixture, in headless Chrome:index.htmlgets a big "EDITED" banner, which starts a full reload.The screenshots are taken right after reopening. Only
useShadowPreviewReload.tsdiffers between the two runs.Before
main: the reload gave up twice while the pane was closed. Each time, Studio reports "The preview did not reload ... Its page had got to: no-page. The previous preview is still showing."After
This branch: the reload waited while its frame was off the page, then loaded when Preview came back. No error.
Size
Under 100 lines on its own: a single fix with its two tests, already approved by both reviewers. The lane has no other open OSS PR to carry it, and it should reach the next release.