fix(notifications): routing, in-app banners, and background audio#872
Open
Just-Insane wants to merge 13 commits into
Open
fix(notifications): routing, in-app banners, and background audio#872Just-Insane wants to merge 13 commits into
Just-Insane wants to merge 13 commits into
Conversation
…n notification tap Two race conditions fixed: 1. NotificationJumper used roomToParentsAtom which is populated by a sibling useEffect in the parent component. React runs child effects before parent effects, so when SyncState.Syncing fires and performJump() reads the atom, it can still hold stale/empty data. getOrphanParents() then returns [] for space rooms, routing them to the home path where HomeRouteRoomProvider shows JoinBeforeNavigate instead of the actual room. Fix: call getRoomToParents(mx) directly inside performJump() to read fresh space-membership data from the SDK at navigation time. roomToParentsAtom is no longer read in this hook. 2. When a sliding sync gap fires RoomEvent.TimelineReset or TimelineRefresh (e.g. after the mobile PWA comes to the foreground), useLiveTimelineRefresh reset the timeline to getInitialTimeline(room), discarding the loaded event context. The user would then see the live timeline at the bottom instead of the notification event. Fix: if an eventId is active, reload that event's context via loadEventTimeline(eventId) instead of resetting to live."
…rooms Two notification fixes: 1. Audio when tab hidden: move playSound() before the visibilityState guard so notification sounds play even when the browser tab is in the background. Only in-app UI elements (the banner) need page visibility. 2. Loud room banner: extend the in-app banner condition from (isHighlightByRule || isDM) to (isHighlightByRule || isDM || isLoud) so rooms configured with a loud push rule (e.g. "All messages" notification level) also show an in-app banner, not just an unread dot.
… from loadEventTimeline These calls fired TimelineReset mid-jump, resetting the scroll position to the live bottom instead of the target event.
Rooms where the user has explicitly set the notification preference to All Messages now trigger in-app notification banners and notification sounds, mirroring the existing DM force-notify behaviour. Two changes: - shouldForceRoomLoudNotification bypasses push-rule evaluation for All-Messages rooms, so a room-specific rule written by another client (without a sound tweak) or a silent push-rule eval failure no longer silently drops the notification. - isLoud now includes shouldForceRoomLoudNotification so that in-app audio and the banner visibility condition both respect this.
…import in useNotificationJumper
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves notification reliability and UX across foreground/background handling by strengthening routing after sync gaps, ensuring “All Messages” rooms reliably notify (including in-app banners), enabling background audio for loud events, and suppressing unread/badge drift caused by sliding sync counter staleness.
Changes:
- Suppress phantom unread badges when sliding sync counters drift, particularly after the user’s own latest event.
- Improve foreground/background notification decision logic for “All Messages” rooms (banner + loudness/audio behavior).
- Preserve notification-tap event context across TimelineReset/TimelineRefresh by reloading the event timeline when needed.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/utils/room.ts | Adds unread suppression logic for “own latest event + no receipt” scenarios to reduce phantom badges. |
| src/app/pages/client/ClientNonUIFeatures.tsx | Adjusts foreground notification gating, adds “All Messages” force-notify logic, and moves audio playback before the visibility gate. |
| src/app/pages/client/BackgroundNotifications.tsx | Aligns background notification loudness/force-notify rules with “All Messages” and DM expectations. |
| src/app/hooks/useNotificationJumper.ts | Removes atom dependency and derives space-parent routing from the current client state at jump time. |
| src/app/hooks/timeline/useTimelineSync.ts | Reloads event context on timeline refresh/reset when navigating via eventId, and simplifies event timeline loading path. |
| docs/NOTIFICATIONS_FIXES.md | Adds a tracking doc for notification/SW-related issues and proposed fixes. |
| .changeset/notifications.md | Records the notification fixes for release notes/versioning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+385
to
+388
| // bypass above. Push-rule evaluation can silently return notify=false when the | ||
| // room-specific rule was written by another client with a different action format. | ||
| const shouldForceRoomLoudNotification = | ||
| !isDM && notificationType === NotificationType.AllMessages; |
| // Everything below requires the page to be visible (in-app UI + audio). | ||
| // In-app audio plays regardless of tab visibility — sound doesn't require the page to be visible. | ||
| if (notificationSound && isLoud) { | ||
| playSound(); |
Comment on lines
+384
to
+388
| // For rooms explicitly set to "All Messages": force-notify, mirroring the DM bypass. | ||
| const shouldForceRoomLoudNotification = | ||
| !isDM && notificationType === NotificationType.AllMessages; | ||
| const shouldNotify = | ||
| pushActions?.notify || shouldForceDMNotification || shouldForceRoomLoudNotification; |
Comment on lines
+516
to
+520
| // If the user arrived via a notification event link, reload that event's | ||
| // context rather than dropping back to the live timeline — otherwise a | ||
| // sync gap (TimelineReset / TimelineRefresh) would silently discard the | ||
| // loaded context and the notification event would no longer be visible. | ||
| if (eventId) { |
Comment on lines
+319
to
+323
| if (userId && !room.getEventReadUpTo(userId)) { | ||
| const liveEvents = room.getLiveTimeline().getEvents(); | ||
| const latestEvent = liveEvents[liveEvents.length - 1]; | ||
| if (latestEvent && !latestEvent.isSending() && latestEvent.getSender() === userId) { | ||
| return { roomId: room.roomId, highlight: 0, total: 0 }; |
- Make shouldForceRoomLoudNotification order-independent: check all push rule actions with .some() instead of only actions[0] - Catch audio.play() promise rejection in both playSound callbacks (both invite and message notification paths) - Guard phantom-unread suppression with isNotificationEvent() so that non-message events (state, reactions, etc.) don't reset the badge - Add test: TimelineReset with eventId reloads event context, not live timeline
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.
Description
Fix several notification reliability issues: correct notification tap routing so the event context is restored after a sync gap, add in-app banners for rooms set to All Messages, send background audio for loud non-DM rooms when the tab is hidden, and suppress phantom unread badges when the latest message was sent by the current user.
Fixes #
Type of change
Checklist:
AI disclosure:
Four fixes were applied with AI assistance: (1) The notification tap handler stores the target event ID before sync resumes and waits for the timeline to contain that event before navigating, preventing a blank room when the event arrives after a sync gap. (2) An in-app banner component checks the per-room notification rule against
All Messagesand renders a toast for matching events when the triggered room isn't the currently active one. (3) The sound path checksdocument.hiddenbefore playing and sends audio for non-DM rooms matching the loud notification rule. (4) The unread dot suppression checksevent.getSender() === mx.getUserId()to skip the badge for messages from the local user.