feat(web): consolidated single-component header for Unraid 7.3+ (fixes mobile overlap)#2037
feat(web): consolidated single-component header for Unraid 7.3+ (fixes mobile overlap)#2037elibosley wants to merge 6 commits into
Conversation
Introduce <unraid-header> (Header.standalone.vue) which owns the entire header in one web component: Unraid logo, OS/API version dropdown and reboot/update banner, server name, server status, notifications bell, and the account dropdown/avatar. The header was previously split across <unraid-header-os-version> and <unraid-user-profile>, with the user-profile cluster absolutely positioned over the logo/version area. On narrow screens the reboot banner, server title, and notification bell overlapped. The consolidated component lays everything out in a single mobile-first flex flow (stacked on mobile, two clusters justified apart on sm+), removing the overlap. Logo + version + reboot banner logic is extracted into a shared Header/HeaderVersion.vue used by both the new header and the legacy HeaderOsVersion.standalone.vue, so there is a single source of truth. The legacy multi-component header stays intact as the < 7.3 fallback. The consolidated header intentionally does not mount the deprecated CriticalNotifications popover (removed by the persistent-notifications work, #2033); persistent alerts now live in the bell. Gating lives in webgui Header.php via version_compare (separate change).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughIntroduces a consolidated ChangesHeader Consolidation Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ComponentRegistry
participant HeaderStandalone
participant useServerProp
participant ServerStore
participant ArrayUsage
participant ApolloClient
ComponentRegistry->>HeaderStandalone: mount with server prop
HeaderStandalone->>useServerProp: getServer()
useServerProp->>ServerStore: setServer(parsed data)
useServerProp->>useServerProp: callbackStore.watcher()
HeaderStandalone->>ArrayUsage: render (if showArrayUsage)
ArrayUsage->>ApolloClient: useQuery(ARRAY_CAPACITY_QUERY)
ApolloClient-->>ArrayUsage: array capacity data
ArrayUsage->>ArrayUsage: compute usedPercent, barColorClass
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Companion webgui gate/mount PR: unraid/webgui#2689 (renders |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2037 +/- ##
==========================================
+ Coverage 52.77% 52.88% +0.10%
==========================================
Files 1035 1041 +6
Lines 72060 72241 +181
Branches 8303 8322 +19
==========================================
+ Hits 38031 38204 +173
- Misses 33903 33911 +8
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. |
Replace the legacy webgui my_usage() widget (jQuery-injected into #array-usage-sidenav for sidebar themes) with a self-contained ArrayUsage.vue inside <unraid-header>, driven by the GraphQL array.capacity field. Shown when webgui passes show-array-usage (sidebar theme + usage display enabled), matching where the legacy widget appeared. Color thresholds follow the legacy usage_color() defaults (70% warning / 90% critical).
Rework the consolidated header onto a CSS grid so a single account-actions block (bell + menu/avatar) repositions across the sm breakpoint without duplicating stateful components: - mobile: logo and account icons share the top row (app-style), then version + reboot banner, then array usage/status, then server name. - sm+: two columns — logo/version left, status over name+actions right (the prior desktop look). Splits the logo into Header/HeaderLogo.vue so it can sit on the account row on mobile; HeaderVersion.vue now renders only the version dropdown + reboot banner. HeaderOsVersion.standalone.vue (the < 7.3 fallback) composes both and is visually unchanged. Fixes the over-tall, left-adrift mobile header from the first pass.
<unraid-header> defaulted to display:inline, so the header grid shrink-wrapped
and the account controls bunched against the logo instead of spanning the row.
Add a global unraid-header.unapi { display:block; width:100% } rule (matching the
existing unraid-modals.unapi precedent).
Move the server status (uptime + license tier) up: it shares the top row with the logo and account controls once there's horizontal room (>=480px), and sits just under them on the narrowest screens. Version and server name drop down below. Tightens the vertical rhythm.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
web/src/components/Header/HeaderVersion.vue (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant comments.
Several comments here restate what the code already expresses (e.g. "Initialize all stores", "Track if we've loaded the versions yet", "Use lazy query and only load when dropdown is opened"). Consider dropping the ones that don't add clarity.
As per coding guidelines: "Never add comments unless they are needed for clarity of function".
Also applies to: 45-51, 59-59
🤖 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/Header/HeaderVersion.vue` at line 36, Remove the redundant comments in HeaderVersion.vue that only restate the surrounding code, including the ones near the store initialization and the versions-loading logic. Keep only comments that add non-obvious clarification, and delete the rest around the relevant setup and query code in the HeaderVersion component.Source: Coding guidelines
web/src/components/Header/HeaderLogo.vue (1)
8-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
querySelector('.logo')removes the class from only the first match.If more than one legacy
.logoelement can exist in the host DOM, only the first is cleaned up. If a single element is guaranteed this is fine; otherwise usequerySelectorAlland iterate. Also note this mutates DOM outside the component's own tree, which is fragile if the surrounding markup changes.🤖 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/Header/HeaderLogo.vue` around lines 8 - 13, The cleanup in HeaderLogo’s onMounted/nextTick block only removes the legacy class from the first `.logo` match, so update it to handle all matching legacy elements if multiple can exist by iterating over the full result set instead of a single querySelector result. Also keep the logic scoped to the component’s own rendered markup where possible, since querying the global document is brittle; use the HeaderLogo wrapper or a local ref if available.web/src/components/Header/ArrayUsage.vue (1)
22-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winQuery errors are silently swallowed.
Only
resultis destructured; if the query errors, the bar simply hides (hasDatabecomes falsy) with no diagnostic signal. Consider also destructuringerrorand logging it for observability.♻️ Proposed fix
-const { result } = useQuery(ARRAY_CAPACITY_QUERY, null, { +const { result, error } = useQuery(ARRAY_CAPACITY_QUERY, null, { pollInterval: 30_000, fetchPolicy: 'cache-and-network', }); + +watch(error, (err) => { + if (err) console.error('[ArrayUsage] failed to fetch array capacity', err); +});🤖 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/Header/ArrayUsage.vue` around lines 22 - 27, The array capacity query in ArrayUsage.vue is swallowing failures because only useQuery(...).result is used and any query error just makes the bar disappear. Update the useQuery destructuring to also capture error, then add logging/diagnostic handling in the same component so query failures are observable even when array data is missing. Keep the existing computed array usage, but wire error handling around ARRAY_CAPACITY_QUERY in ArrayUsage.
🤖 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/Header.test.ts`:
- Around line 197-201: The Header test is over-constrained by checking the exact
thrown message text. Update the test in Header.test.ts so it only asserts that
mounting Header without the server prop throws, using the existing mount/Header
setup, and remove the exact string expectation to make the assertion resilient
to wording changes.
In `@web/src/components/Header.standalone.vue`:
- Around line 98-102: The Header.standalone.vue template is rendering the server
description through v-html without sanitization, which can expose stored XSS.
Update the description flow used by the header: sanitize or escape the value
before it reaches the template, and ensure the serverStore description
assignment from data.description does not pass raw HTML through unchanged. Keep
the change localized around the description handling in the header component and
the store field that feeds it.
In `@web/src/components/Header/HeaderVersion.vue`:
- Around line 83-85: The openApiChangelog helper in HeaderVersion.vue opens a
new tab with window.open but does not prevent reverse tabnabbing. Update the
window.open call to explicitly use noopener (and keep _blank) so the GitHub
releases page cannot access window.opener; the fix belongs in the
openApiChangelog function.
---
Nitpick comments:
In `@web/src/components/Header/ArrayUsage.vue`:
- Around line 22-27: The array capacity query in ArrayUsage.vue is swallowing
failures because only useQuery(...).result is used and any query error just
makes the bar disappear. Update the useQuery destructuring to also capture
error, then add logging/diagnostic handling in the same component so query
failures are observable even when array data is missing. Keep the existing
computed array usage, but wire error handling around ARRAY_CAPACITY_QUERY in
ArrayUsage.
In `@web/src/components/Header/HeaderLogo.vue`:
- Around line 8-13: The cleanup in HeaderLogo’s onMounted/nextTick block only
removes the legacy class from the first `.logo` match, so update it to handle
all matching legacy elements if multiple can exist by iterating over the full
result set instead of a single querySelector result. Also keep the logic scoped
to the component’s own rendered markup where possible, since querying the global
document is brittle; use the HeaderLogo wrapper or a local ref if available.
In `@web/src/components/Header/HeaderVersion.vue`:
- Line 36: Remove the redundant comments in HeaderVersion.vue that only restate
the surrounding code, including the ones near the store initialization and the
versions-loading logic. Keep only comments that add non-obvious clarification,
and delete the rest around the relevant setup and query code in the
HeaderVersion component.
🪄 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: f09bf1e7-2145-4745-b4b2-4bc9635fd693
📒 Files selected for processing (14)
web/__test__/components/Header.test.tsweb/__test__/components/Wrapper/component-registry.test.tsweb/components.d.tsweb/src/components/Header.standalone.vueweb/src/components/Header/ArrayUsage.vueweb/src/components/Header/HeaderLogo.vueweb/src/components/Header/HeaderVersion.vueweb/src/components/Header/arrayCapacity.query.tsweb/src/components/Header/useServerProp.tsweb/src/components/HeaderOsVersion.standalone.vueweb/src/components/Wrapper/component-registry.tsweb/src/composables/gql/gql.tsweb/src/composables/gql/graphql.tsweb/src/locales/en.json
| it('throws when the server prop is missing', () => { | ||
| expect(() => mount(Header, { props: {}, global: { plugins: [pinia], stubs } })).toThrow( | ||
| 'Server data not present' | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Avoid asserting the exact error message text.
Per coding guidelines, tests should assert that an error is thrown, not the exact wording, to avoid breaking on incidental message changes.
✏️ Proposed fix
it('throws when the server prop is missing', () => {
- expect(() => mount(Header, { props: {}, global: { plugins: [pinia], stubs } })).toThrow(
- 'Server data not present'
- );
+ expect(() => mount(Header, { props: {}, global: { plugins: [pinia], stubs } })).toThrow();
});📝 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.
| it('throws when the server prop is missing', () => { | |
| expect(() => mount(Header, { props: {}, global: { plugins: [pinia], stubs } })).toThrow( | |
| 'Server data not present' | |
| ); | |
| }); | |
| it('throws when the server prop is missing', () => { | |
| expect(() => mount(Header, { props: {}, global: { plugins: [pinia], stubs } })).toThrow(); | |
| }); |
🤖 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/Header.test.ts` around lines 197 - 201, The Header
test is over-constrained by checking the exact thrown message text. Update the
test in Header.test.ts so it only asserts that mounting Header without the
server prop throws, using the existing mount/Header setup, and remove the exact
string expectation to make the assertion resilient to wording changes.
Source: Coding guidelines
| <template v-if="description && theme?.descriptionShow"> | ||
| <span | ||
| class="hidden truncate text-right text-base md:!inline-flex md:!items-center" | ||
| v-html="description" | ||
| /> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if description is sanitized anywhere before or during storage/hydration
rg -n -A3 -B3 'description' web/src/components/Header/useServerProp.ts web/src/store/server.ts 2>/dev/null
rg -n 'sanitize|DOMPurify' web/src -g '*.ts' -g '*.vue'Repository: unraid/api
Length of output: 7246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first
ast-grep outline web/src/components/Header.standalone.vue --view expanded
ast-grep outline web/src/store/server.ts --view expanded
ast-grep outline web/src/components/Header/useServerProp.ts --view expanded 2>/dev/null || true
# Read the relevant sections around description flow and template rendering
sed -n '1,180p' web/src/components/Header.standalone.vue
sed -n '1,220p' web/src/components/Header/useServerProp.ts
sed -n '1040,1085p' web/src/store/server.ts
# Look for any sanitization or HTML escaping specifically tied to description/header rendering
rg -n -A3 -B3 'v-html|description|sanitize|DOMPurify|escapeHtml|innerHTML' web/src/components web/src/store web/src/composables web/src/helpers -g '*.ts' -g '*.vue'Repository: unraid/api
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the server contract and any description-related helpers in smaller slices
sed -n '1,220p' web/types/server.ts
sed -n '1,260p' web/src/store/server.ts
sed -n '1,120p' web/src/components/UserProfile.standalone.vue
# Focused search for the server description field and sanitization around the path
rg -n -A2 -B2 'descriptionShow|server\.description|data\.description|description' web/types web/src/components/Header web/src/components/UserProfile web/src/store web/src/composables web/src/helpers -g '*.ts' -g '*.vue'Repository: unraid/api
Length of output: 50366
Sanitize the server description before v-html
web/src/components/Header.standalone.vue:98-102 renders serverStore.description as raw HTML, and the store copies data.description through unchanged. If that field accepts admin-entered HTML, this is stored XSS for every viewer; escape it or sanitize it first.
🤖 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/Header.standalone.vue` around lines 98 - 102, The
Header.standalone.vue template is rendering the server description through
v-html without sanitization, which can expose stored XSS. Update the description
flow used by the header: sanitize or escape the value before it reaches the
template, and ensure the serverStore description assignment from
data.description does not pass raw HTML through unchanged. Keep the change
localized around the description handling in the header component and the store
field that feeds it.
| const openApiChangelog = () => { | ||
| window.open('https://github.com/unraid/api/releases', '_blank'); | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add noopener to window.open to prevent reverse tabnabbing.
Unlike anchor target="_blank" (where modern browsers imply noopener), window.open does not apply it automatically, so the opened page can reach back via window.opener.
🛡️ Proposed fix
const openApiChangelog = () => {
- window.open('https://github.com/unraid/api/releases', '_blank');
+ window.open('https://github.com/unraid/api/releases', '_blank', 'noopener');
};📝 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.
| const openApiChangelog = () => { | |
| window.open('https://github.com/unraid/api/releases', '_blank'); | |
| }; | |
| const openApiChangelog = () => { | |
| window.open('https://github.com/unraid/api/releases', '_blank', 'noopener'); | |
| }; |
🤖 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/Header/HeaderVersion.vue` around lines 83 - 85, The
openApiChangelog helper in HeaderVersion.vue opens a new tab with window.open
but does not prevent reverse tabnabbing. Update the window.open call to
explicitly use noopener (and keep _blank) so the GitHub releases page cannot
access window.opener; the fix belongs in the openApiChangelog function.
Use a single two-row grid at every breakpoint instead of a separate mobile arrangement: logo top-left, uptime/license status top-right, version bottom-left, and name + notifications + account menu bottom-right. The logo column is minmax(0,1fr) and the logo image is max-w-full so it shrinks on narrow screens rather than overflowing, keeping the desktop structure responsive.
1 participant
Summary
Consolidates the Unraid header into a single web component (
<unraid-header>,Header.standalone.vue) that owns the entire header and its responsive layout, and fixes the broken mobile header where the "Reboot Required for Update" banner, the server-name title, and the notification bell overlapped on narrow screens.Gated to Unraid 7.3+; older releases fall back to the existing multi-component header. The gate lives in
webguiHeader.php(see companion PR).Before: header split across components
HeaderOsVersion.standalone.vue(<unraid-header-os-version>)Header.php#array-usage-sidenavHeader.phpUserProfile.standalone.vue(<unraid-user-profile>)myservers2.phpUserProfilewasabsolute top-0 right-0 pl-[30%]positioned over the logo/version area. On narrow screens the reboot banner (left) and the title + bell (absolutely positioned right) collided.After: one component, one layout
<unraid-header>composes all of it in a single mobile-first flex flow:flex-col— logo/version/banner stacks above server-status/name/bell/avatar (no overlap; the bell/avatar rowflex-wraps).sm+:flex-row … justify-between— logo cluster left, account cluster right (the prior desktop look).No cluster is absolutely positioned anymore, which is the actual fix.
Reuse / no duplication
Header/HeaderVersion.vue, now used by both<unraid-header>and the legacyHeaderOsVersion.standalone.vue— single source of truth.Notifications/Sidebar.vue), and the account dropdown are the existingUserProfile/*children, reused as-is.Header/useServerProp.ts.Backwards compatibility (7.3+ gate)
<unraid-header-os-version>+<unraid-user-profile>components are unchanged and remain the< 7.3fallback.version_compare($osVersion, '7.3', '>=')inwebguiHeader.php(companion PR), mirroring the docker-overview 7.3+ approach.Coordination with persistent-notifications (#2033)
The consolidated header does not mount the deprecated
CriticalNotificationspopover. Per #2033 (OS-471) persistent/"pinned" alerts now live in the notification bell, so the header uses only the surviving bell (Notifications/Sidebar.vue) and does not reintroduce the popover being retired.Testing
Header.test.ts(new): renders all regions in one component; hydrates server store; asserts the responsive layout invariant (mobile-firstflex-col→sm:flex-row, no absolute-positioned cluster); LAN-IP copy.component-registry.test.tspriority-order assertion.HeaderOsVersion/UserProfiletests still pass unchanged (extraction preserves output).pnpm buildsucceeds —<unraid-header>compiles to its own chunk;HeaderVersionis a shared chunk.Note
Live mobile rendering should be spot-checked once the dev server is running (I don't start it per repo convention). Companion webgui PR wires the mount + gate.
Summary by CodeRabbit
New Features
Bug Fixes