Extra insurance that we don't mix events in the wrong timelines - v2 (#2856)

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

This PR is a v2 of #2848 since it was reverted in #2853

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`

Author: MadLittleMods

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

@@ -55,6 +55,23 @@ describe('EventTimelineSet', () => {
         });
     };
 
+    const mkThreadResponse = (root: MatrixEvent) => utils.mkEvent({
+        event: true,
+        type: EventType.RoomMessage,
+        user: userA,
+        room: roomId,
+        content: {
+            "body": "Thread response :: " + Math.random(),
+            "m.relates_to": {
+                "event_id": root.getId(),
+                "m.in_reply_to": {
+                    "event_id": root.getId(),
+                },
+                "rel_type": "m.thread",
+            },
+        },
+    }, room.client);
+
     beforeEach(() => {
         client = utils.mock(MatrixClient, 'MatrixClient');
         client.reEmitter = utils.mock(ReEmitter, 'ReEmitter');
@@ -117,6 +134,13 @@ describe('EventTimelineSet', () => {
     });
 
     describe('addEventToTimeline', () => {
+        let thread: Thread;
+
+        beforeEach(() => {
+            (client.supportsExperimentalThreads 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);
@@ -144,6 +168,58 @@ describe('EventTimelineSet', () => {
                 );
             }).not.toThrow();
         });
+
+        it("should not add an event to a timeline that does not belong to the timelineSet", () => {
+            const eventTimelineSet2 = new EventTimelineSet(room);
+            const liveTimeline2 = eventTimelineSet2.getLiveTimeline();
+            expect(liveTimeline2.getEvents().length).toStrictEqual(0);
+
+            expect(() => {
+                eventTimelineSet.addEventToTimeline(messageEvent, liveTimeline2, {
+                    toStartOfTimeline: true,
+                });
+            }).toThrowError();
+        });
+
+        it("should not add a threaded reply to the main room timeline", () => {
+            const liveTimeline = eventTimelineSet.getLiveTimeline();
+            expect(liveTimeline.getEvents().length).toStrictEqual(0);
+
+            const threadedReplyEvent = mkThreadResponse(messageEvent);
+
+            eventTimelineSet.addEventToTimeline(threadedReplyEvent, liveTimeline, {
+                toStartOfTimeline: true,
+            });
+            expect(liveTimeline.getEvents().length).toStrictEqual(0);
+        });
+
+        it("should not add a normal message to the timelineSet representing a thread", () => {
+            const eventTimelineSetForThread = new EventTimelineSet(room, {}, client, thread);
+            const liveTimeline = eventTimelineSetForThread.getLiveTimeline();
+            expect(liveTimeline.getEvents().length).toStrictEqual(0);
+
+            eventTimelineSetForThread.addEventToTimeline(messageEvent, liveTimeline, {
+                toStartOfTimeline: true,
+            });
+            expect(liveTimeline.getEvents().length).toStrictEqual(0);
+        });
+
+        describe('non-room timeline', () => {
+            it('Adds event to timeline', () => {
+                const nonRoomEventTimelineSet = new EventTimelineSet(
+                    // This is what we're specifically testing against, a timeline
+                    // without a `room` defined
+                    undefined,
+                );
+                const nonRoomEventTimeline = new EventTimeline(nonRoomEventTimelineSet);
+
+                expect(nonRoomEventTimeline.getEvents().length).toStrictEqual(0);
+                nonRoomEventTimelineSet.addEventToTimeline(messageEvent, nonRoomEventTimeline, {
+                    toStartOfTimeline: true,
+                });
+                expect(nonRoomEventTimeline.getEvents().length).toStrictEqual(1);
+            });
+        });
     });
 
     describe('aggregateRelations', () => {

src/models/event-timeline-set.ts

@@ -701,6 +701,28 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
             );
         }
 
+        if (timeline.getTimelineSet() !== this) {
+            throw new Error(`EventTimelineSet.addEventToTimeline: Timeline=${timeline.toString()} does not belong " + 
+                "in timelineSet(threadId=${this.thread?.id})`);
+        }
+
+        // Make sure events don't get mixed in timelines they shouldn't be in (e.g. a
+        // threaded message should not be in the main timeline).
+        //
+        // We can only run this check for timelines with a `room` because `canContain`
+        // requires it
+        if (this.room && !this.canContain(event)) {
+            let eventDebugString = `event=${event.getId()}`;
+            if (event.threadRootId) {
+                eventDebugString += `(belongs to thread=${event.threadRootId})`;
+            }
+            logger.warn(
+                `EventTimelineSet.addEventToTimeline: Ignoring ${eventDebugString} that does not belong ` +
+                `in timeline=${timeline.toString()} timelineSet(threadId=${this.thread?.id})`,
+            );
+            return;
+        }
+
         const eventId = event.getId()!;
         timeline.addEvent(event, {
             toStartOfTimeline,

src/models/event.ts

@@ -107,7 +107,7 @@ export interface IEventRelation {
     event_id?: string;
     is_falling_back?: boolean;
     "m.in_reply_to"?: {
-        event_id: string;
+        event_id?: string;
     };
     key?: string;
 }

src/models/room.ts

@@ -1862,7 +1862,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
         shouldLiveInThread: boolean;
         threadId?: string;
     } {
-        if (!this.client.supportsExperimentalThreads()) {
+        if (!this.client?.supportsExperimentalThreads()) {
             return {
                 shouldLiveInRoom: true,
                 shouldLiveInThread: false,