feat(notifications): persistent "sticky until resolved" notifications (OS-471)#2033
Open
elibosley wants to merge 29 commits into
Open
feat(notifications): persistent "sticky until resolved" notifications (OS-471)#2033elibosley wants to merge 29 commits into
elibosley wants to merge 29 commits into
Conversation
Adds a persistent notification capability so condition-style alerts (reboot required, boot device corrupt, etc.) can be surfaced in the notification bell and stay until their condition resolves — replacing the legacy webgui floating yellow banner. API: - Notification/NotificationData gain `persistent` (Boolean) and `key` (stable producer key). Severity stays INFO/WARNING/ALERT (ALERT already drives the red bell), so persistence is orthogonal to severity. - .notify file format + service read/write the new fields. - Keyed creates are idempotent (raising with an existing key replaces it), and `clearNotificationByKey(key)` resolves a condition (removes matching unread). - Persistent notifications are not user-archivable: archiveNotification rejects them and bulk archiveAll skips them. Web: - NotificationFragment exposes `persistent`; the bell Item and the legacy-embedded CriticalNotifications hide the dismiss/archive control for persistent alerts and show "Clears automatically when resolved". - Regenerated GraphQL schema + web codegen. Part of OS-471. Pairs with the webgui PR that routes the yellow-banner conditions through persistent notifications. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 ChangesPersistent / Condition-style Notifications
Sequence Diagram(s)sequenceDiagram
participant Client
participant NotificationsResolver
participant NotificationsService
participant FileSystem
rect rgba(135, 206, 235, 0.5)
Note over Client,FileSystem: Create persistent notification (idempotent)
Client->>NotificationsResolver: createNotification(data { key, persistent: true })
NotificationsResolver->>NotificationsService: createNotification(data)
NotificationsService->>FileSystem: clearNotificationsByKey(data.key)
FileSystem-->>NotificationsService: deleted matching unread files
NotificationsService->>FileSystem: write new .notify file
NotificationsService-->>Client: NotificationOverview
end
rect rgba(255, 165, 0, 0.5)
Note over Client,FileSystem: Clear persistent notification by key
Client->>NotificationsResolver: clearNotificationByKey(key)
NotificationsResolver->>NotificationsService: clearNotificationsByKey(key)
NotificationsService->>FileSystem: load unread, filter by key, delete matches
NotificationsService-->>Client: NotificationOverview
end
rect rgba(220, 100, 100, 0.5)
Note over Client,FileSystem: Archive blocked for persistent
Client->>NotificationsResolver: archiveNotification(id)
NotificationsResolver->>NotificationsService: archiveNotification(id)
NotificationsService->>FileSystem: read .notify (persistent=true)
NotificationsService-->>Client: 409 Conflict
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 863661de40
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (1)
633-638:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
archiveAll(no importance filter) still attempts persistent notifications.This branch archives every unread ID, so persistent notifications hit the 409 path in
archiveNotificationinstead of being skipped up front.Proposed fix
public async archiveAll(importance?: NotificationImportance) { const { UNREAD } = this.paths(); if (!importance) { - await readdir(UNREAD).then((ids) => this.archiveIds(ids)); + const unreadPaths = await this.listFilesInFolder(UNREAD); + const [loaded] = await this.loadNotificationsFromPaths(unreadPaths, { + type: NotificationType.UNREAD, + }); + const archivableIds = loaded.filter((n) => !n.persistent).map((n) => n.id); + await this.archiveIds(archivableIds); return { overview: NotificationsService.overview }; }🤖 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 `@api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts` around lines 633 - 638, In the archiveAll method of NotificationsService, when importance is not provided, the method currently archives all unread notification IDs without filtering out persistent notifications. This causes persistent notifications to reach the archiveNotification method where they encounter a 409 error instead of being skipped upfront. Filter the ids array obtained from readdir(UNREAD) to exclude persistent notifications before passing them to archiveIds. You can check if a notification is persistent by examining its properties before including it in the archive operation.
🤖 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 `@api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts`:
- Around line 302-307: The createNotification method performs a
clear-then-create operation for keyed notifications that is not atomic, allowing
concurrent requests with the same key to bypass idempotency and create
duplicates. Refactor this method to wrap the clearNotificationsByKey and
notification creation operations in a single atomic database transaction, or
better yet, replace the separate clear and create calls with a single atomic
upsert operation at the database level that ensures only one notification with a
given key exists after the operation completes.
---
Outside diff comments:
In `@api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts`:
- Around line 633-638: In the archiveAll method of NotificationsService, when
importance is not provided, the method currently archives all unread
notification IDs without filtering out persistent notifications. This causes
persistent notifications to reach the archiveNotification method where they
encounter a 409 error instead of being skipped upfront. Filter the ids array
obtained from readdir(UNREAD) to exclude persistent notifications before passing
them to archiveIds. You can check if a notification is persistent by examining
its properties before including it in the archive operation.
🪄 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: f785abf8-da18-4d80-a7cb-f98beed713ee
📒 Files selected for processing (11)
api/generated-schema.graphqlapi/src/core/types/states/notification.tsapi/src/unraid-api/graph/resolvers/notifications/notifications.model.tsapi/src/unraid-api/graph/resolvers/notifications/notifications.resolver.tsapi/src/unraid-api/graph/resolvers/notifications/notifications.service.tsweb/__test__/store/notifications.test.tsweb/src/components/Notifications/CriticalNotifications.standalone.vueweb/src/components/Notifications/Item.vueweb/src/components/Notifications/graphql/notification.query.tsweb/src/composables/gql/gql.tsweb/src/composables/gql/graphql.ts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2033 +/- ##
==========================================
+ Coverage 52.77% 52.86% +0.09%
==========================================
Files 1035 1035
Lines 72060 72232 +172
Branches 8293 8305 +12
==========================================
+ Hits 38027 38183 +156
- Misses 33907 33923 +16
Partials 126 126 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Persistent (condition-style) notifications now sort above the rest in the notification list, regardless of timestamp, so sticky alerts stay visible. Display-only stable sort; the seen-tracking watcher still uses the unsorted list so it keys off the genuinely latest notification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ni reader The .notify ini reader coerces persistent="true" into a boolean true, so the strict string compare (persistent === 'true') always evaluated false and every notification came back persistent:false. Normalize via String(persistent) so both the coerced boolean and a literal "true" string resolve correctly. Found on hardware: the bell rendered the archive button and skipped the sticky-to-top sort because persistent was always false over GraphQL. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…en hierarchy - Persistent notifications now render as a pinned card: orange left-accent border, subtle tint, and an uppercase "Pinned" badge in the header. - Tighten the item typography to feel native to the webgui: text-sm base, muted/condensed description, smaller timestamp, less vertical gap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…arning Persistent notifications are no longer hard-locked. A single, explicit dismiss is allowed but gated behind a confirmation that makes clear it only hides the reminder (acknowledge) and does not resolve the underlying condition - which re-pins if the producer raises it again (idempotent key). - API: archiveNotification no longer rejects persistent notifications; bulk archiveAll still skips them so a casual "Archive all" can't hide pinned alerts. - Web: pinned items show a subdued "Dismiss" (X) action that opens a warning dialog (useConfirm) before archiving. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tyling - clearNotificationsByKey now removes matching entries from both unread AND archive, so dismissing then re-raising a keyed condition no longer leaves stale archived copies behind. - Give every notification the card treatment (rounded, bordered, padded), with normal items as a neutral variant and persistent ones as the orange pinned variant. Dropped the list divider in favor of spaced cards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…large solid buttons Archive and Delete used the default large solid Button; switch them to the same ghost + sm + muted styling as Dismiss so the row actions are proportionate and the panel reads lighter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the janky leave animation (card snapped to full width then slid) with a FLIP-based transition: the dismissed card slides out to the right and fades while the remaining cards glide up to close the gap. Leaving card is taken out of flow keeping its width + vertical position so siblings animate instead of jumping. Respects prefers-reduced-motion. Persistent items already render pinned (styling + top sort) in the archive tab too, since both key off `persistent`, not the tab. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Cleaner title (xl, semibold, tracking-tight). - Real segmented Unread/Archived tabs with an active pill (bg + shadow) and counts shown as subtle rounded pills instead of "(n)". - Divider under the controls to separate them from the list; consistent px-4 padding across header, tabs, filters, and list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…apse) Replace the absolute/FLIP leave (card floated out of flow while siblings jumped) with a Web Animations API leave hook: the dismissed card slides right and collapses its own height/margins/padding/borders in one motion, so the rows below flow up continuously. Honors prefers-reduced-motion. CSS still handles enter + reorder (move) transitions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t styling Replace the bare red-hover text links with muted ghost buttons carrying an Archive / Trash icon, matching the per-item actions. Delete all keeps the destructive hover. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The unfiltered "Archive all" path called archiveIds() over every unread file with no persistent filter, so persistent (pinned) notifications were swept into the archive. Only the per-importance path skipped them. Unify both paths to load and filter out persistent notifications before archiving. Add a regression test covering filtered and unfiltered archiveAll. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/src/components/Notifications/List.vue (1)
123-125: ⚡ Quick winAvoid
as HTMLElementin transition hook.This cast is avoidable; narrow the type at runtime before using element-specific APIs.
Proposed change
function onLeave(el: Element, done: () => void) { - const node = el as HTMLElement; + if (!(el instanceof HTMLElement)) { + done(); + return; + } + const node = el;As per coding guidelines, “Avoid using casting whenever possible, prefer proper typing from the start.”
🤖 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/Notifications/List.vue` around lines 123 - 125, The onLeave function uses an unsafe type cast with `as HTMLElement` to convert the Element parameter to HTMLElement. Remove this cast and instead narrow the type at runtime by checking if the element is an instance of HTMLElement before using any HTMLElement-specific APIs on it. This approach ensures proper typing without relying on casting, following the coding guideline to avoid casts whenever possible.Source: Coding guidelines
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (1)
478-493: 💤 Low valueConsider batching deletes for consistency with other bulk operations.
The sequential
awaitin the for-loop works but differs from thebatchProcesspattern used elsewhere (e.g.,archiveAll). While condition-style notifications are typically few, batching would improve consistency and handle edge cases with many keyed notifications more efficiently.♻️ Suggested refactor using batchProcess
public async clearNotificationsByKey(key: string): Promise<NotificationOverview> { // Sweep both states: a keyed condition may have been dismissed (archived) by // the user, so clearing/resolving it - or re-raising it idempotently - should // not leave stale copies behind in either unread or archive. for (const type of [NotificationType.UNREAD, NotificationType.ARCHIVE]) { const list = await this.getNotifications({ type, offset: 0, limit: Number.MAX_SAFE_INTEGER, }); - for (const notification of list.filter((n) => n.key === key)) { - await this.deleteNotification({ id: notification.id, type }); - } + const toDelete = list.filter((n) => n.key === key); + await batchProcess(toDelete, (notification) => + this.deleteNotification({ id: notification.id, type }) + ); } return this.getOverview(); }🤖 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 `@api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts` around lines 478 - 493, The clearNotificationsByKey method currently deletes notifications sequentially with individual awaits in a for-loop, which differs from the batchProcess pattern used elsewhere in the codebase like in archiveAll. Instead of awaiting each deleteNotification call individually, refactor the method to collect all notification IDs that need deletion (from both UNREAD and ARCHIVE types where key matches), then use the batchProcess pattern to delete them in batches for consistency and better efficiency.
🤖 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 `@api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts`:
- Around line 478-493: The clearNotificationsByKey method currently deletes
notifications sequentially with individual awaits in a for-loop, which differs
from the batchProcess pattern used elsewhere in the codebase like in archiveAll.
Instead of awaiting each deleteNotification call individually, refactor the
method to collect all notification IDs that need deletion (from both UNREAD and
ARCHIVE types where key matches), then use the batchProcess pattern to delete
them in batches for consistency and better efficiency.
In `@web/src/components/Notifications/List.vue`:
- Around line 123-125: The onLeave function uses an unsafe type cast with `as
HTMLElement` to convert the Element parameter to HTMLElement. Remove this cast
and instead narrow the type at runtime by checking if the element is an instance
of HTMLElement before using any HTMLElement-specific APIs on it. This approach
ensures proper typing without relying on casting, following the coding guideline
to avoid casts whenever possible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9af5efba-f4cb-4b16-b00f-09053235fbba
📒 Files selected for processing (6)
api/src/unraid-api/graph/resolvers/notifications/notifications.service.spec.tsapi/src/unraid-api/graph/resolvers/notifications/notifications.service.tsweb/src/components/Notifications/Item.vueweb/src/components/Notifications/List.vueweb/src/components/Notifications/Sidebar.vueweb/src/locales/en.json
The pinned card's heavy orange treatment (orange wash, thick orange border, orange badge) read as alarming. Replace with a neutral elevated card: subtle left accent, muted background, and a quiet "Pinned" pill with a Pin icon. Rename the persistent action from "Dismiss" to "Archive" (it archives, gated behind a confirm) and use the archive-box icon for consistency with the regular archive action. Rename confirmDismiss i18n keys to confirmArchive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Restore the orange pinned-notification treatment (preferred) and instead recolor the per-notification "View" link from text-primary (orange) to a neutral secondary-foreground with a foreground hover, on all notifications. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
deleteNotifications() emptied the entire directory and zeroed the overview, so the "Delete all" action (deleteArchivedNotifications) wiped persistent notifications too. Make it selective: load entries, delete only the non-persistent ones, and recompute stats from what remains. Tests previously relied on deleteAllNotifications() for isolation; since that now preserves persistent entries, switch setup/teardown to a hard emptyDir wipe so persistent test data can't leak between tests. Add a regression test covering Delete-all on the archive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Persistent (condition-style) notifications are re-raised by their producer on every page load, so archiving them is futile — the keyed re-raise clears the archived copy and re-pins them. Remove the archive action entirely for persistent items; they clear only when their condition resolves (by key). Drops the now-unused confirm-archive flow and its i18n keys. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pinned (persistent) notifications re-fire on every page load and can't be archived, so a long-running condition (e.g. multiple PR test plugins) can crowd the unread list. Add a "Pinned" toggle next to the importance filter, on by default, that filters persistent notifications out of the list (both unread and archive tabs) when switched off. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wrap the Pinned toggle in the same segmented container and use the same neutral active/inactive button styles as the importance filter, instead of the standalone orange chip, so the two filters read as one consistent row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The importance filters are text-only, so remove the Pin icon (and its gap) from the Pinned toggle to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rename the user-facing badge and filter from "Pinned" to "Active" (clearer that these track an ongoing condition, not a user-pinned item). Keep `persistent` as the internal data/API term; rename the UI state/prop to showPersistent to map directly to the field it filters on. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Importance and "Active" filter selections were plain refs, so they reset on every webgui page navigation (each a full reload). Back them with sessionStorage so the chosen filters survive page changes within the tab. '' is the All-Types sentinel, mapped back to undefined for the query. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/Notifications/Item.vue (1)
118-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTimestamp text should use the same fallback as the tooltip.
Line 126 renders only
reformattedTimestamp. Iftimestampis missing butformattedTimestampexists, the visible timestamp is empty even though Line 118 has a fallback.Suggested fix
- <p class="text-secondary-foreground text-xs whitespace-nowrap">{{ reformattedTimestamp }}</p> + <p class="text-secondary-foreground text-xs whitespace-nowrap"> + {{ formattedTimestamp ?? reformattedTimestamp }} + </p>🤖 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/Notifications/Item.vue` around lines 118 - 127, The timestamp display text on line 126 uses only reformattedTimestamp and will be empty if timestamp is missing but formattedTimestamp exists, even though the tooltip attribute on line 118 has a proper fallback pattern. Update the paragraph element that displays the timestamp to use the same fallback logic as the title attribute: change the template binding from reformattedTimestamp alone to formattedTimestamp ?? reformattedTimestamp to ensure consistent behavior between the visible timestamp and the tooltip.
🤖 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/Notifications/Sidebar.vue`:
- Around line 48-53: Remove the two explanatory comments in the filter state
setup section in Sidebar.vue. The first comment beginning with "Filter
selections persist for the browser session" above the importance
useSessionStorage declaration and the second comment starting with "Persistent
("Active") notifications are shown by default" above the showPersistent
useSessionStorage declaration should both be deleted. Keep the variable
declarations themselves (importance and showPersistent) intact, removing only
the accompanying comments to align with the repository standards that discourage
unnecessary explanatory comments.
---
Outside diff comments:
In `@web/src/components/Notifications/Item.vue`:
- Around line 118-127: The timestamp display text on line 126 uses only
reformattedTimestamp and will be empty if timestamp is missing but
formattedTimestamp exists, even though the tooltip attribute on line 118 has a
proper fallback pattern. Update the paragraph element that displays the
timestamp to use the same fallback logic as the title attribute: change the
template binding from reformattedTimestamp alone to formattedTimestamp ??
reformattedTimestamp to ensure consistent behavior between the visible timestamp
and the tooltip.
🪄 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: 662628d2-3094-4b3d-a20a-6bd3df480a22
📒 Files selected for processing (6)
api/src/unraid-api/graph/resolvers/notifications/notifications.service.spec.tsapi/src/unraid-api/graph/resolvers/notifications/notifications.service.tsweb/src/components/Notifications/Item.vueweb/src/components/Notifications/List.vueweb/src/components/Notifications/Sidebar.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 (2)
- web/src/components/Notifications/List.vue
- api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts
Make the status tab controlled via v-model backed by sessionStorage so the selected Unread/Archived view sticks across webgui page navigations, matching the importance and Active filters. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Make Archive-all / Delete-all icon-only (title + sr-only) so it no longer dominates the tab row. - Merge the importance chips and the Active toggle into a single segmented bar with a divider; shorten "All Types" to "All" and scroll horizontally on narrow widths so filters never wrap to a second line. - Move the set-once notification Settings link out of the filter row into a slim footer pinned at the bottom of the panel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Restore the text label on Archive-all / Delete-all (still compact: text-xs, h-8, tight padding) so the action is self-explanatory. - Move the notification Settings gear out of the footer and back into the header title row (offset left of the sheet close button); drop the footer to reclaim the vertical space. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The gear floated in the top-right near the sheet's X close button without aligning to it, looking cluttered. Place it inline right after the "Notifications" title (size-7, muted) so it aligns with the title and stays clear of the close button. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add reconcileBannerNotifications(currentGeneration) mutation. It clears any unread "banner-" keyed notification whose stored generation no longer matches the page's current page-load generation - i.e. a legacy JS banner (CA, boot checks, ...) whose producer stopped rendering it. Non-banner keys (PR plugins, reboot-required) own their lifecycle and are untouched. The notification drawer calls it on open with the page's window.bannerGen (set by the webgui), so reconciliation happens authoritatively in the backend when notifications are loaded, instead of via a page-side timer. Reads the new `gen` ini field. Adds a regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A notification with no description (or no subject) rendered an empty line, leaving an awkward blank row. Guard both with v-if so only populated fields take vertical space. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Drop @isnotempty from the description field (Notification + NotificationData). Banner/condition notifications legitimately have no description, and requiring one made them fail validation and render as "invalid". Empty is now valid; the UI hides the line. - Reconcile stale banner notifications on the indicator's mount (deferred until the page settles), so the badge self-corrects on every navigation, not only when the bell is opened. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- List.vue onLeave: narrow the transition element via `instanceof HTMLElement` instead of an `as HTMLElement` cast (per coding guidelines). - Item.vue: render `reformattedTimestamp || formattedTimestamp` so the visible timestamp isn't empty when `timestamp` is missing but `formattedTimestamp` exists, matching the tooltip fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
|
CodeRabbit fix-loop resolutions (review-body / outside-diff findings):
|
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
Adds a persistent notification capability so condition-style alerts (reboot required, boot device corrupt, …) can live in the notification bell and stay until their condition resolves — the API foundation for retiring the legacy webgui floating yellow banner (OS-471).
Model
Notification/NotificationDatagainpersistent: Booleanandkey: String(a stable producer key).archiveNotificationrejects persistent ones (409) and bulkarchiveAllskips them.keyreplaces the prior instance (latest wins, no dupes).clearNotificationByKey(key)mutation removes matching unread notifications when the condition clears.Changes
API
notifications.model.ts—persistent+keyonNotificationandNotificationData.core/types/states/notification.ts+notifications.service.ts— read/write the new fields in the.notifyfile; idempotent keyed create;clearNotificationsByKey; archive protection; legacynotifyscript args gain-k/-p.notifications.resolver.ts—clearNotificationByKeymutation.generated-schema.graphql.Web
NotificationFragmentexposespersistent; the bellItemand the legacy-embeddedCriticalNotificationshide the dismiss/archive control for persistent alerts and show "Clears automatically when resolved."Validation
tsc --noEmit✅, webvue-tsc --noEmit✅, GraphQL codegen ✅ (both api + web).keyintentionally not in the web fragment (it collides with Vue's reservedkeyattribute and the UI doesn't need it — clearing is producer-side).Pairs with
The webgui PR (unraid/webgui) that adds
-k/-ptowebGui/scripts/notifyand routes the yellow-banner conditions (reboot-required, boot-device-corrupt) through persistent notifications. Combine via the diff-based PR test plugins.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes
UI Updates