Fix notification counts for encrypted rooms with ignored event rules

spec/unit/notifications.spec.ts

@@ -134,7 +134,7 @@ describe("fixNotificationCountOnDecryption", () => {
 
         fixNotificationCountOnDecryption(mockClient, event);
 
-        expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(2);
+        expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(3);
         expect(room.getUnreadNotificationCount(NotificationCountType.Highlight)).toBe(1);
     });
 
@@ -154,7 +154,7 @@ describe("fixNotificationCountOnDecryption", () => {
 
         fixNotificationCountOnDecryption(mockClient, threadEvent);
 
-        expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(1);
+        expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(2);
         expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(1);
     });
 
@@ -192,6 +192,31 @@ describe("fixNotificationCountOnDecryption", () => {
         expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(0);
     });
 
+    it("does not change the total room count when an event is marked as non-notifying", () => {
+        room.setThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total, 0);
+        room.setUnreadNotificationCount(NotificationCountType.Total, 0);
+        room.setUnreadNotificationCount(NotificationCountType.Highlight, 0);
+
+        event.getPushActions = jest.fn().mockReturnValue(mkPushAction(true, false));
+        mockClient.getPushActionsForEvent = jest.fn().mockReturnValue(mkPushAction(false, false));
+
+        fixNotificationCountOnDecryption(mockClient, event);
+        expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(0);
+        expect(room.getUnreadNotificationCount(NotificationCountType.Highlight)).toBe(0);
+    });
+
+    it("does not change the total room count when a threaded event is marked as non-notifying", () => {
+        room.setThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total, 0);
+        room.setThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight, 0);
+
+        threadEvent.getPushActions = jest.fn().mockReturnValue(mkPushAction(true, false));
+        mockClient.getPushActionsForEvent = jest.fn().mockReturnValue(mkPushAction(false, false));
+
+        fixNotificationCountOnDecryption(mockClient, event);
+        expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(0);
+        expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(0);
+    });
+
     it("emits events", () => {
         const cb = jest.fn();
         room.on(RoomEvent.UnreadNotifications, cb);

src/client.ts

@@ -9483,64 +9483,77 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
  * accurate notification_count
  */
 export function fixNotificationCountOnDecryption(cli: MatrixClient, event: MatrixEvent): void {
-    const oldActions = event.getPushActions();
-    const actions = cli.getPushActionsForEvent(event, true);
+    const ourUserId = cli.getUserId();
+    const eventId = event.getId();
 
     const room = cli.getRoom(event.getRoomId());
-    if (!room || !cli.getUserId()) return;
+    if (!room || !ourUserId || !eventId) return;
+
+    const oldActions = event.getPushActions();
+    const actions = cli.getPushActionsForEvent(event, true);
 
     const isThreadEvent = !!event.threadRootId && !event.isThreadRoot;
 
-    const currentCount = room.getUnreadCountForEventContext(NotificationCountType.Highlight, event);
+    const currentHighlightCount = room.getUnreadCountForEventContext(NotificationCountType.Highlight, event);
 
     // Ensure the unread counts are kept up to date if the event is encrypted
     // We also want to make sure that the notification count goes up if we already
     // have encrypted events to avoid other code from resetting 'highlight' to zero.
     const oldHighlight = !!oldActions?.tweaks?.highlight;
     const newHighlight = !!actions?.tweaks?.highlight;
-    if (oldHighlight !== newHighlight || currentCount > 0) {
+
+    let hasReadEvent;
+    if (isThreadEvent) {
+        const thread = room.getThread(event.threadRootId);
+        hasReadEvent = thread
+            ? thread.hasUserReadEvent(ourUserId, eventId)
+            : // If the thread object does not exist in the room yet, we don't
+              // want to calculate notification for this event yet. We have not
+              // restored the read receipts yet and can't accurately calculate
+              // notifications at this stage.
+              //
+              // This issue can likely go away when MSC3874 is implemented
+              true;
+    } else {
+        hasReadEvent = room.hasUserReadEvent(ourUserId, eventId);
+    }
+
+    if (hasReadEvent) {
+        // If the event has been read, ignore it.
+        return;
+    }
+
+    if (oldHighlight !== newHighlight || currentHighlightCount > 0) {
         // TODO: Handle mentions received while the client is offline
         // See also https://github.com/vector-im/element-web/issues/9069
-        let hasReadEvent;
+        let newCount = currentHighlightCount;
+        if (newHighlight && !oldHighlight) newCount++;
+        if (!newHighlight && oldHighlight) newCount--;
+
         if (isThreadEvent) {
-            const thread = room.getThread(event.threadRootId);
-            hasReadEvent = thread
-                ? thread.hasUserReadEvent(cli.getUserId()!, event.getId()!)
-                : // If the thread object does not exist in the room yet, we don't
-                  // want to calculate notification for this event yet. We have not
-                  // restored the read receipts yet and can't accurately calculate
-                  // highlight notifications at this stage.
-                  //
-                  // This issue can likely go away when MSC3874 is implemented
-                  true;
+            room.setThreadUnreadNotificationCount(event.threadRootId, NotificationCountType.Highlight, newCount);
         } else {
-            hasReadEvent = room.hasUserReadEvent(cli.getUserId()!, event.getId()!);
+            room.setUnreadNotificationCount(NotificationCountType.Highlight, newCount);
         }
+    }
 
-        if (!hasReadEvent) {
-            let newCount = currentCount;
-            if (newHighlight && !oldHighlight) newCount++;
-            if (!newHighlight && oldHighlight) newCount--;
+    // Total count is used to typically increment a room notification counter, but not loudly highlight it.
+    const currentTotalCount = room.getUnreadCountForEventContext(NotificationCountType.Total, event);
 
-            if (isThreadEvent) {
-                room.setThreadUnreadNotificationCount(event.threadRootId, NotificationCountType.Highlight, newCount);
-            } else {
-                room.setUnreadNotificationCount(NotificationCountType.Highlight, newCount);
-            }
+    // `notify` is used in practice for incrementing the total count
+    const newNotify = !!actions?.notify;
 
-            // Fix 'Mentions Only' rooms from not having the right badge count
-            const totalCount =
-                (isThreadEvent
-                    ? room.getThreadUnreadNotificationCount(event.threadRootId, NotificationCountType.Total)
-                    : room.getRoomUnreadNotificationCount(NotificationCountType.Total)) ?? 0;
-
-            if (totalCount < newCount) {
-                if (isThreadEvent) {
-                    room.setThreadUnreadNotificationCount(event.threadRootId, NotificationCountType.Total, newCount);
-                } else {
-                    room.setUnreadNotificationCount(NotificationCountType.Total, newCount);
-                }
-            }
+    // The room total count is NEVER incremented by the server for encrypted rooms. We basically ignore
+    // the server here as it's always going to tell us to increment for encrypted events.
+    if (newNotify) {
+        if (isThreadEvent) {
+            room.setThreadUnreadNotificationCount(
+                event.threadRootId,
+                NotificationCountType.Total,
+                currentTotalCount + 1,
+            );
+        } else {
+            room.setUnreadNotificationCount(NotificationCountType.Total, currentTotalCount + 1);
         }
     }
 }

src/sync.ts

@@ -1299,18 +1299,29 @@ export class SyncApi {
             const accountDataEvents = this.mapSyncEventsFormat(joinObj.account_data);
 
             const encrypted = client.isRoomEncrypted(room.roomId);
-            // we do this first so it's correct when any of the events fire
+            // We store the server-provided value first so it's correct when any of the events fire.
             if (joinObj.unread_notifications) {
-                room.setUnreadNotificationCount(
-                    NotificationCountType.Total,
-                    joinObj.unread_notifications.notification_count ?? 0,
-                );
-
-                // We track unread notifications ourselves in encrypted rooms, so don't
-                // bother setting it here. We trust our calculations better than the
-                // server's for this case, and therefore will assume that our non-zero
-                // count is accurate.
+                /**
+                 * We track unread notifications ourselves in encrypted rooms, so don't
+                 * bother setting it here. We trust our calculations better than the
+                 * server's for this case, and therefore will assume that our non-zero
+                 * count is accurate.
+                 *
+                 * @see import("./client").fixNotificationCountOnDecryption
+                 */
+                if (!encrypted || joinObj.unread_notifications.notification_count === 0) {
+                    // In an encrypted room, if the room has notifications enabled then it's typical for
+                    // the server to flag all new messages as notifying. However, some push rules calculate
+                    // events as ignored based on their event contents (e.g. ignoring msgtype=m.notice messages)
+                    // so we want to calculate this figure on the client in all cases.
+                    room.setUnreadNotificationCount(
+                        NotificationCountType.Total,
+                        joinObj.unread_notifications.notification_count ?? 0,
+                    );
+                }
+
                 if (!encrypted || room.getUnreadNotificationCount(NotificationCountType.Highlight) <= 0) {
+                    // If the locally stored highlight count is zero, use the server provided value.
                     room.setUnreadNotificationCount(
                         NotificationCountType.Highlight,
                         joinObj.unread_notifications.highlight_count ?? 0,
@@ -1327,11 +1338,13 @@ export class SyncApi {
                 // decryption
                 room.resetThreadUnreadNotificationCount(Object.keys(unreadThreadNotifications));
                 for (const [threadId, unreadNotification] of Object.entries(unreadThreadNotifications)) {
-                    room.setThreadUnreadNotificationCount(
-                        threadId,
-                        NotificationCountType.Total,
-                        unreadNotification.notification_count ?? 0,
-                    );
+                    if (!encrypted || unreadNotification.notification_count === 0) {
+                        room.setThreadUnreadNotificationCount(
+                            threadId,
+                            NotificationCountType.Total,
+                            unreadNotification.notification_count ?? 0,
+                        );
+                    }
 
                     const hasNoNotifications =
                         room.getThreadUnreadNotificationCount(threadId, NotificationCountType.Highlight) <= 0;