feat(web): add standalone internal boot wizard#1956
Merged
Conversation
- Purpose: add a standalone internal boot wizard entry point that uses the same full-screen modal shell as onboarding and shares the internal boot apply flow. - Before: internal boot apply logic lived inside the summary step, there was no standalone internal boot modal component, and the wrapper registry could not mount that standalone entry. - Problem: the internal boot experience could not match the onboarding modal UX, and the apply behavior was duplicated instead of shared across flows. - Now: the web app exposes a standalone internal boot modal component, registers it for auto-mounting, and reuses a shared apply helper for consistent success and warning handling. - How: factor the internal boot mutation/result processing into the composable, update onboarding summary to call the shared helper, add standalone component coverage, and refresh wrapper/type registrations for the new mount target.
- Purpose: simplify the dev deploy and cleanup scripts by removing the auth-request whitelist patching steps. - Before: deploy and cleanup rewrote /usr/local/emhttp/auth-request.php on the remote server to add and remove unraid component asset whitelist entries. - Problem: the scripts carried a large amount of brittle remote file surgery that was hard to trust and easy to break during local iteration. - Now: the scripts only handle component sync and cleanup, leaving auth-request.php untouched. - How: delete the remote whitelist injection and cleanup helpers from the deploy and cleanup shell scripts while preserving the surrounding rsync and directory cleanup flow.
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 a standalone OnboardingInternalBoot Vue component, new composables to apply internal-boot selections and summarize BIOS logs (with typed result/log interfaces), integrates them into the summary step and registry, removes auth-request update logic from deploy/cleanup scripts, and adds/updates tests and an i18n key. Changes
Sequence DiagramsequenceDiagram
participant User
participant Component as "OnboardingInternalBoot\n(Component)"
participant DraftStore as "Draft Store"
participant Composable as "applyInternalBootSelection\n(Composable)"
participant Server as "Mutation / Server"
User->>Component: Complete CONFIGURE_BOOT
Component->>DraftStore: read internalBootSelection
alt selection exists
Component->>Composable: invoke(selection, messages)
Composable->>Server: submit internal-boot mutation
Server-->>Composable: return ok/output or error
Composable->>Composable: parse BIOS output -> logs, flags
Composable-->>Component: return applyResult (applySucceeded, hadWarnings, logs)
Component->>DraftStore: setInternalBootApplySucceeded(true/false)
Component->>Component: render logs & result view
else no selection
Component->>Component: append "no changes" log -> set success result
end
User->>Component: close / edit again
Component->>DraftStore: cleanupOnboardingStorage() (on close)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1956 +/- ##
==========================================
+ Coverage 51.74% 51.95% +0.21%
==========================================
Files 1028 1030 +2
Lines 70792 71121 +329
Branches 7881 7937 +56
==========================================
+ Hits 36630 36951 +321
- Misses 34039 34047 +8
Partials 123 123 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 8daf70b257
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1035
to
+1038
| if (completionMarked && baselineLoaded) { | ||
| try { | ||
| await Promise.race([ | ||
| refetchOnboarding(), |
There was a problem hiding this comment.
This refetch runs while the summary step is still active, immediately after completeOnboarding() succeeds. Because the backend now reports onboarding as completed, refetchOnboarding() can flip shouldOpen to false, and OnboardingModal is rendered with v-if="showModal" based on that state, so the modal can unmount before users see the summary result dialog or Next Steps screen. In that path, the normal close handler (which performs onboarding draft cleanup) is also bypassed, so the happy path can end with an abrupt close and stale draft state.
Useful? React with 👍 / 👎.
- Purpose: keep the onboarding summary result dialog and Next Steps screen visible after completion succeeds. - Before: the summary step refetched onboarding visibility immediately after completeOnboarding, while the modal was still rendered from shouldOpen state. - Problem: a successful completion could cause the modal to unmount before users saw the result dialog or Next Steps screen, and that abrupt close bypassed the normal draft cleanup path. - Now: the summary step finishes its local result flow without refetching onboarding visibility mid-step, leaving cleanup to the existing modal close path. - How: remove the eager refetch from the summary step and update the summary tests to assert the dialog stays open until the user confirms.
![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: 4
🧹 Nitpick comments (2)
web/__test__/components/Onboarding/internalBoot.test.ts (1)
170-255: Please add a rejected-mutation case here, and avoid pinning the suite to one full error string.
applyInternalBootSelection()now has separatereturnedErrorandfailedflows, but these tests only coverok=trueandok=false, and theok=falseassertion hardcodes the whole message text. AmutateMock.mockRejectedValueOnce(...)case plus asserting the formatter callback invocation would catch the missing branch without making the test brittle to copy changes.As per coding guidelines, "Don't test exact error message strings unless the message format is specifically what you're testing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/internalBoot.test.ts` around lines 170 - 255, Add a test that covers the rejected-mutation branch by using mutateMock.mockRejectedValueOnce(...) to simulate a network/GraphQL rejection and assert that applyInternalBootSelection invokes the returnedError formatter (and/or uses the failed fallback) rather than relying on ok=false response handling; specifically, add a case that calls applyInternalBootSelection with the same input, sets mutateMock to reject with an Error or object, and then check result.applySucceeded is false, result.logs length and that result.logs[0].message is produced via the returnedError callback (or matches returnedError output using a partial/matcher rather than the full hard-coded string) to avoid brittle exact-string assertions.web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts (1)
902-904: Avoid pinning the rawofflineerror text here.This scenario already verifies the best-effort fallback. Matching the full rendered sentence, including
: offline, makes the test fail on harmless error-wrapping or copy changes.🔧 Suggested fix
expect(wrapper.text()).toContain( - 'Could not mark onboarding complete right now (API may be offline): offline' + 'Could not mark onboarding complete right now' );As per coding guidelines,
**/*.test.{ts,tsx}: 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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts` around lines 902 - 904, The assertion in OnboardingSummaryStep.test.ts pins the exact error suffix ": offline" which makes the test brittle; update the expectation to only assert the stable part of the message (for example check that wrapper.text() contains 'Could not mark onboarding complete right now' or use a regex that omits the dynamic error detail) so the test verifies behavior without depending on the raw error text produced by the API wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts`:
- Line 5: The mock for applyInternalBootSelection is typed with a zero-arg
signature but the real function accepts two parameters; update the mock
declaration in the test so it matches the real signature by typing vi.fn to
accept (selection, messages) and return Promise<InternalBootApplyResult> (use
the imported InternalBootApplyResult type and the applyInternalBootSelection
name to locate the mock). This ensures calls in the suite that pass selection
and messages are type-checked against the real function contract.
In `@web/src/components/Onboarding/composables/internalBoot.ts`:
- Around line 143-145: submitInternalBootCreation currently swallows
Apollo/network exceptions into a synthetic { ok:false, output } which prevents
the failed path and prevents buildOnboardingErrorDiagnostics from seeing the raw
GraphQL/network error; fix by preserving the original exception (e.g., add an
error field or rethrow for network/graphql failures) so callers like the code
around the await submitInternalBootCreation(selection, { reboot: false }) can
inspect result.error (or catch the thrown error) and pass the original exception
into buildOnboardingErrorDiagnostics(...) instead of only using ok=false; adjust
submitInternalBootCreation (or its call sites) and the handling around result to
propagate/consume the original exception so the failed path is reachable and
diagnostics receive the raw error.
- Around line 151-175: When selection.updateBios is true, treat absent or
malformed BIOS output as unverified instead of a silent success: after calling
summarizeInternalBootBiosLogs(result.output) check if biosLogSummary.summaryLine
is missing OR biosLogSummary.failureLines is empty while the raw result.output
is empty/undefined or doesn’t match expected completion; if so set hadWarnings =
true and hadNonOptimisticFailures = true and push a warning entry onto logs (use
the same shape as other entries) indicating BIOS update could not be verified so
callers won’t mark applySucceeded as a clean success. Ensure this logic sits
alongside the existing failureLines/summaryLine checks in the
selection.updateBios block so known failures still produce errors and valid
summaries still produce success messages.
In `@web/src/composables/gql/index.ts`:
- Line 2: Add a trailing newline at the end of the file containing the export
statement to satisfy the eol-last ESLint rule: open the module that contains the
line `export * from "./gql";` (index.ts) and ensure there is a final newline
character after that line so the file ends with a newline.
---
Nitpick comments:
In `@web/__test__/components/Onboarding/internalBoot.test.ts`:
- Around line 170-255: Add a test that covers the rejected-mutation branch by
using mutateMock.mockRejectedValueOnce(...) to simulate a network/GraphQL
rejection and assert that applyInternalBootSelection invokes the returnedError
formatter (and/or uses the failed fallback) rather than relying on ok=false
response handling; specifically, add a case that calls
applyInternalBootSelection with the same input, sets mutateMock to reject with
an Error or object, and then check result.applySucceeded is false, result.logs
length and that result.logs[0].message is produced via the returnedError
callback (or matches returnedError output using a partial/matcher rather than
the full hard-coded string) to avoid brittle exact-string assertions.
In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts`:
- Around line 902-904: The assertion in OnboardingSummaryStep.test.ts pins the
exact error suffix ": offline" which makes the test brittle; update the
expectation to only assert the stable part of the message (for example check
that wrapper.text() contains 'Could not mark onboarding complete right now' or
use a regex that omits the dynamic error detail) so the test verifies behavior
without depending on the raw error text produced by the API wrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3bf7bc75-5bbb-451e-9ff7-019fec491a7f
📒 Files selected for processing (13)
web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.tsweb/__test__/components/Onboarding/OnboardingSummaryStep.test.tsweb/__test__/components/Onboarding/internalBoot.test.tsweb/__test__/components/Wrapper/component-registry.test.tsweb/__test__/components/component-registry.test.tsweb/components.d.tsweb/scripts/clean-unraid.shweb/scripts/deploy-dev.shweb/src/components/Onboarding/composables/internalBoot.tsweb/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vueweb/src/components/Onboarding/steps/OnboardingSummaryStep.vueweb/src/components/Wrapper/component-registry.tsweb/src/composables/gql/index.ts
💤 Files with no reviewable changes (1)
- web/scripts/deploy-dev.sh
web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+143
to
+145
| const result = await submitInternalBootCreation(selection, { reboot: false }); | ||
|
|
||
| if (result.ok) { |
Contributor
There was a problem hiding this comment.
Preserve the original mutation exception here instead of collapsing everything to ok=false.
submitInternalBootCreation() already catches Apollo/network failures and converts them into { ok: false, output }, so this branch never sees the original exception. That makes the new failed path effectively dead for the main failure mode, and buildOnboardingErrorDiagnostics(...) only gets the synthetic ok=false payload instead of the raw GraphQL/network context.
Also applies to: 185-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/Onboarding/composables/internalBoot.ts` around lines 143 -
145, submitInternalBootCreation currently swallows Apollo/network exceptions
into a synthetic { ok:false, output } which prevents the failed path and
prevents buildOnboardingErrorDiagnostics from seeing the raw GraphQL/network
error; fix by preserving the original exception (e.g., add an error field or
rethrow for network/graphql failures) so callers like the code around the await
submitInternalBootCreation(selection, { reboot: false }) can inspect
result.error (or catch the thrown error) and pass the original exception into
buildOnboardingErrorDiagnostics(...) instead of only using ok=false; adjust
submitInternalBootCreation (or its call sites) and the handling around result to
propagate/consume the original exception so the failed path is reachable and
diagnostics receive the raw error.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/src/components/Onboarding/steps/OnboardingSummaryStep.vue (1)
1016-1035: Consider using the existingsleepMshelper for consistency.Line 1028 uses an inline promise for the 1-second delay, but there's already a
sleepMshelper defined at line 336 that does the same thing.♻️ Suggested change
if (completionMarked) { - await new Promise((r) => setTimeout(r, 1000)); + await sleepMs(1000); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Onboarding/steps/OnboardingSummaryStep.vue` around lines 1016 - 1035, Replace the inline 1-second delay with the existing sleep helper to keep consistency: when completionMarked is true after runWithTransientNetworkRetry/completeOnboarding, call sleepMs(1000) instead of creating a new Promise; update the block that currently uses await new Promise((r) => setTimeout(r, 1000)) to use sleepMs so it relies on the shared helper and keeps behavior tied to the sleepMs implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/src/components/Onboarding/steps/OnboardingSummaryStep.vue`:
- Around line 1016-1035: Replace the inline 1-second delay with the existing
sleep helper to keep consistency: when completionMarked is true after
runWithTransientNetworkRetry/completeOnboarding, call sleepMs(1000) instead of
creating a new Promise; update the block that currently uses await new
Promise((r) => setTimeout(r, 1000)) to use sleepMs so it relies on the shared
helper and keeps behavior tied to the sleepMs implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ba2c6114-10fc-4c77-bb1f-a97b3be711c5
📒 Files selected for processing (2)
web/__test__/components/Onboarding/OnboardingSummaryStep.test.tsweb/src/components/Onboarding/steps/OnboardingSummaryStep.vue
- The file `web/src/composables/gql/index.ts` was missing a newline at the end of file - ESLint enforces the `eol-last` rule, which requires all files to end with a newline character - Without it, the `Build Web App` CI check fails with: "Newline required at end of file but not found" - Adding the trailing newline satisfies the linter and unblocks the build pipeline; no runtime behavior changes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- The `completion mutation fails` scenario in `OnboardingSummaryStep.test.ts`
asserted the full rendered string including the raw error suffix
`': offline'` (from `new Error('offline')`)
- Asserting the exact error text from a thrown exception couples the
test to the error-message wording of the `addErrorLog` formatter and
the underlying rejection string — neither of which reflects intended
component behavior
- Per project coding guidelines: "Don't write tests that break when
minor changes are made to error messages or other non-essential
details"
- Changed the assertion to match only the stable, user-visible prefix
`'Could not mark onboarding complete right now'`, which is the
translated string we actually care about
- The scenario still verifies that the failure was surfaced in the UI;
it just no longer pins the dynamic error detail
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ture - The `applyInternalBootSelectionMock` in `OnboardingInternalBootStandalone.test.ts` was typed as `vi.fn<() => Promise<InternalBootApplyResult>>()` — a zero-argument function — while the real implementation accepts two required params: `(selection: InternalBootSelection, messages: InternalBootApplyMessages)` - This mismatch silently dropped compile-time checking on every call site in the test suite that passes `selection` and `messages`, meaning type errors in those arguments would go undetected - Added `InternalBootSelection` and `InternalBootApplyMessages` to the type import from the composable - Updated the mock declaration to match the real two-argument signature so TypeScript enforces the correct call contract throughout the suite Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- When `updateBios=true` and `result.ok=true`, the BIOS log summary was only checked for known failure lines and "with warnings" in the completion line - If the backend returned empty or unrecognizable BIOS output (e.g., format changed, no BIOS-related lines at all), both `summaryLine` and `failureLines` would be empty, causing `hadBiosWarnings` to be false - The function returned `applySucceeded: true` with no warning, even though the BIOS update was never confirmed — silently reporting success for a hardware operation the UI could not verify - Added a `biosUnverified` check: when `summaryLine` is null AND `failureLines` is empty, the result now sets `hadWarnings` and `hadNonOptimisticFailures` to true and pushes an error log with the caller-supplied `biosUnverified` message - Added `biosUnverified` field to `InternalBootApplyMessages` so both the onboarding wizard and standalone boot wizard can supply a localized string - Added locale key `onboarding.summaryStep.logs.internalBootBiosUnverified` - Added test case covering the unverified BIOS output path - Updated existing test message fixtures and call assertions to include the new `biosUnverified` field Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- The `handleComplete` function in `OnboardingSummaryStep.vue` used an inline `await new Promise((r) => setTimeout(r, 1000))` for the post-completion delay - The same file already defines a `sleepMs` helper at line 336 that does exactly the same thing: `const sleepMs = (ms: number) => new Promise<void>((resolve) => setTimeout(resolve, ms))` - Having both patterns in the same file is inconsistent and makes the inline version look like it was missed during an earlier refactor - Replaced with `await sleepMs(1000)` for consistency with the rest of the file's delay usage (e.g., the transient network retry backoff) - No behavior change; the delay duration and semantics are identical Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue (1)
216-224: Consider theme-aware color classes for dark mode compatibility.The hard-coded
border-red-200 bg-red-50/70andborder-amber-200 bg-amber-50/70classes are light-mode specific. If dark mode is supported, these may needdark:variants or design-system semantic color classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue` around lines 216 - 224, The conditional result styling for the OnboardingInternalBoot.standalone.vue template uses light-mode-specific classes when confirmationState === 'result' and resultSeverity is 'error' or 'warning'; update the :class binding on that div to use theme-aware or design-system semantic classes (or add corresponding dark: variants) so colors adapt in dark mode—for example replace 'border-red-200 bg-red-50/70' and 'border-amber-200 bg-amber-50/70' with your app's semantic tokens (e.g., destructive/negative and warning/amber tokens) or add dark variants like 'dark:border-red-700 dark:bg-red-900/40' and 'dark:border-amber-700 dark:bg-amber-900/40' in the :class mapping to ensure proper dark-mode rendering.web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts (1)
1112-1121: Assert that cancel skipscompleteOnboarding()too.Now that
handleComplete()always calls the completion mutation, this interaction test should guard the full cancel boundary, not justapplyInternalBootSelection().As per coding guidelines, "Test component interactions such as clicks and inputs".🧪 Suggested assertion
expect(wrapper.text()).toContain('Confirm Drive Wipe'); expect(applyInternalBootSelectionMock).not.toHaveBeenCalled(); + expect(completeOnboardingMock).not.toHaveBeenCalled(); @@ expect(wrapper.text()).not.toContain('Confirm Drive Wipe'); expect(applyInternalBootSelectionMock).not.toHaveBeenCalled(); + expect(completeOnboardingMock).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts` around lines 1112 - 1121, The test currently asserts cancel doesn't call applyInternalBootSelection(), but since handleComplete() now always invokes the completion mutation you must also assert that cancel skips the completion flow; update the OnboardingSummaryStep.test by adding an assertion after the cancel click that the completion mutation handler (e.g. completeOnboarding or its mock, e.g. completeOnboardingMock) was not called; locate the interaction where applyInternalBootSelectionMock is checked and add expect(completeOnboardingMock).not.toHaveBeenCalled() (or the exact mock/function name used in the test) to fully guard the cancel boundary invoked by handleComplete().
🤖 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/OnboardingSummaryStep.test.ts`:
- Around line 859-867: The test case "baseline unavailable + completion
succeeds" currently sets coreSettingsResult.value = null and coreSettingsError,
but leaves the SSH mutation mock returning the default {} which can still route
the component into the SSH-unverified branch; update the test's apply step to
also set the SSH mutation mock to return a verified-but-SSH-disabled payload
(the same shape your app uses for a verified baseline with SSH disabled) so the
component actually renders the baseline-unavailable messaging; modify the test
around coreSettingsResult/coreSettingsError in OnboardingSummaryStep.test
(referencing coreSettingsResult, coreSettingsError, completeOnboardingMock, and
the assertion that wrapper.text() contains "Setup Saved in Best-Effort Mode") to
inject that verified disabled-SSH response instead of {}.
In
`@web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue`:
- Around line 133-150: Wrap the await applyInternalBootSelection call in a
try/catch so thrown errors are handled: catch the error from
applyInternalBootSelection, call draftStore.setInternalBootApplySucceeded(false)
(or ensure it's false), add an error log via addLogs (or push a log entry
describing the exception), call setResultFromApply with a failure/empty result
if appropriate, and set confirmationState.value to 'result' (or another terminal
error state) so the UI can recover; keep the existing finally that clears
progressTimer. Reference applyInternalBootSelection, confirmationState,
draftStore.setInternalBootApplySucceeded, addLogs, setResultFromApply, and
progressTimer when making the changes.
In `@web/src/components/Onboarding/steps/OnboardingSummaryStep.vue`:
- Around line 1018-1020: The call to completeOnboarding is currently gated by
the SSH-only retry flag; change the flow so completeOnboarding is always wrapped
with runWithTransientNetworkRetry using shouldRetryNetworkMutations.
Specifically, remove reuse of the SSH-change gate and replace the existing await
completeOnboarding() call with await runWithTransientNetworkRetry(() =>
completeOnboarding(), shouldRetryNetworkMutations) (or move the existing
runWithTransientNetworkRetry invocation so it executes unconditionally before
setting completionMarked = true), ensuring completeOnboarding gets
transient-network retry protection regardless of SSH-change logic.
---
Nitpick comments:
In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts`:
- Around line 1112-1121: The test currently asserts cancel doesn't call
applyInternalBootSelection(), but since handleComplete() now always invokes the
completion mutation you must also assert that cancel skips the completion flow;
update the OnboardingSummaryStep.test by adding an assertion after the cancel
click that the completion mutation handler (e.g. completeOnboarding or its mock,
e.g. completeOnboardingMock) was not called; locate the interaction where
applyInternalBootSelectionMock is checked and add
expect(completeOnboardingMock).not.toHaveBeenCalled() (or the exact
mock/function name used in the test) to fully guard the cancel boundary invoked
by handleComplete().
In
`@web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue`:
- Around line 216-224: The conditional result styling for the
OnboardingInternalBoot.standalone.vue template uses light-mode-specific classes
when confirmationState === 'result' and resultSeverity is 'error' or 'warning';
update the :class binding on that div to use theme-aware or design-system
semantic classes (or add corresponding dark: variants) so colors adapt in dark
mode—for example replace 'border-red-200 bg-red-50/70' and 'border-amber-200
bg-amber-50/70' with your app's semantic tokens (e.g., destructive/negative and
warning/amber tokens) or add dark variants like 'dark:border-red-700
dark:bg-red-900/40' and 'dark:border-amber-700 dark:bg-amber-900/40' in the
:class mapping to ensure proper dark-mode rendering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c92f921-c8a4-4aaa-9671-1f98b64231e2
📒 Files selected for processing (7)
web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.tsweb/__test__/components/Onboarding/OnboardingSummaryStep.test.tsweb/__test__/components/Onboarding/internalBoot.test.tsweb/src/components/Onboarding/composables/internalBoot.tsweb/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vueweb/src/components/Onboarding/steps/OnboardingSummaryStep.vueweb/src/locales/en.json
✅ Files skipped from review due to trivial changes (1)
- web/src/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (3)
- web/test/components/Onboarding/OnboardingInternalBootStandalone.test.ts
- web/test/components/Onboarding/internalBoot.test.ts
- web/src/components/Onboarding/composables/internalBoot.ts
Comment on lines
+859
to
+867
| caseName: 'baseline unavailable + completion succeeds', | ||
| apply: () => { | ||
| coreSettingsResult.value = null; | ||
| coreSettingsError.value = new Error('Graphql is offline.'); | ||
| }, | ||
| assertExpected: (wrapper: ReturnType<typeof mountComponent>['wrapper']) => { | ||
| expect(completeOnboardingMock).not.toHaveBeenCalled(); | ||
| expect(refetchOnboardingMock).not.toHaveBeenCalled(); | ||
| expect(wrapper.text()).toContain( | ||
| 'Baseline settings unavailable. Continuing in best-effort mode.' | ||
| ); | ||
| expect(completeOnboardingMock).toHaveBeenCalledTimes(1); | ||
| expect(wrapper.text()).toContain('Setup Saved in Best-Effort Mode'); | ||
| }, |
Contributor
There was a problem hiding this comment.
Make the “baseline unavailable” case prove the intended branch.
With the default {} SSH mutation mock, this scenario can still land in the SSH-unverified path and pass on the shared best-effort title alone. Return a verified disabled-SSH payload here so the test actually covers the baseline-unavailable messaging.
🧪 Suggested test hardening
apply: () => {
coreSettingsResult.value = null;
coreSettingsError.value = new Error('Graphql is offline.');
+ updateSshSettingsMock.mockResolvedValue({
+ data: {
+ updateSshSettings: { id: 'vars', useSsh: false, portssh: 22 },
+ },
+ });
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| caseName: 'baseline unavailable + completion succeeds', | |
| apply: () => { | |
| coreSettingsResult.value = null; | |
| coreSettingsError.value = new Error('Graphql is offline.'); | |
| }, | |
| assertExpected: (wrapper: ReturnType<typeof mountComponent>['wrapper']) => { | |
| expect(completeOnboardingMock).not.toHaveBeenCalled(); | |
| expect(refetchOnboardingMock).not.toHaveBeenCalled(); | |
| expect(wrapper.text()).toContain( | |
| 'Baseline settings unavailable. Continuing in best-effort mode.' | |
| ); | |
| expect(completeOnboardingMock).toHaveBeenCalledTimes(1); | |
| expect(wrapper.text()).toContain('Setup Saved in Best-Effort Mode'); | |
| }, | |
| caseName: 'baseline unavailable + completion succeeds', | |
| apply: () => { | |
| coreSettingsResult.value = null; | |
| coreSettingsError.value = new Error('Graphql is offline.'); | |
| updateSshSettingsMock.mockResolvedValue({ | |
| data: { | |
| updateSshSettings: { id: 'vars', useSsh: false, portssh: 22 }, | |
| }, | |
| }); | |
| }, | |
| assertExpected: (wrapper: ReturnType<typeof mountComponent>['wrapper']) => { | |
| expect(completeOnboardingMock).toHaveBeenCalledTimes(1); | |
| expect(wrapper.text()).toContain('Setup Saved in Best-Effort Mode'); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts` around
lines 859 - 867, The test case "baseline unavailable + completion succeeds"
currently sets coreSettingsResult.value = null and coreSettingsError, but leaves
the SSH mutation mock returning the default {} which can still route the
component into the SSH-unverified branch; update the test's apply step to also
set the SSH mutation mock to return a verified-but-SSH-disabled payload (the
same shape your app uses for a verified baseline with SSH disabled) so the
component actually renders the baseline-unavailable messaging; modify the test
around coreSettingsResult/coreSettingsError in OnboardingSummaryStep.test
(referencing coreSettingsResult, coreSettingsError, completeOnboardingMock, and
the assertion that wrapper.text() contains "Setup Saved in Best-Effort Mode") to
inject that verified disabled-SSH response instead of {}.
Comment on lines
+133
to
+150
| try { | ||
| const applyResult = await applyInternalBootSelection(selection, { | ||
| configured: summaryT('logs.internalBootConfigured'), | ||
| returnedError: (output) => summaryT('logs.internalBootReturnedError', { output }), | ||
| failed: summaryT('logs.internalBootFailed'), | ||
| biosUnverified: summaryT('logs.internalBootBiosUnverified'), | ||
| }); | ||
|
|
||
| if (applyResult.applySucceeded) { | ||
| draftStore.setInternalBootApplySucceeded(true); | ||
| } | ||
|
|
||
| addLogs(applyResult.logs); | ||
| setResultFromApply(applyResult); | ||
| confirmationState.value = 'result'; | ||
| } finally { | ||
| clearInterval(progressTimer); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing error handling leaves UI stuck if applyInternalBootSelection throws.
If the async call throws (network failure, unexpected exception), confirmationState remains 'saving' indefinitely. The user sees an infinite spinner with no recovery path since canEditAgain requires confirmationState === 'result'.
🛡️ Proposed fix to handle errors gracefully
try {
const applyResult = await applyInternalBootSelection(selection, {
configured: summaryT('logs.internalBootConfigured'),
returnedError: (output) => summaryT('logs.internalBootReturnedError', { output }),
failed: summaryT('logs.internalBootFailed'),
biosUnverified: summaryT('logs.internalBootBiosUnverified'),
});
if (applyResult.applySucceeded) {
draftStore.setInternalBootApplySucceeded(true);
}
addLogs(applyResult.logs);
setResultFromApply(applyResult);
+ } catch (error) {
+ addLog({
+ message: summaryT('logs.internalBootFailed'),
+ type: 'error',
+ });
+ resultSeverity.value = 'error';
+ resultTitle.value = summaryT('result.failedTitle');
+ resultMessage.value = summaryT('result.failedMessage');
+ } finally {
confirmationState.value = 'result';
- } finally {
clearInterval(progressTimer);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue`
around lines 133 - 150, Wrap the await applyInternalBootSelection call in a
try/catch so thrown errors are handled: catch the error from
applyInternalBootSelection, call draftStore.setInternalBootApplySucceeded(false)
(or ensure it's false), add an error log via addLogs (or push a log entry
describing the exception), call setResultFromApply with a failure/empty result
if appropriate, and set confirmationState.value to 'result' (or another terminal
error state) so the UI can recover; keep the existing finally that clears
progressTimer. Reference applyInternalBootSelection, confirmationState,
draftStore.setInternalBootApplySucceeded, addLogs, setResultFromApply, and
progressTimer when making the changes.
Comment on lines
+1018
to
+1020
| try { | ||
| await runWithTransientNetworkRetry(() => completeOnboarding(), shouldRetryNetworkMutations); | ||
| completionMarked = true; |
Contributor
There was a problem hiding this comment.
Retry completeOnboarding() outside the SSH-change case.
Line 1019 reuses the SSH-only retry gate, so a transient fetch failure here immediately drops the common “no SSH change” path into the offline/best-effort result. Since this is the final durable state change, it should get the transient-network retry protection too.
🔁 Proposed fix
- await runWithTransientNetworkRetry(() => completeOnboarding(), shouldRetryNetworkMutations);
+ await runWithTransientNetworkRetry(() => completeOnboarding(), true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/Onboarding/steps/OnboardingSummaryStep.vue` around lines
1018 - 1020, The call to completeOnboarding is currently gated by the SSH-only
retry flag; change the flow so completeOnboarding is always wrapped with
runWithTransientNetworkRetry using shouldRetryNetworkMutations. Specifically,
remove reuse of the SSH-change gate and replace the existing await
completeOnboarding() call with await runWithTransientNetworkRetry(() =>
completeOnboarding(), shouldRetryNetworkMutations) (or move the existing
runWithTransientNetworkRetry invocation so it executes unconditionally before
setting completionMarked = true), ensuring completeOnboarding gets
transient-network retry protection regardless of SSH-change logic.
- Purpose: prevent double invocation of the completeOnboarding mutation during the onboarding flow - Before: commit 5be53a4 moved completeOnboarding() from SummaryStep to NextStepsStep so users could see the result dialog before the modal unmounted; PR #1956 re-added it to SummaryStep while NextStepsStep still had it, causing the mutation to fire twice - Problem: the second call is redundant at best, and if the mutation is not idempotent it could produce errors or inconsistent server state - Now: SummaryStep only applies settings (timezone, theme, SSH, plugins, internal boot) without marking onboarding complete; completion is handled exclusively by NextStepsStep when the user clicks "Go to Dashboard" or "Reboot" - How: removed COMPLETE_ONBOARDING_MUTATION import and useMutation call, removed the completionMarked variable and "// 5. Mark Complete" block, restored the pre-5be53a4 result severity logic that does not depend on completion status, restored the settingsApplied success log in the success path, and updated all test assertions from toHaveBeenCalledTimes(1) to not.toHaveBeenCalled()
- Purpose: prevent stale internal boot state from persisting in localStorage after the standalone wizard is dismissed - Before: handleClose() only set isOpen to false, leaving internalBootSelection, internalBootApplySucceeded, and other draft fields in the Pinia store and localStorage indefinitely - Problem: leftover draft state could bleed into a future onboarding session, causing the onboarding modal to display stale boot configuration or incorrect apply-succeeded flags - Now: handleClose() calls cleanupOnboardingStorage() before closing, which resets the Pinia store and removes the localStorage keys - How: imported cleanupOnboardingStorage from the existing onboardingStorageCleanup module and added the call at the top of handleClose(); this covers all three exit paths (X button, Close button on result card, and dialog dismiss event)
…t composable - Purpose: eliminate duplicate error-message extraction logic between the SummaryStep component and the internalBoot composable - Before: both OnboardingSummaryStep.vue and internalBoot.ts defined their own getErrorMessage function with identical logic (extract trimmed message from Error instances or strings, fallback to unknown) - Problem: DRY violation — two copies of the same utility with slightly different fallback strings, making maintenance error-prone - Now: getErrorMessage is exported from the internalBoot composable and imported by SummaryStep, giving a single source of truth - How: added the export keyword to the existing getErrorMessage in internalBoot.ts, imported it in SummaryStep, removed the local duplicate, updated the SummaryStep test mock to use importOriginal so the real getErrorMessage is available alongside the mocked applyInternalBootSelection, and added cleanupOnboardingStorage mock to the standalone test to support the new cleanup-on-close behavior
…able - Purpose: close 6 test coverage gaps identified during engineering review of the standalone internal boot wizard - Before: standalone tests covered happy path, failure retry, and basic close, but missed the X button close path, warning result variant, draft cleanup verification, and the composable had no tests for the mutation rejection or success-without-BIOS paths - Problem: 57% code path coverage left the draft cleanup behavior (Issue 2 fix), partial success UX, and error propagation untested - Now: 6 new test cases bring coverage to all identified paths - How: added standalone tests for X button close with cleanup assertion, apply-with-warnings result (partial success shows warning icon and message), draft cleanup on result-close, and composable tests for mutation rejection (error propagated through ok=false path with diagnostics) and success-without-BIOS (single success log, no BIOS processing)
- Purpose: satisfy ESLint formatting rules after manual edits - Before: two test files had indentation inconsistencies introduced during the review fix commits - Now: indentation matches the project's lint configuration - How: ran pnpm lint:fix which auto-corrected the indentation in vi.hoisted() callback body and a long importOriginal type annotation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
web/src/components/Onboarding/composables/internalBoot.ts (1)
143-145:⚠️ Potential issue | 🟠 MajorPreserve the original mutation exception instead of collapsing it to
ok=false.
submitInternalBootCreation()already converts Apollo/network rejects into{ ok: false, output }, so thecatchpath here is effectively unreachable for the main transport-failure case andbuildOnboardingErrorDiagnostics(...)only sees the synthetic payload from theok=falsebranch. Please preserve the original exception somehow here (for example by returning anerrorfield alongsideok=false) so callers can distinguish backendok=falsefrom GraphQL/network failures and keep the raw diagnostics context.Also applies to: 192-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Onboarding/composables/internalBoot.ts` around lines 143 - 145, The code currently collapses transport/GraphQL exceptions into { ok:false, output } when calling submitInternalBootCreation, causing buildOnboardingErrorDiagnostics to lose the original exception; change submitInternalBootCreation so that when it handles/reduces a reject it returns { ok: false, output, error: originalError } (or rethrow for callers that need it), then update the caller(s) in internalBoot.ts (the try/catch at the submitInternalBootCreation call and the similar block around lines ~192-221) to use the returned error field (or catch the rethrown error) and forward that raw exception into buildOnboardingErrorDiagnostics so backend ok=false vs network/GraphQL failures can be distinguished.web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue (1)
135-150:⚠️ Potential issue | 🟠 MajorHandle rejected apply calls so the wizard can leave the saving state.
If
applyInternalBootSelection()throws here,confirmationStatenever reaches'result', so the standalone flow is stuck on the spinner with no recovery path. Add acatchthat records the failure result before thefinallyruns.💥 Minimal recovery path
try { const applyResult = await applyInternalBootSelection(selection, { configured: summaryT('logs.internalBootConfigured'), returnedError: (output) => summaryT('logs.internalBootReturnedError', { output }), failed: summaryT('logs.internalBootFailed'), biosUnverified: summaryT('logs.internalBootBiosUnverified'), }); if (applyResult.applySucceeded) { draftStore.setInternalBootApplySucceeded(true); } addLogs(applyResult.logs); setResultFromApply(applyResult); confirmationState.value = 'result'; + } catch { + addLog({ + message: summaryT('logs.internalBootFailed'), + type: 'error', + }); + resultSeverity.value = 'error'; + resultTitle.value = summaryT('result.failedTitle'); + resultMessage.value = summaryT('result.failedMessage'); + confirmationState.value = 'result'; } finally { clearInterval(progressTimer); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue` around lines 135 - 150, The try block calling applyInternalBootSelection can throw and leave confirmationState stuck on the spinner; add a catch after the try that handles rejected promises from applyInternalBootSelection by creating/setting an appropriate failure result (call setResultFromApply with a failure-shaped object or otherwise set confirmationState.value = 'result' and record logs via addLogs), ensure draftStore.setInternalBootApplySucceeded(false) is applied when it fails, and rethrow or continue to the finally so the finally block still runs; reference applyInternalBootSelection, confirmationState, draftStore.setInternalBootApplySucceeded, addLogs, and setResultFromApply when implementing this catch.web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts (1)
863-871:⚠️ Potential issue | 🟡 MinorMake the offline scenario prove the baseline-unavailable branch.
With the default
{}SSH mutation mock, this case can still land in the SSH-unverified path and pass on the shared best-effort title alone. Return a verified disabled-SSH payload here so the assertion is actually pinned to the offline/baseline-unavailable behavior.🧪 Suggested test hardening
apply: () => { coreSettingsResult.value = null; coreSettingsError.value = new Error('Graphql is offline.'); + updateSshSettingsMock.mockResolvedValue({ + data: { + updateSshSettings: { useSsh: false, portssh: 22 }, + }, + }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts` around lines 863 - 871, This test’s offline/baseline-unavailable case needs its SSH mutation mock to return a verified-but-disabled SSH payload so the test cannot accidentally take the SSH-unverified path; in the case’s apply block (the one setting coreSettingsResult.value = null and coreSettingsError.value = new Error(...)) update the SSH mutation mock used by the test (e.g., sshMutationMock or the mutation resolver used by mountComponent) to return a payload indicating SSH is verified and disabled (verified: true, enabled: false) so the assertion on the offline/baseline-unavailable behavior (expect(wrapper.text()).toContain('Setup Saved in Best-Effort Mode')) is actually pinned to the offline scenario rather than the default {} SSH response.
🤖 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/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue`:
- Around line 86-89: The dialog close flow currently only hides the UI and
clears storage, but in-progress apply logic (handleStepComplete and its progress
interval) can resume and re-dirty state after cleanup; update handleStepComplete
to short-circuit any post-await updates by checking isOpen.value (or an
"isClosed" flag) and returning early if false, and ensure any progress
timer/interval started in the apply flow is cleared when the dialog is closed;
modify handleClose to clear the progress interval (the same interval identifier
used in the apply/progress logic), set isOpen.value = false, call
cleanupOnboardingStorage(), and cancel/flag any in-flight promises so functions
like setInternalBootApplySucceeded do nothing if isOpen is false (apply the same
guard to the other similar apply flow in the 128-149 region).
---
Duplicate comments:
In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts`:
- Around line 863-871: This test’s offline/baseline-unavailable case needs its
SSH mutation mock to return a verified-but-disabled SSH payload so the test
cannot accidentally take the SSH-unverified path; in the case’s apply block (the
one setting coreSettingsResult.value = null and coreSettingsError.value = new
Error(...)) update the SSH mutation mock used by the test (e.g., sshMutationMock
or the mutation resolver used by mountComponent) to return a payload indicating
SSH is verified and disabled (verified: true, enabled: false) so the assertion
on the offline/baseline-unavailable behavior
(expect(wrapper.text()).toContain('Setup Saved in Best-Effort Mode')) is
actually pinned to the offline scenario rather than the default {} SSH response.
In `@web/src/components/Onboarding/composables/internalBoot.ts`:
- Around line 143-145: The code currently collapses transport/GraphQL exceptions
into { ok:false, output } when calling submitInternalBootCreation, causing
buildOnboardingErrorDiagnostics to lose the original exception; change
submitInternalBootCreation so that when it handles/reduces a reject it returns {
ok: false, output, error: originalError } (or rethrow for callers that need it),
then update the caller(s) in internalBoot.ts (the try/catch at the
submitInternalBootCreation call and the similar block around lines ~192-221) to
use the returned error field (or catch the rethrown error) and forward that raw
exception into buildOnboardingErrorDiagnostics so backend ok=false vs
network/GraphQL failures can be distinguished.
In
`@web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue`:
- Around line 135-150: The try block calling applyInternalBootSelection can
throw and leave confirmationState stuck on the spinner; add a catch after the
try that handles rejected promises from applyInternalBootSelection by
creating/setting an appropriate failure result (call setResultFromApply with a
failure-shaped object or otherwise set confirmationState.value = 'result' and
record logs via addLogs), ensure draftStore.setInternalBootApplySucceeded(false)
is applied when it fails, and rethrow or continue to the finally so the finally
block still runs; reference applyInternalBootSelection, confirmationState,
draftStore.setInternalBootApplySucceeded, addLogs, and setResultFromApply when
implementing this catch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5d1d7179-303b-46fc-a4c3-48668eecd798
📒 Files selected for processing (6)
web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.tsweb/__test__/components/Onboarding/OnboardingSummaryStep.test.tsweb/__test__/components/Onboarding/internalBoot.test.tsweb/src/components/Onboarding/composables/internalBoot.tsweb/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vueweb/src/components/Onboarding/steps/OnboardingSummaryStep.vue
Comment on lines
+86
to
+89
| const handleClose = () => { | ||
| cleanupOnboardingStorage(); | ||
| isOpen.value = false; | ||
| }; |
Contributor
There was a problem hiding this comment.
Cancel the in-flight apply path when the dialog closes.
The close button stays active while the wizard is saving, but handleClose() only hides the dialog and clears storage. The pending handleStepComplete() keeps its interval alive and will still append logs / call setInternalBootApplySucceeded(true) after cleanup once the promise resolves, which can immediately re-dirty the Pinia draft you just cleared. Short-circuit the post-await updates when isOpen is already false, and stop the progress timer on close as well.
Also applies to: 128-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@web/src/components/Onboarding/standalone/OnboardingInternalBoot.standalone.vue`
around lines 86 - 89, The dialog close flow currently only hides the UI and
clears storage, but in-progress apply logic (handleStepComplete and its progress
interval) can resume and re-dirty state after cleanup; update handleStepComplete
to short-circuit any post-await updates by checking isOpen.value (or an
"isClosed" flag) and returning early if false, and ensure any progress
timer/interval started in the apply flow is cleared when the dialog is closed;
modify handleClose to clear the progress interval (the same interval identifier
used in the apply/progress logic), set isOpen.value = false, call
cleanupOnboardingStorage(), and cancel/flag any in-flight promises so functions
like setInternalBootApplySucceeded do nothing if isOpen is false (apply the same
guard to the other similar apply flow in the 128-149 region).
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts (1)
864-872:⚠️ Potential issue | 🟡 MinorPin these cases to the baseline-unavailable branch.
Both tests leave
updateSshSettingsMockat the default{}payload, so they can still go green through the SSH-unverified fallback and never prove the offline/baseline-unavailable path. Seed a verified disabled-SSH response here and assert the baseline-unavailable copy, otherwise a regression in that branch stays hidden.🧪 Suggested hardening
apply: () => { coreSettingsResult.value = null; coreSettingsError.value = new Error('Graphql is offline.'); + updateSshSettingsMock.mockResolvedValue({ + data: { + updateSshSettings: { id: 'vars', useSsh: false, portssh: 22 }, + }, + }); },As per coding guidelines, "Mock external services and API calls".
Also applies to: 952-962
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts` around lines 864 - 872, The two test cases with caseName 'baseline unavailable — shows best-effort (completion deferred to NextSteps)' (and the similar one at lines 952-962) are leaving updateSshSettingsMock at its default empty payload so the SSH-unverified fallback can mask the baseline-unavailable branch; modify the apply block to set updateSshSettingsMock to a verified disabled-SSH response (e.g. a payload indicating verified: true and sshEnabled: false) alongside setting coreSettingsResult.value = null and coreSettingsError.value = new Error(...), then update the assertExpected to assert the baseline-unavailable copy (e.g. that wrapper.text() contains the specific baseline-unavailable message) and that completeOnboardingMock was not called to ensure the offline/baseline-unavailable path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts`:
- Around line 293-300: The test only verifies the dialog was removed but not
that the onboarding cleanup was invoked; after triggering the shared dialog
dismissal (the click on '[data-testid="dialog-dismiss"]' in the test that uses
mountComponent()), add an assertion that the cleanupOnboardingStorageMock was
called (e.g. expect(cleanupOnboardingStorageMock).toHaveBeenCalled()) to ensure
the same cleanup handler runs for the update:modelValue=false dismiss path as
for the X/result-close path.
---
Duplicate comments:
In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts`:
- Around line 864-872: The two test cases with caseName 'baseline unavailable —
shows best-effort (completion deferred to NextSteps)' (and the similar one at
lines 952-962) are leaving updateSshSettingsMock at its default empty payload so
the SSH-unverified fallback can mask the baseline-unavailable branch; modify the
apply block to set updateSshSettingsMock to a verified disabled-SSH response
(e.g. a payload indicating verified: true and sshEnabled: false) alongside
setting coreSettingsResult.value = null and coreSettingsError.value = new
Error(...), then update the assertExpected to assert the baseline-unavailable
copy (e.g. that wrapper.text() contains the specific baseline-unavailable
message) and that completeOnboardingMock was not called to ensure the
offline/baseline-unavailable path is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5be93dcd-d4f1-49db-b25d-53948858ca85
📒 Files selected for processing (2)
web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.tsweb/__test__/components/Onboarding/OnboardingSummaryStep.test.ts
Comment on lines
+293
to
+300
| it('closes when the shared dialog requests dismissal', async () => { | ||
| const wrapper = mountComponent(); | ||
|
|
||
| await wrapper.get('[data-testid="dialog-dismiss"]').trigger('click'); | ||
| await flushPromises(); | ||
|
|
||
| expect(wrapper.find('[data-testid="dialog-stub"]').exists()).toBe(false); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert cleanup on dialog-driven dismissals too.
This close path only proves that the dialog disappears. If update:modelValue=false skips the same cleanup handler as the X button/result-close path, stale onboarding draft state will survive and this suite will still pass. Please assert cleanupOnboardingStorageMock here as well.
🧹 Suggested assertion
await wrapper.get('[data-testid="dialog-dismiss"]').trigger('click');
await flushPromises();
+ expect(cleanupOnboardingStorageMock).toHaveBeenCalledTimes(1);
expect(wrapper.find('[data-testid="dialog-stub"]').exists()).toBe(false);Based on learnings, "Test component behavior and output, not implementation details".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('closes when the shared dialog requests dismissal', async () => { | |
| const wrapper = mountComponent(); | |
| await wrapper.get('[data-testid="dialog-dismiss"]').trigger('click'); | |
| await flushPromises(); | |
| expect(wrapper.find('[data-testid="dialog-stub"]').exists()).toBe(false); | |
| }); | |
| it('closes when the shared dialog requests dismissal', async () => { | |
| const wrapper = mountComponent(); | |
| await wrapper.get('[data-testid="dialog-dismiss"]').trigger('click'); | |
| await flushPromises(); | |
| expect(cleanupOnboardingStorageMock).toHaveBeenCalledTimes(1); | |
| expect(wrapper.find('[data-testid="dialog-stub"]').exists()).toBe(false); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/__test__/components/Onboarding/OnboardingInternalBootStandalone.test.ts`
around lines 293 - 300, The test only verifies the dialog was removed but not
that the onboarding cleanup was invoked; after triggering the shared dialog
dismissal (the click on '[data-testid="dialog-dismiss"]' in the test that uses
mountComponent()), add an assertion that the cleanupOnboardingStorageMock was
called (e.g. expect(cleanupOnboardingStorageMock).toHaveBeenCalled()) to ensure
the same cleanup handler runs for the update:modelValue=false dismiss path as
for the X/result-close path.
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
eol-lastviolation ingql/index.tsvi.fnmock type in standalone test to match the real two-argument signaturesleepMshelper instead of inlinePromise/setTimeoutReview fixes (post eng-review)
completeOnboarding()call: commit 5be53a4 moved completion to NextStepsStep; this PR had re-added it to SummaryStep. Removed the duplicate — completion stays exclusively in NextStepsStep.handleClose()now callscleanupOnboardingStorage()to prevent stale internal boot state from persisting in localStorage after the wizard is dismissed.settingsAppliedsuccess log: the success log line in onboarding summary was accidentally removed during the composable extraction; restored it.getErrorMessagehelper: eliminated duplicate error-message extraction logic between SummaryStep and the internalBoot composable.Testing
610 tests pass, 6 skipped across 64 test files.
Notes
Commits
4dcb99cfeat(web): add standalone internal boot wizard8daf70bchore(web): remove auth-request deploy patching648a985fix(web): defer onboarding refresh until modal closeacb1816fix(web): add missing trailing newline to gql/index.ts3fd8358test(web): loosen brittle error message assertion in SummaryStep0fe7a0etest(web): align applyInternalBootSelection mock type with real signature9128f15fix(web): warn when BIOS boot order update cannot be verified6af4125refactor(web): use sleepMs helper instead of inline Promise/setTimeout1abd331fix(web): remove duplicate completeOnboarding from SummaryStep8efa4c3fix(web): clear draft store when standalone internal boot wizard closes734417erefactor(web): extract shared getErrorMessage helper from internalBoot composable488d189test(web): add missing coverage for standalone boot wizard and composableSummary by CodeRabbit
New Features
Tests
Chores
Localization