diff --git a/spec/integ/matrix-client-event-timeline.spec.ts b/spec/integ/matrix-client-event-timeline.spec.ts index bc28efa72..fd00a3c51 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 18e02624f..a44b3815b 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 bed1b235e..2b84239d3 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/spec/unit/read-receipt.spec.ts b/spec/unit/read-receipt.spec.ts index 2bc941ab0..1ea58dade 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,42 +42,45 @@ 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({ + 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", + 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", () => { @@ -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; diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index 777d46c46..7464faa41 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,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(); diff --git a/src/client.ts b/src/client.ts index 4c27cb0f7..1c2bdcd04 100644 --- a/src/client.ts +++ b/src/client.ts @@ -5000,8 +5000,15 @@ export class MatrixClient extends TypedEventEmitter { } } + /** + * 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,8 +2126,29 @@ export class Room extends ReadReceipt { }; } - // 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 { }; } - 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