Repository navigation
🤖 perf: swap the static first-run view for the live onboarding wizard in one commit (T3 C step 2 PR 1c) - #6381
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ThomasK33
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01c5b69159
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…unt (PR 1c review) Codex P2 on #6381: clicking Next cleared the dialog and overlay's animation:none while both stayed open, so their entry animation replayed on step 2. Only the step's skip clears on Next now. The test checks the dialog and overlay stay without animation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2702e7ec65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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 P2 on #6381: the handoff tests matched the loading and gateway prose, so a reword would fail them with no behavior change. The step element now carries data-onboarding-step={key}, and the tests read it.
Dogfood on
|
| Run | pre+loading frames | pre+live-dialog frames | empty frames | dialog rect before = after | inline animation after | console errors |
|---|---|---|---|---|---|---|
#xum-pre no-auth 390 / 1440 |
0 / 0 | 0 / 0 | 0 / 0 | yes / yes | none / none | 0 / 0 |
#xum-pre token 390 / 1440 |
0 / 0 | 0 / 0 | 0 / 0 | yes / yes | none / none | 0 / 0 |
| plain no-auth 390 / 1440 | 0 / 0 | n/a | n/a | n/a | unchanged | 0 / 0 |
| plain token 390 / 1440 | 0 / 0 | n/a | n/a | n/a | unchanged | 0 / 0 |
The static buttons look disabled because my hand-made copy is inert, and at 1440 px the page behind the overlay is the mobile layout I copied it from. Both come from the injected copy, not from this PR.
390 px, no-auth: static view, then live wizard
1440 px, token: static view, then live wizard
Without #xum-pre (unchanged), 390 px and 1440 px, no-auth
Other runs (390 token, 1440 no-auth, plain token)
Per-run JSON and the scripts are on the perf host in /home/coder/perf-runs/t3/c2-pr1-gates/dogfood/.
Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high • Cost: $311.46
|
Perf owner: gate decisions for the PR 1 stack (#6378, #6379, #6381), on the table above. The stack does not merge yet. It waits for gates 4 and 5.
Generated with |
|
Perf owner: gate 4 passes for the PR 1 stack. This is the workspace-page Lighthouse A/B on the dedicated VM. Base is
Gates 1, 2, 3, 4 and 6 now pass. Gate 5 (page timing) still runs on the shared host and waits for a quiet window. The stack merges only after gate 5 passes. Generated with |
|
Perf owner: gate 5 passes, so the whole PR 1 stack (#6378, #6379, #6381) is ready to merge. Page timing on the dedicated VM (
The earlier local attempt was inconclusive (the P2 A/A missed its noise limit twice), so this VM run replaces it. All six gates now pass: first-load +538 B (budget 1,024), React Compiler 23/24, first-run Lighthouse +1.5 / +0.0 points, workspace-page VM A/B −17.3 / −4.0 ms, page timing, and dogfood. Each PR has Required green, 0 open threads, and a Codex 👍 on its exact head. Generated with |
…unt (PR 1c review) Codex P2 on #6381: clicking Next cleared the dialog and overlay's animation:none while both stayed open, so their entry animation replayed on step 2. Only the step's skip clears on Next now. The test checks the dialog and overlay stay without animation.
Codex P2 on #6381: the handoff tests matched the loading and gateway prose, so a reword would fail them with no behavior change. The step element now carries data-onboarding-step={key}, and the tests read it.
320e63a to
30d83a9
Compare
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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 Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
… step 2 PR 1c) Test only, green on main: without #xum-pre the wizard shows its loading step, then the gateway step, with the dialog, overlay and step entry animations.
… in one commit (T3 C step 2 PR 1c) Under #xum-pre the wizard renders nothing while it loads, so its loading step never paints over the view. Its first real commit removes the view in a layout effect and opens the dialog, overlay and step without entry animations, which return once the user moves on. A wizard that opens past the gateway step removes the view too. Dialog and SplashScreen get a skipEntryAnimation prop. Inert without #xum-pre.
…unt (PR 1c review) Codex P2 on #6381: clicking Next cleared the dialog and overlay's animation:none while both stayed open, so their entry animation replayed on step 2. Only the step's skip clears on Next now. The test checks the dialog and overlay stay without animation.
Codex P2 on #6381: the handoff tests matched the loading and gateway prose, so a reword would fail them with no behavior change. The step element now carries data-onboarding-step={key}, and the tests read it.
… 1a) After 1a's squash merge, the stack sits on main, where #6323 made OnboardingWizardSplash call useLiveAPI(). The test's API mock exported only useAPI, so every wizard handoff test threw "useLiveAPI is not a function". The mock now returns null for it, as useLiveAPI does with no API.
30d83a9 to
930eb40
Compare
|
@codex review Perf owner: #6379 (1b) merged as Generated with |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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 Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…an error or a crash (T3 C step 2 PR 1a) (coder#6378) ## Summary The app removes the static first-run view (`#xum-pre`) whenever it will show something other than the live onboarding wizard: the token prompt, a startup error, the reconnect screen, or an uncaught render error. The entry also registers the service worker when it runs after `load`. Nothing inserts `#xum-pre` yet, so nothing changes for users. **Stack** (T3 coder#5971, Option C step 2, PR 1 split at the 300-line limit into 3 layers, each inert without `#xum-pre`): 1. coder#6378: entry and connection guard (this stack's bottom) 2. coder#6379: splash queue and paint cache 3. coder#6381: wizard handoff Plan: section 5.6 "C step 2: the static first-run view on `/` (P1)" of the T3 Phase 3 plan. Nothing in this stack inserts `#xum-pre`: a later PR (4b) makes `xum server` paint it for `/`. Until then every change here is a no-op, and the gates (Lighthouse, page timing, first-load bytes) run once on the stack's top against the merge base; their table is on coder#6381. ## Implementation - `src/common/constants/firstRunView.ts`: `FIRST_RUN_VIEW_ID = "xum-pre"`, the id `scripts/perf/pageTimingProbe.ts` already treats as a static view. - `SplashScreenProvider.tsx`: `removeFirstRunView()`, shared by every removal path. - `AppLoader.tsx`: `FirstRunViewConnectionGuard` removes the view on `auth_required`, `error` and `reconnecting` (a lost connection must not leave the static view over the reconnect UI). It keeps the view while `connecting`, `connected` and `degraded`. - `main.tsx`: - A boundary around `AppLoader` removes the view on an uncaught render error, then renders the empty root such an error leaves today and reports the error with `window.reportError`. - The service worker registers at once when `document.readyState === "complete"`. C step 2 starts the app JS after the static view paints, which can be after `load`, and then the `load` listener never fires. ## Validation - First commit (`ba5667bf0f`) is test only and green on main: the entry registers the service worker on `load`. - New tests (jest, `tests/ui/onboarding/`), each failing under the named mutation: | Test | Mutation that fails it | |---|---| | registers at once when the entry runs after load | E1: keep only the `load` listener | | an uncaught render error removes the view and leaves the root empty | E2: no `removeFirstRunView()` in `componentDidCatch`. E3: swallow the error instead of `reportError` | | connection guard, 6 statuses | G1: `auth_required` not blocked. G2: `reconnecting` not blocked. G3: remove on every status | - `make check-react-compiler`: 23/24. `make crap-check`: pass. Typecheck (3 configs) passes. ESLint on the changed files passes. The full-repo ESLint run was OOM-killed twice on the shared host, so CI's Static Checks job is the full lint for this PR. ## Risks Low. Every change is a no-op without `#xum-pre`. The new boundary changes only how an uncaught render error ends: the root is empty either way, and the error is still reported as uncaught. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$289.12`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=289.12 -->
…, and cache a viewed wizard (T3 C step 2 PR 1b) (coder#6379) ## Summary `SplashScreenProvider` removes the static first-run view (`#xum-pre`) once its splash queue is known and the onboarding wizard is not first. It also records a new paint cache, so the server's early script (a later PR) can skip its API calls for a browser that has seen the wizard marked viewed. Nothing inserts `#xum-pre` yet. **Stack** (T3 coder#5971, Option C step 2, PR 1 split at the 300-line limit into 3 layers, each inert without `#xum-pre`): 1. coder#6378: entry and connection guard (this stack's bottom) 2. coder#6379: splash queue and paint cache 3. coder#6381: wizard handoff Plan: section 5.6 "C step 2: the static first-run view on `/` (P1)" of the T3 Phase 3 plan. Nothing in this stack inserts `#xum-pre`: a later PR (4b) makes `xum server` paint it for `/`. Until then every change here is a no-op, and the gates (Lighthouse, page timing, first-load bytes) run once on the stack's top against the merge base; their table is on coder#6381. ## Implementation - `useFirstRunViewQueueGuard(loaded, wizardIsFirst)` removes the view in a layout effect when the queue is loaded and the wizard is not first: already viewed, the viewed list could not be read, splash screens off or no API, or another splash comes first. - `onboardingWizardViewed` (`storage.ts`, registered as `"cache"` like `uiTheme`): `cacheViewedWizard()` sets it when the loaded viewed list contains the wizard id, and after `dismiss()` marks the wizard viewed. - coder#6351 (open) changes the same file to keep a shown splash on a lost connection. That rule stands: a lost connection is the `reconnecting` status, where coder#6378's guard removes the static view, not the queue. ## Validation | Test (jest, `tests/ui/onboarding/providerHandoff.test.ts`) | Mutation that fails it | |---|---| | keeps the static view while the wizard is first | Q2: remove before the queue is loaded | | removes it when viewed / read failed / another splash first / no API | Q1: no queue guard | | caches a viewed wizard on load and on dismiss | Q3: no load-time write. Q4: no dismiss write | - `make crap-check` passes: the effect and the cache write are a named hook and a function with two callers, so `SplashScreenProvider` keeps its CC. - The storage registry tests (`storage.test.ts`, `persistedStateBudget.test.ts`, `legacyLocalStorageCleanup.test.ts`) pass with the new key. ## Risks Low. Without `#xum-pre` the only change is one new 4-byte localStorage value per browser (`true`), written when the wizard is viewed. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$289.12`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=289.12 -->
…T3 C step 2 PR 2) (coder#6444) ## Summary Adds `FIRST_RUN_VIEW_HTML` in `src/node/orpc/firstRunView.ts`: the static markup of the onboarding wizard's gateway step ("Xum Gateway (evaluation credits)"). A later PR paints it on `/` before the app JS runs. Nothing imports or serves it in this PR, so no response and no bundle changes. A new happy-dom test fails when the constant drifts from the live wizard. ## Background Option C step 2 of coder#5971 (plan section 5.6, PR 2 of 5). PR 1 (coder#6378, coder#6379, coder#6381) made the app hand over from a `#xum-pre` element to the live wizard in one commit. This PR adds the markup that element will hold. PR 4a adds the early script and PR 4b injects both into the `/` response. ## Implementation - The markup comes from the prototype's capture of the live first-run page at 412 px (61 elements), with these edits: - The app shell's `<main>` is a `<div>`. The boot loader's `<main>` in `#root` stays the only landmark. - Radix runtime artifacts are removed: focus guards, generated ids, `aria-hidden`, `tabindex` and `pointer-events`. - The `disabled` attributes are removed. The live buttons are enabled, and `disabled:opacity-50` made the copy look disabled in PR 1's dogfood screenshots. - The overlay and the dialog use the live class strings. The capture had lost their `data-[state=open]` entry classes, and the new test found that. Those classes do nothing in the copy, because it has no `data-state` attribute. - The root is `<div id="xum-pre" inert>` (the shared `FIRST_RUN_VIEW_ID`), so the whole copy takes no focus and no clicks before the handoff. - The shell's root `<div>` is `h-full` of `#xum-pre`. Outside `#root` (which `globals.css` sizes to the viewport), its nested `h-full` would otherwise collapse to the content height (Codex P2, fixed in `4de5a0bdbb`). - `#xum-pre` has `z-index:1500`, the live overlay's layer. As a fixed element it is a stacking context, so without it the iOS standalone `.ios-status-bar-band` (`z-index: 1001`) would paint over the copy (Codex P2, fixed in `1a76083472`). - SECURITY AUDIT comment at the constant: it is trusted server text, nothing user-controlled goes into it, and PR 4 inserts it with `template.content.cloneNode(true)`, never with `innerHTML`. - `tests/ui/onboarding/firstRunViewParity.test.ts` parses the constant from a `<template>` with `content.cloneNode`, the way the page will. It puts the result in the body as `#xum-pre` and renders `OnboardingWizardSplash` over it (directly, because `DISABLE_SPLASH_SCREENS` is true in test mode). PR 1's handoff then removes the copy. The test compares the tags, classes and own text of every overlay and dialog element with the live ones. It also checks that the root is the inert `#xum-pre` and holds no `<main>`. - Mutation it catches: changing one class on the live step (`gap-3` to `gap-2` at `OnboardingWizardSplash.tsx:1198`) fails the test. - The app shell behind the dialog is not part of the wizard component, so this test does not cover it. PR 3's box-parity tool checks the paragraph's position in a real browser. ## Validation Measured on the perf host, base `7b26188e1a` (origin/main) against head `4de5a0bdbb` (build gates rerun after the review fix; `make static-check` and React Compiler on `e812b9eb05`, CI covers the final head). Files: `/home/coder/perf-runs/t3/c2-pr2-gates/`. | Gate | Result | Numbers | |---|---|---| | First-load JS, normal build pair (exact `dist/<file>.br` sum) | Version-stamp noise only | 465,239 -> 464,905 B (-334 B; -332 B on `e812b9eb05`, raw sizes identical). With a pinned `src/version.ts`: 465,239 -> 465,239 B (0 B) | | Control build, pinned `src/version.ts`, full `dist/` sha256 | Renderer and `dist/index.html` byte-identical | Only difference: added `node/orpc/firstRunView.js` and `.js.map` (main-process tsc output, not imported) | | `make static-check` | Pass | exit 0 | | `make check-react-compiler` | Pass | 23/24 | | `make crap-check` | Pass | 0 violations, 0 warnings | | `tests/ui/onboarding/firstRunViewParity.test.ts` | Pass | 2/2. Mutation `gap-3` -> `gap-2` on the live step fails it | No Lighthouse and no page timing: this PR changes no served response. ## Deferred to PR 4b (blocking precondition, owner decision 10-14) At desktop widths the static shell behind the dialog is the 412 px capture's mobile layout ("Open sidebar" header), while the live first paint shows the default expanded 288 px `LeftSidebar` (Codex round 2). Nothing serves this constant until PR 4b. **PR 4b may not merge until the static shell behind the dialog matches the live first paint at 390, 744, 1200 and 1900 px** (the default expanded 288 px `LeftSidebar` on desktop widths, or a documented neutral background that the live first frame also shows). That check joins PR 4b's parity gate. ## Risks Low. The constant has no importer, so no runtime path changes. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$322.92`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=322.92 -->
1 participant
Summary
The live onboarding wizard takes over from the static first-run view (
#xum-pre) without a flash. Under the view it renders nothing while it loads, so its "Checking your provider configuration…" step never paints over the view. Its first real commit removes the view in a layout effect and opens the dialog, overlay and step without entry animations, because the user already sees that frame. The step animations return when the user moves on. Without#xum-pre, the wizard renders exactly as today.Stack (T3 #5971, Option C step 2, PR 1 split at the 300-line limit into 3 layers, each inert without
#xum-pre):Plan: section 5.6 "C step 2: the static first-run view on
/(P1)" of the T3 Phase 3 plan. Nothing in this stack inserts#xum-pre: a later PR (4b) makesxum serverpaint it for/. Until then every change here is a no-op, and the gates (Lighthouse, page timing, first-load bytes) run once on the stack's top against the merge base; their table is on #6381.Implementation
OnboardingWizardSplash.tsx:useFirstRunViewHandoff(isLoading)reads#xum-preonce at mount. It hides the wizard while it is loading under the view and removes the view in a layout effect once loading ends. The dialog and overlay skip their entry animation for the whole mount: they stay open, so dropping the override later would replay it (Codex round 1, fixed in2702e7ec65). The step skips its entry animation until the user moves on.stepEntryClassName()picks the step's animation class.Dialog.tsxandSplashScreen.tsx: askipEntryAnimationprop setsanimation: noneon the dialog content and overlay.Validation
5c70d0ece6) is test only and green on main: without#xum-prethe wizard shows its loading step, then the gateway step, with the dialog, overlay and step entry animations (plan decision Q1: "the dialog renders the same classes and entry animation as today").tests/ui/onboarding/wizardHandoff.test.ts), each failing under the named mutation:useLayoutEffect→useEffectfor the removal): no test can tell them apart here. Radix mounts the dialog's portal content in a second synchronous commit (its own layout effect), and React flushes pending passive effects before that commit. So both versions remove the view and show the dialog in the same task, before any paint. The layout effect stays, because it does not depend on Radix's portal timing.make crap-checkpasses.OnboardingWizardSplashkeeps CC 125 (a touched-hotspot warning remains).make check-react-compiler: 23/24.Gates (whole PR 1, stack top against the merge base)
Measured on the perf host, base
1c66fd6811(merge base) against head320e63abe2. Every number comes from the gate'ssummary.mdin/home/coder/perf-runs/t3/c2-pr1-gates/<folder>/.dist/<file>.brsum)first-load-js/8786197d9d,3c0a4d302f,320e63abe2. CI Static Checks and Required success on all three headsstatic/lighthouse-p1/lighthouse-p2/aa-p1-2, after a failed first block at UCS 0.472 s). A/A P2 failed after one rerun: MDI UCS 0.256 s (limit 0.25), IR 0.075 s (aa-p2-2; earlieraa-p2-1invalid IR timeout,aa-p2-1-rUCS 1.352 s, IR 5.449 s). No gated A/B ran. Quiet waits 3,073 spage-timing/#xum-prehandoff in 4 runs (no-auth, token x 390, 1440): 0 frames with the loading step under the static view, 0 frames with both, 0 empty frames, same dialog rect before and after, inline animation none. Without#xum-pre: unchanged. Screenshots: commentdogfood/Risks
Low to medium. Without
#xum-pre, the change is two hook calls and one class helper in the wizard, plus an unused-by-default prop on the shared Dialog. The pin tests above cover that path.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$289.12