Skip to content

Commit 433b7af

Browse files
Extra insurance that we don't mix events in the wrong timelines (#2848)
Add checks to `addEventToTimeline` as extra insurance that we don't mix events in the wrong timelines (main timeline vs thread timeline). Split out from #2521 Previously, we just relied on the callers to make sure they're doing the right thing and since it's easy to get it wrong, we mixed and bugs happened. Call stacks for how events get added to a timeline: - `TimelineSet.addEventsToTimeline` -> `TimelineSet.addEventToTimeline` -> `Timeline.addEvent` - `TimelineSet.addEventToTimeline` -> `Timeline.addEvent` - `TimelineSet.addLiveEvent` -> `TimelineSet.addEventToTimeline` -> `Timeline.addEvent`
1 parent 777cf1f commit 433b7af

File tree

5 files changed

+74
-2
lines changed

5 files changed

+74
-2
lines changed

.DS_Store

6 KB
Binary file not shown.

spec/unit/event-timeline-set.spec.ts

+59
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,23 @@ describe('EventTimelineSet', () => {
5555
});
5656
};
5757

58+
const mkThreadResponse = (root: MatrixEvent) => utils.mkEvent({
59+
event: true,
60+
type: EventType.RoomMessage,
61+
user: userA,
62+
room: roomId,
63+
content: {
64+
"body": "Thread response :: " + Math.random(),
65+
"m.relates_to": {
66+
"event_id": root.getId(),
67+
"m.in_reply_to": {
68+
"event_id": root.getId(),
69+
},
70+
"rel_type": "m.thread",
71+
},
72+
},
73+
}, room.client);
74+
5875
beforeEach(() => {
5976
client = utils.mock(MatrixClient, 'MatrixClient');
6077
client.reEmitter = utils.mock(ReEmitter, 'ReEmitter');
@@ -117,6 +134,13 @@ describe('EventTimelineSet', () => {
117134
});
118135

119136
describe('addEventToTimeline', () => {
137+
let thread: Thread;
138+
139+
beforeEach(() => {
140+
(client.supportsExperimentalThreads as jest.Mock).mockReturnValue(true);
141+
thread = new Thread("!thread_id:server", messageEvent, { room, client });
142+
});
143+
120144
it("Adds event to timeline", () => {
121145
const liveTimeline = eventTimelineSet.getLiveTimeline();
122146
expect(liveTimeline.getEvents().length).toStrictEqual(0);
@@ -144,6 +168,41 @@ describe('EventTimelineSet', () => {
144168
);
145169
}).not.toThrow();
146170
});
171+
172+
it("should not add an event to a timeline that does not belong to the timelineSet", () => {
173+
const eventTimelineSet2 = new EventTimelineSet(room);
174+
const liveTimeline2 = eventTimelineSet2.getLiveTimeline();
175+
expect(liveTimeline2.getEvents().length).toStrictEqual(0);
176+
177+
expect(() => {
178+
eventTimelineSet.addEventToTimeline(messageEvent, liveTimeline2, {
179+
toStartOfTimeline: true,
180+
});
181+
}).toThrowError();
182+
});
183+
184+
it("should not add a threaded reply to the main room timeline", () => {
185+
const liveTimeline = eventTimelineSet.getLiveTimeline();
186+
expect(liveTimeline.getEvents().length).toStrictEqual(0);
187+
188+
const threadedReplyEvent = mkThreadResponse(messageEvent);
189+
190+
eventTimelineSet.addEventToTimeline(threadedReplyEvent, liveTimeline, {
191+
toStartOfTimeline: true,
192+
});
193+
expect(liveTimeline.getEvents().length).toStrictEqual(0);
194+
});
195+
196+
it("should not add a normal message to the timelineSet representing a thread", () => {
197+
const eventTimelineSetForThread = new EventTimelineSet(room, {}, client, thread);
198+
const liveTimeline = eventTimelineSetForThread.getLiveTimeline();
199+
expect(liveTimeline.getEvents().length).toStrictEqual(0);
200+
201+
eventTimelineSetForThread.addEventToTimeline(messageEvent, liveTimeline, {
202+
toStartOfTimeline: true,
203+
});
204+
expect(liveTimeline.getEvents().length).toStrictEqual(0);
205+
});
147206
});
148207

149208
describe('aggregateRelations', () => {

src/models/event-timeline-set.ts

+13
Original file line numberDiff line numberDiff line change
@@ -702,6 +702,19 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
702702
);
703703
}
704704

705+
if (timeline.getTimelineSet() !== this) {
706+
throw new Error(`EventTimelineSet.addEventToTimeline: Timeline=${timeline.toString()} does not belong " +
707+
"in timelineSet(threadId=${this.thread?.id})`);
708+
}
709+
710+
// Make sure events don't get mixed in timelines they shouldn't be in
711+
// (e.g. a threaded message should not be in the main timeline).
712+
if (!this.canContain(event)) {
713+
logger.warn(`EventTimelineSet.addEventToTimeline: Ignoring event=${event.getId()} that does not belong " +
714+
"in timeline=${timeline.toString()} timelineSet(threadId=${this.thread?.id})`);
715+
return;
716+
}
717+
705718
const eventId = event.getId()!;
706719
timeline.addEvent(event, {
707720
toStartOfTimeline,

src/models/event.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ export interface IEventRelation {
107107
event_id?: string;
108108
is_falling_back?: boolean;
109109
"m.in_reply_to"?: {
110-
event_id: string;
110+
event_id?: string;
111111
};
112112
key?: string;
113113
}

src/models/room.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1862,7 +1862,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
18621862
shouldLiveInThread: boolean;
18631863
threadId?: string;
18641864
} {
1865-
if (!this.client.supportsExperimentalThreads()) {
1865+
if (!this.client?.supportsExperimentalThreads()) {
18661866
return {
18671867
shouldLiveInRoom: true,
18681868
shouldLiveInThread: false,

0 commit comments

Comments
 (0)