Fix edge cases around non-thread relations to thread roots and read receipts

spec/unit/read-receipt.spec.ts

@@ -73,6 +73,7 @@ describe("Read receipt", () => {
     beforeEach(() => {
         httpBackend = new MockHttpBackend();
         client = new MatrixClient({
+            userId: "@user:server",
             baseUrl: "https://my.home.server",
             accessToken: "my.access.token",
             fetchFn: httpBackend.fetchFn as typeof global.fetch,
@@ -81,7 +82,7 @@ describe("Read receipt", () => {
     });
 
     describe("sendReceipt", () => {
-        it("sends a thread read receipt", async () => {
+        it("sends a thread read receipt (deprecated)", async () => {
             httpBackend
                 .when(
                     "POST",
@@ -102,7 +103,28 @@ describe("Read receipt", () => {
             await flushPromises();
         });
 
-        it("sends an unthreaded receipt", async () => {
+        it("sends a thread read receipt", async () => {
+            httpBackend
+                .when(
+                    "POST",
+                    encodeUri("/rooms/$roomId/receipt/$receiptType/$eventId", {
+                        $roomId: ROOM_ID,
+                        $receiptType: ReceiptType.Read,
+                        $eventId: threadEvent.getId()!,
+                    }),
+                )
+                .check((request) => {
+                    expect(request.data.thread_id).toEqual(THREAD_ID);
+                })
+                .respond(200, {});
+
+            client.sendReceipt(threadEvent, ReceiptType.Read, THREAD_ID);
+
+            await httpBackend.flushAllExpected();
+            await flushPromises();
+        });
+
+        it("sends an unthreaded receipt (deprecated)", async () => {
             httpBackend
                 .when(
                     "POST",
@@ -123,7 +145,91 @@ describe("Read receipt", () => {
             await flushPromises();
         });
 
-        it("sends a room read receipt", async () => {
+        it("sends an unthreaded receipt", async () => {
+            httpBackend
+                .when(
+                    "POST",
+                    encodeUri("/rooms/$roomId/receipt/$receiptType/$eventId", {
+                        $roomId: ROOM_ID,
+                        $receiptType: ReceiptType.Read,
+                        $eventId: threadEvent.getId()!,
+                    }),
+                )
+                .check((request) => {
+                    expect(request.data.thread_id).toBeUndefined();
+                })
+                .respond(200, {});
+
+            client.sendReadReceipt(threadEvent, ReceiptType.Read, null);
+
+            await httpBackend.flushAllExpected();
+            await flushPromises();
+        });
+
+        it("sends an main timeline receipt for a thread root (deprecated)", async () => {
+            const threadRootEvent = utils.mkEvent({
+                event: true,
+                type: EventType.RoomMessage,
+                user: "@bob:matrix.org",
+                room: ROOM_ID,
+                content: {
+                    body: "Hello, this is a thread root",
+                },
+            });
+            threadRootEvent.setThreadId(threadRootEvent.getId());
+
+            httpBackend
+                .when(
+                    "POST",
+                    encodeUri("/rooms/$roomId/receipt/$receiptType/$eventId", {
+                        $roomId: ROOM_ID,
+                        $receiptType: ReceiptType.Read,
+                        $eventId: threadRootEvent.getId()!,
+                    }),
+                )
+                .check((request) => {
+                    expect(request.data.thread_id).toBe("main");
+                })
+                .respond(200, {});
+            client.sendReadReceipt(threadRootEvent, ReceiptType.Read, false);
+
+            await httpBackend.flushAllExpected();
+            await flushPromises();
+        });
+
+        it("sends an main timeline receipt for a reaction to a thread root (deprecated)", async () => {
+            const threadRootEvent = utils.mkEvent({
+                event: true,
+                type: EventType.RoomMessage,
+                user: "@bob:matrix.org",
+                room: ROOM_ID,
+                content: {
+                    body: "Hello, this is a thread root",
+                },
+            });
+            const reactionToThreadRoot = utils.mkReaction(threadRootEvent, client, client.getSafeUserId(), ROOM_ID);
+            reactionToThreadRoot.setThreadId(threadRootEvent.getId());
+
+            httpBackend
+                .when(
+                    "POST",
+                    encodeUri("/rooms/$roomId/receipt/$receiptType/$eventId", {
+                        $roomId: ROOM_ID,
+                        $receiptType: ReceiptType.Read,
+                        $eventId: reactionToThreadRoot.getId()!,
+                    }),
+                )
+                .check((request) => {
+                    expect(request.data.thread_id).toBe("main");
+                })
+                .respond(200, {});
+            client.sendReadReceipt(reactionToThreadRoot, ReceiptType.Read, false);
+
+            await httpBackend.flushAllExpected();
+            await flushPromises();
+        });
+
+        it("sends a room read receipt (deprecated)", async () => {
             httpBackend
                 .when(
                     "POST",
@@ -143,6 +249,27 @@ describe("Read receipt", () => {
             await httpBackend.flushAllExpected();
             await flushPromises();
         });
+
+        it("sends a main timeline room read receipt", async () => {
+            httpBackend
+                .when(
+                    "POST",
+                    encodeUri("/rooms/$roomId/receipt/$receiptType/$eventId", {
+                        $roomId: ROOM_ID,
+                        $receiptType: ReceiptType.Read,
+                        $eventId: roomEvent.getId()!,
+                    }),
+                )
+                .check((request) => {
+                    expect(request.data.thread_id).toEqual(MAIN_ROOM_TIMELINE);
+                })
+                .respond(200, {});
+
+            client.sendReceipt(roomEvent, ReceiptType.Read, "main");
+
+            await httpBackend.flushAllExpected();
+            await flushPromises();
+        });
     });
 
     describe("synthesizeReceipt", () => {

spec/unit/room.spec.ts

@@ -2945,6 +2945,32 @@ describe("Room", function () {
             expect(room.eventShouldLiveIn(reply2, events, roots).shouldLiveInThread).toBeFalsy();
         });
 
+        it("edit to thread root should live in both", () => {
+            const threadRoot = mkMessage();
+            const threadResponse1 = mkThreadResponse(threadRoot);
+            const threadRootEdit = mkEdit(threadRoot);
+            threadRoot.makeReplaced(threadRootEdit);
+
+            const thread = room.createThread(threadRoot.getId()!, threadRoot, [threadResponse1], false);
+            threadResponse1.setThread(thread);
+            threadRootEdit.setThread(thread);
+
+            const roots = new Set([threadRoot.getId()!]);
+            const events = [threadRoot, threadResponse1, threadRootEdit];
+
+            expect(room.eventShouldLiveIn(threadRoot, events, roots).shouldLiveInRoom).toBeTruthy();
+            expect(room.eventShouldLiveIn(threadRoot, events, roots).shouldLiveInThread).toBeTruthy();
+            expect(room.eventShouldLiveIn(threadRoot, events, roots).threadId).toBe(threadRoot.getId());
+
+            expect(room.eventShouldLiveIn(threadResponse1, events, roots).shouldLiveInRoom).toBeFalsy();
+            expect(room.eventShouldLiveIn(threadResponse1, events, roots).shouldLiveInThread).toBeTruthy();
+            expect(room.eventShouldLiveIn(threadResponse1, events, roots).threadId).toBe(threadRoot.getId());
+
+            expect(room.eventShouldLiveIn(threadRootEdit, events, roots).shouldLiveInRoom).toBeTruthy();
+            expect(room.eventShouldLiveIn(threadRootEdit, events, roots).shouldLiveInThread).toBeTruthy();
+            expect(room.eventShouldLiveIn(threadRootEdit, events, roots).threadId).toBe(threadRoot.getId());
+        });
+
         it("should aggregate relations in thread event timeline set", async () => {
             Thread.setServerSideSupport(FeatureSupport.Stable);
             const threadRoot = mkMessage();

src/client.ts

@@ -4973,43 +4973,84 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         return this.sendMessage(roomId, threadId, content);
     }
 
+    // Deprecated signature of MatrixClient::sendReceipt allows only specifying unthreaded (boolean) which means it is
+    // up to the SDK to guess which thread to send the receipt to. This isn't always possible, as for example a relation
+    // to the thread root shows up in both timelines and can be read separately in each.
+    // This method guesses which thread to send to, with known issues around thread root relations only sending to
+    // main timeline.
+    private guessThreadForReceipt(event: MatrixEvent): string {
+        // XXX: the spec currently says a threaded read receipt can be sent for the root of a thread,
+        // but in practice this isn't possible and the spec needs updating.
+        const isThread =
+            !!event.threadRootId &&
+            // A thread cannot be just a thread root and a thread root can only be read in the main timeline
+            !event.isThreadRoot &&
+            // Similarly non-thread relations upon the thread root (reactions, edits) should also be for the main timeline.
+            event.isRelation() &&
+            (event.isRelation(RelationType.Thread) || event.relationEventId !== event.threadRootId);
+        return isThread ? event.threadRootId : MAIN_ROOM_TIMELINE;
+    }
+
     /**
      * Send a receipt.
      * @param event - The event being acknowledged
      * @param receiptType - The kind of receipt e.g. "m.read". Other than
      * ReceiptType.Read are experimental!
      * @param body - Additional content to send alongside the receipt.
-     * @param unthreaded - An unthreaded receipt will clear room+thread notifications
+     * @param thread - the ID of the thread to which the read receipt corresponds,
+     *     "main" for main timeline, `null` for unthreaded (all timelines).
      * @returns Promise which resolves: to an empty object `{}`
      * @returns Rejects: with an error response.
      */
     public async sendReceipt(
         event: MatrixEvent,
         receiptType: ReceiptType,
+        thread: "main" | string | null,
         body?: Record<string, any>,
-        unthreaded = false,
+    ): Promise<{}>;
+    /**
+     * @deprecated backwards compatibility signature
+     */
+    public async sendReceipt(
+        event: MatrixEvent,
+        receiptType: ReceiptType,
+        body?: Record<string, any>,
+        unthreaded?: boolean,
+    ): Promise<{}>;
+    public async sendReceipt(
+        event: MatrixEvent,
+        receiptType: ReceiptType,
+        bodyOrThread?: Record<string, any> | string | null,
+        unthreadedOrBody?: boolean | Record<string, any>,
     ): Promise<{}> {
         if (this.isGuest()) {
             return Promise.resolve({}); // guests cannot send receipts so don't bother.
         }
 
+        let thread: string | null;
+        let body: Record<string, any>;
+
+        if ((bodyOrThread !== null && typeof bodyOrThread === "object") || typeof unthreadedOrBody === "boolean") {
+            // Backwards compatible signature case
+            thread = !unthreadedOrBody ? this.guessThreadForReceipt(event) : null;
+            body = (bodyOrThread as Record<string, any>) ?? {};
+        } else {
+            thread = bodyOrThread as string | null;
+            body = unthreadedOrBody ?? {};
+        }
+
         const path = utils.encodeUri("/rooms/$roomId/receipt/$receiptType/$eventId", {
             $roomId: event.getRoomId()!,
             $receiptType: receiptType,
             $eventId: event.getId()!,
         });
 
-        if (!unthreaded) {
-            // A thread cannot be just a thread root and a thread root can only be read in the main timeline
-            const isThread = !!event.threadRootId && !event.isThreadRoot;
-            body = {
-                ...body,
-                // Only thread replies should define a specific thread. Thread roots can only be read in the main timeline.
-                thread_id: isThread ? event.threadRootId : MAIN_ROOM_TIMELINE,
-            };
+        if (thread !== null) {
+            // Only thread replies should define a specific thread. Thread roots can only be read in the main timeline.
+            body.thread_id = thread;
         }
 
-        const promise = this.http.authedRequest<{}>(Method.Post, path, undefined, body || {});
+        const promise = this.http.authedRequest<{}>(Method.Post, path, undefined, body);
 
         const room = this.getRoom(event.getRoomId());
         if (room && this.credentials.userId) {
@@ -5022,13 +5063,28 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * Send a read receipt.
      * @param event - The event that has been read.
      * @param receiptType - other than ReceiptType.Read are experimental! Optional.
+     * @param thread - the ID of the thread to which the read receipt corresponds,
+     *     "main" for main timeline, `null` for unthreaded (all timelines).
      * @returns Promise which resolves: to an empty object `{}`
      * @returns Rejects: with an error response.
      */
+    public async sendReadReceipt(
+        event: MatrixEvent,
+        receiptType: ReceiptType,
+        thread: "main" | string | null,
+    ): Promise<{}>;
+    /**
+     * @deprecated backwards compatibility signature
+     */
+    public async sendReadReceipt(
+        event: MatrixEvent | null,
+        receiptType?: ReceiptType,
+        unthreaded?: boolean,
+    ): Promise<{} | undefined>;
     public async sendReadReceipt(
         event: MatrixEvent | null,
         receiptType = ReceiptType.Read,
-        unthreaded = false,
+        threadOrUnthreaded: boolean | string | null = false,
     ): Promise<{} | undefined> {
         if (!event) return;
         const eventId = event.getId()!;
@@ -5037,7 +5093,11 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
             throw new Error(`Cannot set read receipt to a pending event (${eventId})`);
         }
 
-        return this.sendReceipt(event, receiptType, {}, unthreaded);
+        if (typeof threadOrUnthreaded === "boolean") {
+            // Backwards compatible case
+            return this.sendReceipt(event, receiptType, {}, threadOrUnthreaded);
+        }
+        return this.sendReceipt(event, receiptType, threadOrUnthreaded);
     }
 
     /**