Clarify code that chooses a thread ID to include in a receipt (#3797)

* Extract threadIdForReceipt function from sendReceipt

* Tests for threadIdForReceipt

* Correct test of threadIdForReceipt to expect main for redaction of threaded

* Expand and comment implementation of threadIdForReceipt

spec/unit/read-receipt.spec.ts

@@ -18,7 +18,7 @@ import MockHttpBackend from "matrix-mock-request";
 
 import { MAIN_ROOM_TIMELINE, ReceiptType, WrappedReceipt } from "../../src/@types/read_receipts";
 import { MatrixClient } from "../../src/client";
-import { EventType, MatrixEvent, Room } from "../../src/matrix";
+import { EventType, MatrixEvent, RelationType, Room, threadIdForReceipt } from "../../src/matrix";
 import { synthesizeReceipt } from "../../src/models/read-receipt";
 import { encodeUri } from "../../src/utils";
 import * as utils from "../test-utils/test-utils";
@@ -258,4 +258,200 @@ describe("Read receipt", () => {
             expect(room.getEventReadUpTo(userId)).toBe(mainTimelineReceipt.eventId);
         });
     });
+
+    describe("Determining the right thread ID for a receipt", () => {
+        it("provides the thread root ID for a normal threaded message", () => {
+            const event = utils.mkEvent({
+                event: true,
+                type: EventType.RoomMessage,
+                user: "@bob:matrix.org",
+                room: "!roomx",
+                content: {
+                    "body": "Hello from a thread",
+                    "m.relates_to": {
+                        "event_id": "$thread1",
+                        "m.in_reply_to": {
+                            event_id: "$thread1",
+                        },
+                        "rel_type": "m.thread",
+                    },
+                },
+            });
+
+            expect(threadIdForReceipt(event)).toEqual("$thread1");
+        });
+
+        it("provides 'main' for a non-thread message", () => {
+            const event = utils.mkEvent({
+                event: true,
+                type: EventType.RoomMessage,
+                user: "@bob:matrix.org",
+                room: "!roomx",
+                content: { body: "Hello" },
+            });
+
+            expect(threadIdForReceipt(event)).toEqual("main");
+        });
+
+        it("provides 'main' for a thread root", () => {
+            const event = utils.mkEvent({
+                event: true,
+                type: EventType.RoomMessage,
+                user: "@bob:matrix.org",
+                room: "!roomx",
+                content: { body: "Hello" },
+            });
+            // Set thread ID to this event's ID, meaning this is the thread root
+            event.setThreadId(event.getId());
+
+            expect(threadIdForReceipt(event)).toEqual("main");
+        });
+
+        it("provides 'main' for a reaction to a thread root", () => {
+            const event = utils.mkEvent({
+                event: true,
+                type: EventType.Reaction,
+                user: "@bob:matrix.org",
+                room: "!roomx",
+                content: {
+                    "m.relates_to": {
+                        rel_type: RelationType.Annotation,
+                        event_id: "$thread1",
+                        key: Math.random().toString(),
+                    },
+                },
+            });
+
+            // Set thread Id, meaning this looks like it's in the thread (this
+            // happens for relations like this, so that they appear in the
+            // thread's timeline).
+            event.setThreadId("$thread1");
+
+            // But because it's a reaction to the thread root, it's in main
+            expect(threadIdForReceipt(event)).toEqual("main");
+        });
+
+        it("provides the thread ID for a reaction to a threaded message", () => {
+            const event = utils.mkEvent({
+                event: true,
+                type: EventType.Reaction,
+                user: "@bob:matrix.org",
+                room: "!roomx",
+                content: {
+                    "m.relates_to": {
+                        rel_type: RelationType.Annotation,
+                        event_id: "$withinthread2",
+                        key: Math.random().toString(),
+                    },
+                },
+            });
+
+            // Set thread Id, to say this message is in the thread. This happens
+            // when the message arrived and is classified.
+            event.setThreadId("$thread1");
+
+            // It's in the thread because it refers to something else, not the
+            // thread root
+            expect(threadIdForReceipt(event)).toEqual("$thread1");
+        });
+
+        it("(suprisingly?) provides 'main' for a redaction of a threaded message", () => {
+            const event = utils.mkEvent({
+                event: true,
+                type: EventType.RoomRedaction,
+                content: {
+                    reason: "Spamming",
+                },
+                redacts: "$withinthread2",
+                room: "!roomx",
+                user: "@bob:matrix.org",
+            });
+
+            // Set thread Id, to say this message is in the thread.
+            event.setThreadId("$thread1");
+
+            // Because redacting a message removes all its m.relations, the
+            // message is no longer in the thread, so we must send a receipt for
+            // it in the main timeline.
+            //
+            // This is surprising, but it follows the spec (at least up to
+            // current latest room version, 11). In fact, the event should no
+            // longer have a thread ID set on it, so this testcase should not
+            // come up. (At time of writing, this is not the case though - it
+            // does still have threadId set.)
+            expect(threadIdForReceipt(event)).toEqual("main");
+        });
+
+        it("provides the thread ID for an edit of a threaded message", () => {
+            const event = utils.mkEvent({
+                event: true,
+                type: EventType.RoomRedaction,
+                content: {
+                    "body": "Edited!",
+                    "m.new_content": {
+                        body: "Edited!",
+                    },
+                    "m.relates_to": {
+                        rel_type: RelationType.Replace,
+                        event_id: "$withinthread2",
+                    },
+                },
+                room: "!roomx",
+                user: "@bob:matrix.org",
+            });
+
+            // Set thread Id, to say this message is in the thread.
+            event.setThreadId("$thread1");
+
+            // It's in the thread, because it redacts something inside the
+            // thread (not the thread root)
+            expect(threadIdForReceipt(event)).toEqual("$thread1");
+        });
+
+        it("provides 'main' for an edit of a thread root", () => {
+            const event = utils.mkEvent({
+                event: true,
+                type: EventType.RoomRedaction,
+                content: {
+                    "body": "Edited!",
+                    "m.new_content": {
+                        body: "Edited!",
+                    },
+                    "m.relates_to": {
+                        rel_type: RelationType.Replace,
+                        event_id: "$thread1",
+                    },
+                },
+                room: "!roomx",
+                user: "@bob:matrix.org",
+            });
+
+            // Set thread Id, to say this message is in the thread.
+            event.setThreadId("$thread1");
+
+            // It's in the thread, because it redacts something inside the
+            // thread (not the thread root)
+            expect(threadIdForReceipt(event)).toEqual("main");
+        });
+
+        it("provides 'main' for a redaction of the thread root", () => {
+            const event = utils.mkEvent({
+                event: true,
+                type: EventType.RoomRedaction,
+                content: {
+                    reason: "Spamming",
+                },
+                redacts: "$thread1",
+                room: "!roomx",
+                user: "@bob:matrix.org",
+            });
+
+            // Set thread Id, to say this message is in the thread.
+            event.setThreadId("$thread1");
+
+            // It's in the thread, because it redacts something inside the
+            // thread (not the thread root)
+            expect(threadIdForReceipt(event)).toEqual("main");
+        });
+    });
 });

src/client.ts

@@ -5157,24 +5157,12 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
             $eventId: event.getId()!,
         });
 
-        if (!unthreaded && this.supportsThreads()) {
+        // Unless we're explicitly making an unthreaded receipt or we don't
-            // XXX: the spec currently says a threaded read receipt can be sent for the root of a thread,
+        // support threads, include the `thread_id` property in the body.
-            // but in practice this isn't possible and the spec needs updating.
+        const shouldAddThreadId = !unthreaded && this.supportsThreads();
-            const isThread =
+        const fullBody = shouldAddThreadId ? { ...body, thread_id: threadIdForReceipt(event) } : body;
-                !!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(THREAD_RELATION_TYPE.name) || event.relationEventId !== event.threadRootId);
-            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,
-            };
-        }
 
-        const promise = this.http.authedRequest<{}>(Method.Post, path, undefined, body || {});
+        const promise = this.http.authedRequest<{}>(Method.Post, path, undefined, fullBody || {});
 
         const room = this.getRoom(event.getRoomId());
         if (room && this.credentials.userId) {
@@ -9925,3 +9913,66 @@ export function fixNotificationCountOnDecryption(cli: MatrixClient, event: Matri
         }
     }
 }
+
+/**
+ * Given an event, figure out the thread ID we should use for it in a receipt.
+ *
+ * This will either be "main", or event.threadRootId. For the thread root, or
+ * e.g. reactions to the thread root, this will be main. For events inside the
+ * thread, or e.g. reactions to them, this will be event.threadRootId.
+ *
+ * (Exported for test.)
+ */
+export function threadIdForReceipt(event: MatrixEvent): string {
+    return inMainTimelineForReceipt(event) ? MAIN_ROOM_TIMELINE : event.threadRootId!;
+}
+
+/**
+ * a) True for non-threaded messages, thread roots and non-thread relations to thread roots.
+ * b) False for messages with thread relations to the thread root.
+ * c) False for messages with any kind of relation to a message from case b.
+ *
+ * Note: true for redactions of messages that are in threads. Redacted messages
+ * are not really in threads (because their relations are gone), so if they look
+ * like they are in threads, that is a sign of a bug elsewhere. (At time of
+ * writing, this bug definitely exists - messages are not moved to another
+ * thread when they are redacted.)
+ *
+ * @returns true if this event is considered to be in the main timeline as far
+ *               as receipts are concerned.
+ */
+function inMainTimelineForReceipt(event: MatrixEvent): boolean {
+    if (!event.threadRootId) {
+        // Not in a thread: then it is in the main timeline
+        return true;
+    }
+
+    if (event.isThreadRoot) {
+        // Thread roots are in the main timeline. Note: the spec is ambiguous (or
+        // wrong) on this - see
+        // https://github.com/matrix-org/matrix-spec-proposals/pull/4037
+        return true;
+    }
+
+    if (!event.isRelation()) {
+        // If it's not related to anything, it can't be related via a chain of
+        // relations to a thread root.
+        //
+        // Note: this is a bug, because how does it have a threadRootId if it is
+        // neither a thread root, nor related to one?
+        logger.warn(`Event is not a relation or a thread root, but still has a threadRootId! id=${event.getId()}`);
+        return true;
+    }
+
+    if (event.isRelation(THREAD_RELATION_TYPE.name)) {
+        // It's a message in a thread - definitely not in the main timeline.
+        return false;
+    }
+
+    const isRelatedToRoot = event.relationEventId === event.threadRootId;
+
+    // If it's related to the thread root (and we already know it's not a thread
+    // relation) then it's in the main timeline. If it's related to something
+    // else, then it's in the thread (because it has a thread ID).
+    return isRelatedToRoot;
+}