Skip to content

Commit

Permalink
Fix edge cases around 2nd order relations and threads (#3437)
Browse files Browse the repository at this point in the history 
* Fix tests oversimplifying threads fixtures

* Check for unsigned thread_id in MatrixEvent::threadRootId

* Fix threads order being racy

* Make Sonar happier

* Iterate
  • Loading branch information
@t3chguy @toger5
t3chguy authored and toger5 committed Jun 7, 2023
1 parent 6f79cd2 commit 1e2f701
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 35 deletions.
9 changes: 5 additions & 4 deletions spec/integ/matrix-client-event-timeline.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
]);
});
Expand Down Expand Up @@ -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
};
});
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand Down
27 changes: 19 additions & 8 deletions spec/unit/event-timeline-set.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
});

Expand Down
15 changes: 12 additions & 3 deletions src/models/event.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -579,9 +579,18 @@ export class MatrixEvent extends TypedEventEmitter<MatrixEventEmittedEvents, Mat
const relatesTo = this.getWireContent()?.["m.relates_to"];
if (relatesTo?.rel_type === THREAD_RELATION_TYPE.name) {
return relatesTo.event_id;
} else {
return this.getThread()?.id || this.threadId;
}
if (this.thread) {
return this.thread.id;
}
if (this.threadId !== undefined) {
return this.threadId;
}
const unsigned = this.getUnsigned();
if (typeof unsigned[UNSIGNED_THREAD_ID_FIELD.name] === "string") {
return unsigned[UNSIGNED_THREAD_ID_FIELD.name];
}
return undefined;
}

/**
Expand All @@ -593,7 +602,7 @@ export class MatrixEvent extends TypedEventEmitter<MatrixEventEmittedEvents, Mat
// Bundled relationships only returned when the sync response is limited
// hence us having to check both bundled relation and inspect the thread
// model
return !!threadDetails || this.getThread()?.id === this.getId();
return !!threadDetails || this.threadRootId === this.getId();
}

public get replyEventId(): string | undefined {
Expand Down
29 changes: 9 additions & 20 deletions src/models/room.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1957,7 +1957,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
}
}

this.on(ThreadEvent.NewReply, this.onThreadNewReply);
this.on(ThreadEvent.Update, this.onThreadUpdate);
this.on(ThreadEvent.Delete, this.onThreadDelete);
this.threadsReady = true;
}
Expand Down Expand Up @@ -2055,7 +2055,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
}
}

private onThreadNewReply(thread: Thread): void {
private onThreadUpdate(thread: Thread): void {
this.updateThreadRootEvents(thread, false, true);
}

Expand Down Expand Up @@ -2113,12 +2113,13 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
};
}

// 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,
};
}

Expand Down Expand Up @@ -2149,15 +2150,6 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
};
}

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
Expand Down Expand Up @@ -2890,12 +2882,9 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
private findThreadRoots(events: MatrixEvent[]): Set<string> {
const threadRoots = new Set<string>();
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;
Expand Down

0 comments on commit 1e2f701

Please sign in to comment.