fix: share internal boot state across onboarding#1920
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Disabled knowledge base sources:
WalkthroughAdds a new boolean Vars field Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as "UI / Client"
participant Resolver as "VarsResolver"
participant Service as "InternalBootStateService"
participant Array as "ArrayService"
participant Disks as "DisksService"
UI->>Resolver: query vars
Resolver->>Service: getBootedFromFlashWithInternalBootSetup()
Service->>Array: getArray() / read boot disk
Array-->>Service: bootDisk info
Service->>Disks: getInternalBootDevices(bootDisk) (if FLASH)
Disks-->>Service: devices[]
Service-->>Resolver: boolean bootedFromFlashWithInternalBootSetup
Resolver-->>UI: vars { ..., bootedFromFlashWithInternalBootSetup }
UI->>UI: hide/disable internal-boot step (if true)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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
Comment Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9abf6f218
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bootedFromFlashWithInternalBootSetup = | ||
| await this.internalBootStateService.getBootedFromFlashWithInternalBootSetup(); |
There was a problem hiding this comment.
vars() now blocks on internalBootStateService.getBootedFromFlashWithInternalBootSetup(), but this probe can throw (it reaches DisksService.getInternalBootDevices(), which calls disk discovery methods that are not guarded for all failures). Because there is no try/catch or fallback here, a transient disk-inspection error will fail the entire vars query (and serverState queries that include vars) instead of returning the existing vars payload.
Useful? React with 👍 / 👎.
| }; | ||
| } | ||
|
|
||
| if (await this.isBootedFromFlashWithInternalBootSetup()) { |
There was a problem hiding this comment.
The new isBootedFromFlashWithInternalBootSetup() check runs before the method’s try/catch, so if internal-boot state lookup throws (for example while reading array/disk state), createInternalBootPool() rejects with a GraphQL error instead of returning the structured OnboardingInternalBootResult failure response this mutation otherwise guarantees.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1920 +/- ##
==========================================
+ Coverage 50.99% 51.04% +0.05%
==========================================
Files 1024 1025 +1
Lines 70569 70640 +71
Branches 7717 7738 +21
==========================================
+ Hits 35985 36061 +76
+ Misses 34458 34453 -5
Partials 126 126 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/onboarding/onboarding-internal-boot.service.spec.ts`:
- Around line 243-247: The test is asserting the full error string which couples
it to wording; instead assert behavior and a meaningful substring: keep checking
result for ok: false and code: 3 (e.g., via toMatchObject or separate expects
against result) and replace the exact-string check on result.output with a
substring assertion (e.g., expect(result.output).toContain('boot is already
configured') or 'internal boot'), referencing the test variable result in
onboarding-internal-boot.service.spec.ts.
In
`@api/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.ts`:
- Around line 351-357: The current pre-check calls
this.isBootedFromFlashWithInternalBootSetup() outside the surrounding try/catch
so any error from that lookup can escape and crash the mutation; move the
boot-state lookup into the guarded path by invoking
this.isBootedFromFlashWithInternalBootSetup() inside the existing try/catch (or
wrap it in its own try/catch) and return the same { ok:false, code:3, output:...
} response on a successful check while ensuring any thrown error is caught and
handled by the function's error handling.
In `@api/src/unraid-api/graph/resolvers/vars/vars.resolver.ts`:
- Around line 24-30: The vars() resolver currently awaits
internalBootStateService.getBootedFromFlashWithInternalBootSetup() which can
throw and cause the whole vars query to fail; wrap that call in a try/catch
inside vars() and on error set a safe default (e.g., null or false) for
bootedFromFlashWithInternalBootSetup and optionally log the error, then continue
returning the merged object (id: 'vars', ...(getters.emhttp().var ?? {}),
bootedFromFlashWithInternalBootSetup) so failures in internalBootStateService do
not surface as fatal resolver errors.
In `@web/src/store/server.ts`:
- Around line 1072-1074: The current guard using "typeof
data?.bootedFromFlashWithInternalBootSetup !== 'undefined'" prevents clearing
stale reportedBootedFromFlashWithInternalBootSetup values; change the check to
use "'bootedFromFlashWithInternalBootSetup' in data" and when that property
exists assign reportedBootedFromFlashWithInternalBootSetup.value =
data.bootedFromFlashWithInternalBootSetup so the value can be set to
undefined/false by mutateServerStateFromApi mapping; update the code around the
existing reportedBootedFromFlashWithInternalBootSetup assignment in server.ts
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a76466e7-5467-4496-b5a1-3e9733144beb
📒 Files selected for processing (22)
api/generated-schema.graphqlapi/src/unraid-api/graph/resolvers/disks/disks.module.tsapi/src/unraid-api/graph/resolvers/disks/internal-boot-notification.service.spec.tsapi/src/unraid-api/graph/resolvers/disks/internal-boot-notification.service.tsapi/src/unraid-api/graph/resolvers/disks/internal-boot-state.service.tsapi/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.spec.tsapi/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.tsapi/src/unraid-api/graph/resolvers/vars/vars.model.tsapi/src/unraid-api/graph/resolvers/vars/vars.resolver.spec.tsapi/src/unraid-api/graph/resolvers/vars/vars.resolver.tsweb/__test__/components/Onboarding/OnboardingInternalBootStep.test.tsweb/__test__/components/Onboarding/OnboardingModal.test.tsweb/src/components/Onboarding/OnboardingModal.vueweb/src/components/Onboarding/graphql/getInternalBootContext.query.tsweb/src/components/Onboarding/graphql/getInternalBootStepVisibility.query.tsweb/src/components/Onboarding/steps/OnboardingInternalBootStep.vueweb/src/components/Registration.standalone.vueweb/src/composables/gql/gql.tsweb/src/composables/gql/graphql.tsweb/src/store/server.fragment.tsweb/src/store/server.tsweb/types/server.ts
api/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.spec.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.ts
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
elibosley
changed the title
1 participant
Summary
varsand use it to hide/block onboarding internal boot setup when the system is still booted from flash but internal boot is already configuredValidation
pnpm lintinapi/pnpm type-checkinapi/pnpm lintinweb/pnpm type-checkinweb/Summary by CodeRabbit
New Features
Improvements