Repository navigation
fix(studio): a failed preview reload says which step it was still waiting for - #5700
Conversation
Edit accuracy: accurate 271 (base branch 271), smooth 166 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
terencecho
left a comment
There was a problem hiding this comment.
Reviewed heygen-com/hyperframes#5700 at HEAD 0cd1591e6f8bcd288832d65d88987c59d4833401 against effective/API BASE 4e6fcdb3a9f310a9c483622d6a4ca5354a63fb22.
[P2] Keep failure diagnostics from bypassing shadow cleanup
useShadowPreviewReload.ts:107–112: the new shadowWin?.__player read can throw SecurityError on an inaccessible cross-origin iframe; optional chaining only handles a null/undefined window. This runs before every cleanup and failure notification. For example, while the old live preview is showing, an edited composition can navigate its hidden shadow to another origin before promotion. The actual iframe allows scripts/self-navigation, and CompositionProbe already handles inaccessible frames by catching SecurityError and reporting its eight-second timeout. That error reaches this callback through Player → NLEPreview.onPreviewError → onShadowError; the hook's own terminal wait budget also reaches it. BASE discards that failed shadow, clears the reload flags, and reports the failure through EditorShell's error toast. HEAD instead throws before invalidating the generation, discarding the shadow, clearing previewReloading, or invoking the failure callback; the later timeout hits the same failing read. The old preview stays visible, but the reload remains stuck until another reload/reset/unmount.
Make the diagnostic read best-effort with a safe fallback, so cleanup and reporting always run. Add a current-generation inaccessible-window/property regression asserting retained live preview, discarded shadow, cleared reloading state, and exactly one failure callback; retain the stale-generation control.
Validation / scope: source-only review, with full BASE-before-HEAD reads of both changed files and an independent adversarial pass. Traced the actual iframe policy, authored preview route, probe/error forwarding, reload timer, and toast callback. Normal readiness ordering, restore-seek paint, stale-generation rejection, visual-readiness re-raise, superseding reloads, and cleanup otherwise remain unchanged. This finding is a source-derived failure path, not a claimed browser reproduction. No tests, repository/browser/native runtime, or CI queries were run; author/bot test reports are not independent validation.
— tai
Verdict: Changes requested.
Reasoning: The new diagnostic introduces a throwing operation before the existing handled-failure exit, preventing cleanup and user-visible failure reporting for an inaccessible current shadow.
…p and reports Naming the step the reload waited for read the hidden frame's player, which throws for a cross-origin frame and skipped the cleanup and the failure report.
|
Fixed in 4845b65. You were right: the diagnostic read could throw before the cleanup ran.
|
terencecho
left a comment
There was a problem hiding this comment.
Re-reviewed at HEAD d3d77676038e0e4c94170c73400ac6ca11d1ae93 against API/effective BASE 4e6fcdb3a9f310a9c483622d6a4ca5354a63fb22. No introduced blocker found in this bounded source review.
- Prior P2 resolved:
useShadowPreviewReload.ts:38–44now catches the entire iframe/window/player access. An inaccessible window no longer lets the new diagnostic bypass shadow cleanup and failure reporting. I checked fixing commit4845b65band the subsequent helper rename at this head. - Meaningful regression source:
useTimelinePlayer.shadowReload.test.ts:523–546preserves a throwing__playergetter through the real hook harness and asserts shadow discard, old-live identity, cleared reload state, and exactly one fallback report. useShadowPreviewReload.ts:112–130snapshots the waiting step before clearing refs, while retaining generation rejection, cleanup, the previous-preview message, and the latest failure callback.
Audited: both changed files end-to-end at BASE then HEAD; actual adapter/readiness/error-to-toast paths; normal, fallback, and subcomposition runtime injection; diagnostic/cleanup, stale-callback, seek-paint/readiness, supersession/reset, and debug-consumer interactions. An independent adversarial source pass also found no blocker.
Trusting / limits: unchanged subsystems outside those paths and unselected player internals; native DOM/origin and runtime behavior were not executed. Regression tests were inspected as source only. No tests, browser/runtime execution, or CI queries performed; author-reported results are not my validation.
— tai
Verdict: APPROVE
Reasoning: The previously reported cleanup bypass is repaired at this exact head, and the diagnostic addition preserves the inspected failure, promotion, and live-preview ownership paths.
What
When an edit's preview reload fails, the message now also says which step the new document was still waiting for: "The preview did not reload (it took too long to load). It was still waiting for the page and its runtime to load / its timeline to be ready / its loaders to clear. The previous preview is still showing."
Why
Reload failures report how they ended (the reload budget ran out, or the player's probe gave up) but not what never happened, so a stuck reload cannot be traced to a cause from the report alone. Each failure now names the first readiness step that had not completed.
How
failShadowinuseShadowPreviewReload.tsreads the shadow's state before it is discarded: no runtime bridge (__player) means the page and its runtime had not loaded; no pending commit means its timeline (a positive length and the restore seek's paint) was not ready; otherwise its loaders had not cleared.logReload("shadow-failed", ...).No visible change
Only the text of an existing error message changes; nothing on screen moves or renders differently.
Test plan
useTimelinePlayer.shadowReload.test.tsasserts the step for each case (no runtime, runtime with no length, timeline ready but loaders up). Breaking the timeline check makes its test fail.packages/studio/src/player: 2303/2303, 3 runs in a row;tsc --noEmitclean.