fix: hide onboarding wizard when tracker state is unavailable#1906
Open
Ajit-Mehrotra wants to merge 1 commit intomainfrom
Open
fix: hide onboarding wizard when tracker state is unavailable#1906Ajit-Mehrotra wants to merge 1 commit intomainfrom
Ajit-Mehrotra wants to merge 1 commit intomainfrom
Conversation
- Purpose: prevent the onboarding wizard from appearing when the Unraid API is offline or the onboarding tracker cannot be read reliably. - Before: tracker read failures were treated like an empty tracker state, so the API could report onboarding as incomplete and the web modal could still auto-open. - Why that was a problem: users could be dropped into onboarding during degraded startup or API failure states, which made the flow noisy and misleading. - What changed: the tracker service now distinguishes a missing tracker file from a real read failure, the customization resolver surfaces tracker-read failure as an onboarding query error, and the web onboarding gate suppresses the modal while the query is loading or errored. - How it works: new tracker-health tests cover missing-vs-failed reads, resolver tests cover the error path, and a new onboarding status store test verifies the modal stays hidden for loading and error states.
Contributor
WalkthroughThe changes introduce error state tracking for the onboarding tracker across the API backend and web frontend. The API service now tracks read failures and exposes an accessor method. The GraphQL resolver guards against unavailable tracker state. The web store adds error handling logic and exposes error state to the UI. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Resolver as GraphQL Resolver
participant Service as OnboardingTrackerService
participant Store as Onboarding Store
participant UI as User Interface
Client->>Resolver: resolveOnboarding()
Resolver->>Service: didTrackerStateReadFail()
alt Tracker Read Failed
Service-->>Resolver: true
Resolver-->>Client: GraphQLError<br/>(unavailable state)
Client->>Store: onboarding query error
Store-->>UI: hasOnboardingQueryError = true
UI->>UI: canDisplayOnboardingModal = false
else Tracker Read Success
Service-->>Resolver: false
Resolver->>Service: getState()
Service-->>Resolver: tracker state
Resolver-->>Client: onboarding data
Client->>Store: success result
Store-->>UI: hasOnboardingQueryError = false
UI->>UI: canDisplayOnboardingModal = true
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 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 |
![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
🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts (1)
67-74: Consider removing exact error message from assertion.Per coding guidelines, prefer
.rejects.toThrow()without arguments unless you're specifically testing the error message format. This avoids brittle tests that break when error messages change.♻️ Proposed change
await expect(resolver.resolveOnboarding()).rejects.toThrow( - 'Onboarding tracker state is unavailable.' );The assertion that
getStatewas not called (line 73) is a good verification that the guard short-circuits correctly.🤖 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 67 - 74, The test currently asserts a specific error message when calling resolver.resolveOnboarding() which makes the test brittle; change the assertion to use await expect(resolver.resolveOnboarding()).rejects.toThrow() with no argument so it only checks that an error is thrown, keeping the existing mocked onboardingTracker.didTrackerStateReadFail and the verification that onboardingTracker.getState was not called to ensure the guard short-circuits.web/__test__/store/onboardingStatus.test.ts (1)
93-99: Consider movingvi.mock()declarations to top of file.Per coding guidelines, mock declarations should be placed at the top level of the file. While Vitest hoists
vi.mock()calls automatically, placing them at the top improves readability and follows project conventions.♻️ Suggested reorganization
Move lines 93-99 to immediately after the imports (after line 7):
import { useOnboardingStore } from '~/components/Onboarding/store/onboardingStatus'; import { useServerStore } from '~/store/server'; vi.mock('@vue/apollo-composable', () => ({ useQuery: () => useQueryMock(), })); vi.mock('~/store/server', () => ({ useServerStore: vi.fn(), })); type OnboardingQueryResult = { // ...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 93 - 99, Move the vi.mock declarations for '@vue/apollo-composable' and '~/store/server' to the top-level of the test file immediately after the import block (so the mocks are declared before any test code/execution); ensure you keep the factory functions as currently written (the mock using useQueryMock for useQuery and the vi.fn() for useServerStore) so the symbols vi.mock, useQueryMock, useQuery, and useServerStore are preserved and hoisted correctly and follow project conventions.
🤖 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/config/onboarding-tracker.service.ts`:
- Around line 178-188: The multi-line conditional and template string cause
Prettier failures; collapse the condition and the log call into single lines.
Specifically, in the method containing trackerStateReadFailed, change the if
check to a single-line form like: if (error instanceof Error && 'code' in error
&& error.code === 'ENOENT') { ... } and make the logger.debug call a single-line
statement that references this.trackerPath. Keep the assignment
this.trackerStateReadFailed = false; and return undefined; inside the same
block.
---
Nitpick comments:
In
`@api/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts`:
- Around line 67-74: The test currently asserts a specific error message when
calling resolver.resolveOnboarding() which makes the test brittle; change the
assertion to use await expect(resolver.resolveOnboarding()).rejects.toThrow()
with no argument so it only checks that an error is thrown, keeping the existing
mocked onboardingTracker.didTrackerStateReadFail and the verification that
onboardingTracker.getState was not called to ensure the guard short-circuits.
In `@web/__test__/store/onboardingStatus.test.ts`:
- Around line 93-99: Move the vi.mock declarations for '@vue/apollo-composable'
and '~/store/server' to the top-level of the test file immediately after the
import block (so the mocks are declared before any test code/execution); ensure
you keep the factory functions as currently written (the mock using useQueryMock
for useQuery and the vi.fn() for useServerStore) so the symbols vi.mock,
useQueryMock, useQuery, and useServerStore are preserved and hoisted correctly
and follow project conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2c0eb7d3-2e8d-4573-8a3e-b85e8683a225
📒 Files selected for processing (6)
api/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.tsweb/__test__/store/onboardingStatus.test.tsweb/src/components/Onboarding/store/onboardingStatus.ts
Comment on lines
+178
to
+188
| if ( | ||
| error instanceof Error && | ||
| 'code' in error && | ||
| error.code === 'ENOENT' | ||
| ) { | ||
| this.trackerStateReadFailed = false; | ||
| this.logger.debug( | ||
| `Onboarding tracker state does not exist yet at ${this.trackerPath}.` | ||
| ); | ||
| return undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
Fix Prettier formatting violations (CI failure).
The CI pipeline reports Prettier formatting issues. The multiline condition and string should be formatted on single lines.
🔧 Proposed fix for formatting
} catch (error) {
- if (
- error instanceof Error &&
- 'code' in error &&
- error.code === 'ENOENT'
- ) {
+ if (error instanceof Error && 'code' in error && error.code === 'ENOENT') {
this.trackerStateReadFailed = false;
- this.logger.debug(
- `Onboarding tracker state does not exist yet at ${this.trackerPath}.`
- );
+ this.logger.debug(`Onboarding tracker state does not exist yet at ${this.trackerPath}.`);
return undefined;
}🧰 Tools
🪛 GitHub Actions: CI - Main (API)
[error] 178-178: Prettier formatting issue detected by prettier/prettier. Replace multiline condition with single-line: error instanceof Error && 'code' in error && error.code === 'ENOENT'.
🪛 GitHub Check: Test API
[failure] 184-184:
Replace ⏎····················Onboarding·tracker·state·does·not·exist·yet·at·${this.trackerPath}.⏎················ with Onboarding·tracker·state·does·not·exist·yet·at·${this.trackerPath}.
[failure] 178-178:
Replace ⏎················error·instanceof·Error·&&⏎················'code'·in·error·&&⏎················error.code·===·'ENOENT'⏎············ with error·instanceof·Error·&&·'code'·in·error·&&·error.code·===·'ENOENT'
🤖 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 178 -
188, The multi-line conditional and template string cause Prettier failures;
collapse the condition and the log call into single lines. Specifically, in the
method containing trackerStateReadFailed, change the if check to a single-line
form like: if (error instanceof Error && 'code' in error && error.code ===
'ENOENT') { ... } and make the logger.debug call a single-line statement that
references this.trackerPath. Keep the assignment this.trackerStateReadFailed =
false; and return undefined; inside the same block.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This changes onboarding visibility so the wizard does not appear when the Unraid API is unavailable or when onboarding tracker state cannot be read reliably.
Problem
Before this change, onboarding tracker read failures were treated the same as an empty tracker file. That meant the API could report onboarding as
INCOMPLETE, and the web app could still auto-open the onboarding wizard even though the backend was in a degraded state.That showed up in two bad cases:
onboarding-tracker.jsonfailed for a reason other than the file simply not existing yetIn both cases, the safer behavior is to suppress the wizard entirely.
What changed
API
OnboardingTrackerServicenow tracks whether readingonboarding-tracker.jsonfailed.ENOENT) is still treated as a normal first-run case.CustomizationResolver.resolveOnboarding()now throws a GraphQL error when tracker state is unavailable instead of reporting an incomplete onboarding state.Web
useOnboardingStore()now blocks onboarding modal display when the onboarding query is still loading.useOnboardingStore()also blocks onboarding modal display when the onboarding query has errored.Behavior after this change
onboarding-tracker.jsonread failure: onboarding wizard does not render.Tests
Targeted verification run in this branch: