Fix reactions in threads sometimes causing stuck notifications (#3146)

* Associate event with thread before adding it to the thread timeline

* Make sure events can be added to thread correctly

* Write initial test case

* Add additional comment for why the code had to be reordered

Author: justjanne

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

@@ -18,8 +18,21 @@ import "fake-indexeddb/auto";
 
 import HttpBackend from "matrix-mock-request";
 
-import { Category, ISyncResponse, MatrixClient, NotificationCountType, Room } from "../../src";
+import {
+    Category,
+    ClientEvent,
+    EventType,
+    ISyncResponse,
+    MatrixClient,
+    MatrixEvent,
+    NotificationCountType,
+    RelationType,
+    Room,
+} 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";
@@ -51,6 +64,86 @@ describe("MatrixClient syncing", () => {
         return httpBackend!.stop();
     });
 
+    it("reactions in thread set the correct timeline to unread", async () => {
+        const roomId = "!room:localhost";
+
+        // start the client, and wait for it to initialise
+        httpBackend!.when("GET", "/sync").respond(200, {
+            next_batch: "s_5_3",
+            rooms: {
+                [Category.Join]: {},
+                [Category.Leave]: {},
+                [Category.Invite]: {},
+            },
+        });
+        client!.startClient({ threadSupport: true });
+        await Promise.all([
+            httpBackend?.flushAllExpected(),
+            new Promise<void>((resolve) => {
+                client!.on(ClientEvent.Sync, (state) => state === SyncState.Syncing && resolve());
+            }),
+        ]);
+
+        const room = new Room(roomId, client!, selfUserId);
+        jest.spyOn(client!, "getRoom").mockImplementation((id) => (id === roomId ? room : null));
+
+        const thread = mkThread({ room, client: client!, authorId: selfUserId, participantUserIds: [selfUserId] });
+        const threadReply = thread.events.at(-1)!;
+        room.addLiveEvents([thread.rootEvent]);
+
+        // Initialize read receipt datastructure before testing the reaction
+        room.addReceiptToStructure(thread.rootEvent.getId()!, ReceiptType.Read, selfUserId, { ts: 1 }, false);
+        thread.thread.addReceiptToStructure(
+            threadReply.getId()!,
+            ReceiptType.Read,
+            selfUserId,
+            { thread_id: thread.thread.id, ts: 1 },
+            false,
+        );
+        expect(room.getReadReceiptForUserId(selfUserId, false)?.eventId).toEqual(thread.rootEvent.getId());
+        expect(thread.thread.getReadReceiptForUserId(selfUserId, false)?.eventId).toEqual(threadReply.getId());
+
+        const reactionEventId = `$9-${Math.random()}-${Math.random()}`;
+        let lastEvent: MatrixEvent | null = null;
+        jest.spyOn(client! as any, "sendEventHttpRequest").mockImplementation((event) => {
+            lastEvent = event as MatrixEvent;
+            return { event_id: reactionEventId };
+        });
+
+        await client!.sendEvent(roomId, EventType.Reaction, {
+            "m.relates_to": {
+                rel_type: RelationType.Annotation,
+                event_id: threadReply.getId(),
+                key: "",
+            },
+        });
+
+        expect(lastEvent!.getId()).toEqual(reactionEventId);
+        room.handleRemoteEcho(new MatrixEvent(lastEvent!.event), lastEvent!);
+
+        // Our ideal state after this is the following:
+        //
+        // Room: [synthetic: threadroot, actual: threadroot]
+        // Thread: [synthetic: threadreaction, actual: threadreply]
+        //
+        // The reaction and reply are both in the thread, and their receipts should be isolated to the thread.
+        // The reaction has not been acknowledged in a dedicated read receipt message, so only the synthetic receipt
+        // should be updated.
+
+        // Ensure the synthetic receipt for the room has not been updated
+        expect(room.getReadReceiptForUserId(selfUserId, false)?.eventId).toEqual(thread.rootEvent.getId());
+        expect(room.getEventReadUpTo(selfUserId, false)).toEqual(thread.rootEvent.getId());
+        // Ensure the actual receipt for the room has not been updated
+        expect(room.getReadReceiptForUserId(selfUserId, true)?.eventId).toEqual(thread.rootEvent.getId());
+        expect(room.getEventReadUpTo(selfUserId, true)).toEqual(thread.rootEvent.getId());
+        // Ensure the synthetic receipt for the thread has been updated
+        expect(thread.thread.getReadReceiptForUserId(selfUserId, false)?.eventId).toEqual(reactionEventId);
+        expect(thread.thread.getEventReadUpTo(selfUserId, false)).toEqual(reactionEventId);
+        // Ensure the actual receipt for the thread has not been updated
+        expect(thread.thread.getReadReceiptForUserId(selfUserId, true)?.eventId).toEqual(threadReply.getId());
+        expect(thread.thread.getEventReadUpTo(selfUserId, true)).toEqual(threadReply.getId());
+    });
+
     describe("Stuck unread notifications integration tests", () => {
         const ROOM_ID = "!room:localhost";
 

src/models/room.ts

@@ -2151,14 +2151,17 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
         // a reference to the cached receipts anymore.
         this.cachedThreadReadReceipts.delete(threadId);
 
+        // If we managed to create a thread and figure out its `id` then we can use it
+        // This has to happen before thread.addEvents, because that adds events to the eventtimeline, and the
+        // eventtimeline sometimes looks up thread information via the room.
+        this.threads.set(thread.id, thread);
+
         // This is necessary to be able to jump to events in threads:
         // If we jump to an event in a thread where neither the event, nor the root,
         // nor any thread event are loaded yet, we'll load the event as well as the thread root, create the thread,
         // and pass the event through this.
         thread.addEvents(events, false);
 
-        // If we managed to create a thread and figure out its `id` then we can use it
-        this.threads.set(thread.id, thread);
         this.reEmitter.reEmit(thread, [
             ThreadEvent.Delete,
             ThreadEvent.Update,
@@ -2467,6 +2470,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
 
         const { shouldLiveInRoom, threadId } = this.eventShouldLiveIn(remoteEvent);
         const thread = threadId ? this.getThread(threadId) : null;
+        thread?.setEventMetadata(localEvent);
         thread?.timelineSet.handleRemoteEcho(localEvent, oldEventId, newEventId);
 
         if (shouldLiveInRoom) {
@@ -2548,6 +2552,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
 
             const { shouldLiveInRoom, threadId } = this.eventShouldLiveIn(event);
             const thread = threadId ? this.getThread(threadId) : undefined;
+            thread?.setEventMetadata(event);
             thread?.timelineSet.replaceEventId(oldEventId, newEventId!);
 
             if (shouldLiveInRoom) {