From 27403bf4b1b9fb2d0f7e2ac9d869933aae6a2c27 Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Mon, 13 Feb 2023 04:27:00 +0100 Subject: [PATCH 1/4] Associate event with thread before adding it to the thread timeline --- src/models/room.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/models/room.ts b/src/models/room.ts index 93bdb99a759..950d686c031 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -2467,6 +2467,7 @@ export class Room extends ReadReceipt { 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 +2549,7 @@ export class Room extends ReadReceipt { const { shouldLiveInRoom, threadId } = this.eventShouldLiveIn(event); const thread = threadId ? this.getThread(threadId) : undefined; + thread?.setEventMetadata(event); thread?.timelineSet.replaceEventId(oldEventId, newEventId!); if (shouldLiveInRoom) { From c0fcd5971f60ddba9df82739d8ceefcfc31ea93f Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Mon, 20 Feb 2023 18:49:57 +0100 Subject: [PATCH 2/4] Make sure events can be added to thread correctly --- src/models/room.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/models/room.ts b/src/models/room.ts index 950d686c031..262b971c3a0 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -2151,14 +2151,15 @@ export class Room extends ReadReceipt { // 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.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, From 994ac6be4ae8ff038e67d178f6d769e0d99dc6c8 Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Wed, 22 Feb 2023 15:40:01 +0100 Subject: [PATCH 3/4] Write initial test case --- ...matrix-client-unread-notifications.spec.ts | 95 ++++++++++++++++++- 1 file changed, 94 insertions(+), 1 deletion(-) diff --git a/spec/integ/matrix-client-unread-notifications.spec.ts b/spec/integ/matrix-client-unread-notifications.spec.ts index d65478ed84e..8274d7afaba 100644 --- a/spec/integ/matrix-client-unread-notifications.spec.ts +++ b/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((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"; From 020e8630e871a378350aefeba718315fdd14c6b1 Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 24 Feb 2023 12:45:11 +0100 Subject: [PATCH 4/4] Add additional comment for why the code had to be reordered --- src/models/room.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/models/room.ts b/src/models/room.ts index 262b971c3a0..399f9861437 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -2152,6 +2152,8 @@ export class Room extends ReadReceipt { 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: