Rework onboarding around one default harness - #3574
Conversation
|
🤖 ## Screenshots Choose a default harnessThe chooser is a pure local selection. Clicking a card advances immediately; setup is deferred to the selected harness. Install a missing harnessThe setup page explains what is missing, runs installation locally, and keeps Finish disabled until the harness is ready. Configure provider and modelProvider-backed harnesses show only the required first-run defaults. Labels stay accessible while the compact onboarding treatment uses field placeholders. |
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
b477195 to
917a7ae
Compare
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Reviewed base bbd20fae75ecc3bd7a83cc12a65379fac22a2b79 through head 0027b461bf817a69416973fe80ae5e4d74097c0a.
Blocking finding
[P1] Fail closed when the existing global agent config cannot be read — desktop/src/features/onboarding/ui/DefaultConfigStep.tsx:561-585,622-624,650-658
loadDefaults() uses Promise.allSettled, but a rejected getGlobalAgentConfig() is silently ignored. The component therefore leaves config at EMPTY_GLOBAL_CONFIG and clears isConfigLoading. For an already-ready Claude/Codex selection, canFinish becomes true. Finish then persists the empty fallback plus the selected runtime, erasing unread provider/model values and credential environment variables from the user's existing config.
This is not hypothetical: a temporary sequenced Playwright probe seeded non-empty config, allowed the app's initial read to succeed, rejected the config-page read, and asserted that Finish remained disabled. It failed causally with Expected disabled; Received enabled. The probe was removed afterward and the checkout returned clean.
Please track config-read failure as an explicit blocking/error state, offer retry/back, and do not enable Finish or call setGlobalAgentConfig until the existing config has loaded successfully. Add permanent E2E coverage proving a rejected read cannot Finish or mutate persisted config, then that retry succeeds and preserves all existing fields except the intended runtime change.
Validation
At exact clean head 0027b461bf817a69416973fe80ae5e4d74097c0a:
- Desktop check and typecheck passed (two pre-existing informational
useTemplatediagnostics outside the PR diff). - Full Desktop JS suite passed: 3,769/3,769.
- Focused onboarding Playwright suite passed: 20/20 after a clean rerun. An earlier local Python server exit caused four
ERR_CONNECTION_REFUSEDinfrastructure failures after 16 passes; the rerun cleared them. - All applicable GitHub checks are green.
- Native GUI/lifecycle validation was not run; focused Playwright exercised the changed UI.
- No relay/schema/identity/release changes were found in the 16-file three-dot diff.
- GitHub currently reports the PR as conflicting/dirty. Conflict resolution will create a new head and require fresh delta review.
The happy path is well covered. The read boundary currently fails open and can destroy saved credentials, so this head is not safe to merge.
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
…nt-config Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — changes requested at 8b85a2b315b686b4abf6a43e8a0d1e570004a72c.
The config-read failure is now fenced on initial load, but this head still has two material blockers.
[P1] Retry re-enables Finish before the replacement config read succeeds
desktop/src/features/onboarding/ui/DefaultConfigStep.tsx:94-100 sets isLoading=true but also clears configLoadError before getGlobalAgentConfig() settles. The completion gate at :220-235 checks !configLoadError but not loading, and handleComplete at :350-367 does not defensively re-check persistenceState.canComplete.
After Back preserves a valid dirty draft, a failed read followed by Try again therefore makes Finish actionable during the pending retry. The stale commit can overwrite newer/external config before disk state is known.
A causal Playwright probe delayed the successful retry read by 2s and asserted Finish was disabled immediately after Try again. It failed with Expected disabled; Received enabled. Please include loading in the completion fence, guard handleComplete with the current completion state, and add a deferred-read regression proving no completion/write occurs until retry succeeds.
[P1] The merge replaced the PR’s single-harness journey with the old multi-harness setup
The PR contract says: choose one default harness, advance immediately, then configure only that harness. Instead, desktop/src/features/onboarding/ui/SetupStep.tsx:619-642 renders Set up your agent harnesses and a RuntimeCard for every visible runtime. At :694-729 it collects every ready runtime and advances only through a generic Next button; MachineOnboardingFlow.tsx:539-550 receives that entire set. The intended chooser is now dead code: repo-wide search under desktop/src and desktop/tests finds HarnessChoiceCard only in its own definition.
This is user-visible and opposite to the PR title/body: newcomers again face installation/sign-in choices for all harnesses rather than one selected default. The changed screenshot journey catches it: the focused onboarding/docked-CTA run fails because Choose your default harness is absent (onboarding-docked-cta-screenshots.spec.ts:129-131; 3 passed, 1 failed). The rewritten defaults spec passing does not validate the promised journey.
Please restore the single-choice setup behavior, scope the following page to only that runtime, and restore a journey test proving card click immediately advances, other harness controls are absent, Back preserves selection, and no config is written before Finish.
Validation at this exact head
- Live PR head and local clean HEAD:
8b85a2b315b686b4abf6a43e8a0d1e570004a72c git diff --check f88cda9eb886500ec7d205e1d265ac6f654aa433...HEAD: passpnpm --dir desktop check: pass with four diagnostics in unchanged filespnpm --dir desktop typecheck: pass- Full Desktop JS suite: 5,099/5,099 pass
- Focused config rejection/preserve/retry regression: pass; mutation-removing the fail-closed branch makes it fail
- Focused defaults spec: 26/26 pass
- Onboarding/docked-CTA impact run: 3/4; fails on missing chooser heading
- Deferred retry mutation probe: fails because Finish is enabled while read is pending
- GitHub checks at this head are non-green: Desktop Smoke E2E (2), Desktop Smoke E2E (3), Desktop E2E Integration (1/2), and aggregate Desktop E2E Integration fail
- Scope is 11 Desktop frontend/E2E files; no relay, schema, identity, or release changes found
- Native GUI was not launched on the shared machine; Playwright exercised the renderer journey
The initial read-error repair is substantial, but a timed persistence hole and removal of the feature’s primary journey both remain merge-blocking.



Summary
Product behavior
preferred_runtimeis persisted only on Finish.Testing