Refactor onboarding flow to use Nuxt UI primitives#1949
Draft
Ajit-Mehrotra wants to merge 14 commits intomainfrom
Draft
Refactor onboarding flow to use Nuxt UI primitives#1949Ajit-Mehrotra wants to merge 14 commits intomainfrom
Ajit-Mehrotra wants to merge 14 commits intomainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
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.94% 51.87% -0.08%
==========================================
Files 1030 1030
Lines 71121 71037 -84
Branches 7933 7951 +18
==========================================
- Hits 36947 36853 -94
- Misses 34051 34061 +10
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/__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
- 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
- Purpose: eliminate @headlessui/vue dependency from onboarding flow
by migrating the last two Disclosure usages to @unraid/ui Accordion
- Before: InternalBootStep and SummaryStep imported Disclosure,
DisclosureButton, and DisclosurePanel from @headlessui/vue, with
custom <transition> wrappers for expand/collapse animation
- Problem: onboarding was the only consumer of HeadlessUI within the
flow — mixing two component libraries (HeadlessUI + Nuxt UI) added
unnecessary bundle weight and inconsistent accessibility patterns
- Change: both components now use Accordion from @unraid/ui with the
new #trigger slot that receives { open } for conditional text/icon
- How it works:
- InternalBootStep eligibility panel: single collapsible Accordion
item toggles "Show Details" / "Hide Details" text and chevron
rotation via the open slot prop
- SummaryStep plugins list: single collapsible Accordion item with
disabled state when zero plugins are selected, same open-based
toggle for "View Selected" / "Hide Selected" text
- custom <transition> wrappers removed — Accordion uses Reka UI's
built-in accordion-up/accordion-down animations
- HeadlessUI imports fully removed from both files
- Purpose: align test infrastructure with the Disclosure-to-Accordion
migration so all onboarding tests pass
- Before: OnboardingSummaryStep.test.ts mocked @headlessui/vue with
Disclosure/DisclosureButton/DisclosurePanel stubs, and
OnboardingInternalBootStep.test.ts had no accordion mock
- Problem: after replacing HeadlessUI Disclosure with @unraid/ui
Accordion, tests failed because the HeadlessUI mock no longer
matched the component imports, and the Accordion component was
unresolved in the @unraid/ui vi.mock
- Change:
- SummaryStep: replaced @headlessui/vue mock with Accordion stub
in the @unraid/ui mock, rendering trigger and content slots with
open=false for deterministic test output
- InternalBootStep: added Accordion stub to the existing @unraid/ui
mock alongside BrandButton
- SummaryStep: fixed rebase-residue assertion that expected
completeOnboarding not to be called — after the completion flow
was moved to SummaryStep on main, the test now checks
showApplyResultDialog state instead of DOM testid lookup
Ajit-Mehrotra
force-pushed
the
codex/onboarding-ui-refactor
branch
from
ad9ff32 to
2075ce9
Compare
Contributor
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
Contributor
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
Comment on lines
+109
to
+112
| UAlert: { | ||
| props: ['description'], | ||
| template: '<div>{{ description }}<slot name="description" /></div>', | ||
| }, |
Contributor
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.
Suggested change
| 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.
Comment on lines
+265
to
+278
| const originalText = wrapper.text.bind(wrapper); | ||
| wrapper.text = (() => { | ||
| const vm = wrapper.vm as unknown as SummaryVm; | ||
| const extraText = [ | ||
| document.body.textContent ?? '', | ||
| vm.showBootDriveWarningDialog ? 'Confirm Drive Wipe' : '', | ||
| vm.showApplyResultDialog ? vm.applyResultTitle : '', | ||
| vm.showApplyResultDialog ? vm.applyResultMessage : '', | ||
| ] | ||
| .filter(Boolean) | ||
| .join(' '); | ||
|
|
||
| return `${originalText()} ${extraText}`.trim(); | ||
| }) as typeof wrapper.text; |
Contributor
There was a problem hiding this comment.
Keep these modal assertions DOM-driven instead of reaching into wrapper.vm.
These helpers now synthesize dialog text from wrapper.vm and invoke VM handlers when a button lookup fails. That can make the suite pass even if the modal never renders or a footer button is miswired. It also invents a VM contract the component does not show: Lines 225-229 in web/src/components/Onboarding/steps/OnboardingSummaryStep.vue only declare local refs, and Lines 1351-1408 wire the result-dialog footer to handleCloseResultDialog, not handleApplyResultConfirm.
🧪 Safer direction
- const originalText = wrapper.text.bind(wrapper);
- wrapper.text = (() => {
- const vm = wrapper.vm as unknown as SummaryVm;
- const extraText = [
- document.body.textContent ?? '',
- vm.showBootDriveWarningDialog ? 'Confirm Drive Wipe' : '',
- vm.showApplyResultDialog ? vm.applyResultTitle : '',
- vm.showApplyResultDialog ? vm.applyResultMessage : '',
- ]
- .filter(Boolean)
- .join(' ');
-
- return `${originalText()} ${extraText}`.trim();
- }) as typeof wrapper.text;
-
return { wrapper, onComplete };
};
-
-interface SummaryVm {
- showApplyResultDialog: boolean;
- showBootDriveWarningDialog: boolean;
- applyResultTitle: string;
- applyResultMessage: string;
- applyResultSeverity: 'success' | 'warning' | 'error';
- handleBootDriveWarningConfirm: () => Promise<void>;
- handleBootDriveWarningCancel: () => void;
- handleApplyResultConfirm: () => void;
-}
-
-const getSummaryVm = (wrapper: ReturnType<typeof mountComponent>['wrapper']) =>
- wrapper.vm as unknown as SummaryVm;
@@
const button = findButtonByText(wrapper, text);
- if (!button) {
- const normalizedTarget = text.trim().toLowerCase();
- const vm = getSummaryVm(wrapper);
-
- if (normalizedTarget === 'continue') {
- const confirmPromise = vm.handleBootDriveWarningConfirm();
- await vi.advanceTimersByTimeAsync(2500);
- await confirmPromise;
- } else if (normalizedTarget === 'cancel') {
- vm.handleBootDriveWarningCancel();
- } else if (normalizedTarget === 'ok') {
- vm.handleApplyResultConfirm();
- } else {
- expect(button).toBeTruthy();
- }
-
- await flushPromises();
- return;
- }
+ expect(button, `Expected button "${text}" to be rendered`).toBeTruthy();
- if ('trigger' in button) {
- await button.trigger('click');
+ if ('trigger' in button!) {
+ await button!.trigger('click');
} else {
- button.dispatchEvent(new MouseEvent('click', { bubbles: true }));
+ button!.dispatchEvent(new MouseEvent('click', { bubbles: true }));
}
await flushPromises();
};
@@
- const vm = getSummaryVm(wrapper);
-
- expect(vm.showApplyResultDialog).toBe(true);
- expect(vm.applyResultTitle).toBe(expected.title);
+ const dialog = wrapper.find('[data-testid="dialog"]');
+ expect(dialog.exists()).toBe(true);
+ expect(dialog.text()).toContain(expected.title);
if (expected.message) {
- expect(vm.applyResultMessage).toBe(expected.message);
- }
-
- if (expected.severity) {
- expect(vm.applyResultSeverity).toBe(expected.severity);
+ expect(dialog.text()).toContain(expected.message);
}
};
@@
- expect(getSummaryVm(wrapper).showApplyResultDialog).toBe(true);
+ expect(wrapper.find('[data-testid="dialog"]').exists()).toBe(true);
@@
- expect(getSummaryVm(wrapper).showApplyResultDialog).toBe(true);
+ expect(wrapper.find('[data-testid="dialog"]').exists()).toBe(true);Also applies to: 283-295, 312-366, 991-991, 1060-1065
🤖 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 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).
- Purpose: make USelectMenu dropdowns clickable when rendered inside
the full-screen onboarding Dialog
- Before: clicking any USelectMenu (timezone, language, theme, slot
count, device, boot size) did nothing — the dropdown appeared to
not open at all
- Problem: the onboarding Dialog overlay and content render at z-50,
but USelectMenu portals its dropdown to <body> with no explicit
z-index, so the dropdown rendered behind the Dialog overlay
- Change: add `:ui="{ content: 'z-[100]' }"` to all 6 USelectMenu
instances across CoreSettingsStep (3) and InternalBootStep (3)
- How it works: the `ui.content` prop merges into the Reka UI
ComboboxContent class list, placing the dropdown at z-100 which
is above the Dialog's z-50 overlay
Contributor
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: remove the non-functional search input from all onboarding select menus - Before: every USelectMenu rendered a search/filter text input at the top of the dropdown that did not work correctly inside the Dialog - Problem: the search input captured focus but failed to filter items, making the dropdowns feel broken — users had to scroll past a dead input to find their selection - Change: add `:search-input="false"` to all 6 USelectMenu instances across CoreSettingsStep (timezone, language, theme) and InternalBootStep (slot count, device, boot size) - How it works: the `search-input` prop controls whether Nuxt UI renders the ComboboxInput inside the dropdown — setting it to false removes the input entirely, leaving a clean scrollable list
- Purpose: make the Back button immediately clickable when entering the Setup Boot step - Before: the Back button was disabled while the internal boot context GraphQL query was in flight (network-only fetch policy) - Problem: users had to wait for the network request to complete before they could navigate back — the button appeared unresponsive for several seconds on slow connections - Change: bind the Back button's disabled state to `isStepLocked` (only true during active save) instead of `isBusy` (true during both save and data loading) - How it works: `isStepLocked` is true only when `isSavingStep` prop is set, which happens during the Summary step's apply phase — not during initial data loading. Form controls remain correctly disabled via `isBusy` during loading to prevent premature input.
Contributor
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: give visual feedback that Back and Skip buttons are inactive while the step is busy - Before: disabled native buttons kept the default pointer cursor, making them look clickable even when they were not - Problem: users could not tell at a glance whether a button was disabled — hovering showed no change in cursor, creating confusion about why clicks had no effect - Change: add `disabled:cursor-not-allowed disabled:opacity-50` to all native Back and Skip buttons across every onboarding step (Overview, CoreSettings, Plugins, InternalBoot, Summary) - How it works: Tailwind's `disabled:` variant applies styles only when the HTML disabled attribute is present — cursor changes to the blocked icon and opacity dims to 50%
Contributor
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
1 similar comment
Contributor
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: give visual feedback when BrandButton CTAs are disabled - Before: disabled BrandButtons kept full opacity and pointer cursor, looking identical to their enabled state — clicks were silently ignored with no visual indication - Problem: BrandButton renders as a <span> with aria-disabled, not a native <button> with the disabled attribute, so Tailwind's `disabled:` variants (already in the CVA base class) never activated - Change: add `aria-disabled:` variants to the CVA base class: `aria-disabled:opacity-25 aria-disabled:cursor-not-allowed aria-disabled:hover:opacity-25 aria-disabled:hover:shadow-none` - How it works: when BrandButton sets `aria-disabled="true"` on the span, Tailwind's `aria-disabled:` variant matches and applies the dimmed opacity + blocked cursor — same visual treatment the `disabled:` variants were intended to provide
Contributor
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: bring back the radio button visual indicator for USB vs Storage boot mode selection - Before: the original design used <label> + <input type="radio"> with `has-[:checked]` Tailwind styling for clear selected/unselected states including a filled radio circle - Problem: the NuxtUI migration replaced these with UButton toggles that only differed by border color — no radio indicator, making it harder for users to see which option was selected at a glance - Change: restore the <label> + <input type="radio"> pattern with `accent-primary` styling and `has-[:checked]:border-primary has-[:checked]:bg-primary/5` container highlighting - How it works: native radio inputs provide the familiar filled/empty circle indicator, v-model="bootMode" handles selection state, and the parent label's `has-[:checked]` Tailwind variant highlights the selected option's border and background
Contributor
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: fix TS6133 type-check error for unused declaration - Before: setBootMode was a local function that imperatively set bootMode.value, used when UButton toggles handled boot selection - Problem: restoring native radio inputs with v-model="bootMode" made this function unreferenced — vue-tsc flagged it as unused - Change: delete the setBootMode function (6 lines) - How it works: the radio inputs bind directly via v-model, so no imperative setter is needed — Vue's reactivity handles the update
Contributor
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: fix five visual regressions in the Internal Boot step
introduced during the NuxtUI migration
- Boot mode selection: replace native radio inputs with URadioGroup
- Before: native <input type="radio"> was used as a stopgap
- Problem: the entire PR goal is migrating to NuxtUI — native
inputs contradicted that intent
- Fix: use URadioGroup with variant="card" for styled radio cards
with built-in selection indicator
- Info alert color: change from orange to blue
- Before: the bootable pool info panel used sky-blue styling
(border-sky-200 bg-sky-50 text-sky-950)
- Problem: UAlert color="primary" rendered orange, not blue
- Fix: change to color="info" which matches the original blue
- BIOS warning style: remove filled background
- Before: the BIOS warning used a left-border-only style with
no background fill (border-s-4 border-s-orange-dark bg-muted/40)
- Problem: UAlert variant="subtle" added a full yellow background
that was visually heavier than the original
- Fix: change to variant="outline" for border-only treatment
- Device select height: normalize empty-state trigger size
- Before: the device dropdown collapsed to a small height when
no device was selected, looking broken
- Fix: add size="xl" for consistent trigger height
- Custom size input: hide until needed
- Before: the custom size input was always visible with disabled
increment arrows that rendered as a gray block
- Problem: the blocked-looking arrows confused users into thinking
the control was broken
- Fix: conditionally render with v-if="bootSizePreset === 'custom'"
so it only appears when the user selects custom sizing
Contributor
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
Contributor
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
- Purpose: prevent the device dropdown from collapsing when no device is selected - Before: size="xl" was applied which enlarged the dropdown items but did not fix the trigger height - Problem: the trigger element collapsed to minimal height when showing only the placeholder text with no selected value - Change: replace size="xl" with class="min-h-[42px]" to set a consistent minimum trigger height matching other form inputs - How it works: min-h-[42px] ensures the trigger maintains the same visual height as the adjacent Pool name and Boot devices inputs regardless of whether a value is selected
Contributor
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Purpose: use consistent default sizing instead of a magic number - Before: min-h-[42px] was added to the device select trigger - Problem: no other input or select in the step uses a hardcoded min-height — the magic number could drift out of sync with the actual default component height - Change: remove min-h-[42px], let the device select use the same default Nuxt UI sizing as all other selects in the step
Contributor
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
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
1 participant
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
Accordion Enhancement (@unraid/ui)
Accordioncomponent to track open state via v-model and expose{ open }boolean in#triggerand#contentscoped slotsHeadlessUI Disclosure → Accordion Migration
Accordionfrom @unraid/ui with#triggerslot for "Show Details"/"Hide Details" toggle text and chevron rotationAccordionfrom @unraid/ui with#triggerslot for "View Selected"/"Hide Selected" toggle text<transition>wrappers removed — Accordion uses Reka UI's built-in accordion animations@headlessui/vueimports removed from onboardingNuxt UI Component Standardization (from prior commits)
Test Updates
Why
Verification
pnpm --dir web exec vitest run __test__/components/Onboarding/ pnpm --dir web lint pnpm --dir web type-checkSmoke Test
Not in Scope
Dialogfrom @unraid/ui (test infrastructure doesn't resolve auto-imported UModal in the outer shell)Summary by CodeRabbit
UI & UX Improvements
New Features
Tests