feat(notify): persistent/keyed notifications; reboot notice -> bell (OS-471)#2676
Open
elibosley wants to merge 10 commits into
Open
feat(notify): persistent/keyed notifications; reboot notice -> bell (OS-471)#2676elibosley wants to merge 10 commits into
elibosley wants to merge 10 commits into
Conversation
…the bell
Adds the producer side of persistent ("sticky until resolved") notifications so
the API notification bell can replace the legacy floating yellow banner.
- webGui/scripts/notify:
- `add` gains `-k <key>` (stable key; keyed notices use the key as the filename
so re-raising is idempotent) and `-p` (persistent: written as persistent=true,
not user-archivable, and not copied to the archive folder).
- new `notify clear -k <key>` resolves a keyed notification.
- PluginAPI.php: the reboot-required notice now also raises a persistent,
keyed (`reboot-required`) ALERT notification, and clears it once no reboot
reasons remain. It also clears naturally on reboot (RAM-backed /tmp is wiped).
Pairs with unraid/api#2033 (persistent + key fields, clearNotificationByKey).
Part of OS-471. Follow-up: migrate the remaining banner call sites
(boot-device-corrupt -> persistent; transient -> toaster) and remove the
.upgrade_notice element.
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe ChangesStable-key persistent notifications integration
Sequence DiagramsequenceDiagram
participant Plugin
participant PluginAPI as PluginAPI.php
participant HeadJS as HeadInlineJS.php
participant NotifyPHP as Notify.php
participant NotifyScript as notify script
participant ActiveDir as active dir
participant WebUI as Web UI Toast
Plugin->>PluginAPI: addRebootNotice(message)
PluginAPI->>NotifyScript: notify add -k reboot-required -p
NotifyScript->>ActiveDir: write reboot-required.notify
HeadJS->>HeadJS: addBannerWarning(text, noDismiss)
alt persistent banner (noDismiss)
HeadJS->>NotifyPHP: POST action:add<br/>-k keyHash -p
NotifyPHP->>NotifyScript: notify add -k keyHash -p
NotifyScript->>ActiveDir: write keyHash.notify
else transient banner
HeadJS->>WebUI: window.toast(text, actions)
end
Plugin->>PluginAPI: removeRebootNotice(message)
PluginAPI->>NotifyScript: notify clear -k reboot-required
NotifyScript->>ActiveDir: delete reboot-required.notify
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![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
🤖 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 `@emhttp/plugins/dynamix/scripts/notify`:
- Line 270: The early break statement when checking if an archive file exists
prevents the processing of new unread notifications for keyed notifications that
already have an archived version. Instead of breaking immediately when a file
exists, modify the logic to continue processing when dealing with keyed
notifications that may have new unread content. The archive existence check
should not short-circuit the entire operation for keyed re-raise scenarios where
the same key may need to raise new notifications even if a previous archive
exists. Restructure the conditional logic to differentiate between keyed and
non-keyed notifications, ensuring that keyed notifications can proceed through
the notification handling flow regardless of existing archive files.
🪄 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: 6da37b25-77fe-44e1-ab4e-8a026b4073ef
📒 Files selected for processing (2)
emhttp/plugins/dynamix.plugin.manager/scripts/PluginAPI.phpemhttp/plugins/dynamix/scripts/notify
Two bugs found while deploying to a live server: - PluginAPI.php called `notify add -p -k ...`; PHP getopt() stops at the first non-option arg, so the literal `add` made it parse zero options. The script dispatches to 'add' whenever the first arg is an option, so call it options-first (no `add` literal). - `notify clear` parsed its key with getopt(), which likewise stops at the 'clear' sub-command. Parse argv directly to support `clear <key>` and `clear -k <key>`. Verified on hardware: `notify -p -k reboot-required ...` writes a persistent keyed notification and `notify clear -k reboot-required` removes it (safe_filename maps the key symmetrically for add + clear). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ved copy The legacy "if archived copy exists, skip" dedup blocked re-raising a keyed condition after the user dismissed (archived) it - so a still-true condition could never re-pin. Keyed notifications now clear any stale archived copy and proceed, so re-raising re-pins and keeps the archive clean. Non-keyed notifications keep the legacy dedup behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Retire the fixed yellow .upgrade_notice bar by routing addBannerWarning by intent: - Persistent conditions (noDismiss, not a transient "forced" in-progress banner) -> a keyed notification in the bell via Notify.php (idempotent re-raise, header-reserved, survives reloads, clears when resolved). Boot-device-corrupt now routes here too. - Everything else -> a transient toast (globalThis.toast), with any banner link preserved as a toast action button. Notify.php add gains -p/-k/-l passthrough plus a clear (clear-by-key) command. Reboot notices are no longer double-shown: they rely solely on the PluginAPI "reboot-required" bell notification (dropped the client banner + on-load re-display). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Banner page raises a persistent, keyed bell notification (key=webgui-pr-PR_PLACEHOLDER) instead of a banner. Clear it in the plugin removal script so uninstalling the PR test plugin also removes the notification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Legacy banners (CA's "Action Centre Enabled", boot checks, ...) routed to the bell have no explicit clear: they re-render on every page load while active and simply stop when resolved, so a persistent bell notification would stick forever. Stamp each banner -> bell notification with a per-page-load generation, and sweep any 'banner-' keyed notification not re-raised in the current generation once the page settles. Resolved banners now clear on the next navigation. Non-banner keys (PR plugins, reboot-required) own their lifecycle and are left untouched. - notify: add -g <gen> on add; add `banner-sweep <gen>` subcommand - Notify.php: forward -g on add; add cmd=banner-sweep - HeadInlineJS: stamp bell POST with a per-load bannerGen; sweep 3s after load Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Move banner reconciliation to the backend (authoritative, triggered when the
notification drawer loads). The page's role is reduced to:
- stamp each banner -> bell notification with a per-page-load generation
(notify add -g <gen>), so the API can tell which banners were re-raised
this load
- expose window.bannerGen for the drawer to hand to the API
Drop the page-side banner-sweep (notify subcommand, Notify.php command, and the
HeadInlineJS post-load timer) in favor of the API's reconcileBannerNotifications
mutation (unraid/api). The -g stamp on `notify add` is retained.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The generic "This stays in your notifications until the condition is resolved" description was filler - the "Active" badge already conveys persistence - and rendered as a redundant second line. Send an empty description so the bell card shows only the banner message (paired with the web fix that hides empty lines). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
|
CodeRabbit fix-loop resolution:
|
Persistent (-p) notifications now write to $path/active/ instead of unread/, so they can be retrieved cheaply and are never swept by bulk archive/delete. To keep the legacy nchan poller working, `notify get` unions active/ with unread/. `clear -k` and `init` sweep active/ too, and a raise clears any stale twin left in the other store. No migration needed — persistent notifications regenerate on page load. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Condition-style ("active") notifications are derived state, re-raised on every
page load. They were written to $path/active, but $path follows the user-facing
"Store notifications to boot drive" setting -- so on systems with that enabled,
a condition notice was persisted to flash and stranded forever once its
condition was gone (e.g. the plugin that raised it was removed).
Hardcode the active dir to /tmp/notifications/active, decoupled from $path.
A reboot now wipes condition notifications and the next page load re-raises only
those still true -- making "stuck forever" structurally impossible without any
generation-sweep reconciliation. Event notifications (unread/archive) still
honor the flash setting via $path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
Producer side of persistent ("sticky until resolved") notifications — the webgui half of replacing the floating yellow banner with the API notification bell (OS-471). Pairs with unraid/api#2033 (which adds the
persistent/keyfields, idempotent keyed create, andclearNotificationByKey).Changes
webGui/scripts/notify(canonical file;webGui/scripts/notifyis a symlink to it):addgains-k <key>— a stable key; keyed notices use the key as the filename so re-raising is idempotent (no dupes).addgains-p— persistent: written aspersistent=true, and not copied to the archive folder (it isn't an archival event).notify clear -k <key>— resolves a keyed notification (removes it from the bell).usage()updated.PluginAPI.php— the reboot-required notice now also raises a persistent, keyed (reboot-required) ALERT notification, and clears it once no reboot reasons remain. It also clears naturally on reboot (RAM-backed/tmpis wiped and the bell repopulates from files).How it combines with the API PR
The API watches
/tmp/notifications/unread/*.notify; these new fields flow straight through:notify add -p -k reboot-required …→ a sticky ALERT in the bell that the user can't dismiss and that auto-resolves. The API'sgetLegacyScriptArgsalready passes-k/-p, so API-originated persistent notifications use this same script.Validation
php -lclean on both files.Scope / follow-ups
This wires the clearest condition (reboot-required) end to end. Remaining banner call sites — boot-device-corrupt → persistent, and transient ones (popup blocked, etc.) → toaster — plus removing the
.upgrade_noticeelement, are the documented next step.🤖 Generated with Claude Code
Summary by CodeRabbit
notify clear -kto dismiss specific keyed alerts.