[WIP] Rewrite receipt handling

spec/test-utils/thread.ts

@@ -161,3 +161,23 @@ export const mkThread = ({
 
     return { thread, rootEvent, events };
 };
+
+/**
+ * Create a thread, and make sure the events added to the thread and the room's
+ * timeline as if they came in via sync.
+ *
+ * Note that mkThread doesn't actually add the events properly to the room.
+ */
+export const populateThread = async ({
+    room,
+    client,
+    authorId,
+    participantUserIds,
+    length = 2,
+    ts = 1,
+}: MakeThreadProps): Promise<MakeThreadResult> => {
+    const ret = mkThread({ room, client, authorId, participantUserIds, length, ts });
+    ret.thread.initialEventsFetched = true;
+    room.addLiveEvents(ret.events);
+    return ret;
+};

spec/unit/models/thread.spec.ts

@@ -19,7 +19,7 @@ import { mocked } from "jest-mock";
 import { MatrixClient, PendingEventOrdering } from "../../../src/client";
 import { Room, RoomEvent } from "../../../src/models/room";
 import { FeatureSupport, Thread, THREAD_RELATION_TYPE, ThreadEvent } from "../../../src/models/thread";
-import { makeThreadEvent, mkThread } from "../../test-utils/thread";
+import { makeThreadEvent, mkThread, populateThread } from "../../test-utils/thread";
 import { TestClient } from "../../TestClient";
 import { emitPromise, mkEdit, mkMessage, mkReaction, mock } from "../../test-utils/test-utils";
 import { Direction, EventStatus, EventType, MatrixEvent } from "../../../src";
@@ -148,21 +148,39 @@ describe("Thread", () => {
             expect(thread.hasUserReadEvent(myUserId, events.at(-1)!.getId() ?? "")).toBeTruthy();
         });
 
-        it("considers other events with no RR as unread", () => {
-            const { thread, events } = mkThread({
+        it("considers other events with no RR as unread", async () => {
+            // Given a long thread exists
+            const { thread, events } = await populateThread({
                 room,
                 client,
-                authorId: myUserId,
-                participantUserIds: [myUserId],
+                authorId: "@other:foo.com",
+                participantUserIds: ["@other:foo.com"],
                 length: 25,
                 ts: 190,
             });
 
-            // Before alice's last unthreaded receipt
-            expect(thread.hasUserReadEvent("@alice:example.org", events.at(1)!.getId() ?? "")).toBeTruthy();
+            const event1 = events.at(1)!;
+            const event2 = events.at(2)!;
+            const event24 = events.at(24)!;
+
+            // And we have read the second message in it with an unthreaded receipt
+            const receipt = new MatrixEvent({
+                type: "m.receipt",
+                room_id: room.roomId,
+                content: {
+                    // unthreaded receipt for the second message in the thread
+                    [event2.getId()!]: {
+                        [ReceiptType.Read]: {
+                            [myUserId]: { ts: 200 },
+                        },
+                    },
+                },
+            });
+            room.addReceipt(receipt);
 
-            // After alice's last unthreaded receipt
-            expect(thread.hasUserReadEvent("@alice:example.org", events.at(-1)!.getId() ?? "")).toBeFalsy();
+            // Then we have read the first message in the thread, and not the last
+            expect(thread.hasUserReadEvent(myUserId, event1.getId()!)).toBe(true);
+            expect(thread.hasUserReadEvent(myUserId, event24.getId()!)).toBe(false);
         });
 
         it("considers event as read if there's a more recent unthreaded receipt", () => {
@@ -481,13 +499,13 @@ describe("Thread", () => {
 
                 // And a thread with an added event (with later timestamp)
                 const userId = "user1";
-                const { thread, message } = await createThreadAndEvent(client, 1, 100, userId);
+                const { thread, message2 } = await createThreadAnd2Events(client, 1, 100, 200, userId);
 
                 // Then a receipt was added to the thread
                 const receipt = thread.getReadReceiptForUserId(userId);
                 expect(receipt).toBeTruthy();
-                expect(receipt?.eventId).toEqual(message.getId());
-                expect(receipt?.data.ts).toEqual(100);
+                expect(receipt?.eventId).toEqual(message2.getId());
+                expect(receipt?.data.ts).toEqual(200);
                 expect(receipt?.data.thread_id).toEqual(thread.id);
 
                 // (And the receipt was synthetic)
@@ -505,14 +523,14 @@ describe("Thread", () => {
 
                 // And a thread with an added event with a lower timestamp than its other events
                 const userId = "user1";
-                const { thread } = await createThreadAndEvent(client, 200, 100, userId);
+                const { thread, message1 } = await createThreadAnd2Events(client, 300, 200, 100, userId);
 
-                // Then no receipt was added to the thread (the receipt is still
-                // for the thread root). This happens because since we have no
+                // Then the receipt is for the first message, because its
+                // timestamp is later. This happens because since we have no
                 // recursive relations support, we know that sometimes events
                 // appear out of order, so we have to check their timestamps as
                 // a guess of the correct order.
-                expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(thread.rootEvent?.getId());
+                expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(message1.getId());
             });
         });
 
@@ -530,11 +548,11 @@ describe("Thread", () => {
 
                 // And a thread with an added event (with later timestamp)
                 const userId = "user1";
-                const { thread, message } = await createThreadAndEvent(client, 1, 100, userId);
+                const { thread, message2 } = await createThreadAnd2Events(client, 1, 100, 200, userId);
 
                 // Then a receipt was added to the thread
                 const receipt = thread.getReadReceiptForUserId(userId);
-                expect(receipt?.eventId).toEqual(message.getId());
+                expect(receipt?.eventId).toEqual(message2.getId());
             });
 
             it("Creates a local echo receipt even for events BEFORE an existing receipt", async () => {
@@ -550,22 +568,24 @@ describe("Thread", () => {
 
                 // And a thread with an added event with a lower timestamp than its other events
                 const userId = "user1";
-                const { thread, message } = await createThreadAndEvent(client, 200, 100, userId);
+                const { thread, message2 } = await createThreadAnd2Events(client, 300, 200, 100, userId);
 
-                // Then a receipt was added to the thread, because relations
-                // recursion is available, so we trust the server to have
-                // provided us with events in the right order.
+                // Then a receipt was added for the last message, even though it
+                // has lower ts, because relations recursion is available, so we
+                // trust the server to have provided us with events in the right
+                // order.
                 const receipt = thread.getReadReceiptForUserId(userId);
-                expect(receipt?.eventId).toEqual(message.getId());
+                expect(receipt?.eventId).toEqual(message2.getId());
             });
         });
 
-        async function createThreadAndEvent(
+        async function createThreadAnd2Events(
             client: MatrixClient,
             rootTs: number,
-            eventTs: number,
+            message1Ts: number,
+            message2Ts: number,
             userId: string,
-        ): Promise<{ thread: Thread; message: MatrixEvent }> {
+        ): Promise<{ thread: Thread; message1: MatrixEvent; message2: MatrixEvent }> {
             const room = new Room("room1", client, userId);
 
             // Given a thread
@@ -576,24 +596,41 @@ describe("Thread", () => {
                 participantUserIds: [],
                 ts: rootTs,
             });
-            // Sanity: the current receipt is for the thread root
-            expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(thread.rootEvent?.getId());
+            // Sanity: there is no read receipt on the thread yet because the
+            // thread events don't get properly added to the room by mkThread.
+            expect(thread.getReadReceiptForUserId(userId)).toBeNull();
 
             const awaitTimelineEvent = new Promise<void>((res) => thread.on(RoomEvent.Timeline, () => res()));
 
-            // When we add a message that is before the latest receipt
-            const message = makeThreadEvent({
+            // Add a message with ts message1Ts
+            const message1 = makeThreadEvent({
+                event: true,
+                rootEventId: thread.id,
+                replyToEventId: thread.id,
+                user: userId,
+                room: room.roomId,
+                ts: message1Ts,
+            });
+            await thread.addEvent(message1, false, true);
+            await awaitTimelineEvent;
+
+            // Sanity: the thread now has a properly-added event, so this event
+            // has a synthetic receipt.
+            expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(message1.getId());
+
+            // Add a message with ts message2Ts
+            const message2 = makeThreadEvent({
                 event: true,
                 rootEventId: thread.id,
                 replyToEventId: thread.id,
                 user: userId,
                 room: room.roomId,
-                ts: eventTs,
+                ts: message2Ts,
             });
-            await thread.addEvent(message, false, true);
+            await thread.addEvent(message2, false, true);
             await awaitTimelineEvent;
 
-            return { thread, message };
+            return { thread, message1, message2 };
         }
 
         function createClientWithEventMapper(canSupport: Map<Feature, ServerSupport> = new Map()): MatrixClient {

spec/unit/read-receipt.spec.ts

@@ -43,8 +43,10 @@ const THREAD_ID = "$thread_event_id";
 const ROOM_ID = "!123:matrix.org";
 
 describe("Read receipt", () => {
+    let threadRoot: MatrixEvent;
     let threadEvent: MatrixEvent;
     let roomEvent: MatrixEvent;
+    let editOfThreadRoot: MatrixEvent;
 
     beforeEach(() => {
         httpBackend = new MockHttpBackend();
@@ -57,6 +59,15 @@ describe("Read receipt", () => {
         client.isGuest = () => false;
         client.supportsThreads = () => true;
 
+        threadRoot = utils.mkEvent({
+            event: true,
+            type: EventType.RoomMessage,
+            user: "@bob:matrix.org",
+            room: ROOM_ID,
+            content: { body: "This is the thread root" },
+        });
+        threadRoot.event.event_id = THREAD_ID;
+
         threadEvent = utils.mkEvent({
             event: true,
             type: EventType.RoomMessage,
@@ -82,6 +93,9 @@ describe("Read receipt", () => {
                 body: "Hello from a room",
             },
         });
+
+        editOfThreadRoot = utils.mkEdit(threadRoot, client, "@bob:matrix.org", ROOM_ID);
+        editOfThreadRoot.setThreadId(THREAD_ID);
     });
 
     describe("sendReceipt", () => {
@@ -208,6 +222,7 @@ describe("Read receipt", () => {
         it.each([
             { getEvent: () => roomEvent, destinationId: MAIN_ROOM_TIMELINE },
             { getEvent: () => threadEvent, destinationId: THREAD_ID },
+            { getEvent: () => editOfThreadRoot, destinationId: MAIN_ROOM_TIMELINE },
         ])("adds the receipt to $destinationId", ({ getEvent, destinationId }) => {
             const event = getEvent();
             const userId = "@bob:example.org";

spec/unit/room.spec.ts

@@ -1743,13 +1743,73 @@ describe("Room", function () {
         });
 
         describe("hasUserReadUpTo", function () {
-            it("should acknowledge if an event has been read", function () {
+            it("returns true if there is a receipt for this event (main timeline)", function () {
                 const ts = 13787898424;
+                room.addLiveEvents([eventToAck]);
                 room.addReceipt(mkReceipt(roomId, [mkRecord(eventToAck.getId()!, "m.read", userB, ts)]));
-                room.findEventById = jest.fn().mockReturnValue({} as MatrixEvent);
+                room.findEventById = jest.fn().mockReturnValue({ getThread: jest.fn() } as unknown as MatrixEvent);
                 expect(room.hasUserReadEvent(userB, eventToAck.getId()!)).toEqual(true);
             });
-            it("return false for an unknown event", function () {
+
+            it("returns true if there is a receipt for a later event (main timeline)", async function () {
+                // Given some events exist in the room
+                const events: MatrixEvent[] = [
+                    utils.mkMessage({
+                        room: roomId,
+                        user: userA,
+                        msg: "1111",
+                        event: true,
+                    }),
+                    utils.mkMessage({
+                        room: roomId,
+                        user: userA,
+                        msg: "2222",
+                        event: true,
+                    }),
+                    utils.mkMessage({
+                        room: roomId,
+                        user: userA,
+                        msg: "3333",
+                        event: true,
+                    }),
+                ];
+                await room.addLiveEvents(events);
+
+                // When I add a receipt for the latest one
+                room.addReceipt(mkReceipt(roomId, [mkRecord(events[2].getId()!, "m.read", userB, 102)]));
+
+                // Then the older ones are read too
+                expect(room.hasUserReadEvent(userB, events[0].getId()!)).toEqual(true);
+                expect(room.hasUserReadEvent(userB, events[1].getId()!)).toEqual(true);
+            });
+
+            describe("threads enabled", () => {
+                let oldSupportsThreads: () => boolean;
+
+                beforeEach(() => {
+                    oldSupportsThreads = room.client.supportsThreads;
+                    room.client.supportsThreads = jest.fn().mockReturnValue(true);
+                });
+
+                afterEach(() => {
+                    room.client.supportsThreads = oldSupportsThreads;
+                });
+
+                it("returns true if there is an unthreaded receipt for a later event in a thread", async () => {
+                    // Given a thread exists in the room
+                    const { thread, events } = mkThread({ room, length: 3 });
+                    thread.initialEventsFetched = true;
+                    await room.addLiveEvents(events);
+
+                    // When I add an unthreaded receipt for the latest thread message
+                    room.addReceipt(mkReceipt(roomId, [mkRecord(events[2].getId()!, "m.read", userB, 102)]));
+
+                    // Then the main timeline message is read
+                    expect(room.hasUserReadEvent(userB, events[0].getId()!)).toEqual(true);
+                });
+            });
+
+            it("returns false for an unknown event", function () {
                 expect(room.hasUserReadEvent(userB, "unknown_event")).toEqual(false);
             });
         });
@@ -2970,7 +3030,7 @@ describe("Room", function () {
             const threadRoot = mkMessage();
             const threadResponse1 = mkThreadResponse(threadRoot);
             const threadReaction1 = utils.mkReaction(threadRoot, room.client, userA, roomId);
-            const threadReaction2 = utils.mkReaction(threadRoot, room.client, userA, roomId);
+            const threadReaction2 = utils.mkEdit(threadRoot, room.client, userA, roomId);
             const threadReaction2Redaction = mkRedaction(threadReaction2);
 
             const roots = new Set([threadRoot.getId()!]);