fix: consistently clear onboarding draft#1916
Open
Ajit-Mehrotra wants to merge 5 commits intomainfrom
Open
Conversation
- Purpose: decouple onboarding draft cleanup from summary apply success so draft lifecycle follows explicit user exit paths. - Before: the summary step cleared onboardingDraft only after completeOnboarding succeeded, while overview skip and modal exit used different cleanup timing. - Problem: local draft persistence depended on API completion behavior instead of whether the user actually stayed in or left onboarding. - Change: remove draft cleanup from the summary apply path and clear immediately when overview skip triggers an explicit exit. - How it works: summary now preserves onboardingDraft while the flow continues to the result dialog and next step, and overview skip clears storage before attempting completion so failures do not keep stale draft state around. - Verification: added focused tests covering successful summary apply, failed summary completion, and explicit skip cleanup behavior.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughMoves onboarding storage cleanup earlier for skip and reboot flows and removes the post-completion cleanup from the successful apply path. Tests were added/updated and module mocks hoisted to expose Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Onboarding UI
participant Storage as onboardingStorageCleanup
participant API as Server/API
User->>UI: Click "Skip setup" or "Confirm reboot"
UI->>Storage: cleanupOnboardingStorage({ clearTemporaryBypassSessionState: true })
Storage-->>UI: ACK
UI->>API: completeOnboarding / submitReboot / refetchOnboarding
API-->>UI: success or failure
alt completion failure
UI->>Storage: (cleanup already executed earlier)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 unit tests (beta)
📝 Coding Plan
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1916 +/- ##
==========================================
+ Coverage 50.96% 50.98% +0.02%
==========================================
Files 1023 1024 +1
Lines 70556 70554 -2
Branches 7687 7709 +22
==========================================
+ Hits 35956 35971 +15
+ Misses 34476 34457 -19
- Partials 124 126 +2 ☔ 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.
🧹 Nitpick comments (1)
web/__test__/components/Onboarding/OnboardingOverviewStep.test.ts (1)
146-165: Consider using a more robust selector for the skip button.Using
wrapper.findAll('button')[1]is fragile and may break if button order changes. Consider finding the button by its text content or a semantic attribute.♻️ Suggested improvement
it('clears onboarding draft immediately when skipping setup', async () => { const wrapper = mountComponent(); - await wrapper.findAll('button')[1]?.trigger('click'); + const skipButton = wrapper.findAll('button').find((btn) => btn.text().includes('Skip')); + await skipButton?.trigger('click'); expect(cleanupOnboardingStorageMock).toHaveBeenCalledWith({ clearTemporaryBypassSessionState: true, }); }); it('still clears onboarding draft when skip completion fails', async () => { completeOnboardingMock.mockRejectedValueOnce(new Error('offline')); const wrapper = mountComponent(); - await wrapper.findAll('button')[1]?.trigger('click'); + const skipButton = wrapper.findAll('button').find((btn) => btn.text().includes('Skip')); + await skipButton?.trigger('click'); expect(cleanupOnboardingStorageMock).toHaveBeenCalledWith({ clearTemporaryBypassSessionState: true, }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingOverviewStep.test.ts` around lines 146 - 165, The tests use a fragile index selector wrapper.findAll('button')[1] to click the skip button; update both tests ('clears onboarding draft immediately when skipping setup' and 'still clears onboarding draft when skip completion fails') to select the skip button more robustly (e.g., find by visible text or a semantic selector) — replace the index-based find with either wrapper.findAll('button').find(b => b.text().includes('Skip')) or, preferably, add/use a stable attribute like data-testid="skip-setup-button" on the skip button in the component and call wrapper.find('[data-testid="skip-setup-button"]').trigger('click') so the tests reliably target the intended button (make sure to update both test cases and, if adding an attribute, the component template).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/__test__/components/Onboarding/OnboardingOverviewStep.test.ts`:
- Around line 146-165: The tests use a fragile index selector
wrapper.findAll('button')[1] to click the skip button; update both tests
('clears onboarding draft immediately when skipping setup' and 'still clears
onboarding draft when skip completion fails') to select the skip button more
robustly (e.g., find by visible text or a semantic selector) — replace the
index-based find with either wrapper.findAll('button').find(b =>
b.text().includes('Skip')) or, preferably, add/use a stable attribute like
data-testid="skip-setup-button" on the skip button in the component and call
wrapper.find('[data-testid="skip-setup-button"]').trigger('click') so the tests
reliably target the intended button (make sure to update both test cases and, if
adding an attribute, the component template).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 641c7f8f-d403-44ef-9798-19871e0c143a
📒 Files selected for processing (4)
web/__test__/components/Onboarding/OnboardingOverviewStep.test.tsweb/__test__/components/Onboarding/OnboardingSummaryStep.test.tsweb/src/components/Onboarding/steps/OnboardingOverviewStep.vueweb/src/components/Onboarding/steps/OnboardingSummaryStep.vue
💤 Files with no reviewable changes (1)
- web/src/components/Onboarding/steps/OnboardingSummaryStep.vue
Ajit-Mehrotra
changed the title
- Purpose: make the internal-boot reboot action follow the same explicit exit path as other onboarding exits. - Before: the Next Steps reboot confirmation submitted a reboot form directly from the step component and bypassed the modal's shared exit cleanup flow. - Problem: onboardingDraft could survive a reboot-triggered exit even though the user intentionally left onboarding. - Change: add a reboot-aware exit callback in the modal and have Next Steps request reboot through that shared exit path. - How it works: the modal now supports an after-close action, cleans onboarding storage through closeModal, and then triggers the reboot submission. - Verification: added Next Steps coverage and reran the focused onboarding Vitest suite, including modal, summary, overview, cleanup, visibility, and Next Steps tests.
- Purpose: replace the reboot-specific modal exit callback with direct draft cleanup in the Next Steps reboot path. - Before: reboot was routed through extra modal callback plumbing so it shared the closeModal exit machinery. - Problem: that approach worked, but it added indirection for a simple requirement and made the flow harder to follow. - Change: remove the reboot-specific modal abstraction and clear onboarding storage directly when the reboot confirmation is accepted. - How it works: the Next Steps reboot handler now calls onboarding storage cleanup and then submits the reboot form, while the modal returns to its simpler generic close behavior. - Verification: reran the focused onboarding Vitest suite covering Next Steps, modal, summary, overview, storage cleanup, and modal visibility.
- Purpose: simplify currentStepProps in OnboardingModal now that Summary and Next Steps use the default base props. - Before: the switch kept separate SUMMARY and NEXT_STEPS branches that each returned the same baseProps object. - Problem: this duplication added noise without changing behavior. - Change: remove the redundant branches and let both steps fall through to the default return. - How it works: the modal still passes the same common props to both steps, just without explicit duplicate cases. - Verification: reran the targeted modal, next steps, and summary Vitest suite.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/src/components/Onboarding/steps/OnboardingNextStepsStep.vue (1)
96-100: Consider removing unnecessaryasynckeyword.The function is marked
asyncbut nothing is awaited inside it. BothcleanupOnboardingStorageandsubmitInternalBootRebootare synchronous calls. While this doesn't cause issues, removingasyncwould be slightly cleaner.♻️ Optional: Remove async keyword
-const handleConfirmReboot = async () => { +const handleConfirmReboot = () => { showRebootWarningDialog.value = false; cleanupOnboardingStorage({ clearTemporaryBypassSessionState: true }); submitInternalBootReboot(); };🤖 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 96 - 100, The function handleConfirmReboot is declared async but contains no awaits; remove the unnecessary async keyword from its declaration to keep the code clean and avoid misleading callers; locate the handleConfirmReboot function (it toggles showRebootWarningDialog.value and calls cleanupOnboardingStorage and submitInternalBootReboot) and change its declaration to a plain synchronous function, ensuring no await usage remains or is added unintentionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/src/components/Onboarding/steps/OnboardingNextStepsStep.vue`:
- Around line 96-100: The function handleConfirmReboot is declared async but
contains no awaits; remove the unnecessary async keyword from its declaration to
keep the code clean and avoid misleading callers; locate the handleConfirmReboot
function (it toggles showRebootWarningDialog.value and calls
cleanupOnboardingStorage and submitInternalBootReboot) and change its
declaration to a plain synchronous function, ensuring no await usage remains or
is added unintentionally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b5940370-7599-455f-998c-769c2da09b5a
📒 Files selected for processing (3)
web/__test__/components/Onboarding/OnboardingNextStepsStep.test.tsweb/src/components/Onboarding/OnboardingModal.vueweb/src/components/Onboarding/steps/OnboardingNextStepsStep.vue
💤 Files with no reviewable changes (1)
- web/src/components/Onboarding/OnboardingModal.vue
- Purpose: resolve the remaining CodeRabbit nitpicks on the onboarding draft cleanup PR. - Before: overview tests used an index-based button selector and the reboot confirm handler was marked async without awaiting anything. - Problem: the test selector was fragile to markup order changes, and the async modifier implied behavior that the handler did not have. - Change: add a stable test id for the overview skip button, update the tests to use it, and remove the unnecessary async keyword from the reboot confirm handler. - How it works: the overview tests now target the intended button directly and the reboot handler remains a plain synchronous function with unchanged behavior. - Verification: reran targeted Vitest coverage for the overview and next-steps onboarding files while working each nit one at a time.
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
pujitm
approved these changes
Mar 16, 2026
![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: 95d12b2d10
ℹ️ 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".
| it('clears onboarding draft immediately when skipping setup', async () => { | ||
| const wrapper = mountComponent(); | ||
|
|
||
| await wrapper.find('[data-testid="skip-setup-button"]').trigger('click'); |
There was a problem hiding this comment.
/workspace/api/AGENTS.md (Vue Component Testing) says to use semantic queries like find('button') instead of data-test IDs, but this change introduces find('[data-testid="skip-setup-button"]') and requires a matching data-testid in production markup. Keeping this pattern will propagate test-only attributes and brittle selector conventions, so the test should target the skip action semantically and avoid the test-id dependency.
Useful? React with 👍 / 👎.
elibosley
approved these changes
Mar 16, 2026
Member
elibosley
left a comment
elibosley
left a comment
There was a problem hiding this comment.
There's still too much frontend lifecycle state around the onboarding for me to be fully comfortable, but this is a good fix.
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
3 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.
Problem
We found that
onboardingDraftcleanup was not consistently tied to the user's actual onboarding lifecycle.The original undesirable case was in the summary/apply flow:
completeOnboarding()could fail because the API was unavailable or the tracker write failedonboardingDraftwas treated differently from other explicit exit pathsWe also found a second explicit-exit gap after that:
onboardingDraftcould survive even though the user had intentionally left onboarding by rebootingThat meant draft lifecycle still depended on which exit branch the user took, instead of a simple UX rule based on whether they were still in onboarding or had intentionally left it.
Desired Behavior
This PR aligns the flow to the rule we agreed on:
enter / resume / in-progress=> keeponboardingDraftexplicit exit for any non-passive reason=> clearonboardingDraftrefresh / crash / passive interruption=> keeponboardingDraftIn practice, local draft cleanup should have nothing to do with whether applying settings or marking onboarding complete succeeds, and every explicit exit path should clear draft state.
What Changed
onboardingDraftcleanup from the summary apply pathcompleteOnboarding(), and showing result state while the user remains in the flowResulting Flow
After this change:
completeOnboarding()fails afterwardWhy This Is Better
This keeps the behavior aligned with the agreed spec while staying simple:
Verification
Ran the targeted onboarding test suite:
pnpm exec vitest run __test__/components/Onboarding/OnboardingNextStepsStep.test.ts __test__/components/Onboarding/OnboardingSummaryStep.test.ts __test__/components/Onboarding/OnboardingOverviewStep.test.ts __test__/components/Onboarding/OnboardingModal.test.ts __test__/components/Onboarding/onboardingStorageCleanup.test.ts __test__/store/onboardingModalVisibility.test.tsSummary by CodeRabbit
Tests
Bug Fixes