From 1e2f701f4c15920c89cb5b13830459c33a774493 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Mon, 5 Jun 2023 16:18:56 +0100 Subject: [PATCH] Fix edge cases around 2nd order relations and threads (#3437) * Fix tests oversimplifying threads fixtures * Check for unsigned thread_id in MatrixEvent::threadRootId * Fix threads order being racy * Make Sonar happier * Iterate --- .../matrix-client-event-timeline.spec.ts | 9 +++--- spec/unit/event-timeline-set.spec.ts | 27 ++++++++++++----- src/models/event.ts | 15 ++++++++-- src/models/room.ts | 29 ++++++------------- 4 files changed, 45 insertions(+), 35 deletions(-) diff --git a/spec/integ/matrix-client-event-timeline.spec.ts b/spec/integ/matrix-client-event-timeline.spec.ts index 680e408380b..c7083db1968 100644 --- a/spec/integ/matrix-client-event-timeline.spec.ts +++ b/spec/integ/matrix-client-event-timeline.spec.ts @@ -1274,6 +1274,7 @@ describe("MatrixClient event timelines", function () { THREAD_ROOT.event_id, THREAD_REPLY.event_id, THREAD_REPLY2.getId(), + THREAD_ROOT_REACTION.getId(), THREAD_REPLY3.getId(), ]); }); @@ -1322,7 +1323,7 @@ describe("MatrixClient event timelines", function () { request.respond(200, function () { return { original_event: root, - chunk: [replies], + chunk: replies, // no next batch as this is the oldest end of the timeline }; }); @@ -1479,7 +1480,7 @@ describe("MatrixClient event timelines", function () { user: userId, type: "m.room.message", content: { - "body": "thread reply", + "body": "thread2 reply", "msgtype": "m.text", "m.relates_to": { // We can't use the const here because we change server support mode for test @@ -1499,7 +1500,7 @@ describe("MatrixClient event timelines", function () { user: userId, type: "m.room.message", content: { - "body": "thread reply", + "body": "thread reply2", "msgtype": "m.text", "m.relates_to": { // We can't use the const here because we change server support mode for test @@ -1567,7 +1568,7 @@ describe("MatrixClient event timelines", function () { // Test adding a second event to the first thread const thread = room.getThread(THREAD_ROOT.event_id!)!; thread.initialEventsFetched = true; - const prom = emitPromise(room, ThreadEvent.NewReply); + const prom = emitPromise(room, ThreadEvent.Update); respondToEvent(THREAD_ROOT_UPDATED); respondToEvent(THREAD_ROOT_UPDATED); respondToEvent(THREAD_ROOT_UPDATED); diff --git a/spec/unit/event-timeline-set.spec.ts b/spec/unit/event-timeline-set.spec.ts index 7afee718967..b5445a03397 100644 --- a/spec/unit/event-timeline-set.spec.ts +++ b/spec/unit/event-timeline-set.spec.ts @@ -142,13 +142,6 @@ describe("EventTimelineSet", () => { }); describe("addEventToTimeline", () => { - let thread: Thread; - - beforeEach(() => { - (client.supportsThreads as jest.Mock).mockReturnValue(true); - thread = new Thread("!thread_id:server", messageEvent, { room, client }); - }); - it("Adds event to timeline", () => { const liveTimeline = eventTimelineSet.getLiveTimeline(); expect(liveTimeline.getEvents().length).toStrictEqual(0); @@ -167,6 +160,15 @@ describe("EventTimelineSet", () => { eventTimelineSet.addEventToTimeline(messageEvent, liveTimeline, true, false); }).not.toThrow(); }); + }); + + describe("addEventToTimeline (thread timeline)", () => { + let thread: Thread; + + beforeEach(() => { + (client.supportsThreads as jest.Mock).mockReturnValue(true); + thread = new Thread("!thread_id:server", messageEvent, { room, client }); + }); it("should not add an event to a timeline that does not belong to the timelineSet", () => { const eventTimelineSet2 = new EventTimelineSet(room); @@ -197,7 +199,14 @@ describe("EventTimelineSet", () => { const liveTimeline = eventTimelineSetForThread.getLiveTimeline(); expect(liveTimeline.getEvents().length).toStrictEqual(0); - eventTimelineSetForThread.addEventToTimeline(messageEvent, liveTimeline, { + const normalMessage = utils.mkMessage({ + room: roomId, + user: userA, + msg: "Hello!", + event: true, + }); + + eventTimelineSetForThread.addEventToTimeline(normalMessage, liveTimeline, { toStartOfTimeline: true, }); expect(liveTimeline.getEvents().length).toStrictEqual(0); @@ -336,7 +345,9 @@ describe("EventTimelineSet", () => { }); it("should return true if the timeline set is not for a thread and the event is a thread root", () => { + const thread = new Thread(messageEvent.getId()!, messageEvent, { room, client }); const eventTimelineSet = new EventTimelineSet(room, {}, client); + messageEvent.setThread(thread); expect(eventTimelineSet.canContain(messageEvent)).toBeTruthy(); }); diff --git a/src/models/event.ts b/src/models/event.ts index feb21fbba74..3b372c776a9 100644 --- a/src/models/event.ts +++ b/src/models/event.ts @@ -579,9 +579,18 @@ export class MatrixEvent extends TypedEventEmitter { } } - this.on(ThreadEvent.NewReply, this.onThreadNewReply); + this.on(ThreadEvent.Update, this.onThreadUpdate); this.on(ThreadEvent.Delete, this.onThreadDelete); this.threadsReady = true; } @@ -2055,7 +2055,7 @@ export class Room extends ReadReceipt { } } - private onThreadNewReply(thread: Thread): void { + private onThreadUpdate(thread: Thread): void { this.updateThreadRootEvents(thread, false, true); } @@ -2113,12 +2113,13 @@ export class Room extends ReadReceipt { }; } - // A thread relation is always only shown in a thread - if (event.isRelation(THREAD_RELATION_TYPE.name)) { + // 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: event.threadRootId, + threadId: threadRootId, }; } @@ -2149,15 +2150,6 @@ export class Room extends ReadReceipt { }; } - const unsigned = event.getUnsigned(); - if (typeof unsigned[UNSIGNED_THREAD_ID_FIELD.name] === "string") { - return { - shouldLiveInRoom: false, - shouldLiveInThread: true, - threadId: unsigned[UNSIGNED_THREAD_ID_FIELD.name], - }; - } - // 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 @@ -2890,12 +2882,9 @@ export class Room extends ReadReceipt { private findThreadRoots(events: MatrixEvent[]): Set { const threadRoots = new Set(); for (const event of events) { - if (event.isRelation(THREAD_RELATION_TYPE.name)) { - threadRoots.add(event.relationEventId ?? ""); - } - const unsigned = event.getUnsigned(); - if (typeof unsigned[UNSIGNED_THREAD_ID_FIELD.name] === "string") { - threadRoots.add(unsigned[UNSIGNED_THREAD_ID_FIELD.name]!); + const threadRootId = event.threadRootId; + if (threadRootId != undefined) { + threadRoots.add(threadRootId); } } return threadRoots;