Skip to content

refactor(onboarding): unify modal gating and force-open controls#1895

Merged
elibosley merged 4 commits into
mainfrom
refactor/onboarding-wizard-gating
Mar 10, 2026
Merged

refactor(onboarding): unify modal gating and force-open controls#1895
elibosley merged 4 commits into
mainfrom
refactor/onboarding-wizard-gating

Conversation

@Ajit-Mehrotra

@Ajit-Mehrotra Ajit-Mehrotra commented Mar 9, 2026

Copy link
Copy Markdown
Contributor
  • Purpose: simplify onboarding visibility rules and make manual opening reliable for all server states while preserving fresh-install auto onboarding behavior.
  • Before: modal visibility mixed legacy activation-code naming, upgrade/downgrade auto-show assumptions, and open paths that only flipped hidden state.
  • Before: admin preset Open could fail for upgrade/downgrade states because non-fresh installs were still blocked by normal render gating.
  • Why this was a problem: behavior differed by entry path, debugging was expensive, and local/admin test workflows could not reliably validate non-fresh-install scenarios.
  • What changed: renamed onboarding stores/files to onboarding-specific names, removed legacy aliases/shims, and centralized visibility semantics around fresh-install auto show plus explicit force-open.
  • What changed: added explicit force-open handling in store/modal flows (URL action/event/store action), and ensured force-open exits do not mark onboarding complete.
  • What changed: admin panel Open actions now use a shared open helper and support an Extra Settings toggle to simulate normal gating instead of force-open for negative-path testing.
  • What changed: moved onboarding constants out of global consts into an onboarding-local constants module and updated all imports/tests.
  • How it works: auto visibility stays fresh-install only (INCOMPLETE plus local guards), bypass remains session+boot scoped, and manual open uses forceOpenModal() unless normal-gating simulation is enabled.
  • Verification: ran onboarding-focused Vitest coverage for modal visibility/store/cleanup and broader onboarding component suites; tests passed.

Summary by CodeRabbit

  • New Features

    • Added support for forcing onboarding modal open via URL parameters and programmatic triggers.
    • Added built-in case model configuration from activation codes.
    • Introduced comprehensive internal boot transfer management with eligibility checking, device selection, and detailed error messaging for configuration issues.
  • Changes

    • Onboarding modal now displays automatically only on fresh installs; manual opening remains available.

@coderabbitai

coderabbitai Bot commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ca386582-c25c-4af9-9c0d-9ff983bc028d

📥 Commits

Reviewing files that changed from the base of the PR and between 3634468 and b882bbe.

📒 Files selected for processing (29)
  • api/generated-schema.graphql
  • api/src/unraid-api/graph/resolvers/customization/activation-code.model.ts
  • api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts
  • api/src/unraid-api/graph/resolvers/customization/onboarding.service.ts
  • api/src/unraid-api/graph/resolvers/onboarding/onboarding.model.ts
  • web/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.ts
  • web/__test__/components/Onboarding/OnboardingInternalBootStep.test.ts
  • web/__test__/components/Onboarding/OnboardingModal.test.ts
  • web/__test__/components/Onboarding/OnboardingOverviewStep.test.ts
  • web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts
  • web/__test__/components/Onboarding/onboardingStorageCleanup.test.ts
  • web/__test__/store/onboardingModalVisibility.test.ts
  • web/src/components/DevModalTest.standalone.vue
  • web/src/components/Onboarding/OnboardingModal.vue
  • web/src/components/Onboarding/UPGRADE_ONBOARDING.md
  • web/src/components/Onboarding/constants.ts
  • web/src/components/Onboarding/graphql/activationCode.query.ts
  • web/src/components/Onboarding/graphql/getInternalBootContext.query.ts
  • web/src/components/Onboarding/standalone/OnboardingAdminPanel.standalone.vue
  • web/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vue
  • web/src/components/Onboarding/steps/OnboardingInternalBootStep.vue
  • web/src/components/Onboarding/steps/OnboardingOverviewStep.vue
  • web/src/components/Onboarding/steps/OnboardingSummaryStep.vue
  • web/src/components/Onboarding/store/onboardingModalVisibility.ts
  • web/src/components/Onboarding/store/onboardingStatus.ts
  • web/src/components/Onboarding/store/onboardingStorageCleanup.ts
  • web/src/composables/gql/gql.ts
  • web/src/composables/gql/graphql.ts
  • web/src/locales/en.json

Disabled knowledge base sources:

  • Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.


Walkthrough

This pull request refactors the onboarding system's store architecture, renaming upgradeOnboarding to onboardingStatus and activationCodeModal to onboardingModalVisibility. It introduces a force-open mechanism for the onboarding modal, adds a caseModel field to branding configuration, extends the internal boot step with per-device eligibility tracking, and expands GraphQL queries to include interfaceType for disk analysis.

Changes

Cohort / File(s) Summary
Store Refactoring - Onboarding Status
web/src/components/Onboarding/store/onboardingStatus.ts, web/src/components/Onboarding/store/upgradeOnboarding.ts
Renamed store file from upgradeOnboarding.ts to onboardingStatus.ts. Changed shouldShowOnboarding logic to return true only for INCOMPLETE status (excluding UPGRADE/DOWNGRADE). Removed useUpgradeOnboardingStore export alias.
Store Refactoring - Modal Visibility
web/src/components/Onboarding/store/onboardingModalVisibility.ts, web/src/components/Onboarding/store/activationCodeModal.ts
Renamed store from activationCodeModal to onboardingModalVisibility. Renamed hook from useActivationCodeModalStore to useOnboardingModalStore. Added force-open mechanism (isForceOpened, forceOpenModal, clearForceOpened). Replaced applyBypassFromUrlParam with applyOnboardingUrlAction to support bypass/resume/open actions. Renamed isVisibleisAutoVisible and isTemporarilyBypassedisBypassActive. Added global event handling for force-open behavior.
Constants and Storage Keys
web/src/components/Onboarding/constants.ts, web/src/components/Onboarding/store/onboardingStorageCleanup.ts, web/__test__/components/Onboarding/onboardingStorageCleanup.test.ts
Renamed storage key constant from ACTIVATION_CODE_MODAL_HIDDEN_STORAGE_KEY to ONBOARDING_MODAL_HIDDEN_STORAGE_KEY. Moved ONBOARDING_TEMP_BYPASS_STORAGE_KEY import to component-scoped constants file.
Component Import Updates
web/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vue, web/src/components/Onboarding/steps/OnboardingOverviewStep.vue, web/src/components/Onboarding/steps/OnboardingSummaryStep.vue
Updated imports from useUpgradeOnboardingStore/useActivationCodeModalStore to useOnboardingStore/useOnboardingModalStore. Changed store import paths to new module locations.
Modal and Main Onboarding Component Updates
web/src/components/Onboarding/OnboardingModal.vue, web/src/components/DevModalTest.standalone.vue
Updated store imports and initialization for onboarding modal and status stores. Modified visibility logic to account for force-open state. Changed modal opening/closing logic to use new forceOpenModal and clearForceOpened APIs. Updated handler functions and UI bindings accordingly.
Admin Panel and Standalone Components
web/src/components/Onboarding/standalone/OnboardingAdminPanel.standalone.vue
Added caseModel field to BrandingConfigPayload. Introduced simulateNormalRenderGatingOnOpen flag for testing render gating behavior. Updated modal opening flow to respect gating logic. Updated grid layout for settings section.
Test File Updates - Store Mocking
web/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.ts, web/__test__/components/Onboarding/OnboardingOverviewStep.test.ts, web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts
Updated mock imports and store references from old store paths to new ones (onboardingStatus, onboardingModalVisibility). Adjusted mocked hook names and store shapes in test scaffolding.
Comprehensive Modal Test Updates
web/__test__/components/Onboarding/OnboardingModal.test.ts
Extensive test refactoring: updated store mocks to new paths and shapes; replaced isVisible/isTemporarilyBypassed with isAutoVisible/isBypassActive/isForceOpened; added tests for force-opened modal scenarios; updated cleanup logic tests; added boot transfer visibility tests; adjusted assertions for new store API methods.
Modal Store Test Updates
web/__test__/store/onboardingModalVisibility.test.ts
Comprehensive test refactoring for renamed store: updated storage key constants; replaced isVisible/isTemporarilyBypassed with isAutoVisible/isBypassActive; renamed method from applyBypassFromUrlParam to applyOnboardingUrlAction; added tests for force-open behavior, URL action handling, and event-driven modal triggering.
Documentation
web/src/components/Onboarding/UPGRADE_ONBOARDING.md
Updated documentation to reflect store renaming, new force-open semantics, removal of server-side per-step metadata, and focus on fresh-install automatic visibility. Documented changes to bypass persistence and URL-driven behaviors.
Internal Boot Step - Eligibility Tracking
web/src/components/Onboarding/steps/OnboardingInternalBootStep.vue, web/__test__/components/Onboarding/OnboardingInternalBootStep.test.ts
Added per-device ineligibility tracking with new type system (InternalBootTransferState, InternalBootEligibilityState, InternalBootSystemEligibilityCode, InternalBootDiskEligibilityCode). Introduced eligibility panel with localized messages. Added constants for boot size minimums (MIN_BOOT_SIZE_MIB, MIN_ELIGIBLE_DEVICE_SIZE_MIB). Added comprehensive test suite for various eligibility scenarios.
GraphQL Schema and Queries
api/generated-schema.graphql, web/src/composables/gql/gql.ts, web/src/composables/gql/graphql.ts, web/src/components/Onboarding/graphql/activationCode.query.ts, web/src/components/Onboarding/graphql/getInternalBootContext.query.ts
Added caseModel field to BrandingConfig and BrandingConfigInput in GraphQL schema. Added interfaceType field to disks in GetInternalBootContext query. Updated generated TypeScript types and graphql function overloads to reflect new fields.
Backend Case Model Support
api/src/unraid-api/graph/resolvers/customization/activation-code.model.ts, api/src/unraid-api/graph/resolvers/customization/onboarding.service.ts, api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts, api/src/unraid-api/graph/resolvers/onboarding/onboarding.model.ts
Added caseModel field to BrandingConfig class with validation and sanitization. Extended onboarding.service.ts to write built-in case model config when no custom asset exists. Added test case for case model config application.
Localization
web/src/locales/en.json
Added 19 new localization keys under onboarding.internalBootStep.eligibility for system and disk eligibility messages, titles, and descriptions.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Modal as OnboardingModal
    participant ModalStore as onboardingModalStore
    participant StatusStore as onboardingStore
    participant UI as Components

    User->>Modal: Visit page or trigger action
    activate Modal
    
    Modal->>ModalStore: Check isForceOpened
    Modal->>StatusStore: Check shouldShowOnboarding
    Modal->>ModalStore: Check isAutoVisible/isBypassActive
    
    alt Force-opened
        ModalStore-->>Modal: isForceOpened = true
        Modal->>UI: Display modal
    else Fresh install & not bypassed
        StatusStore-->>Modal: shouldShowOnboarding = true
        ModalStore-->>Modal: isAutoVisible = true, isBypassActive = false
        Modal->>UI: Display modal
    else Bypassed or not fresh install
        Modal->>UI: Hide modal
    end
    
    User->>Modal: Complete onboarding
    Modal->>ModalStore: clearForceOpened()
    Modal->>ModalStore: setIsHidden()
    Modal->>StatusStore: refetchOnboarding()
    Modal->>UI: Close modal
    
    deactivate Modal
Loading
sequenceDiagram
    participant User
    participant BootStep as OnboardingInternalBootStep
    participant Query as Apollo Query
    participant Backend as API
    participant UI as Template

    User->>BootStep: Mount component
    activate BootStep
    
    BootStep->>Query: GET_INTERNAL_BOOT_CONTEXT
    Query->>Backend: Fetch array/disk/vars state
    Backend-->>Query: Return boot context with interfaceType
    Query-->>BootStep: Cache result
    
    BootStep->>BootStep: Compute system eligibility codes<br/>(array stopped, boot transfer enabled, etc.)
    BootStep->>BootStep: Aggregate disk eligibility codes<br/>per device
    BootStep->>BootStep: Derive internalBootTransferState<br/>& bootEligibilityState
    
    alt Eligible
        BootStep->>UI: Show device options with sizes
        BootStep->>UI: Hide eligibility panel
    else Ineligible
        BootStep->>UI: Show eligibility panel
        BootStep->>UI: Display system & disk codes<br/>with localized messages
        BootStep->>UI: Disable primary action
    end
    
    deactivate BootStep
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Rationale: This pull request involves substantial, heterogeneous changes across multiple systems: comprehensive store refactoring with renamed exports and altered API shapes affecting dozens of files; new eligibility tracking logic in the internal boot step with new type definitions; GraphQL schema extensions with backend support; and coordinated test updates across multiple test suites. While many changes follow consistent patterns (import updates), the diversity of affected domains (stores, components, GraphQL, backend services, localization) and the density of logic changes (force-open mechanism, eligibility aggregation, URL action handling) require careful verification that all wiring is correct and logic flows as intended.

Poem

🐰 The Stores Have Spoken

With whiskers twitched and code in hand,
These stores were shuffled, renamed, and planned—
From upgradeOnboarding to onboardingStatus bright,
And modal visibility reborn in new light!
Force-open and eligibility now lead the way,
A hoppy refactor to modernize the day! 🌟

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and concisely identifies the main refactoring effort: unifying modal visibility controls and introducing force-open functionality for the onboarding modal.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/onboarding-wizard-gating

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

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/OnboardingModal.test.ts (1)

214-221: ⚠️ Potential issue | 🟡 Minor

Cover the blocked-display force-open path.

Both new force-opened cases keep canDisplayOnboardingModal at true, so they never exercise the branch where normal modal gating blocks the dialog. That leaves the main override path in this refactor unverified.

Suggested coverage
+  it('renders when force-opened even when modal display is blocked', () => {
+    activationCodeDataStore.isFreshInstall.value = false;
+    onboardingModalStoreState.isAutoVisible.value = false;
+    onboardingModalStoreState.isForceOpened.value = true;
+    onboardingStatusStore.canDisplayOnboardingModal.value = false;
+
+    const wrapper = mountComponent();
+
+    expect(wrapper.find('[data-testid="dialog"]').exists()).toBe(true);
+  });

Also applies to: 245-264

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/__test__/components/Onboarding/OnboardingModal.test.ts` around lines 214
- 221, The refactor added "force-opened" test cases that never cover the branch
where normal gating blocks the modal because they leave
onboardingStatusStore.canDisplayOnboardingModal true; add tests that set
onboardingModalStoreState.isAutoVisible.value = true and
onboardingStatusStore.shouldShowOnboarding.value = true but set
onboardingStatusStore.canDisplayOnboardingModal.value = false while also
exercising the force-opened cases (e.g., toggle the same force-open flag used in
the existing tests), asserting that the dialog does NOT render; add analogous
blocked-display assertions for the other force-opened test block referenced
around lines 245-264 so the blocked-display path is covered for both variants.
🧹 Nitpick comments (1)
web/__test__/components/Onboarding/OnboardingModal.test.ts (1)

29-35: Let the exit tests prove the dialog actually closes.

clearForceOpened and setIsHidden are pure spies here, so these assertions only confirm an implementation detail. If those stubs update the mocked state, the test can assert the dialog disappears after exit and stay focused on user-visible behavior.

As per coding guidelines, "Test component behavior and output, not implementation details" and "Check component state through rendered output".

Also applies to: 375-376, 398-399, 420-421

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/__test__/components/Onboarding/OnboardingModal.test.ts` around lines 29 -
35, The tests currently assert that the spies
onboardingModalStoreState.setIsHidden and
onboardingModalStoreState.clearForceOpened were called, which verifies
implementation details rather than user-visible behavior; update the tests so
that when those spies are invoked they also mutate the mocked
onboardingModalStoreState (e.g., set isAutoVisible.value to false or
isForceOpened.value appropriately) or call the real handler that does so, then
assert the dialog is removed from the rendered output (e.g., expect
queryByText/queryByRole to be null) after the exit sequence to prove the dialog
actually closes; apply the same change for the other occurrences mentioned
(lines ~375-376, ~398-399, ~420-421) referencing the same
onboardingModalStoreState/setIsHidden/clearForceOpened symbols.
🤖 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/OnboardingModal.test.ts`:
- Around line 402-422: The test currently never enables the bypass flag so it
doesn't exercise the bypass-cleanup path: set
activationCodeDataStore.isBypassActive.value = true (or the appropriate setter
for isBypassActive) before calling mountComponent() in the test to enable bypass
behavior, then assert cleanupOnboardingStorageMock was called and mutateMock was
not; alternatively, if you intend to test the non-bypass force-open exit, rename
the test from "preserves bypass cleanup behavior..." to reflect the actual
scenario; update references to isBypassActive, activationCodeDataStore,
mountComponent, and the existing expectations accordingly.

In `@web/src/components/Onboarding/OnboardingModal.vue`:
- Around line 118-127: The computed showModal short-circuits on isLoginPage or
!canDisplayOnboardingModal before checking isForceOpened, preventing the manual
force-open path (forceOpenModal / onboarding=open / onboarding open event) from
working; fix by moving the isForceOpened check to run before the
login/canDisplay gate (i.e., return true if isForceOpened.value) or include
isForceOpened.value in the initial condition so the computed allows forced opens
even when canDisplayOnboardingModal is false, ensuring showModal respects manual
force-open actions.

In
`@web/src/components/Onboarding/standalone/OnboardingAdminPanel.standalone.vue`:
- Around line 368-375: The branch in openOnboardingModalFromPanel currently
calls onboardingModalStore.setIsHidden(false), which bypasses the fresh-install
gating because isAutoVisible becomes true whenever isHidden === false; change
this to restore the modal's automatic visibility mode instead of forcing it
visible — replace setIsHidden(false) with a call that clears the manual hidden
flag (e.g., onboardingModalStore.resetToAutomaticVisibility() or
onboardingModalStore.clearManualHidden()), and if that helper doesn't exist add
it to onboardingModalStore to unset the manual override so normal gating
(isAutoVisible) is respected; keep the simulateNormalRenderGatingOnOpen check
and use onboardingModalStore.forceOpenModal() only in the else path as before.

In `@web/src/components/Onboarding/store/onboardingModalVisibility.ts`:
- Around line 148-150: The current call to window.history.replaceState({}, '',
nextPath || '/') overwrites any existing history entry state; update the
replaceState call in onboardingModalVisibility where url.searchParams.delete and
nextPath are computed to pass the existing window.history.state (or null if
undefined) instead of {} so the current history state (router/scroll metadata)
is preserved when removing the ONBOARDING_QUERY_ACTION_PARAM from the URL.

---

Outside diff comments:
In `@web/__test__/components/Onboarding/OnboardingModal.test.ts`:
- Around line 214-221: The refactor added "force-opened" test cases that never
cover the branch where normal gating blocks the modal because they leave
onboardingStatusStore.canDisplayOnboardingModal true; add tests that set
onboardingModalStoreState.isAutoVisible.value = true and
onboardingStatusStore.shouldShowOnboarding.value = true but set
onboardingStatusStore.canDisplayOnboardingModal.value = false while also
exercising the force-opened cases (e.g., toggle the same force-open flag used in
the existing tests), asserting that the dialog does NOT render; add analogous
blocked-display assertions for the other force-opened test block referenced
around lines 245-264 so the blocked-display path is covered for both variants.

---

Nitpick comments:
In `@web/__test__/components/Onboarding/OnboardingModal.test.ts`:
- Around line 29-35: The tests currently assert that the spies
onboardingModalStoreState.setIsHidden and
onboardingModalStoreState.clearForceOpened were called, which verifies
implementation details rather than user-visible behavior; update the tests so
that when those spies are invoked they also mutate the mocked
onboardingModalStoreState (e.g., set isAutoVisible.value to false or
isForceOpened.value appropriately) or call the real handler that does so, then
assert the dialog is removed from the rendered output (e.g., expect
queryByText/queryByRole to be null) after the exit sequence to prove the dialog
actually closes; apply the same change for the other occurrences mentioned
(lines ~375-376, ~398-399, ~420-421) referencing the same
onboardingModalStoreState/setIsHidden/clearForceOpened symbols.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4f419f8a-c52c-44f3-ac9d-3f296251c84a

📥 Commits

Reviewing files that changed from the base of the PR and between 4b231ad and 3e43340.

📒 Files selected for processing (17)
  • web/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.ts
  • web/__test__/components/Onboarding/OnboardingModal.test.ts
  • web/__test__/components/Onboarding/OnboardingOverviewStep.test.ts
  • web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts
  • web/__test__/components/Onboarding/onboardingStorageCleanup.test.ts
  • web/__test__/store/onboardingModalVisibility.test.ts
  • web/src/components/DevModalTest.standalone.vue
  • web/src/components/Onboarding/OnboardingModal.vue
  • web/src/components/Onboarding/UPGRADE_ONBOARDING.md
  • web/src/components/Onboarding/constants.ts
  • web/src/components/Onboarding/standalone/OnboardingAdminPanel.standalone.vue
  • web/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vue
  • web/src/components/Onboarding/steps/OnboardingOverviewStep.vue
  • web/src/components/Onboarding/steps/OnboardingSummaryStep.vue
  • web/src/components/Onboarding/store/onboardingModalVisibility.ts
  • web/src/components/Onboarding/store/onboardingStatus.ts
  • web/src/components/Onboarding/store/onboardingStorageCleanup.ts

Comment thread web/__test__/components/Onboarding/OnboardingModal.test.ts
Comment thread web/src/components/Onboarding/constants.ts
Comment thread web/src/components/Onboarding/OnboardingModal.vue
Comment thread web/src/components/Onboarding/store/onboardingModalVisibility.ts Outdated
@codecov

codecov Bot commented Mar 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.59341% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.63%. Comparing base (0736709) to head (b882bbe).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ing/standalone/OnboardingAdminPanel.standalone.vue 0.00% 48 Missing ⚠️
...ts/Onboarding/steps/OnboardingInternalBootStep.vue 95.02% 10 Missing ⚠️
web/src/components/DevModalTest.standalone.vue 0.00% 7 Missing ⚠️
...ents/Onboarding/store/onboardingModalVisibility.ts 97.95% 1 Missing ⚠️
...rc/components/Onboarding/store/onboardingStatus.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1895      +/-   ##
==========================================
+ Coverage   49.83%   50.63%   +0.79%     
==========================================
  Files        1021     1021              
  Lines       70037    70262     +225     
  Branches     7421     7565     +144     
==========================================
+ Hits        34901    35575     +674     
+ Misses      35013    34564     -449     
  Partials      123      123              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Ajit-Mehrotra

Copy link
Copy Markdown
Contributor Author

addressing coderabbit, testing briefly, then should be good for another round of PR review

@elibosley
elibosley marked this pull request as ready for review March 10, 2026 20:07

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
api/src/unraid-api/graph/resolvers/customization/onboarding.service.ts (1)

785-812: ⚠️ Potential issue | 🟠 Major

Validate caseModel before writing it to case-model.cfg.

This branch now persists branding.caseModel after only a trim(). A malformed activation payload can therefore save an invalid model token straight into the live config, which is a risky sink for data that should really be constrained to the case-model format you expect downstream.

Possible hardening
-        const configuredCaseModel = this.activationData.branding?.caseModel?.trim() ?? '';
+        const rawCaseModel = this.activationData.branding?.caseModel?.trim() ?? '';
+        const configuredCaseModel = /^[A-Za-z0-9._-]{1,128}$/.test(rawCaseModel)
+            ? rawCaseModel
+            : '';
+
+        if (rawCaseModel && !configuredCaseModel) {
+            this.logger.warn('Ignoring invalid case-model value from activation data.');
+        }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/src/unraid-api/graph/resolvers/customization/onboarding.service.ts`
around lines 785 - 812, The code writes activationData.branding.caseModel
(configuredCaseModel) directly to case-model.cfg after only trimming; update the
onboarding flow (the method using configuredCaseModel in onboarding.service.ts
that also calls this.replaceTargetWithSource, fileExists and writes to
this.caseModelCfg) to validate and sanitize configuredCaseModel before
persisting: require it to match a strict pattern (e.g. only alphanumerics, dot,
underscore, hyphen, no path separators), enforce max length, and if you expect
filenames or specific extensions validate those (or require a token format),
then use path.basename(safeValue) and only write when the value passes
validation; if validation fails, log a warning and skip writing to
this.caseModelCfg.
🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts (1)

951-969: Loosen the logger assertions in these tests.

These expectations are pinned to exact log text, so harmless wording changes will break the tests even if the case-model behavior is still correct.

Suggested test tightening
-            expect(loggerLogSpy).toHaveBeenCalledWith(`Case model set to mid-tower in ${caseModelCfg}`);
+            expect(loggerLogSpy).toHaveBeenCalledWith(
+                expect.stringContaining('Case model set to mid-tower')
+            );

-        expect(loggerLogSpy).toHaveBeenCalledWith(
-            'No partner case-model configured in activation code, skipping case model setup.'
-        );
+        expect(loggerLogSpy).toHaveBeenCalledWith(
+            expect.stringContaining('skipping case model setup')
+        );

Based on learnings: Test what the code does, not implementation details like exact error message wording - avoid writing tests that break when minor changes are made to error messages, log formats, or other non-essential details.

Also applies to: 1501-1503

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts`
around lines 951 - 969, The test pins logger text too tightly; in the
applyCaseModelConfig tests (referencing applyCaseModelConfig, loggerLogSpy,
caseModelCfg, fs.writeFile) replace the exact-string assertion on loggerLogSpy
with a looser check: assert the logger was called and that the message contains
the important fragment (e.g., contains 'Case model' and the model name) or use a
generic toHaveBeenCalled() so the test verifies behavior (fs.writeFile called
with caseModelCfg and 'mid-tower') without depending on exact wording or
formatting of the log.
web/src/components/Onboarding/standalone/OnboardingAdminPanel.standalone.vue (1)

142-145: The stock partner preset still masks the new caseModel fallback.

genericBrandingConfig sets caseModelImage alongside caseModel, so the shared preset keeps exercising the custom-image path instead of the built-in case-model path described in the reference table. Consider leaving caseModelImage unset in one default preset so this new field is actually testable from the panel.

Also applies to: 1031-1040

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/src/components/Onboarding/standalone/OnboardingAdminPanel.standalone.vue`
around lines 142 - 145, The genericBrandingConfig preset is providing both
caseModel and caseModelImage which prevents the UI from falling back to the
built-in case-model renderer; update the preset (the object named
genericBrandingConfig of type BrandingConfigPayload) to omit or set
caseModelImage to null/undefined so the component will exercise the caseModel
fallback path, and do the same for the other default preset referenced around
the second occurrence to ensure one preset tests the built-in case-model
behavior.
🤖 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/standalone/OnboardingAdminPanel.standalone.vue`:
- Around line 370-377: openOnboardingModalFromPanel currently calls
onboardingModalStore.forceOpenModal(), which only mutates transient in-memory
state and is lost on hard reload; change the flow to persist the "force open"
intent so the modal survives reloads: update openOnboardingModalFromPanel (and
the duplicate block around lines 432-441) to set a persistent flag (e.g.,
onboardingModalStore.setPersistentForceOpen(true) or write a short-lived key to
localStorage/sessionStorage) before triggering the modal open, and update
onboardingModalStore initialization to check that persistent flag on load (clear
it once handled) and open the modal accordingly instead of relying solely on
forceOpenModal(). Ensure you reference onboardingModalStore.forceOpenModal,
onboardingModalStore.setIsHidden, and openOnboardingModalFromPanel when making
the changes.

---

Outside diff comments:
In `@api/src/unraid-api/graph/resolvers/customization/onboarding.service.ts`:
- Around line 785-812: The code writes activationData.branding.caseModel
(configuredCaseModel) directly to case-model.cfg after only trimming; update the
onboarding flow (the method using configuredCaseModel in onboarding.service.ts
that also calls this.replaceTargetWithSource, fileExists and writes to
this.caseModelCfg) to validate and sanitize configuredCaseModel before
persisting: require it to match a strict pattern (e.g. only alphanumerics, dot,
underscore, hyphen, no path separators), enforce max length, and if you expect
filenames or specific extensions validate those (or require a token format),
then use path.basename(safeValue) and only write when the value passes
validation; if validation fails, log a warning and skip writing to
this.caseModelCfg.

---

Nitpick comments:
In `@api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts`:
- Around line 951-969: The test pins logger text too tightly; in the
applyCaseModelConfig tests (referencing applyCaseModelConfig, loggerLogSpy,
caseModelCfg, fs.writeFile) replace the exact-string assertion on loggerLogSpy
with a looser check: assert the logger was called and that the message contains
the important fragment (e.g., contains 'Case model' and the model name) or use a
generic toHaveBeenCalled() so the test verifies behavior (fs.writeFile called
with caseModelCfg and 'mid-tower') without depending on exact wording or
formatting of the log.

In
`@web/src/components/Onboarding/standalone/OnboardingAdminPanel.standalone.vue`:
- Around line 142-145: The genericBrandingConfig preset is providing both
caseModel and caseModelImage which prevents the UI from falling back to the
built-in case-model renderer; update the preset (the object named
genericBrandingConfig of type BrandingConfigPayload) to omit or set
caseModelImage to null/undefined so the component will exercise the caseModel
fallback path, and do the same for the other default preset referenced around
the second occurrence to ensure one preset tests the built-in case-model
behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b1066f02-3964-467c-a4cc-fb5c1f3422c3

📥 Commits

Reviewing files that changed from the base of the PR and between 3e43340 and 3634468.

📒 Files selected for processing (9)
  • api/generated-schema.graphql
  • api/src/unraid-api/graph/resolvers/customization/activation-code.model.ts
  • api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts
  • api/src/unraid-api/graph/resolvers/customization/onboarding.service.ts
  • api/src/unraid-api/graph/resolvers/onboarding/onboarding.model.ts
  • web/src/components/Onboarding/graphql/activationCode.query.ts
  • web/src/components/Onboarding/standalone/OnboardingAdminPanel.standalone.vue
  • web/src/composables/gql/gql.ts
  • web/src/composables/gql/graphql.ts

Comment on lines +370 to +377
const openOnboardingModalFromPanel = () => {
if (simulateNormalRenderGatingOnOpen.value) {
onboardingModalStore.setIsHidden(false);
return;
}

onboardingModalStore.forceOpenModal();
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Hard refresh drops the force-open path.

openOnboardingModalFromPanel() defaults to forceOpenModal(), but this flow reloads the page immediately afterward when Reset Draft + Hard Refresh on Open is enabled. Since forceOpenModal() only flips transient store state, upgrade/downgrade/completed presets fall back to normal gating after the reload and the modal no longer opens.

💡 One way to preserve the open intent across the reload
+const PENDING_FORCE_OPEN_AFTER_REFRESH_KEY =
+  'onboardingAdminPanel.pendingForceOpenAfterRefresh';
+
 onMounted(() => {
   if (typeof window === 'undefined') return;
   resetDraftAndHardRefreshOnOpen.value = localStorage.getItem(RESET_DRAFT_AND_REFRESH_KEY) === 'true';
   simulateNormalRenderGatingOnOpen.value =
     localStorage.getItem(SIMULATE_NORMAL_RENDER_GATING_KEY) === 'true';
+
+  if (localStorage.getItem(PENDING_FORCE_OPEN_AFTER_REFRESH_KEY) === 'true') {
+    localStorage.removeItem(PENDING_FORCE_OPEN_AFTER_REFRESH_KEY);
+    void nextTick(() => onboardingModalStore.forceOpenModal());
+  }
 });
@@
   await applyOverrides();
-  await nextTick();
-  openOnboardingModalFromPanel();
 
   if (resetDraftAndHardRefreshOnOpen.value) {
+    if (!simulateNormalRenderGatingOnOpen.value && typeof window !== 'undefined') {
+      localStorage.setItem(PENDING_FORCE_OPEN_AFTER_REFRESH_KEY, 'true');
+    }
     await hardRefreshPageBestEffort();
+    return;
   }
+
+  await nextTick();
+  openOnboardingModalFromPanel();

Also applies to: 432-441

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/src/components/Onboarding/standalone/OnboardingAdminPanel.standalone.vue`
around lines 370 - 377, openOnboardingModalFromPanel currently calls
onboardingModalStore.forceOpenModal(), which only mutates transient in-memory
state and is lost on hard reload; change the flow to persist the "force open"
intent so the modal survives reloads: update openOnboardingModalFromPanel (and
the duplicate block around lines 432-441) to set a persistent flag (e.g.,
onboardingModalStore.setPersistentForceOpen(true) or write a short-lived key to
localStorage/sessionStorage) before triggering the modal open, and update
onboardingModalStore initialization to check that persistent flag on load (clear
it once handled) and open the modal accordingly instead of relying solely on
forceOpenModal(). Ensure you reference onboardingModalStore.forceOpenModal,
onboardingModalStore.setIsHidden, and openOnboardingModalFromPanel when making
the changes.

Ajit-Mehrotra and others added 4 commits March 10, 2026 16:46
- Purpose: simplify onboarding visibility rules and make manual opening reliable for all server states while preserving fresh-install auto onboarding behavior.
- Before: modal visibility mixed legacy activation-code naming, upgrade/downgrade auto-show assumptions, and open paths that only flipped hidden state.
- Before: admin preset Open could fail for upgrade/downgrade states because non-fresh installs were still blocked by normal render gating.
- Why this was a problem: behavior differed by entry path, debugging was expensive, and local/admin test workflows could not reliably validate non-fresh-install scenarios.
- What changed: renamed onboarding stores/files to onboarding-specific names, removed legacy aliases/shims, and centralized visibility semantics around fresh-install auto show plus explicit force-open.
- What changed: added explicit force-open handling in store/modal flows (URL action/event/store action), and ensured force-open exits do not mark onboarding complete.
- What changed: admin panel Open actions now use a shared open helper and support an Extra Settings toggle to simulate normal gating instead of force-open for negative-path testing.
- What changed: moved onboarding constants out of global consts into an onboarding-local constants module and updated all imports/tests.
- How it works: auto visibility stays fresh-install only (INCOMPLETE plus local guards), bypass remains session+boot scoped, and manual open uses forceOpenModal() unless normal-gating simulation is enabled.
- Verification: ran onboarding-focused Vitest coverage for modal visibility/store/cleanup and broader onboarding component suites; tests passed.
- Purpose: surface every internal-boot blocker the web client can currently detect during onboarding.

- Before: the Setup Boot step collapsed failures into generic unavailable states and did not explain per-disk exclusions.

- Problem: users could fix one issue only to hit the next blocker later, and USB-backed disks were not called out before submission.

- Now: the step aggregates server-level and disk-level eligibility codes, excludes assigned/USB/too-small disks from selection, and keeps labels concise when the option id matches the device name.

- How: query disk interface type for onboarding context, derive diagnostics in the step component, regenerate GraphQL artifacts, and add focused onboarding step tests.
…date admin panel to use it, and preserve history state during URL cleanup.
@elibosley
elibosley force-pushed the refactor/onboarding-wizard-gating branch from 1038449 to b882bbe Compare March 10, 2026 20:46
@elibosley
elibosley merged commit 86e87c9 into main Mar 10, 2026
10 of 11 checks passed
@elibosley
elibosley deleted the refactor/onboarding-wizard-gating branch March 10, 2026 20:53
@github-actions

Copy link
Copy Markdown
Contributor

This plugin has been deployed to Cloudflare R2 and is available for testing.
Download it at this URL:

https://preview.dl.unraid.net/unraid-api/tag/PR1895/dynamix.unraid.net.plg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants