Fix issues with getEventTimeline and thread roots (#2444)

t3chguy · web-flow · commit 8e896c4da3f9 · 2022-06-08T22:11:22.000Z
* Add additional tests for thread timelines

* Fix issues around mixing up event timeline sets with /context/ API

* Increase coverage

* Increase coverage

* Better scope assertions

* Iterate PR
diff --git a/spec/integ/matrix-client-event-timeline.spec.js b/spec/integ/matrix-client-event-timeline.spec.js
@@ -1,5 +1,5 @@
 import * as utils from "../test-utils/test-utils";
-import { EventTimeline } from "../../src/matrix";
+import { EventTimeline, Filter, MatrixEvent } from "../../src/matrix";
 import { logger } from "../../src/logger";
 import { TestClient } from "../TestClient";
 import { Thread, THREAD_RELATION_TYPE } from "../../src/models/thread";
@@ -500,7 +500,8 @@ describe("MatrixClient event timelines", function() {
             Thread.setServerSideSupport(true);
             client.stopClient(); // we don't need the client to be syncing at this time
             const room = client.getRoom(roomId);
-            const timelineSet = room.getTimelineSets()[0];
+            const thread = room.createThread(THREAD_ROOT.event_id, undefined, [], false);
+            const timelineSet = thread.timelineSet;
 
             httpBackend.when("GET", "/rooms/!foo%3Abar/context/" + encodeURIComponent(THREAD_REPLY.event_id))
                 .respond(200, function() {
@@ -538,6 +539,92 @@ describe("MatrixClient event timelines", function() {
             expect(timeline.getEvents().find(e => e.getId() === THREAD_ROOT.event_id)).toBeTruthy();
             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 () => {
+            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/event/" + encodeURIComponent(THREAD_ROOT.event_id))
+                .respond(200, function() {
+                    return THREAD_ROOT;
+                });
+
+            httpBackend.when("GET", "/rooms/!foo%3Abar/context/" + encodeURIComponent(EVENTS[0].event_id))
+                .respond(200, function() {
+                    return {
+                        start: "start_token0",
+                        events_before: [],
+                        event: EVENTS[0],
+                        events_after: [],
+                        end: "end_token0",
+                        state: [],
+                    };
+                });
+
+            const timelinePromise = client.getEventTimeline(timelineSet, EVENTS[0].event_id);
+            await httpBackend.flushAllExpected();
+
+            const timeline = await timelinePromise;
+            expect(timeline).toBeUndefined();
+        });
+
+        it("should return undefined when event is within a thread but timelineSet is not", 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 timelineSet = room.getTimelineSets()[0];
+
+            httpBackend.when("GET", "/rooms/!foo%3Abar/context/" + encodeURIComponent(THREAD_REPLY.event_id))
+                .respond(200, function() {
+                    return {
+                        start: "start_token0",
+                        events_before: [],
+                        event: THREAD_REPLY,
+                        events_after: [],
+                        end: "end_token0",
+                        state: [],
+                    };
+                });
+
+            const timelinePromise = client.getEventTimeline(timelineSet, THREAD_REPLY.event_id);
+            await httpBackend.flushAllExpected();
+
+            const timeline = await timelinePromise;
+            expect(timeline).toBeUndefined();
+        });
+
+        it("should should add lazy loading filter when requested", async () => {
+            client.clientOpts.lazyLoadMembers = true;
+            client.stopClient(); // we don't need the client to be syncing at this time
+            const room = client.getRoom(roomId);
+            const timelineSet = room.getTimelineSets()[0];
+
+            const req = httpBackend.when("GET", "/rooms/!foo%3Abar/context/" + encodeURIComponent(EVENTS[0].event_id));
+            req.respond(200, function() {
+                return {
+                    start: "start_token0",
+                    events_before: [],
+                    event: EVENTS[0],
+                    events_after: [],
+                    end: "end_token0",
+                    state: [],
+                };
+            });
+            req.check((request) => {
+                expect(request.opts.qs.filter).toEqual(JSON.stringify(Filter.LAZY_LOADING_MESSAGES_FILTER));
+            });
+
+            await Promise.all([
+                client.getEventTimeline(timelineSet, EVENTS[0].event_id),
+                httpBackend.flushAllExpected(),
+            ]);
+        });
     });
 
     describe("getLatestTimeline", function() {
diff --git a/src/client.ts b/src/client.ts
@@ -5245,14 +5245,15 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * <p>If the EventTimelineSet object already has the given event in its store, the
      * corresponding timeline will be returned. Otherwise, a /context request is
      * made, and used to construct an EventTimeline.
+     * If the event does not belong to this EventTimelineSet then undefined will be returned.
      *
-     * @param {EventTimelineSet} timelineSet  The timelineSet to look for the event in
+     * @param {EventTimelineSet} timelineSet  The timelineSet to look for the event in, must be bound to a room
      * @param {string} eventId  The ID of the event to look for
      *
      * @return {Promise} Resolves:
      *    {@link module:models/event-timeline~EventTimeline} including the given event
      */
-    public async getEventTimeline(timelineSet: EventTimelineSet, eventId: string): Promise<EventTimeline> {
+    public async getEventTimeline(timelineSet: EventTimelineSet, eventId: string): Promise<EventTimeline | undefined> {
         // don't allow any timeline support unless it's been enabled.
         if (!this.timelineSupport) {
             throw new Error("timeline support is disabled. Set the 'timelineSupport'" +
@@ -5297,38 +5298,44 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
             ...res.events_before.map(mapper),
         ];
 
-        // Where the event is a thread reply (not a root) and running in MSC-enabled mode the Thread timeline only
-        // functions contiguously, so we have to jump through some hoops to get our target event in it.
-        // XXX: workaround for https://github.com/vector-im/element-meta/issues/150
-        if (Thread.hasServerSideSupport &&
-            this.supportsExperimentalThreads() &&
-            event.isRelation(THREAD_RELATION_TYPE.name)
-        ) {
-            const [, threadedEvents] = timelineSet.room.partitionThreadedEvents(events);
-            let thread = timelineSet.room.getThread(event.threadRootId);
-            if (!thread) {
-                thread = timelineSet.room.createThread(event.threadRootId, undefined, threadedEvents, true);
+        if (this.supportsExperimentalThreads()) {
+            const { threadId, shouldLiveInRoom } = timelineSet.room.eventShouldLiveIn(event);
+
+            if (!timelineSet.thread && !shouldLiveInRoom) {
+                // Thread response does not belong in this timelineSet
+                return undefined;
             }
 
-            const opts: IRelationsRequestOpts = {
-                direction: Direction.Backward,
-                limit: 50,
-            };
+            if (timelineSet.thread?.id !== threadId) {
+                // Event does not belong in this timelineSet
+                return undefined;
+            }
 
-            await thread.fetchInitialEvents();
-            let nextBatch = thread.liveTimeline.getPaginationToken(Direction.Backward);
+            // Where the event is a thread reply (not a root) and running in MSC-enabled mode the Thread timeline only
+            // functions contiguously, so we have to jump through some hoops to get our target event in it.
+            // XXX: workaround for https://github.com/vector-im/element-meta/issues/150
+            if (Thread.hasServerSideSupport && timelineSet.thread) {
+                const thread = timelineSet.thread;
+                const opts: IRelationsRequestOpts = {
+                    direction: Direction.Backward,
+                    limit: 50,
+                };
 
-            // Fetch events until we find the one we were asked for, or we run out of pages
-            while (!thread.findEventById(eventId)) {
-                if (nextBatch) {
-                    opts.from = nextBatch;
+                await thread.fetchInitialEvents();
+                let nextBatch = thread.liveTimeline.getPaginationToken(Direction.Backward);
+
+                // Fetch events until we find the one we were asked for, or we run out of pages
+                while (!thread.findEventById(eventId)) {
+                    if (nextBatch) {
+                        opts.from = nextBatch;
+                    }
+
+                    ({ nextBatch } = await thread.fetchEvents(opts));
+                    if (!nextBatch) break;
                 }
 
-                ({ nextBatch } = await thread.fetchEvents(opts));
-                if (!nextBatch) break;
+                return thread.liveTimeline;
             }
-
-            return thread.liveTimeline;
         }
 
         // Here we handle non-thread timelines only, but still process any thread events to populate thread summaries.
diff --git a/src/models/event-timeline-set.ts b/src/models/event-timeline-set.ts
@@ -27,6 +27,7 @@ import { RoomState } from "./room-state";
 import { TypedEventEmitter } from "./typed-event-emitter";
 import { RelationsContainer } from "./relations-container";
 import { MatrixClient } from "../client";
+import { Thread } from "./thread";
 
 const DEBUG = true;
 
@@ -110,7 +111,7 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
      * map from event_id to timeline and index.
      *
      * @constructor
-     * @param {?Room} room
+     * @param {Room=} room
      * Room for this timelineSet. May be null for non-room cases, such as the
      * notification timeline.
      * @param {Object} opts Options inherited from Room.
@@ -119,13 +120,15 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
      * Set to true to enable improved timeline support.
      * @param {Object} [opts.filter = null]
      * The filter object, if any, for this timelineSet.
-     * @param {MatrixClient} client the Matrix client which owns this EventTimelineSet,
+     * @param {MatrixClient=} client the Matrix client which owns this EventTimelineSet,
      * can be omitted if room is specified.
+     * @param {Thread=} thread the thread to which this timeline set relates.
      */
     constructor(
         public readonly room: Room | undefined,
         opts: IOpts = {},
         client?: MatrixClient,
+        public readonly thread?: Thread,
     ) {
         super();
 
diff --git a/src/models/event.ts b/src/models/event.ts
@@ -537,7 +537,7 @@ export class MatrixEvent extends TypedEventEmitter<EmittedEvents, MatrixEventHan
         return mRelatesTo?.['m.in_reply_to']?.event_id;
     }
 
-    public get relationEventId(): string {
+    public get relationEventId(): string | undefined {
         return this.getWireContent()
             ?.["m.relates_to"]
             ?.event_id;
diff --git a/src/models/thread.ts b/src/models/thread.ts
@@ -90,7 +90,7 @@ export class Thread extends TypedEventEmitter<EmittedEvents, EventHandlerMap> {
         this.timelineSet = new EventTimelineSet(this.room, {
             timelineSupport: true,
             pendingEvents: true,
-        });
+        }, this.client, this);
         this.reEmitter = new TypedReEmitter(this);
 
         this.reEmitter.reEmit(this.timelineSet, [

Original file line number	Diff line number	Diff line change
`@@ -537,7 +537,7 @@ export class MatrixEvent extends TypedEventEmitter<EmittedEvents, MatrixEventHan`
`537`	`537`	`return mRelatesTo?.['m.in_reply_to']?.event_id;`
`538`	`538`	`}`
`539`	`539`
`540`		`- public get relationEventId(): string {`
	`540`	`+ public get relationEventId(): string \| undefined {`
`541`	`541`	`return this.getWireContent()`
`542`	`542`	`?.["m.relates_to"]`
`543`	`543`	`?.event_id;`