Rewrite receipt-handling code (#3901)

* Rewrite receipt-handling code

* Add tests around dangling receipts

* Fix mark as read for some rooms

* Add missing word

---------

Co-authored-by: Florian Duros <florian.duros@ormaz.fr>
Co-authored-by: Florian Duros <florianduros@element.io>

spec/test-utils/thread.ts

@@ -161,3 +161,23 @@ export const mkThread = ({
 
     return { thread, rootEvent, events };
 };
+
+/**
+ * Create a thread, and make sure the events are 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 = ({
+    room,
+    client,
+    authorId,
+    participantUserIds,
+    length = 2,
+    ts = 1,
+}: MakeThreadProps): MakeThreadResult => {
+    const ret = mkThread({ room, client, authorId, participantUserIds, length, ts });
+    ret.thread.initialEventsFetched = true;
+    room.addLiveEvents(ret.events);
+    return ret;
+};

spec/unit/models/room-receipts.spec.ts

@@ -0,0 +1,541 @@
+/*
+Copyright 2023 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import { FeatureSupport, MatrixClient, MatrixEvent, ReceiptContent, THREAD_RELATION_TYPE, Thread } from "../../../src";
+import { Room } from "../../../src/models/room";
+
+/**
+ * Note, these tests check the functionality of the RoomReceipts class, but most
+ * of them access that functionality via the surrounding Room class, because a
+ * room is required for RoomReceipts to function, and this matches the pattern
+ * of how this code is used in the wild.
+ */
+describe("RoomReceipts", () => {
+    beforeAll(() => {
+        jest.replaceProperty(Thread, "hasServerSideSupport", FeatureSupport.Stable);
+    });
+
+    afterAll(() => {
+        jest.restoreAllMocks();
+    });
+
+    it("reports events unread if there are no receipts", () => {
+        // Given there are no receipts in the room
+        const room = createRoom();
+        const [event] = createEvent();
+        room.addLiveEvents([event]);
+
+        // When I ask about any event, then it is unread
+        expect(room.hasUserReadEvent(readerId, event.getId()!)).toBe(false);
+    });
+
+    it("reports events we sent as read even if there are no (real) receipts", () => {
+        // Given there are no receipts in the room
+        const room = createRoom();
+        const [event] = createEventSentBy(readerId);
+        room.addLiveEvents([event]);
+
+        // When I ask about an event I sent, it is read (because a synthetic
+        // receipt was created and stored in RoomReceipts)
+        expect(room.hasUserReadEvent(readerId, event.getId()!)).toBe(true);
+    });
+
+    it("reports read if we receive an unthreaded receipt for this event", () => {
+        // Given my event exists and is unread
+        const room = createRoom();
+        const [event, eventId] = createEvent();
+        room.addLiveEvents([event]);
+        expect(room.hasUserReadEvent(readerId, eventId)).toBe(false);
+
+        // When we receive a receipt for this event+user
+        room.addReceipt(createReceipt(readerId, event));
+
+        // Then that event is read
+        expect(room.hasUserReadEvent(readerId, eventId)).toBe(true);
+    });
+
+    it("reports read if we receive an unthreaded receipt for a later event", () => {
+        // Given we have 2 events
+        const room = createRoom();
+        const [event1, event1Id] = createEvent();
+        const [event2] = createEvent();
+        room.addLiveEvents([event1, event2]);
+
+        // When we receive a receipt for the later event
+        room.addReceipt(createReceipt(readerId, event2));
+
+        // Then the earlier one is read
+        expect(room.hasUserReadEvent(readerId, event1Id)).toBe(true);
+    });
+
+    it("reports read for a non-live event if we receive an unthreaded receipt for a live one", () => {
+        // Given we have 2 events: one live and one old
+        const room = createRoom();
+        const [oldEvent, oldEventId] = createEvent();
+        const [liveEvent] = createEvent();
+        room.addLiveEvents([liveEvent]);
+        createOldTimeline(room, [oldEvent]);
+
+        // When we receive a receipt for the live event
+        room.addReceipt(createReceipt(readerId, liveEvent));
+
+        // Then the earlier one is read
+        expect(room.hasUserReadEvent(readerId, oldEventId)).toBe(true);
+    });
+
+    it("compares by timestamp if two events are in separate old timelines", () => {
+        // Given we have 2 events, both in old timelines, with event2 after
+        // event1 in terms of timestamps
+        const room = createRoom();
+        const [event1, event1Id] = createEvent();
+        const [event2, event2Id] = createEvent();
+        event1.event.origin_server_ts = 1;
+        event2.event.origin_server_ts = 2;
+        createOldTimeline(room, [event1]);
+        createOldTimeline(room, [event2]);
+
+        // When we receive a receipt for the older event
+        room.addReceipt(createReceipt(readerId, event1));
+
+        // Then the earlier one is read and the later one is not
+        expect(room.hasUserReadEvent(readerId, event1Id)).toBe(true);
+        expect(room.hasUserReadEvent(readerId, event2Id)).toBe(false);
+    });
+
+    it("reports unread if we receive an unthreaded receipt for an earlier event", () => {
+        // Given we have 2 events
+        const room = createRoom();
+        const [event1] = createEvent();
+        const [event2, event2Id] = createEvent();
+        room.addLiveEvents([event1, event2]);
+
+        // When we receive a receipt for the earlier event
+        room.addReceipt(createReceipt(readerId, event1));
+
+        // Then the later one is unread
+        expect(room.hasUserReadEvent(readerId, event2Id)).toBe(false);
+    });
+
+    it("reports unread if we receive an unthreaded receipt for a different user", () => {
+        // Given my event exists and is unread
+        const room = createRoom();
+        const [event, eventId] = createEvent();
+        room.addLiveEvents([event]);
+        expect(room.hasUserReadEvent(readerId, eventId)).toBe(false);
+
+        // When we receive a receipt for another user
+        room.addReceipt(createReceipt(otherUserId, event));
+
+        // Then the event is still unread since the receipt was not for us
+        expect(room.hasUserReadEvent(readerId, eventId)).toBe(false);
+
+        // But it's read for the other person
+        expect(room.hasUserReadEvent(otherUserId, eventId)).toBe(true);
+    });
+
+    it("reports events we sent as read even if an earlier receipt arrives", () => {
+        // Given we sent an event after some other event
+        const room = createRoom();
+        const [previousEvent] = createEvent();
+        const [myEvent] = createEventSentBy(readerId);
+        room.addLiveEvents([previousEvent, myEvent]);
+
+        // And I just received a receipt for the previous event
+        room.addReceipt(createReceipt(readerId, previousEvent));
+
+        // When I ask about the event I sent, it is read (because of synthetic receipts)
+        expect(room.hasUserReadEvent(readerId, myEvent.getId()!)).toBe(true);
+    });
+
+    it("considers events after ones we sent to be unread", () => {
+        // Given we sent an event, then another event came in
+        const room = createRoom();
+        const [myEvent] = createEventSentBy(readerId);
+        const [laterEvent] = createEvent();
+        room.addLiveEvents([myEvent, laterEvent]);
+
+        // When I ask about the later event, it is unread (because it's after the synthetic receipt)
+        expect(room.hasUserReadEvent(readerId, laterEvent.getId()!)).toBe(false);
+    });
+
+    it("correctly reports readness even when receipts arrive out of order", () => {
+        // Given we have 3 events
+        const room = createRoom();
+        const [event1] = createEvent();
+        const [event2, event2Id] = createEvent();
+        const [event3, event3Id] = createEvent();
+        room.addLiveEvents([event1, event2, event3]);
+
+        // When we receive receipts for the older events out of order
+        room.addReceipt(createReceipt(readerId, event2));
+        room.addReceipt(createReceipt(readerId, event1));
+
+        // Then we correctly ignore the older receipt
+        expect(room.hasUserReadEvent(readerId, event2Id)).toBe(true);
+        expect(room.hasUserReadEvent(readerId, event3Id)).toBe(false);
+    });
+
+    it("reports read if we receive a threaded receipt for this event (main)", () => {
+        // Given my event exists and is unread
+        const room = createRoom();
+        const [event, eventId] = createEvent();
+        room.addLiveEvents([event]);
+        expect(room.hasUserReadEvent(readerId, eventId)).toBe(false);
+
+        // When we receive a receipt for this event+user
+        room.addReceipt(createThreadedReceipt(readerId, event, "main"));
+
+        // Then that event is read
+        expect(room.hasUserReadEvent(readerId, eventId)).toBe(true);
+    });
+
+    it("reports read if we receive a threaded receipt for this event (non-main)", () => {
+        // Given my event exists and is unread
+        const room = createRoom();
+        const [root, rootId] = createEvent();
+        const [event, eventId] = createThreadedEvent(root);
+        setupThread(room, root);
+        room.addLiveEvents([root, event]);
+        expect(room.hasUserReadEvent(readerId, eventId)).toBe(false);
+
+        // When we receive a receipt for this event on this thread
+        room.addReceipt(createThreadedReceipt(readerId, event, rootId));
+
+        // Then that event is read
+        expect(room.hasUserReadEvent(readerId, eventId)).toBe(true);
+    });
+
+    it("reports read if we receive an threaded receipt for a later event", () => {
+        // Given we have 2 events in a thread
+        const room = createRoom();
+        const [root, rootId] = createEvent();
+        const [event1, event1Id] = createThreadedEvent(root);
+        const [event2] = createThreadedEvent(root);
+        setupThread(room, root);
+        room.addLiveEvents([root, event1, event2]);
+
+        // When we receive a receipt for the later event
+        room.addReceipt(createThreadedReceipt(readerId, event2, rootId));
+
+        // Then the earlier one is read
+        expect(room.hasUserReadEvent(readerId, event1Id)).toBe(true);
+    });
+
+    it("reports unread if we receive an threaded receipt for an earlier event", () => {
+        // Given we have 2 events in a thread
+        const room = createRoom();
+        const [root, rootId] = createEvent();
+        const [event1] = createThreadedEvent(root);
+        const [event2, event2Id] = createThreadedEvent(root);
+        setupThread(room, root);
+        room.addLiveEvents([root, event1, event2]);
+
+        // When we receive a receipt for the earlier event
+        room.addReceipt(createThreadedReceipt(readerId, event1, rootId));
+
+        // Then the later one is unread
+        expect(room.hasUserReadEvent(readerId, event2Id)).toBe(false);
+    });
+
+    it("reports unread if we receive an threaded receipt for a different user", () => {
+        // Given my event exists and is unread
+        const room = createRoom();
+        const [root, rootId] = createEvent();
+        const [event, eventId] = createThreadedEvent(root);
+        setupThread(room, root);
+        room.addLiveEvents([root, event]);
+        expect(room.hasUserReadEvent(readerId, eventId)).toBe(false);
+
+        // When we receive a receipt for another user
+        room.addReceipt(createThreadedReceipt(otherUserId, event, rootId));
+
+        // Then the event is still unread since the receipt was not for us
+        expect(room.hasUserReadEvent(readerId, eventId)).toBe(false);
+
+        // But it's read for the other person
+        expect(room.hasUserReadEvent(otherUserId, eventId)).toBe(true);
+    });
+
+    it("reports unread if we receive a receipt for a later event in a different thread", () => {
+        // Given 2 events exist in different threads
+        const room = createRoom();
+        const [root1] = createEvent();
+        const [root2] = createEvent();
+        const [thread1, thread1Id] = createThreadedEvent(root1);
+        const [thread2] = createThreadedEvent(root2);
+        setupThread(room, root1);
+        setupThread(room, root2);
+        room.addLiveEvents([root1, root2, thread1, thread2]);
+
+        // When we receive a receipt for the later event
+        room.addReceipt(createThreadedReceipt(readerId, thread2, root2.getId()!));
+
+        // Then the old one is still unread since the receipt was not for this thread
+        expect(room.hasUserReadEvent(readerId, thread1Id)).toBe(false);
+    });
+
+    it("correctly reports readness even when threaded receipts arrive out of order", () => {
+        // Given we have 3 events
+        const room = createRoom();
+        const [root, rootId] = createEvent();
+        const [event1] = createThreadedEvent(root);
+        const [event2, event2Id] = createThreadedEvent(root);
+        const [event3, event3Id] = createThreadedEvent(root);
+        setupThread(room, root);
+        room.addLiveEvents([root, event1, event2, event3]);
+
+        // When we receive receipts for the older events out of order
+        room.addReceipt(createThreadedReceipt(readerId, event2, rootId));
+        room.addReceipt(createThreadedReceipt(readerId, event1, rootId));
+
+        // Then we correctly ignore the older receipt
+        expect(room.hasUserReadEvent(readerId, event2Id)).toBe(true);
+        expect(room.hasUserReadEvent(readerId, event3Id)).toBe(false);
+    });
+
+    it("correctly reports readness when mixing threaded and unthreaded receipts", () => {
+        // Given we have a setup from this presentation:
+        // https://docs.google.com/presentation/d/1H1gxRmRFAm8d71hCILWmpOYezsvdlb7cB6ANl-20Gns/edit?usp=sharing
+        //
+        //                     Main1----\
+        //                       |       ---Thread1a <- threaded receipt
+        //                       |             |
+        //                       |          Thread1b
+        // threaded receipt -> Main2--\
+        //                       |     ----------------Thread2a <- unthreaded receipt
+        //                     Main3                      |
+        //                                             Thread2b <- threaded receipt
+        //
+        const room = createRoom();
+        const [main1, main1Id] = createEvent();
+        const [main2, main2Id] = createEvent();
+        const [main3, main3Id] = createEvent();
+        const [thread1a, thread1aId] = createThreadedEvent(main1);
+        const [thread1b, thread1bId] = createThreadedEvent(main1);
+        const [thread2a, thread2aId] = createThreadedEvent(main2);
+        const [thread2b, thread2bId] = createThreadedEvent(main2);
+        setupThread(room, main1);
+        setupThread(room, main2);
+        room.addLiveEvents([main1, thread1a, thread1b, main2, thread2a, main3, thread2b]);
+
+        // And the timestamps on the events are consistent with the order above
+        main1.event.origin_server_ts = 1;
+        thread1a.event.origin_server_ts = 2;
+        thread1b.event.origin_server_ts = 3;
+        main2.event.origin_server_ts = 4;
+        thread2a.event.origin_server_ts = 5;
+        main3.event.origin_server_ts = 6;
+        thread2b.event.origin_server_ts = 7;
+        // (Note: in principle, we have the information needed to order these
+        // events without using their timestamps, since they all came in via
+        // addLiveEvents. In reality, some of them would have come in via the
+        // /relations API, making it impossible to get the correct ordering
+        // without MSC4033, which is why we fall back to timestamps. I.e. we
+        // definitely could fix the code to make the above
+        // timestamp-manipulation unnecessary, but it would only make this test
+        // neater, not actually help in the real world.)
+
+        // When the receipts arrive
+        room.addReceipt(createThreadedReceipt(readerId, main2, "main"));
+        room.addReceipt(createThreadedReceipt(readerId, thread1a, main1Id));
+        room.addReceipt(createReceipt(readerId, thread2a));
+        room.addReceipt(createThreadedReceipt(readerId, thread2b, main2Id));
+
+        // Then we correctly identify that only main3 is unread
+        expect(room.hasUserReadEvent(readerId, main1Id)).toBe(true);
+        expect(room.hasUserReadEvent(readerId, main2Id)).toBe(true);
+        expect(room.hasUserReadEvent(readerId, main3Id)).toBe(false);
+        expect(room.hasUserReadEvent(readerId, thread1aId)).toBe(true);
+        expect(room.hasUserReadEvent(readerId, thread1bId)).toBe(true);
+        expect(room.hasUserReadEvent(readerId, thread2aId)).toBe(true);
+        expect(room.hasUserReadEvent(readerId, thread2bId)).toBe(true);
+    });
+
+    describe("dangling receipts", () => {
+        it("reports unread if the unthreaded receipt is in a dangling state", () => {
+            const room = createRoom();
+            const [event, eventId] = createEvent();
+            // When we receive a receipt for this event+user
+            room.addReceipt(createReceipt(readerId, event));
+
+            // The event is not added in the room
+            // So the receipt is in a dangling state
+            expect(room.hasUserReadEvent(readerId, eventId)).toBe(false);
+
+            // Add the event to the room
+            // The receipt is removed from the dangling state
+            room.addLiveEvents([event]);
+
+            // Then the event is read
+            expect(room.hasUserReadEvent(readerId, eventId)).toBe(true);
+        });
+
+        it("reports unread if the threaded receipt is in a dangling state", () => {
+            const room = createRoom();
+            const [root, rootId] = createEvent();
+            const [event, eventId] = createThreadedEvent(root);
+            setupThread(room, root);
+
+            // When we receive a receipt for this event+user
+            room.addReceipt(createThreadedReceipt(readerId, event, rootId));
+
+            // The event is not added in the room
+            // So the receipt is in a dangling state
+            expect(room.hasUserReadEvent(readerId, eventId)).toBe(false);
+
+            // Add the events to the room
+            // The receipt is removed from the dangling state
+            room.addLiveEvents([root, event]);
+
+            // Then the event is read
+            expect(room.hasUserReadEvent(readerId, eventId)).toBe(true);
+        });
+
+        it("should handle multiple dangling receipts for the same event", () => {
+            const room = createRoom();
+            const [event, eventId] = createEvent();
+            // When we receive a receipt for this event+user
+            room.addReceipt(createReceipt(readerId, event));
+            // We receive another receipt in the same event for another user
+            room.addReceipt(createReceipt(otherUserId, event));
+
+            // The event is not added in the room
+            // So the receipt is in a dangling state
+            expect(room.hasUserReadEvent(readerId, eventId)).toBe(false);
+
+            // Add the event to the room
+            // The two receipts should be processed
+            room.addLiveEvents([event]);
+
+            // Then the event is read
+            // We expect that the receipt of `otherUserId` didn't replace/erase the receipt of `readerId`
+            expect(room.hasUserReadEvent(readerId, eventId)).toBe(true);
+        });
+    });
+});
+
+function createFakeClient(): MatrixClient {
+    return {
+        getUserId: jest.fn(),
+        getEventMapper: jest.fn().mockReturnValue(jest.fn()),
+        isInitialSyncComplete: jest.fn().mockReturnValue(true),
+        supportsThreads: jest.fn().mockReturnValue(true),
+        fetchRoomEvent: jest.fn().mockResolvedValue({}),
+        paginateEventTimeline: jest.fn(),
+        canSupport: { get: jest.fn() },
+    } as unknown as MatrixClient;
+}
+
+const senderId = "sender:s.ss";
+const readerId = "reader:r.rr";
+const otherUserId = "other:o.oo";
+
+function createRoom(): Room {
+    return new Room("!rid", createFakeClient(), "@u:s.nz", { timelineSupport: true });
+}
+
+let idCounter = 0;
+function nextId(): string {
+    return "$" + (idCounter++).toString(10);
+}
+
+/**
+ * Create an event and return it and its ID.
+ */
+function createEvent(): [MatrixEvent, string] {
+    return createEventSentBy(senderId);
+}
+
+/**
+ * Create an event with the supplied sender and return it and its ID.
+ */
+function createEventSentBy(customSenderId: string): [MatrixEvent, string] {
+    const event = new MatrixEvent({ sender: customSenderId, event_id: nextId() });
+    return [event, event.getId()!];
+}
+
+/**
+ * Create an event in the thread of the supplied root and return it and its ID.
+ */
+function createThreadedEvent(root: MatrixEvent): [MatrixEvent, string] {
+    const rootEventId = root.getId()!;
+    const event = new MatrixEvent({
+        sender: senderId,
+        event_id: nextId(),
+        content: {
+            "m.relates_to": {
+                event_id: rootEventId,
+                rel_type: THREAD_RELATION_TYPE.name,
+                ["m.in_reply_to"]: {
+                    event_id: rootEventId,
+                },
+            },
+        },
+    });
+    return [event, event.getId()!];
+}
+
+function createReceipt(userId: string, referencedEvent: MatrixEvent): MatrixEvent {
+    const content: ReceiptContent = {
+        [referencedEvent.getId()!]: {
+            "m.read": {
+                [userId]: {
+                    ts: 123,
+                },
+            },
+        },
+    };
+
+    return new MatrixEvent({
+        type: "m.receipt",
+        content,
+    });
+}
+
+function createThreadedReceipt(userId: string, referencedEvent: MatrixEvent, threadId: string): MatrixEvent {
+    const content: ReceiptContent = {
+        [referencedEvent.getId()!]: {
+            "m.read": {
+                [userId]: {
+                    ts: 123,
+                    thread_id: threadId,
+                },
+            },
+        },
+    };
+
+    return new MatrixEvent({
+        type: "m.receipt",
+        content,
+    });
+}
+
+/**
+ * Create a timeline in the timeline set that is not the live timeline.
+ */
+function createOldTimeline(room: Room, events: MatrixEvent[]) {
+    const oldTimeline = room.getUnfilteredTimelineSet().addTimeline();
+    room.getUnfilteredTimelineSet().addEventsToTimeline(events, true, oldTimeline);
+}
+
+/**
+ * Perform the hacks required for this room to create a thread based on the root
+ * event supplied.
+ */
+function setupThread(room: Room, root: MatrixEvent) {
+    const thread = room.createThread(root.getId()!, root, [root], false);
+    thread.initialEventsFetched = true;
+}

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";
@@ -149,20 +149,38 @@ describe("Thread", () => {
         });
 
         it("considers other events with no RR as unread", () => {
-            const { thread, events } = mkThread({
+            // Given a long thread exists
+            const { thread, events } = populateThread({
                 room,
                 client,
-                authorId: myUserId,
+                authorId: "@other:foo.com",
-                participantUserIds: [myUserId],
+                participantUserIds: ["@other:foo.com"],
                 length: 25,
                 ts: 190,
             });
 
-            // Before alice's last unthreaded receipt
+            const event1 = events.at(1)!;
-            expect(thread.hasUserReadEvent("@alice:example.org", events.at(1)!.getId() ?? "")).toBeTruthy();
+            const event2 = events.at(2)!;
+            const event24 = events.at(24)!;
 
-            // After alice's last unthreaded receipt
+            // And we have read the second message in it with an unthreaded receipt
-            expect(thread.hasUserReadEvent("@alice:example.org", events.at(-1)!.getId() ?? "")).toBeFalsy();
+            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);
+
+            // 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?.eventId).toEqual(message2.getId());
-                expect(receipt?.data.ts).toEqual(100);
+                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
+                // Then the receipt is for the first message, because its
-                // for the thread root). This happens because since we have no
+                // 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
+                // Then a receipt was added for the last message, even though it
-                // recursion is available, so we trust the server to have
+                // has lower ts, because relations recursion is available, so we
-                // provided us with events in the right order.
+                // 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
+            // Sanity: there is no read receipt on the thread yet because the
-            expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(thread.rootEvent?.getId());
+            // 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
+            // Add a message with ts message1Ts
-            const message = makeThreadEvent({
+            const message1 = makeThreadEvent({
                 event: true,
                 rootEventId: thread.id,
                 replyToEventId: thread.id,
                 user: userId,
                 room: room.roomId,
-                ts: eventTs,
+                ts: message1Ts,
             });
-            await thread.addEvent(message, false, true);
+            await thread.addEvent(message1, false, true);
             await awaitTimelineEvent;
 
-            return { thread, message };
+            // 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: message2Ts,
+            });
+            await thread.addEvent(message2, false, true);
+            await awaitTimelineEvent;
+
+            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,70 @@ 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", () => {
+                beforeEach(() => {
+                    jest.spyOn(room.client, "supportsThreads").mockReturnValue(true);
+                });
+
+                afterEach(() => {
+                    jest.restoreAllMocks();
+                });
+
+                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);
             });
         });

src/client.ts

@@ -5168,7 +5168,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
 
         const room = this.getRoom(event.getRoomId());
         if (room && this.credentials.userId) {
-            room.addLocalEchoReceipt(this.credentials.userId, event, receiptType);
+            room.addLocalEchoReceipt(this.credentials.userId, event, receiptType, unthreaded);
         }
         return promise;
     }
@@ -9916,7 +9916,7 @@ export function threadIdForReceipt(event: MatrixEvent): string {
  * @returns true if this event is considered to be in the main timeline as far
  *               as receipts are concerned.
  */
-function inMainTimelineForReceipt(event: MatrixEvent): boolean {
+export function inMainTimelineForReceipt(event: MatrixEvent): boolean {
     if (!event.threadRootId) {
         // Not in a thread: then it is in the main timeline
         return true;

src/models/compare-event-ordering.ts

@@ -0,0 +1,139 @@
+/*
+Copyright 2023 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import { MatrixEvent } from "./event";
+import { Room } from "./room";
+import { inMainTimelineForReceipt, threadIdForReceipt } from "../client";
+
+/**
+ * Determine the order of two events in a room.
+ *
+ * In principle this should use the same order as the server, but in practice
+ * this is difficult for events that were not received over the Sync API. See
+ * MSC4033 for details.
+ *
+ * This implementation leans on the order of events within their timelines, and
+ * falls back to comparing event timestamps when they are in different
+ * timelines.
+ *
+ * See https://github.com/matrix-org/matrix-js-sdk/issues/3325 for where we are
+ * tracking the work to fix this.
+ *
+ * @param room - the room we are looking in
+ * @param leftEventId - the id of the first event
+ * @param rightEventId - the id of the second event
+
+ * @returns -1 if left \< right, 1 if left \> right, 0 if left == right, null if
+ *          we can't tell (because we can't find the events).
+ */
+export function compareEventOrdering(room: Room, leftEventId: string, rightEventId: string): number | null {
+    const leftEvent = room.findEventById(leftEventId);
+    const rightEvent = room.findEventById(rightEventId);
+
+    if (!leftEvent || !rightEvent) {
+        // Without the events themselves, we can't find their thread or
+        // timeline, or guess based on timestamp, so we just don't know.
+        return null;
+    }
+
+    // Check whether the events are in the main timeline
+    const isLeftEventInMainTimeline = inMainTimelineForReceipt(leftEvent);
+    const isRightEventInMainTimeline = inMainTimelineForReceipt(rightEvent);
+
+    if (isLeftEventInMainTimeline && isRightEventInMainTimeline) {
+        return compareEventsInMainTimeline(room, leftEventId, rightEventId, leftEvent, rightEvent);
+    } else {
+        // At least one event is not in the timeline, so we can't use the room's
+        // unfiltered timeline set.
+        return compareEventsInThreads(leftEventId, rightEventId, leftEvent, rightEvent);
+    }
+}
+
+function compareEventsInMainTimeline(
+    room: Room,
+    leftEventId: string,
+    rightEventId: string,
+    leftEvent: MatrixEvent,
+    rightEvent: MatrixEvent,
+): number | null {
+    // Get the timeline set that contains all the events.
+    const timelineSet = room.getUnfilteredTimelineSet();
+
+    // If they are in the same timeline, compareEventOrdering does what we need
+    const compareSameTimeline = timelineSet.compareEventOrdering(leftEventId, rightEventId);
+    if (compareSameTimeline !== null) {
+        return compareSameTimeline;
+    }
+
+    // Find which timeline each event is in. Refuse to provide an ordering if we
+    // can't find either of the events.
+
+    const leftTimeline = timelineSet.getTimelineForEvent(leftEventId);
+    if (leftTimeline === timelineSet.getLiveTimeline()) {
+        // The left event is part of the live timeline, so it must be after the
+        // right event (since they are not in the same timeline or we would have
+        // returned after compareEventOrdering.
+        return 1;
+    }
+
+    const rightTimeline = timelineSet.getTimelineForEvent(rightEventId);
+    if (rightTimeline === timelineSet.getLiveTimeline()) {
+        // The right event is part of the live timeline, so it must be after the
+        // left event.
+        return -1;
+    }
+
+    // They are in older timeline sets (because they were fetched by paging up).
+    return guessOrderBasedOnTimestamp(leftEvent, rightEvent);
+}
+
+function compareEventsInThreads(
+    leftEventId: string,
+    rightEventId: string,
+    leftEvent: MatrixEvent,
+    rightEvent: MatrixEvent,
+): number | null {
+    const leftEventThreadId = threadIdForReceipt(leftEvent);
+    const rightEventThreadId = threadIdForReceipt(rightEvent);
+
+    const leftThread = leftEvent.getThread();
+
+    if (leftThread && leftEventThreadId === rightEventThreadId) {
+        // They are in the same thread, so we can ask the thread's timeline to
+        // figure it out for us
+        return leftThread.timelineSet.compareEventOrdering(leftEventId, rightEventId);
+    } else {
+        return guessOrderBasedOnTimestamp(leftEvent, rightEvent);
+    }
+}
+
+/**
+ * Guess the order of events based on server timestamp. This is not good, but
+ * difficult to avoid without MSC4033.
+ *
+ * See https://github.com/matrix-org/matrix-js-sdk/issues/3325
+ */
+function guessOrderBasedOnTimestamp(leftEvent: MatrixEvent, rightEvent: MatrixEvent): number {
+    const leftTs = leftEvent.getTs();
+    const rightTs = rightEvent.getTs();
+    if (leftTs < rightTs) {
+        return -1;
+    } else if (leftTs > rightTs) {
+        return 1;
+    } else {
+        return 0;
+    }
+}

src/models/event-timeline-set.ts

@@ -899,11 +899,10 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
      * @param eventId1 -   The id of the first event
      * @param eventId2 -   The id of the second event
 
-     * @returns a number less than zero if eventId1 precedes eventId2, and
+     * @returns -1 if eventId1 precedes eventId2, and +1 eventId1 succeeds
-     *    greater than zero if eventId1 succeeds eventId2. zero if they are the
+     * eventId2. 0 if they are the same event; null if we can't tell (either
-     *    same event; null if we can't tell (either because we don't know about one
+     * because we don't know about one of the events, or because they are in
-     *    of the events, or because they are in separate timelines which don't join
+     * separate timelines which don't join up).
-     *    up).
      */
     public compareEventOrdering(eventId1: string, eventId2: string): number | null {
         if (eventId1 == eventId2) {
@@ -935,7 +934,16 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
                     idx2 = idx;
                 }
             }
-            return idx1! - idx2!;
+            const difference = idx1! - idx2!;
+
+            // Return the sign of difference.
+            if (difference < 0) {
+                return -1;
+            } else if (difference > 0) {
+                return 1;
+            } else {
+                return 0;
+            }
         }
 
         // the events are in different timelines. Iterate through the

src/models/read-receipt.ts

@@ -27,15 +27,29 @@ import { EventTimelineSet } from "./event-timeline-set";
 import { MapWithDefault } from "../utils";
 import { NotificationCountType } from "./room";
 import { logger } from "../logger";
+import { inMainTimelineForReceipt, threadIdForReceipt } from "../client";
 
-export function synthesizeReceipt(userId: string, event: MatrixEvent, receiptType: ReceiptType): MatrixEvent {
+/**
+ * Create a synthetic receipt for the given event
+ * @param userId - The user ID if the receipt sender
+ * @param event - The event that is to be acknowledged
+ * @param receiptType - The type of receipt
+ * @param unthreaded - the receipt is unthreaded
+ * @returns a new event with the synthetic receipt in it
+ */
+export function synthesizeReceipt(
+    userId: string,
+    event: MatrixEvent,
+    receiptType: ReceiptType,
+    unthreaded = false,
+): MatrixEvent {
     return new MatrixEvent({
         content: {
             [event.getId()!]: {
                 [receiptType]: {
                     [userId]: {
                         ts: event.getTs(),
-                        thread_id: event.threadRootId ?? MAIN_ROOM_TIMELINE,
+                        ...(!unthreaded && { thread_id: threadIdForReceipt(event) }),
                     },
                 },
             },
@@ -160,11 +174,8 @@ export abstract class ReadReceipt<
             // The receipt is for the main timeline: we check that the event is
             // in the main timeline.
 
-            // There are two ways to know an event is in the main timeline:
+            // Check if the event is in the main timeline
-            // either it has no threadRootId, or it is a thread root.
+            const eventIsInMainTimeline = inMainTimelineForReceipt(event);
-            // (Note: it's a little odd because the thread root is in the main
-            // timeline, but it still has a threadRootId.)
-            const eventIsInMainTimeline = !event.threadRootId || event.isThreadRoot;
 
             if (eventIsInMainTimeline) {
                 // The receipt is for the main timeline, and so is the event, so
@@ -367,9 +378,10 @@ export abstract class ReadReceipt<
      * @param userId - The user ID if the receipt sender
      * @param e - The event that is to be acknowledged
      * @param receiptType - The type of receipt
+     * @param unthreaded - the receipt is unthreaded
      */
-    public addLocalEchoReceipt(userId: string, e: MatrixEvent, receiptType: ReceiptType): void {
+    public addLocalEchoReceipt(userId: string, e: MatrixEvent, receiptType: ReceiptType, unthreaded = false): void {
-        this.addReceipt(synthesizeReceipt(userId, e, receiptType), true);
+        this.addReceipt(synthesizeReceipt(userId, e, receiptType, unthreaded), true);
     }
 
     /**
@@ -395,33 +407,7 @@ export abstract class ReadReceipt<
      * @param eventId - The event ID to check if the user read.
      * @returns True if the user has read the event, false otherwise.
      */
-    public hasUserReadEvent(userId: string, eventId: string): boolean {
+    public abstract hasUserReadEvent(userId: string, eventId: string): boolean;
-        const readUpToId = this.getEventReadUpTo(userId, false);
-        if (readUpToId === eventId) return true;
-
-        if (
-            this.timeline?.length &&
-            this.timeline[this.timeline.length - 1].getSender() &&
-            this.timeline[this.timeline.length - 1].getSender() === userId
-        ) {
-            // It doesn't matter where the event is in the timeline, the user has read
-            // it because they've sent the latest event.
-            return true;
-        }
-
-        for (let i = this.timeline?.length - 1; i >= 0; --i) {
-            const ev = this.timeline[i];
-
-            // If we encounter the target event first, the user hasn't read it
-            // however if we encounter the readUpToId first then the user has read
-            // it. These rules apply because we're iterating bottom-up.
-            if (ev.getId() === eventId) return false;
-            if (ev.getId() === readUpToId) return true;
-        }
-
-        // We don't know if the user has read it, so assume not.
-        return false;
-    }
 
     /**
      * Returns the most recent unthreaded receipt for a given user
@@ -429,6 +415,8 @@ export abstract class ReadReceipt<
      * @returns an unthreaded Receipt. Can be undefined if receipts have been disabled
      * or a user chooses to use private read receipts (or we have simply not received
      * a receipt from this user yet).
+     *
+     * @deprecated use `hasUserReadEvent` or `getEventReadUpTo` instead
      */
     public abstract getLastUnthreadedReceiptFor(userId: string): Receipt | undefined;
 }

src/models/room-receipts.ts

@@ -0,0 +1,429 @@
+/*
+Copyright 2023 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import { MAIN_ROOM_TIMELINE, Receipt, ReceiptContent } from "../@types/read_receipts";
+import { threadIdForReceipt } from "../client";
+import { Room, RoomEvent } from "./room";
+import { MatrixEvent } from "./event";
+
+/**
+ * The latest receipts we have for a room.
+ */
+export class RoomReceipts {
+    private room: Room;
+    private threadedReceipts: ThreadedReceipts;
+    private unthreadedReceipts: ReceiptsByUser;
+    private danglingReceipts: DanglingReceipts;
+
+    public constructor(room: Room) {
+        this.room = room;
+        this.threadedReceipts = new ThreadedReceipts(room);
+        this.unthreadedReceipts = new ReceiptsByUser(room);
+        this.danglingReceipts = new DanglingReceipts();
+        // We listen for timeline events so we can process dangling receipts
+        room.on(RoomEvent.Timeline, this.onTimelineEvent);
+    }
+
+    /**
+     * Remember the receipt information supplied. For each receipt:
+     *
+     * If we don't have the event for this receipt, store it as "dangling" so we
+     * can process it later.
+     *
+     * Otherwise store it per-user in either the threaded store for its
+     * thread_id, or the unthreaded store if there is no thread_id.
+     *
+     * Ignores any receipt that is before an existing receipt for the same user
+     * (in the same thread, if applicable). "Before" is defined by the
+     * unfilteredTimelineSet of the room.
+     */
+    public add(receiptContent: ReceiptContent, synthetic: boolean): void {
+        /*
+            Transform this structure:
+            {
+              "$EVENTID": {
+                "m.read|m.read.private": {
+                  "@user:example.org": {
+                    "ts": 1661,
+                    "thread_id": "main|$THREAD_ROOT_ID" // or missing/undefined for an unthreaded receipt
+                  }
+                }
+              },
+              ...
+            }
+            into maps of:
+            threaded :: threadid :: userId :: ReceiptInfo
+            unthreaded :: userId :: ReceiptInfo
+            dangling :: eventId :: DanglingReceipt
+        */
+        for (const [eventId, eventReceipt] of Object.entries(receiptContent)) {
+            for (const [receiptType, receiptsByUser] of Object.entries(eventReceipt)) {
+                for (const [userId, receipt] of Object.entries(receiptsByUser)) {
+                    const referencedEvent = this.room.findEventById(eventId);
+                    if (!referencedEvent) {
+                        this.danglingReceipts.add(
+                            new DanglingReceipt(eventId, receiptType, userId, receipt, synthetic),
+                        );
+                    } else if (receipt.thread_id) {
+                        this.threadedReceipts.set(
+                            receipt.thread_id,
+                            eventId,
+                            receiptType,
+                            userId,
+                            receipt.ts,
+                            synthetic,
+                        );
+                    } else {
+                        this.unthreadedReceipts.set(eventId, receiptType, userId, receipt.ts, synthetic);
+                    }
+                }
+            }
+        }
+    }
+
+    /**
+     * Look for dangling receipts for the given event ID,
+     * and add them to the thread of unthread receipts if found.
+     * @param eventId - the event ID to look for
+     */
+    private onTimelineEvent = (event: MatrixEvent): void => {
+        const eventId = event.getId();
+        if (!eventId) return;
+
+        const danglingReceipts = this.danglingReceipts.remove(eventId);
+
+        danglingReceipts?.forEach((danglingReceipt) => {
+            // The receipt is a thread receipt
+            if (danglingReceipt.receipt.thread_id) {
+                this.threadedReceipts.set(
+                    danglingReceipt.receipt.thread_id,
+                    danglingReceipt.eventId,
+                    danglingReceipt.receiptType,
+                    danglingReceipt.userId,
+                    danglingReceipt.receipt.ts,
+                    danglingReceipt.synthetic,
+                );
+            } else {
+                this.unthreadedReceipts.set(
+                    eventId,
+                    danglingReceipt.receiptType,
+                    danglingReceipt.userId,
+                    danglingReceipt.receipt.ts,
+                    danglingReceipt.synthetic,
+                );
+            }
+        });
+    };
+
+    public hasUserReadEvent(userId: string, eventId: string): boolean {
+        const unthreaded = this.unthreadedReceipts.get(userId);
+        if (unthreaded) {
+            if (isAfterOrSame(unthreaded.eventId, eventId, this.room)) {
+                // The unthreaded receipt is after this event, so we have read it.
+                return true;
+            }
+        }
+
+        const event = this.room.findEventById(eventId);
+        if (!event) {
+            // We don't know whether the user has read it - default to caution and say no.
+            return false;
+        }
+
+        const threadId = threadIdForReceipt(event);
+        const threaded = this.threadedReceipts.get(threadId, userId);
+        if (threaded) {
+            if (isAfterOrSame(threaded.eventId, eventId, this.room)) {
+                // The threaded receipt is after this event, so we have read it.
+                return true;
+            }
+        }
+
+        // TODO: what if they sent the second-last event in the thread?
+        if (this.userSentLatestEventInThread(threadId, userId)) {
+            // The user sent the latest message in this event's thread, so we
+            // consider everything in the thread to be read.
+            //
+            // Note: maybe we don't need this because synthetic receipts should
+            // do this job for us?
+            return true;
+        }
+
+        // Neither of the receipts were after the event, so it's unread.
+        return false;
+    }
+
+    /**
+     * @returns true if the thread with this ID can be found, and the supplied
+     *          user sent the latest message in it.
+     */
+    private userSentLatestEventInThread(threadId: string, userId: String): boolean {
+        const timeline =
+            threadId === MAIN_ROOM_TIMELINE
+                ? this.room.getLiveTimeline().getEvents()
+                : this.room.getThread(threadId)?.timeline;
+
+        return !!(timeline && timeline.length > 0 && timeline[timeline.length - 1].getSender() === userId);
+    }
+}
+
+// --- implementation details ---
+
+/**
+ * The information "inside" a receipt once it has been stored inside
+ * RoomReceipts - what eventId it refers to, its type, and its ts.
+ *
+ * Does not contain userId or threadId since these are stored as keys of the
+ * maps in RoomReceipts.
+ */
+class ReceiptInfo {
+    public constructor(public eventId: string, public receiptType: string, public ts: number) {}
+}
+
+/**
+ * Everything we know about a receipt that is "dangling" because we can't find
+ * the event to which it refers.
+ */
+class DanglingReceipt {
+    public constructor(
+        public eventId: string,
+        public receiptType: string,
+        public userId: string,
+        public receipt: Receipt,
+        public synthetic: boolean,
+    ) {}
+}
+
+class UserReceipts {
+    private room: Room;
+
+    /**
+     * The real receipt for this user.
+     */
+    private real: ReceiptInfo | undefined;
+
+    /**
+     * The synthetic receipt for this user. If this is defined, it is later than real.
+     */
+    private synthetic: ReceiptInfo | undefined;
+
+    public constructor(room: Room) {
+        this.room = room;
+        this.real = undefined;
+        this.synthetic = undefined;
+    }
+
+    public set(synthetic: boolean, receiptInfo: ReceiptInfo): void {
+        if (synthetic) {
+            this.synthetic = receiptInfo;
+        } else {
+            this.real = receiptInfo;
+        }
+
+        // Preserve the invariant: synthetic is only defined if it's later than real
+        if (this.synthetic && this.real) {
+            if (isAfterOrSame(this.real.eventId, this.synthetic.eventId, this.room)) {
+                this.synthetic = undefined;
+            }
+        }
+    }
+
+    /**
+     * Return the latest receipt we have - synthetic if we have one (and it's
+     * later), otherwise real.
+     */
+    public get(): ReceiptInfo | undefined {
+        // Relies on the invariant that synthetic is only defined if it's later than real.
+        return this.synthetic ?? this.real;
+    }
+
+    /**
+     * Return the latest receipt we have of the specified type (synthetic or not).
+     */
+    public getByType(synthetic: boolean): ReceiptInfo | undefined {
+        return synthetic ? this.synthetic : this.real;
+    }
+}
+
+/**
+ * The latest receipt info we have, either for a single thread, or all the
+ * unthreaded receipts for a room.
+ *
+ * userId: ReceiptInfo
+ */
+class ReceiptsByUser {
+    private room: Room;
+
+    /** map of userId: UserReceipts */
+    private data: Map<String, UserReceipts>;
+
+    public constructor(room: Room) {
+        this.room = room;
+        this.data = new Map<string, UserReceipts>();
+    }
+
+    /**
+     * Add the supplied receipt to our structure, if it is not earlier than the
+     * one we already hold for this user.
+     */
+    public set(eventId: string, receiptType: string, userId: string, ts: number, synthetic: boolean): void {
+        const userReceipts = getOrCreate(this.data, userId, () => new UserReceipts(this.room));
+
+        const existingReceipt = userReceipts.getByType(synthetic);
+        if (existingReceipt && isAfter(existingReceipt.eventId, eventId, this.room)) {
+            // The new receipt is before the existing one - don't store it.
+            return;
+        }
+
+        // Possibilities:
+        //
+        // 1. there was no existing receipt, or
+        // 2. the existing receipt was before this one, or
+        // 3. we were unable to compare the receipts.
+        //
+        // In the case of 3 it's difficult to decide what to do, so the
+        // most-recently-received receipt wins.
+        //
+        // Case 3 can only happen if the events for these receipts have
+        // disappeared, which is quite unlikely since the new one has just been
+        // checked, and the old one was checked before it was inserted here.
+        //
+        // We go ahead and store this receipt (replacing the other if it exists)
+        userReceipts.set(synthetic, new ReceiptInfo(eventId, receiptType, ts));
+    }
+
+    /**
+     * Find the latest receipt we have for this user. (Note - there is only one
+     * receipt per user, because we are already inside a specific thread or
+     * unthreaded list.)
+     *
+     * If there is a later synthetic receipt for this user, return that.
+     * Otherwise, return the real receipt.
+     *
+     * @returns the found receipt info, or undefined if we have no receipt for this user.
+     */
+    public get(userId: string): ReceiptInfo | undefined {
+        return this.data.get(userId)?.get();
+    }
+}
+
+/**
+ * The latest threaded receipts we have for a room.
+ */
+class ThreadedReceipts {
+    private room: Room;
+
+    /** map of threadId: ReceiptsByUser */
+    private data: Map<string, ReceiptsByUser>;
+
+    public constructor(room: Room) {
+        this.room = room;
+        this.data = new Map<string, ReceiptsByUser>();
+    }
+
+    /**
+     * Add the supplied receipt to our structure, if it is not earlier than one
+     * we already hold for this user in this thread.
+     */
+    public set(
+        threadId: string,
+        eventId: string,
+        receiptType: string,
+        userId: string,
+        ts: number,
+        synthetic: boolean,
+    ): void {
+        const receiptsByUser = getOrCreate(this.data, threadId, () => new ReceiptsByUser(this.room));
+        receiptsByUser.set(eventId, receiptType, userId, ts, synthetic);
+    }
+
+    /**
+     * Find the latest threaded receipt for the supplied user in the supplied thread.
+     *
+     * @returns the found receipt info or undefined if we don't have one.
+     */
+    public get(threadId: string, userId: string): ReceiptInfo | undefined {
+        return this.data.get(threadId)?.get(userId);
+    }
+}
+
+/**
+ * All the receipts that we have received but can't process because we can't
+ * find the event they refer to.
+ *
+ * We hold on to them so we can process them if their event arrives later.
+ */
+class DanglingReceipts {
+    /**
+     * eventId: DanglingReceipt[]
+     */
+    private data = new Map<string, Array<DanglingReceipt>>();
+
+    /**
+     * Remember the supplied dangling receipt.
+     */
+    public add(danglingReceipt: DanglingReceipt): void {
+        const danglingReceipts = getOrCreate(this.data, danglingReceipt.eventId, () => []);
+        danglingReceipts.push(danglingReceipt);
+    }
+
+    /**
+     * Remove and return the dangling receipts for the given event ID.
+     * @param eventId - the event ID to look for
+     * @returns the found dangling receipts, or undefined if we don't have one.
+     */
+    public remove(eventId: string): Array<DanglingReceipt> | undefined {
+        const danglingReceipts = this.data.get(eventId);
+        this.data.delete(eventId);
+        return danglingReceipts;
+    }
+}
+
+function getOrCreate<K, V>(m: Map<K, V>, key: K, createFn: () => V): V {
+    const found = m.get(key);
+    if (found) {
+        return found;
+    } else {
+        const created = createFn();
+        m.set(key, created);
+        return created;
+    }
+}
+
+/**
+ * Is left after right (or the same)?
+ *
+ * Only returns true if both events can be found, and left is after or the same
+ * as right.
+ *
+ * @returns left \>= right
+ */
+function isAfterOrSame(leftEventId: string, rightEventId: string, room: Room): boolean {
+    const comparison = room.compareEventOrdering(leftEventId, rightEventId);
+    return comparison !== null && comparison >= 0;
+}
+
+/**
+ * Is left strictly after right?
+ *
+ * Only returns true if both events can be found, and left is strictly after right.
+ *
+ * @returns left \> right
+ */
+function isAfter(leftEventId: string, rightEventId: string, room: Room): boolean {
+    const comparison = room.compareEventOrdering(leftEventId, rightEventId);
+    return comparison !== null && comparison > 0;
+}

src/models/room.ts

@@ -66,6 +66,8 @@ import { IStateEventWithRoomId } from "../@types/search";
 import { RelationsContainer } from "./relations-container";
 import { ReadReceipt, synthesizeReceipt } from "./read-receipt";
 import { isPollEvent, Poll, PollEvent } from "./poll";
+import { RoomReceipts } from "./room-receipts";
+import { compareEventOrdering } from "./compare-event-ordering";
 
 // These constants are used as sane defaults when the homeserver doesn't support
 // the m.room_versions capability. In practice, KNOWN_SAFE_ROOM_VERSION should be
@@ -432,6 +434,12 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
      */
     private visibilityEvents = new Map<string, MatrixEvent[]>();
 
+    /**
+     * The latest receipts (synthetic and real) for each user in each thread
+     * (and unthreaded).
+     */
+    private roomReceipts = new RoomReceipts(this);
+
     /**
      * Construct a new Room.
      *
@@ -2935,6 +2943,10 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
      */
     public addReceipt(event: MatrixEvent, synthetic = false): void {
         const content = event.getContent<ReceiptContent>();
+
+        this.roomReceipts.add(content, synthetic);
+
+        // TODO: delete the following code when it has been replaced by RoomReceipts
         Object.keys(content).forEach((eventId: string) => {
             Object.keys(content[eventId]).forEach((receiptType: ReceiptType | string) => {
                 Object.keys(content[eventId][receiptType]).forEach((userId: string) => {
@@ -2996,6 +3008,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
                 });
             });
         });
+        // End of code to delete when replaced by RoomReceipts
 
         // send events after we've regenerated the structure & cache, otherwise things that
         // listened for the event would read stale data.
@@ -3582,6 +3595,19 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
         return this.oldestThreadedReceiptTs;
     }
 
+    /**
+     * Determines if the given user has read a particular event ID with the known
+     * history of the room. This is not a definitive check as it relies only on
+     * what is available to the room at the time of execution.
+     *
+     * @param userId - The user ID to check the read state of.
+     * @param eventId - The event ID to check if the user read.
+     * @returns true if the user has read the event, false otherwise.
+     */
+    public hasUserReadEvent(userId: string, eventId: string): boolean {
+        return this.roomReceipts.hasUserReadEvent(userId, eventId);
+    }
+
     /**
      * Returns the most recent unthreaded receipt for a given user
      * @param userId - the MxID of the User
@@ -3615,6 +3641,30 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
             thread.fixupNotifications(userId);
         }
     }
+
+    /**
+     * Determine the order of two events in this room.
+     *
+     * In principle this should use the same order as the server, but in practice
+     * this is difficult for events that were not received over the Sync API. See
+     * MSC4033 for details.
+     *
+     * This implementation leans on the order of events within their timelines, and
+     * falls back to comparing event timestamps when they are in different
+     * timelines.
+     *
+     * See https://github.com/matrix-org/matrix-js-sdk/issues/3325 for where we are
+     * tracking the work to fix this.
+     *
+     * @param leftEventId - the id of the first event
+     * @param rightEventId - the id of the second event
+
+     * @returns -1 if left \< right, 1 if left \> right, 0 if left == right, null if
+     *          we can't tell (because we can't find the events).
+     */
+    public compareEventOrdering(leftEventId: string, rightEventId: string): number | null {
+        return compareEventOrdering(this, leftEventId, rightEventId);
+    }
 }
 
 // a map from current event status to a list of allowed next statuses

src/models/thread.ts

@@ -748,6 +748,27 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM
      * @returns ID of the latest event that the given user has read, or null.
      */
     public getEventReadUpTo(userId: string, ignoreSynthesized?: boolean): string | null {
+        // TODO: we think the implementation here is not right. Here is a sketch
+        // of the right answer:
+        //
+        // for event in timeline.events.reversed():
+        //     if room.hasUserReadEvent(event):
+        //         return event
+        // return null
+        //
+        // If this is too slow, we might be able to improve it by trying walking
+        // forward from the threaded receipt in this thread. We could alternate
+        // between backwards-from-front and forwards-from-threaded-receipt to
+        // improve our chances of hitting the right answer sooner.
+        //
+        // Either way, it's still fundamentally slow because we have to walk
+        // events.
+        //
+        // We also might just want to limit the time we spend on this by giving
+        // up after, say, 100 events.
+        //
+        // --- andyb
+
         const isCurrentUser = userId === this.client.getUserId();
         const lastReply = this.timeline[this.timeline.length - 1];
         if (isCurrentUser && lastReply) {
@@ -816,7 +837,7 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM
             }
         }
 
-        return super.hasUserReadEvent(userId, eventId);
+        return this.room.hasUserReadEvent(userId, eventId);
     }
 
     public setUnread(type: NotificationCountType, count: number): void {