Skip to content

fix: hide onboarding wizard when tracker state is unavailable#1906

Open
Ajit-Mehrotra wants to merge 4 commits intomainfrom
codex/onboarding-offline-gate
Open

fix: hide onboarding wizard when tracker state is unavailable#1906
Ajit-Mehrotra wants to merge 4 commits intomainfrom
codex/onboarding-offline-gate

Conversation

@Ajit-Mehrotra
Copy link
Contributor

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

Summary

This PR makes onboarding fail closed when backend onboarding state is unavailable, while preserving the normal first-run experience and fixing a modal regression introduced by the new gate.

In practice:

  • the onboarding wizard no longer auto-opens when the Unraid API is offline
  • the onboarding wizard no longer auto-opens when onboarding-tracker.json cannot be read reliably
  • a missing tracker file (ENOENT) is still treated as a normal first-run case
  • a transient tracker read failure gets one bounded retry before onboarding is suppressed
  • the onboarding modal now stays mounted during the post-apply refetch so the completion/result popup can still appear

Problem

Before this change, the API treated these two cases the same:

  • the tracker file does not exist yet
  • the tracker file read failed for some other reason

That meant degraded tracker reads could be reported to the web app as a normal INCOMPLETE onboarding state. On the frontend, that allowed the onboarding wizard to auto-open even when the backend onboarding state was not trustworthy.

That was especially problematic in two cases:

  • the Unraid API was offline and the onboarding query errored
  • onboarding-tracker.json failed to read because of a real filesystem/runtime problem

While addressing that, the stricter frontend gate also exposed a regression: after Confirm & Apply, the summary step refetched onboarding state, briefly toggled the onboarding query back into a loading state, and the modal could unmount before the result popup rendered.

What changed

API

  • OnboardingTrackerService now tracks whether the tracker state read failed for a real reason.
  • ENOENT is still treated as the expected "tracker not created yet" case.
  • Non-ENOENT read failures now mark tracker state as unavailable.
  • CustomizationResolver.resolveOnboarding() now refuses to return onboarding state when tracker state is unavailable.
  • Before throwing, the resolver gives the tracker service one bounded refresh attempt so transient startup failures can recover.
  • If that one retry succeeds, in-memory tracker state is refreshed and onboarding proceeds normally.
  • If it still fails, the resolver throws and the frontend suppresses the wizard.

Web

  • useOnboardingStore() now blocks the initial onboarding modal open when the onboarding query is still loading and no onboarding state has been loaded yet.
  • useOnboardingStore() also blocks the initial modal open when the onboarding query has errored.
  • The store now preserves modal eligibility during refetches if onboarding state is already present, which fixes the post-apply popup regression.
  • This means we still fail closed on the first load, but we do not tear down an already-open modal during a later refetch.

Generated and support files

  • Regenerated GraphQL/codegen outputs to reflect the updated resolver behavior.
  • Cleaned up duplicated corepack / npm / npx symlink blocks in plugin/source/dynamix.unraid.net/install/doinst.sh.

Behavior after this change

  • Fresh install with no tracker file: onboarding still works normally.
  • API offline: onboarding wizard does not auto-open.
  • Persistent tracker read failure: onboarding wizard does not auto-open.
  • Transient tracker read failure during bootstrap: onboarding gets one retry and can recover without requiring a restart.
  • Applying onboarding changes: the log flow can complete and the result popup still renders after the refetch.

Non-goals / compatibility

  • No onboarding-tracker.json schema change.
  • No new REST endpoint.
  • No new GraphQL query, mutation, or field.
  • Existing healthy onboarding behavior is unchanged outside the new degraded-state gate.

Tests

Targeted verification run for this branch:

cd api
pnpm exec vitest run src/unraid-api/config/onboarding-tracker.service.spec.ts src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts

cd ../web
pnpm exec vitest run __test__/store/onboardingStatus.test.ts __test__/components/Onboarding/OnboardingModal.test.ts __test__/components/Onboarding/OnboardingSummaryStep.test.ts

cd ../plugin/source/dynamix.unraid.net/install
bash -n doinst.sh

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 12, 2026

Walkthrough

Adds tracker-read-failure tracking to the onboarding tracker service, makes the GraphQL resolver abort with an error when tracker state is unavailable, and surfaces onboarding query error state in the frontend store with corresponding tests and small packaging/schema formatting changes.

Changes

Cohort / File(s) Summary
Backend Tracker Service
api/src/unraid-api/config/onboarding-tracker.service.ts, api/src/unraid-api/config/onboarding-tracker.service.spec.ts
Adds private trackerStateReadFailed flag and public didTrackerStateReadFail() getter; treats ENOENT as non-fatal and marks other read errors as failures. Tests added for tracker-state-available and tracker-state-unavailable scenarios.
GraphQL Resolver
api/src/unraid-api/graph/resolvers/customization/customization.resolver.ts, api/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts
Imports GraphQLError and throws GraphQLError('Onboarding tracker state is unavailable.') when didTrackerStateReadFail() is true. Tests updated to mock the new hook and verify getState is not called on failure.
Frontend Store & Tests
web/src/components/Onboarding/store/onboardingStatus.ts, web/__test__/store/onboardingStatus.test.ts
Adds hasOnboardingQueryError and hasOnboardingState computed values; updates canDisplayOnboardingModal logic to consider query errors. New tests cover loading, error, and success onboarding query states.
Misc / Packaging & Schema
plugin/source/dynamix.unraid.net/install/doinst.sh, api/generated-schema.graphql, web/src/composables/gql/graphql.ts
Adds symlink recreation commands for corepack/npm/npx in installer script; trivial EOF/formatting changes to generated schema and a newline tweak in GraphQL document.

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant Resolver as GraphQL Resolver
    participant Service as OnboardingTrackerService
    participant Store as Frontend Store
    participant UI as User Interface

    Client->>Resolver: query resolveOnboarding()
    Resolver->>Service: didTrackerStateReadFail()
    alt tracker read failed
        Service-->>Resolver: true
        Resolver-->>Client: throw GraphQLError("Onboarding tracker state is unavailable.")
        Client->>Store: receives error
        Store-->>UI: hasOnboardingQueryError = true
        UI-->>UI: canDisplayOnboardingModal = false
    else tracker read OK
        Service-->>Resolver: false
        Resolver->>Service: getState()
        Service-->>Resolver: tracker state
        Resolver-->>Client: onboarding data
        Client->>Store: receives data
        Store-->>UI: hasOnboardingQueryError = false
        UI-->>UI: canDisplayOnboardingModal = true
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I peeked into the tracker’s den,
If reads go dark I sound the then.
Resolver stops where faults may creep,
The store keeps watch while UIs sleep,
A tiny hop — all safe again.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: hide onboarding wizard when tracker state is unavailable' accurately and concisely describes the main change: preventing the onboarding wizard from appearing when tracker state cannot be read. It is clear, specific, and directly reflects the primary objective of the pull request.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/onboarding-offline-gate
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

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: 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 getState was 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 moving vi.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

📥 Commits

Reviewing files that changed from the base of the PR and between 23f7836 and 8a81962.

📒 Files selected for processing (6)
  • api/src/unraid-api/config/onboarding-tracker.service.spec.ts
  • api/src/unraid-api/config/onboarding-tracker.service.ts
  • api/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts
  • api/src/unraid-api/graph/resolvers/customization/customization.resolver.ts
  • web/__test__/store/onboardingStatus.test.ts
  • web/src/components/Onboarding/store/onboardingStatus.ts

@codecov
Copy link

codecov bot commented Mar 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.01%. Comparing base (23f7836) to head (7dd4ecf).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1906      +/-   ##
==========================================
+ Coverage   50.81%   51.01%   +0.19%     
==========================================
  Files        1023     1023              
  Lines       70545    70569      +24     
  Branches     7652     7690      +38     
==========================================
+ Hits        35848    36000     +152     
+ Misses      34573    34447     -126     
+ Partials      124      122       -2     

☔ 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: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
plugin/source/dynamix.unraid.net/install/doinst.sh (1)

28-45: ⚠️ Potential issue | 🟠 Major

Triplicated symlink operations - appears to be unintended auto-generated artifact accumulation.

Lines 28-33, 34-39, and 40-45 all perform the identical rm -rf and ln -sf operations for corepack, npm, and npx. While these operations are idempotent, the triplication is unnecessary and suggests the makepkg auto-generation is appending rather than replacing.

Consider removing the duplicate blocks (keeping only one set of these operations).

Proposed fix
 # auto-generated actions from makepkg:
 ( 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 )
-( 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 )
-( 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 28 - 45, The
doinst.sh contains three identical blocks that repeatedly remove and symlink
corepack, npm, and npx (the sequences of "( cd usr/local/bin ; rm -rf corepack
)" etc.), which are duplicated; edit doinst.sh to keep only a single set of
these rm -rf + ln -sf operations for corepack, npm, and npx (remove the two
extra duplicate blocks), preserving the same command order and paths so the
script remains idempotent and has no repeated lines.
🧹 Nitpick comments (1)
plugin/source/dynamix.unraid.net/install/doinst.sh (1)

40-45: Optional: Add || exit to cd commands for robustness.

Shellcheck flags the subshell cd commands. Since the existing code (lines 28-39) follows the same pattern, this is a pre-existing style issue. If you choose to address it, apply consistently across all similar lines.

Example pattern
-( cd usr/local/bin ; rm -rf corepack )
+( cd usr/local/bin || exit ; rm -rf corepack )
🤖 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, The
subshell cd invocations like "( cd usr/local/bin ; rm -rf corepack )" can fail
silently; update each subshell that uses "cd usr/local/bin" (the lines
creating/removing symlinks for corepack, npm, npx and any other similar blocks)
to guard the cd with "|| exit" (e.g., "cd usr/local/bin || exit") so the
subshell exits on failure, and apply this same change consistently across all
matching lines in doinst.sh.
🤖 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 88-90: didTrackerStateReadFail currently returns a cached
trackerStateReadFailed flag which can remain stale after transient disk/read
errors; modify the logic so a fresh read is attempted before enforcing the
resolver gate: add a retry/refresh path (e.g., implement or call an existing
loadTrackerState/fetchTrackerState method) from didTrackerStateReadFail (or
expose a new refreshTrackerState method) that re-reads the bootstrap/tracker
state and updates trackerStateReadFailed accordingly, and update the resolver
gate in customization.resolver.ts to call this refresh path before blocking;
make sure the retry is bounded (single quick retry or configurable attempts) and
any errors update the flag and are logged.

---

Outside diff comments:
In `@plugin/source/dynamix.unraid.net/install/doinst.sh`:
- Around line 28-45: The doinst.sh contains three identical blocks that
repeatedly remove and symlink corepack, npm, and npx (the sequences of "( cd
usr/local/bin ; rm -rf corepack )" etc.), which are duplicated; edit doinst.sh
to keep only a single set of these rm -rf + ln -sf operations for corepack, npm,
and npx (remove the two extra duplicate blocks), preserving the same command
order and paths so the script remains idempotent and has no repeated lines.

---

Nitpick comments:
In `@plugin/source/dynamix.unraid.net/install/doinst.sh`:
- Around line 40-45: The subshell cd invocations like "( cd usr/local/bin ; rm
-rf corepack )" can fail silently; update each subshell that uses "cd
usr/local/bin" (the lines creating/removing symlinks for corepack, npm, npx and
any other similar blocks) to guard the cd with "|| exit" (e.g., "cd
usr/local/bin || exit") so the subshell exits on failure, and apply this same
change consistently across all matching lines in doinst.sh.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ae3b7382-6a1a-46b9-8730-d233bcecab3c

📥 Commits

Reviewing files that changed from the base of the PR and between 8a81962 and 4d8d525.

⛔ Files ignored due to path filters (1)
  • api/src/unraid-api/cli/generated/graphql.ts is excluded by !**/generated/**
📒 Files selected for processing (5)
  • api/generated-schema.graphql
  • api/src/unraid-api/config/onboarding-tracker.service.ts
  • api/src/unraid-api/graph/resolvers/customization/customization.resolver.ts
  • plugin/source/dynamix.unraid.net/install/doinst.sh
  • web/src/composables/gql/graphql.ts
✅ Files skipped from review due to trivial changes (2)
  • api/generated-schema.graphql
  • web/src/composables/gql/graphql.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • api/src/unraid-api/graph/resolvers/customization/customization.resolver.ts

Ajit-Mehrotra added a commit that referenced this pull request Mar 13, 2026
- Purpose: resolve the valid CodeRabbit comments on PR #1906 without taking unrelated or low-value suggestions.
- Before: onboarding could remain blocked after a transient tracker read failure, one resolver spec used a brittle exact error-message assertion, the new onboarding status test had mock declarations below the test body, and doinst.sh contained duplicated symlink blocks.
- Why that was a problem: the tracker-read gate could stay stale after recovery, the spec was more fragile than needed, the test file missed local mock-placement conventions, and the plugin install script had repeated generated commands that added noise and risked future confusion.
- What changed: added a bounded tracker refresh before enforcing the onboarding gate, updated the resolver tests to cover refresh behavior and use .rejects.toThrow() without an exact message, moved the web test mocks to top-level, and collapsed the duplicated corepack/npm/npx symlink block to a single copy.
- How it works: focused API vitest coverage confirms the retry path and guard behavior, focused web vitest coverage confirms the store/modal tests still pass, and bash -n confirms the trimmed install script still parses cleanly.
- 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.
- Purpose: capture the follow-up formatting, generated artifacts, and install-script updates after the onboarding gating change.
- Before: the branch had uncommitted lint-fix/codegen output and the plugin install script update was not yet recorded.
- Why that was a problem: reviewers would not see the final generated API/web types or the latest plugin install behavior reflected in the branch history.
- What changed: committed the lint-driven formatting updates in the onboarding files, regenerated GraphQL schema/types for API and web, and included the install script changes that add the corepack symlink and refresh npm/npx links.
- How it works: this is a follow-up housekeeping commit only; it does not introduce a new onboarding behavior change beyond keeping generated artifacts and packaging scripts in sync with the branch state.
- Purpose: capture the final lint-driven cleanup after the onboarding follow-up commits.
- Before: the generated API and web barrel files still had the previous codegen formatting and missing EOF newline.
- Why that was a problem: the branch was left with a small staged diff after linting, which would keep the PR out of sync with the checked-in formatting.
- What changed: committed the normalized export formatting in the generated barrel files for API and web.
- How it works: this is a no-behavior-change cleanup commit that only aligns generated files with the latest lint output.
Ajit-Mehrotra added a commit that referenced this pull request Mar 13, 2026
- Purpose: resolve the valid CodeRabbit comments on PR #1906 without taking unrelated or low-value suggestions.
- Before: onboarding could remain blocked after a transient tracker read failure, one resolver spec used a brittle exact error-message assertion, the new onboarding status test had mock declarations below the test body, and doinst.sh contained duplicated symlink blocks.
- Why that was a problem: the tracker-read gate could stay stale after recovery, the spec was more fragile than needed, the test file missed local mock-placement conventions, and the plugin install script had repeated generated commands that added noise and risked future confusion.
- What changed: added a bounded tracker refresh before enforcing the onboarding gate, updated the resolver tests to cover refresh behavior and use .rejects.toThrow() without an exact message, moved the web test mocks to top-level, and collapsed the duplicated corepack/npm/npx symlink block to a single copy.
- How it works: focused API vitest coverage confirms the retry path and guard behavior, focused web vitest coverage confirms the store/modal tests still pass, and bash -n confirms the trimmed install script still parses cleanly.
@Ajit-Mehrotra Ajit-Mehrotra force-pushed the codex/onboarding-offline-gate branch from ddc10b3 to bdbd171 Compare March 13, 2026 16:08
- Purpose: prevent the onboarding modal from unmounting during the summary-step refetch that runs after Confirm & Apply.
- Before: the modal gate treated any onboarding query loading/error state as not displayable, even when prior onboarding data was already loaded.
- Why that was a problem: the summary flow could tear down the modal mid-apply, which made the user land back on the summary step without ever seeing the result dialog.
- What changed: the web onboarding gate now allows the modal to remain displayable when cached onboarding state already exists, and the store tests cover both the initial load/error gate and the refetch-with-existing-data case.
- How it works: initial open is still blocked when there is no onboarding state and the query is loading or errored, but subsequent refetches reuse the last known onboarding state so the active modal can finish its flow.
@Ajit-Mehrotra Ajit-Mehrotra force-pushed the codex/onboarding-offline-gate branch from bdbd171 to 7dd4ecf Compare March 13, 2026 18:19
@github-actions
Copy link
Contributor

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

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

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: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@plugin/source/dynamix.unraid.net/install/doinst.sh`:
- Around line 40-45: Remove the duplicated relink blocks for corepack, npm, and
npx and replace the unsafe subshell cd pattern with direct-path operations: for
each tool (corepack, npm, npx) perform a single rm -rf on usr/local/bin/<name>
and then a single ln -sf to usr/local/bin/<name> pointing at
../lib/node_modules/<package>/... (e.g. corepack ->
../lib/node_modules/corepack/dist/corepack.js, npm ->
../lib/node_modules/npm/bin/npm-cli.js, npx ->
../lib/node_modules/npm/bin/npx-cli.js); this eliminates redundant blocks and
avoids running rm/ln when a cd fails by explicitly acting on the target paths in
doinst.sh.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 65cbaf6b-38e6-4e58-9daf-08788b8cad0a

📥 Commits

Reviewing files that changed from the base of the PR and between 4d8d525 and 7dd4ecf.

⛔ Files ignored due to path filters (1)
  • api/src/unraid-api/cli/generated/graphql.ts is excluded by !**/generated/**
📒 Files selected for processing (9)
  • api/generated-schema.graphql
  • api/src/unraid-api/config/onboarding-tracker.service.spec.ts
  • api/src/unraid-api/config/onboarding-tracker.service.ts
  • api/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts
  • api/src/unraid-api/graph/resolvers/customization/customization.resolver.ts
  • plugin/source/dynamix.unraid.net/install/doinst.sh
  • web/__test__/store/onboardingStatus.test.ts
  • web/src/components/Onboarding/store/onboardingStatus.ts
  • web/src/composables/gql/graphql.ts
✅ Files skipped from review due to trivial changes (1)
  • api/generated-schema.graphql
🚧 Files skipped from review as they are similar to previous changes (5)
  • web/test/store/onboardingStatus.test.ts
  • api/src/unraid-api/graph/resolvers/customization/customization.resolver.spec.ts
  • api/src/unraid-api/config/onboarding-tracker.service.ts
  • web/src/composables/gql/graphql.ts
  • api/src/unraid-api/config/onboarding-tracker.service.spec.ts

Comment on lines +40 to +45
( 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 )
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:

#!/bin/bash
set -euo pipefail

FILE="plugin/source/dynamix.unraid.net/install/doinst.sh"

echo "Pattern occurrences:"
rg -n '^\( cd usr/local/bin ; (rm -rf|ln -sf) ' "$FILE"

echo
python - <<'PY'
from pathlib import Path
p = Path("plugin/source/dynamix.unraid.net/install/doinst.sh")
lines = p.read_text().splitlines()
b1 = lines[27:33]  # Line 28-33
b2 = lines[33:39]  # Line 34-39
b3 = lines[39:45]  # Line 40-45
print("28-33 equals 34-39:", b1 == b2)
print("34-39 equals 40-45:", b2 == b3)
PY

Repository: unraid/api

Length of output: 1193


Remove duplicate relink blocks and use direct paths to prevent unsafe cd fallback execution

Lines 40–45 duplicate the exact same relink commands from lines 28–39 and 34–39. Additionally, the ( cd ... ; cmd ) pattern causes rm -rf/ln -sf to execute even if cd fails, which is unsafe in installer scripts.

Suggested fix
-( 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 )
-( 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 )
-( 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 )
+rm -rf usr/local/bin/corepack
+ln -sf ../lib/node_modules/corepack/dist/corepack.js usr/local/bin/corepack
+rm -rf usr/local/bin/npm
+ln -sf ../lib/node_modules/npm/bin/npm-cli.js usr/local/bin/npm
+rm -rf usr/local/bin/npx
+ln -sf ../lib/node_modules/npm/bin/npx-cli.js usr/local/bin/npx
🧰 Tools
🪛 Shellcheck (0.11.0)

[warning] 40-40: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 41-41: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 42-42: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 43-43: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 44-44: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 45-45: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

🤖 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 relink blocks for corepack, npm, and npx and replace the
unsafe subshell cd pattern with direct-path operations: for each tool (corepack,
npm, npx) perform a single rm -rf on usr/local/bin/<name> and then a single ln
-sf to usr/local/bin/<name> pointing at ../lib/node_modules/<package>/... (e.g.
corepack -> ../lib/node_modules/corepack/dist/corepack.js, npm ->
../lib/node_modules/npm/bin/npm-cli.js, npx ->
../lib/node_modules/npm/bin/npx-cli.js); this eliminates redundant blocks and
avoids running rm/ln when a cd fails by explicitly acting on the target paths in
doinst.sh.

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.

1 participant