Repository navigation
feat(studio): a host that draws its own loader can turn off both of Studio's preview loaders - #6911
Conversation
There was a problem hiding this comment.
Reviewed at b90a8e2d. I found no blocker in the code, but two required repo checks are red on this head, so I'm requesting changes until they pass.
Blockers (CI):
- Comments / "A package file's comment share may not rise":
NLEPreview.tsxgoes from 13 to 14 comment lines andPlayer.tsxfrom 60 to 62, which raises each file's comment share. Cut those comment lines or move the meaning into names or types. For example,loadingUi: "studio" | "none"could become a self-describing union, or the doc lines could be dropped from theNLEPreviewpass-through. - PR captures / "Require Before and After captures": this PR changes
.tsxinpackages/studio, so the body needs## Beforeand## Aftersections, each with an image or video. Per the check's hint, edit the body text first, then attach withgh pr edit 6911 --attach.
Code: no issues found.
compositionWaitShownandassetWaitShownuse the same conditions the oldshow*flags did, and drawing is now gated separately onloadingUi === "studio". Defaults and thePreviewPanepath are unchanged, andonCompositionLoadingChangereports exactly what it did before.Player.test.tsandNLEPreview.test.tspass 50/50 at this head. WithPlayer.tsxandNLEPreview.tsxreverted tomain, only theloadingUi: "none"test fails, with the message quoted in the body. That matches thefails without the fix:line.
Non-blocking: with loadingUi: "none" the host also loses Studio's "Continue anyway" button. The asset poll's 10 s cap still ends the wait, so nothing hangs. A Desktop loader that wants that escape would need an imperative hook later.
Edit accuracy: accurate 271 (base branch 271), smooth 122 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
There was a problem hiding this comment.
Re-reviewed at ac0e8733. The code is fine, but one required check is still red and the body no longer matches the code, so I'm requesting changes again.
Resolved: the Comments check now passes. loadingUi: "studio" | "host" reads clearly without the doc lines, and Player.test.ts plus NLEPreview.test.ts pass 50/50 at this head.
Still blocking:
- PR captures / "Require Before and After captures" still fails on this head. The body has no
## Beforeor## Aftersection. This change edits.tsxinpackages/studio, so the check needs both, each with an image or video. Edit the body text first, then rungh pr edit 6911 --attach ./before.png --attach ./after.png. - The body describes the old API. What, How, Test plan, the
fails without the fix:line and "What I did NOT execute" all still sayloadingUi: "none", but the prop is now"studio" | "host". Please update the body to"host"in the same edit as the captures, so the named failing test matches the code.
The code itself has no remaining issues.
…tudio's preview loaders Player and NLEPreview take loadingUi: 'none', which hides the composition cover and the 'Preparing preview assets' wait while still reporting that the preview is loading. suppressLoadingOverlay keeps its narrower meaning: skip only the composition cover on a later load.
…studio' | 'host' The value says the host draws its own loader, so no doc line has to tell it apart from suppressLoadingOverlay, and the files' comment share does not rise.
…ze beside NLEPreview Player's draw decision moves to a small helper so the component's complexity does not rise, and resolvePreviewStageSize moves to its own file so NLEPreview stays under the 600-line cap with the new prop.
…er destructured prop Destructuring one more prop raised Player's cognitive complexity above main's; the rest of the props carries loadingUi instead.
ac0e873 to
5825d4a
Compare
There was a problem hiding this comment.
Re-reviewed at 5825d4ab. Requesting changes for a formatting failure only; the code is ready.
Blocker (CI): bun run format:check fails on packages/studio/src/components/nle/NLEPreview.tsx. The CI "Format" job and preview-regression's "Preflight (lint + format)" job both fail on it, so preview-regression is red too. I see the same result locally with oxfmt --check. Two spots need fixing:
- line 31: the
./previewStageimport is over the print width and needs wrapping onto multiple lines; - lines 70–71: two blank lines are left where the stage constants were moved out.
Running bunx oxfmt packages/studio/src/components/nle/NLEPreview.tsx fixes both.
Resolved since the last head:
- The description now has Before and After captures, and the PR captures check passes.
- The description now matches the code (
loadingUi: "studio" | "host"), including thefails without the fix:line. - The Comments check passes.
Code: no issues found.
studioDraws(wait, loadingUi)gates only the drawing, andonCompositionLoadingChangestill reports the waits, so the default andPreviewPanebehaviour is unchanged.- The constants moved into
previewStage.tsare unchanged, and their only importers areNLEPreview.tsxandNLEPreview.test.ts. tsc --noEmit -p packages/studiois clean.Player.test.tsandNLEPreview.test.tspass 50/50.
Noted, not blocking: the "Found on the way" finding that the asset wait never runs on a real first load (the about:blank load counts as load 1) deserves its own issue.
|
Fixed in |
There was a problem hiding this comment.
Re-reviewed at 3a7d5839. Approving.
- Formatting only: the only change since
5825d4abis one fast-forward commit,3a7d5839 style(studio): format NLEPreview's stage import. It wraps the./previewStageimport and removes the extra blank line. No behaviour changed. oxfmt --checkpasses onNLEPreview.tsx,previewStage.tsandPlayer.tsx.- CI at this head: all 8 workflow runs completed successfully: CI, preview-regression, Comments, PR captures, regression, Player perf, Windows render verification and CodeQL.
My review of 5825d4ab covers the code: no issues, and the fails without the fix: test was verified earlier.
What
PlayerandNLEPreviewtake a new prop,loadingUi: "studio" | "host"(default"studio"). With"host", the host draws its own loader, so Studio draws neither of its preview loaders: neither the "Loading composition" cover nor the "Preparing preview assets" wait.onCompositionLoadingChangestill reports that the preview is loading.suppressLoadingOverlayis unchanged.Why
A host that draws its own loader over the preview had no way to turn off both of Studio's.
suppressLoadingOverlayhides only the composition cover, and the asset wait is its own black overlay.Folding the asset wait into
suppressLoadingOverlaywould have changed Studio's own preview pane:PreviewPanepassessuppressLoadingOverlayafter the first load to skip only the composition cover on later loads, and those loads keep their asset wait. So "skip the cover on a later load" and "the host draws its own loader" are two props.Found on the way, not changed here
Studio's asset wait never starts on a real first load today. The player's iframe fires
loadforabout:blankfirst, so the composition's own load is load 2, andPlayertreats any load after the first as a content refresh and skips the asset scan. A log line inPlayer's load handler, on a Linux box at this branch's base, showed exactly that: the first load wasabout:blankwith no media, the second was the composition with its video still unloaded, and that second one skipped the scan as a refresh.So this PR is a guard: it keeps a host's loader alone on screen once that path runs again. Whether to revive the asset wait (count only the composition's load) or delete it is a separate product decision; this PR leaves it as it is.
Related work
The player element already has the matching opt-out for its own overlay,
assets-loading-ui="none".How
Player.tsxkeeps whether each wait is on (compositionWaitShown,assetWaitShown, the same conditions as before) apart from whether Studio draws it (studioDraws(wait, loadingUi)). The loading callback reads the waits, not the drawing, so callers hear exactly what they heard before.NLEPreviewpassesloadingUito the live preview slot.NLEPreview's stage constants (PREVIEW_STAGE_INSET_PX,SHADOW_IFRAME_STYLE,resolvePreviewStageSize) move unchanged intonle/previewStage.ts, andPlayerreadsloadingUifrom its remaining props instead of destructuring one more.Test plan
Player.test.ts. One loads a preview whose audio has no data withsuppressLoadingOverlayalone: the asset wait still shows (PreviewPane's meaning, unchanged). The other usesloadingUi: "host": neither "Preparing preview assets" nor "Loading composition" is drawn, andonCompositionLoadingChangewas last called withtrue.loadingUi: "host"test fails withexpected 'Preparing preview assetsWaiting for m…' not to contain 'Preparing preview assets'; the other passes.Before (shot-only setup)
Studio at this branch's base, preview pane passing
suppressLoadingOverlayas a host does, a video whose first 300 KB arrive and the rest is held: Studio's black asset wait covers the picture.After (shot-only setup)
The same at this head with
loadingUi="host": Studio draws nothing over the picture; the host's own loader would sit there.Both shots are shot-only setups, labelled as such and never committed: the preview pane edited to pass the host's props, the player's own overlay off with
assets-loading-ui="none"(as a host sets it), and one line inPlayerso the composition's load counts as the first (see "Found on the way"). Without that line neither state shows any Studio loader.What I measured
Player.test.ts+NLEPreview.test.ts: 50 of 50 pass, 3 runs in a row, on a Linux box.tsc --noEmit -p packages/studio: exit 0.fallow auditagainst the merge-base with--fail-on-issues: exit 0.comment-ratchet.mjsandcheck-comment-citations.mjsagainst the merge-base: both pass.NLEPreview.tsxis 560 lines.What I did NOT execute
NLEPreview; they already passsuppressLoadingOverlayand sit behind the live slot.Size
About 100 changed lines, half of them the unchanged stage constants moving file: one prop, one behaviour, the two tests that pin it, and the moves the size and complexity budgets asked for.