Fix edge cases around non-thread relations to thread roots and read receipts (#3607)

* Ensure non-thread relations to a thread root are actually in both timelines

* Make thread in sendReceipt & sendReadReceipt explicit rather than guessing it

* Apply suggestions from code review

* Fix Room::eventShouldLiveIn to better match Synapse to diverging ideas of notifications

* Update read receipt sending behaviour to align with Synapse

* Fix tests

* Fix thread rel type

spec/integ/matrix-client-event-timeline.spec.ts

@@ -1284,7 +1284,6 @@ describe("MatrixClient event timelines", function () {
             THREAD_ROOT.event_id,
             THREAD_REPLY.event_id,
             THREAD_REPLY2.getId(),
-            THREAD_ROOT_REACTION.getId(),
             THREAD_REPLY3.getId(),
         ]);
     });

spec/integ/matrix-client-methods.spec.ts

@@ -656,7 +656,7 @@ describe("MatrixClient", function () {
             expect(threaded).toEqual([]);
         });
 
-        it("copies pre-thread in-timeline vote events onto both timelines", function () {
+        it("should not copy pre-thread in-timeline vote events onto both timelines", function () {
             // @ts-ignore setting private property
             client.clientOpts = {
                 ...defaultClientOpts,
@@ -684,10 +684,10 @@ describe("MatrixClient", function () {
             const eventRefWithThreadId = withThreadId(eventPollResponseReference, eventPollStartThreadRoot.getId()!);
             expect(eventRefWithThreadId.threadRootId).toBeTruthy();
 
-            expect(threaded).toEqual([eventPollStartThreadRoot, eventMessageInThread, eventRefWithThreadId]);
+            expect(threaded).toEqual([eventPollStartThreadRoot, eventMessageInThread]);
         });
 
-        it("copies pre-thread in-timeline reactions onto both timelines", function () {
+        it("should not copy pre-thread in-timeline reactions onto both timelines", function () {
             // @ts-ignore setting private property
             client.clientOpts = {
                 ...defaultClientOpts,
@@ -704,14 +704,10 @@ describe("MatrixClient", function () {
 
             expect(timeline).toEqual([eventPollStartThreadRoot, eventReaction]);
 
-            expect(threaded).toEqual([
-                eventPollStartThreadRoot,
-                eventMessageInThread,
-                withThreadId(eventReaction, eventPollStartThreadRoot.getId()!),
-            ]);
+            expect(threaded).toEqual([eventPollStartThreadRoot, eventMessageInThread]);
         });
 
-        it("copies post-thread in-timeline vote events onto both timelines", function () {
+        it("should not copy post-thread in-timeline vote events onto both timelines", function () {
             // @ts-ignore setting private property
             client.clientOpts = {
                 ...defaultClientOpts,
@@ -728,14 +724,10 @@ describe("MatrixClient", function () {
 
             expect(timeline).toEqual([eventPollStartThreadRoot, eventPollResponseReference]);
 
-            expect(threaded).toEqual([
-                eventPollStartThreadRoot,
-                withThreadId(eventPollResponseReference, eventPollStartThreadRoot.getId()!),
-                eventMessageInThread,
-            ]);
+            expect(threaded).toEqual([eventPollStartThreadRoot, eventMessageInThread]);
         });
 
-        it("copies post-thread in-timeline reactions onto both timelines", function () {
+        it("should not copy post-thread in-timeline reactions onto both timelines", function () {
             // @ts-ignore setting private property
             client.clientOpts = {
                 ...defaultClientOpts,
@@ -752,11 +744,7 @@ describe("MatrixClient", function () {
 
             expect(timeline).toEqual([eventPollStartThreadRoot, eventReaction]);
 
-            expect(threaded).toEqual([
-                eventPollStartThreadRoot,
-                eventMessageInThread,
-                withThreadId(eventReaction, eventPollStartThreadRoot.getId()!),
-            ]);
+            expect(threaded).toEqual([eventPollStartThreadRoot, eventMessageInThread]);
         });
 
         it("sends room state events to the main timeline only", function () {
@@ -809,11 +797,7 @@ describe("MatrixClient", function () {
             ]);
 
             // Thread should contain only stuff that happened in the thread - no room state events
-            expect(threaded).toEqual([
-                eventPollStartThreadRoot,
-                withThreadId(eventPollResponseReference, eventPollStartThreadRoot.getId()!),
-                eventMessageInThread,
-            ]);
+            expect(threaded).toEqual([eventPollStartThreadRoot, eventMessageInThread]);
         });
 
         it("sends redactions of reactions to thread responses to thread timeline only", () => {
@@ -878,9 +862,9 @@ describe("MatrixClient", function () {
 
             const [timeline, threaded] = room.partitionThreadedEvents(events);
 
-            expect(timeline).toEqual([threadRootEvent, replyToThreadResponse]);
+            expect(timeline).toEqual([threadRootEvent]);
 
-            expect(threaded).toEqual([threadRootEvent, eventMessageInThread]);
+            expect(threaded).toEqual([threadRootEvent, eventMessageInThread, replyToThreadResponse]);
         });
     });
 

spec/test-utils/thread.ts

@@ -18,7 +18,7 @@ import { RelationType } from "../../src/@types/event";
 import { MatrixClient } from "../../src/client";
 import { MatrixEvent, MatrixEventEvent } from "../../src/models/event";
 import { Room } from "../../src/models/room";
-import { Thread } from "../../src/models/thread";
+import { Thread, THREAD_RELATION_TYPE } from "../../src/models/thread";
 import { mkMessage } from "./test-utils";
 
 export const makeThreadEvent = ({
@@ -34,7 +34,7 @@ export const makeThreadEvent = ({
         ...props,
         relatesTo: {
             event_id: rootEventId,
-            rel_type: "m.thread",
+            rel_type: THREAD_RELATION_TYPE.name,
             ["m.in_reply_to"]: {
                 event_id: replyToEventId,
             },

spec/unit/read-receipt.spec.ts

@@ -18,7 +18,7 @@ import MockHttpBackend from "matrix-mock-request";
 
 import { MAIN_ROOM_TIMELINE, ReceiptType } from "../../src/@types/read_receipts";
 import { MatrixClient } from "../../src/client";
-import { EventType } from "../../src/matrix";
+import { EventType, MatrixEvent, Room } from "../../src/matrix";
 import { synthesizeReceipt } from "../../src/models/read-receipt";
 import { encodeUri } from "../../src/utils";
 import * as utils from "../test-utils/test-utils";
@@ -42,7 +42,21 @@ let httpBackend: MockHttpBackend;
 const THREAD_ID = "$thread_event_id";
 const ROOM_ID = "!123:matrix.org";
 
-const threadEvent = utils.mkEvent({
+describe("Read receipt", () => {
+    let threadEvent: MatrixEvent;
+    let roomEvent: MatrixEvent;
+
+    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,
+        });
+        client.isGuest = () => false;
+
+        threadEvent = utils.mkEvent({
             event: true,
             type: EventType.RoomMessage,
             user: "@bob:matrix.org",
@@ -57,9 +71,8 @@ const threadEvent = utils.mkEvent({
                     "rel_type": "m.thread",
                 },
             },
-});
-
-const roomEvent = utils.mkEvent({
+        });
+        roomEvent = utils.mkEvent({
             event: true,
             type: EventType.RoomMessage,
             user: "@bob:matrix.org",
@@ -67,17 +80,7 @@ const roomEvent = utils.mkEvent({
             content: {
                 body: "Hello from a room",
             },
-});
-
-describe("Read receipt", () => {
-    beforeEach(() => {
-        httpBackend = new MockHttpBackend();
-        client = new MatrixClient({
-            baseUrl: "https://my.home.server",
-            accessToken: "my.access.token",
-            fetchFn: httpBackend.fetchFn as typeof global.fetch,
         });
-        client.isGuest = () => false;
     });
 
     describe("sendReceipt", () => {
@@ -143,13 +146,46 @@ describe("Read receipt", () => {
             await httpBackend.flushAllExpected();
             await flushPromises();
         });
+
+        it("should send a main timeline read receipt for a reaction to a thread root", async () => {
+            roomEvent.event.event_id = THREAD_ID;
+            const reaction = utils.mkReaction(roomEvent, client, client.getSafeUserId(), ROOM_ID);
+            const thread = new Room(ROOM_ID, client, client.getSafeUserId()).createThread(
+                THREAD_ID,
+                roomEvent,
+                [threadEvent],
+                false,
+            );
+            threadEvent.setThread(thread);
+            reaction.setThread(thread);
+
+            httpBackend
+                .when(
+                    "POST",
+                    encodeUri("/rooms/$roomId/receipt/$receiptType/$eventId", {
+                        $roomId: ROOM_ID,
+                        $receiptType: ReceiptType.Read,
+                        $eventId: reaction.getId()!,
+                    }),
+                )
+                .check((request) => {
+                    expect(request.data.thread_id).toEqual(MAIN_ROOM_TIMELINE);
+                })
+                .respond(200, {});
+
+            client.sendReceipt(reaction, ReceiptType.Read, {});
+
+            await httpBackend.flushAllExpected();
+            await flushPromises();
+        });
     });
 
     describe("synthesizeReceipt", () => {
         it.each([
-            { event: roomEvent, destinationId: MAIN_ROOM_TIMELINE },
-            { event: threadEvent, destinationId: threadEvent.threadRootId! },
-        ])("adds the receipt to $destinationId", ({ event, destinationId }) => {
+            { getEvent: () => roomEvent, destinationId: MAIN_ROOM_TIMELINE },
+            { getEvent: () => threadEvent, destinationId: THREAD_ID },
+        ])("adds the receipt to $destinationId", ({ getEvent, destinationId }) => {
+            const event = getEvent();
             const userId = "@bob:example.org";
             const receiptType = ReceiptType.Read;
 

spec/unit/room.spec.ts

@@ -2849,7 +2849,7 @@ describe("Room", function () {
         Thread.setServerSideSupport(FeatureSupport.Stable);
         const room = new Room(roomId, client, userA);
 
-        it("thread root and its relations&redactions should be in both", () => {
+        it("thread root and its relations&redactions should be in main timeline", () => {
             const randomMessage = mkMessage();
             const threadRoot = mkMessage();
             const threadResponse1 = mkThreadResponse(threadRoot);
@@ -2867,6 +2867,9 @@ describe("Room", function () {
                 threadReaction2Redaction,
             ];
 
+            const thread = room.createThread(threadRoot.getId()!, threadRoot, [], false);
+            events.slice(1).forEach((ev) => ev.setThread(thread));
+
             expect(room.eventShouldLiveIn(randomMessage, events, roots).shouldLiveInRoom).toBeTruthy();
             expect(room.eventShouldLiveIn(randomMessage, events, roots).shouldLiveInThread).toBeFalsy();
 
@@ -2878,14 +2881,11 @@ describe("Room", function () {
             expect(room.eventShouldLiveIn(threadResponse1, events, roots).threadId).toBe(threadRoot.getId());
 
             expect(room.eventShouldLiveIn(threadReaction1, events, roots).shouldLiveInRoom).toBeTruthy();
-            expect(room.eventShouldLiveIn(threadReaction1, events, roots).shouldLiveInThread).toBeTruthy();
-            expect(room.eventShouldLiveIn(threadReaction1, events, roots).threadId).toBe(threadRoot.getId());
+            expect(room.eventShouldLiveIn(threadReaction1, events, roots).shouldLiveInThread).toBeFalsy();
             expect(room.eventShouldLiveIn(threadReaction2, events, roots).shouldLiveInRoom).toBeTruthy();
-            expect(room.eventShouldLiveIn(threadReaction2, events, roots).shouldLiveInThread).toBeTruthy();
-            expect(room.eventShouldLiveIn(threadReaction2, events, roots).threadId).toBe(threadRoot.getId());
+            expect(room.eventShouldLiveIn(threadReaction2, events, roots).shouldLiveInThread).toBeFalsy();
             expect(room.eventShouldLiveIn(threadReaction2Redaction, events, roots).shouldLiveInRoom).toBeTruthy();
-            expect(room.eventShouldLiveIn(threadReaction2Redaction, events, roots).shouldLiveInThread).toBeTruthy();
-            expect(room.eventShouldLiveIn(threadReaction2Redaction, events, roots).threadId).toBe(threadRoot.getId());
+            expect(room.eventShouldLiveIn(threadReaction2Redaction, events, roots).shouldLiveInThread).toBeFalsy();
         });
 
         it("thread response and its relations&redactions should be only in thread timeline", () => {
@@ -2909,25 +2909,39 @@ describe("Room", function () {
             expect(room.eventShouldLiveIn(threadReaction2Redaction, events, roots).threadId).toBe(threadRoot.getId());
         });
 
-        it("reply to thread response and its relations&redactions should be only in main timeline", () => {
+        it("reply to thread response and its relations&redactions should be only in thread timeline", () => {
             const threadRoot = mkMessage();
-            const threadResponse1 = mkThreadResponse(threadRoot);
-            const reply1 = mkReply(threadResponse1);
-            const reaction1 = utils.mkReaction(reply1, room.client, userA, roomId);
-            const reaction2 = utils.mkReaction(reply1, room.client, userA, roomId);
-            const reaction2Redaction = mkRedaction(reply1);
+            const threadResp1 = mkThreadResponse(threadRoot);
+            const threadResp1Reply1 = mkReply(threadResp1);
+            const threadResp1Reply1Reaction1 = utils.mkReaction(threadResp1Reply1, room.client, userA, roomId);
+            const threadResp1Reply1Reaction2 = utils.mkReaction(threadResp1Reply1, room.client, userA, roomId);
+            const thResp1Rep1React2Redaction = mkRedaction(threadResp1Reply1);
 
             const roots = new Set([threadRoot.getId()!]);
-            const events = [threadRoot, threadResponse1, reply1, reaction1, reaction2, reaction2Redaction];
+            const events = [
+                threadRoot,
+                threadResp1,
+                threadResp1Reply1,
+                threadResp1Reply1Reaction1,
+                threadResp1Reply1Reaction2,
+                thResp1Rep1React2Redaction,
+            ];
 
-            expect(room.eventShouldLiveIn(reply1, events, roots).shouldLiveInRoom).toBeTruthy();
-            expect(room.eventShouldLiveIn(reply1, events, roots).shouldLiveInThread).toBeFalsy();
-            expect(room.eventShouldLiveIn(reaction1, events, roots).shouldLiveInRoom).toBeTruthy();
-            expect(room.eventShouldLiveIn(reaction1, events, roots).shouldLiveInThread).toBeFalsy();
-            expect(room.eventShouldLiveIn(reaction2, events, roots).shouldLiveInRoom).toBeTruthy();
-            expect(room.eventShouldLiveIn(reaction2, events, roots).shouldLiveInThread).toBeFalsy();
-            expect(room.eventShouldLiveIn(reaction2Redaction, events, roots).shouldLiveInRoom).toBeTruthy();
-            expect(room.eventShouldLiveIn(reaction2Redaction, events, roots).shouldLiveInThread).toBeFalsy();
+            const thread = room.createThread(threadRoot.getId()!, threadRoot, [], false);
+            events.forEach((ev) => ev.setThread(thread));
+
+            expect(room.eventShouldLiveIn(threadResp1Reply1, events, roots).shouldLiveInRoom).toBeFalsy();
+            expect(room.eventShouldLiveIn(threadResp1Reply1, events, roots).shouldLiveInThread).toBeTruthy();
+            expect(room.eventShouldLiveIn(threadResp1Reply1, events, roots).threadId).toBe(thread.id);
+            expect(room.eventShouldLiveIn(threadResp1Reply1Reaction1, events, roots).shouldLiveInRoom).toBeFalsy();
+            expect(room.eventShouldLiveIn(threadResp1Reply1Reaction1, events, roots).shouldLiveInThread).toBeTruthy();
+            expect(room.eventShouldLiveIn(threadResp1Reply1Reaction1, events, roots).threadId).toBe(thread.id);
+            expect(room.eventShouldLiveIn(threadResp1Reply1Reaction2, events, roots).shouldLiveInRoom).toBeFalsy();
+            expect(room.eventShouldLiveIn(threadResp1Reply1Reaction2, events, roots).shouldLiveInThread).toBeTruthy();
+            expect(room.eventShouldLiveIn(threadResp1Reply1Reaction2, events, roots).threadId).toBe(thread.id);
+            expect(room.eventShouldLiveIn(thResp1Rep1React2Redaction, events, roots).shouldLiveInRoom).toBeFalsy();
+            expect(room.eventShouldLiveIn(thResp1Rep1React2Redaction, events, roots).shouldLiveInThread).toBeTruthy();
+            expect(room.eventShouldLiveIn(thResp1Rep1React2Redaction, events, roots).threadId).toBe(thread.id);
         });
 
         it("reply to reply to thread root should only be in the main timeline", () => {
@@ -2939,12 +2953,40 @@ describe("Room", function () {
             const roots = new Set([threadRoot.getId()!]);
             const events = [threadRoot, threadResponse1, reply1, reply2];
 
+            const thread = room.createThread(threadRoot.getId()!, threadRoot, [], false);
+            threadResponse1.setThread(thread);
+
             expect(room.eventShouldLiveIn(reply1, events, roots).shouldLiveInRoom).toBeTruthy();
             expect(room.eventShouldLiveIn(reply1, events, roots).shouldLiveInThread).toBeFalsy();
             expect(room.eventShouldLiveIn(reply2, events, roots).shouldLiveInRoom).toBeTruthy();
             expect(room.eventShouldLiveIn(reply2, events, roots).shouldLiveInThread).toBeFalsy();
         });
 
+        it("edit to thread root should live in main timeline only", () => {
+            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).toBeFalsy();
+        });
+
         it("should aggregate relations in thread event timeline set", async () => {
             Thread.setServerSideSupport(FeatureSupport.Stable);
             const threadRoot = mkMessage();

src/client.ts

@@ -5000,8 +5000,15 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         });
 
         if (!unthreaded) {
+            // 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
-            const isThread = !!event.threadRootId && !event.isThreadRoot;
+                !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.

src/models/room.ts

@@ -2093,6 +2093,14 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
         }
     }
 
+    /**
+     * Determine which timeline(s) a given event should live in
+     * Thread roots live in both the main timeline and their corresponding thread timeline
+     * Relations, redactions, replies to thread relation events live only in the thread timeline
+     * Relations (other than m.thread), redactions, replies to a thread root live only in the main timeline
+     * Relations, redactions, replies where the parent cannot be found live in no timelines but should be aggregated regardless.
+     * Otherwise, the event lives in the main timeline only.
+     */
     public eventShouldLiveIn(
         event: MatrixEvent,
         events?: MatrixEvent[],
@@ -2109,7 +2117,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
             };
         }
 
-        // A thread root is always shown in both timelines
+        // A thread root is the only event shown in both timelines
         if (event.isThreadRoot || roots?.has(event.getId()!)) {
             return {
                 shouldLiveInRoom: true,
@@ -2118,8 +2126,29 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
             };
         }
 
-        // A thread relation (1st and 2nd order) is always only shown in a thread
+        const isThreadRelation = event.isRelation(THREAD_RELATION_TYPE.name);
+        const parentEventId = event.getAssociatedId();
         const threadRootId = event.threadRootId;
+
+        // Where the parent is the thread root and this is a non-thread relation this should live only in the main timeline
+        if (!!parentEventId && !isThreadRelation && (threadRootId === parentEventId || roots?.has(parentEventId!))) {
+            return {
+                shouldLiveInRoom: true,
+                shouldLiveInThread: false,
+            };
+        }
+
+        let parentEvent: MatrixEvent | undefined;
+        if (parentEventId) {
+            parentEvent = this.findEventById(parentEventId) ?? events?.find((e) => e.getId() === parentEventId);
+        }
+
+        // Treat non-thread-relations, redactions, and replies as extensions of their parents so evaluate parentEvent instead
+        if (parentEvent && !isThreadRelation) {
+            return this.eventShouldLiveIn(parentEvent, events, roots);
+        }
+
+        // A thread relation (1st and 2nd order) is always only shown in a thread
         if (threadRootId != undefined) {
             return {
                 shouldLiveInRoom: false,
@@ -2128,33 +2157,13 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
             };
         }
 
-        const parentEventId = event.getAssociatedId();
-        let parentEvent: MatrixEvent | undefined;
-        if (parentEventId) {
-            parentEvent = this.findEventById(parentEventId) ?? events?.find((e) => e.getId() === parentEventId);
-        }
-
-        // Treat relations and redactions as extensions of their parents so evaluate parentEvent instead
-        if (parentEvent && (event.isRelation() || event.isRedaction())) {
-            return this.eventShouldLiveIn(parentEvent, events, roots);
-        }
-
-        if (!event.isRelation()) {
+        if (!parentEventId) {
             return {
                 shouldLiveInRoom: true,
                 shouldLiveInThread: false,
             };
         }
 
-        // Edge case where we know the event is a relation but don't have the parentEvent
-        if (roots?.has(event.relationEventId!)) {
-            return {
-                shouldLiveInRoom: true,
-                shouldLiveInThread: true,
-                threadId: event.relationEventId,
-            };
-        }
-
         // We've exhausted all scenarios,
         // we cannot assume that it lives in the main timeline as this may be a relation for an unknown thread
         // adding the event in the wrong timeline causes stuck notifications and can break ability to send read receipts