Fix highlights from threads disappearing on new messages (#4106)

* Fix highlights from threads disappearing on new messages This changes interface of Room, so this is a BREAKING CHANGE. Correctly mirrors the logic we use for room notifications for thread notifications, ie. set only the total notifications count from the server if it's zero. I'm not delighted with this since it ends up with function on room whose contract is to do something frankly, deeply weird and unintuitive. However, this is the hack we use for room notifications and it, empirically, works well enough. To do better, we'd need much more complex logic to overlay notification counts for decrypted messages. Fixes element-hq/element-web#25523 * Add tests for the special notification behaviour in syncing
matrix-org · Mar 13, 2024 · 2f1ae5c · 2f1ae5c
1 parent 78d0594
commit 2f1ae5c
Show file tree

Hide file tree

Showing 4 changed files with 144 additions and 25 deletions.
diff --git a/spec/integ/matrix-client-syncing.spec.ts b/spec/integ/matrix-client-syncing.spec.ts
@@ -1645,6 +1645,99 @@ describe("MatrixClient syncing", () => {
             });
         });
 
+        it("should zero total notifications for threads when absent from the notifications object", async () => {
+            syncData.rooms.join[roomOne][UNREAD_THREAD_NOTIFICATIONS.name] = {
+                [THREAD_ID]: {
+                    highlight_count: 2,
+                    notification_count: 5,
+                },
+            };
+
+            httpBackend!.when("GET", "/sync").respond(200, syncData);
+
+            client!.startClient();
+
+            await Promise.all([httpBackend!.flushAllExpected(), awaitSyncEvent()]);
+
+            const room = client!.getRoom(roomOne);
+
+            expect(room!.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(5);
+
+            syncData.rooms.join[roomOne][UNREAD_THREAD_NOTIFICATIONS.name] = {};
+
+            httpBackend!.when("GET", "/sync").respond(200, syncData);
+
+            await Promise.all([httpBackend!.flushAllExpected(), awaitSyncEvent()]);
+
+            expect(room!.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(0);
+        });
+
+        it("should zero highlight notifications for threads in encrypted rooms", async () => {
+            syncData.rooms.join[roomOne][UNREAD_THREAD_NOTIFICATIONS.name] = {
+                [THREAD_ID]: {
+                    highlight_count: 2,
+                    notification_count: 5,
+                },
+            };
+
+            httpBackend!.when("GET", "/sync").respond(200, syncData);
+
+            client!.startClient();
+
+            await Promise.all([httpBackend!.flushAllExpected(), awaitSyncEvent()]);
+
+            const room = client!.getRoom(roomOne);
+
+            expect(room!.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(5);
+
+            syncData.rooms.join[roomOne][UNREAD_THREAD_NOTIFICATIONS.name] = {
+                [THREAD_ID]: {
+                    highlight_count: 0,
+                    notification_count: 0,
+                },
+            };
+
+            httpBackend!.when("GET", "/sync").respond(200, syncData);
+
+            await Promise.all([httpBackend!.flushAllExpected(), awaitSyncEvent()]);
+
+            expect(room!.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(0);
+        });
+
+        it("should not zero highlight notifications for threads in encrypted rooms", async () => {
+            syncData.rooms.join[roomOne][UNREAD_THREAD_NOTIFICATIONS.name] = {
+                [THREAD_ID]: {
+                    highlight_count: 2,
+                    notification_count: 5,
+                },
+            };
+
+            httpBackend!.when("GET", "/sync").respond(200, syncData);
+
+            client!.startClient();
+
+            await Promise.all([httpBackend!.flushAllExpected(), awaitSyncEvent()]);
+
+            const room = client!.getRoom(roomOne);
+            room!.hasEncryptionStateEvent = jest.fn().mockReturnValue(true);
+
+            expect(room!.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(5);
+
+            syncData.rooms.join[roomOne][UNREAD_THREAD_NOTIFICATIONS.name] = {
+                [THREAD_ID]: {
+                    highlight_count: 0,
+                    notification_count: 0,
+                },
+            };
+
+            httpBackend!.when("GET", "/sync").respond(200, syncData);
+
+            await Promise.all([httpBackend!.flushAllExpected(), awaitSyncEvent()]);
+
+            expect(room!.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(0);
+            expect(room!.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(2);
+        });
+
         it("caches unknown threads receipts and replay them when the thread is created", async () => {
             const THREAD_ID = "$unknownthread:localhost";
 

diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts
@@ -3441,12 +3441,12 @@ describe("Room", function () {
 
             expect(room.threadsAggregateNotificationType).toBe(NotificationCountType.Highlight);
 
-            room.resetThreadUnreadNotificationCount();
+            room.resetThreadTotalUnreadNotificationCount();
 
-            expect(room.threadsAggregateNotificationType).toBe(null);
+            expect(room.threadsAggregateNotificationType).toBe(NotificationCountType.Highlight);
 
             expect(room.getThreadUnreadNotificationCount("123", NotificationCountType.Total)).toBe(0);
-            expect(room.getThreadUnreadNotificationCount("123", NotificationCountType.Highlight)).toBe(0);
+            expect(room.getThreadUnreadNotificationCount("123", NotificationCountType.Highlight)).toBe(123);
         });
 
         it("sets the room threads notification type", () => {
@@ -3460,12 +3460,24 @@ describe("Room", function () {
 
         it("partially resets room notifications", () => {
             room.setThreadUnreadNotificationCount("123", NotificationCountType.Total, 666);
+            room.setThreadUnreadNotificationCount("321", NotificationCountType.Total, 555);
             room.setThreadUnreadNotificationCount("456", NotificationCountType.Highlight, 123);
 
-            room.resetThreadUnreadNotificationCount(["123"]);
+            room.resetThreadTotalUnreadNotificationCount(["123"]);
 
             expect(room.getThreadUnreadNotificationCount("123", NotificationCountType.Total)).toBe(666);
-            expect(room.getThreadUnreadNotificationCount("456", NotificationCountType.Highlight)).toBe(0);
+            expect(room.getThreadUnreadNotificationCount("321", NotificationCountType.Total)).toBe(0);
+            expect(room.getThreadUnreadNotificationCount("456", NotificationCountType.Highlight)).toBe(123);
+        });
+
+        it("resets total count to zero", () => {
+            room.setThreadUnreadNotificationCount("123", NotificationCountType.Total, 666);
+            room.setThreadUnreadNotificationCount("123", NotificationCountType.Highlight, 555);
+
+            room.resetThreadTotalUnreadNotificationCount();
+
+            expect(room.getThreadUnreadNotificationCount("123", NotificationCountType.Total)).toBe(0);
+            expect(room.getThreadUnreadNotificationCount("123", NotificationCountType.Highlight)).toBe(555);
         });
 
         it("emits event on notifications reset", () => {
@@ -3476,7 +3488,7 @@ describe("Room", function () {
             room.setThreadUnreadNotificationCount("123", NotificationCountType.Total, 666);
             room.setThreadUnreadNotificationCount("456", NotificationCountType.Highlight, 123);
 
-            room.resetThreadUnreadNotificationCount();
+            room.resetThreadTotalUnreadNotificationCount();
 
             expect(cb).toHaveBeenLastCalledWith();
         });
@@ -3498,10 +3510,10 @@ describe("Room", function () {
         });
 
         it("lets you reset", () => {
-            room.setThreadUnreadNotificationCount("123", NotificationCountType.Highlight, 1);
+            room.setThreadUnreadNotificationCount("123", NotificationCountType.Total, 1);
             expect(room.hasThreadUnreadNotification()).toBe(true);
 
-            room.resetThreadUnreadNotificationCount();
+            room.resetThreadTotalUnreadNotificationCount();
 
             expect(room.hasThreadUnreadNotification()).toBe(false);
         });
@@ -3529,12 +3541,22 @@ describe("Room", function () {
         it("allows reset", () => {
             room.setThreadUnreadNotificationCount("$123", NotificationCountType.Total, 1);
             room.setThreadUnreadNotificationCount("$456", NotificationCountType.Total, 1);
+            expect(room.threadsAggregateNotificationType).toBe(NotificationCountType.Total);
+
+            room.resetThreadTotalUnreadNotificationCount();
+
+            expect(room.threadsAggregateNotificationType).toBeNull();
+        });
+
+        it("retains highlight on reset", () => {
+            room.setThreadUnreadNotificationCount("$123", NotificationCountType.Total, 2);
+            room.setThreadUnreadNotificationCount("$456", NotificationCountType.Total, 1);
             room.setThreadUnreadNotificationCount("$123", NotificationCountType.Highlight, 1);
             expect(room.threadsAggregateNotificationType).toBe(NotificationCountType.Highlight);
 
-            room.resetThreadUnreadNotificationCount();
+            room.resetThreadTotalUnreadNotificationCount();
 
-            expect(room.threadsAggregateNotificationType).toBeNull();
+            expect(room.threadsAggregateNotificationType).toBe(NotificationCountType.Highlight);
         });
     });
 

diff --git a/src/models/room.ts b/src/models/room.ts
@@ -1474,18 +1474,22 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
     }
 
     /**
-     * Resets the thread notifications for this room
+     * Resets the total thread notifications for this room to zero, excluding any threads
+     * whose IDs are given in `exceptThreadIds`.
+     *
+     * This is intended for use from the sync code since we calculate highlight notification
+     * counts locally from decrypted messages. We want to partially trust the total from the
+     * server such that we clear notifications when read receipts arrive.
+     *
+     * @param exceptThreadIds - The thread IDs to exclude from the reset.
      */
-    public resetThreadUnreadNotificationCount(notificationsToKeep?: string[]): void {
-        if (notificationsToKeep) {
-            for (const [threadId] of this.threadNotifications) {
-                if (!notificationsToKeep.includes(threadId)) {
-                    this.threadNotifications.delete(threadId);
-                }
+    public resetThreadTotalUnreadNotificationCount(exceptThreadIds: string[] = []): void {
+        for (const [threadId, notifs] of this.threadNotifications) {
+            if (!exceptThreadIds.includes(threadId)) {
+                notifs.total = 0;
             }
-        } else {
-            this.threadNotifications.clear();
         }
+
         this.emit(RoomEvent.UnreadNotifications);
     }
 

diff --git a/src/sync.ts b/src/sync.ts
@@ -1354,11 +1354,11 @@ export class SyncApi {
             const unreadThreadNotifications =
                 joinObj[UNREAD_THREAD_NOTIFICATIONS.name] ?? joinObj[UNREAD_THREAD_NOTIFICATIONS.altName!];
             if (unreadThreadNotifications) {
-                // Only partially reset unread notification
-                // We want to keep the client-generated count. Particularly important
-                // for encrypted room that refresh their notification count on event
-                // decryption
-                room.resetThreadUnreadNotificationCount(Object.keys(unreadThreadNotifications));
+                // This mirrors the logic above for rooms: take the *total* notification count from
+                // the server for unencrypted rooms or is it's zero. Any threads not present in this
+                // object implicitly have zero notifications, so start by clearing the total counts
+                // for all such threads.
+                room.resetThreadTotalUnreadNotificationCount(Object.keys(unreadThreadNotifications));
                 for (const [threadId, unreadNotification] of Object.entries(unreadThreadNotifications)) {
                     if (!encrypted || unreadNotification.notification_count === 0) {
                         room.setThreadUnreadNotificationCount(
@@ -1379,7 +1379,7 @@ export class SyncApi {
                     }
                 }
             } else {
-                room.resetThreadUnreadNotificationCount();
+                room.resetThreadTotalUnreadNotificationCount();
             }
 
             joinObj.timeline = joinObj.timeline || ({} as ITimeline);