refactor(onboarding): migrate to Nuxt UI + browser history support#1949
refactor(onboarding): migrate to Nuxt UI + browser history support#1949Ajit-Mehrotra merged 36 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces Headless UI / Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 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)
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 #1949 +/- ##
==========================================
+ Coverage 51.99% 52.02% +0.03%
==========================================
Files 1030 1030
Lines 71171 71349 +178
Branches 7959 8053 +94
==========================================
+ Hits 37002 37121 +119
- Misses 34046 34104 +58
- Partials 123 124 +1 ☔ 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}
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/__test__/components/Onboarding/OnboardingPluginsStep.test.ts (1)
89-126:⚠️ Potential issue | 🔴 CriticalStub template missing
role="switch"attribute anddata-stateattribute required by test queries.The
USwitchstub template renders an<input type="checkbox">withoutroleordata-stateattributes, but the tests at lines 119 and 151 query for[role="switch"]and check.attributes('data-state'). These mismatches will cause thefindAll('[role="switch"]')queries to return an empty array and fail assertions at lines 120, 121-123, 152, and 153-155.🐛 Proposed fix to add missing attributes to stub
USwitch: { props: ['modelValue', 'disabled'], emits: ['update:modelValue'], template: ` <input data-testid="plugin-switch" type="checkbox" + role="switch" + :data-state="modelValue ? 'checked' : 'unchecked'" :checked="modelValue" :disabled="disabled" `@change`="$emit('update:modelValue', $event.target.checked)" /> `, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingPluginsStep.test.ts` around lines 89 - 126, The USwitch stub template is missing the role="switch" and data-state attributes the tests expect; update the USwitch stub (the component object named USwitch in the stubs) so its template includes role="switch" and sets data-state based on the modelValue prop (e.g., 'checked' when modelValue truthy, otherwise 'unchecked'), while still reflecting the disabled prop and emitting update:modelValue on change so the existing test queries for [role="switch"] and assertions on .attributes('data-state') succeed.
🧹 Nitpick comments (2)
web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts (1)
386-391: Timer advancement change looks intentional.The change from
vi.runAllTimersAsync()tovi.advanceTimersByTimeAsync(2500)provides more precise control over timer simulation. Ensure this 2500ms value aligns with any debounce/delay logic in the component (e.g., confirmation dialog transitions).Consider extracting the
2500constant to a named variable (e.g.,const DIALOG_TRANSITION_MS = 2500) to document its purpose and make future adjustments easier.🤖 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 386 - 391, The test now advances timers with vi.advanceTimersByTimeAsync(2500) which should match the component's debounce/transition delay; replace the magic number 2500 with a descriptive constant (e.g., const DIALOG_TRANSITION_MS = 2500) used where vi.advanceTimersByTimeAsync is called and ensure the value matches the component's delay/debounce used in the OnboardingSummaryStep (verify any timeout used in the component code that controls the confirmation dialog/transition). Update references in this test around wrapper and clickButtonByText so the intent of the wait is clear and maintainable.web/__test__/components/Onboarding/OnboardingInternalBootStep.test.ts (1)
20-22: Accessing internal VM methods is a pragmatic workaround for stubbed components.The
InternalBootVmtype andgetDeviceSelectItems()calls test internal computed values rather than rendered output. While this slightly deviates from "test behavior, not implementation details" guidance, it's a reasonable trade-off whenUSelectMenuis stubbed and the actual options rendering is masked.Consider adding a comment explaining why VM access is necessary here, or alternatively, enhance the stub to render option labels in a queryable format.
Also applies to: 294-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingInternalBootStep.test.ts` around lines 20 - 22, The test accesses the component VM via the InternalBootVm type and calls getDeviceSelectItems() to inspect computed option values because USelectMenu is stubbed and doesn't render option labels; add a brief inline comment next to the InternalBootVm type and each getDeviceSelectItems() usage explaining that VM access is intentional (pragmatic workaround for the stubbed USelectMenu) so future readers know this is not accidental coupling to implementation, or alternatively update the USelectMenu test stub to render option labels (so tests can query DOM text) and replace getDeviceSelectItems() calls with DOM queries if you prefer behavior-level testing.
🤖 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/auto-imports.d.ts`:
- Around line 9-37: The auto-imports.d.ts contains hardcoded
../../api/node_modules paths (seen in symbols like avatarGroupInjectionKey,
defineLocale, useLocale, useToast, etc.) which break in different checkout
layouts; update the auto-import generation configuration (Nuxt/Vite/auto-imports
plugin config used to produce web/auto-imports.d.ts) to emit
checkout-independent module paths (use paths relative to web/, e.g.
../api/node_modules or better ../node_modules when deps are consolidated) and
then regenerate the declarations (run the type-check/generation command such as
pnpm --dir web type-check or your auto-imports regenerate task) so the produced
file no longer contains ../../api/node_modules references.
---
Outside diff comments:
In `@web/__test__/components/Onboarding/OnboardingPluginsStep.test.ts`:
- Around line 89-126: The USwitch stub template is missing the role="switch" and
data-state attributes the tests expect; update the USwitch stub (the component
object named USwitch in the stubs) so its template includes role="switch" and
sets data-state based on the modelValue prop (e.g., 'checked' when modelValue
truthy, otherwise 'unchecked'), while still reflecting the disabled prop and
emitting update:modelValue on change so the existing test queries for
[role="switch"] and assertions on .attributes('data-state') succeed.
---
Nitpick comments:
In `@web/__test__/components/Onboarding/OnboardingInternalBootStep.test.ts`:
- Around line 20-22: The test accesses the component VM via the InternalBootVm
type and calls getDeviceSelectItems() to inspect computed option values because
USelectMenu is stubbed and doesn't render option labels; add a brief inline
comment next to the InternalBootVm type and each getDeviceSelectItems() usage
explaining that VM access is intentional (pragmatic workaround for the stubbed
USelectMenu) so future readers know this is not accidental coupling to
implementation, or alternatively update the USelectMenu test stub to render
option labels (so tests can query DOM text) and replace getDeviceSelectItems()
calls with DOM queries if you prefer behavior-level testing.
In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts`:
- Around line 386-391: The test now advances timers with
vi.advanceTimersByTimeAsync(2500) which should match the component's
debounce/transition delay; replace the magic number 2500 with a descriptive
constant (e.g., const DIALOG_TRANSITION_MS = 2500) used where
vi.advanceTimersByTimeAsync is called and ensure the value matches the
component's delay/debounce used in the OnboardingSummaryStep (verify any timeout
used in the component code that controls the confirmation dialog/transition).
Update references in this test around wrapper and clickButtonByText so the
intent of the wait is clear and maintainable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0981f0f9-fcc1-4a4d-ad9a-6ac2eba76860
📒 Files selected for processing (14)
web/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.tsweb/__test__/components/Onboarding/OnboardingInternalBootStep.test.tsweb/__test__/components/Onboarding/OnboardingLicenseStep.test.tsweb/__test__/components/Onboarding/OnboardingNextStepsStep.test.tsweb/__test__/components/Onboarding/OnboardingPluginsStep.test.tsweb/__test__/components/Onboarding/OnboardingSummaryStep.test.tsweb/auto-imports.d.tsweb/components.d.tsweb/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vueweb/src/components/Onboarding/steps/OnboardingInternalBootStep.vueweb/src/components/Onboarding/steps/OnboardingLicenseStep.vueweb/src/components/Onboarding/steps/OnboardingNextStepsStep.vueweb/src/components/Onboarding/steps/OnboardingPluginsStep.vueweb/src/components/Onboarding/steps/OnboardingSummaryStep.vue
Ajit-Mehrotra
force-pushed
the
codex/onboarding-ui-refactor
branch
from
ad9ff32 to
2075ce9
Compare
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/__test__/components/Onboarding/OnboardingInternalBootStep.test.ts (1)
68-71: Minor: Accordion mock passesopenas string.The mock template uses
open="false"(string) rather than:open="false"(boolean). This doesn't affect test behavior since the slots only need to render, but for consistency with the actual component's booleanopenslot-scope:Optional fix for boolean binding
Accordion: { props: ['items', 'type', 'collapsible', 'class'], - template: `<div data-testid="accordion"><template v-for="item in items" :key="item.value"><slot name="trigger" :item="item" :open="false" /><slot name="content" :item="item" :open="false" /></template></div>`, + template: `<div data-testid="accordion"><template v-for="item in items" :key="item.value"><slot name="trigger" :item="item" :open="false" /><slot name="content" :item="item" :open="false" /></template></div>`, },Actually, looking again -
:open="false"in template strings already works correctly since it's a JavaScript expression evaluated as boolean. No change needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingInternalBootStep.test.ts` around lines 68 - 71, The Accordion test mock currently sets the slot prop as open="false" (a string) in the template; update the template in the Accordion mock (the component definition with props ['items','type','collapsible','class'] and its template string) to bind a boolean by changing open="false" to :open="false" for both trigger and content slots so the slot-scope receives a boolean open value.
🤖 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/__test__/components/Onboarding/OnboardingNextStepsStep.test.ts`:
- Around line 109-112: The UAlert test stub currently renders a plain <div>
without the alert role so the test assertion looking for [role="alert"] (in
OnboardingNextStepsStep.test.ts) no longer matches; update the UAlert stub (the
component defined as UAlert with props ['description']) so its root element
includes role="alert" (e.g., render <div role="alert"> or equivalent) and still
exposes the description prop and named slot so the error banner path exercises
the alert markup.
In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts`:
- Around line 265-278: Tests currently synthesize modal text by reaching into
wrapper.vm (the wrapper.text override using SummaryVm and properties like
showBootDriveWarningDialog, showApplyResultDialog, applyResultTitle,
applyResultMessage) which masks missing DOM rendering and calls VM handlers
indirectly; revert this by removing the wrapper.text override and instead assert
modals and their content from the rendered DOM (use wrapper.find/findAll to
locate the dialog elements, text(), and footer buttons) and simulate user
interactions via DOM events (click on the actual footer button elements) so you
test output/behavior not internal refs or methods (do not call VM handlers like
handleApplyResultConfirm or read local refs declared in
OnboardingSummaryStep.vue).
---
Nitpick comments:
In `@web/__test__/components/Onboarding/OnboardingInternalBootStep.test.ts`:
- Around line 68-71: The Accordion test mock currently sets the slot prop as
open="false" (a string) in the template; update the template in the Accordion
mock (the component definition with props ['items','type','collapsible','class']
and its template string) to bind a boolean by changing open="false" to
:open="false" for both trigger and content slots so the slot-scope receives a
boolean open value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c53a7606-7757-4069-94a7-b98e7a377f05
📒 Files selected for processing (15)
unraid-ui/src/components/common/accordion/Accordion.vueweb/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.tsweb/__test__/components/Onboarding/OnboardingInternalBootStep.test.tsweb/__test__/components/Onboarding/OnboardingLicenseStep.test.tsweb/__test__/components/Onboarding/OnboardingNextStepsStep.test.tsweb/__test__/components/Onboarding/OnboardingPluginsStep.test.tsweb/__test__/components/Onboarding/OnboardingSummaryStep.test.tsweb/auto-imports.d.tsweb/components.d.tsweb/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vueweb/src/components/Onboarding/steps/OnboardingInternalBootStep.vueweb/src/components/Onboarding/steps/OnboardingLicenseStep.vueweb/src/components/Onboarding/steps/OnboardingNextStepsStep.vueweb/src/components/Onboarding/steps/OnboardingPluginsStep.vueweb/src/components/Onboarding/steps/OnboardingSummaryStep.vue
✅ Files skipped from review due to trivial changes (3)
- web/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vue
- web/auto-imports.d.ts
- web/src/components/Onboarding/steps/OnboardingSummaryStep.vue
🚧 Files skipped from review as they are similar to previous changes (6)
- web/test/components/Onboarding/OnboardingPluginsStep.test.ts
- web/test/components/Onboarding/OnboardingLicenseStep.test.ts
- web/src/components/Onboarding/steps/OnboardingLicenseStep.vue
- web/src/components/Onboarding/steps/OnboardingPluginsStep.vue
- web/src/components/Onboarding/steps/OnboardingNextStepsStep.vue
- web/components.d.ts
| UAlert: { | ||
| props: ['description'], | ||
| template: '<div>{{ description }}<slot name="description" /></div>', | ||
| }, |
There was a problem hiding this comment.
Preserve the alert role in this UAlert stub.
Line 199 still asserts [role="alert"], but this stub now renders a plain <div>. That makes the completion-failure path stop exercising the alert markup and will likely fail once the error banner is shown.
🔧 Minimal fix
UAlert: {
props: ['description'],
- template: '<div>{{ description }}<slot name="description" /></div>',
+ template: '<div role="alert">{{ description }}<slot name="description" /></div>',
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| UAlert: { | |
| props: ['description'], | |
| template: '<div>{{ description }}<slot name="description" /></div>', | |
| }, | |
| UAlert: { | |
| props: ['description'], | |
| template: '<div role="alert">{{ description }}<slot name="description" /></div>', | |
| }, |
🤖 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 109 - 112, The UAlert test stub currently renders a plain <div> without
the alert role so the test assertion looking for [role="alert"] (in
OnboardingNextStepsStep.test.ts) no longer matches; update the UAlert stub (the
component defined as UAlert with props ['description']) so its root element
includes role="alert" (e.g., render <div role="alert"> or equivalent) and still
exposes the description prop and named slot so the error banner path exercises
the alert markup.
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
7 similar comments
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/src/components/Onboarding/steps/OnboardingSummaryStep.vue (1)
1351-1385: Modal state may persist across open/close cycles with:openvsv-if.The previous
Dialogimplementation usedv-ifwhich destroyed the component on close, guaranteeing fresh state on each open. WithUModalusing:openbinding, the modal content remains mounted but hidden. For this specific modal, this should be fine since there's no internal form state to reset, but be aware of this behavioral difference.🤖 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 1351 - 1385, The UModal is bound with :open="showBootDriveWarningDialog" which keeps the modal mounted when closed (unlike the prior v-if behavior) causing potential persisted internal state; either revert to conditional mounting by replacing :open with v-if="showBootDriveWarningDialog" on the UModal to destroy and recreate the component, or explicitly reset any internal state when closing by calling reset logic from handleBootDriveWarningCancel/handleBootDriveWarningConfirm (and ensure selectedBootDevices is refreshed before opening) so the modal has fresh state each time.
🤖 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/OnboardingSummaryStep.vue`:
- Around line 1351-1385: The UModal is bound with
:open="showBootDriveWarningDialog" which keeps the modal mounted when closed
(unlike the prior v-if behavior) causing potential persisted internal state;
either revert to conditional mounting by replacing :open with
v-if="showBootDriveWarningDialog" on the UModal to destroy and recreate the
component, or explicitly reset any internal state when closing by calling reset
logic from handleBootDriveWarningCancel/handleBootDriveWarningConfirm (and
ensure selectedBootDevices is refreshed before opening) so the modal has fresh
state each time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2f091bc6-773a-4626-a266-b629345a87fa
📒 Files selected for processing (6)
unraid-ui/src/components/brand/brand-button.variants.tsweb/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vueweb/src/components/Onboarding/steps/OnboardingInternalBootStep.vueweb/src/components/Onboarding/steps/OnboardingOverviewStep.vueweb/src/components/Onboarding/steps/OnboardingPluginsStep.vueweb/src/components/Onboarding/steps/OnboardingSummaryStep.vue
✅ Files skipped from review due to trivial changes (1)
- web/src/components/Onboarding/steps/OnboardingOverviewStep.vue
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
4 similar comments
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/steps/OnboardingSummaryStep.vue`:
- Around line 1380-1387: The UModal instance bound to showApplyResultDialog
allows overlay/ESC dismissal which can bypass the completion flow; update the
UModal usage (the component with props :open="showApplyResultDialog" and events
`@update`:open) to include :dismissible="false" so the modal cannot be closed via
overlay or ESC, ensuring closure only goes through handleApplyResultConfirm
which triggers props.onComplete().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 592a4d16-d963-4040-9cf3-dd08d154f6fc
📒 Files selected for processing (3)
unraid-ui/src/components/ui/accordion/AccordionTrigger.vueweb/src/components/Onboarding/steps/OnboardingInternalBootStep.vueweb/src/components/Onboarding/steps/OnboardingSummaryStep.vue
✅ Files skipped from review due to trivial changes (1)
- unraid-ui/src/components/ui/accordion/AccordionTrigger.vue
| <UModal | ||
| :open="showApplyResultDialog" | ||
| :portal="false" | ||
| :title="applyResultTitle" | ||
| :description="applyResultMessage" | ||
| :ui="{ footer: 'justify-end', overlay: 'z-50', content: 'z-50 w-[calc(100vw-2rem)] max-w-3xl' }" | ||
| @update:open="showApplyResultDialog = $event" | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Nuxt UI's UModal (the version used in this repo), does the modal close by default on overlay click or Escape key when bound with :open and @update:open? What is the recommended prop/config to make it non-dismissible?
💡 Result:
In Nuxt UI's UModal (current version as of 2026), when bound with :open and @update:open (v-model:open), the modal closes by default on overlay click or Escape key, as the dismissible prop defaults to true. The recommended prop to make it non-dismissible is :dismissible="false". When set to false, attempts to close via overlay or Escape emit a close:prevent event (handle with @close:prevent if needed).
Citations:
- 1: https://ui.nuxt.com/docs/components/modal
- 2: UDrawer ignores :dismissible="false" prop and still closes when interacting outside nuxt/ui#3727
- 3: http://ui4.nuxt.com/docs/components/popover
- 4: close prevented event on Modal nuxt/ui#3126
🏁 Script executed:
# Find the OnboardingSummaryStep.vue file
fd -t f "OnboardingSummaryStep.vue"Repository: unraid/api
Length of output: 115
🏁 Script executed:
# Check the file size first
wc -l web/src/components/Onboarding/steps/OnboardingSummaryStep.vueRepository: unraid/api
Length of output: 120
🏁 Script executed:
# Read the modal code at lines 1380-1387
sed -n '1375,1395p' web/src/components/Onboarding/steps/OnboardingSummaryStep.vueRepository: unraid/api
Length of output: 889
🏁 Script executed:
# Search for handleApplyResultConfirm method
sed -n '1040,1050p' web/src/components/Onboarding/steps/OnboardingSummaryStep.vueRepository: unraid/api
Length of output: 246
🏁 Script executed:
# Look for the modal footer content with OK button
sed -n '1380,1420p' web/src/components/Onboarding/steps/OnboardingSummaryStep.vueRepository: unraid/api
Length of output: 1778
🏁 Script executed:
# Check if dismissible prop is set anywhere for this modal
sed -n '1375,1410p' web/src/components/Onboarding/steps/OnboardingSummaryStep.vue | grep -i dismissibleRepository: unraid/api
Length of output: 36
🏁 Script executed:
# Check all UModal instances in this file to see if any use dismissible prop
rg -n "UModal" web/src/components/Onboarding/steps/OnboardingSummaryStep.vue -A 8Repository: unraid/api
Length of output: 1163
🏁 Script executed:
# Check if dismissible prop is used elsewhere in this component or similar components
rg -n "dismissible" web/src/components/Onboarding/ -B 2 -A 2Repository: unraid/api
Length of output: 36
🏁 Script executed:
# Check the boot drive warning dialog handler to see if it follows the same pattern
rg -n "showBootDriveWarningDialog" web/src/components/Onboarding/steps/OnboardingSummaryStep.vueRepository: unraid/api
Length of output: 363
🏁 Script executed:
# Check what props.onComplete is and how critical it is
rg -n "onComplete" web/src/components/Onboarding/steps/OnboardingSummaryStep.vue -B 2 -A 2Repository: unraid/api
Length of output: 299
🏁 Script executed:
# Find where OnboardingSummaryStep is used to understand the context of onComplete
rg -n "OnboardingSummaryStep" web/src/components/ --type vue -B 2 -A 2Repository: unraid/api
Length of output: 81
🏁 Script executed:
# Find where OnboardingSummaryStep is used
rg -n "OnboardingSummaryStep" web/src/components/ -B 2 -A 2Repository: unraid/api
Length of output: 4624
🏁 Script executed:
# Check if there are any event handlers or side effects triggered by showApplyResultDialog changes
rg -n "showApplyResultDialog" web/src/components/Onboarding/steps/OnboardingSummaryStep.vue -B 1 -A 1Repository: unraid/api
Length of output: 878
🏁 Script executed:
# Check if there's a watcher on showApplyResultDialog that might prevent early closing
rg -n "watch.*showApplyResultDialog" web/src/components/Onboarding/steps/OnboardingSummaryStep.vue -B 2 -A 5Repository: unraid/api
Length of output: 36
Prevent accidental modal dismissal from bypassing completion flow.
Line 1386 directly mirrors @update:open into state. Without :dismissible="false", users can close this modal via overlay click or ESC, which skips handleApplyResultConfirm (Line 1043) and therefore skips props.onComplete() (Line 1045).
Add :dismissible="false" to disable overlay/ESC dismissal:
Proposed fix
<UModal
:open="showApplyResultDialog"
:portal="false"
+ :dismissible="false"
:title="applyResultTitle"
:description="applyResultMessage"
:ui="{ footer: 'justify-end', overlay: 'z-50', content: 'z-50 w-[calc(100vw-2rem)] max-w-3xl' }"
`@update`:open="showApplyResultDialog = $event"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <UModal | |
| :open="showApplyResultDialog" | |
| :portal="false" | |
| :title="applyResultTitle" | |
| :description="applyResultMessage" | |
| :ui="{ footer: 'justify-end', overlay: 'z-50', content: 'z-50 w-[calc(100vw-2rem)] max-w-3xl' }" | |
| @update:open="showApplyResultDialog = $event" | |
| > | |
| <UModal | |
| :open="showApplyResultDialog" | |
| :portal="false" | |
| :dismissible="false" | |
| :title="applyResultTitle" | |
| :description="applyResultMessage" | |
| :ui="{ footer: 'justify-end', overlay: 'z-50', content: 'z-50 w-[calc(100vw-2rem)] max-w-3xl' }" | |
| `@update`:open="showApplyResultDialog = $event" | |
| > |
🤖 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
1380 - 1387, The UModal instance bound to showApplyResultDialog allows
overlay/ESC dismissal which can bypass the completion flow; update the UModal
usage (the component with props :open="showApplyResultDialog" and events
`@update`:open) to include :dismissible="false" so the modal cannot be closed via
overlay or ESC, ensuring closure only goes through handleApplyResultConfirm
which triggers props.onComplete().
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
6 similar comments
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: replace remaining mixed onboarding form primitives and modal actions with Nuxt UI components so the flow feels visually and behaviorally consistent end to end. - Before: onboarding still mixed native inputs, bespoke Headless UI switches, custom callouts, and older dialog/button patterns across Core Settings, Plugins, Internal Boot, License, Summary, and Next Steps. - Problem: the flow looked uneven, dark-mode surfaces were inconsistent, and the test suite still assumed pre-refactor modal behavior. - Now: onboarding uses Nuxt UI switches, inputs, select menus, checkboxes, alerts, modals, and dialog buttons throughout the targeted steps while keeping the intentional native footer CTA buttons unchanged. - How: migrated the step components, regenerated Nuxt auto-import typings, and updated the onboarding Vitest specs to assert the new modal/result flows and storage-boot confirmation behavior.
- Purpose: enable consumers to react to accordion open/close state
- Before: the Accordion wrapper only passed `item` to #trigger and
#content slots, with no way to know if a given item was expanded
- Problem: onboarding Disclosure components rely on `v-slot="{ open }"`
to toggle label text ("Show Details" / "Hide Details") and rotate
chevron icons — the Accordion wrapper could not replace them
- Change: track open items via v-model/modelValue on AccordionRoot,
compute per-item `open` boolean, and pass it to both #trigger and
#content scoped slots as `{ item, open }`
- How it works:
- internal `openValue` ref mirrors modelValue or defaultValue
- `isItemOpen(value)` checks whether a value is in the active set
- `handleUpdate` syncs internal state and emits update:modelValue
- slots receive `open` alongside `item` for conditional rendering
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: soften the initialization ready alert on the summary page - Before: UAlert used variant="solid" producing a bold green block - Problem: solid green was visually heavy against the page background - Change: switch to variant="subtle" for a lighter, more subdued look
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: fix the Setup Applied dialog showing a large empty body - Before: #body slot always rendered (even when empty), and the dialog used max-w-3xl regardless of whether logs were present - Problem: empty #body slot produced a tall blank white area in the success case; dialog was also unnecessarily wide - Change: conditionally render #body slot only when diagnostic logs exist; use max-w-md for the compact success case and max-w-3xl only when the wider log console is shown
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: fix misleading log ordering in the summary step - Before: "Finalizing setup..." appeared between SSH and identity update, making it look like a separate intermediate step - Problem: confusing to users — "Finalizing" should come after all substantive mutations are complete - Change: move the "Finalizing setup" log to after the server identity update so the sequence is SSH → Identity → Finalizing
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: fix two edge cases found during adversarial review - Reboot modal: restore :disabled="isCompleting" on Cancel and Confirm buttons so double-clicking can't fire duplicate reboot requests while the mutation is in-flight - SlotCount: guard Number() conversion with Number.isFinite() and minimum check so if USelectMenu ever emits undefined, slotCount stays at its current value instead of becoming NaN (which would break the device selector loop)
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: merge feat/onboarding-use-history into our Nuxt UI branch - Brings in browser back/forward navigation support for onboarding - Adds "No Updates Needed" messaging when no config changes apply - Resolves 1 merge conflict in OnboardingSummaryStep.test.ts: kept our VM-based assertion approach (showApplyResultDialog) and adopted PR 1964's updated "No Updates Needed" text - Fixed additional test assertion for the wider dialog test case that also expected the old "Setup Applied" title - All 618 web tests pass after merge
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
Ajit-Mehrotra
changed the title
- Purpose: fix TypeScript errors introduced by merging PR 1964
- sessionId extraction: cast to string after typeof guard so TS
narrows from string | {} to string
- historySessionId.value: add ?? '' fallback since ref is string | null
but callers guard non-null before calling buildHistoryState
- handleClose onClick: wrap in arrow function to prevent Vue from
passing MouseEvent to the options parameter
- Remove dead code: hasCoreSettingChanges and hasAnyChangesToApply
computeds replaced by inline hasUpdatesToApply in handleComplete
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: fix the same accordion styling issues as the plugins accordion on the storage boot eligibility details panel - Before: default AccordionTrigger added a bottom border, hover underline, and a second chevron icon alongside the custom one - Change: add item-class="border-none" to remove the bottom border, trigger-class with hover:no-underline and [&>svg]:hidden to remove the hover underline and hide the duplicate default chevron
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: align eligibility accordion with plugins accordion style - Before: custom ChevronDownIcon in the trigger slot plus the default AccordionTrigger chevron resulted in two chevrons; the default was hidden via [&>svg]:hidden - Change: remove the custom ChevronDownIcon entirely and use the default AccordionTrigger chevron colored with [&>svg]:text-primary, matching the plugins accordion implementation - Also removes unused ChevronDownIcon import
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: fix chevron color and alignment on eligibility panel - Before: chevron was text-primary (orange) and misaligned below the "View details" text due to items-start on the trigger div - Change: remove text-primary override so chevron uses default color, switch trigger div from items-start to items-center so the chevron aligns inline with the "View details" text
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
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: fddfd10192
ℹ️ 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".
| if ( | ||
| availableSteps.value.includes(nextHistoryState.stepId) && | ||
| currentStepId.value !== nextHistoryState.stepId |
There was a problem hiding this comment.
When a step disappears mid-session (for example ACTIVATE_LICENSE after registration state changes), browser back can land on a same-session history entry whose stepId is no longer in availableSteps. This branch returns without changing currentStepId, so the UI appears stuck on the current step and users need extra back presses to move. In practice this breaks the new browser-history navigation whenever step availability changes during onboarding.
Useful? React with 👍 / 👎.
| historyPosition.value += 1; | ||
| window.history.pushState(buildHistoryState(stepId, historyPosition.value), '', window.location.href); |
There was a problem hiding this comment.
Step changes always call pushState, including automatic onboarding sessions. But automatic close paths do not rewind history (rewind only happens for manual sessions), so closing onboarding leaves one inert history entry per step. After onboarding closes, pressing browser back walks through these stale entries with no visible state change, which is a regression in normal page navigation behavior.
Useful? React with 👍 / 👎.
2 participants
Summary
What Changed
HeadlessUI → Nuxt UI Migration
USelectMenu,UInput,UCheckbox,UAlert,UModal,UButton,URadioGroup) instead of HeadlessUI or native HTML elements@headlessui/vueimports removed from onboarding<transition>wrappers removed — Accordion uses Reka UI's built-in animationsAccordion Enhancement (@unraid/ui)
Accordioncomponent with optionalv-modelsupport,itemClassprop, and{ open }boolean in scoped slotsUX Polish
aria-disabledwithpointer-events-nonecursor-not-allowedon disabled Back/Skip navigation buttonsslotCountNaN from undefined USelectMenu values:disabledon reboot modal buttons to prevent double-clickBrowser History + No-Changes Messaging (from #1964)
Test Updates
global.stubs)Why
Includes
Verification
Smoke Test