fix(web): allow OS update/reboot UI when registration key state has an error#2029
fix(web): allow OS update/reboot UI when registration key state has an error#2029elibosley wants to merge 8 commits into
Conversation
The update confirmation body and footer were gated on `!stateDataError`, which suppressed the entire confirm UI whenever the server had any state error (e.g. an EGUID key/GUID mismatch). A standalone OS update on such a server rendered a blank "install update" modal with no confirm button, even though OS update eligibility is independent of key validity. Swap the guard to the existing `showPostInstallKeyError` computed, which is only true after a key install left the server in an error state. This preserves the original intent (prioritize the key error in the combined key-install + update flow) without blocking standalone updates. Add tests covering the standalone-update-with-state-error case and the combined key-install error case.
|
Warning Review limit reached
More reviews will be available in 42 minutes and 54 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA new ChangesCentralize OS Update Status Logic
Sequence DiagramsequenceDiagram
participant Component as HeaderOsVersion/<br/>Status/DropdownContent
participant Composable as useOsUpdateStatus
participant ServerStore as serverStore
participant UpdateOsStore as updateOsStore
Component->>Composable: useOsUpdateStatus()
Composable->>ServerStore: read regUpdatesExpired, stateDataError, rebootType
Composable->>UpdateOsStore: read available, availableWithRenewal
Composable->>Composable: compute entitlementExpired
Composable->>Composable: compute blockedByKeyState
Composable->>Composable: compute updateAvailable
Composable->>Composable: compute rebootRequired
Composable-->>Component: return flags + underlying refs
Component->>Component: render UI based on flags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: e080f53fb5
ℹ️ 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".
| </template> | ||
|
|
||
| <template v-if="updateOsStatus === 'confirming' && !stateDataError"> | ||
| <template v-if="updateOsStatus === 'confirming' && !showPostInstallKeyError"> |
There was a problem hiding this comment.
In a combined key-install + OS-update callback, actOnUpdateOsAction can put updateOsStatus into confirming before the delayed refreshServerState reconciliation has finished. If the server still has the pre-refresh stateDataError, showPostInstallKeyError is false while callbackCallsCompleted is false, so this predicate now exposes the confirm button and lets the user start the OS update before we know whether the key install actually cleared the license error. Keep state errors suppressing the update confirmation while a key-install reconciliation is still pending.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/__test__/components/CallbackFeedback.test.ts (1)
352-352: 💤 Low valueConsider clarifying the inline comment.
The comment says "where the install errored" but the test setup has
keyInstallStatus.value = 'success'. The test is actually covering the scenario where the key install succeeded but the server remains in an error state afterward (post-install reconciliation error, such as EGUID mismatch). The test name "when a key install left the server in an error state" is more accurate.Consider rewording to match the test name or clarify: "where the server remains in error state after install" or "where post-install reconciliation shows an error".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/__test__/components/CallbackFeedback.test.ts` at line 352, The inline comment at the test for key-install and update flow incorrectly describes the scenario as "where the install errored" but the test setup shows keyInstallStatus.value is set to 'success', meaning the install actually succeeded. Reword the comment to accurately reflect that while the key install succeeded, the server remains in an error state afterward due to post-install reconciliation failure (such as EGUID mismatch), or similar wording that clarifies the server error state persists after a successful install rather than the install itself erroring.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/__test__/components/CallbackFeedback.test.ts`:
- Line 352: The inline comment at the test for key-install and update flow
incorrectly describes the scenario as "where the install errored" but the test
setup shows keyInstallStatus.value is set to 'success', meaning the install
actually succeeded. Reword the comment to accurately reflect that while the key
install succeeded, the server remains in an error state afterward due to
post-install reconciliation failure (such as EGUID mismatch), or similar wording
that clarifies the server error state persists after a successful install rather
than the install itself erroring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4c574ecd-0263-42a2-a352-5f6de5dfb58b
📒 Files selected for processing (2)
web/__test__/components/CallbackFeedback.test.tsweb/src/components/UserProfile/CallbackFeedback.vue
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2029 +/- ##
==========================================
+ Coverage 52.63% 52.75% +0.12%
==========================================
Files 1035 1036 +1
Lines 72034 72069 +35
Branches 8248 8298 +50
==========================================
+ Hits 37917 38022 +105
+ Misses 33991 33921 -70
Partials 126 126 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
OS update eligibility and a pending reboot are independent of registration /key validity (they key off the update-entitlement window, not key state), but two more spots gated this UI on `!stateDataError`: - HeaderOsVersion: the pending-reboot badge sat below the stateDataError early-return, so an EGUID server that had already installed an update showed no "reboot to apply" prompt. Surface rebootTypeText before the stateDataError gate. - DropdownContent: the update button (check-for-update / update-available / reboot-required) was hidden whenever the server had a state error. Show it regardless — users may update without a valid key. Add regression tests for both.
elibosley
changed the title
Distinguish registration/key-state errors (which should NOT gate updates) from an expired update entitlement (which SHOULD). The user-profile dropdown now omits the check-for-update / update-available button when regUpdatesExpired is true, leaving the renewal/eligibility link to stand in for it. A pending reboot still surfaces, since it applies an already-installed update.
Introduce a single source of truth for whether the server can check for / install an OS update and whether a reboot is pending, so the key-vs- entitlement rule lives in one place instead of being re-derived (and drifting) per component. The composable names the two gating concepts explicitly: - entitlementExpired (regUpdatesExpired) — DOES gate the update affordance - blockedByKeyState (stateDataError) — informational only, never gates Migrate the three components that previously re-derived this inline: - HeaderOsVersion: surface the update-available badge regardless of key state (consistent with the dropdown); reboot still shown first. - DropdownContent: read rebootRequired/entitlementExpired from the seam. - UpdateOs/Status: read rebootRequired/updateAvailable/entitlementExpired from the seam. Pure-display components (CheckUpdateResponseModal, Registration/* expiration, UpdateIneligible) consume availability/entitlement getters and never gate on key state, so they're left untouched. Add a unit test for the composable and update component tests.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/__test__/components/DropdownContent.test.ts (1)
110-130:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear mock call history between tests.
This suite uses shared
vi.fnmocks (including call-count assertions later), but doesn’t reset them each test run. Addvi.clearAllMocks()inbeforeEach/afterEachto keep cases isolated.
As per coding guidelines, "Reset mocks between tests withvi.clearAllMocks()".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/__test__/components/DropdownContent.test.ts` around lines 110 - 130, The beforeEach hook in this test suite initializes state values but does not reset the shared vi.fn mock call history, which causes test isolation issues and can lead to incorrect call-count assertions. Add a call to vi.clearAllMocks() within the beforeEach hook to ensure all mocks are cleared between test runs, maintaining proper test isolation.Source: Coding guidelines
web/__test__/components/HeaderOsVersion.test.ts (1)
122-132:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest setup doesn’t exercise the stateDataError path described by the case.
On Line 131 you set
errorsStore.errors, but this component’s prior gating concern wasserverStore.stateDataError. This test can still pass ifstateDataErrorgating regresses. Use server-state inputs that actually producestateDataErrorfor this scenario (similar to Line 147’s EGUID setup) while keeping update availability true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/__test__/components/HeaderOsVersion.test.ts` around lines 122 - 132, The test currently sets errorsStore.errors to simulate a state error, but the component's actual gating logic checks serverStore.stateDataError, so this test doesn't exercise the path it claims to test. Modify the test setup to use server-state inputs that actually produce stateDataError (following the pattern at Line 147 with EGUID setup), while still keeping serverStore.updateOsResponse available to verify the update badge renders correctly despite the state error.
🧹 Nitpick comments (2)
web/__test__/components/DropdownContent.test.ts (1)
146-181: ⚡ Quick winUse less copy-coupled assertions for update/reboot visibility.
The new checks depend on exact rendered phrases (
"Check for Update","OS Update Eligibility Expired","Reboot Required for Update"). Prefer behavior-oriented assertions that won’t fail on minor wording/translation edits.
As per coding guidelines, "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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/__test__/components/DropdownContent.test.ts` around lines 146 - 181, The test assertions in the three test cases are tightly coupled to exact rendered text strings like "Check for Update", "OS Update Eligibility Expired", and "Reboot Required for Update". Replace these text-based assertions with behavior-oriented checks that verify the presence or absence of specific items based on their purpose or identifying properties (such as data attributes, item keys, or other non-text properties) rather than their displayed text. This will make the tests resilient to translation changes and minor wording updates while still verifying the correct behavior based on the serverStoreRefs values (regUpdatesExpired and rebootType).Source: Coding guidelines
web/__test__/components/HeaderOsVersion.test.ts (1)
141-153: ⚡ Quick winAvoid asserting full literal status copy in behavior tests.
These assertions couple to exact wording (
"Unraid OS 6.13.0 Update Available","Reboot Required for Update"), which makes tests fragile to copy-only changes. Prefer asserting behavior/state (status control exists for update/reboot condition) with less copy-sensitive selectors/expectations.
As per coding guidelines, "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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/__test__/components/HeaderOsVersion.test.ts` around lines 141 - 153, The test assertions in the pending-reboot badge test are checking for exact literal strings in title attributes and text content, which makes the tests brittle to copy changes. Replace the assertions checking for exact text like `title="Unraid OS 6.13.0 Update Available"` and `title="Reboot Required for Update"` with assertions that verify the behavior/state instead, such as checking for the presence of status badge elements using less copy-sensitive selectors like class names, data attributes, or role attributes. Similarly, replace the `toContain` assertion that checks for the exact text `'Reboot Required for Update'` with an assertion that verifies the status control exists for the reboot condition without depending on the specific wording.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/__test__/components/DropdownContent.test.ts`:
- Around line 110-130: The beforeEach hook in this test suite initializes state
values but does not reset the shared vi.fn mock call history, which causes test
isolation issues and can lead to incorrect call-count assertions. Add a call to
vi.clearAllMocks() within the beforeEach hook to ensure all mocks are cleared
between test runs, maintaining proper test isolation.
In `@web/__test__/components/HeaderOsVersion.test.ts`:
- Around line 122-132: The test currently sets errorsStore.errors to simulate a
state error, but the component's actual gating logic checks
serverStore.stateDataError, so this test doesn't exercise the path it claims to
test. Modify the test setup to use server-state inputs that actually produce
stateDataError (following the pattern at Line 147 with EGUID setup), while still
keeping serverStore.updateOsResponse available to verify the update badge
renders correctly despite the state error.
---
Nitpick comments:
In `@web/__test__/components/DropdownContent.test.ts`:
- Around line 146-181: The test assertions in the three test cases are tightly
coupled to exact rendered text strings like "Check for Update", "OS Update
Eligibility Expired", and "Reboot Required for Update". Replace these text-based
assertions with behavior-oriented checks that verify the presence or absence of
specific items based on their purpose or identifying properties (such as data
attributes, item keys, or other non-text properties) rather than their displayed
text. This will make the tests resilient to translation changes and minor
wording updates while still verifying the correct behavior based on the
serverStoreRefs values (regUpdatesExpired and rebootType).
In `@web/__test__/components/HeaderOsVersion.test.ts`:
- Around line 141-153: The test assertions in the pending-reboot badge test are
checking for exact literal strings in title attributes and text content, which
makes the tests brittle to copy changes. Replace the assertions checking for
exact text like `title="Unraid OS 6.13.0 Update Available"` and `title="Reboot
Required for Update"` with assertions that verify the behavior/state instead,
such as checking for the presence of status badge elements using less
copy-sensitive selectors like class names, data attributes, or role attributes.
Similarly, replace the `toContain` assertion that checks for the exact text
`'Reboot Required for Update'` with an assertion that verifies the status
control exists for the reboot condition without depending on the specific
wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a3c86ea6-97fa-4613-b4f7-79be9d086d2f
📒 Files selected for processing (7)
web/__test__/components/DropdownContent.test.tsweb/__test__/components/HeaderOsVersion.test.tsweb/__test__/composables/useOsUpdateStatus.test.tsweb/src/components/HeaderOsVersion.standalone.vueweb/src/components/UpdateOs/Status.vueweb/src/components/UserProfile/DropdownContent.vueweb/src/composables/useOsUpdateStatus.ts
The test hits a real network endpoint (expired-cert.badssl.com) and can exceed the 5s default in CI. Bump to 20s for headroom.
…ey reconciliation In a combined key-install + OS-update callback, the confirm button was gated only on showPostInstallKeyError, which is false until callbackCallsCompleted. During the delayed refreshServerState reconciliation window the pre-refresh stateDataError still exposed the confirm button, letting the user start an OS update before we knew the key install cleared the license error. Introduce keyErrorBlocksUpdate (stateDataError + a resolved key install in this callback) and gate the confirm UI on it so the confirmation stays suppressed through the pending window, not just after completion.
…e tests - HeaderOsVersion: drive the state-error precondition via state='EGUID' (which sets stateData.error) instead of errorsStore.errors, which never feeds stateDataError. Assert stateDataError is defined so the badge test actually exercises the path it names. - DropdownContent: clear shared mock call history in beforeEach for isolation.
A different network-dependent test in the suite timed out at 5s in CI after the per-test bump. These integration tests all make real outbound calls (discovery, SSL/DNS probes), so raise the timeout for the whole suite instead of one test.
1 participant
Problem
OS update eligibility is intentionally decoupled from key validity — it's gated on the update-entitlement window (
regUpdatesExpired), not registration state. But several places in the web UI used!stateDataError(a registration-error flag derived from thestateenum, e.g.EGUIDkey/GUID mismatch) as a proxy for "can't update". On a server with a mismatched key, this stranded users at various points in the update/reboot flow.Users may update without a valid key, and that's intended.
Fixes (three latent copies of the same conflation)
CallbackFeedback.vue — the update-confirm modal body + buttons were gated on
!stateDataError, so a standalone update on anEGUIDserver rendered a blank "install update" modal with no Confirm button. Swapped to!showPostInstallKeyError(only true after a key install left the server in an error state), preserving the combined key-install + update flow's intent.HeaderOsVersion.standalone.vue — the pending-reboot badge sat below the
stateDataErrorearly-return inupdateOsStatus, so anEGUIDserver that had already installed an update showed no "reboot to apply" prompt. Moved therebootTypeTextcheck above the gate — a pending reboot applies an already-installed update and must always surface.DropdownContent.vue — the user-profile dropdown's update button (check-for-update / update-available / reboot-required) was hidden whenever the server had a state error. Removed the
!stateDataErrorgate so it shows regardless.Tests
CallbackFeedback.test.ts: confirm UI renders with a state error but no key install; confirm click installs; suppressed after a key-install error.HeaderOsVersion.test.ts: reboot badge renders withstate = 'EGUID'+rebootType = 'update'.DropdownContent.test.ts: "Check for Update" item present whenstateDataErroris set.All affected suites pass (17 tests); lint clean. Web-only; no change to update-eligibility logic itself.
Follow-up
A broader standardization — making the server authoritative for "can update / reboot required / eligibility" so the client stops recomputing this in ~35 scattered places — was discussed and is out of scope here. Tracked separately.
Related: CLD-517
Summary by CodeRabbit