feat(operator): opt-in Web Push notifications (flag-gated)#188
Conversation
fro-bot
left a comment
There was a problem hiding this comment.
Reviewed the diff directly (server auth/routing, operator-client, fixture harness, service worker, subscribe/reconcile orchestration, consent UI) and ran the suite locally (pnpm build:web && pnpm exec vitest run → 1834/1834 passing, pnpm check-types clean, pnpm lint clean).
Verdict: PASS
Blocking issues
None.
Non-blocking concerns
web/src/shell/AppShell.tsx: push teardown on logout (teardownPushOnLogout) runs viaPromise.allSettledalongside the logout POST and is bounded bybuildPushClient's internal 10sAbortSignal.timeout, so it can't hang navigation — good, just flagging for awareness since it's a new failure mode surface (swallowed errors are intentional per the comment).src/gateway/operator-client.tssubscribePush/unsubscribePushCSRF-400 retry reuses the same stale CSRF token (mirrors the pre-existingdecideRunApprovalpattern) rather than refreshing first, unlike the browser-sideweb/src/push/subscribe.tsclient which does refresh CSRF before retrying. This is consistent with existing server-side conventions in this file, not a regression, but worth confirming the Gateway's CSRF TTL makes the server-side blind retry actually useful.
Missing tests
None — coverage is unusually thorough: CSRF/idempotency retry paths, endpoint-hash correlation, drift-matrix reconcile (reconcile.test.ts), safe-copy notification mapping (no payload leak assertions in sw-notification.test.ts and notifications-copy.test.ts), the no-dashboard-proxy 404 invariant (operator-ui.test.ts), and production-bundle fixture-literal-leak checks (static-assets.test.ts) are all present and passing.
Risk assessment: LOW
Rationale:
- The dashboard never mounts
/operator/push/*— confirmed by readingserver.ts(the push flag only injects a<meta>tag) and by the passingpush routes — no-dashboard-proxy 404 invarianttest. - Service worker push handling is defensively parsed and renders only fixed, allowlisted copy (
sw-notification.ts);notificationclicknever reads payload data as a navigation target (open-redirect guard). - Fixture harness push routes are dev-only, gated by NODE_ENV + loopback-bind double guard (unchanged pattern from existing fixture infra), never echo raw endpoint/keys, and are excluded from production bundles by an explicit literal-scan test.
operator-client.tspath/ID validation (traversal, protocol-relative, scheme-prefixed) and CSRF/idempotency guards are unchanged in shape from the pre-existingdecideRunApproval/launchRunpattern, just extended correctly to the push endpoints.- No new dependencies were added; no auth-flow or route-ordering changes outside the flag-gated additions.
- No breaking changes: all new routes/fields are additive and flag-gated (default off).
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/dashboard |
| Run ID | 29115361300 |
| Cache | hit |
| Session | ses_0b2aad9aaffeacigXY3FLY2VNp |
Adds opt-in operator Web Push notifications to the dashboard PWA, behind the
DASHBOARD_OPERATOR_PUSH_ENABLEDflag (default off). Closes #108.Operators can subscribe a device to receive notifications for pending approvals and failed run outcomes. The dashboard stays a thin browser client — it does not persist or proxy push subscriptions; the Gateway owns subscription lifecycle, VAPID config/dispatch, opt-in state, dead-sub cleanup, and retention/export/delete.
What's in scope
web/src/push/): fetches the VAPID key, subscribes/unsubscribes via the Gateway push routes, and sends transient presence — all same-origin/operator/push/*, reverse-proxied to the Gateway (never mounted by the dashboard).pushalways renders a safe notification with a defensive payload parse;notificationclickopens/focuses/(never deep-links to action-bearing state); no cached operator data is read or written.unsubscribe()+ Gateway unsubscribe + runtime-cache purge; in-flight subscribes are aborted.deniedpermission reconciles Gateway state via unsubscribe.Security posture
/auth/*is never cached, and cross-origin/redirected responses can't poison the monitoring cache.Verification
check-types,lint, andbuild:webclean; theweb → srcimport-boundary guard passes; a real Docker build succeeds.fcm.googleapis.comPushManagersubscription → disable → back to enable, zero console errors.Known limitations (follow-ups filed)
pushsubscriptionchangereconciliation is best-effort; page-load reconcile owns correctness (Operator push: pushsubscriptionchange resubscribe can be undone by page reconcile #184).