[codex] Embed internal OS update checks on Tools page#2020
Draft
elibosley wants to merge 1 commit into
Draft
Conversation
Contributor
WalkthroughThis PR modularizes the UpdateOs component by removing Account-store integration, adding embedded modal rendering, implementing a dedicated refresh button, and updating tests to verify the new architecture. The standalone component now drives its own update-check and modal visibility on the tools update page via the updateOs store. ChangesUpdateOs Component Refactor and Modal Updates
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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/__test__/components/UpdateOs.test.ts (1)
87-87: ⚡ Quick winRename these test cases to remove path-based wording that no longer affects behavior.
Current names suggest pathname controls rendering, but assertions now only verify status rendering independent of path.
Suggested rename pattern
- it('shows the internal update status when path matches and rebootType is empty', async () => { + it('renders update status when rebootType is empty', async () => { - it('shows status when path does not match', async () => { + it('renders update status for non-Update paths', async () => { - it('shows status when rebootType is not empty', async () => { + it('renders update status when rebootType is set', async () => {As per coding guidelines, “Test what the code does, not implementation details.”
Also applies to: 107-107, 126-126
🤖 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/UpdateOs.test.ts` at line 87, Rename the three test case descriptions in UpdateOs.test.ts that reference path-based behavior (the it(...) strings at the tests currently starting at lines with "shows the internal update status when path matches and rebootType is empty" and the two similar cases at the other locations) so they describe what the test actually asserts: that the component renders the internal update status (and handles rebootType being empty) regardless of pathname; update the it(...) titles to something like "renders internal update status when rebootType is empty" (or equivalent concise wording) for each affected test so the names reflect behavior rather than path implementation details.
🤖 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.
Inline comments:
In `@web/__test__/components/UpdateOs.test.ts`:
- Around line 103-104: The test is asserting a prop on UpdateOsStatusStub which
only verifies the test stub, not the real UpdateOsStatus behavior; change the
test to either mount the real UpdateOsStatus component so you can assert its
runtime behavior (e.g., check emitted events or rendered DOM from
UpdateOsStatus) or remove the stub-prop assertion and instead assert observable
output — for example, keep
expect(wrapper.find('[data-testid="update-os-status"]').exists()).toBe(true) and
replace
expect(wrapper.findComponent(UpdateOsStatusStub).props('showUpdateCheck')).toBe(true)
with an assertion against the actual rendered DOM or emitted event from
UpdateOsStatus (or mount UpdateOs and assert its prop/state) using
wrapper.findComponent(UpdateOsStatus) or inspecting its emitted events.
---
Nitpick comments:
In `@web/__test__/components/UpdateOs.test.ts`:
- Line 87: Rename the three test case descriptions in UpdateOs.test.ts that
reference path-based behavior (the it(...) strings at the tests currently
starting at lines with "shows the internal update status when path matches and
rebootType is empty" and the two similar cases at the other locations) so they
describe what the test actually asserts: that the component renders the internal
update status (and handles rebootType being empty) regardless of pathname;
update the it(...) titles to something like "renders internal update status when
rebootType is empty" (or equivalent concise wording) for each affected test so
the names reflect behavior rather than path implementation details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8658f9da-443f-4dc7-9d07-2c18eb570b92
📒 Files selected for processing (4)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/Update.pageweb/__test__/components/UpdateOs.test.tsweb/src/components/UpdateOs.standalone.vueweb/src/components/UpdateOs/Status.vue
Comment on lines
+103
to
+104
| expect(wrapper.find('[data-testid="update-os-status"]').exists()).toBe(true); | ||
| expect(wrapper.findComponent(UpdateOsStatusStub).props('showUpdateCheck')).toBe(true); |
Contributor
There was a problem hiding this comment.
Avoid asserting a stub-only prop that the real component doesn’t define.
This assertion can pass even if production behavior is wrong, because it only verifies data passed into a test stub, not UpdateOsStatus runtime behavior.
Suggested change
- expect(wrapper.findComponent(UpdateOsStatusStub).props('showUpdateCheck')).toBe(true);
+ // Assert rendered behavior instead of stub-only prop plumbing.
+ expect(wrapper.find('[data-testid="update-os-status"]').exists()).toBe(true);As per coding guidelines, “Test what the code does, not implementation details like exact error message wording.”
📝 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
| expect(wrapper.find('[data-testid="update-os-status"]').exists()).toBe(true); | |
| expect(wrapper.findComponent(UpdateOsStatusStub).props('showUpdateCheck')).toBe(true); | |
| expect(wrapper.find('[data-testid="update-os-status"]').exists()).toBe(true); | |
| // Assert rendered behavior instead of stub-only prop plumbing. | |
| expect(wrapper.find('[data-testid="update-os-status"]').exists()).toBe(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/UpdateOs.test.ts` around lines 103 - 104, The test is
asserting a prop on UpdateOsStatusStub which only verifies the test stub, not
the real UpdateOsStatus behavior; change the test to either mount the real
UpdateOsStatus component so you can assert its runtime behavior (e.g., check
emitted events or rendered DOM from UpdateOsStatus) or remove the stub-prop
assertion and instead assert observable output — for example, keep
expect(wrapper.find('[data-testid="update-os-status"]').exists()).toBe(true) and
replace
expect(wrapper.findComponent(UpdateOsStatusStub).props('showUpdateCheck')).toBe(true)
with an assertion against the actual rendered DOM or emitted event from
UpdateOsStatus (or mount UpdateOs and assert its prop/state) using
wrapper.findComponent(UpdateOsStatus) or inspecting its emitted events.
elibosley
force-pushed
the
codex/internal-update-os-flow
branch
from
43ce321 to
6e81f99
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@web/src/components/UpdateOs.standalone.vue`:
- Around line 76-81: The parent passes a no-op prop show-update-check to the
UpdateOsStatus component; either remove that prop from the caller or wire it
into the child’s visibility logic. To fix: in UpdateOsStatus
(UpdateOs/Status.vue) add a prop showUpdateCheck (Boolean, default false) and
include it in the computed showRefreshButton (alongside updateAvailable,
showRebootButton, and props.showExternalDowngrade) so the “check for update”
button respects showUpdateCheck, or alternatively remove
:show-update-check="true" from the UpdateOs.standalone.vue usage to avoid
passing an unused prop (change whichever approach matches intent).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 00d566c2-7c8d-4fad-b6f3-d99534e21147
📒 Files selected for processing (4)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/Update.pageweb/__test__/components/UpdateOs.test.tsweb/src/components/UpdateOs.standalone.vueweb/src/components/UpdateOs/Status.vue
🚧 Files skipped from review as they are similar to previous changes (3)
- plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/Update.page
- web/src/components/UpdateOs/Status.vue
- web/test/components/UpdateOs.test.ts
Comment on lines
+76
to
+81
| <UpdateOsStatus | ||
| :show-update-check="true" | ||
| :title="t('updateOs.updateUnraidOs')" | ||
| :subtitle="subtitle" | ||
| /> | ||
| <UpdateOsThirdPartyDrivers v-if="rebootType === 'thirdPartyDriversDownloading'" /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm UpdateOsStatus declares and uses a showUpdateCheck prop
fd -i 'Status.vue' -p 'UpdateOs'
rg -nP -C3 '(showUpdateCheck|show-update-check)' -g '*Status.vue'
# Inspect the defineProps block for the component
ast-grep --pattern 'defineProps<$_>()'Repository: unraid/api
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find where UpdateOsStatus is defined/implemented
rg -n "UpdateOsStatus" web/src/components/UpdateOs.standalone.vue web/src/components -S
# 2) Locate any Status component that corresponds to UpdateOsStatus
fd -e vue -a -t f . web/src/components -S | head -n 200
# 3) Search for the prop name variants across the repo
rg -n "(showUpdateCheck|show-update-check)" web/src/components -S
rg -n "(showUpdateCheck|show-update-check)" web/src -S
# 4) Find the component file that likely declares showUpdateCheck
# If UpdateOsStatus is imported from another file, inspect that.
imports=$(rg -n "import\s+.*UpdateOsStatus.*from" web/src/components/UpdateOs.standalone.vue -S || true)
echo "$imports"
# 5) If we can locate the file, inspect defineProps/props usage for showUpdateCheck
# (we'll re-run with the discovered file if needed)Repository: unraid/api
Length of output: 715
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="web/src/components/UpdateOs/Status.vue"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "== wc -l =="
wc -l "$FILE"
echo "== occurrences of showUpdateCheck/show-update-check =="
rg -n "(showUpdateCheck|show-update-check)" "$FILE" -S || true
echo "== defineProps/props block context =="
# Show likely prop declaration area (covers both defineProps and classic props:)
rg -n -C5 "(defineProps|props\s*:)" "$FILE" -S || true
echo "== template usage around any showUpdateCheck reference =="
# If occurrences exist, show a bit more context for each match
rg -n -C8 "(showUpdateCheck|show-update-check)" "$FILE" -S || trueRepository: unraid/api
Length of output: 566
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="web/src/components/UpdateOs/Status.vue"
rg -n "(\$attrs|useAttrs|attrs\s*\.|attrs\s*\[|inheritAttrs)" "$FILE" -S || true
rg -n "show-update-check|showUpdateCheck" "$FILE" -S || trueRepository: unraid/api
Length of output: 36
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "(showUpdateCheck|show-update-check)" web/src/components/UpdateOs -S || true
rg -n "(showUpdateCheck|show-update-check)" web/src -S || trueRepository: unraid/api
Length of output: 132
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="web/src/components/UpdateOs/Status.vue"
echo "== occurrences: showRefreshButton =="
rg -n "showRefreshButton" "$FILE" -S || true
echo "== occurrences: updateCheck / update-check =="
rg -n "(updateCheck|update-check|checkFor|check for|updates)" "$FILE" -S || true
echo "== template region around any refresh-related UI =="
rg -n "refresh" "$FILE" -S || true
# show a small window around the first few refresh/update occurrences (if any)
FIRST=$(rg -n "(refresh|update|updates|check)" "$FILE" -S | head -n 1 | cut -d: -f1 || true)
if [ -n "${FIRST:-}" ]; then
START=$((FIRST-15))
END=$((FIRST+60))
[ $START -lt 1 ] && START=1
echo "== showing lines $START-$END around first match line $FIRST =="
sed -n "${START},${END}p" "$FILE" | nl -ba -w2 -s':'
fiRepository: unraid/api
Length of output: 771
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="web/src/components/UpdateOs/Status.vue"
echo "=== Props area (lines 1-60) ==="
sed -n '1,80p' "$FILE"
echo
echo "=== showRefreshButton computed area (lines 70-130) ==="
sed -n '70,140p' "$FILE"
echo
echo "=== Template refresh/update UI (lines 200-270) ==="
sed -n '200,280p' "$FILE"Repository: unraid/api
Length of output: 7665
show-update-check is not consumed by UpdateOsStatus (no-op)
<UpdateOsStatus
:show-update-check="true"
:title="t('updateOs.updateUnraidOs')"
:subtitle="subtitle"
/>web/src/components/UpdateOs/Status.vue does not declare showUpdateCheck in its props, and the “check for update” button is controlled by v-if="showRefreshButton" where showRefreshButton is computed from updateAvailable, showRebootButton, and props.showExternalDowngrade—not showUpdateCheck. Remove the prop from the parent or wire showUpdateCheck into the button visibility logic.
🤖 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/src/components/UpdateOs.standalone.vue` around lines 76 - 81, The parent
passes a no-op prop show-update-check to the UpdateOsStatus component; either
remove that prop from the caller or wire it into the child’s visibility logic.
To fix: in UpdateOsStatus (UpdateOs/Status.vue) add a prop showUpdateCheck
(Boolean, default false) and include it in the computed showRefreshButton
(alongside updateAvailable, showRebootButton, and props.showExternalDowngrade)
so the “check for update” button respects showUpdateCheck, or alternatively
remove :show-update-check="true" from the UpdateOs.standalone.vue usage to avoid
passing an unused prop (change whichever approach matches intent).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2020 +/- ##
==========================================
- Coverage 52.61% 52.60% -0.01%
==========================================
Files 1035 1035
Lines 72033 72174 +141
Branches 8239 8258 +19
==========================================
+ Hits 37901 37968 +67
- Misses 34006 34080 +74
Partials 126 126 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
elibosley
force-pushed
the
codex/internal-update-os-flow
branch
from
6e81f99 to
9d7219d
Compare
elibosley
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/src/components/UpdateOs/CheckUpdateResponseModal.vue (1)
287-430: 🏗️ Heavy liftEmbedded layout duplicates the ResponsiveModal content.
The embedded
CardWrapperblock (lines 288-429) is a near-verbatim copy of the innerResponsiveModalcontent (lines 440-610): the title/description header, loading state, update-highlight section, no-update content, extra-links, ignored-releases list, and the "more options" tooltip + action buttons are all repeated. The two copies have already diverged (see the missing ignore-release tooltip below), which is exactly the failure mode duplication invites. Consider extracting the shared body into a child component (or a single slot rendered inside eitherCardWrapperorResponsiveModal) so both modes stay in sync.🤖 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/src/components/UpdateOs/CheckUpdateResponseModal.vue` around lines 287 - 430, The CardWrapper block duplicates the same modal body rendered in ResponsiveModal (causing drift); extract the shared content (title/description, loading state, update/no-update sections, extraLinks, ignored releases, and action button area) into a single child component (e.g., UpdateCheckBody) or a named slot, and then render that component/slot inside both CardWrapper and ResponsiveModal; ensure the new component accepts and emits the same props/state used in the template (modalCopy, checkForUpdatesLoading, available, availableWithRenewal, ignoreThisRelease, userFormattedReleaseDate, availableRequiresAuth, extraLinks, updateOsIgnoredReleases, actionButtons, t, accountStore.updateOs()) so behavior and event bindings (including Switch v-model and BrandButton clicks) remain unchanged.
🤖 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.
Inline comments:
In `@web/src/components/UpdateOs/CheckUpdateResponseModal.vue`:
- Around line 415-427: The embedded rendering of actionButtons drops the tooltip
for the Close button when ignoreThisRelease is true; update the v-for block that
renders actionButtons so that when ignoreThisRelease is true and the current
item corresponds to the Close action (identify by item.key === 'close' or the
localized text match), wrap that BrandButton with the same Tooltip using the
translation key updateOs.checkUpdateResponseModal.youCanOptBackInTo; keep all
other props and the `@click` behavior intact. Ensure you reference the existing
actionButtons array, ignoreThisRelease flag, BrandButton component and the
translation key so the embedded branch mirrors the ResponsiveModal tooltip
behavior.
---
Nitpick comments:
In `@web/src/components/UpdateOs/CheckUpdateResponseModal.vue`:
- Around line 287-430: The CardWrapper block duplicates the same modal body
rendered in ResponsiveModal (causing drift); extract the shared content
(title/description, loading state, update/no-update sections, extraLinks,
ignored releases, and action button area) into a single child component (e.g.,
UpdateCheckBody) or a named slot, and then render that component/slot inside
both CardWrapper and ResponsiveModal; ensure the new component accepts and emits
the same props/state used in the template (modalCopy, checkForUpdatesLoading,
available, availableWithRenewal, ignoreThisRelease, userFormattedReleaseDate,
availableRequiresAuth, extraLinks, updateOsIgnoredReleases, actionButtons, t,
accountStore.updateOs()) so behavior and event bindings (including Switch
v-model and BrandButton clicks) remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3df4c64c-7cba-4789-86ab-b831bfe6a1ac
📒 Files selected for processing (4)
web/__test__/components/UpdateOs.test.tsweb/src/components/UpdateOs.standalone.vueweb/src/components/UpdateOs/CheckUpdateResponseModal.vueweb/src/components/UpdateOs/Status.vue
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/components/UpdateOs/Status.vue
- web/src/components/UpdateOs.standalone.vue
- web/test/components/UpdateOs.test.ts
Comment on lines
+415
to
+427
| <div v-if="actionButtons.length > 0" :class="cn('xs:!flex-row flex flex-col justify-end gap-3')"> | ||
| <BrandButton | ||
| v-for="item in actionButtons" | ||
| :key="item.text" | ||
| :variant="item.variant ?? undefined" | ||
| :icon="item.icon" | ||
| :icon-right="item.iconRight" | ||
| :icon-right-hover-display="item.iconRightHoverDisplay" | ||
| :text="item.text ?? ''" | ||
| :title="item.title ? item.title : undefined" | ||
| @click="item.click?.()" | ||
| /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Embedded mode drops the ignore-release tooltip on the close button.
In the ResponsiveModal branch, when ignoreThisRelease is true and the action is the localized Close button, the button is wrapped in a tooltip showing updateOs.checkUpdateResponseModal.youCanOptBackInTo (lines 575-595). The embedded branch renders all action buttons directly, so this guidance no longer appears when a release is being ignored from the Tools page. If this is intentional, ignore; otherwise mirror the conditional tooltip wrapper here.
🤖 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/src/components/UpdateOs/CheckUpdateResponseModal.vue` around lines 415 -
427, The embedded rendering of actionButtons drops the tooltip for the Close
button when ignoreThisRelease is true; update the v-for block that renders
actionButtons so that when ignoreThisRelease is true and the current item
corresponds to the Close action (identify by item.key === 'close' or the
localized text match), wrap that BrandButton with the same Tooltip using the
translation key updateOs.checkUpdateResponseModal.youCanOptBackInTo; keep all
other props and the `@click` behavior intact. Ensure you reference the existing
actionButtons array, ignoreThisRelease flag, BrandButton component and the
translation key so the embedded branch mirrors the ResponsiveModal tooltip
behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/Tools/Updateinstead of showing an account-app redirect prompt.Check for Updateaction visible even when an update is already available, alongside the update CTA.Why
Users should be able to check for OS updates and use the internal updater from Tools > Update OS without leaving the OS. The previous landing prompt pushed them to the account app, and the update-check response appeared as a popup instead of as part of the page workflow.
Validation
pnpm type-checkfromweb/pnpm exec vitest run __test__/components/UpdateOs.test.ts __test__/components/CheckUpdateResponseModal.test.tsfromweb/Summary by CodeRabbit
Release Notes
New Features
Bug Fixes