Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix edge cases around 2nd order relations and threads #3437

Merged
merged 5 commits into from
Jun 5, 2023
Merged

Fix edge cases around 2nd order relations and threads #3437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b279003
Fix tests oversimplifying threads fixtures
t3chguy Jun 2, 2023
1dbc596
Check for unsigned thread_id in MatrixEvent::threadRootId
t3chguy Jun 2, 2023
73b65d9
Fix threads order being racy
t3chguy Jun 2, 2023
ffac8c5
Make Sonar happier
t3chguy Jun 2, 2023
f96296f
Iterate
t3chguy Jun 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions spec/integ/matrix-client-event-timeline.spec.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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