Studio: load the settings tab panels when they are shown, not at launch - #8966
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…down A panel is fetched the first time it is shown, so it can now fail where it could not before: offline, or a page whose entry bundle predates an in-place rewrite of dist/ and still names chunks that have been replaced. The dialog is mounted at the app root and nothing above it catches, so the throw unmounted the whole of Studio rather than one panel. Blocking a panel's module in a browser reproduced it: the dialog, its nav and the rest of the page went. The panel area now sits in an error boundary that offers a reload, and the Suspense fallback is a delayed loading line rather than an empty pane, so a slow first open shows something and a prompt one still shows no flash. Reload rather than retry: React caches a lazy rejection for the life of the page and the browser's module map caches the failed import, so re-importing the same URL rethrows without a new request. index.html is served no-store, so a reload does pick up the current chunk names. tests/settings-tab-panel-loading.test.ts gains a case that walks the JSX and asserts every panel Suspense is inside a class that defines getDerivedStateFromError. tests/studio/playwright_settings_tabs.py drives the real dialog in a browser: all twelve tabs, deep-open, the search jump, and the blocked-module case.
for more information, see https://pre-commit.ci
The idle prefetch warms every panel once the dialog opens, so a chunk it cannot fetch reached the page as an unhandled rejection for a tab nobody had asked for. Reproduced by blocking one panel's module in a browser: the rejection landed on window even though the boundary handled the panel that was actually on screen.
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ling on a placeholder tsconfig.app.json lists the three existing smoke entries explicitly, so the new one was outside the project and npm run typecheck skipped it. Added, and the harness handle it installs on window is now optional, since app code sits in the same project and must not be able to reach a handle only the harness page installs. tsc --listFiles now names the file. The Playwright harness settled on whatever held still for 600ms. The panel renders from a deferred value, so a switch keeps the outgoing content up until the incoming panel is ready, and on a loaded machine that hand-off outlives the window: one run read the placeholder as the final panel and called a correct recovery a failure. It now refuses to settle on something with almost no content. A run that dies on a cold dev server also writes its report instead of leaving none.
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
tests/test_source_read_encoding.py holds every checked-in file read and write in the test trees to an explicit utf-8, so it does not depend on the platform default and break on Windows the day the file gains a non-ASCII byte. The report write was the one that did not.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…heck Pre-existing flake in this step, not something this branch introduced. The step reads scrollTop straight after keyboard.press, but Radix scrolls the highlighted item into view off the back of that keypress, so the value is a mid-scroll sample: instrumented on the ubuntu CI image the viewport went on to settle 24-35px further down in 20 runs out of 20, on this branch and on its merge base alike. Two things break as a result. The stale sample is not the floor the wheel has to beat, which is why the failure reads '20 -> 44' as though the viewport had moved the wrong way when 44 is simply where the keyboard scroll ended up. And a wheel dispatched into a scroll Chromium is still animating can be swallowed outright, which is the actual failure: at a maximum scrollTop of 243 a working -400 wheel lands on 0 every time. So wait for the scroll to stop before taking the floor, keep the pointer inside a viewport that is not always 40px tall, and re-send the wheel on a bounded retry. A viewport that genuinely refuses the wheel still never moves and still fails, just after more tries.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
The abandoned-deep-open step routed every request through a handler that sleeps 2.5s, and that sleep runs on the driver thread, so the whole page's module load queued behind it and arrived at the main thread in one go. On a two-core runner sharing the box with a live Studio that pushed the reopen past its 15s timeout, which reads as a settings dialog that would not open when nothing was wrong with it. Route the Data module alone. The assertion is unchanged and still goes red on the pre-change store: the next ordinary visit to Data reopens the archive listing.
# Conflicts: # .github/workflows/studio-frontend-ci.yml # studio/frontend/tsconfig.app.json
Merged
|
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Vite dev proxies /api to 127.0.0.1:8888. With a Studio listening there and no token those calls answer 401, the app's auth handling navigates, and the harness window goes with it, after which every step times out waiting for a dialog that cannot exist. It happens on main too, where the harness is gone before the first open, so it says nothing about the panels. Report it.
Gate 1: is this worth merging, and does it break anythingVerdict: yes. The win reproduces, the rendered UI is identical, and nothing is silently lost. Three behaviours genuinely differ and are named below rather than left implied, including one that is a real regression and belongs in the PR body. The win reproducesRollup module graph of two production builds:
Nine interleaved fresh contexts per side, cache disabled, production builds, each side on its own loopback origin: DOMContentLoaded 263.0 ms to 81.6 ms, warm tab switch 59.6 ms to 50.5 ms. Ranges are non-overlapping on every row, so this is not inside the noise floor. One correction to how this should be quoted: the multi-megabyte decoded-bytes figure from that harness overstates the app, because the measurement entry imports only The rendered screen is unchangedLive Studio, real backend, real hardware, all 12 tabs, two passes per side: zero differences in labels, control inventory, values, checked and disabled state, ordering, positions or element counts. Every one of the 16 raw-text diffs is the two servers being two servers: live CPU and RAM samples, differing ports inside generated commands, and each server's own log filename. Old and absent state behaves identically too: first launch with no localStorage, state written by an older build, corrupt JSON, and a localStorage that throws are byte-identical between the two trees. No persisted format changed. The three behaviours that genuinely differ
The stale-panel window that No module-evaluation side effect was lostThis is the real risk in this class of change, so it was checked rather than assumed. An AST scan of all 72 lazified modules found 20 module-evaluation effects and every one is an inert CI triageCross-platform on staging is green on
Two test-robustness fixes pushed as a result: Stated gaps, rather than implied coverageThe hardware-exposing panels were exercised against a real CUDA backend on Linux with NVIDIA. That is one cell; staging covered Linux, macOS and Windows but CPU-only, and AMD, WSL and macOS-with-GPU were not run. No module that reads hardware changed eager or lazy status. Playwright WebKit is a proxy for the WKWebView, WebKitGTK and WebView2 that Desktop embeds, not those webviews, and shipping Safari cannot be tested here. A real deploy-in-flight chunk 404 was simulated by blocking the module, not reproduced against a live CDN. |
|
@codex review |
2 participants
SettingsDialogis rendered at the app root and is closed for the whole launch, but it statically imported all twelve of its tab panels, so the browser fetched, parsed and executed every one of them before the first paint.This loads each panel when it is first shown, and prefetches the rest on idle once the dialog has been opened, so a tab click never waits on a round trip.
Results
Ten interleaved fresh Chromium contexts per side against production builds with the HTTP cache disabled, each side served from its own loopback origin, alternating which side goes first. Medians.
394 KB less to decode. On a fast machine over loopback that buys about 10 ms and LCP does not move; the effect is in parse and execute, so it grows with how slow the machine is, and at 6x throttling it is 126 ms off DOMContentLoaded and 90 ms off LCP. The desktop app's embedded webview and the machines that report slow launches are the throttled end of that range, not the 1x end.
Two numbers worth stating plainly because they are smaller than a static reading of the bundle suggests. Walking the entry chunk's static import closure says the settings chunk is 2,493 KB of a 5,207 KB eager total, which looks like a much larger win than this. It is not: most of that chunk is code shared with the chat page, which is mounted at the root and is eager regardless, so removing the static edge mostly moved the attribution between chunks. The eager total falls from 5,224.8 KB to 4,832.2 KB raw, and 1,503.7 KB to 1,414.5 KB transfer, measured on this branch merged up to
2cf7a2888. The measured 394 KB is the real figure. Those two totals were 5,207.2 KB and 4,814.6 KB before that merge; the ~18 KB is upstream main's own growth, not this change, and both sides move together.Change
TAB_LOADERSmaps each tab id to a dynamic import;LAZY_TABSwraps them inReact.lazy. The panel area gets aSuspenseboundary.openis true,scheduleIdleTaskwalksTAB_LOADERSand warms the rest. The prefetch reads the same map the renderer does, so a tab cannot be lazy in one place and missing from the other.@/features/settingsbarrel are untouched, so no importer changes.What happens when a panel cannot be fetched
Fetching a panel is a new way for the dialog to fail, and it is worth being explicit about, because before this the panel code was already resident and could not.
lazyrejection for the life of the page, and the browser's module map caches the failed import, so re-importing the same URL rethrows without a new request.index.htmlis servedno-store, so a reload does pick up the current chunk names.Suspensefallback is a loading line that only appears after 300 ms, so a slow first open shows something and a prompt one, which is every panel on a local install, still shows no flash.The reachable case is a page whose entry bundle predates an in-place rewrite of
dist/:/assetsis servedimmutable, so already-fetched chunks keep working and only a not-yet-fetched one 404s. The desktop app is not exposed, since Tauri embeds the frontend in the binary and an update replaces the whole bundle and relaunches.Coverage
tests/settings-tab-panel-loading.test.tsparses the import declarations with the TypeScript compiler, rather than grepping, because a deferredimport(...)is a call expression and a static one is a declaration, which is exactly the distinction being bought:./tabs/*-tabimport, and has one deferred import per panel file on disksrc/statically imports a tab panel, since one such edge from any eagerly reached module puts all of them backTAB_LOADERS, and consumes a rejected loadSuspenseis inside a class that definesgetDerivedStateFromError, walked over the JSX rather than matched on texttests/studio/playwright_settings_tabs.pydrives the real dialog in a browser againstsmoke-settings.html: all twelve tabs render when selected, deep-open lands on the same panel as clicking to it, the settings search jumps to a row and flashes it, and a blocked panel module leaves the dialog and its twelve nav entries standing with another tab still working.Testing
npm test(2,786 passed)npm run typechecknpm run i18n:check:strictnpm run buildnpx eslinton the changed files, cleantests/studio/playwright_settings_tabs.pyon chromium, firefox and webkit, plus the blocked-module case. Every tab's settled panel is identical tomain's, as is deep-open and the search jump.tests/studio/playwright_settings_tabs.pyre-run on all three engines after mergingmain, plus the blocked-module case three times