From 2363703b64f52794bf3984fcf5e5afce8a2dc4c1 Mon Sep 17 00:00:00 2001
From: David Baker <dbkr@users.noreply.github.com>
Date: Wed, 6 Dec 2023 16:25:10 +0000
Subject: [PATCH] Prevent phantom notifications from events not in a room's
 timeline (#3942)

* Test whether an event not in a room's timeline causes notification count increase

Commited separately to demonstrate test failing before.

* Don't fix up notification counts if event isn't in the room

As explained by the comment, hopefully.

* Fix other test
---
 ...matrix-client-unread-notifications.spec.ts | 70 ++++++++++++++-----
 spec/unit/notifications.spec.ts               |  2 +
 src/client.ts                                 | 13 ++++
 3 files changed, 69 insertions(+), 16 deletions(-)

diff --git a/spec/integ/matrix-client-unread-notifications.spec.ts b/spec/integ/matrix-client-unread-notifications.spec.ts
index 95f977820..8238f4e1e 100644
--- a/spec/integ/matrix-client-unread-notifications.spec.ts
+++ b/spec/integ/matrix-client-unread-notifications.spec.ts
@@ -28,32 +28,70 @@ import {
     NotificationCountType,
     RelationType,
     Room,
+    fixNotificationCountOnDecryption,
 } from "../../src";
 import { TestClient } from "../TestClient";
 import { ReceiptType } from "../../src/@types/read_receipts";
 import { mkThread } from "../test-utils/thread";
 import { SyncState } from "../../src/sync";
 
+const userA = "@alice:localhost";
+const userB = "@bob:localhost";
+const selfUserId = userA;
+const selfAccessToken = "aseukfgwef";
+
+function setupTestClient(): [MatrixClient, HttpBackend] {
+    const testClient = new TestClient(selfUserId, "DEVICE", selfAccessToken);
+    const httpBackend = testClient.httpBackend;
+    const client = testClient.client;
+    httpBackend!.when("GET", "/versions").respond(200, {});
+    httpBackend!.when("GET", "/pushrules").respond(200, {});
+    httpBackend!.when("POST", "/filter").respond(200, { filter_id: "a filter id" });
+    return [client, httpBackend];
+}
+
+describe("Notification count fixing", () => {
+    let client: MatrixClient | undefined;
+
+    beforeEach(() => {
+        [client] = setupTestClient();
+    });
+
+    it("doesn't increment notification count for events that can't be found in a room", async () => {
+        const roomId = "!room:localhost";
+
+        client!.startClient({ threadSupport: true });
+        const room = new Room(roomId, client!, selfUserId);
+        jest.spyOn(client!, "getRoom").mockImplementation((id) => (id === roomId ? room : null));
+
+        const event = new MatrixEvent({
+            room_id: roomId,
+            type: "m.reaction",
+            event_id: "$foo",
+            content: {
+                "m.relates_to": {
+                    rel_type: RelationType.Annotation,
+                    event_id: "$foo",
+                    key: "x",
+                },
+            },
+        });
+
+        jest.spyOn(event, "getPushActions").mockReturnValue({
+            notify: true,
+            tweaks: {},
+        });
+
+        fixNotificationCountOnDecryption(client!, event);
+
+        expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(0);
+    });
+});
+
 describe("MatrixClient syncing", () => {
-    const userA = "@alice:localhost";
-    const userB = "@bob:localhost";
-
-    const selfUserId = userA;
-    const selfAccessToken = "aseukfgwef";
-
     let client: MatrixClient | undefined;
     let httpBackend: HttpBackend | undefined;
 
-    const setupTestClient = (): [MatrixClient, HttpBackend] => {
-        const testClient = new TestClient(selfUserId, "DEVICE", selfAccessToken);
-        const httpBackend = testClient.httpBackend;
-        const client = testClient.client;
-        httpBackend!.when("GET", "/versions").respond(200, {});
-        httpBackend!.when("GET", "/pushrules").respond(200, {});
-        httpBackend!.when("POST", "/filter").respond(200, { filter_id: "a filter id" });
-        return [client, httpBackend];
-    };
-
     beforeEach(() => {
         [client, httpBackend] = setupTestClient();
     });
diff --git a/spec/unit/notifications.spec.ts b/spec/unit/notifications.spec.ts
index b0ebd23d9..4c8421dd6 100644
--- a/spec/unit/notifications.spec.ts
+++ b/spec/unit/notifications.spec.ts
@@ -106,6 +106,8 @@ describe("fixNotificationCountOnDecryption", () => {
             mockClient,
         );
 
+        room.addLiveEvents([event]);
+
         THREAD_ID = event.getId()!;
         threadEvent = mkEvent({
             type: EventType.RoomMessage,
diff --git a/src/client.ts b/src/client.ts
index a9531fcff..9afcddf29 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -9845,6 +9845,19 @@ export function fixNotificationCountOnDecryption(cli: MatrixClient, event: Matri
     const room = cli.getRoom(event.getRoomId());
     if (!room || !ourUserId || !eventId) return;
 
+    // Due to threads, we can get relation events (eg. edits & reactions) that never get
+    // added to a timeline and so cannot be found in their own room (their edit / reaction
+    // still applies to the event it needs to, so it doesn't matter too much). However, if
+    // we try to process notification about this event, we'll get very confused because we
+    // won't be able to find the event in the room, so will assume it must be unread, even
+    // if it's actually read. We therefore skip anything that isn't in the room. This isn't
+    // *great*, so if we can fix the homeless events (eg. with MSC4023) then we should probably
+    // remove this workaround.
+    if (!room.findEventById(eventId)) {
+        logger.info("Decrypted event is not in the room: ignoring");
+        return;
+    }
+
     const isThreadEvent = !!event.threadRootId && !event.isThreadRoot;
 
     let hasReadEvent;