fix(onboarding): separate apply and completion flows#1948
Merged
Conversation
- Purpose: split onboarding setting application from persistent completion so the final step owns completion explicitly. - Before: the Summary step applied settings and also called completeOnboarding, while Go to Dashboard and Reboot used separate exit paths that did not share a completion flow. - Problem: this mixed three concerns (apply, bypass, complete), made the Summary step finish onboarding too early, and left the final actions inconsistent. - Change: Summary now applies and validates settings only, and Next Steps now uses one shared completion helper for both dashboard and reboot actions. - How: removed completion/refetch from Summary, added a shared completeOnboarding path in Next Steps with non-blocking refetch handling and user-facing failure messaging, and updated regression tests/docs to lock the behavior in.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Disabled knowledge base sources:
WalkthroughThe onboarding completion responsibility is moved from Summary to Next Steps: Summary now only applies settings and classifies results; Next Steps runs the COMPLETE_ONBOARDING_MUTATION, refetches onboarding state (best-effort), performs cleanup, and then triggers reboot or the completion callback with error handling and UI state. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NextSteps as Next Steps Component
participant Apollo as Apollo useMutation
participant Server as Server (Onboarding)
participant Store as Onboarding Store
participant Boot as Boot System
User->>NextSteps: Click Confirm / Confirm Reboot
NextSteps->>NextSteps: finishOnboarding({reboot}) / set isCompleting, clear errors
rect rgba(100,150,200,0.5)
NextSteps->>Apollo: execute COMPLETE_ONBOARDING_MUTATION
Apollo->>Server: POST complete onboarding
alt mutation success
Server-->>Apollo: success
Apollo-->>NextSteps: success
else mutation failure
Server-->>Apollo: error
Apollo-->>NextSteps: error -> set completionError
end
end
rect rgba(150,200,100,0.5)
NextSteps->>Store: refetchOnboarding() (best-effort)
Store->>Server: fetch status
Server-->>Store: state
Store-->>NextSteps: updated (failures logged, non-blocking)
end
rect rgba(200,150,100,0.5)
NextSteps->>NextSteps: cleanupOnboardingStorage()
alt reboot = true
NextSteps->>Boot: submitInternalBootReboot()
Boot-->>User: reboot initiated
else
NextSteps->>NextSteps: props.onComplete()
NextSteps-->>User: navigate/close
end
end
NextSteps->>NextSteps: isCompleting = false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1948 +/- ##
==========================================
- Coverage 51.74% 51.73% -0.01%
==========================================
Files 1026 1026
Lines 70725 70727 +2
Branches 7894 7892 -2
==========================================
- Hits 36599 36593 -6
- Misses 34003 34011 +8
Partials 123 123 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/Onboarding/steps/OnboardingSummaryStep.vue (1)
1087-1105:⚠️ Potential issue | 🟡 MinorDon't end warning/timeout flows with a success log.
Line 1087 unconditionally appends a green
settingsAppliedentry before the result is classified. Because the diagnostics panel is mainly surfaced on non-success outcomes, warning/timeout runs now end with a success line even when the dialog says the apply only partially succeeded. Emit this log only on the true-success branch, or downgrade it to a neutral log type when any warning flag is set.Suggested adjustment
- addLog(summaryT('logs.settingsApplied'), 'success'); - if (hadInstallTimeout) { applyResultSeverity.value = 'warning'; applyResultTitle.value = summaryT('result.timeoutTitle'); applyResultMessage.value = summaryT('result.timeoutMessage'); } else if (hadNonOptimisticFailures) { applyResultSeverity.value = 'warning'; applyResultTitle.value = summaryT('result.warningsTitle'); applyResultMessage.value = summaryT('result.warningsMessage'); } else if (hadSshVerificationUncertainty) { applyResultSeverity.value = 'warning'; applyResultTitle.value = summaryT('result.bestEffortTitle'); applyResultMessage.value = summaryT('result.sshUnverifiedMessage'); } else if (hadWarnings) { applyResultSeverity.value = 'warning'; applyResultTitle.value = summaryT('result.bestEffortTitle'); applyResultMessage.value = summaryT('result.bestEffortMessage'); } else { + addLog(summaryT('logs.settingsApplied'), 'success'); applyResultSeverity.value = 'success'; applyResultTitle.value = summaryT('result.successTitle'); applyResultMessage.value = summaryT('result.successMessage'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Onboarding/steps/OnboardingSummaryStep.vue` around lines 1087 - 1105, The unconditional call to addLog(summaryT('logs.settingsApplied'), 'success') causes a success log even when later branches set warnings/timeouts; update the logic in OnboardingSummaryStep.vue so addLog is only invoked for the true-success path (i.e., when none of hadInstallTimeout, hadNonOptimisticFailures, hadSshVerificationUncertainty, or hadWarnings are true) or, alternatively, change the log level to a neutral type when any of those warning flags are set; locate the call to addLog and the result classification using applyResultSeverity/applyResultTitle/applyResultMessage and move or gate the addLog call accordingly.
🧹 Nitpick comments (2)
web/src/components/Onboarding/steps/OnboardingNextStepsStep.vue (1)
437-439: Expose the async completion error as an alert.This message appears only after a background action fails. Without a live region/alert role, screen readers may never announce it.
Suggested markup
- <p v-if="completionError" class="mt-6 text-sm text-red-600"> + <p + v-if="completionError" + role="alert" + aria-live="polite" + class="mt-6 text-sm text-red-600 dark:text-red-400" + > {{ completionError }} </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Onboarding/steps/OnboardingNextStepsStep.vue` around lines 437 - 439, The displayed async error (completionError) in OnboardingNextStepsStep.vue is not announced to screen readers; change the current <p v-if="completionError"> to an accessible alert by rendering the message with an appropriate live region/alert role (e.g., role="alert" and aria-live="assertive" or an element with role="status"/aria-live as appropriate) so that when completionError appears it is immediately announced; ensure the conditional still uses completionError and keep styling (class="mt-6 text-sm text-red-600") while adding the accessibility attributes.web/__test__/components/Onboarding/OnboardingNextStepsStep.test.ts (1)
172-184: Assert the failure state, not the exact English sentence.This case is guarding “show an error and do not advance.” Pinning it to the full localized copy will make harmless i18n/content edits fail the suite. Once the template exposes the error semantically, prefer asserting that element instead.
More stable assertion
- expect(wrapper.text()).toContain("We couldn't finish onboarding right now. Please try again."); + expect(wrapper.find('[role="alert"]').exists()).toBe(true);As per coding guidelines, "Test what the code does, not implementation details like exact error message wording".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingNextStepsStep.test.ts` around lines 172 - 184, The test "shows an error and stays on the page when completion fails" currently asserts the exact English error string; change it to assert the error state semantically instead: after triggering the button (using mountComponent and completeOnboardingMock), check for the presence/visibility of the error UI element (e.g., a component or element with a test id like data-testid="onboarding-error" or an error CSS/class exposed by the template) and keep the existing assertions that cleanupOnboardingStorageMock, refetchOnboardingMock, onComplete, and submitInternalBootRebootMock were not called to verify no advancement; remove the hard-coded text assertion so the test no longer depends on exact localized copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/Onboarding/UPGRADE_ONBOARDING.md`:
- Around line 126-127: The doc contradicts itself about where onboarding
completion happens; update the "Apply Sequence" list and the operation mapping
to match the note that final completion occurs from the Next Steps flow (not the
Summary step). Specifically, remove or change any phrasing that says the Summary
step calls CompleteOnboarding or refetches onboarding state, and instead state
that Summary only applies/validates draft settings while Next Steps triggers
CompleteOnboarding and final state refetch; ensure all mentions of "Summary",
"CompleteOnboarding", "Apply Sequence", and the operation mapping are updated so
the file tells a single, consistent completion story.
---
Outside diff comments:
In `@web/src/components/Onboarding/steps/OnboardingSummaryStep.vue`:
- Around line 1087-1105: The unconditional call to
addLog(summaryT('logs.settingsApplied'), 'success') causes a success log even
when later branches set warnings/timeouts; update the logic in
OnboardingSummaryStep.vue so addLog is only invoked for the true-success path
(i.e., when none of hadInstallTimeout, hadNonOptimisticFailures,
hadSshVerificationUncertainty, or hadWarnings are true) or, alternatively,
change the log level to a neutral type when any of those warning flags are set;
locate the call to addLog and the result classification using
applyResultSeverity/applyResultTitle/applyResultMessage and move or gate the
addLog call accordingly.
---
Nitpick comments:
In `@web/__test__/components/Onboarding/OnboardingNextStepsStep.test.ts`:
- Around line 172-184: The test "shows an error and stays on the page when
completion fails" currently asserts the exact English error string; change it to
assert the error state semantically instead: after triggering the button (using
mountComponent and completeOnboardingMock), check for the presence/visibility of
the error UI element (e.g., a component or element with a test id like
data-testid="onboarding-error" or an error CSS/class exposed by the template)
and keep the existing assertions that cleanupOnboardingStorageMock,
refetchOnboardingMock, onComplete, and submitInternalBootRebootMock were not
called to verify no advancement; remove the hard-coded text assertion so the
test no longer depends on exact localized copy.
In `@web/src/components/Onboarding/steps/OnboardingNextStepsStep.vue`:
- Around line 437-439: The displayed async error (completionError) in
OnboardingNextStepsStep.vue is not announced to screen readers; change the
current <p v-if="completionError"> to an accessible alert by rendering the
message with an appropriate live region/alert role (e.g., role="alert" and
aria-live="assertive" or an element with role="status"/aria-live as appropriate)
so that when completionError appears it is immediately announced; ensure the
conditional still uses completionError and keep styling (class="mt-6 text-sm
text-red-600") while adding the accessibility attributes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e70d46c6-f7f8-43c8-bce0-fce0aabc24ed
📒 Files selected for processing (6)
web/__test__/components/Onboarding/OnboardingNextStepsStep.test.tsweb/__test__/components/Onboarding/OnboardingSummaryStep.test.tsweb/src/components/Onboarding/UPGRADE_ONBOARDING.mdweb/src/components/Onboarding/steps/OnboardingNextStepsStep.vueweb/src/components/Onboarding/steps/OnboardingSummaryStep.vueweb/src/locales/en.json
- Purpose: tighten the onboarding follow-up PR around the valid CodeRabbit review comments. - Before: Summary could append a success log even when the dialog was warning/timeout, the async completion error was not announced semantically, one test depended on exact English copy, and the onboarding doc still had stale completion wording. - Problem: these issues made the diagnostics slightly misleading, reduced accessibility, made a regression test more brittle, and left the doc inconsistent with the shipped flow. - Change: gated the Summary success log to the true-success branch, exposed the Next Steps failure as an alert, switched the failure test to assert the semantic error state, and corrected the doc flow description. - How: updated the affected Vue templates/tests/docs and re-ran the related web test commands after each fix.
- Purpose: keep the accessibility follow-up narrowly scoped to semantics. - Before: the async completion error also gained a dark-mode color class while we were fixing the alert semantics. - Problem: that color adjustment was unrelated to the intended behavior change and increased diff noise. - Change: removed the extra dark-mode color class while keeping the alert/live-region behavior intact. - How: reverted the styling-only class addition in OnboardingNextStepsStep.vue.
- Purpose: prevent completed onboarding from recreating a resumable draft on the settings step when the modal is force-opened again. - Before: the final Next Steps screen cleared the draft and then reused the modal's generic next-step handler; once currentStepId was reset to null, that handler treated the flow like it was back on step 1 and persisted CONFIGURE_SETTINGS. - Why: this made Go to Dashboard look complete at first, but force-opening onboarding resumed on step 2 instead of the Overview step. - New: the final step now closes the modal through an explicit completion handler instead of the generic step-advance path. - How: OnboardingModal gives NEXT_STEPS a dedicated close-and-reload callback and adds a regression test that simulates cleanup before completion.
- Purpose: keep the new regression test compatible with the web package type-check. - Before: the NEXT_STEPS test double called the completion prop through component instance state, which worked at runtime but failed vue-tsc. - Why: that left the branch with passing runtime tests but a failing type-check, which blocks a clean verification pass. - New: the stub now calls the completion prop from a typed setup function. - How: the mock component returns a typed click handler from setup, preserving the same regression coverage without relying on untyped instance access.
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![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: e1f6fc7b49
ℹ️ 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".
Comment on lines
+1105
to
1108
| addLog(summaryT('logs.settingsApplied'), 'success'); | ||
| applyResultSeverity.value = 'success'; | ||
| applyResultTitle.value = summaryT('result.successTitle'); | ||
| applyResultMessage.value = summaryT('result.successMessage'); |
There was a problem hiding this comment.
If the page reloads, the tab crashes, or the user navigates away after Confirm & Apply succeeds but before dismissing this dialog, onboarding is still left in the backend INCOMPLETE state and the persisted draft step is still SUMMARY (handleApplyResultConfirm() is the only place that advances, and currentStepId is persisted in onboardingDraft.ts). On the next load the modal will auto-resume at Summary and let the user run the same plugin/internal-boot mutations again, which is especially risky for non-idempotent apply paths like storage-boot setup. Previously this flow was marked complete before the success dialog appeared, so this regression only exists after the new separation of apply vs. completion.
Useful? React with 👍 / 👎.
elibosley
pushed a commit
that referenced
this pull request
Mar 23, 2026
🤖 I have created a release *beep* *boop* --- ## [4.31.1](v4.31.0...v4.31.1) (2026-03-23) ### Bug Fixes * **onboarding:** separate apply and completion flows ([#1948](#1948)) ([5be53a4](5be53a4)) * reload var state when emhttp writes temp files ([#1950](#1950)) ([7265105](7265105)) * **state-watch:** watch canonical ini files atomically ([#1953](#1953)) ([6471b3f](6471b3f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CONFIGURE_SETTINGSvue-tscValidation
completeOnboarding()before this fix15bd7477(feat(onboarding): add new onboarding flows for Unraid OS), so this was not a recent regression from this PR seriesGo to Dashboardcleared the draft before calling the modal's generic next-step handler, which then fell back to Overview and immediately re-saved step 2; Reboot did not share that bug because it never calls the modal advance path after cleanupTesting
pnpm --filter @unraid/web lintpnpm --filter @unraid/web type-checkpnpm --filter @unraid/web testNotes
Summary by CodeRabbit
New Features
Bug Fixes