From c350296de707cc40788fb180a62230f9dc47c871 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 18 Jul 2023 11:18:44 +0100 Subject: [PATCH 1/7] Ensure non-thread relations to a thread root are actually in both timelines --- spec/unit/room.spec.ts | 26 ++++++++++++++++++++++++++ src/models/room.ts | 38 ++++++++++++++++++++------------------ 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index 777d46c469f..e3bb1b385e8 100644 --- a/spec/unit/room.spec.ts +++ b/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(); diff --git a/src/models/room.ts b/src/models/room.ts index a73c750bad3..7a0cae6963b 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -2118,40 +2118,42 @@ export class Room extends ReadReceipt { }; } - // A thread relation (1st and 2nd order) is always only shown in a thread - const threadRootId = event.threadRootId; - if (threadRootId != undefined) { - return { - shouldLiveInRoom: false, - shouldLiveInThread: true, - threadId: threadRootId, - }; - } + const isRelation = event.isRelation(); + const isThreadRelation = event.isRelation(RelationType.Thread); 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())) { + // Treat non-thread-relations and redactions as extensions of their parents so evaluate parentEvent instead + if (parentEvent && !isThreadRelation && (isRelation || event.isRedaction())) { return this.eventShouldLiveIn(parentEvent, events, roots); } - if (!event.isRelation()) { + // Edge case where we know the event is a non-thread relation but don't have the parentEvent + if (isRelation && !isThreadRelation && roots?.has(event.relationEventId!)) { return { shouldLiveInRoom: true, - shouldLiveInThread: false, + shouldLiveInThread: true, + threadId: event.relationEventId, }; } - // Edge case where we know the event is a relation but don't have the parentEvent - if (roots?.has(event.relationEventId!)) { + // A thread relation (1st and 2nd order) is always only shown in a thread + const threadRootId = event.threadRootId; + if (threadRootId != undefined) { return { - shouldLiveInRoom: true, + shouldLiveInRoom: false, shouldLiveInThread: true, - threadId: event.relationEventId, + threadId: threadRootId, + }; + } + + if (!isRelation) { + return { + shouldLiveInRoom: true, + shouldLiveInThread: false, }; } From 06bdb4f7e2263899a1fcafc0ad769d977dd658e3 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 18 Jul 2023 11:46:00 +0100 Subject: [PATCH 2/7] Make thread in sendReceipt & sendReadReceipt explicit rather than guessing it --- spec/unit/read-receipt.spec.ts | 133 ++++++++++++++++++++++++++++++++- src/client.ts | 86 +++++++++++++++++---- 2 files changed, 203 insertions(+), 16 deletions(-) diff --git a/spec/unit/read-receipt.spec.ts b/spec/unit/read-receipt.spec.ts index 2bc941ab0af..c29847363d8 100644 --- a/spec/unit/read-receipt.spec.ts +++ b/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", () => { diff --git a/src/client.ts b/src/client.ts index a85b04f7dc2..ddda09578e1 100644 --- a/src/client.ts +++ b/src/client.ts @@ -4973,43 +4973,84 @@ export class MatrixClient extends TypedEventEmitter, - unthreaded = false, + ): Promise<{}>; + /** + * @deprecated backwards compatibility signature + */ + public async sendReceipt( + event: MatrixEvent, + receiptType: ReceiptType, + body?: Record, + unthreaded?: boolean, + ): Promise<{}>; + public async sendReceipt( + event: MatrixEvent, + receiptType: ReceiptType, + bodyOrThread?: Record | string | null, + unthreadedOrBody?: boolean | Record, ): Promise<{}> { if (this.isGuest()) { return Promise.resolve({}); // guests cannot send receipts so don't bother. } + let thread: string | null; + let body: Record; + + if ((bodyOrThread !== null && typeof bodyOrThread === "object") || typeof unthreadedOrBody === "boolean") { + // Backwards compatible signature case + thread = !unthreadedOrBody ? this.guessThreadForReceipt(event) : null; + body = (bodyOrThread as Record) ?? {}; + } 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; + /** + * @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 Date: Tue, 18 Jul 2023 12:07:51 +0100 Subject: [PATCH 3/7] Apply suggestions from code review --- spec/unit/read-receipt.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/unit/read-receipt.spec.ts b/spec/unit/read-receipt.spec.ts index c29847363d8..8126256e35c 100644 --- a/spec/unit/read-receipt.spec.ts +++ b/spec/unit/read-receipt.spec.ts @@ -166,7 +166,7 @@ describe("Read receipt", () => { await flushPromises(); }); - it("sends an main timeline receipt for a thread root (deprecated)", async () => { + it("sends a main timeline receipt for a thread root (deprecated)", async () => { const threadRootEvent = utils.mkEvent({ event: true, type: EventType.RoomMessage, @@ -197,7 +197,7 @@ describe("Read receipt", () => { await flushPromises(); }); - it("sends an main timeline receipt for a reaction to a thread root (deprecated)", async () => { + it("sends a main timeline receipt for a reaction to a thread root (deprecated)", async () => { const threadRootEvent = utils.mkEvent({ event: true, type: EventType.RoomMessage, From bed148f0d1ec2f30d8b4f2829aa2b29afac28080 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 18 Jul 2023 15:50:15 +0100 Subject: [PATCH 4/7] Fix Room::eventShouldLiveIn to better match Synapse to diverging ideas of notifications --- spec/unit/room.spec.ts | 66 ++++++++++++++++++++++++++---------------- src/models/room.ts | 39 +++++++++++++++---------- 2 files changed, 64 insertions(+), 41 deletions(-) diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index e3bb1b385e8..7464faa4174 100644 --- a/spec/unit/room.spec.ts +++ b/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,13 +2953,16 @@ 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 both", () => { + it("edit to thread root should live in main timeline only", () => { const threadRoot = mkMessage(); const threadResponse1 = mkThreadResponse(threadRoot); const threadRootEdit = mkEdit(threadRoot); @@ -2967,8 +2984,7 @@ describe("Room", function () { 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()); + expect(room.eventShouldLiveIn(threadRootEdit, events, roots).shouldLiveInThread).toBeFalsy(); }); it("should aggregate relations in thread event timeline set", async () => { diff --git a/src/models/room.ts b/src/models/room.ts index 7a0cae6963b..0dec76da2dd 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -2093,6 +2093,14 @@ export class Room extends ReadReceipt { } } + /** + * 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 { }; } - // 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,30 +2126,29 @@ export class Room extends ReadReceipt { }; } - const isRelation = event.isRelation(); const isThreadRelation = event.isRelation(RelationType.Thread); - 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 and redactions as extensions of their parents so evaluate parentEvent instead - if (parentEvent && !isThreadRelation && (isRelation || event.isRedaction())) { - return this.eventShouldLiveIn(parentEvent, events, roots); - } - // Edge case where we know the event is a non-thread relation but don't have the parentEvent - if (isRelation && !isThreadRelation && roots?.has(event.relationEventId!)) { - return { - shouldLiveInRoom: true, - shouldLiveInThread: true, - threadId: event.relationEventId, - }; + // 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 - const threadRootId = event.threadRootId; if (threadRootId != undefined) { return { shouldLiveInRoom: false, @@ -2150,7 +2157,7 @@ export class Room extends ReadReceipt { }; } - if (!isRelation) { + if (!parentEventId) { return { shouldLiveInRoom: true, shouldLiveInThread: false, From d1c79fb601be9056ddf148ff7435e758ba2d60b1 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 18 Jul 2023 15:58:50 +0100 Subject: [PATCH 5/7] Update read receipt sending behaviour to align with Synapse --- spec/unit/read-receipt.spec.ts | 193 +++++++++------------------------ src/client.ts | 93 ++++------------ 2 files changed, 71 insertions(+), 215 deletions(-) diff --git a/spec/unit/read-receipt.spec.ts b/spec/unit/read-receipt.spec.ts index 8126256e35c..1ea58dadedb 100644 --- a/spec/unit/read-receipt.spec.ts +++ b/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,34 +42,10 @@ let httpBackend: MockHttpBackend; const THREAD_ID = "$thread_event_id"; const ROOM_ID = "!123:matrix.org"; -const threadEvent = utils.mkEvent({ - event: true, - type: EventType.RoomMessage, - user: "@bob:matrix.org", - room: ROOM_ID, - content: { - "body": "Hello from a thread", - "m.relates_to": { - "event_id": THREAD_ID, - "m.in_reply_to": { - event_id: THREAD_ID, - }, - "rel_type": "m.thread", - }, - }, -}); - -const roomEvent = utils.mkEvent({ - event: true, - type: EventType.RoomMessage, - user: "@bob:matrix.org", - room: ROOM_ID, - content: { - body: "Hello from a room", - }, -}); - describe("Read receipt", () => { + let threadEvent: MatrixEvent; + let roomEvent: MatrixEvent; + beforeEach(() => { httpBackend = new MockHttpBackend(); client = new MatrixClient({ @@ -79,30 +55,35 @@ describe("Read receipt", () => { fetchFn: httpBackend.fetchFn as typeof global.fetch, }); client.isGuest = () => false; - }); - - describe("sendReceipt", () => { - it("sends a thread read receipt (deprecated)", 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, {}); - await httpBackend.flushAllExpected(); - await flushPromises(); + threadEvent = utils.mkEvent({ + event: true, + type: EventType.RoomMessage, + user: "@bob:matrix.org", + room: ROOM_ID, + content: { + "body": "Hello from a thread", + "m.relates_to": { + "event_id": THREAD_ID, + "m.in_reply_to": { + event_id: THREAD_ID, + }, + "rel_type": "m.thread", + }, + }, + }); + roomEvent = utils.mkEvent({ + event: true, + type: EventType.RoomMessage, + user: "@bob:matrix.org", + room: ROOM_ID, + content: { + body: "Hello from a room", + }, }); + }); + describe("sendReceipt", () => { it("sends a thread read receipt", async () => { httpBackend .when( @@ -118,28 +99,7 @@ describe("Read receipt", () => { }) .respond(200, {}); - client.sendReceipt(threadEvent, ReceiptType.Read, THREAD_ID); - - await httpBackend.flushAllExpected(); - await flushPromises(); - }); - - it("sends an unthreaded receipt (deprecated)", 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, true); + client.sendReceipt(threadEvent, ReceiptType.Read, {}); await httpBackend.flushAllExpected(); await flushPromises(); @@ -160,76 +120,13 @@ describe("Read receipt", () => { }) .respond(200, {}); - client.sendReadReceipt(threadEvent, ReceiptType.Read, null); - - await httpBackend.flushAllExpected(); - await flushPromises(); - }); - - it("sends a 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 a 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); + client.sendReadReceipt(threadEvent, ReceiptType.Read, true); await httpBackend.flushAllExpected(); await flushPromises(); }); - it("sends a room read receipt (deprecated)", async () => { + it("sends a room read receipt", async () => { httpBackend .when( "POST", @@ -250,14 +147,25 @@ describe("Read receipt", () => { await flushPromises(); }); - it("sends a main timeline room read receipt", async () => { + 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: roomEvent.getId()!, + $eventId: reaction.getId()!, }), ) .check((request) => { @@ -265,7 +173,7 @@ describe("Read receipt", () => { }) .respond(200, {}); - client.sendReceipt(roomEvent, ReceiptType.Read, "main"); + client.sendReceipt(reaction, ReceiptType.Read, {}); await httpBackend.flushAllExpected(); await flushPromises(); @@ -274,9 +182,10 @@ describe("Read receipt", () => { 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; diff --git a/src/client.ts b/src/client.ts index ddda09578e1..4fd20cd73de 100644 --- a/src/client.ts +++ b/src/client.ts @@ -4973,84 +4973,50 @@ export class MatrixClient extends TypedEventEmitter, - ): Promise<{}>; - /** - * @deprecated backwards compatibility signature - */ public async sendReceipt( event: MatrixEvent, receiptType: ReceiptType, body?: Record, - unthreaded?: boolean, - ): Promise<{}>; - public async sendReceipt( - event: MatrixEvent, - receiptType: ReceiptType, - bodyOrThread?: Record | string | null, - unthreadedOrBody?: boolean | Record, + unthreaded = false, ): Promise<{}> { if (this.isGuest()) { return Promise.resolve({}); // guests cannot send receipts so don't bother. } - let thread: string | null; - let body: Record; - - if ((bodyOrThread !== null && typeof bodyOrThread === "object") || typeof unthreadedOrBody === "boolean") { - // Backwards compatible signature case - thread = !unthreadedOrBody ? this.guessThreadForReceipt(event) : null; - body = (bodyOrThread as Record) ?? {}; - } 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 (thread !== null) { - // Only thread replies should define a specific thread. Thread roots can only be read in the main timeline. - body.thread_id = thread; + 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 + !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); + 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, body || {}); const room = this.getRoom(event.getRoomId()); if (room && this.credentials.userId) { @@ -5063,28 +5029,13 @@ export class MatrixClient extends TypedEventEmitter; - /** - * @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, - threadOrUnthreaded: boolean | string | null = false, + unthreaded = false, ): Promise<{} | undefined> { if (!event) return; const eventId = event.getId()!; @@ -5093,11 +5044,7 @@ export class MatrixClient extends TypedEventEmitter Date: Tue, 18 Jul 2023 16:17:21 +0100 Subject: [PATCH 6/7] Fix tests --- .../matrix-client-event-timeline.spec.ts | 1 - spec/integ/matrix-client-methods.spec.ts | 38 ++++++------------- spec/test-utils/thread.ts | 4 +- src/models/room.ts | 2 +- 4 files changed, 14 insertions(+), 31 deletions(-) diff --git a/spec/integ/matrix-client-event-timeline.spec.ts b/spec/integ/matrix-client-event-timeline.spec.ts index bc28efa7222..fd00a3c51ed 100644 --- a/spec/integ/matrix-client-event-timeline.spec.ts +++ b/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(), ]); }); diff --git a/spec/integ/matrix-client-methods.spec.ts b/spec/integ/matrix-client-methods.spec.ts index 18e02624fa2..a44b3815bd5 100644 --- a/spec/integ/matrix-client-methods.spec.ts +++ b/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]); }); }); diff --git a/spec/test-utils/thread.ts b/spec/test-utils/thread.ts index bed1b235e87..2b84239d367 100644 --- a/spec/test-utils/thread.ts +++ b/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, }, diff --git a/src/models/room.ts b/src/models/room.ts index 0dec76da2dd..ac42eb83411 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -2126,7 +2126,7 @@ export class Room extends ReadReceipt { }; } - const isThreadRelation = event.isRelation(RelationType.Thread); + const isThreadRelation = event.isRelation(THREAD_RELATION_TYPE.name); const parentEventId = event.getAssociatedId(); const threadRootId = event.threadRootId; From 9b2301614bfddb5eb961aa7e4bcc71be51ef4afa Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 18 Jul 2023 17:07:09 +0100 Subject: [PATCH 7/7] Fix thread rel type --- src/client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client.ts b/src/client.ts index 4fd20cd73de..88ae6739aae 100644 --- a/src/client.ts +++ b/src/client.ts @@ -5008,7 +5008,7 @@ export class MatrixClient extends TypedEventEmitter