feat(notifications): notification center (bell, banner, admin) [#1259]

[dawsontoth]

Closes #1259.

The bones of a GitHub-esque notification center for Fabric Studio, backed by the existing central-manager SystemStatus table.

What's here

• Bell + unread badge in the navbar (desktop + mobile) with a preview dropdown — NotificationBell.tsx.
• Notification center at /notifications — full list grouped active / upcoming / expired — features/notifications/index.tsx.
• Bottom banner auto-showing active, un-acknowledged notices — NotificationBanner.tsx.
• Admin management at /admin/notifications (fabric-admin) — create / edit / delete via DataTable + dialog — features/admin/notifications/.
• Live via subscription, not polling — Harper's table WebSocket; a 60s poll only as a backstop — NotificationsSubscriptionManager.tsx.
• Client-side (localStorage) acks, time-windowed (UTC) active state, and optional internal (router) / external (new-tab) deep links.

Notable decisions

• WebSocket, not SSE. Probing stage: GET /SystemStatus/ with Accept: text/event-stream returns 0 bytes (the edge buffers text/event-stream, even for CM's own tables), while the WS upgrade to wss://…/SystemStatus returns HTTP 101 and connects anonymously (matches the table's public read). Still a true subscription; no polling.
• No central-manager change needed. SystemStatus already exposes public read, fabric-admin-gated writes, and a WebSocket subscription — so start notification center for fabric studio #1259's "blocked on CM" is unblocked by the existing table.
• Banner is bottom-anchored. Studio's chrome hardcodes the top band (fixed header top-0, sub-nav top-20, content mt-32) on every page, so a top banner would need a global offset refactor; a bottom bar is robust everywhere and never hides the nav. Easy to revisit.

Scope / follow-ups

Starts global; org/user scoping is an additive next step (indexed columns + query filter, same center). Server-side per-user read state and a title field are also easy follow-ups. A global notice should also render for signed-out users (currently the surfaces mount inside the authed Dashboard layout) — noted as a fast follow.

Testing

• 17 new unit tests (window/active filtering, deep-link classification, ack store, Radix bell dropdown); full suite green (1804 pass); tsc, oxlint, dprint clean.
• Verified end-to-end in the local preview against stage: bell badge, preview dropdown, bottom banner, center page, severity styling, internal + external deep links, and ack persistence — via synthetic client-side data (this session's account is least_privileged, so the admin CRUD + live WS push are being verified on dev with an admin account).

🤖 Generated with Claude Code

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	53.18%	5897 / 11088
🔵	Statements	53.75%	6322 / 11761
🔵	Functions	45.76%	1463 / 3197
🔵	Branches	46.09%	3964 / 8599

File Coverage

File	Stmts	Branches	Functions	Lines	Uncovered Lines
Changed Files
src/components/Navbar.tsx	30%	20.93%	15.78%	32.43%	33-43, 99-122, 165-312
src/components/NotificationBanner.tsx	30%	25%	20%	25%	23-53
src/components/NotificationBell.tsx	85.71%	77.27%	62.5%	85%	79-88, 127
src/components/StudioCloud.tsx	100%	100%	100%	100%
src/features/admin/routes.ts	63.63%	100%	42.85%	63.63%	14-15, 21-22
src/features/admin/components/AdminShell.tsx	14.28%	0%	0%	20%	18-33
src/features/notifications/NotificationsSubscriptionManager.tsx	93.65%	87.5%	83.33%	98.18%	69, 90, 92, 105
src/features/notifications/acks.ts	93.33%	50%	100%	91.66%	34
src/features/notifications/hooks.ts	86.66%	100%	78.57%	91.3%	20-21, 35
src/features/notifications/notificationHelpers.ts	100%	100%	100%	100%
src/features/notifications/queries.ts	75%	0%	100%	75%	11
src/features/notifications/routes.ts	60%	100%	33.33%	60%	7-8
src/features/notifications/components/NotificationLink.tsx	83.33%	62.5%	100%	80%	35-38
src/lib/storage/localStorageKeys.ts	100%	100%	100%	100%
src/router/rootRouteTree.ts	100%	50%	100%	100%

Generated in workflow #1609 for commit 668c994 by the Vitest Coverage Report Action

[gemini-code-assist]

Code Review

This pull request introduces a comprehensive system-wide notification feature, including a notification bell in the navigation bar, a bottom-anchored notification banner, a dedicated notifications center, and an admin management interface. It also implements a WebSocket subscription manager for real-time updates and client-side acknowledgment persistence. The review feedback highlights a critical security vulnerability where unvalidated external links could allow stored XSS via 'javascript:' or 'data:' URIs. Additionally, feedback points out a potential sorting instability caused by subtracting 'Infinity' from 'Infinity' in the notifications center, and a potential runtime crash in the 'toMs' helper if a non-string value is trimmed.

[DavidCockerill]

Really nice piece of work — this is a lot of surface area and the shape is right throughout. Non-blocking overall; the subscription pair below is the one thing I'd want fixed before it ships, and the two halves need fixing together.

Two things I checked hard and found sound, worth stating since they're the scary parts of a feature like this:

• The authorization isn't presentational. AdminShell.tsx:14's <Navigate to="/" /> is UI-only, but the server enforces independently at central-manager/src/resources/SystemStatus.js:9-19 (isFabricAdmin(context) on create/update/delete) and at roles.yaml:96 (insert/update/delete: false for the standard role). isFabricAdmin throws on a missing user rather than returning falsy, so it fails closed. A normal user POSTing directly gets rejected. Your "no central-manager change needed" claim is accurate.
• The datetime-local ↔ UTC round trip is correct — this is the bug I most expected to find. toInputValue reads with local getters; fromInputValue calls Date.parse on an offset-less date-time string, which ES2015+ specifies as local, then .toISOString(). Same zone both directions, lossless.

Significant

1 & 2 — the subscription has two opposite failure modes sharing one root cause (NotificationsSubscriptionManager.tsx:30 and :44). failures is reset by a successful handshake, not a healthy session.

Silent permanent give-up. Backoff sums to 2+4+8+16+32 = 62s, then if (failures > 5) return; — and because StudioCloud never unmounts, the effect never re-runs. The socket is dead for the rest of the session, with no log line.

Plainly: one wifi hiccup and the live feed is dead in that tab until reload. Nothing tells the user, nothing tells us — it silently degrades to the 60s poll. When someone reports "the maintenance banner took a minute to appear," there's no signal anywhere pointing at the cause.

Unbounded reconnect loop — the mirror image. If the server accepts the upgrade then closes (post-handshake notAllowed(), an edge dropping non-conforming upgrades, a CM restart loop), onopen zeroes failures before every onclose, so it can never exceed 1 and the five-attempt cap is dead code. Every open tab — including tabs on the sign-in page belonging to people who never logged in — retries every 2s forever. (Credit: Codex found this half; I verified it.)

Please note the interaction: the trigger for #2 is exactly the kind of server-side hardening someone might add later, so fix this before anyone tightens the CM side.

Suggested: cap the backoff, not the attempts (the Math.min(…, 30_000) already does that); only zero failures after the socket has stayed open ~10s; re-arm on window.addEventListener('online', …); and console.warn on give-up so it's greppable in a RUM session.

3 — an unparseable endAt means "never expires" (notificationHelpers.ts:86). toMs returns null for both "absent" and "present but unparseable", so an unreadable field short-circuits the expiry check — and getWindowStatus compounds it by labelling the row a confident 'Active'. Trigger: a SystemStatus row in a shape toMs doesn't handle (a Date object, a {$date: …} wrapper, a non-ISO locale string) — most plausibly created outside Studio, e.g. by the CM integration suite or by hand.

Plainly: a "maintenance complete at 3am" banner sits on every user's screen indefinitely, labelled Active, until an admin deletes the row by hand. Users can dismiss it — but only per-device, so it's back on their phone.

Worth making toMs distinguish the two cases and having isActive fail closed on unparseable. The instinct is already in this file — the severity mapping defaults up to critical — it just isn't applied to the time window. One case in the existing isActive describe block at notificationHelpers.test.ts:41 covers it.

4 — NotificationBell.test.tsx mocks away what it's testing (:16). With both hooks and acks mocked, the only production code under test is the bell's JSX. useActiveNotifications and useUnackedActiveNotifications return the same array, so "badge shows 2" asserts the mock, not the unread computation. And useNotificationAcks returning an empty Set means the acked-first branch of the SEVERITY_ORDER sort (NotificationBell.tsx:19) is never exercised — nothing in the PR sorts a mixed acked/unacked list.

Plainly: this test would still pass if the unread-count logic were replaced with "however many notifications exist."

Cheap upgrade: mock only the query (or seed the real queryClient) and let the hooks run. Worth saying that notificationHelpers.test.ts is the opposite and genuinely good — real functions, real-shaped sta- fixtures, boundary cases, XSS rejections. That file is the model.

Non-blocking

• SEVERITY_ORDER is defined twice, identically (NotificationBell.tsx:19, index.tsx:17). It belongs next to SEVERITY_CONFIG in notificationHelpers.ts, exported once — two copies of an ordering rule drift the moment someone adds a fourth severity, and then the bell and the center disagree about which notice is most serious.
• readAcks() trusts the shape, not just the JSON (acks.ts:18). safeParse handles malformed JSON correctly — good reuse — but valid JSON of the wrong shape ({}) flows through the string[] cast, and [...readAcks(), id] / new Set(data) then throw TypeError: not iterable, taking down bell, banner and center at once since all three are globally mounted. Array.isArray(v) ? v.filter(x => typeof x === 'string') : [] closes it. (Codex flagged this too.)
• ackNotification can throw on a full localStorage (acks.ts:39). writeAcks persists before queryClient.setQueryData, so a QuotaExceededError throws out of an onClick and the notice doesn't even disappear in-memory. Swap the order so a quota failure degrades to "ack didn't survive reload" rather than "dismiss is broken." Low likelihood, but Safari private mode has historically thrown on first write. (Codex too.)
• Nothing prunes acks for deleted notifications (acks.ts:39) — slow growth (~12 bytes/id), so hygiene rather than a real quota risk; a two-line intersection on query success makes it self-limiting. The per-browser scoping I'd leave alone — you state it as a deliberate v1 trade with a named successor, and being re-nagged once per device is a fair price for shipping without a CM schema change. One line in the PR body confirming that's deliberate would stop it being re-litigated. (Codex flagged the pruning.)
• The admin form accepts a link it will never render (index.tsx:248). submit() validates the message and start/end ordering but passes url through unchecked — type javascript:alert(1) or a typo'd htp://… and it saves happily, then parseNotificationLink returns null and the link silently never appears. Success toast, broken notice. Call parseNotificationLink(url) in submit so the form can't drift from the renderer.
• new WebSocket(url) is unguarded (:30) — the constructor throws synchronously on a malformed URL, inside a useEffect with no try. VITE_CENTRAL_MANAGER_API_URL is absolute in every checked-in env file so it's fine today, but a relative or //-prefixed value in a future deploy config would throw out of the effect on a page anonymous users load.
• Following on from cb1kenobi's banner note: aria-live="polite" is right for info/maintenance, but your severity model has a critical tier — consider assertive when any strip in the stack is critical. That was in the original Barber AI comment; only the polite half landed.

Verified and dropped

Recording these so nobody re-runs the same checks: the admin gate is not client-side-only (above); the datetime-local↔UTC round trip is correct (above); expiring notices do clear within 30s (useNow(30_000) re-renders and useActiveNotifications re-filters); there's no StrictMode double-subscribe (empty dep array, teardown sets cancelled, the stale closure's onclose early-returns, close() during CONNECTING is legal); and mutations do surface errors via the global MutationCache.onError. I also tried several XSS variants through the link and message fields and found no bypass.

One scoping note for the record: CM#483 does not cover SystemStatus — that guard was scoped to resources declaring static loadAsInstance = false, which SystemStatus doesn't. So the anonymous WS subscribe here works by omission rather than by decision. Not a leak — the table is public-read and carries admin-authored broadcast content — but worth making that permissiveness explicit somewhere rather than incidental, since the next person to read SystemStatus.js alongside #483 will wonder.

Performance is a non-blocking note: four independent 30s clocks where one shared tick would do, and an unmemoized sort in the bell that runs on every render of a component present on every page. Both cheap at this scale.

Cross-model note: ran this by Codex, whose findings are incorporated above and credited inline. The Gemini leg timed out at 240s on the 1539-line diff — not an auth failure, just size.

— Reviewed by DAIvid (Claude Opus 5), cross-checked with Codex

[dawsontoth]

Thanks for the exceptionally thorough pass — the subscription analysis especially. All addressed in d04ce324 (pushed here and to dev).

Blocking — the subscription pair (NotificationsSubscriptionManager.tsx). Fixed both halves at the shared root you identified (failures was keyed to the handshake, not a healthy session):

• failures now resets only after the socket stays open ~10s, so an accept-then-close server backs off to the 30s cap instead of retry-storming every 2s.
• We cap the backoff, not the attempt count — it keeps retrying at 30s rather than giving up permanently, so one wifi hiccup can't strand the tab on the poll backstop for the session.
• Added a window online re-arm, a try/catch around new WebSocket, and a one-time console.warn when it goes unstable so it's greppable in RUM.

Significant:

• Fail-closed expiry — new parseInstant distinguishes absent from unparseable; isActive now returns false and getWindowStatus reports “Schedule unavailable” for an unreadable bound, so a bad endAt can't pin a notice on screen forever. Covered in the isActive describe block as you noted.
• Bell test now mocks only the query and runs the real hooks/ack store/sort — it asserts the expired notice is filtered out, that an ack drops the count, and that un-acked sorts ahead of acked. (notificationHelpers.test.ts stays the model.)

Non-blocking, also done: readAcks validates the array-of-strings shape (wrong-shape JSON can no longer take down all three surfaces); writeAcks updates the cache before persisting so a full localStorage degrades to “didn't survive reload”; the admin form validates the link via parseNotificationLink before saving; the banner uses aria-live="assertive" when any active strip is critical; SEVERITY_ORDER is exported once from notificationHelpers; one shared 30s clock replaces the four; the center sort is memoized.

Consciously deferred:

• Ack pruning for deleted notifications — left as a follow-up; it's ~12 bytes/id and the new shape-guard removes the crash path, so it's hygiene rather than a quota risk.
• Per-browser ack scoping — deliberate v1 trade (noted in the PR body); the named successor is a server-side per-user read-state table once org/user scoping lands.
• CM#483 / anonymous subscribe — documented inline in NotificationsSubscriptionManager.tsx (public-read broadcast content, permissive by omission not decision); a matching note in central-manager's SystemStatus.js is a good separate-repo follow-up.
• Perf — shared clock and memoized center sort landed; the bell sort was already memoized.

🤖 Addressed by Claude Code

[DavidCockerill]

Approving at d04ce32. One commit fixed all four significants plus every non-blocking item — nice.

The part I most wanted to check: the subscription pair was fixed together at the shared root, not one half at a time. Verified with a fake-timer WebSocket harness — 14 reconnects over 5 minutes in both the offline case (was 5-then-permanently-silent) and the accept-then-close case (was ~150). Fixing either half alone would have made the other worse, so that mattered.

Also verified: an unparseable endAt now fails closed, and the bell test survives three mutations — it's a real test now rather than an assertion about its own mocks. Suite green, tsc clean, CI passing.

Two non-blocking threads. The second comes with a ~45-line test I already wrote and ran against this head — say the word and it's yours rather than something you have to write.

— Reviewed by DAIvid (Claude Opus 5)