fix(onboarding): prevent Apply Result dialog from being dismissed#1971
Merged
fix(onboarding): prevent Apply Result dialog from being dismissed#1971
Conversation
- The Apply Result dialog (success/warning/error/timeout) that appears after "Confirm and Apply" could previously be closed by pressing ESC, clicking outside the overlay, or clicking the header close (X) button - This allowed users to bypass the result acknowledgment and miss important error/warning information or skip navigation to Next Steps - Add `:dismissible="false"` to block ESC, click-outside, and focus-outside close events (Nuxt UI built-in via reka-ui) - Add `:close="false"` to remove the header close (X) button, which would otherwise bypass the dismissible guard via reka-ui's DialogClose - Remove the now-dead `@update:open` handler since no external close events can fire - The only way to close the dialog is now the OK button, which calls `handleApplyResultConfirm()` and advances to the Next Steps page - Add test verifying OK is the sole close path for the result dialog
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1971 +/- ##
=======================================
Coverage 52.09% 52.09%
=======================================
Files 1031 1031
Lines 71573 71574 +1
Branches 8093 8092 -1
=======================================
+ Hits 37284 37285 +1
Misses 34164 34164
Partials 125 125 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Contributor
WalkthroughChanges prevent user-initiated dismissal of the apply-result dialog by setting Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 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 |
![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/OnboardingSummaryStep.test.ts (1)
587-599: Test verifies the expected behavior; consider adding an assertion for the modal props.The test correctly verifies that
handleApplyResultConfirm()closes the dialog and invokesonComplete. To more explicitly verify the modal is configured as non-dismissible, consider asserting on the rendered attributes:♻️ Optional enhancement to verify modal configuration
it('only allows closing the apply result dialog via the OK button', async () => { const { wrapper, onComplete } = mountComponent(); await clickApply(wrapper); const vm = getSummaryVm(wrapper); expect(vm.showApplyResultDialog).toBe(true); expect(onComplete).not.toHaveBeenCalled(); + // Verify modal is configured as non-dismissible + const dialog = wrapper.find('[data-testid="dialog"]'); + expect(dialog.attributes('data-dismissible')).toBe('false'); + expect(dialog.attributes('data-close')).toBe('false'); + vm.handleApplyResultConfirm(); expect(vm.showApplyResultDialog).toBe(false); expect(onComplete).toHaveBeenCalledOnce(); });As per coding guidelines: "Check for expected prop handling and event emissions in Vue components."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts` around lines 587 - 599, Add an assertion that the rendered apply-result modal is configured as non-dismissible: after clickApply(wrapper) and before calling vm.handleApplyResultConfirm(), locate the rendered modal via the test wrapper (e.g., wrapper.findComponent(...) or wrapper.find(selector)), and assert its props such as persistent / noCloseOnBackdrop / closeOnEsc (or the equivalent props used by the modal in OnboardingSummaryStep) are set to prevent dismissal; keep existing checks of vm.showApplyResultDialog and onComplete. Use the existing helpers (mountComponent, getSummaryVm, clickApply) to find the modal and assert the modal props before confirming.
🤖 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/OnboardingSummaryStep.test.ts`:
- Around line 587-599: Add an assertion that the rendered apply-result modal is
configured as non-dismissible: after clickApply(wrapper) and before calling
vm.handleApplyResultConfirm(), locate the rendered modal via the test wrapper
(e.g., wrapper.findComponent(...) or wrapper.find(selector)), and assert its
props such as persistent / noCloseOnBackdrop / closeOnEsc (or the equivalent
props used by the modal in OnboardingSummaryStep) are set to prevent dismissal;
keep existing checks of vm.showApplyResultDialog and onComplete. Use the
existing helpers (mountComponent, getSummaryVm, clickApply) to find the modal
and assert the modal props before confirming.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7341f0a0-cd1f-4b7a-8927-15171a8e3f81
📒 Files selected for processing (2)
web/__test__/components/Onboarding/OnboardingSummaryStep.test.tsweb/src/components/Onboarding/steps/OnboardingSummaryStep.vue
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
What changed
OnboardingSummaryStep.vue— Apply ResultUModal(line 1387)::dismissible="false"— uses Nuxt UI's built-in mechanism to calle.preventDefault()on reka-ui'spointerDownOutside,interactOutside, andescapeKeyDownevents:close="false"— removes the header close (X) button, which would otherwise bypass thedismissibleguard via reka-ui'sDialogClosecomponent@update:openhandler — dead code since no external close events can fire with both props setOnboardingSummaryStep.test.ts:dismissibleandcloseprops and render them as data attributeshandleApplyResultConfirm()(triggered by OK button) is the sole path to close the dialog and advance to Next StepsModal state audit
Scope decisions
Only the Apply Result Dialog was made non-dismissible. The Boot Drive Warning and InternalBootConfirmDialog retain their current dismissible behavior — these are pre-action confirmations where canceling/escaping is a valid user choice. The Apply Result Dialog is post-action; the settings have already been applied, and the user must acknowledge the outcome.
Test plan
Summary by CodeRabbit
Bug Fixes
Tests