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

spec/integ/matrix-client-unread-notifications.spec.ts

@@ -28,23 +28,19 @@ 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";
 
-describe("MatrixClient syncing", () => {
-    const userA = "@alice:localhost";
-    const userB = "@bob:localhost";
+const userA = "@alice:localhost";
+const userB = "@bob:localhost";
+const selfUserId = userA;
+const selfAccessToken = "aseukfgwef";
 
-    const selfUserId = userA;
-    const selfAccessToken = "aseukfgwef";
-
-    let client: MatrixClient | undefined;
-    let httpBackend: HttpBackend | undefined;
-
-    const setupTestClient = (): [MatrixClient, HttpBackend] => {
+function setupTestClient(): [MatrixClient, HttpBackend] {
     const testClient = new TestClient(selfUserId, "DEVICE", selfAccessToken);
     const httpBackend = testClient.httpBackend;
     const client = testClient.client;
@@ -52,7 +48,49 @@ describe("MatrixClient syncing", () => {
     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", () => {
+    let client: MatrixClient | undefined;
+    let httpBackend: HttpBackend | undefined;
 
     beforeEach(() => {
         [client, httpBackend] = setupTestClient();

spec/unit/notifications.spec.ts

@@ -106,6 +106,8 @@ describe("fixNotificationCountOnDecryption", () => {
             mockClient,
         );
 
+        room.addLiveEvents([event]);
+
         THREAD_ID = event.getId()!;
         threadEvent = mkEvent({
             type: EventType.RoomMessage,

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;