Skip to content

feat: onboarding use history#1964

Closed
elibosley wants to merge 1 commit intomainfrom
feat/onboarding-use-history
Closed

feat: onboarding use history#1964
elibosley wants to merge 1 commit intomainfrom
feat/onboarding-use-history

Conversation

@elibosley
Copy link
Member

@elibosley elibosley commented Mar 25, 2026

Summary by CodeRabbit

  • New Features

    • Browser back/forward navigation support within onboarding flows
  • Improvements

    • Refined messaging to display "No Updates Needed" when no configuration updates are required
    • Enhanced session handling with improved distinction between automatic and manual onboarding modes
    • Improved history state tracking for better navigation experience during onboarding

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 25, 2026

Walkthrough

This PR introduces browser history tracking to the onboarding flow, enabling users to navigate backward through steps using the browser back button. It adds history state management with session tracking, popstate event handlers, and distinguishes between automatic and manual session sources for different close behaviors. Result messaging for "no changes" scenarios is also updated.

Changes

Cohort / File(s) Summary
Test Setup & History Navigation
web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts
Added history state helpers (__unraidOnboardingInternalBoot key, getInternalBootHistoryState(), dispatchPopstate()), test setup with enableAutoUnmount() and window.history.state reset, updated assertions for "no selection" path to "No Updates Needed" messaging, and added new test cases for browser-back behavior covering reversible and fully-applied results with history state transitions.
Onboarding Test Mocks & Session Sources
web/__test__/components/Onboarding/OnboardingModal.test.ts
Updated mocked step components to accept onComplete, onBack, and showBack props with button-based templates, added sessionSource ('automatic' | 'manual') to mocked store state, and added test case for manual sessions asserting window.history.go(-1) call and deferred closeModal after popstate event.
Minor Test Assertion Updates
web/__test__/components/Onboarding/OnboardingInternalBootStep.test.ts, web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts
Removed eligibility toggle interaction and HAS_INTERNAL_BOOT_PARTITIONS assertion; updated result text expectations from "Setup Applied" to "No Updates Needed" for no-changes scenarios.
History State Management & Navigation
web/src/components/Onboarding/OnboardingModal.vue
Expanded imports (nextTick, onUnmounted, watch), added session-scoped history tracking with __unraidOnboarding state key, introduced sessionSource-driven control flow (reload for automatic, history.back() for manual sessions), implemented popstate handler for step replay/rewind, and added watch to persist history entries on step changes.
Standalone History Navigation & Result Messaging
web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue
Added __unraidOnboardingInternalBoot history tracking with session/position refs and guard flag, updated handleClose() to support history-driven back navigation, modified "Edit Again" gating logic (only when in SUMMARY and result wasn't fully successful), changed "no selection" messaging to noChangesTitle/noChangesMessage, and added popstate listener registration on mount/unmount.
Eligibility Filtering & Result Detection
web/src/components/Onboarding/steps/OnboardingInternalBootStep.vue, web/src/components/Onboarding/steps/OnboardingSummaryStep.vue
Updated eligibility issue aggregation to exclude warning codes (only ineligibilityCodes), replaced hasAnyChangesToApply with hasUpdatesToApply computed from core settings/plugins/internal-boot needs, and added conditional result handling for "no updates" scenarios using result.noChangesTitle/noChangesMessage.
Store & Locale Updates
web/src/components/Onboarding/store/onboardingModalVisibility.ts, web/src/locales/en.json
Exported new OnboardingModalSessionSource type, added reactive sessionSource store property (set to 'manual' on force/resume, reset to 'automatic' on close), and added/updated locale strings for "No Updates Needed" messaging.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Browser
    participant OnboardingModal
    participant Store
    participant History

    User->>Browser: Click back button
    Browser->>History: Trigger popstate event
    Browser->>OnboardingModal: Emit popstate event
    
    OnboardingModal->>OnboardingModal: Check if popstate state<br/>matches session id
    
    alt Matches current session
        OnboardingModal->>OnboardingModal: Get previous step from<br/>popped history state
        OnboardingModal->>Store: Update draftStore &<br/>currentStep
        OnboardingModal->>History: Sync with nextTick
    else Manual session, doesn't match
        OnboardingModal->>OnboardingModal: Set isClosingModal flag
        OnboardingModal->>Store: Close modal &<br/>clear history
        OnboardingModal->>OnboardingModal: Reset isClosingModal
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 A hop through history's twisted vine,
Back and forth, each step's refined,
Popstate whispers, steps replay,
No needless changes, cleaner way!
Browser back now guides the race,
Onboarding finds its perfect place. 🌿

🚥 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 title 'feat: onboarding use history' directly and clearly summarizes the main change—adding browser history integration to the onboarding flow across multiple components.
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
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/onboarding-use-history

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

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e38cdb5e4c

ℹ️ 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".

Comment on lines +522 to +523
historyPosition.value += 1;
window.history.pushState(buildHistoryState(stepId, historyPosition.value), '', window.location.href);

Choose a reason for hiding this comment

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

P2 Badge Do not push history entries for automatic wizard steps

This watcher unconditionally calls window.history.pushState on every step transition, including automatically opened onboarding sessions. In the automatic flow, closeModal does not rewind history, so users who complete or dismiss onboarding are left with multiple same-URL entries and must press Back repeatedly before leaving the page. This regression is introduced by the new history integration and is user-visible whenever onboarding opens automatically and the user advances through steps.

Useful? React with 👍 / 👎.

@codecov
Copy link

codecov bot commented Mar 25, 2026

Codecov Report

❌ Patch coverage is 83.12102% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.09%. Comparing base (9e0e89e) to head (e38cdb5).

Files with missing lines Patch % Lines
web/src/components/Onboarding/OnboardingModal.vue 74.00% 38 Missing and 1 partial ⚠️
...g/standalone/OnboardingInternalBoot.standalone.vue 90.14% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1964      +/-   ##
==========================================
+ Coverage   51.99%   52.09%   +0.10%     
==========================================
  Files        1030     1030              
  Lines       71171    71465     +294     
  Branches     7959     8036      +77     
==========================================
+ Hits        37002    37233     +231     
- Misses      34046    34108      +62     
- Partials      123      124       +1     

☔ 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🤖 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/OnboardingInternalBootStandalone.test.ts`:
- Around line 140-141: Add an explicit cleanup hook to restore spies and timers
after each test: keep the existing enableAutoUnmount(afterEach) for unmounting,
and add an afterEach block that calls vi.restoreAllMocks() to restore any spies
(e.g., vi.spyOn(window.history, 'go')) and vi.useRealTimers() to revert
vi.useFakeTimers(), so mocks and fake timers do not leak between tests.
- Around line 142-172: getInternalBootHistoryState currently re-casts
window.history.state candidate repeatedly which prevents TypeScript from
narrowing types (so sessionId ends up typed as unknown); after verifying
candidate exists, assign it once to a typed local (e.g., const candidateObj =
candidate as Record<string, unknown>) and then read sessionId, stepId and
position from candidateObj so the typeof checks correctly narrow sessionId to
string and stepId to the expected union; keep the existing guards (typeof
checks, specific stepId comparisons, Number.isInteger for position) and return
the InternalBootHistoryState object when all checks pass.

In `@web/src/components/Onboarding/OnboardingModal.vue`:
- Around line 306-315: The closeModal logic currently only rewinds history for
manual sessions, leaving pushState entries from automatic sessions in the back
stack; update closeModal (and its use of historyPosition and isManualSession) to
rewind the same number of entries for automatic sessions as well — i.e., when
historyPosition.value >= 0 call window.history.go(-(historyPosition.value + 1))
before returning regardless of isManualSession, and then reset
historyPosition/related state after the rewind so subsequent closes don't
attempt to rewind again; ensure this change aligns with the existing
replaceState/pushState behavior used during step navigation (see the code around
the pushState/replaceState calls).
- Around line 146-195: getHistoryState repeats the (candidate as Record<string,
unknown>) cast multiple times and buildHistoryState uses historySessionId.value
(string | null) directly inside a object declared to satisfy
OnboardingHistoryState (which requires sessionId: string), causing strict-type
errors; fix by in getHistoryState extracting candidate into a local const (e.g.,
const candidateObj = candidate as Record<string, unknown>) and use that for all
property reads, and in buildHistoryState ensure you pass a validated non-null
sessionId (either accept a sessionId parameter or read a previously-validated
sessionId variable instead of historySessionId.value) so the object placed under
ONBOARDING_HISTORY_STATE_KEY satisfies OnboardingHistoryState without asserting
a nullable string; update callers that invoke buildHistoryState to provide the
validated sessionId if you change the signature.

In
`@web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue`:
- Around line 27-31: The history entry only stores sessionId, stepId and
position (InternalBootHistoryState) so navigating back can restore a SUMMARY
step without its confirmation/result data; extend InternalBootHistoryState to
include the summary substate (e.g., confirmationState and the summary/result
payload) and update every place that writes history (the code that
pushes/replaces history when changing steps or clearing "Edit Again") to persist
that extra fields, or alternatively ensure the SUMMARY entry is replaced when
leaving it (use history.replaceState instead of pushState) so the old SUMMARY
cannot be restored without its result; update references to stepId/position
writes and any code that reads confirmationState to use the new persisted
fields.

In `@web/src/components/Onboarding/steps/OnboardingSummaryStep.vue`:
- Around line 699-707: hasUpdatesToApply is computed from a snapshot
pluginsToInstall taken before refetchInstalledPlugins() runs, causing false
"Setup Applied" when the cache was stale; move the calculation to after the
plugin refresh (or recompute using pluginIdsToInstall.value after awaiting
refetchInstalledPlugins()) and use that fresh value (and
internalBootSelection.value) to decide no-op. Update both occurrences (the one
using pluginsToInstall and the duplicate at 1027-1030) so the final
hasUpdatesToApply is derived from the post-refetch state and handle refetch
failures by treating a failed refresh conservatively (e.g., recompute based on
the latest successfully fetched values or mark as non-noop).

In `@web/src/components/Onboarding/store/onboardingModalVisibility.ts`:
- Around line 45-46: sessionSource is set to 'manual' before awaiting
openOnboardingMutation (and similarly before
resumeOnboardingMutation/refetchOnboarding), which leaves the store stuck in
manual mode if the awaits reject; move the assignment so sessionSource.value =
'manual' only after the awaited calls (openOnboardingMutation() and
refetchOnboarding() / resumeOnboardingMutation()) succeed, or wrap the awaits in
try/catch and revert sessionSource.value to its prior value on error; update the
handlers that use sessionSource (the blocks calling openOnboardingMutation and
resumeOnboardingMutation/refetchOnboarding) to perform the assignment post-await
or to rollback in the catch.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: eb098f2e-00f0-471d-9934-4dd77acce94a

📥 Commits

Reviewing files that changed from the base of the PR and between 9e0e89e and e38cdb5.

📒 Files selected for processing (10)
  • web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts
  • web/__test__/components/Onboarding/OnboardingInternalBootStep.test.ts
  • web/__test__/components/Onboarding/OnboardingModal.test.ts
  • web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts
  • web/src/components/Onboarding/OnboardingModal.vue
  • web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue
  • web/src/components/Onboarding/steps/OnboardingInternalBootStep.vue
  • web/src/components/Onboarding/steps/OnboardingSummaryStep.vue
  • web/src/components/Onboarding/store/onboardingModalVisibility.ts
  • web/src/locales/en.json

Comment on lines +140 to +141
enableAutoUnmount(afterEach);

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, check the test file structure and find it
fd "OnboardingInternalBootStandalone.test.ts" --type f

Repository: unraid/api

Length of output: 129


🏁 Script executed:

#!/bin/bash
# Read the test file to understand cleanup hooks and timer/mock usage
cat -n web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts | head -200

Repository: unraid/api

Length of output: 7495


🏁 Script executed:

#!/bin/bash
# Search for useFakeTimers, history mocking, and afterEach patterns in the file
rg "useFakeTimers|history\.go|afterEach|restoreAllMocks|clearAllMocks" web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts -n

Repository: unraid/api

Length of output: 219


🏁 Script executed:

#!/bin/bash
# Check for afterEach hooks and window.history.go stubbing
rg "afterEach|window\.history\.go|vi\.spyOn\(.*window\.history" web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts -A 3 -B 1

Repository: unraid/api

Length of output: 1403


🏁 Script executed:

#!/bin/bash
# Get the full test file to see all test cases and their setup
wc -l web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts

Repository: unraid/api

Length of output: 133


🏁 Script executed:

#!/bin/bash
# Read more of the file to see all test cases and whether there's history stubbing
tail -n +140 web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts | head -300

Repository: unraid/api

Length of output: 10146


Add explicit cleanup hook to restore spies and real timers between tests.

enableAutoUnmount(afterEach) only unmounts wrappers. Multiple tests create spies with vi.spyOn(window.history, 'go').mockImplementation(...) and vi.useFakeTimers() is set up in beforeEach, but neither are properly restored. vi.clearAllMocks() in beforeEach clears call history only; it does not restore spies or switch back to real timers, allowing the mocked History API and fake timers to leak into subsequent tests.

Suggested fix
 enableAutoUnmount(afterEach);
+
+afterEach(() => {
+  vi.restoreAllMocks();
+  vi.useRealTimers();
+});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts`
around lines 140 - 141, Add an explicit cleanup hook to restore spies and timers
after each test: keep the existing enableAutoUnmount(afterEach) for unmounting,
and add an afterEach block that calls vi.restoreAllMocks() to restore any spies
(e.g., vi.spyOn(window.history, 'go')) and vi.useRealTimers() to revert
vi.useFakeTimers(), so mocks and fake timers do not leak between tests.

Comment on lines +142 to +172
const getInternalBootHistoryState = (): InternalBootHistoryState | null => {
const state =
typeof window.history.state === 'object' && window.history.state !== null
? (window.history.state as Record<string, unknown>)
: null;
const candidate = state?.[INTERNAL_BOOT_HISTORY_STATE_KEY];
if (!candidate || typeof candidate !== 'object') {
return null;
}

const sessionId =
typeof (candidate as Record<string, unknown>).sessionId === 'string'
? (candidate as Record<string, unknown>).sessionId
: null;
const stepId =
(candidate as Record<string, unknown>).stepId === 'CONFIGURE_BOOT' ||
(candidate as Record<string, unknown>).stepId === 'SUMMARY'
? ((candidate as Record<string, unknown>).stepId as InternalBootHistoryState['stepId'])
: null;
const position = Number((candidate as Record<string, unknown>).position);

if (!sessionId || !stepId || !Number.isInteger(position)) {
return null;
}

return {
sessionId,
stepId,
position,
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

fd -t f "OnboardingInternalBootStandalone.test.ts"

Repository: unraid/api

Length of output: 129


🏁 Script executed:

cat -n web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts | sed -n '130,180p'

Repository: unraid/api

Length of output: 1959


🏁 Script executed:

rg -t ts "interface InternalBootHistoryState|type InternalBootHistoryState" --context 5

Repository: unraid/api

Length of output: 1256


🏁 Script executed:

cat -n web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts | sed -n '1,30p'

Repository: unraid/api

Length of output: 1118


🏁 Script executed:

cat -n web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts | sed -n '179,210p'

Repository: unraid/api

Length of output: 1444


🏁 Script executed:

fd -t f "tsconfig" | head -5

Repository: unraid/api

Length of output: 276


🏁 Script executed:

cat web/tsconfig.json 2>/dev/null || cat api/tsconfig.json | head -40

Repository: unraid/api

Length of output: 869


🏁 Script executed:

python3 << 'EOF'
# Analyze the type narrowing behavior
code = """
const candidate: unknown = {};

// Current approach - repeated casts create separate expressions
const sessionId1 = 
  typeof (candidate as Record<string, unknown>).sessionId === 'string'
    ? (candidate as Record<string, unknown>).sessionId  // NEW expression, narrowing from line above doesn't apply
    : null;
// Type of sessionId1: unknown | null (NOT string | null)

// Fixed approach - single cast, single expression
const historyState = candidate as Record<string, unknown>;
const sessionId2 = 
  typeof historyState.sessionId === 'string'
    ? historyState.sessionId  // SAME expression, narrowing applies
    : null;
// Type of sessionId2: string | null
"""

print("Analysis of TypeScript narrowing:")
print("=" * 60)
print("\nThe issue is that repeated casts create separate expressions.")
print("Narrowing from a typeof check only applies to that specific")
print("expression, not to a later identical-looking cast.\n")
print("When sessionId is typed as unknown|null instead of string|null,")
print("the guard '!sessionId' narrows it to 'unknown', not 'string'.")
print("Then returning {sessionId} where sessionId: unknown to a function")
print("expecting sessionId: string causes TS2322 error.\n")
EOF

Repository: unraid/api

Length of output: 565


🏁 Script executed:

cat -n web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts | sed -n '380,430p'

Repository: unraid/api

Length of output: 2218


🏁 Script executed:

cat -n web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts | sed -n '460,480p'

Repository: unraid/api

Length of output: 1012


🏁 Script executed:

cat -n web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts | sed -n '180,196p'

Repository: unraid/api

Length of output: 679


🏁 Script executed:

cat -n web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts | tail -20

Repository: unraid/api

Length of output: 862


Fix TypeScript type narrowing in getInternalBootHistoryState().

Line 168 returns sessionId typed as unknown instead of string. The repeated casts at lines 153–154, 157–159, and 161 prevent narrowing from the typeof checks—each cast is a separate expression, so narrowing from a typeof guard doesn't apply to the next cast. Cache the cast in a single variable and reuse it.

Suggested fix
 const getInternalBootHistoryState = (): InternalBootHistoryState | null => {
   const state =
     typeof window.history.state === 'object' && window.history.state !== null
       ? (window.history.state as Record<string, unknown>)
       : null;
   const candidate = state?.[INTERNAL_BOOT_HISTORY_STATE_KEY];
   if (!candidate || typeof candidate !== 'object') {
     return null;
   }
+  const historyState = candidate as Record<string, unknown>;

   const sessionId =
-    typeof (candidate as Record<string, unknown>).sessionId === 'string'
-      ? (candidate as Record<string, unknown>).sessionId
+    typeof historyState.sessionId === 'string'
+      ? historyState.sessionId
       : null;
   const stepId =
-    (candidate as Record<string, unknown>).stepId === 'CONFIGURE_BOOT' ||
-    (candidate as Record<string, unknown>).stepId === 'SUMMARY'
-      ? ((candidate as Record<string, unknown>).stepId as InternalBootHistoryState['stepId'])
+    historyState.stepId === 'CONFIGURE_BOOT' || historyState.stepId === 'SUMMARY'
+      ? (historyState.stepId as InternalBootHistoryState['stepId'])
       : null;
-  const position = Number((candidate as Record<string, unknown>).position);
+  const position = Number(historyState.position);
🧰 Tools
🪛 GitHub Actions: CI - Main (API)

[error] 168-168: vue-tsc type-check failed (TS2322): Type '{}' is not assignable to type 'string'.

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

In `@web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts`
around lines 142 - 172, getInternalBootHistoryState currently re-casts
window.history.state candidate repeatedly which prevents TypeScript from
narrowing types (so sessionId ends up typed as unknown); after verifying
candidate exists, assign it once to a typed local (e.g., const candidateObj =
candidate as Record<string, unknown>) and then read sessionId, stepId and
position from candidateObj so the typeof checks correctly narrow sessionId to
string and stepId to the expected union; keep the existing guards (typeof
checks, specific stepId comparisons, Number.isInteger for position) and return
the InternalBootHistoryState object when all checks pass.

Comment on lines +146 to +195
const getHistoryState = (state: unknown): OnboardingHistoryState | null => {
if (!state || typeof state !== 'object') {
return null;
}

const candidate = (state as Record<string, unknown>)[ONBOARDING_HISTORY_STATE_KEY];
if (!candidate || typeof candidate !== 'object') {
return null;
}

const sessionId =
typeof (candidate as Record<string, unknown>).sessionId === 'string'
? (candidate as Record<string, unknown>).sessionId
: null;
const stepId =
typeof (candidate as Record<string, unknown>).stepId === 'string'
? ((candidate as Record<string, unknown>).stepId as StepId)
: null;
const source = (candidate as Record<string, unknown>).source === 'manual' ? 'manual' : 'automatic';
const parsedPosition = Number((candidate as Record<string, unknown>).position);
const position = Number.isInteger(parsedPosition) ? parsedPosition : null;

if (!sessionId || !stepId || position === null) {
return null;
}

return {
sessionId,
stepId,
source,
position,
};
};

const buildHistoryState = (stepId: StepId, position: number) => {
const currentState =
typeof window.history.state === 'object' && window.history.state !== null
? (window.history.state as Record<string, unknown>)
: {};

return {
...currentState,
[ONBOARDING_HISTORY_STATE_KEY]: {
sessionId: historySessionId.value,
stepId,
source: sessionSource.value,
position,
} satisfies OnboardingHistoryState,
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the file structure and find the exact lines
fd "OnboardingModal.vue" --type f

Repository: unraid/api

Length of output: 103


🏁 Script executed:

# Check the file size
wc -l web/src/components/Onboarding/OnboardingModal.vue

Repository: unraid/api

Length of output: 107


🏁 Script executed:

# Read the relevant section and surrounding context
sed -n '140,200p' web/src/components/Onboarding/OnboardingModal.vue

Repository: unraid/api

Length of output: 1887


🏁 Script executed:

# Also check the types and state declarations at the top of the file
head -n 100 web/src/components/Onboarding/OnboardingModal.vue

Repository: unraid/api

Length of output: 4310


🏁 Script executed:

# Find where historySessionId is declared
grep -n "historySessionId" web/src/components/Onboarding/OnboardingModal.vue | head -20

Repository: unraid/api

Length of output: 584


🏁 Script executed:

# Find all calls to buildHistoryState
grep -n "buildHistoryState" web/src/components/Onboarding/OnboardingModal.vue

Repository: unraid/api

Length of output: 343


🏁 Script executed:

# Get more context around historySessionId declaration and usage
sed -n '1,250p' web/src/components/Onboarding/OnboardingModal.vue | grep -A 5 -B 5 "historySessionId"

Repository: unraid/api

Length of output: 924


🏁 Script executed:

# Get the watcher context around buildHistoryState calls
sed -n '490,530p' web/src/components/Onboarding/OnboardingModal.vue

Repository: unraid/api

Length of output: 1162


🏁 Script executed:

# Check the closeModal context around line 348
sed -n '340,370p' web/src/components/Onboarding/OnboardingModal.vue

Repository: unraid/api

Length of output: 846


🏁 Script executed:

# Check TypeScript config to see if strict mode is enabled
find . -name "tsconfig.json" -o -name "tsconfig.*.json" | head -5

Repository: unraid/api

Length of output: 286


🏁 Script executed:

# Read the tsconfig to check strictNullChecks and strict mode
cat tsconfig.json | grep -A 10 -B 2 "strict"

Repository: unraid/api

Length of output: 99


🏁 Script executed:

# Check the main tsconfig in the API directory
cat api/tsconfig.json | grep -A 15 '"compilerOptions"' | head -30

Repository: unraid/api

Length of output: 1090


🏁 Script executed:

# Also let's check if there's a root tsconfig or vite config in the web directory
find web -maxdepth 2 -name "tsconfig*" -o -name "vite.config*"

Repository: unraid/api

Length of output: 135


🏁 Script executed:

# Look for any tsconfig in the repository root or web directory
fd "tsconfig" web/ --max-depth 2

Repository: unraid/api

Length of output: 116


🏁 Script executed:

# Check the web tsconfig
cat web/tsconfig.json | grep -A 20 '"compilerOptions"'

Repository: unraid/api

Length of output: 607


🏁 Script executed:

# Also check tsconfig.app.json
cat web/tsconfig.app.json | grep -A 20 '"compilerOptions"'

Repository: unraid/api

Length of output: 565


🏁 Script executed:

# Let's verify the actual TypeScript error by checking if there are any compilation errors
# First, let's see what the actual source shows around those satisfies calls
sed -n '175,195p' web/src/components/Onboarding/OnboardingModal.vue

Repository: unraid/api

Length of output: 550


🏁 Script executed:

# Let's also check how sessionSource.value is typed
grep -n "sessionSource" web/src/components/Onboarding/OnboardingModal.vue | head -10

Repository: unraid/api

Length of output: 325


🏁 Script executed:

# Search for the sessionSource definition in the store
fd "onboardingModalVisibility" web/src --type f

Repository: unraid/api

Length of output: 118


🏁 Script executed:

# Check the onboardingModalVisibility store for sessionSource type
cat web/src/components/Onboarding/store/onboardingModalVisibility.ts

Repository: unraid/api

Length of output: 4925


🏁 Script executed:

# Final verification: check if there are any other places where getHistoryState or buildHistoryState might have similar issues
grep -n "satisfies" web/src/components/Onboarding/OnboardingModal.vue

Repository: unraid/api

Length of output: 97


🏁 Script executed:

# Also verify the exact context where buildHistoryState is called with sessionId checks
sed -n '500,525p' web/src/components/Onboarding/OnboardingModal.vue

Repository: unraid/api

Length of output: 906


Tighten the modal history helper typings.

buildHistoryState() uses historySessionId.value (type string | null) directly in a satisfies OnboardingHistoryState block, but OnboardingHistoryState.sessionId requires string. With TypeScript strict mode enabled, this fails type checking. Additionally, getHistoryState() repeats the Record<string, unknown> cast six times—extract it to a local variable for clarity.

Suggested fix
 const getHistoryState = (state: unknown): OnboardingHistoryState | null => {
   if (!state || typeof state !== 'object') {
     return null;
   }

   const candidate = (state as Record<string, unknown>)[ONBOARDING_HISTORY_STATE_KEY];
   if (!candidate || typeof candidate !== 'object') {
     return null;
   }
+  const historyState = candidate as Record<string, unknown>;

   const sessionId =
-    typeof (candidate as Record<string, unknown>).sessionId === 'string'
-      ? (candidate as Record<string, unknown>).sessionId
+    typeof historyState.sessionId === 'string'
+      ? historyState.sessionId
       : null;
   const stepId =
-    typeof (candidate as Record<string, unknown>).stepId === 'string'
-      ? ((candidate as Record<string, unknown>).stepId as StepId)
+    typeof historyState.stepId === 'string'
+      ? (historyState.stepId as StepId)
       : null;
-  const source = (candidate as Record<string, unknown>).source === 'manual' ? 'manual' : 'automatic';
-  const parsedPosition = Number((candidate as Record<string, unknown>).position);
+  const source = historyState.source === 'manual' ? 'manual' : 'automatic';
+  const parsedPosition = Number(historyState.position);
   const position = Number.isInteger(parsedPosition) ? parsedPosition : null;

   if (!sessionId || !stepId || position === null) {
     return null;
   }

   return {
     sessionId,
     stepId,
     source,
     position,
   };
 };

-const buildHistoryState = (stepId: StepId, position: number) => {
+const buildHistoryState = (sessionId: string, stepId: StepId, position: number) => {
   const currentState =
     typeof window.history.state === 'object' && window.history.state !== null
       ? (window.history.state as Record<string, unknown>)
       : {};

   return {
     ...currentState,
     [ONBOARDING_HISTORY_STATE_KEY]: {
-      sessionId: historySessionId.value,
+      sessionId,
       stepId,
       source: sessionSource.value,
       position,
     } satisfies OnboardingHistoryState,
   };
 };

Update callers to pass the validated sessionId after the null check at lines 506–507:

  if (!historySessionId.value) {
    historySessionId.value = createHistorySessionId();
    historyPosition.value = 0;
    const method = isManualSession.value ? 'pushState' : 'replaceState';
-   window.history[method](buildHistoryState(stepId, historyPosition.value), '', window.location.href);
+   window.history[method](buildHistoryState(historySessionId.value, stepId, historyPosition.value), '', window.location.href);
    return;
  }
  ...
-   window.history.pushState(buildHistoryState(stepId, historyPosition.value), '', window.location.href);
+   window.history.pushState(buildHistoryState(historySessionId.value, stepId, historyPosition.value), '', window.location.href);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/src/components/Onboarding/OnboardingModal.vue` around lines 146 - 195,
getHistoryState repeats the (candidate as Record<string, unknown>) cast multiple
times and buildHistoryState uses historySessionId.value (string | null) directly
inside a object declared to satisfy OnboardingHistoryState (which requires
sessionId: string), causing strict-type errors; fix by in getHistoryState
extracting candidate into a local const (e.g., const candidateObj = candidate as
Record<string, unknown>) and use that for all property reads, and in
buildHistoryState ensure you pass a validated non-null sessionId (either accept
a sessionId parameter or read a previously-validated sessionId variable instead
of historySessionId.value) so the object placed under
ONBOARDING_HISTORY_STATE_KEY satisfies OnboardingHistoryState without asserting
a nullable string; update callers that invoke buildHistoryState to provide the
validated sessionId if you change the signature.

Comment on lines 306 to 315
const closeModal = async (options?: { reload?: boolean }) => {
if (typeof window !== 'undefined' && isManualSession.value && historyPosition.value >= 0) {
window.history.go(-(historyPosition.value + 1));
return;
}

await onboardingModalStore.closeModal();
cleanupOnboardingStorage();
clearHistorySession();

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, locate and read the OnboardingModal.vue file
find . -name "OnboardingModal.vue" -type f

Repository: unraid/api

Length of output: 105


🏁 Script executed:

# Read the file around lines 306-315 to see the closeModal function and its full context
head -n 330 ./web/src/components/Onboarding/OnboardingModal.vue | tail -n +280

Repository: unraid/api

Length of output: 1281


🏁 Script executed:

# Now read lines 487-523 to see what the reviewer said also applies there
head -n 523 ./web/src/components/Onboarding/OnboardingModal.vue | tail -n +487

Repository: unraid/api

Length of output: 1148


🏁 Script executed:

# Let's search for where pushState is called and how historyPosition is used
rg -n "pushState\|replaceState\|historyPosition" ./web/src/components/Onboarding/OnboardingModal.vue | head -n 30

Repository: unraid/api

Length of output: 36


🏁 Script executed:

# Read the entire OnboardingModal.vue file to understand the history management
wc -l ./web/src/components/Onboarding/OnboardingModal.vue

Repository: unraid/api

Length of output: 109


🏁 Script executed:

# Read the entire file to understand the full context
cat -n ./web/src/components/Onboarding/OnboardingModal.vue

Repository: unraid/api

Length of output: 26370


Automatic sessions leave dead browser history entries that persist after modal closure.

The history management differs between session types: automatic sessions use replaceState for the first step (line 509) to avoid history pollution, but subsequent steps unconditionally call pushState (line 523) for all session types. When closeModal() is called, only manual sessions rewind the history stack (line 307-310); automatic sessions skip the rewind and leave the accumulated pushState entries in the browser history. If a user dismisses or completes an automatic onboarding run at step N, the browser back button will traverse through N dead entries, all displaying the same URL with the modal already closed.

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

In `@web/src/components/Onboarding/OnboardingModal.vue` around lines 306 - 315,
The closeModal logic currently only rewinds history for manual sessions, leaving
pushState entries from automatic sessions in the back stack; update closeModal
(and its use of historyPosition and isManualSession) to rewind the same number
of entries for automatic sessions as well — i.e., when historyPosition.value >=
0 call window.history.go(-(historyPosition.value + 1)) before returning
regardless of isManualSession, and then reset historyPosition/related state
after the rewind so subsequent closes don't attempt to rewind again; ensure this
change aligns with the existing replaceState/pushState behavior used during step
navigation (see the code around the pushState/replaceState calls).

Comment on lines +27 to +31
type InternalBootHistoryState = {
sessionId: string;
stepId: StepId;
position: number;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Browser-back can reopen SUMMARY without the result state.

The history payload only stores stepId/position. After the existing "Edit Again" path clears confirmationState, logs, and the result copy, going back to the older SUMMARY entry restores only currentStep, so the summary branch renders with confirmationState === 'idle' and no content. Either persist the summary/result substate in history, or replace the old summary entry when leaving it.

Also applies to: 236-268

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

In
`@web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue`
around lines 27 - 31, The history entry only stores sessionId, stepId and
position (InternalBootHistoryState) so navigating back can restore a SUMMARY
step without its confirmation/result data; extend InternalBootHistoryState to
include the summary substate (e.g., confirmationState and the summary/result
payload) and update every place that writes history (the code that
pushes/replaces history when changing steps or clearing "Edit Again") to persist
that extra fields, or alternatively ensure the SUMMARY entry is replaced when
leaving it (use history.replaceState instead of pushState) so the old SUMMARY
cannot be restored without its result; update references to stepId/position
writes and any code that reads confirmationState to use the new persisted
fields.

Comment on lines +699 to +707
const pluginsToInstall = pluginIdsToInstall.value;
const hasUpdatesToApply =
shouldApplyTimeZone ||
shouldApplyServerIdentity ||
shouldApplyTheme ||
shouldApplyLocale ||
shouldApplySsh ||
pluginsToInstall.length > 0 ||
Boolean(internalBootSelection.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Recompute the no-op result after the plugin refresh.

hasUpdatesToApply is frozen from pluginIdsToInstall before refetchInstalledPlugins() runs. If the cached plugin list is stale and the refresh shows everything is already installed, this path still reports “Setup Applied” even though every step was skipped. The unconditional refetch also means a true no-op can degrade to a warning when that request fails. Pick the final result from the work that actually ran, or recompute after the refresh.

Also applies to: 1027-1030

🤖 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
699 - 707, hasUpdatesToApply is computed from a snapshot pluginsToInstall taken
before refetchInstalledPlugins() runs, causing false "Setup Applied" when the
cache was stale; move the calculation to after the plugin refresh (or recompute
using pluginIdsToInstall.value after awaiting refetchInstalledPlugins()) and use
that fresh value (and internalBootSelection.value) to decide no-op. Update both
occurrences (the one using pluginsToInstall and the duplicate at 1027-1030) so
the final hasUpdatesToApply is derived from the post-refetch state and handle
refetch failures by treating a failed refresh conservatively (e.g., recompute
based on the latest successfully fetched values or mark as non-noop).

Comment on lines +45 to 46
sessionSource.value = 'manual';
await openOnboardingMutation();
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Do not leave sessionSource in "manual" when open/resume fails.

If either mutation or refetchOnboarding() rejects here, the store stays in manual mode even though no manual session was established. The next automatically opened modal will then take the browser-history close path. Set the flag after both awaits succeed, or roll it back on failure.

Suggested fix
   const forceOpenModal = async () => {
     if (!canDisplayOnboardingModal.value) {
       return false;
     }

-    sessionSource.value = 'manual';
     await openOnboardingMutation();
     await refetchOnboarding();
+    sessionSource.value = 'manual';
     return true;
   };
@@
   const resumeOnboarding = async () => {
-    sessionSource.value = 'manual';
     await resumeOnboardingMutation();
     await refetchOnboarding();
+    sessionSource.value = 'manual';
   };

Also applies to: 71-73

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

In `@web/src/components/Onboarding/store/onboardingModalVisibility.ts` around
lines 45 - 46, sessionSource is set to 'manual' before awaiting
openOnboardingMutation (and similarly before
resumeOnboardingMutation/refetchOnboarding), which leaves the store stuck in
manual mode if the awaits reject; move the assignment so sessionSource.value =
'manual' only after the awaited calls (openOnboardingMutation() and
refetchOnboarding() / resumeOnboardingMutation()) succeed, or wrap the awaits in
try/catch and revert sessionSource.value to its prior value on error; update the
handlers that use sessionSource (the blocks calling openOnboardingMutation and
resumeOnboardingMutation/refetchOnboarding) to perform the assignment post-await
or to rollback in the catch.

@Ajit-Mehrotra
Copy link
Contributor

Merged into #1949

Ajit-Mehrotra added a commit that referenced this pull request Mar 25, 2026
…1949)

## Summary

- Standardize the onboarding flow on Nuxt UI primitives across all steps
(Core Settings, Plugins, Internal Boot, License, Summary, Next Steps)
- Replace HeadlessUI Disclosure components with @unraid/ui Accordion
(enhanced with v-model and open state slot props)
- Integrate browser back/forward navigation support and "No Updates
Needed" messaging (from #1964)
- Polish UX: disabled button states, alert styling, skip button
behavior, radio button boot mode selection

## What Changed

### HeadlessUI → Nuxt UI Migration
- **All onboarding steps** now use Nuxt UI components (`USelectMenu`,
`UInput`, `UCheckbox`, `UAlert`, `UModal`, `UButton`, `URadioGroup`)
instead of HeadlessUI or native HTML elements
- **All `@headlessui/vue` imports removed from onboarding**
- Custom `<transition>` wrappers removed — Accordion uses Reka UI's
built-in animations

### Accordion Enhancement (@unraid/ui)
- Extended `Accordion` component with optional `v-model` support,
`itemClass` prop, and `{ open }` boolean in scoped slots
- Changes are **additive** — existing accordion usage is unaffected (no
global style changes)

### UX Polish
- Radio buttons for boot mode selection (USB vs Storage)
- Disabled state on BrandButton via `aria-disabled` with
`pointer-events-none`
- `cursor-not-allowed` on disabled Back/Skip navigation buttons
- Neutral-colored UAlerts for info panels and warnings
- Solid green UAlert for initialization ready state
- Outline variant for Skip button in license step
- Placeholder text in empty device select menus
- Guard against `slotCount` NaN from undefined USelectMenu values
- `:disabled` on reboot modal buttons to prevent double-click

### Browser History + No-Changes Messaging (from #1964)
- Browser back/forward navigation support within onboarding flows
- "No Updates Needed" messaging when no configuration changes are
applied
- Enhanced session handling with improved distinction between automatic
and manual onboarding modes

### Test Updates
- Replaced HeadlessUI Disclosure mocks with Accordion stubs
- Fixed auto-import bypass: tests use VM state for USelectMenu
interaction (Nuxt UI auto-imports bypass `global.stubs`)
- Updated "Setup Applied" → "No Updates Needed" assertions for
no-changes path
- All 618 web tests pass, all 1962 API tests pass

## Why

- The onboarding flow mixed native controls, HeadlessUI components, and
older modal patterns — making the UI feel uneven
- HeadlessUI was an unnecessary dependency when @unraid/ui (backed by
Reka UI) provides equivalent components
- Disabled states were not communicated visually to users

## Includes

- #1964 (feat: onboarding use history) — merged into this branch

## Verification

```bash
pnpm --dir web test -- --run
pnpm --dir web lint
pnpm --dir web type-check
pnpm --dir api test -- --run
pnpm --dir api lint
pnpm --dir api type-check
```

## Smoke Test

1. Walk the full onboarding flow — confirm all controls use consistent
Nuxt UI styling
2. Setup Boot: verify radio buttons for USB/Storage selection, disabled
states on Back/Skip during load
3. Internal Boot: expand/collapse eligibility details, verify device
selects have placeholder text
4. License: test Skip dialog with solid info alert and outline Skip
button
5. Summary: expand/collapse plugins accordion, verify "No Updates
Needed" when no changes apply
6. Next Steps: verify reboot confirmation modal buttons disable during
submission
7. Browser back/forward: navigate between steps using browser history
buttons

---------

Co-authored-by: Eli Bosley <eli@bosley.dev>
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