feat: top-bar notification control + Cmd+K test notification#282
Merged
Conversation
Add a bell icon button + dropdown to the top bar for managing Web Push, alongside the existing connection dot (kept as a separate signal). The bell reflects subscription state (filled U+F0F3 when subscribed, bell-slash U+F1F6 otherwise) and hides itself entirely when push is unsupported (insecure context / no service worker). The dropdown offers "Enable notifications" and "Send test notification". - lib/push.ts: add sendTestNotification() — a client-side registration.showNotification() that bypasses the server and the push service, isolating the OS-delivery leg for diagnosis. Harden getPushState()/enablePushSubscription() with a 3s timeout on serviceWorker.ready so the indicator can never hang. - use-push-subscription.ts: expose enable/sendTest; add the "Notifications: Send test notification" Cmd+K palette action. - top-bar.tsx: NotificationControl component (mirrors TerminalFontControl's button+popover pattern). Tests: push.test.ts sendTestNotification cases; top-bar.test.tsx NotificationControl block (state glyph, hidden-when-unsupported, dropdown actions, Escape-to-close). Full suite 731 green, tsc clean.
Add docs/site/notifications.md — a setup & troubleshooting guide centered on the common "it says sent but nothing appears" case (almost always an OS-level block: macOS Focus, blocked site permission, insecure context, or a subpath proxy not exposing /sw.js at the root). Wire it into the bell dropdown as a "Notifications help…" link (opens the GitHub-rendered page in a new tab) and reference it from the README's Push notifications section. Adds a top-bar test asserting the help link's href/target/rel.
There was a problem hiding this comment.
Pull request overview
Adds an in-app Web Push management surface following the Web Push feature from #270: a top-bar bell/dropdown to enable push + send a local OS-delivery test notification, plus a troubleshooting doc page linked from the UI and README. This helps distinguish “server/push-service worked” from “OS/browser suppressed delivery” (e.g., Focus mode), and avoids hangs by timeout-guarding the service worker readiness wait.
Changes:
- Add
NotificationControl(bell + dropdown) in the top bar and aCmd+Kpalette action to send a local test notification when subscribed. - Add
sendTestNotification()(client-sideregistration.showNotification()) and timeout-guardserviceWorker.readyreads. - Add/refresh documentation and tests covering the new push behaviors and UI.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates push opt-in instructions and links to the new notifications troubleshooting guide. |
| docs/site/notifications.md | New setup + troubleshooting guide for Web Push delivery issues. |
| app/frontend/src/lib/push.ts | Adds timeout-guarded SW readiness helper and sendTestNotification(). |
| app/frontend/src/lib/push.test.ts | Adds unit tests for sendTestNotification() and extends SW mocks. |
| app/frontend/src/hooks/use-push-subscription.ts | Extends hook to expose enable + sendTest handlers and adds palette test action when subscribed. |
| app/frontend/src/components/top-bar.tsx | Adds the top-bar NotificationControl bell + dropdown and help link. |
| app/frontend/src/components/top-bar.test.tsx | Adds UI tests for NotificationControl and mocks push lib for deterministic states. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+76
to
+79
| return Promise.race([ | ||
| navigator.serviceWorker.ready.catch(() => null), | ||
| new Promise<null>((resolve) => setTimeout(() => resolve(null), timeoutMs)), | ||
| ]); |
Collaborator
Author
There was a problem hiding this comment.
Fixed in 2923999 — readyRegistration now clears the timeout in a finally block when serviceWorker.ready wins, so the fast path no longer leaves a dangling timer.
Comment on lines
+732
to
+738
| const statusLabel = subscribed | ||
| ? "Notifications: on" | ||
| : denied | ||
| ? "Blocked in browser settings" | ||
| : "Notifications: off"; | ||
| const ariaLabel = subscribed ? "Notifications on" : "Notifications off"; | ||
|
|
Collaborator
Author
There was a problem hiding this comment.
Fixed in 2923999 — the trigger aria-label now announces "Notifications blocked" in the denied state (distinct from "Notifications off"); added a regression test.
- push.ts readyRegistration: clear the timeout when serviceWorker.ready wins the race, so the common (fast) path no longer leaves a dangling 3s timer on every call (e.g. mount-time state polling). - top-bar NotificationControl: the trigger aria-label now announces "Notifications blocked" in the denied state (was indistinguishable from plain "Notifications off" to screen readers). Added a regression test for the denied a11y label.
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
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
Follow-up to the merged Web Push feature (#270). Adds a real top-bar notification control (bell icon button + dropdown) for managing Web Push, a client-side test notification reachable from both the dropdown and
Cmd+K, and a troubleshooting help page linked from the dropdown.Motivated by a live debugging session: a real
rk notifyreportedsent:1(server + push service worked) but no OS notification appeared — the gap was OS-level delivery (macOS Focus mode), with no easy in-app way to test it, see subscription state, or learn how to fix it. This adds all three.What changed
Top-bar
NotificationControl(top-bar.tsx) — a Nerd Font bell button (filledU+F0F3when subscribed, bell-slashU+F1F6otherwise), styled like the neighboringTerminalFontControl/ThemeToggle. Sits alongside the existing SSE connection dot (kept as a separate signal, not merged). Hides itself entirely when push is unsupported. Dropdown: status line + Enable notifications + Send test notification (disabled until subscribed) + Notifications help… link.Client-side test send (
lib/push.tssendTestNotification) — a localregistration.showNotification()that bypasses the server and the push service, isolating the OS-delivery leg. A successful test proves SW + OS work; a failing one points at an OS-level block.Cmd+Kparity (use-push-subscription.ts) — addsNotifications: Send test notificationto the command palette (when subscribed).Hardening —
getPushState()/enablePushSubscription()wrap theserviceWorker.readywait with a 3s timeout so the indicator can never hang.Help page (
docs/site/notifications.md) — setup + the common "it says sent but nothing appears" troubleshooting (macOS/Windows notification settings, secure-context requirement, iOS PWA caveat, subpath-proxy caveat, how the chain works). Linked from the bell dropdown (opens the GitHub-rendered page in a new tab) and from the README's Push notifications section.Design notes
Cmd+Ktest action).blob/main/...GitHub URL, so it resolves once this merges tomain(matching how the README referencesdocs/site/pages by repo path).Tests
lib/push.test.ts:sendTestNotificationcases (unsupported / ungranted no-ops, granted →showNotificationcalled).top-bar.test.tsx:NotificationControlblock — state glyph (on/off), hidden-when-unsupported, dropdown actions (enable / test enabled+disabled), help-link href/target/rel, Escape-to-close + focus return.tsc --noEmitclean, production build clean.🤖 Generated with Claude Code