fix: prevent onboarding on API errors#1917
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds GraphQL notification types and deduplicates schema blocks; converts onboarding tracker to a discriminated async result type; introduces OnboardingService APIs; refactors resolvers, mutations, frontend stores, GraphQL bootstrapping, and tests to consume unified onboarding response and resume-draft logic. Changes
sequenceDiagram
participant Client
participant Resolver
participant OnboardingService
participant Tracker as OnboardingTrackerService
participant FS as Filesystem/Override
rect rgba(200,220,255,0.5)
Client->>Resolver: GraphQL query/mutation (resolveOnboarding / complete / reset)
end
rect rgba(200,255,200,0.5)
Resolver->>OnboardingService: getOnboardingResponse({ includeActivationCode: true })
OnboardingService->>Tracker: await getStateResult()
Tracker->>FS: read tracker file or check override
FS-->>Tracker: { kind: 'ok' | 'missing' | 'error', state? }
Tracker-->>OnboardingService: OnboardingTrackerStateResult
OnboardingService-->>Resolver: structured Onboarding response (or throw)
end
alt tracker result.kind === 'error'
Resolver->>Client: error (propagated)
else kind === 'ok' or 'missing'
Resolver->>Client: success (onboarding payload)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Ajit-Mehrotra
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1917 +/- ##
==========================================
+ Coverage 51.04% 51.50% +0.45%
==========================================
Files 1025 1024 -1
Lines 70640 70790 +150
Branches 7738 7803 +65
==========================================
+ Hits 36061 36459 +398
+ Misses 34453 34209 -244
+ Partials 126 122 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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)
api/src/unraid-api/config/onboarding-tracker.service.ts (1)
57-109:⚠️ Potential issue | 🟠 MajorDon't leave
getState()as a non-erroring runtime read path.
getStateResult()only helps callers that opt in.api/src/unraid-api/graph/resolvers/onboarding/onboarding.mutation.ts:32-61still builds its response fromgetState(), so those mutation paths can keep returning cached/default onboarding status when the tracker file can't be read. That means this PR fixes the customization query, but not all onboarding responses.Please migrate the remaining runtime readers to
getStateResult()or makegetState()internal/cached-only so tracker read failures are surfaced consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/config/onboarding-tracker.service.ts` around lines 57 - 109, The current getState() exposes a non-erroring runtime read path and must be made cached-only or replaced by an error-propagating API; change usages to call getStateResult() (which surfaces read errors) instead of getState() so tracker-read failures aren't silently ignored. Specifically, make getState() private or document it as cached-only, update public readers (isCompleted(), getCompletedAtVersion(), and any external callers such as the GraphQL mutation that currently uses getState()) to call getStateResult() and handle the returned OnboardingTrackerStateResult (extract state on kind==='ok' or propagate the error), and ensure readTrackerStateResult() remains the single source of truth for runtime reads. Use the function names getState(), getStateResult(), isCompleted(), getCompletedAtVersion(), getCurrentVersion(), and readTrackerStateResult() to locate and update all references.
🧹 Nitpick comments (3)
plugin/source/dynamix.unraid.net/install/doinst.sh (1)
40-45: Remove this duplicated symlink setup block.Line 40 through Line 45 duplicate the same
corepack/npm/npxlink recreation already done earlier, so this just re-runs identical operations. Keep only one block to avoid redundant install steps and future drift.Suggested diff
-( cd usr/local/bin ; rm -rf corepack ) -( cd usr/local/bin ; ln -sf ../lib/node_modules/corepack/dist/corepack.js corepack ) -( cd usr/local/bin ; rm -rf npm ) -( cd usr/local/bin ; ln -sf ../lib/node_modules/npm/bin/npm-cli.js npm ) -( cd usr/local/bin ; rm -rf npx ) -( cd usr/local/bin ; ln -sf ../lib/node_modules/npm/bin/npx-cli.js npx )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/source/dynamix.unraid.net/install/doinst.sh` around lines 40 - 45, Remove the duplicated symlink recreation block that repeats the corepack/npm/npx setup (the lines performing "( cd usr/local/bin ; rm -rf corepack )", "( cd usr/local/bin ; ln -sf ../lib/node_modules/corepack/dist/corepack.js corepack )", "( cd usr/local/bin ; rm -rf npm )", "( cd usr/local/bin ; ln -sf ../lib/node_modules/npm/bin/npm-cli.js npm )", "( cd usr/local/bin ; rm -rf npx )", "( cd usr/local/bin ; ln -sf ../lib/node_modules/npm/bin/npx-cli.js npx )"); keep the original single block earlier in the script and delete this repeated block so the symlinks for corepack, npm and npx are only created once.web/src/components/Onboarding/store/onboardingStatus.ts (1)
90-93: Verify partial-data GraphQL errors don’t over-block modal display.With
errorPolicy: 'all', Apollo can return bothresultanderror. Current logic blocks on any error unless a draft exists, which may hide the modal even when onboarding data is still present.Suggested adjustment (after verification)
- const canDisplayOnboardingModal = computed( - () => isVersionSupported.value && (hasResumableDraft.value || !hasOnboardingQueryError.value) - ); + const canDisplayOnboardingModal = computed( + () => + isVersionSupported.value && + (hasResumableDraft.value || Boolean(onboardingData.value) || !hasOnboardingQueryError.value) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Onboarding/store/onboardingStatus.ts` around lines 90 - 93, The current blocking logic treats any Apollo error as fatal; change hasOnboardingQueryError to only consider an error blocking when there is no usable onboarding data returned. Replace the Boolean(onboardingError.value) check with one that also ensures the query result is empty (e.g., computed(() => Boolean(onboardingError.value) && !onboardingQueryResult.value?.data)), so canDisplayOnboardingModal uses errors only when there is truly no onboarding data; update references to onboardingError, onboardingQueryResult, hasOnboardingQueryError, and canDisplayOnboardingModal accordingly.api/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts (1)
50-58: Avoid asserting the exact error text here.The behavior that matters is that tracker read failures reject. This assertion will fail on harmless message edits while the resolver behavior is still correct.
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"♻️ Suggested tweak
- await expect(resolver.resolveOnboarding()).rejects.toThrow( - 'Onboarding tracker state is unavailable.' - ); + await expect(resolver.resolveOnboarding()).rejects.toThrow();🤖 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/customization.resolver.spec.ts` around lines 50 - 58, The test currently asserts the exact error message from resolver.resolveOnboarding; change it to only assert that the promise rejects (or rejects with an Error) rather than matching the exact text. Update the spec that mocks onboardingTracker.getStateResult (in this test using onboardingTracker.getStateResult and resolver.resolveOnboarding) to assert using await expect(resolver.resolveOnboarding()).rejects.toThrow() or .rejects.toBeInstanceOf(Error) instead of checking the specific message string so the test verifies failure behavior without depending on exact wording.
🤖 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__/store/onboardingStatus.test.ts`:
- Around line 74-82: Rename the test title to accurately reflect the asserted
behavior: change the it(...) description that currently says the modal is
blocked while loading to something like "blocks auto-show while the onboarding
query is still loading" so it matches the assertions on
canDisplayOnboardingModal and shouldShowOnboarding; update the test description
surrounding the useOnboardingStore invocation and the expectations for
canDisplayOnboardingModal and shouldShowOnboarding accordingly.
---
Outside diff comments:
In `@api/src/unraid-api/config/onboarding-tracker.service.ts`:
- Around line 57-109: The current getState() exposes a non-erroring runtime read
path and must be made cached-only or replaced by an error-propagating API;
change usages to call getStateResult() (which surfaces read errors) instead of
getState() so tracker-read failures aren't silently ignored. Specifically, make
getState() private or document it as cached-only, update public readers
(isCompleted(), getCompletedAtVersion(), and any external callers such as the
GraphQL mutation that currently uses getState()) to call getStateResult() and
handle the returned OnboardingTrackerStateResult (extract state on kind==='ok'
or propagate the error), and ensure readTrackerStateResult() remains the single
source of truth for runtime reads. Use the function names getState(),
getStateResult(), isCompleted(), getCompletedAtVersion(), getCurrentVersion(),
and readTrackerStateResult() to locate and update all references.
---
Nitpick comments:
In
`@api/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts`:
- Around line 50-58: The test currently asserts the exact error message from
resolver.resolveOnboarding; change it to only assert that the promise rejects
(or rejects with an Error) rather than matching the exact text. Update the spec
that mocks onboardingTracker.getStateResult (in this test using
onboardingTracker.getStateResult and resolver.resolveOnboarding) to assert using
await expect(resolver.resolveOnboarding()).rejects.toThrow() or
.rejects.toBeInstanceOf(Error) instead of checking the specific message string
so the test verifies failure behavior without depending on exact wording.
In `@plugin/source/dynamix.unraid.net/install/doinst.sh`:
- Around line 40-45: Remove the duplicated symlink recreation block that repeats
the corepack/npm/npx setup (the lines performing "( cd usr/local/bin ; rm -rf
corepack )", "( cd usr/local/bin ; ln -sf
../lib/node_modules/corepack/dist/corepack.js corepack )", "( cd usr/local/bin ;
rm -rf npm )", "( cd usr/local/bin ; ln -sf
../lib/node_modules/npm/bin/npm-cli.js npm )", "( cd usr/local/bin ; rm -rf npx
)", "( cd usr/local/bin ; ln -sf ../lib/node_modules/npm/bin/npx-cli.js npx )");
keep the original single block earlier in the script and delete this repeated
block so the symlinks for corepack, npm and npx are only created once.
In `@web/src/components/Onboarding/store/onboardingStatus.ts`:
- Around line 90-93: The current blocking logic treats any Apollo error as
fatal; change hasOnboardingQueryError to only consider an error blocking when
there is no usable onboarding data returned. Replace the
Boolean(onboardingError.value) check with one that also ensures the query result
is empty (e.g., computed(() => Boolean(onboardingError.value) &&
!onboardingQueryResult.value?.data)), so canDisplayOnboardingModal uses errors
only when there is truly no onboarding data; update references to
onboardingError, onboardingQueryResult, hasOnboardingQueryError, and
canDisplayOnboardingModal accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6e51ce32-b72a-4f08-9707-5c1451ccb2c2
⛔ Files ignored due to path filters (1)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (9)
api/generated-schema.graphqlapi/src/unraid-api/config/onboarding-tracker.service.spec.tsapi/src/unraid-api/config/onboarding-tracker.service.tsapi/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.tsapi/src/unraid-api/graph/resolvers/customization/customization.resolver.tsplugin/source/dynamix.unraid.net/install/doinst.shweb/__test__/store/onboardingStatus.test.tsweb/src/components/Onboarding/store/onboardingDraft.tsweb/src/components/Onboarding/store/onboardingStatus.ts
Ajit-Mehrotra
added a commit
that referenced
this pull request
Mar 15, 2026
- Purpose: resolve the outstanding CodeRabbit findings on PR #1917 after rebasing onto the latest mainline. - Before: the onboarding store over-blocked on partial Apollo errors, tracker read failures could still be flattened through mutation/runtime helper paths, one resolver spec asserted exact error text, and the plugin install script repeated the symlink block. - Problem: review comments highlighted behavior drift, brittle tests, and redundant install steps that could leave the PR confusing or incomplete. - Change: tighten the onboarding error gate for partial data, route runtime tracker readers through the error-propagating result path, relax the brittle spec assertion, rename the mismatched test, and collapse the duplicated install block. - How: update the API/web logic and focused tests for onboarding state handling, mutation responses, and tracker reads, then validate with targeted Vitest runs plus a shell syntax check for the install script.
Ajit-Mehrotra
force-pushed
the
codex/onboarding-error-fallback
branch
from
997272a to
98a07b3
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
web/__test__/store/onboardingStatus.test.ts (1)
162-172: Consider movingvi.mock()calls above the describe block for readability.While Vitest automatically hoists
vi.mock()calls, placing them after imports (before the describe block) is the conventional location and improves readability for developers scanning the file.♻️ Suggested reordering
import { useOnboardingDraftStore } from '~/components/Onboarding/store/onboardingDraft'; import { useOnboardingStore } from '~/components/Onboarding/store/onboardingStatus'; import { useServerStore } from '~/store/server'; +vi.mock('@vue/apollo-composable', () => ({ + useQuery: () => useQueryMock(), +})); + +vi.mock('~/components/Onboarding/store/onboardingDraft', () => ({ + useOnboardingDraftStore: vi.fn(), +})); + +vi.mock('~/store/server', () => ({ + useServerStore: vi.fn(), +})); + type OnboardingQueryResult = { // ... }; // ... rest of file ... -vi.mock('@vue/apollo-composable', () => ({ - useQuery: () => useQueryMock(), -})); - -vi.mock('~/components/Onboarding/store/onboardingDraft', () => ({ - useOnboardingDraftStore: vi.fn(), -})); - -vi.mock('~/store/server', () => ({ - useServerStore: vi.fn(), -}));As per coding guidelines: "Place all mock declarations at the top level in Pinia store tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/store/onboardingStatus.test.ts` around lines 162 - 172, Move the three vi.mock declarations for '@vue/apollo-composable', '~/components/Onboarding/store/onboardingDraft', and '~/store/server' out of the describe block and place them at the top-level (after the imports, before the describe) so they are easier to find; keep the mocked factories the same (useQueryMock for useQuery, vi.fn() for useOnboardingDraftStore and useServerStore) and ensure they remain hoisted by Vitest.api/src/unraid-api/config/onboarding-tracker.service.spec.ts (1)
171-186: Consider removing exact error message assertions.These tests assert the exact error message string
'permission denied'. Per coding guidelines, tests should use.rejects.toThrow()without arguments to avoid brittle tests that break when error messages change.♻️ Suggested refactor
- await expect(tracker.isCompleted()).rejects.toThrow('permission denied'); + await expect(tracker.isCompleted()).rejects.toThrow();- await expect(tracker.getCompletedAtVersion()).rejects.toThrow('permission denied'); + await expect(tracker.getCompletedAtVersion()).rejects.toThrow();As per coding guidelines: "Use
.rejects.toThrow()without arguments to test that functions throw errors. Don't test exact error message strings unless the message format is specifically what you're testing."Also applies to: 188-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/config/onboarding-tracker.service.spec.ts` around lines 171 - 186, The test asserts the exact error message 'permission denied' which is brittle; update the assertions in the OnboardingTrackerService spec so they only check that the promise rejects (use .rejects.toThrow() with no argument) for calls to tracker.isCompleted() after onApplicationBootstrap(), and similarly update the other occurrence noted (lines ~188-203) that uses mockReadFile to throw to avoid matching exact error strings; leave the mock behavior and error throw intact but remove the specific message argument from the .rejects.toThrow assertions.api/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts (1)
50-57: Consider adding test coverage for the'missing'kind variant.The resolver treats both
'ok'and'missing'state kinds identically—both provide a state object that is processed normally, while only'error'throws. Currently, all successful tests use'ok', leaving no coverage for the'missing'variant. Adding a test case would document that a missing tracker file is a valid scenario (not an error) and ensure all three union type variants are exercised.🤖 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/customization.resolver.spec.ts` around lines 50 - 57, Add a new unit test that covers the onboardingTracker.getStateResult "missing" variant: mock vi.mocked(onboardingTracker.getStateResult) to resolve to { kind: 'missing' } and call resolver.resolveOnboarding(), asserting it does not throw and returns/handles the default state result the same way as the 'ok' path; place this alongside the existing tests for 'ok' and 'error' to ensure the resolver.resolveOnboarding behavior for the 'missing' kind is explicitly verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/src/unraid-api/config/onboarding-tracker.service.spec.ts`:
- Around line 171-186: The test asserts the exact error message 'permission
denied' which is brittle; update the assertions in the OnboardingTrackerService
spec so they only check that the promise rejects (use .rejects.toThrow() with no
argument) for calls to tracker.isCompleted() after onApplicationBootstrap(), and
similarly update the other occurrence noted (lines ~188-203) that uses
mockReadFile to throw to avoid matching exact error strings; leave the mock
behavior and error throw intact but remove the specific message argument from
the .rejects.toThrow assertions.
In
`@api/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts`:
- Around line 50-57: Add a new unit test that covers the
onboardingTracker.getStateResult "missing" variant: mock
vi.mocked(onboardingTracker.getStateResult) to resolve to { kind: 'missing' }
and call resolver.resolveOnboarding(), asserting it does not throw and
returns/handles the default state result the same way as the 'ok' path; place
this alongside the existing tests for 'ok' and 'error' to ensure the
resolver.resolveOnboarding behavior for the 'missing' kind is explicitly
verified.
In `@web/__test__/store/onboardingStatus.test.ts`:
- Around line 162-172: Move the three vi.mock declarations for
'@vue/apollo-composable', '~/components/Onboarding/store/onboardingDraft', and
'~/store/server' out of the describe block and place them at the top-level
(after the imports, before the describe) so they are easier to find; keep the
mocked factories the same (useQueryMock for useQuery, vi.fn() for
useOnboardingDraftStore and useServerStore) and ensure they remain hoisted by
Vitest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5db607d5-941a-40ea-9de7-b662abd53980
⛔ Files ignored due to path filters (1)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (13)
api/generated-schema.graphqlapi/src/unraid-api/config/api-config.test.tsapi/src/unraid-api/config/onboarding-tracker.service.spec.tsapi/src/unraid-api/config/onboarding-tracker.service.tsapi/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.tsapi/src/unraid-api/graph/resolvers/customization/customization.resolver.tsapi/src/unraid-api/graph/resolvers/customization/onboarding.service.tsapi/src/unraid-api/graph/resolvers/onboarding/onboarding.mutation.spec.tsapi/src/unraid-api/graph/resolvers/onboarding/onboarding.mutation.tsplugin/source/dynamix.unraid.net/install/doinst.shweb/__test__/store/onboardingStatus.test.tsweb/src/components/Onboarding/store/onboardingDraft.tsweb/src/components/Onboarding/store/onboardingStatus.ts
💤 Files with no reviewable changes (1)
- plugin/source/dynamix.unraid.net/install/doinst.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/Onboarding/store/onboardingStatus.ts
- web/src/components/Onboarding/store/onboardingDraft.ts
Member
|
I hit one more regression while reviewing the resume flow:
Because the internal-boot visibility query is now |
elibosley
reviewed
Mar 16, 2026
| */ | ||
| private async buildOnboardingResponse(): Promise<Onboarding> { | ||
| const state = this.onboardingTracker.getState(); | ||
| const stateResult = await this.onboardingTracker.getStateResult(); |
Member
There was a problem hiding this comment.
This is duplicated from
const trackerStateResult = await this.onboardingTracker.getStateResult();
if (trackerStateResult.kind === 'error') {
throw new GraphQLError('Onboarding tracker state is unavailable.');
}
const state = trackerStateResult.state;
elibosley
reviewed
Mar 16, 2026
| */ | ||
| private async buildOnboardingResponse(): Promise<Onboarding> { | ||
| const state = this.onboardingTracker.getState(); | ||
| const stateResult = await this.onboardingTracker.getStateResult(); |
Member
There was a problem hiding this comment.
This entire function is duplicated and must be moved into a service.
- Purpose: tighten onboarding error handling so real API/tracker failures hide the modal, while missing tracker state and saved drafts can still resume the flow. - Before: onboarding tracker reads flattened missing and failed states together, and the modal gate did not preserve resumable drafts during restart windows. - Problem: summary-step API restarts could strand users mid-flow, while tracker failures could be misinterpreted as normal onboarding state. - Change: model tracker reads with explicit ok/missing/error results, surface true tracker failures as GraphQL errors, and allow the web modal to stay displayable when a resumable draft exists. - How: add tracker/result tests, update the customization resolver and generated GraphQL artifacts, and compute resumable draft visibility in the onboarding stores.
- Purpose: make the packaged plugin expose the expected package-manager entrypoints after install. - Before: the install script only recreated npm and npx links, leaving corepack unavailable from the installed environment. - Problem: package-manager workflows that rely on corepack could fail even though Node tooling was otherwise installed. - Change: create the corepack symlink during plugin install alongside the npm and npx links. - How: extend doinst.sh to link usr/local/bin/corepack to the bundled corepack entrypoint while preserving the existing npm and npx setup.
- Purpose: resolve the outstanding CodeRabbit findings on PR #1917 after rebasing onto the latest mainline. - Before: the onboarding store over-blocked on partial Apollo errors, tracker read failures could still be flattened through mutation/runtime helper paths, one resolver spec asserted exact error text, and the plugin install script repeated the symlink block. - Problem: review comments highlighted behavior drift, brittle tests, and redundant install steps that could leave the PR confusing or incomplete. - Change: tighten the onboarding error gate for partial data, route runtime tracker readers through the error-propagating result path, relax the brittle spec assertion, rename the mismatched test, and collapse the duplicated install block. - How: update the API/web logic and focused tests for onboarding state handling, mutation responses, and tracker reads, then validate with targeted Vitest runs plus a shell syntax check for the install script.
- Purpose: address the PR feedback about duplicated onboarding response assembly in the mutation resolver. - Before: both the customization resolver and onboarding mutation resolver rebuilt the same onboarding payload, status calculation, and tracker error handling separately. - Problem: the duplicated logic was easy to let drift, harder to test cleanly, and made the mutation path smellier than it needed to be. - Change: move onboarding response construction into OnboardingService and make both resolvers delegate to that shared implementation. - How: add a shared service method for building onboarding responses, keep tracker/error handling in one place, and reshape the resolver specs to validate delegation while the service spec covers the response-building logic.
Ajit-Mehrotra
force-pushed
the
codex/onboarding-error-fallback
branch
from
bfc580b to
aea36b8
Compare
- Purpose: keep resumed onboarding drafts anchored to the intended step even when the visible step list changes while boot visibility loads. - Before: onboarding only persisted currentStepIndex, so a network-only internal boot visibility refetch could temporarily remove CONFIGURE_BOOT and reinterpret saved drafts against a shorter list. - Problem: resumed internal boot drafts could jump to the plugins step, and later-step resumes could briefly render a blank modal until the query returned. - Change: persist currentStepId in the draft store, resolve the active modal step by id first, and keep a resumed CONFIGURE_BOOT step available while the visibility query is still loading. - How: add normalized step-id persistence, sync modal navigation through draftStore.setCurrentStep, and cover the loading-window resume regression in OnboardingModal tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
web/src/components/Onboarding/store/onboardingStatus.ts (1)
11-11: Use the.jsESM path for this local store import.Keep new local TypeScript imports on the repo’s
.jssuffix convention so ESM output stays consistent.♻️ Proposed fix
-import { useOnboardingDraftStore } from '~/components/Onboarding/store/onboardingDraft'; +import { useOnboardingDraftStore } from '~/components/Onboarding/store/onboardingDraft.js';As per coding guidelines,
TypeScript imports use.jsextensions for ESM compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Onboarding/store/onboardingStatus.ts` at line 11, The import of the local store should use the repo's ESM .js suffix: update the import of useOnboardingDraftStore in onboardingStatus.ts to point to the compiled ESM path (change '~/components/Onboarding/store/onboardingDraft' to './onboardingDraft.js' style ESM import) so TypeScript output remains consistent with .js extensions while keeping the same exported symbol useOnboardingDraftStore.api/src/unraid-api/graph/resolvers/onboarding/onboarding.mutation.spec.ts (1)
47-49: Prefer typed fixtures overas neverhere.These casts disable the test's ability to catch constructor/input drift at compile time. Typing the doubles as narrow
Pick<...>shapes and usingOnboardingOverrideInputdirectly keeps the spec aligned with the real resolver contract.As per coding guidelines, "Avoid using casting whenever possible, prefer proper typing from the start."
Also applies to: 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/graph/resolvers/onboarding/onboarding.mutation.spec.ts` around lines 47 - 49, Replace the "as never" casts on the test fixtures (onboardingOverrides, onboardingService, onboardingInternalBootService) with properly typed doubles that mirror the resolver contract: type onboardingOverrides as OnboardingOverrideInput (or a narrow Pick<OnboardingOverrideInput, 'fieldA'|'fieldB'> shape for only the used fields) and type service doubles with Pick types of the actual service interfaces used by the resolver (e.g., Pick<OnboardingService, 'methodX'|'methodY'> and Pick<OnboardingInternalBootService, 'bootMethod'>). Update the spec to provide objects matching those types instead of casting to never so the compiler can catch constructor/input drift against OnboardingOverrideInput and the real service interfaces.api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts (1)
272-336: Add an explicit'missing'tracker-state case to this suite.The new contract in this PR is that a missing
onboarding-tracker.jsonbehaves like an empty onboarding state, not an error. Right now the suite only pinsokanderror, so the'missing'branch could regress unnoticed.Suggested test
it('throws when tracker state is unavailable', async () => { onboardingTrackerMock.getStateResult.mockResolvedValue({ kind: 'error', error: new Error('permission denied'), }); await expect(service.getOnboardingResponse()).rejects.toThrow( 'Onboarding tracker state is unavailable.' ); }); + + it('treats a missing tracker file as an incomplete onboarding state', async () => { + onboardingTrackerMock.getStateResult.mockResolvedValue({ + kind: 'missing', + state: { + completed: false, + completedAtVersion: undefined, + }, + }); + + await expect(service.getOnboardingResponse()).resolves.toMatchObject({ + status: OnboardingStatus.INCOMPLETE, + completed: false, + completedAtVersion: undefined, + }); + });🤖 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 272 - 336, Add a test case for the "missing" tracker-state branch by mocking onboardingTrackerMock.getStateResult to resolve with { kind: 'missing' } and asserting service.getOnboardingResponse() returns an empty/incomplete onboarding response (same behavior as an empty state) rather than throwing; update the describe('getOnboardingResponse') suite to include an it(...) that calls service.getOnboardingResponse (without includeActivationCode) and expects a resolved value matching OnboardingStatus.INCOMPLETE, completed: false, completedAtVersion: undefined, and that getActivationData is not called, referencing the onboardingTrackerMock.getStateResult and service.getOnboardingResponse 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 `@api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts`:
- Around line 288-294: The mock for getPublicPartnerInfo uses branding: null
which violates PublicPartnerInfo (branding?: BrandingConfig) — change it to
branding: undefined; also replace the casted ActivationCode mock
(mockResolvedValue({ code: ' ABC123 '} as ActivationCode)) with a proper
instance using plainToInstance(ActivationCode, { code: ' ABC123 ' }) to match
the file's pattern; update the spies on service.getPublicPartnerInfo and
service.getActivationData accordingly to use PublicPartnerInfo with undefined
branding and an ActivationCode instance.
In `@web/src/components/Onboarding/store/onboardingDraft.ts`:
- Around line 121-128: hasResumableDraft is derived from live Pinia refs but
clearing localStorage (in onboardingStorageCleanup.ts) or bypassing the modal
(in onboardingModalVisibility.ts) doesn’t reset those live refs, leaving the
draft resumable in-memory; update both modules to also reset the onboarding
draft store: import the onboarding draft store used by hasResumableDraft and
either call its reset method (if available) or explicitly set currentStepIndex =
0 and booleans coreSettingsInitialized, pluginSelectionInitialized,
internalBootInitialized, internalBootApplySucceeded to false when clearing
storage or when bypassing the modal so the computed hasResumableDraft reflects
the cleared state.
---
Nitpick comments:
In `@api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts`:
- Around line 272-336: Add a test case for the "missing" tracker-state branch by
mocking onboardingTrackerMock.getStateResult to resolve with { kind: 'missing' }
and asserting service.getOnboardingResponse() returns an empty/incomplete
onboarding response (same behavior as an empty state) rather than throwing;
update the describe('getOnboardingResponse') suite to include an it(...) that
calls service.getOnboardingResponse (without includeActivationCode) and expects
a resolved value matching OnboardingStatus.INCOMPLETE, completed: false,
completedAtVersion: undefined, and that getActivationData is not called,
referencing the onboardingTrackerMock.getStateResult and
service.getOnboardingResponse symbols.
In `@api/src/unraid-api/graph/resolvers/onboarding/onboarding.mutation.spec.ts`:
- Around line 47-49: Replace the "as never" casts on the test fixtures
(onboardingOverrides, onboardingService, onboardingInternalBootService) with
properly typed doubles that mirror the resolver contract: type
onboardingOverrides as OnboardingOverrideInput (or a narrow
Pick<OnboardingOverrideInput, 'fieldA'|'fieldB'> shape for only the used fields)
and type service doubles with Pick types of the actual service interfaces used
by the resolver (e.g., Pick<OnboardingService, 'methodX'|'methodY'> and
Pick<OnboardingInternalBootService, 'bootMethod'>). Update the spec to provide
objects matching those types instead of casting to never so the compiler can
catch constructor/input drift against OnboardingOverrideInput and the real
service interfaces.
In `@web/src/components/Onboarding/store/onboardingStatus.ts`:
- Line 11: The import of the local store should use the repo's ESM .js suffix:
update the import of useOnboardingDraftStore in onboardingStatus.ts to point to
the compiled ESM path (change '~/components/Onboarding/store/onboardingDraft' to
'./onboardingDraft.js' style ESM import) so TypeScript output remains consistent
with .js extensions while keeping the same exported symbol
useOnboardingDraftStore.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c6d65741-9179-44f6-b941-f6bef11bc793
⛔ Files ignored due to path filters (1)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (15)
api/generated-schema.graphqlapi/src/unraid-api/config/api-config.test.tsapi/src/unraid-api/config/onboarding-tracker.service.spec.tsapi/src/unraid-api/config/onboarding-tracker.service.tsapi/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.tsapi/src/unraid-api/graph/resolvers/customization/customization.resolver.tsapi/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.tsapi/src/unraid-api/graph/resolvers/customization/onboarding.service.tsapi/src/unraid-api/graph/resolvers/onboarding/onboarding.mutation.spec.tsapi/src/unraid-api/graph/resolvers/onboarding/onboarding.mutation.tsplugin/source/dynamix.unraid.net/install/doinst.shweb/__test__/store/onboardingStatus.test.tsweb/src/components/Onboarding/store/onboardingDraft.tsweb/src/components/Onboarding/store/onboardingStatus.tsweb/src/composables/gql/graphql.ts
💤 Files with no reviewable changes (1)
- plugin/source/dynamix.unraid.net/install/doinst.sh
✅ Files skipped from review due to trivial changes (1)
- web/src/composables/gql/graphql.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- web/test/store/onboardingStatus.test.ts
- api/src/unraid-api/config/onboarding-tracker.service.spec.ts
- api/src/unraid-api/config/api-config.test.ts
- api/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts
api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts
Outdated
Show resolved
Hide resolved
- Purpose: reduce the onboarding modal's startup data flow to one shared bootstrap query instead of separate onboarding, activation-code, and internal-boot visibility requests. - Before: onboardingStatus, activationCodeData, and OnboardingModal each loaded overlapping onboarding data independently, which complicated loading/error behavior and made the step list timing harder to reason about. - Problem: split queries introduced unnecessary duplication and made modal bootstrap state depend on multiple request lifecycles. - Change: add a shared onboardingContextData store backed by one OnboardingBootstrap query, derive the existing activationCodeData and onboardingStatus stores from it, and read internal-boot visibility from the same source. - How: create a single GraphQL document for customization plus vars, regenerate GraphQL artifacts, remove obsolete query files, and update focused store/modal tests around the new bootstrap path.
- Purpose: restore green type-checking after the onboarding bootstrap query consolidation and resolver test refactor. - Before: pnpm type-check failed on overly narrow test mocks in the web onboarding tests and on API spec values that no longer matched the stricter types. - Problem: the branch looked healthy at runtime but CI would still fail on test-only typing issues. - Change: tighten the onboarding modal/context test mock types and align the API onboarding specs with the actual model types. - How: type currentStepId as StepId | null in the modal test, simplify the shared query store mock setup, replace invalid null/casts in API specs, and verify with pnpm type-check plus focused Vitest runs.
- Purpose: keep the in-memory onboarding draft aligned with storage cleanup and bypass flows. - Before: cleanup paths removed localStorage entries, but the live Pinia draft store still held resumable state in the same tab. - Problem: hasResumableDraft could stay true after cleanup or bypass, which could reopen onboarding from stale in-memory selections. - Change: add an explicit draft reset action and invoke it whenever onboarding draft storage is cleared. - How: reset the draft store inside clearOnboardingDraftStorage, cover the live reset in cleanup tests, and verify bypass clears both persisted and in-memory draft state.
- Purpose: make the onboarding shared-store consumers work with both real Pinia stores and the ref-shaped test doubles used across the broader web suite. - Before: activation code and onboarding status used storeToRefs on the shared context store, which crashed when tests mocked that store as plain refs or plain objects. - Problem: unrelated component tests started failing after the onboarding bootstrap consolidation even though their runtime behavior was otherwise unchanged. - Change: replace those storeToRefs reads with computed unref accessors so the stores consume values consistently regardless of whether the source is a live Pinia store or a mocked ref/object. - Result: the full web test suite, lint fix pass, and web type-check all succeed again without changing onboarding behavior in the app.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
web/__test__/components/Onboarding/onboardingStorageCleanup.test.ts (1)
36-54: SeedcurrentStepIdbefore asserting it was cleared.This setup only mutates
currentStepIndex, so Line 52 is alreadynulleven if the cleanup path forgets to reset the new step-id field. Initialize the draft viasetCurrentStep(...)first so this test actually guards the resume regression. Based on learnings: Test Pinia action side effects and state changes; Set initial state for focused Pinia store testing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/onboardingStorageCleanup.test.ts` around lines 36 - 54, The test doesn't seed currentStepId so the assertion for it being cleared is ineffective; update the setup to initialize the draft's step before calling clearOnboardingDraftStorage by calling useOnboardingDraftStore().setCurrentStep(...) (or the appropriate setter used in the store) to set a non-null currentStepId/step prior to clearOnboardingDraftStorage(), then call clearOnboardingDraftStorage() and assert currentStepId is null along with the existing assertions on hasResumableDraft, currentStepIndex, and coreSettingsInitialized.web/__test__/components/Onboarding/OnboardingModal.test.ts (1)
57-60: Remove the unused double-cast mock field.
onboardingContextDataStoreis not consumed, andnull as unknown as ...adds avoidable type debt.Proposed fix
- onboardingContextDataStore: { - internalBootVisibility: null as unknown as InternalBootVisibilityResult, - loading: { value: false }, - },As per coding guidelines, "Avoid using casting whenever possible, prefer proper typing from the start".
🤖 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 57 - 60, Remove the unused double-cast mock field onboardingContextDataStore (and its value using null as unknown as InternalBootVisibilityResult) from the test mock; if the test truly needs that context later, replace the double-cast with a properly typed value matching InternalBootVisibilityResult instead of using null as unknown as ..., but since onboardingContextDataStore is not consumed here simply delete the onboardingContextDataStore entry from the mock to eliminate the unnecessary type cast.
🤖 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`:
- Line 5: Update the ESM import for the StepId type to include the .js extension
to satisfy TypeScript ESM resolution: modify the import that currently reads
importing StepId from '~/components/Onboarding/stepRegistry' so it points to
'~/components/Onboarding/stepRegistry.js' (refer to the StepId type and the
stepRegistry module/import statement to locate and update the import).
In `@web/src/components/Onboarding/graphql/onboardingBootstrap.query.ts`:
- Line 1: The import in onboardingBootstrap.query.ts uses a bare specifier
without the .js extension; update the import statement that brings in graphql
from '~/composables/gql/gql' to use the TypeScript ESM .js extension (i.e.,
import from '~/composables/gql/gql.js') so the module resolution follows the
repo's TS ESM rule and avoids runtime import issues.
In `@web/src/components/Onboarding/OnboardingModal.vue`:
- Around line 93-98: The guard in the computed shouldKeepResumedInternalBootStep
(and the similar guard around lines 129-135) is using currentStepId directly and
thus ignores the fallback logic used by preferredStepId; update these computed
checks to use the same resolved step id as preferredStepId (i.e., the
resolved/fallback value derived from currentStepId and
HARDCODED_STEPS[currentStepIndex]) instead of currentStepId.value so that when
currentStepId is missing but currentStepIndex points to 'CONFIGURE_BOOT' the
boot step isn't incorrectly filtered; reference preferredStepId,
currentStepIndex, HARDCODED_STEPS and internalBootVisibility when implementing
the replacement.
- Around line 162-173: The watchEffect is writing the fallback OVERVIEW step
back into the draft immediately after cleanup; prevent this by adding a guard
before calling draftStore.setCurrentStep: only sync when the stored draft
already has a saved step or when the currentStep isn't just the fallback (e.g.,
if (currentStepId.value == null && currentStep.value === OVERVIEW) return;),
otherwise proceed to compare currentStepId/currentStepIndex and call
draftStore.setCurrentStep(stepId, stepIndex). This ensures
cleanupOnboardingStorage() can clear the draft without this effect immediately
recreating a resumable state.
In `@web/src/components/Onboarding/store/onboardingDraft.ts`:
- Around line 225-232: The setters can get out of sync: update setStepIndex so
it does not leave a stale currentStepId by routing index-only changes through
setCurrentStep or by clearing/resetting currentStepId; specifically, modify
setStepIndex to either call setCurrentStep(stepIdForIndex, index) (lookup the
stepId for that index) or explicitly set currentStepId.value = undefined/null
when only the index is being changed, keeping currentStepId, currentStepIndex,
setCurrentStep and setStepIndex consistent.
---
Nitpick comments:
In `@web/__test__/components/Onboarding/OnboardingModal.test.ts`:
- Around line 57-60: Remove the unused double-cast mock field
onboardingContextDataStore (and its value using null as unknown as
InternalBootVisibilityResult) from the test mock; if the test truly needs that
context later, replace the double-cast with a properly typed value matching
InternalBootVisibilityResult instead of using null as unknown as ..., but since
onboardingContextDataStore is not consumed here simply delete the
onboardingContextDataStore entry from the mock to eliminate the unnecessary type
cast.
In `@web/__test__/components/Onboarding/onboardingStorageCleanup.test.ts`:
- Around line 36-54: The test doesn't seed currentStepId so the assertion for it
being cleared is ineffective; update the setup to initialize the draft's step
before calling clearOnboardingDraftStorage by calling
useOnboardingDraftStore().setCurrentStep(...) (or the appropriate setter used in
the store) to set a non-null currentStepId/step prior to
clearOnboardingDraftStorage(), then call clearOnboardingDraftStorage() and
assert currentStepId is null along with the existing assertions on
hasResumableDraft, currentStepIndex, and coreSettingsInitialized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c9141d47-1190-4601-99f9-3e623d9ed2c9
📒 Files selected for processing (19)
api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.tsapi/src/unraid-api/graph/resolvers/onboarding/onboarding.mutation.spec.tsweb/__test__/components/Onboarding/OnboardingModal.test.tsweb/__test__/components/Onboarding/onboardingStorageCleanup.test.tsweb/__test__/store/activationCodeData.test.tsweb/__test__/store/onboardingContextData.test.tsweb/__test__/store/onboardingModalVisibility.test.tsweb/__test__/store/onboardingStatus.test.tsweb/src/components/Onboarding/OnboardingModal.vueweb/src/components/Onboarding/graphql/activationOnboarding.query.tsweb/src/components/Onboarding/graphql/getInternalBootStepVisibility.query.tsweb/src/components/Onboarding/graphql/onboardingBootstrap.query.tsweb/src/components/Onboarding/store/activationCodeData.tsweb/src/components/Onboarding/store/onboardingContextData.tsweb/src/components/Onboarding/store/onboardingDraft.tsweb/src/components/Onboarding/store/onboardingStatus.tsweb/src/components/Onboarding/store/onboardingStorageCleanup.tsweb/src/composables/gql/gql.tsweb/src/composables/gql/graphql.ts
💤 Files with no reviewable changes (2)
- web/src/components/Onboarding/graphql/activationOnboarding.query.ts
- web/src/components/Onboarding/graphql/getInternalBootStepVisibility.query.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/Onboarding/store/onboardingStatus.ts
- web/test/store/onboardingStatus.test.ts
web/src/components/Onboarding/graphql/onboardingBootstrap.query.ts
Outdated
Show resolved
Hide resolved
- Purpose: make onboarding obey a single product rule that any bootstrap/API error should prevent the modal from opening. - Before: automatic open and manual force-open were handled through separate gates, which let admin-panel or URL-driven opens slip through even when the bootstrap query was unhealthy. - Problem: that split behavior was harder to reason about and created a stale-cache/manual-open loophole that did not match the intended UX. - Change: collapse the modal gating to one shared onboarding-error rule and use that same gate for force-open paths as well. - Result: automatic open, admin-panel open, and ?onboarding=open all stay blocked whenever the onboarding bootstrap query reports an error, while the code stays simpler and easier to maintain.
- Purpose: address the remaining valid CodeRabbit findings in the onboarding PR. - Before: legacy step-index resumes could lose the boot step during bootstrap, cleanup could immediately re-seed the default OVERVIEW draft, and the API specs still missed the tracker 'missing' branch while using weak 'as never' doubles. - Why: those gaps left a real resume regression in the modal and reduced the review suite's ability to catch future drift. - What: reuse the fallback-aware step id in the modal, keep draft step id/index in sync, add explicit missing-tracker coverage, replace the resolver spec's 'as never' fixtures with typed doubles, and clean up the related test/import nits. - How: route the internal-boot preservation guard through preferredStepId, skip syncing the default OVERVIEW fallback back into storage after cleanup, derive currentStepId from STEP_IDS inside setStepIndex, and tighten the affected specs.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/__test__/store/onboardingStatus.test.ts (1)
117-123: Consider moving mock declarations closer to the top of the file.The mocks work correctly due to Vitest's hoisting behavior, but the coding guidelines recommend placing mock declarations at the top level. Moving these
vi.mock()calls above thedescribeblock would improve readability and align with the guideline pattern.♻️ Suggested reorganization
+vi.mock('~/components/Onboarding/store/onboardingContextData', () => ({ + useOnboardingContextDataStore: vi.fn(), +})); + +vi.mock('~/store/server', () => ({ + useServerStore: vi.fn(), +})); + describe('onboardingStatus store', () => { // ... }); - -vi.mock('~/components/Onboarding/store/onboardingContextData', () => ({ - useOnboardingContextDataStore: vi.fn(), -})); - -vi.mock('~/store/server', () => ({ - useServerStore: vi.fn(), -}));As per coding guidelines: "Place all mock declarations at the top level and use factory functions for module mocks to avoid hoisting issues."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/store/onboardingStatus.test.ts` around lines 117 - 123, Move the two vi.mock() declarations for '~/components/Onboarding/store/onboardingContextData' and '~/store/server' to the top of the test file (above the describe block) and replace them with top-level factory-style mocks that return mocked implementations for useOnboardingContextDataStore and useServerStore; ensure the mocks are declared before any imports that rely on them so they aren’t affected by hoisting and keep the mocked symbols (useOnboardingContextDataStore, useServerStore) intact for the tests to reference.api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts (1)
328-336: Avoid pinning the failure-path test to a literal message.Line 334 makes this spec fail on harmless wording changes even if the tracker-unavailable behavior is still correct. Prefer asserting that it rejects, or asserting on the exception type/status instead of the exact text.
Based on learnings: "Applies to **/*.test.ts : Test what the code does, not implementation details like exact error message wording"
🤖 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 328 - 336, The test currently pins the failure to an exact error message; update the assertion on service.getOnboardingResponse() so it does not depend on literal wording. Replace the expect(...).rejects.toThrow('Onboarding tracker state is unavailable.') check with a non-message-specific assertion such as expect(...).rejects.toThrow() or expect(...).rejects.toBeInstanceOf(Error) (keeping the existing onboardingTrackerMock.getStateResult setup) so the spec verifies failure behavior without coupling to message text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts`:
- Around line 328-336: The test currently pins the failure to an exact error
message; update the assertion on service.getOnboardingResponse() so it does not
depend on literal wording. Replace the expect(...).rejects.toThrow('Onboarding
tracker state is unavailable.') check with a non-message-specific assertion such
as expect(...).rejects.toThrow() or expect(...).rejects.toBeInstanceOf(Error)
(keeping the existing onboardingTrackerMock.getStateResult setup) so the spec
verifies failure behavior without coupling to message text.
In `@web/__test__/store/onboardingStatus.test.ts`:
- Around line 117-123: Move the two vi.mock() declarations for
'~/components/Onboarding/store/onboardingContextData' and '~/store/server' to
the top of the test file (above the describe block) and replace them with
top-level factory-style mocks that return mocked implementations for
useOnboardingContextDataStore and useServerStore; ensure the mocks are declared
before any imports that rely on them so they aren’t affected by hoisting and
keep the mocked symbols (useOnboardingContextDataStore, useServerStore) intact
for the tests to reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d3011b9-5411-4958-a85b-5f3dc906091e
📒 Files selected for processing (11)
api/src/unraid-api/graph/resolvers/customization/onboarding.service.spec.tsapi/src/unraid-api/graph/resolvers/onboarding/onboarding.mutation.spec.tsweb/__test__/components/Onboarding/OnboardingModal.test.tsweb/__test__/components/Onboarding/onboardingStorageCleanup.test.tsweb/__test__/store/onboardingModalVisibility.test.tsweb/__test__/store/onboardingStatus.test.tsweb/src/components/Onboarding/OnboardingModal.vueweb/src/components/Onboarding/graphql/onboardingBootstrap.query.tsweb/src/components/Onboarding/store/onboardingDraft.tsweb/src/components/Onboarding/store/onboardingModalVisibility.tsweb/src/components/Onboarding/store/onboardingStatus.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/test/components/Onboarding/onboardingStorageCleanup.test.ts
- Purpose: simplify onboarding draft resume behavior for the new feature without carrying backward-compatibility logic. - Before: the modal could restore from currentStepIndex when currentStepId was missing, which required extra guards to stop cleanup from recreating a fake OVERVIEW draft. - Why: index-based fallback made the modal state machine harder to reason about and reintroduced stale draft state after cleanup. - What: make currentStepId the only persisted resume key, treat currentStepIndex as supporting metadata only, remove the index-only setter, and update tests to seed explicit step ids. - How: stop deriving resume state from bare indexes in the modal/store, only sync draft steps when a real currentStepId exists, and tighten the affected web tests and API spec helper typing.
- Purpose: remove the last implicit resume remap based on persisted step indexes in the onboarding modal. - Before: when a saved currentStepId was no longer visible, the modal silently fell back to currentStepIndex, which could map a draft to an unrelated conditional step. - Why: that split the resume source of truth across two keys and made conditional-step changes harder to reason about for a new feature. - What: keep currentStepId authoritative and resolve invalid saved steps with an explicit step-order policy instead of reusing the stored index. - How: add a nearest-visible-step helper based on the hardcoded step order and use it whenever a persisted step id is filtered out of the current available step list.
- Purpose: prevent onboarding cleanup from leaving an all-default persisted draft entry in localStorage after reboot/bypass flows. - Before: resetDraft() cleared the live state, but the persisted-state plugin could immediately write the default empty store back out, so users still saw an onboardingDraft key even though nothing was resumable. - Why that was a problem: it looked like cleanup had failed and made the reboot/internal-boot flow harder to reason about even though runtime behavior was technically correct. - What this change accomplishes: meaningful drafts still persist exactly as before, but the all-default empty shell is removed instead of being re-saved. - How it works: the store now uses a custom persisted-state storage wrapper that removes the key when the serialized draft has no resumable step, initialized section, plugin selection, or internal-boot selection, and a focused test covers that policy.
This reverts commit 15b1758.
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
elibosley
pushed a commit
that referenced
this pull request
Mar 18, 2026
🤖 I have created a release *beep* *boop* --- ## [4.30.0](v4.29.2...v4.30.0) (2026-03-18) ### Features * add internal boot step to onboarding flow ([#1881](#1881)) ([337aecc](337aecc)) * Add TPM licensing availability to registration ([#1908](#1908)) ([aa162eb](aa162eb)) * add UPS power ([#1874](#1874)) ([b531aed](b531aed)) * **api:** alert when usb boot has internal boot target ([#1898](#1898)) ([b94df47](b94df47)) * **api:** expose internal boot devices in array GraphQL ([#1894](#1894)) ([0736709](0736709)) * docker overview ([#1855](#1855)) ([9ef1cf1](9ef1cf1)) * **docker:** add update actions to container context menu ([#1867](#1867)) ([4ca3e06](4ca3e06)) * **docker:** disable containers page file modification ([#1870](#1870)) ([aaa0372](aaa0372)) * issues/1597: Temperature Monitoring - Thanks @MitchellThompkins ([a1be458](a1be458)) * New Crowdin updates ([#1809](#1809)) ([a7b3f07](a7b3f07)) * New Crowdin updates ([#1883](#1883)) ([14a8fa8](14a8fa8)) * **onboarding:** add new onboarding flows for Unraid OS ([#1746](#1746)) ([15bd747](15bd747)) * registration and trial actions use Account app ([#1928](#1928)) ([c2c0425](c2c0425)) * share internal boot state ([#1921](#1921)) ([8e4d44d](8e4d44d)) * **web:** show TPM move control for trial licenses ([#1911](#1911)) ([d00fb63](d00fb63)) ### Bug Fixes * Add dedicated TPM license move option ([#1909](#1909)) ([36c56f7](36c56f7)) * allow free USB targets in onboarding internal boot setup ([#1903](#1903)) ([298da54](298da54)) * API key key display truncation ([#1890](#1890)) ([b12f75c](b12f75c)) * **api:** harden PHP wrapper args for newer PHP versions ([#1901](#1901)) ([849f177](849f177)) * **api:** prevent flash notification startup fd exhaustion ([#1893](#1893)) ([4b231ad](4b231ad)) * clear stale onboarding modal session state ([#1904](#1904)) ([23f7836](23f7836)) * consistently clear onboarding draft ([#1916](#1916)) ([199d803](199d803)) * correct graphql-api.log timestamp formatting ([#1918](#1918)) ([243c5a8](243c5a8)) * **deps:** pin dependencies ([#1878](#1878)) ([db88eb8](db88eb8)) * **docker:** change "visit" to "webui" & use correct link ([#1863](#1863)) ([cab0880](cab0880)) * **docker:** improve start/stop UX with visual feedback ([#1865](#1865)) ([c084e25](c084e25)) * **docker:** remove aggressive caching to ensure data correctness ([#1864](#1864)) ([1c1bae8](1c1bae8)) * **docker:** sync template mappings in organizer to prevent false orphan warnings ([#1866](#1866)) ([38a6f0c](38a6f0c)) * onboarding internal-boot warning panel contrast and semantics ([#1927](#1927)) ([bb6f241](bb6f241)) * **onboarding:** add explicit EFI loader path for flash entry ([#1926](#1926)) ([429b438](429b438)) * **onboarding:** extend onboarding refresh timeout ([#1925](#1925)) ([e2a5f44](e2a5f44)) * **onboarding:** persist installed plugins in summary ([#1915](#1915)) ([07f4ebd](07f4ebd)) * **onboarding:** refine storage boot setup UX ([#1900](#1900)) ([1108d0a](1108d0a)) * polish onboarding flow ([#1902](#1902)) ([8742cac](8742cac)) * preserve registration device limits after refresh ([#1905](#1905)) ([234bfc7](234bfc7)) * prevent onboarding on API errors ([#1917](#1917)) ([540d6f9](540d6f9)) * remap TPM guid prefix to 01 ([#1924](#1924)) ([5360b5b](5360b5b)) * Return null for corrupted/invalid API key files and add Connect fixtures test ([#1886](#1886)) ([013e6c5](013e6c5)) * share internal boot state across onboarding ([#1920](#1920)) ([f9b293f](f9b293f)) * too many file descriptors with thousands of notifications ([#1887](#1887)) ([7956987](7956987)) * Treat onboarding patch updates as completed ([#1884](#1884)) ([d03b25e](d03b25e)) * unify onboarding internal boot state refresh ([#1923](#1923)) ([d3032c1](d3032c1)) * **web:** refresh internal boot onboarding state ([#1913](#1913)) ([1ca2129](1ca2129)) * **web:** stop showing callback errors after successful key installs ([#1892](#1892)) ([45f1402](45f1402)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR hardens onboarding around API/restart/error windows and fixes a related resume-flow regression.
The main goal is to make onboarding behave safely when we cannot reliably read onboarding state, without breaking legitimate resume flows:
It also includes the small plugin install-script cleanup already present on the branch and the generated GraphQL/schema updates needed by the onboarding query consolidation.
Problem
There were a few overlapping issues in the onboarding flow:
API-side onboarding state handling flattened too many tracker outcomes together.
A missing
onboarding-tracker.jsonfile, a real tracker read failure, and a valid tracker state were not being represented clearly enough for the web client to make safe decisions.Web-side modal gating treated onboarding errors too bluntly.
We wanted the modal to disappear when onboarding state was genuinely unavailable, but not disappear in legitimate resume situations such as summary-step settings changes or API restart windows.
Resume flow depended on a persisted numeric step index.
Because the onboarding step list is conditional, persisting
currentStepIndexalone meant a resumed draft could be reinterpreted against a temporarily shorter step list while the internal-boot visibility data was still loading.Bootstrap data for the onboarding modal was split across multiple requests.
onboardingStatus,activationCodeData, and the modal itself were each making overlapping onboarding-related queries, which made loading/error behavior harder to reason about and review.Draft cleanup only cleared storage, not the live Pinia store.
After introducing live
hasResumableDraftbehavior, clearing or bypassing onboarding could leave stale in-memory draft state in the current tab.Behavior Changes
API onboarding state
onboarding-tracker.jsonis treated as valid empty onboarding state.OnboardingService, so customization queries and onboarding mutations now compute tracker/error/version behavior consistently.Modal display / gating
Resume flow
currentStepIdinstead of relying only oncurrentStepIndex.CONFIGURE_BOOTdraft is kept visible while internal-boot visibility is still loading, preventing temporary jumps to plugins or blank modal content.Bootstrap data flow
activationCodeDataandonboardingStatusnow derive from shared onboarding context data rather than issuing overlapping startup requests.Draft cleanup
Implementation Notes
API
getStateResult()and preserved the distinction between missing tracker state and true tracker failures.OnboardingService.Web
onboardingContextDatastore backed by a singleOnboardingBootstrapquery.activationCodeData,onboardingStatus, andOnboardingModalto read from that shared context.Review / cleanup follow-ups
pnpm type-check.corepack/npm/npxsymlink block from the plugin install script.Files / Areas Touched
Validation
Monorepo
pnpm type-checkAPI
pnpm --dir /Users/ajitmehrotra/Projects/wt-onboarding-error-fallback/api exec vitest run src/unraid-api/config/onboarding-tracker.service.spec.ts src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts src/unraid-api/graph/resolvers/customization/onboarding.service.spec.ts src/unraid-api/graph/resolvers/onboarding/onboarding.mutation.spec.ts src/unraid-api/config/api-config.test.tsWeb
pnpm --dir /Users/ajitmehrotra/Projects/wt-onboarding-error-fallback/web codegenpnpm --dir /Users/ajitmehrotra/Projects/wt-onboarding-error-fallback/web exec vitest run __test__/store/onboardingContextData.test.ts __test__/store/activationCodeData.test.ts __test__/store/onboardingStatus.test.ts __test__/components/Onboarding/OnboardingModal.test.tspnpm --dir /Users/ajitmehrotra/Projects/wt-onboarding-error-fallback/web exec vitest run __test__/components/Onboarding/onboardingStorageCleanup.test.ts __test__/store/onboardingModalVisibility.test.tspnpm --dir /Users/ajitmehrotra/Projects/wt-onboarding-error-fallback/web exec vitest run __test__/store/onboardingStatus.test.ts __test__/components/Onboarding/OnboardingModal.test.tsPlugin
sh -n plugin/source/dynamix.unraid.net/install/doinst.shNotes For Reviewers
Summary by CodeRabbit
New Features
Bug Fixes
Refactor