Uh oh!
There was an error while loading. Please reload this page.
fix(notifications): keep notification websocket alive and respect permission - #825
Conversation
…mission The notification websocket was torn down on virtually every react-query refetch (the 60s unread-count poll, window focus, and the socket's own on-message refetch), because the connect/disconnect effect depended on `init`, whose identity changes on each refetch. The effect cleanup called disconnect(), and for the same logged-in user it never reconnected. Since the connection is kept alive server-side, it should only close when the app closes it, so this made websocket notifications silently stop after a few seconds. It only surfaced in browsers without FCM (e.g. Brave), which are the only ones that rely on this socket. - Hold `init` in a ref so the lifecycle effect no longer depends on its identity. The effect now runs only on login/logout/account switch and unmount, so refetches no longer tear the socket down. - Harden disconnect()/onclose: detach the socket handlers before closing so a deliberate close doesn't trigger the reconnect path, and clear the global socket ref unconditionally. Previously a socket closed while still CONNECTING was left dangling and permanently blocked future connect() calls. Added an isConnecting guard against duplicate connects. - Only show a popup and play a sound when Notification permission is granted; otherwise stay silent (the unread badge still updates in the background). The previous fallback fired an in-app toast, sound, and auto-opened the panel even when permission was not granted, which annoyed the users who had opted out. Notifications remain controllable via the settings and the browser permission.
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 12 minutes and 2 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWebSocket connection handling is hardened to prevent race conditions during async browser notification permission prompting, aggressive socket teardown is implemented, notification display now requires explicit permission without toast fallback, and the provider's socket lifecycle is simplified to track only the current active user. ChangesNotifications WebSocket and Provider Lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/api/notifications-ws-api.ts (1)
256-263: 💤 Low valueConsider moving sound after notification creation for consistency.
If
new Notification()throws (as noted in the comment for mobile browsers), the sound will have already played without a visible notification appearing. MovingplayNotificationSound()after successful notification creation would provide more consistent UX.♻️ Optional fix
- playNotificationSound();- // `new Notification` can throw on some mobile browsers (requires a service // worker); the caller's `.catch` handles that gracefully. const notification = new Notification(i18next.t("notification.popup-title"), { body: toastBody, icon: logo }); ++ playNotificationSound(); notification.onclick = () => {🤖 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 `@apps/web/src/api/notifications-ws-api.ts` around lines 256 - 263, Move the call to playNotificationSound() so it runs only after new Notification(...) succeeds: create the Notification instance first (the code constructing notification using i18next.t("notification.popup-title"), toastBody and logo) and then call playNotificationSound(); this ensures the sound is only played when the Notification (variable notification) was created without throwing.
🤖 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 `@apps/web/src/api/notifications-ws-api.ts`:
- Around line 256-263: Move the call to playNotificationSound() so it runs only
after new Notification(...) succeeds: create the Notification instance first
(the code constructing notification using i18next.t("notification.popup-title"),
toastBody and logo) and then call playNotificationSound(); this ensures the
sound is only played when the Notification (variable notification) was created
without throwing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 265c34aa-9a4b-4927-9f04-f0abde4b06e8
📒 Files selected for processing (2)
apps/web/src/api/notifications-ws-api.tsapps/web/src/features/push-notifications/index.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:b64d0bc9e7
ℹ️ 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".
| ws.disconnect(); | ||
| }; | ||
| }, [activeUser?.username, previousActiveUsr?.username, init]); | ||
| }, [activeUser?.username]); |
There was a problem hiding this comment.
This effect now runs only when activeUser?.username changes, but in websocket fallback mode the allowed types are set only inside init via setEnabledNotificationsTypes(...). As a result, if a logged-in user changes notification type preferences, the active socket keeps using stale enabledNotifyTypes until a reload/account switch, so muted types can still notify (or newly enabled types can stay silent).
Useful? React with 👍 / 👎.
| const socket = window.nws; | ||
| if (socket !== undefined) { | ||
| // Detach handlers first so this deliberate close does not trigger the |
There was a problem hiding this comment.
disconnect() now closes window.nws unconditionally when it exists, even if this NotificationsWebSocket instance never established that connection. Since the app mounts multiple websocket wrappers (e.g. global push provider plus NotificationHandler), a secondary instance calling disconnect() can tear down another instance's live socket, causing avoidable drops/reconnect churn in fallback mode.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes WebSocket notification instability on browsers without FCM (e.g., Brave) and cleans up permission handling. The root cause — react-query result objects getting new identities on every refetch triggering effect re-runs that tear down the socket — is resolved by holding
Confidence Score: 4/5The WebSocket lifetime and reconnect logic are well-hardened; the main risk is in the interaction between NotificationHandler and the new notification-gating logic during query load. The core changes are solid — the initRef pattern, per-instance socket ownership, cancellable reconnect timer, and handler-detach-on-disconnect all address real production bugs with careful reasoning. The one gap is in NotificationHandler: switching from a Zustand store value to the raw query result means hasNotifications is false while the settings query is loading, silently dropping any messages that arrive in that window. apps/web/src/features/shared/notification-handler.tsx — the settings query loading state now directly gates notification delivery Important Files Changed
Sequence DiagramsequenceDiagram
participant R as React Effect
participant P as PushNotificationsProvider
participant W as NotificationsWebSocket
participant WS as WebSocket (browser)
participant NWS as window.nws
Note over R,NWS: Login / username change
R->>P: useEffect([username]) fires
P->>W: withActiveUser(null).disconnect() (prev cleanup)
W-->>WS: detach handlers → close()
W-->>NWS: "window.nws = undefined"
P->>P: initRef.current(username) [async]
P->>P: await requestPermission / refetch settings
P->>P: stale-user check (activeUserRef)
P->>W: withActiveUser(user).connect()
W->>W: "isConnecting = true"
W->>WS: new WebSocket(url)
W-->>NWS: "window.nws = socket"
WS-->>W: "onopen → isConnecting=false, isConnected=true"
Note over R,NWS: Incoming message
WS-->>W: onmessage → onMessageReceive()
W->>W: check hasNotifications + allowedToNotify
W->>W: queueNotification → burstTimer
W->>W: flushPendingNotifications()
alt "Notification.permission === granted"
W->>WS: new Notification(title, body)
W->>W: playNotificationSound()
else permission not granted
W->>W: silent return (badge updated via callback)
end
Note over R,NWS: Network drop
WS-->>W: onclose (handlers still attached)
W->>W: "isConnected=false, scheduleReconnect()"
W->>W: setTimeout 2s → connect()
Note over R,NWS: Logout
R->>P: "useEffect([username=undefined]) fires"
P->>W: withActiveUser(null)
P->>W: disconnect()
W->>W: cancel reconnectTimer
W-->>WS: detach handlers → close()
W-->>NWS: "window.nws = undefined"
Reviews (6): Last reviewed commit: "fix(notifications): guard init() against..." | Re-trigger Greptile |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…+ sound Close the gap where websocket toasts/sound ignored parts of the user's settings, and address PR review feedback on the connection lifecycle. Settings gating: - Respect the global on/off: hasNotifications now reflects `allows_notify` instead of being hardcoded true (the Decks NotificationHandler uses it too, replacing the never-updated `globalNotifications` flag). - Map favorites and bookmarks in getNotificationType so disabling them in settings actually suppresses their toasts. Types without a settings toggle (delegations, checkins, payouts, monthly-posts, weekly-earnings) stay always-allowed, gated only by the global switch. - An empty notify_types list now means "none", not "all". - Sync enabled types + the global toggle to the live socket when settings change, so edits take effect without a reload/account switch. Connection lifecycle (review feedback): - disconnect() only tears down the socket this instance created, so a secondary wrapper can't close the global provider's live connection. - Reconnect on any unexpected close, including a clean server close frame (deliberate disconnects detach the handler, so they don't reconnect). - Reset withActiveUser(null) on logout so a pending reconnect can't revive the socket; cancel the reconnect timer on disconnect.
Uh oh!
There was an error while loading. Please reload this page.
…ew fixes Bell toggle: drive the notifications modal directly from the global `uiNotifications` store instead of mirroring it into a local `useState` synced by an effect. The mirror left a one-render window where the value was a stale `false`, which made the base Modal fire its `onHide` effect and reset the store flag right after the bell set it — so a click could appear to do nothing and you'd have to click again. Intermittent, and widened by the lazy-loaded dialog mounting after the click. Review feedback (PR #825): - connect(): wrap requestNotificationPermission() in try/catch so a thrown prompt (sandboxed iframe / extension conflict) can't leave isConnecting stuck true and block every future connect(). - flush: construct the Notification before playing the sound, so a throw on mobile no longer plays a sound with no visible notification.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/api/notifications-ws-api.ts (1)
113-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCancel the in-flight
connect()whendisconnect()runs.While
connect()is awaiting the permission prompt,disconnect()only clears flags and the current socket. If this instance still hasactiveUser, the resumed call can still pass Line 129 and recreate a socket after logout, unmount, or account switch.🛠️ Proposed fix
export class NotificationsWebSocket { + private connectAttempt = 0; private activeUser: ActiveUser | null = null; private hasNotifications = false; private hasUiNotifications = false; @@ public async connect() { if (this.isConnected || this.isConnecting) { return; } + const connectAttempt = ++this.connectAttempt; if (!this.activeUser) { this.disconnect(); @@ if ("Notification" in window) { try { await requestNotificationPermission(); @@ } } ++ if (connectAttempt !== this.connectAttempt) {+ this.isConnecting = false;+ return;+ } // Re-check after the async permission prompt: the user may have logged out, // or another connect() call may have already created the socket. @@ public disconnect() { + this.connectAttempt++; if (this.burstTimer) { clearTimeout(this.burstTimer); this.burstTimer = null;Also applies to: 183-211
🤖 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 `@apps/web/src/api/notifications-ws-api.ts` around lines 113 - 132, The connect() path must be cancellable so awaiting the notification permission doesn't allow a stale connect to proceed; add a connection generation token (e.g., this.connectSeq or this.connectToken) that you increment at the start of connect() and again in disconnect(), capture the token in a local variable before awaiting requestNotificationPermission(), and immediately after the await compare the local token to this.connectSeq and return early if they differ; apply the same pattern to the second connect code path around lines 183-211 so resume after async waits never creates a socket for a disconnected/changed activeUser.
🤖 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 `@apps/web/src/api/notifications-ws-api.ts`:
- Around line 134-145: The connect() method sets this.isConnecting = true before
constructing new WebSocket(`${defaults.nwsServer}/ws?user=${user.username}`) but
doesn’t catch synchronous constructor errors, so isConnecting can remain true
and block future attempts; wrap the new WebSocket(...) creation in a try/catch
(around the WebSocket instantiation and any immediate setup that could throw),
and in the catch ensure this.isConnecting = false and surface/log the error,
leaving other socket-related assignments (this.socket, window.nws,
socket.onopen/onmessage/onclose) only after successful construction so that
connect(), isConnecting, and socket lifecycle are consistent.
In `@apps/web/src/features/push-notifications/index.tsx`:
- Around line 158-169: The effect currently updates only wsRef.current but not
the FCM listener created in init/listenFCM, so FCM still uses the old settings
snapshot (and ignores allows_notify); update the FCM path when
notificationsSettingsQuery.data changes by re-applying the new settings to the
FCM handler: detect the granted-permission branch (where listenFCM was used),
and either call a dedicated updater on that FCM instance (e.g., a method
analogous to setEnabledNotificationsTypes/setHasNotifications) or re-run
listenFCM with the new notify_types and allows_notify values so the FCM handler
respects toggles immediately; ensure the change references wsRef.current,
listenFCM, init, allows_notify and notify_types so the live FCM subscription is
updated instead of relying on reinitialization.
---
Outside diff comments:
In `@apps/web/src/api/notifications-ws-api.ts`:
- Around line 113-132: The connect() path must be cancellable so awaiting the
notification permission doesn't allow a stale connect to proceed; add a
connection generation token (e.g., this.connectSeq or this.connectToken) that
you increment at the start of connect() and again in disconnect(), capture the
token in a local variable before awaiting requestNotificationPermission(), and
immediately after the await compare the local token to this.connectSeq and
return early if they differ; apply the same pattern to the second connect code
path around lines 183-211 so resume after async waits never creates a socket for
a disconnected/changed activeUser.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5b3cffc-db46-4987-a49d-9b716d82df44
📒 Files selected for processing (4)
apps/web/src/api/notifications-ws-api.tsapps/web/src/features/push-notifications/index.tsxapps/web/src/features/shared/notification-handler.tsxapps/web/src/features/shared/notifications/index.tsx
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…pes, FCM sync) - connect(): guard `new WebSocket(...)` with try/catch so a synchronous constructor throw resets isConnecting instead of permanently blocking future reconnects. - onMessageReceive(): keep an empty/unset notify_types as "allow all" (a full opt-out is the global toggle) so existing users whose saved notify_types is still [] aren't silenced; per-type filtering still applies for non-empty lists. - FCM path: read the latest settings via a ref and gate the sound by allows_notify + the current per-type set, instead of a stale closure that ignored later settings changes.
…t owner PushNotificationsProvider's instance owns the live socket on the websocket path but was never given withToggleUi, so an OS notification's onclick was a no-op (the panel never opened) for Brave / permission-granted users. And hasUiNotifications was set to `permission !== "granted"` once at init and never re-synced, so clicking a notification while the panel was open would have toggled it closed. - Wire .withToggleUi(toggleUiProp) and drop the permission-based hasUi flag. - Add an effect syncing hasUiNotifications to the live uiNotifications store so onclick opens the panel only when it's actually closed.
Uh oh!
There was an error while loading. Please reload this page.
init() runs several awaits (isSupported, requestPermission, settings refetch, mutateAsync) before wiring up the socket. If the user logged out or switched accounts mid-flight, the logout effect's withActiveUser(null).disconnect() would be undone when init() resumed and called withActiveUser(staleUser) .connect(), opening a socket for the wrong / logged-out user. Re-check the current active user via a ref after the awaits and bail if it changed; otherwise connect with the current user object.
Problem
On browsers without FCM (e.g. Brave), the app falls back to the notification WebSocket. The socket connects, but notifications silently stop arriving after a few seconds — the connection is closed by the client and never comes back for the same logged-in user.
Root cause
The connect/disconnect effect in
PushNotificationsProviderdepended oninit, andinit'suseCallbackdeps include the react-query result objects. Those objects get a new identity on every refetch — the 60s unread-count poll, window-focus refetch, and the socket's own on-message refetch. Each refetch ⇒ newinit⇒ the effect re-runs ⇒ its cleanup callsdisconnect(). For the same user the effect never callsconnect()again, and a deliberate close doesn't hit the reconnect path, so the socket stays dead.Because the connection is kept alive on the server side, the client should only ever close it intentionally — so this churn was the sole reason it dropped.
Changes
push-notifications/index.tsx— holdinitin a ref so the lifecycle effect no longer depends on its identity. The effect now runs only on login / logout / account switch (and unmount). Refetches no longer tear the socket down. Removed the now-unusedusePrevious.notifications-ws-api.tsdisconnect()detaches the socket handlers before closing (so a deliberate close can't trigger the reconnect path) and clears the global socket ref unconditionally. Previously a socket closed while stillCONNECTINGwas left dangling and permanently blocked futureconnect()calls via thewindow.nws !== undefinedguard.isConnectingguard to prevent duplicate sockets across the async permission prompt.onclosereconnects only on genuinely unexpected closes (network drops).Notification.permission === "granted"; otherwise the client stays silent and just lets the unread badge update. The old fallback fired an in-app toast + sound + auto-opened the panel even when permission was not granted, which annoyed users who had opted out. Notifications stay controllable via settings and the browser permission.How to test
In Brave (or Chrome with notifications denied so it takes the WebSocket path): log in, open DevTools → Network → WS, and confirm the notification WebSocket stays open past 60+ seconds (across the unread-count poll) instead of closing shortly after connecting. With permission granted, incoming notifications should produce a single OS notification + sound; with permission not granted, no popup/sound, but the unread badge still updates.
Summary by CodeRabbit
Bug Fixes
New Features
Refactor