Fix issue with getEventTimeline returning undefined for thread roots in main timeline (#2454)

* Fix test message utils using overload

* Tweak existing tests

* Add test around `MatrixClient::getEventTimeline`

* Fix test to actually exercise the faulty behaviour

* Extract timelineSet thread belongs logic and test it

* tweak method name

Author: t3chguy

spec/integ/matrix-client-event-timeline.spec.js

@@ -70,10 +70,23 @@ const EVENTS = [
     }),
 ];
 
-const THREAD_ROOT = utils.mkMessage({
+const THREAD_ROOT = utils.mkEvent({
     room: roomId,
     user: userId,
-    msg: "thread root",
+    type: "m.room.message",
+    content: {
+        "body": "thread root",
+        "msgtype": "m.text",
+    },
+    unsigned: {
+        "m.relations": {
+            "io.element.thread": {
+                "latest_event": undefined,
+                "count": 1,
+                "current_user_participated": true,
+            },
+        },
+    },
 });
 
 const THREAD_REPLY = utils.mkEvent({
@@ -91,6 +104,8 @@ const THREAD_REPLY = utils.mkEvent({
     },
 });
 
+THREAD_ROOT.unsigned["m.relations"]["io.element.thread"].latest_event = THREAD_REPLY;
+
 // start the client, and wait for it to initialise
 function startClient(httpBackend, client) {
     httpBackend.when("GET", "/versions").respond(200, {});
@@ -540,20 +555,46 @@ describe("MatrixClient event timelines", function() {
             expect(timeline.getEvents().find(e => e.getId() === THREAD_REPLY.event_id)).toBeTruthy();
         });
 
-        it("should return undefined when event is not within a thread but timelineSet is", async () => {
+        it("should return relevant timeline from non-thread timelineSet when asking for the thread root", async () => {
             client.clientOpts.experimentalThreadSupport = true;
             Thread.setServerSideSupport(true);
             client.stopClient(); // we don't need the client to be syncing at this time
             const room = client.getRoom(roomId);
             const threadRoot = new MatrixEvent(THREAD_ROOT);
             const thread = room.createThread(THREAD_ROOT.event_id, threadRoot, [threadRoot], false);
-            const timelineSet = thread.timelineSet;
+            const timelineSet = room.getTimelineSets()[0];
 
-            httpBackend.when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id))
+            httpBackend.when("GET", "/rooms/!foo%3Abar/context/" + encodeURIComponent(THREAD_ROOT.event_id))
                 .respond(200, function() {
-                    return THREAD_ROOT;
+                    return {
+                        start: "start_token0",
+                        events_before: [],
+                        event: THREAD_ROOT,
+                        events_after: [],
+                        end: "end_token0",
+                        state: [],
+                    };
                 });
 
+            const [timeline] = await Promise.all([
+                client.getEventTimeline(timelineSet, THREAD_ROOT.event_id),
+                httpBackend.flushAllExpected(),
+            ]);
+
+            expect(timeline).not.toBe(thread.liveTimeline);
+            expect(timelineSet.getTimelines()).toContain(timeline);
+            expect(timeline.getEvents().find(e => e.getId() === THREAD_ROOT.event_id)).toBeTruthy();
+        });
+
+        it("should return undefined when event is not in the thread that the given timelineSet is representing", () => {
+            client.clientOpts.experimentalThreadSupport = true;
+            Thread.setServerSideSupport(true);
+            client.stopClient(); // we don't need the client to be syncing at this time
+            const room = client.getRoom(roomId);
+            const threadRoot = new MatrixEvent(THREAD_ROOT);
+            const thread = room.createThread(THREAD_ROOT.event_id, threadRoot, [threadRoot], false);
+            const timelineSet = thread.timelineSet;
+
             httpBackend.when("GET", "/rooms/!foo%3Abar/context/" + encodeURIComponent(EVENTS[0].event_id))
                 .respond(200, function() {
                     return {
@@ -566,14 +607,13 @@ describe("MatrixClient event timelines", function() {
                     };
                 });
 
-            const timelinePromise = client.getEventTimeline(timelineSet, EVENTS[0].event_id);
-            await httpBackend.flushAllExpected();
-
-            const timeline = await timelinePromise;
-            expect(timeline).toBeUndefined();
+            return Promise.all([
+                expect(client.getEventTimeline(timelineSet, EVENTS[0].event_id)).resolves.toBeUndefined(),
+                httpBackend.flushAllExpected(),
+            ]);
         });
 
-        it("should return undefined when event is within a thread but timelineSet is not", async () => {
+        it("should return undefined when event is within a thread but timelineSet is not", () => {
             client.clientOpts.experimentalThreadSupport = true;
             Thread.setServerSideSupport(true);
             client.stopClient(); // we don't need the client to be syncing at this time
@@ -592,11 +632,10 @@ describe("MatrixClient event timelines", function() {
                     };
                 });
 
-            const timelinePromise = client.getEventTimeline(timelineSet, THREAD_REPLY.event_id);
-            await httpBackend.flushAllExpected();
-
-            const timeline = await timelinePromise;
-            expect(timeline).toBeUndefined();
+            return Promise.all([
+                expect(client.getEventTimeline(timelineSet, THREAD_REPLY.event_id)).resolves.toBeUndefined(),
+                httpBackend.flushAllExpected(),
+            ]);
         });
 
         it("should should add lazy loading filter when requested", async () => {

spec/test-utils/test-utils.ts

@@ -74,7 +74,6 @@ interface IEventOpts {
     sender?: string;
     skey?: string;
     content: IContent;
-    event?: boolean;
     user?: string;
     unsigned?: IUnsigned;
     redacts?: string;
@@ -93,7 +92,9 @@ let testEventIndex = 1; // counter for events, easier for comparison of randomly
  * @param {MatrixClient} client If passed along with opts.event=true will be used to set up re-emitters.
  * @return {Object} a JSON object representing this event.
  */
-export function mkEvent(opts: IEventOpts, client?: MatrixClient): object | MatrixEvent {
+export function mkEvent(opts: IEventOpts & { event: true }, client?: MatrixClient): MatrixEvent;
+export function mkEvent(opts: IEventOpts & { event?: false }, client?: MatrixClient): object;
+export function mkEvent(opts: IEventOpts & { event?: boolean }, client?: MatrixClient): object | MatrixEvent {
     if (!opts.type || !opts.content) {
         throw new Error("Missing .type or .content =>" + JSON.stringify(opts));
     }
@@ -143,7 +144,9 @@ interface IPresenceOpts {
  * @param {Object} opts Values for the presence.
  * @return {Object|MatrixEvent} The event
  */
-export function mkPresence(opts: IPresenceOpts): object | MatrixEvent {
+export function mkPresence(opts: IPresenceOpts & { event: true }): MatrixEvent;
+export function mkPresence(opts: IPresenceOpts & { event?: false }): object;
+export function mkPresence(opts: IPresenceOpts & { event?: boolean }): object | MatrixEvent {
     const event = {
         event_id: "$" + Math.random() + "-" + Math.random(),
         type: "m.presence",
@@ -182,7 +185,9 @@ interface IMembershipOpts {
  * @param {boolean} opts.event True to make a MatrixEvent.
  * @return {Object|MatrixEvent} The event
  */
-export function mkMembership(opts: IMembershipOpts): object | MatrixEvent {
+export function mkMembership(opts: IMembershipOpts & { event: true }): MatrixEvent;
+export function mkMembership(opts: IMembershipOpts & { event?: false }): object;
+export function mkMembership(opts: IMembershipOpts & { event?: boolean }): object | MatrixEvent {
     const eventOpts: IEventOpts = {
         ...opts,
         type: EventType.RoomMember,
@@ -220,7 +225,9 @@ interface IMessageOpts {
  * @param {MatrixClient} client If passed along with opts.event=true will be used to set up re-emitters.
  * @return {Object|MatrixEvent} The event
  */
-export function mkMessage(opts: IMessageOpts, client?: MatrixClient): object | MatrixEvent {
+export function mkMessage(opts: IMessageOpts & { event: true }, client?: MatrixClient): MatrixEvent;
+export function mkMessage(opts: IMessageOpts & { event?: false }, client?: MatrixClient): object;
+export function mkMessage(opts: IMessageOpts & { event?: boolean }, client?: MatrixClient): object | MatrixEvent {
     const eventOpts: IEventOpts = {
         ...opts,
         type: EventType.RoomMessage,
@@ -252,7 +259,12 @@ interface IReplyMessageOpts extends IMessageOpts {
  * @param {MatrixClient} client If passed along with opts.event=true will be used to set up re-emitters.
  * @return {Object|MatrixEvent} The event
  */
-export function mkReplyMessage(opts: IReplyMessageOpts, client?: MatrixClient): object | MatrixEvent {
+export function mkReplyMessage(opts: IReplyMessageOpts & { event: true }, client?: MatrixClient): MatrixEvent;
+export function mkReplyMessage(opts: IReplyMessageOpts & { event?: false }, client?: MatrixClient): object;
+export function mkReplyMessage(
+    opts: IReplyMessageOpts & { event?: boolean },
+    client?: MatrixClient,
+): object | MatrixEvent {
     const eventOpts: IEventOpts = {
         ...opts,
         type: EventType.RoomMessage,

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

@@ -25,6 +25,8 @@ import {
     Room,
     DuplicateStrategy,
 } from '../../src';
+import { Thread } from "../../src/models/thread";
+import { ReEmitter } from "../../src/ReEmitter";
 
 describe('EventTimelineSet', () => {
     const roomId = '!foo:bar';
@@ -54,6 +56,7 @@ describe('EventTimelineSet', () => {
 
     beforeEach(() => {
         client = utils.mock(MatrixClient, 'MatrixClient');
+        client.reEmitter = utils.mock(ReEmitter, 'ReEmitter');
         room = new Room(roomId, client, userA);
         eventTimelineSet = new EventTimelineSet(room);
         eventTimeline = new EventTimeline(eventTimelineSet);
@@ -62,14 +65,14 @@ describe('EventTimelineSet', () => {
             user: userA,
             msg: 'Hi!',
             event: true,
-        }) as MatrixEvent;
+        });
         replyEvent = utils.mkReplyMessage({
             room: roomId,
             user: userA,
             msg: 'Hoo!',
             event: true,
             replyToMessage: messageEvent,
-        }) as MatrixEvent;
+        });
     });
 
     describe('addLiveEvent', () => {
@@ -91,7 +94,7 @@ describe('EventTimelineSet', () => {
             // make a duplicate
             const duplicateMessageEvent = utils.mkMessage({
                 room: roomId, user: userA, msg: "dupe", event: true,
-            }) as MatrixEvent;
+            });
             duplicateMessageEvent.event.event_id = messageEvent.getId();
 
             // Adding the duplicate event should replace the `messageEvent`
@@ -220,4 +223,72 @@ describe('EventTimelineSet', () => {
             });
         });
     });
+
+    describe("canContain", () => {
+        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);
+
+        let thread: Thread;
+
+        beforeEach(() => {
+            (client.supportsExperimentalThreads as jest.Mock).mockReturnValue(true);
+            thread = new Thread("!thread_id:server", messageEvent, { room, client });
+        });
+
+        it("should throw if timeline set has no room", () => {
+            const eventTimelineSet = new EventTimelineSet(undefined, {}, client);
+            expect(() => eventTimelineSet.canContain(messageEvent)).toThrowError();
+        });
+
+        it("should return false if timeline set is for thread but event is not threaded", () => {
+            const eventTimelineSet = new EventTimelineSet(room, {}, client, thread);
+            expect(eventTimelineSet.canContain(replyEvent)).toBeFalsy();
+        });
+
+        it("should return false if timeline set it for thread but event it for a different thread", () => {
+            const eventTimelineSet = new EventTimelineSet(room, {}, client, thread);
+            const event = mkThreadResponse(replyEvent);
+            expect(eventTimelineSet.canContain(event)).toBeFalsy();
+        });
+
+        it("should return false if timeline set is not for a thread but event is a thread response", () => {
+            const eventTimelineSet = new EventTimelineSet(room, {}, client);
+            const event = mkThreadResponse(replyEvent);
+            expect(eventTimelineSet.canContain(event)).toBeFalsy();
+        });
+
+        it("should return true if the timeline set is not for a thread and the event is a thread root", () => {
+            const eventTimelineSet = new EventTimelineSet(room, {}, client);
+            expect(eventTimelineSet.canContain(messageEvent)).toBeTruthy();
+        });
+
+        it("should return true if the timeline set is for a thread and the event is its thread root", () => {
+            const thread = new Thread(messageEvent.getId(), messageEvent, { room, client });
+            const eventTimelineSet = new EventTimelineSet(room, {}, client, thread);
+            messageEvent.setThread(thread);
+            expect(eventTimelineSet.canContain(messageEvent)).toBeTruthy();
+        });
+
+        it("should return true if the timeline set is for a thread and the event is a response to it", () => {
+            const thread = new Thread(messageEvent.getId(), messageEvent, { room, client });
+            const eventTimelineSet = new EventTimelineSet(room, {}, client, thread);
+            messageEvent.setThread(thread);
+            const event = mkThreadResponse(messageEvent);
+            expect(eventTimelineSet.canContain(event)).toBeTruthy();
+        });
+    });
 });

spec/unit/filter-component.spec.ts

@@ -1,7 +1,4 @@
-import {
-    MatrixEvent,
-    RelationType,
-} from "../../src";
+import { RelationType } from "../../src";
 import { FilterComponent } from "../../src/filter-component";
 import { mkEvent } from '../test-utils/test-utils';
 
@@ -14,7 +11,7 @@ describe("Filter Component", function() {
                 content: { },
                 room: 'roomId',
                 event: true,
-            }) as MatrixEvent;
+            });
 
             const checkResult = filter.check(event);
 
@@ -28,7 +25,7 @@ describe("Filter Component", function() {
                 content: { },
                 room: 'roomId',
                 event: true,
-            }) as MatrixEvent;
+            });
 
             const checkResult = filter.check(event);
 
@@ -55,7 +52,7 @@ describe("Filter Component", function() {
                         },
                     },
                 },
-            }) as MatrixEvent;
+            });
 
             expect(filter.check(threadRootNotParticipated)).toBe(false);
         });
@@ -80,7 +77,7 @@ describe("Filter Component", function() {
                 user: '@someone-else:server.org',
                 room: 'roomId',
                 event: true,
-            }) as MatrixEvent;
+            });
 
             expect(filter.check(threadRootParticipated)).toBe(true);
         });
@@ -100,7 +97,7 @@ describe("Filter Component", function() {
                         [RelationType.Reference]: {},
                     },
                 },
-            }) as MatrixEvent;
+            });
 
             expect(filter.check(referenceRelationEvent)).toBe(false);
         });
@@ -123,7 +120,7 @@ describe("Filter Component", function() {
                 },
                 room: 'roomId',
                 event: true,
-            }) as MatrixEvent;
+            });
 
             const eventWithMultipleRelations = mkEvent({
                 "type": "m.room.message",
@@ -148,7 +145,7 @@ describe("Filter Component", function() {
                 },
                 "room": 'roomId',
                 "event": true,
-            }) as MatrixEvent;
+            });
 
             const noMatchEvent = mkEvent({
                 "type": "m.room.message",
@@ -160,7 +157,7 @@ describe("Filter Component", function() {
                 },
                 "room": 'roomId',
                 "event": true,
-            }) as MatrixEvent;
+            });
 
             expect(filter.check(threadRootEvent)).toBe(true);
             expect(filter.check(eventWithMultipleRelations)).toBe(true);