fix(web): validate the onboarding runtime before Continue is pressed - #7440
fix(web): validate the onboarding runtime before Continue is pressed#7440open-design-crew[bot] wants to merge 2 commits into
Conversation
Continue on the Local Agent step ran the runtime smoke test inside the click handler, so every user paid a full agent spawn plus a real model round trip between pressing it and reaching the next screen: 7.1s for Claude Code and 12.1s for Codex CLI measured against a local daemon, against a 45s budget. The button was only greyed out through that wait and still read "Continue", so it looked like the click had done nothing. Before #7164 the click itself was instant, because the validation came from the separate Test button. Start the validation as soon as the selection settles instead, the way BYOK already validates a settled provider, so it overlaps with the time the user spends reading the panel and picking a model. Continue then takes the already-validated result with no round trip at all. #7164's guarantee is unchanged: onboarding still cannot complete on an unproven runtime. `startOrJoinInlineTest` keeps that to one round trip per selection. A click landing mid-flight joins the pass already validating those inputs rather than being swallowed or spawning the agent a second time, and an attempt whose inputs the user has moved past is aborted — the daemon kills the agent child along with the request. A background pass no longer disables Continue; only a click that is itself waiting marks the button busy, with a spinner and "Testing connection…" in place of the label. Internal report: OPEND-2281.
lefarcen
added
size/L
|
🧪 This PR has changes that need a manual QA pass before merge — please hold off self-merging for now; we'll loop QA in once it's merge-ready. |
The surrounding sentences already carry the reason the validation has to be under way before Continue is pressed, so the id added nothing a reader of this tree could act on.
Visual regression reviewHead: 2 changed · 51 unchanged · 0 new without baseline · 0 failed Changed cases
Unchanged cases
Visual diff is advisory only and does not block merging. |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found two non-blocking lifecycle issues in the new onboarding validation flow; both are detailed inline below. The background Local Agent validation and Continue join behavior are otherwise covered by the passing unit and Playwright checks.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| const providerModelsAutoFetchKeyRef = useRef<string | null>(null); | ||
| const providerAutoTestKeyRef = useRef<string | null>(null); | ||
| const agentAutoTestKeyRef = useRef<string | null>(null); | ||
| const agentTestRunRef = useRef<OnboardingInlineTestRun | null>(null); |
There was a problem hiding this comment.
These new run refs are only aborted when a different input starts another run. The existing unmount cleanup (the useEffect around lines 2331–2339) and handleBackWithTracking only clear reveal timers/state, so leaving this setup step—Back, switching Local to BYOK, navigating away, or closing onboarding—leaves the automatic /api/test/connection request alive. For a local agent this can keep a spawned CLI running until the daemon timeout, and repeated abandoned visits can consume model calls and processes. Please add a shared cleanup that aborts both refs when the component unmounts and when setup/runtime is left, then cover Back/unmount with a hung-request test.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| const model = config.model; | ||
| const apiVersion = | ||
| protocol === 'azure' ? config.apiVersion?.trim() || undefined : undefined; | ||
| return startOrJoinInlineTest(providerTestRunRef, inputKey, async (signal) => { |
There was a problem hiding this comment.
startOrJoinInlineTest now accepts a signal, but BYOK's auto-validation effect still returns while providerTestState.status is running (the effect at lines 3443–3458). If the user edits the key, base URL, or model during an in-flight request, providerTestInputKey changes and the effect exits without scheduling the new input; no new test begins until the old request resolves or times out, unless the user clicks Test or Continue. That leaves the selected configuration unvalidated and defeats the intended cancellation path. Remove that running-state early return, or otherwise schedule through this helper, so the changed-input run aborts the old request and starts immediately; add a regression test for editing while auto-validation is held.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.2 participants
Why
A user reported that after picking a local agent in onboarding, pressing Continue takes a long time to reach the next screen — "it used to go through instantly." I picked the report up, reproduced it, and traced it to a regression window.
#7164 moved the runtime connectivity check into the Continue click handler (
handlePrimaryAction). That check is not a lightweight probe — it spawns the agent CLI as a child process, sends a smoke prompt, and waits for a real model reply. Measured against a local daemon (/api/test/connection):--model haiku)DEFAULT_AGENT_TIMEOUT_MSbudgetSo every user on the local-agent path stares at a greyed-out button still labelled "Continue" for several seconds after clicking it, which reads as the click having done nothing. Before #7164 that click was instant, because the validation came from the separate Test button next to it — which is exactly the "used to go through instantly" the report describes.
#7164's intent is right: nobody should finish onboarding carrying a runtime that cannot actually run. This PR keeps that guarantee and only moves the check off the click's critical path.
What users will see
Surface area
settings.testRunningScreenshots
The change is mostly about timing and button state, which a still frame does not capture well. Reproducible comparison:
The panel's visual archive is covered by
visual-onboarding-local-agent-paneline2e/ui/visual-entry.test.ts; this PR adds a wait there so the capture lands on the settled state instead of racing the transient "testing" copy.Bug fix verification
apps/web/tests/components/EntryShell.onboarding.test.tsxvalidates the selected Local Agent in the background so Continue does not wait on a spawnlets Continue join the in-flight Local Agent validation instead of dropping the clickmain(d88941ba0, which already includes the just-merged fix(web): allow signed-out Local CLI and BYOK onboarding #7381): red — both stop atexpect(testCalls).toBe(1)receiving0, i.e. nothing validates before the click. The other 43 cases in the same file stay green, so the failure is precise. On this branch all 45 pass.mainto 3083 ms on this branch. The 3083 ms floor is the harness's own navigation work and is identical in both runs, so roughly 6 s of dead wait is gone.Validation
pnpm guard✅pnpm typecheck(whole repo) ✅pnpm --filter @open-design/web typecheck✅apps/webonboarding unit tests: 45 passed ✅ (after confirming the two new cases go red on themainbaseline)apps/webcomponent suite (tests/components, 387 files / 4166 tests) ✅ — run before rebasing onto fix(web): allow signed-out Local CLI and BYOK onboarding #7381e2e/ui/amr-onboarding.test.ts: 33 passed ✅e2e/ui/visual-entry.test.tslocal-agent capture ✅/api/test/connectionrequest reaps the spawned agent child —startOrJoinInlineTest's cancellation path depends on itAdjacent issue (fixed here because it is a direct consequence of this change)
BYOK had the identical dead-button window: Continue was held
disabledwhile its auto-validation ran. RemovingconnectStepTestRunningfromdisabledwould have turned that from "greyed out" into "click silently dropped", so both paths now share the same join-the-in-flight-validation logic.