Ensure we don't overinflate the total notification count (#3634)

* Ensure we don't overinflate the total notification count

By correctly comparing push rules before & after decryption

* DRY the code

* Testsssss

* Update tests

Author: t3chguy

spec/unit/event-timeline-set.spec.ts

@@ -303,8 +303,13 @@ describe("EventTimelineSet", () => {
                     messageEventIsDecryptionFailureSpy.mockReturnValue(true);
                     replyEventIsDecryptionFailureSpy.mockReturnValue(true);
 
-                    messageEvent.emit(MatrixEventEvent.Decrypted, messageEvent);
-                    replyEvent.emit(MatrixEventEvent.Decrypted, replyEvent);
+                    messageEvent.emit(
+                        MatrixEventEvent.Decrypted,
+                        messageEvent,
+                        undefined,
+                        messageEvent.getPushDetails(),
+                    );
+                    replyEvent.emit(MatrixEventEvent.Decrypted, replyEvent, undefined, replyEvent.getPushDetails());
 
                     // simulate decryption
                     messageEventIsDecryptionFailureSpy.mockReturnValue(false);
@@ -313,8 +318,13 @@ describe("EventTimelineSet", () => {
                     messageEventShouldAttemptDecryptionSpy.mockReturnValue(false);
                     replyEventShouldAttemptDecryptionSpy.mockReturnValue(false);
 
-                    messageEvent.emit(MatrixEventEvent.Decrypted, messageEvent);
-                    replyEvent.emit(MatrixEventEvent.Decrypted, replyEvent);
+                    messageEvent.emit(
+                        MatrixEventEvent.Decrypted,
+                        messageEvent,
+                        undefined,
+                        messageEvent.getPushDetails(),
+                    );
+                    replyEvent.emit(MatrixEventEvent.Decrypted, replyEvent, undefined, replyEvent.getPushDetails());
                 });
 
                 itShouldReturnTheRelatedEvents();

spec/unit/notifications.spec.ts

@@ -54,7 +54,7 @@ describe("fixNotificationCountOnDecryption", () => {
         mockClient = getMockClientWithEventEmitter({
             ...mockClientMethodsUser(),
             isInitialSyncComplete: jest.fn().mockReturnValue(false),
-            getPushActionsForEvent: jest.fn().mockReturnValue(mkPushAction(true, true)),
+            getPushActionsForEvent: jest.fn((ev) => event.getPushActions() ?? mkPushAction(true, true)),
             getRoom: jest.fn().mockImplementation(() => room),
             decryptEventIfNeeded: jest.fn().mockResolvedValue(void 0),
             supportsThreads: jest.fn().mockReturnValue(true),
@@ -125,15 +125,15 @@ describe("fixNotificationCountOnDecryption", () => {
         room.setThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total, 1);
         room.setThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight, 0);
 
-        event.getPushActions = jest.fn().mockReturnValue(mkPushAction(false, false));
-        threadEvent.getPushActions = jest.fn().mockReturnValue(mkPushAction(false, false));
+        event.getPushActions = jest.fn().mockReturnValue(mkPushAction(true, true));
+        threadEvent.getPushActions = jest.fn().mockReturnValue(mkPushAction(true, true));
     });
 
     it("changes the room count to highlight on decryption", () => {
         expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(2);
         expect(room.getUnreadNotificationCount(NotificationCountType.Highlight)).toBe(0);
 
-        fixNotificationCountOnDecryption(mockClient, event);
+        fixNotificationCountOnDecryption(mockClient, event, {});
 
         expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(3);
         expect(room.getUnreadNotificationCount(NotificationCountType.Highlight)).toBe(1);
@@ -143,7 +143,7 @@ describe("fixNotificationCountOnDecryption", () => {
         room.setUnreadNotificationCount(NotificationCountType.Total, 0);
         room.setUnreadNotificationCount(NotificationCountType.Highlight, 0);
 
-        fixNotificationCountOnDecryption(mockClient, event);
+        fixNotificationCountOnDecryption(mockClient, event, {});
 
         expect(room.getRoomUnreadNotificationCount(NotificationCountType.Total)).toBe(1);
         expect(room.getRoomUnreadNotificationCount(NotificationCountType.Highlight)).toBe(1);
@@ -153,7 +153,7 @@ describe("fixNotificationCountOnDecryption", () => {
         expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(1);
         expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(0);
 
-        fixNotificationCountOnDecryption(mockClient, threadEvent);
+        fixNotificationCountOnDecryption(mockClient, threadEvent, {});
 
         expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(2);
         expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(1);
@@ -163,7 +163,7 @@ describe("fixNotificationCountOnDecryption", () => {
         room.setThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total, 0);
         room.setThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight, 0);
 
-        fixNotificationCountOnDecryption(mockClient, event);
+        fixNotificationCountOnDecryption(mockClient, event, {});
 
         expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(0);
         expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(0);
@@ -187,7 +187,7 @@ describe("fixNotificationCountOnDecryption", () => {
             event: true,
         });
 
-        fixNotificationCountOnDecryption(mockClient, unknownThreadEvent);
+        fixNotificationCountOnDecryption(mockClient, unknownThreadEvent, {});
 
         expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(0);
         expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(0);
@@ -201,7 +201,7 @@ describe("fixNotificationCountOnDecryption", () => {
         event.getPushActions = jest.fn().mockReturnValue(mkPushAction(true, false));
         mockClient.getPushActionsForEvent = jest.fn().mockReturnValue(mkPushAction(false, false));
 
-        fixNotificationCountOnDecryption(mockClient, event);
+        fixNotificationCountOnDecryption(mockClient, event, {});
         expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(0);
         expect(room.getUnreadNotificationCount(NotificationCountType.Highlight)).toBe(0);
     });
@@ -213,7 +213,7 @@ describe("fixNotificationCountOnDecryption", () => {
         threadEvent.getPushActions = jest.fn().mockReturnValue(mkPushAction(true, false));
         mockClient.getPushActionsForEvent = jest.fn().mockReturnValue(mkPushAction(false, false));
 
-        fixNotificationCountOnDecryption(mockClient, event);
+        fixNotificationCountOnDecryption(mockClient, event, {});
         expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(0);
         expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(0);
     });
@@ -231,4 +231,15 @@ describe("fixNotificationCountOnDecryption", () => {
         room.setThreadUnreadNotificationCount("$123", NotificationCountType.Highlight, 5);
         expect(cb).toHaveBeenLastCalledWith({ highlight: 5 }, "$123");
     });
+
+    it("should not increment notification count where event was already contributing notification", () => {
+        expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(2);
+        expect(room.getUnreadNotificationCount(NotificationCountType.Highlight)).toBe(0);
+
+        event.getPushActions = jest.fn().mockReturnValue(mkPushAction(true, false));
+        fixNotificationCountOnDecryption(mockClient, event, { actions: { notify: true, tweaks: {} } });
+
+        expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(2);
+        expect(room.getUnreadNotificationCount(NotificationCountType.Highlight)).toBe(0);
+    });
 });

spec/unit/room-state.spec.ts

@@ -1037,7 +1037,12 @@ describe("RoomState", function () {
 
                 // this event is a message after decryption
                 decryptingRelatedEvent.event.type = EventType.RoomMessage;
-                decryptingRelatedEvent.emit(MatrixEventEvent.Decrypted, decryptingRelatedEvent);
+                decryptingRelatedEvent.emit(
+                    MatrixEventEvent.Decrypted,
+                    decryptingRelatedEvent,
+                    undefined,
+                    decryptingRelatedEvent.getPushDetails(),
+                );
 
                 expect(addLocationsSpy).not.toHaveBeenCalled();
             });

spec/unit/room.spec.ts

@@ -3428,13 +3428,13 @@ describe("Room", function () {
             expect(room.polls.get(pollStartEventId)).toBeUndefined();
 
             // now emit a Decrypted event but keep the decryption failure
-            pollStartEvent.emit(MatrixEventEvent.Decrypted, pollStartEvent);
+            pollStartEvent.emit(MatrixEventEvent.Decrypted, pollStartEvent, undefined, pollStartEvent.getPushDetails());
             // still do not expect a poll to show up for the room
             expect(room.polls.get(pollStartEventId)).toBeUndefined();
 
             // clear decryption failure and emit a Decrypted event again
             isDecryptionFailureSpy.mockRestore();
-            pollStartEvent.emit(MatrixEventEvent.Decrypted, pollStartEvent);
+            pollStartEvent.emit(MatrixEventEvent.Decrypted, pollStartEvent, undefined, pollStartEvent.getPushDetails());
 
             // the poll should now show up in the room's polls
             const poll = room.polls.get(pollStartEventId);

src/client.ts

@@ -1371,8 +1371,8 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         // actions for themselves, so we have to kinda help them out when they are encrypted.
         // We do this so that push rules are correctly executed on events in their decrypted
         // state, such as highlights when the user's name is mentioned.
-        this.on(MatrixEventEvent.Decrypted, (event) => {
-            fixNotificationCountOnDecryption(this, event);
+        this.on(MatrixEventEvent.Decrypted, (event, _, pushDetails) => {
+            fixNotificationCountOnDecryption(this, event, pushDetails);
         });
 
         // Like above, we have to listen for read receipts from ourselves in order to
@@ -9851,26 +9851,23 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
  * Servers do not have enough knowledge about encrypted events to calculate an
  * accurate notification_count
  */
-export function fixNotificationCountOnDecryption(cli: MatrixClient, event: MatrixEvent): void {
+export function fixNotificationCountOnDecryption(
+    cli: MatrixClient,
+    event: MatrixEvent,
+    pushDetails: PushDetails,
+): void {
     const ourUserId = cli.getUserId();
     const eventId = event.getId();
 
     const room = cli.getRoom(event.getRoomId());
     if (!room || !ourUserId || !eventId) return;
 
-    const oldActions = event.getPushActions();
+    // We cannot call event.getPushActions here as the decryption loop just nulled them for re-calculation
+    const oldActions = pushDetails.actions;
     const actions = cli.getPushActionsForEvent(event, true);
 
     const isThreadEvent = !!event.threadRootId && !event.isThreadRoot;
 
-    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;
-
     let hasReadEvent;
     if (isThreadEvent) {
         const thread = room.getThread(event.threadRootId);
@@ -9892,37 +9889,35 @@ export function fixNotificationCountOnDecryption(cli: MatrixClient, event: Matri
         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 newCount = currentHighlightCount;
-        if (newHighlight && !oldHighlight) newCount++;
-        if (!newHighlight && oldHighlight) newCount--;
-
-        if (isThreadEvent) {
-            room.setThreadUnreadNotificationCount(event.threadRootId, NotificationCountType.Highlight, newCount);
+    // 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.
+    // TODO: Handle mentions received while the client is offline
+    // See also https://github.com/vector-im/element-web/issues/9069
+    for (const type of [NotificationCountType.Highlight, NotificationCountType.Total]) {
+        let count = room.getUnreadCountForEventContext(type, event);
+
+        let oldValue: boolean;
+        let newValue: boolean;
+        if (type === NotificationCountType.Total) {
+            // Total count is used to typically increment a room notification counter, but not loudly highlight it.
+            // `notify` is used in practice for incrementing the total count
+            oldValue = !!oldActions?.notify;
+            newValue = !!actions?.notify;
         } else {
-            room.setUnreadNotificationCount(NotificationCountType.Highlight, newCount);
+            oldValue = !!oldActions?.tweaks?.highlight;
+            newValue = !!actions?.tweaks?.highlight;
         }
-    }
 
-    // Total count is used to typically increment a room notification counter, but not loudly highlight it.
-    const currentTotalCount = room.getUnreadCountForEventContext(NotificationCountType.Total, event);
+        if (oldValue !== newValue || count > 0) {
+            if (newValue && !oldValue) count++;
+            if (!newValue && oldValue) count--;
 
-    // `notify` is used in practice for incrementing the total count
-    const newNotify = !!actions?.notify;
-
-    // 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);
+            if (isThreadEvent) {
+                room.setThreadUnreadNotificationCount(event.threadRootId, type, count);
+            } else {
+                room.setUnreadNotificationCount(type, count);
+            }
         }
     }
 }

src/models/event.ts

@@ -226,7 +226,7 @@ export type MatrixEventHandlerMap = {
      * @param event - The matrix event which has been decrypted
      * @param err - The error that occurred during decryption, or `undefined` if no error occurred.
      */
-    [MatrixEventEvent.Decrypted]: (event: MatrixEvent, err?: Error) => void;
+    [MatrixEventEvent.Decrypted]: (event: MatrixEvent, err: Error | undefined, pushDetails: PushDetails) => void;
     [MatrixEventEvent.BeforeRedaction]: (event: MatrixEvent, redactionEvent: MatrixEvent) => void;
     [MatrixEventEvent.VisibilityChange]: (event: MatrixEvent, visible: boolean) => void;
     [MatrixEventEvent.LocalEventIdReplaced]: (event: MatrixEvent) => void;
@@ -916,6 +916,8 @@ export class MatrixEvent extends TypedEventEmitter<MatrixEventEmittedEvents, Mat
             this.retryDecryption = false;
             this.setClearData(res);
 
+            const pushDetails = this.getPushDetails();
+
             // Before we emit the event, clear the push actions so that they can be recalculated
             // by relevant code. We do this because the clear event has now changed, making it
             // so that existing rules can be re-run over the applicable properties. Stuff like
@@ -925,7 +927,7 @@ export class MatrixEvent extends TypedEventEmitter<MatrixEventEmittedEvents, Mat
             this.setPushDetails();
 
             if (options.emit !== false) {
-                this.emit(MatrixEventEvent.Decrypted, this, err);
+                this.emit(MatrixEventEvent.Decrypted, this, err, pushDetails);
             }
 
             return;