From 4ba1341f8fcab473d0d2c342e5316cc3761c6079 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 21 Mar 2024 16:29:00 +0000 Subject: [PATCH] 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 https://github.com/element-hq/element-web/issues/25523 * Add tests for the special notification behaviour in syncing * Correctly copy the room logic for reseting notifications We were always ignoring the highlight count, even for encrypted rooms, which was broken because we don't do the local calculation for unencrypted rooms. --- spec/integ/matrix-client-syncing.spec.ts | 93 ++++++++++++++++++++++++ spec/unit/room.spec.ts | 48 ++++++++---- src/models/room.ts | 29 ++++++-- src/sync.ts | 12 +-- 4 files changed, 152 insertions(+), 30 deletions(-) diff --git a/spec/integ/matrix-client-syncing.spec.ts b/spec/integ/matrix-client-syncing.spec.ts index dbf038d91..3e97c648b 100644 --- a/spec/integ/matrix-client-syncing.spec.ts +++ b/spec/integ/matrix-client-syncing.spec.ts @@ -1648,6 +1648,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 index 36d9d0877..6ca454ef8 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -3548,7 +3548,7 @@ describe("Room", function () { expect(room.threadsAggregateNotificationType).toBe(NotificationCountType.Highlight); - room.resetThreadUnreadNotificationCount(); + room.resetThreadUnreadNotificationCountFromSync(); expect(room.threadsAggregateNotificationType).toBe(null); @@ -3565,16 +3565,6 @@ describe("Room", function () { expect(room.threadsAggregateNotificationType).toBe(NotificationCountType.Highlight); }); - it("partially resets room notifications", () => { - room.setThreadUnreadNotificationCount("123", NotificationCountType.Total, 666); - room.setThreadUnreadNotificationCount("456", NotificationCountType.Highlight, 123); - - room.resetThreadUnreadNotificationCount(["123"]); - - expect(room.getThreadUnreadNotificationCount("123", NotificationCountType.Total)).toBe(666); - expect(room.getThreadUnreadNotificationCount("456", NotificationCountType.Highlight)).toBe(0); - }); - it("emits event on notifications reset", () => { const cb = jest.fn(); @@ -3583,7 +3573,7 @@ describe("Room", function () { room.setThreadUnreadNotificationCount("123", NotificationCountType.Total, 666); room.setThreadUnreadNotificationCount("456", NotificationCountType.Highlight, 123); - room.resetThreadUnreadNotificationCount(); + room.resetThreadUnreadNotificationCountFromSync(); expect(cb).toHaveBeenLastCalledWith(); }); @@ -3605,10 +3595,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.resetThreadUnreadNotificationCountFromSync(); expect(room.hasThreadUnreadNotification()).toBe(false); }); @@ -3636,12 +3626,38 @@ 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.resetThreadUnreadNotificationCountFromSync(); + + expect(room.threadsAggregateNotificationType).toBeNull(); + }); + + it("retains highlight for encrypted rooms on reset", () => { + room.hasEncryptionStateEvent = jest.fn().mockReturnValue(true); + + 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.resetThreadUnreadNotificationCountFromSync(); - expect(room.threadsAggregateNotificationType).toBeNull(); + expect(room.threadsAggregateNotificationType).toBe(NotificationCountType.Highlight); + }); + + it("resets highlight for unencrypted rooms on reset", () => { + room.hasEncryptionStateEvent = jest.fn().mockReturnValue(false); + + 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.resetThreadUnreadNotificationCountFromSync(); + + expect(room.threadsAggregateNotificationType).toBe(null); + expect(room.getThreadUnreadNotificationCount("$123", NotificationCountType.Highlight)).toBe(0); }); }); diff --git a/src/models/room.ts b/src/models/room.ts index 456c6ba54..f36885809 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -1609,18 +1609,31 @@ export class Room extends ReadReceipt { } /** - * Resets the thread notifications for this room + * Resets the total thread notifications for all threads in this room to zero, + * excluding any threads whose IDs are given in `exceptThreadIds`. + * + * If the room is not encrypted, also resets the highlight notification count to zero + * for the same set of threads. + * + * 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. The weird name is + * intended to reflect this. You probably do not want to use this. + * + * @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 resetThreadUnreadNotificationCountFromSync(exceptThreadIds: string[] = []): void { + const isEncrypted = this.hasEncryptionStateEvent(); + + for (const [threadId, notifs] of this.threadNotifications) { + if (!exceptThreadIds.includes(threadId)) { + notifs.total = 0; + if (!isEncrypted) { + notifs.highlight = 0; } } - } else { - this.threadNotifications.clear(); } + this.emit(RoomEvent.UnreadNotifications); } diff --git a/src/sync.ts b/src/sync.ts index 8fe60698e..92015bd4e 100644 --- a/src/sync.ts +++ b/src/sync.ts @@ -1355,11 +1355,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.resetThreadUnreadNotificationCountFromSync(Object.keys(unreadThreadNotifications)); for (const [threadId, unreadNotification] of Object.entries(unreadThreadNotifications)) { if (!encrypted || unreadNotification.notification_count === 0) { room.setThreadUnreadNotificationCount( @@ -1380,7 +1380,7 @@ export class SyncApi { } } } else { - room.resetThreadUnreadNotificationCount(); + room.resetThreadUnreadNotificationCountFromSync(); } joinObj.timeline = joinObj.timeline || ({} as ITimeline);