fix(ui): make first-run onboarding recoverable instead of looping on a finish error - #11884
fix(ui): make first-run onboarding recoverable instead of looping on a finish error#11884roninjin10 wants to merge 1 commit into
Conversation
…a finish error
The in-chat first-run wizard could loop forever with no escape. On any
finish/provision error, `seedError` re-appended the runtime CHOICE
(`${message}\n\n${RUNTIME_CHOICE}`), so a persistent failure — e.g. the
"Not found" 404 from `POST /api/first-run` — re-offered the same runtime
question indefinitely with no distinct error surface and no way out. The
"Other / configure in Settings" provider pick made it worse: it ran a local
finish that hit the same 404 and re-looped instead of ever opening Settings.
This is the UX/navigation fix (the underlying 404 is a separate backend/env
issue, out of scope):
- Finish errors now seed a DISTINCT, non-looping recovery turn
(`first-run:error:*`) with a human message and a dedicated
`[CHOICE:first-run id=error]`: Try again (`error:retry`, re-runs the last
runtime's finish), Choose a different way to run (`error:restart`, re-offers a
fresh unlocked runtime CHOICE), and Configure in Settings (`error:settings`).
- "Other / configure in Settings" (`provider:other`) now opens the Settings tab
(`setTab("settings")`) and exits first-run (`completeFirstRun("settings")`)
via a shared `exitToSettings` helper, latched by `completedRef` so a
double-tap can't flip the gate twice.
- Raw terse errors ("Not found", "Failed to fetch", …) are wrapped in a clear,
human sentence via `finishErrorMessage`.
Local-success, cloud, and needs-cloud-login paths are unchanged.
Tests: updated the affected conductor tests and added coverage for the
persistent-404 no-loop + retry + Settings escape, the cloud `error:retry`
re-run, and `provider:other` -> Settings exit. Also hardened `beforeEach` to
restore default mock implementations (clearAllMocks keeps implementations, so a
leaked mockRejectedValue would poison later tests). Full src/first-run suite:
191 passed.
Closes elizaOS#11882
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
HOLD — the error-recovery half is right; the To merge: restore the |
…instead of dead-ending to Settings; honest cloud-failure copy; completedRef guard (#11884 review fixes, fuzz pending) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Closes #11882
The bug
The in-chat first-run wizard could loop forever with no escape, so a fresh install can never finish configuring a model provider:
Selecting "Other / configure in Settings" did not escape either — it re-ran the same finish flow, hit the same failure, and re-looped.
Root cause (UX/navigation)
In
packages/ui/src/first-run/use-first-run-conductor.ts:seedError(message)seeded a turn as`${message}\n\n${RUNTIME_CHOICE}`— it re-appended the runtime question. A persistent finish error (the 404 fromPOST /api/first-run) therefore re-offered the same runtime choice indefinitely, with no distinct error surface, no explanation, and no way out.provider:other("Other / configure in Settings") ran a local finish (configure-later) that, on the same 404, also fell intoseedErrorand re-looped instead of ever opening Settings.Scope note — the 404 is NOT fixed here (it's a separate issue)
The underlying
POST /api/first-run"Not found" 404 is a backend/dev-environment problem (the local app-core/agent server not serving/api/first-run) and requires a running environment to diagnose. This PR does not touch it. It fixes the UX/navigation defect so onboarding is recoverable — retry + an explicit escape into Settings — even when finish keeps failing.The fix
first-run:error:*) with a human-readable message and its own[CHOICE:first-run id=error]:error:retry) — re-runs the last runtime's finish (local re-POSTs; cloud re-provisions, re-seeding the OAuth turn).error:restart) — re-offers a fresh, unlocked runtime CHOICE so the user can switch cloud↔local.error:settings) — the guaranteed escape.The runtime question is never re-appended inline again, so a repeating error can't loop the greeting forever.
provider:otheropens the Settings view viasetTab("settings")and exits first-run viacompleteFirstRun("settings")(route id"settings", the same id used everywhere inApp.tsxand the existing needs-provider banner infirst-run-finish.ts). It no longer runs a finish flow that could fail and trap the user.exitToSettingshelper, latched bycompletedRefso a double-tap can't flip the gate twice.finishErrorMessage.Local-success, cloud, and needs-cloud-login paths are unchanged.
Before / after behavior
setTab("settings")) and exits first-runerror:retryre-runs cloud provisioningTests (primary evidence — the conductor is headless)
bun run --cwd packages/ui test src/first-run/— 191 passed (15 files), including:POST /api/first-run404 does not re-loop the runtime question — the error turn is distinct (human message, noruntime:localbutton),error:retryre-attempts (2× POST, one bounded error turn per attempt), anderror:settingscallssetTab("settings")+completeFirstRun("settings")to escape.error:retryafter a cloud listing failure re-seeds the OAuth turn and re-runs provisioning (0 agents → auto-provision → tutorial).provider:otheropens the Settings tab and exits first-run without any finish flow / POST / model download / tutorial, and a double-tap flips the gate exactly once.error:restart→ runtime re-pick path.beforeEachrestores default mock implementations (clearAllMockskeeps implementations, so a leakedmockRejectedValuewould poison later tests).use-first-run-conductor.fuzz.test.ts) still hold all invariants (≤1 POST, ≤1 provision, ≤1 completeFirstRun, bounded transcript).Verification run
bun run --cwd packages/ui typecheck— clean.bunx @biomejs/biome checkon changed files — clean.bun run --cwd packages/ui test src/first-run/— 191 passed.Evidence checklist
/api/first-run404 (a separate, out-of-scope environment issue). The conductor logic is fully covered by the real finish use case under test.🤖 Generated with Claude Code