From 85b8d4f83ac135a304bf789db7931bcd5b26bcfc Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 29 Mar 2022 09:24:45 +0100 Subject: [PATCH] Fix issues with /search and /context API handling for threads (#2261) --- .../matrix-client-event-timeline.spec.js | 82 +++++++- spec/integ/matrix-client-methods.spec.js | 191 ++++++++++++++++-- src/client.ts | 118 ++++++----- src/models/event-context.ts | 2 +- src/models/room.ts | 48 +++-- src/models/search-result.ts | 13 +- src/models/thread.ts | 10 +- src/timeline-window.ts | 27 +-- 8 files changed, 371 insertions(+), 120 deletions(-) diff --git a/spec/integ/matrix-client-event-timeline.spec.js b/spec/integ/matrix-client-event-timeline.spec.js index 6499dad18..ad475f3fc 100644 --- a/spec/integ/matrix-client-event-timeline.spec.js +++ b/spec/integ/matrix-client-event-timeline.spec.js @@ -2,6 +2,7 @@ import * as utils from "../test-utils/test-utils"; import { EventTimeline } from "../../src/matrix"; import { logger } from "../../src/logger"; import { TestClient } from "../TestClient"; +import { Thread, THREAD_RELATION_TYPE } from "../../src/models/thread"; const userId = "@alice:localhost"; const userName = "Alice"; @@ -69,6 +70,27 @@ const EVENTS = [ }), ]; +const THREAD_ROOT = utils.mkMessage({ + room: roomId, + user: userId, + msg: "thread root", +}); + +const THREAD_REPLY = utils.mkEvent({ + room: roomId, + user: userId, + type: "m.room.message", + content: { + "body": "thread reply", + "msgtype": "m.text", + "m.relates_to": { + // We can't use the const here because we change server support mode for test + rel_type: "io.element.thread", + event_id: THREAD_ROOT.event_id, + }, + }, +}); + // start the client, and wait for it to initialise function startClient(httpBackend, client) { httpBackend.when("GET", "/versions").respond(200, {}); @@ -116,9 +138,7 @@ describe("getEventTimeline support", function() { return startClient(httpBackend, client).then(function() { const room = client.getRoom(roomId); const timelineSet = room.getTimelineSets()[0]; - expect(function() { - client.getEventTimeline(timelineSet, "event"); - }).toThrow(); + expect(client.getEventTimeline(timelineSet, "event")).rejects.toBeTruthy(); }); }); @@ -136,16 +156,12 @@ describe("getEventTimeline support", function() { return startClient(httpBackend, client).then(() => { const room = client.getRoom(roomId); const timelineSet = room.getTimelineSets()[0]; - expect(function() { - client.getEventTimeline(timelineSet, "event"); - }).not.toThrow(); + expect(client.getEventTimeline(timelineSet, "event")).rejects.toBeFalsy(); }); }); - it("scrollback should be able to scroll back to before a gappy /sync", - function() { + it("scrollback should be able to scroll back to before a gappy /sync", function() { // need a client with timelineSupport disabled to make this work - let room; return startClient(httpBackend, client).then(function() { @@ -229,6 +245,7 @@ describe("MatrixClient event timelines", function() { afterEach(function() { httpBackend.verifyNoOutstandingExpectation(); client.stopClient(); + Thread.setServerSideSupport(false); }); describe("getEventTimeline", function() { @@ -355,8 +372,7 @@ describe("MatrixClient event timelines", function() { ]); }); - it("should join timelines where they overlap a previous /context", - function() { + it("should join timelines where they overlap a previous /context", function() { const room = client.getRoom(roomId); const timelineSet = room.getTimelineSets()[0]; @@ -478,6 +494,50 @@ describe("MatrixClient event timelines", function() { httpBackend.flushAllExpected(), ]); }); + + it("should handle thread replies with server support by fetching a contiguous thread timeline", async () => { + 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: [], + }; + }); + + httpBackend.when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id)) + .respond(200, function() { + return THREAD_ROOT; + }); + + httpBackend.when("GET", "/rooms/!foo%3Abar/relations/" + + encodeURIComponent(THREAD_ROOT.event_id) + "/" + + encodeURIComponent(THREAD_RELATION_TYPE.name) + "?limit=20") + .respond(200, function() { + return { + original_event: THREAD_ROOT, + chunk: [THREAD_REPLY], + next_batch: "next_batch_token0", + prev_batch: "prev_batch_token0", + }; + }); + + const timelinePromise = client.getEventTimeline(timelineSet, THREAD_REPLY.event_id); + await httpBackend.flushAllExpected(); + + const timeline = await timelinePromise; + + expect(timeline.getEvents().find(e => e.getId() === THREAD_ROOT.event_id)); + expect(timeline.getEvents().find(e => e.getId() === THREAD_REPLY.event_id)); + }); }); describe("paginateEventTimeline", function() { diff --git a/spec/integ/matrix-client-methods.spec.js b/spec/integ/matrix-client-methods.spec.js index bdb36e1e9..0df2dafd1 100644 --- a/spec/integ/matrix-client-methods.spec.js +++ b/spec/integ/matrix-client-methods.spec.js @@ -3,6 +3,7 @@ import { CRYPTO_ENABLED } from "../../src/client"; import { MatrixEvent } from "../../src/models/event"; import { Filter, MemoryStore, Room } from "../../src/matrix"; import { TestClient } from "../TestClient"; +import { THREAD_RELATION_TYPE } from "../../src/models/thread"; describe("MatrixClient", function() { let client = null; @@ -14,9 +15,7 @@ describe("MatrixClient", function() { beforeEach(function() { store = new MemoryStore(); - const testClient = new TestClient(userId, "aliceDevice", accessToken, undefined, { - store: store, - }); + const testClient = new TestClient(userId, "aliceDevice", accessToken, undefined, { store }); httpBackend = testClient.httpBackend; client = testClient.client; }); @@ -244,14 +243,15 @@ describe("MatrixClient", function() { }); describe("searching", function() { - const response = { - search_categories: { - room_events: { - count: 24, - results: { - "$flibble:localhost": { + it("searchMessageText should perform a /search for room_events", function() { + const response = { + search_categories: { + room_events: { + count: 24, + results: [{ rank: 0.1, result: { + event_id: "$flibble:localhost", type: "m.room.message", user_id: "@alice:localhost", room_id: "!feuiwhf:localhost", @@ -260,13 +260,11 @@ describe("MatrixClient", function() { msgtype: "m.text", }, }, - }, + }], }, }, - }, - }; + }; - it("searchMessageText should perform a /search for room_events", function(done) { client.searchMessageText({ query: "monkeys", }); @@ -280,8 +278,171 @@ describe("MatrixClient", function() { }); }).respond(200, response); - httpBackend.flush().then(function() { - done(); + return httpBackend.flush(); + }); + + describe("should filter out context from different timelines (threads)", () => { + it("filters out thread replies when result is in the main timeline", async () => { + const response = { + search_categories: { + room_events: { + count: 24, + results: [{ + rank: 0.1, + result: { + event_id: "$flibble:localhost", + type: "m.room.message", + user_id: "@alice:localhost", + room_id: "!feuiwhf:localhost", + content: { + body: "main timeline", + msgtype: "m.text", + }, + }, + context: { + events_after: [{ + event_id: "$ev-after:server", + type: "m.room.message", + user_id: "@alice:localhost", + room_id: "!feuiwhf:localhost", + content: { + "body": "thread reply", + "msgtype": "m.text", + "m.relates_to": { + "event_id": "$some-thread:server", + "rel_type": THREAD_RELATION_TYPE.name, + }, + }, + }], + events_before: [{ + event_id: "$ev-before:server", + type: "m.room.message", + user_id: "@alice:localhost", + room_id: "!feuiwhf:localhost", + content: { + body: "main timeline again", + msgtype: "m.text", + }, + }], + }, + }], + }, + }, + }; + + const data = { + results: [], + highlights: [], + }; + client.processRoomEventsSearch(data, response); + + expect(data.results).toHaveLength(1); + expect(data.results[0].context.timeline).toHaveLength(2); + expect(data.results[0].context.timeline.find(e => e.getId() === "$ev-after:server")).toBeFalsy(); + }); + + it("filters out thread replies from threads other than the thread the result replied to", () => { + const response = { + search_categories: { + room_events: { + count: 24, + results: [{ + rank: 0.1, + result: { + event_id: "$flibble:localhost", + type: "m.room.message", + user_id: "@alice:localhost", + room_id: "!feuiwhf:localhost", + content: { + "body": "thread 1 reply 1", + "msgtype": "m.text", + "m.relates_to": { + "event_id": "$thread1:server", + "rel_type": THREAD_RELATION_TYPE.name, + }, + }, + }, + context: { + events_after: [{ + event_id: "$ev-after:server", + type: "m.room.message", + user_id: "@alice:localhost", + room_id: "!feuiwhf:localhost", + content: { + "body": "thread 2 reply 2", + "msgtype": "m.text", + "m.relates_to": { + "event_id": "$thread2:server", + "rel_type": THREAD_RELATION_TYPE.name, + }, + }, + }], + events_before: [], + }, + }], + }, + }, + }; + + const data = { + results: [], + highlights: [], + }; + client.processRoomEventsSearch(data, response); + + expect(data.results).toHaveLength(1); + expect(data.results[0].context.timeline).toHaveLength(1); + expect(data.results[0].context.timeline.find(e => e.getId() === "$flibble:localhost")).toBeTruthy(); + }); + + it("filters out main timeline events when result is a thread reply", () => { + const response = { + search_categories: { + room_events: { + count: 24, + results: [{ + rank: 0.1, + result: { + event_id: "$flibble:localhost", + type: "m.room.message", + user_id: "@alice:localhost", + room_id: "!feuiwhf:localhost", + content: { + "body": "thread 1 reply 1", + "msgtype": "m.text", + "m.relates_to": { + "event_id": "$thread1:server", + "rel_type": THREAD_RELATION_TYPE.name, + }, + }, + }, + context: { + events_after: [{ + event_id: "$ev-after:server", + type: "m.room.message", + user_id: "@alice:localhost", + room_id: "!feuiwhf:localhost", + content: { + "body": "main timeline", + "msgtype": "m.text", + }, + }], + events_before: [], + }, + }], + }, + }, + }; + + const data = { + results: [], + highlights: [], + }; + client.processRoomEventsSearch(data, response); + + expect(data.results).toHaveLength(1); + expect(data.results[0].context.timeline).toHaveLength(1); + expect(data.results[0].context.timeline.find(e => e.getId() === "$flibble:localhost")).toBeTruthy(); }); }); }); diff --git a/src/client.ts b/src/client.ts index 09d610b94..d1b6dff72 100644 --- a/src/client.ts +++ b/src/client.ts @@ -5231,19 +5231,17 @@ export class MatrixClient extends TypedEventEmitter { + public async getEventTimeline(timelineSet: EventTimelineSet, eventId: string): Promise { // don't allow any timeline support unless it's been enabled. if (!this.timelineSupport) { throw new Error("timeline support is disabled. Set the 'timelineSupport'" + - " parameter to true when creating MatrixClient to enable" + - " it."); + " parameter to true when creating MatrixClient to enable it."); } if (timelineSet.getTimelineForEvent(eventId)) { - return Promise.resolve(timelineSet.getTimelineForEvent(eventId)); + return timelineSet.getTimelineForEvent(eventId); } const path = utils.encodeUri( @@ -5253,56 +5251,82 @@ export class MatrixClient extends TypedEventEmitter = undefined; if (this.clientOpts.lazyLoadMembers) { params = { filter: JSON.stringify(Filter.LAZY_LOADING_MESSAGES_FILTER) }; } - // TODO: we should implement a backoff (as per scrollback()) to deal more - // nicely with HTTP errors. - const promise = this.http.authedRequest(undefined, Method.Get, path, params).then(async (res) => { // TODO types - if (!res.event) { - throw new Error("'event' not in '/context' result - homeserver too old?"); - } + // TODO: we should implement a backoff (as per scrollback()) to deal more nicely with HTTP errors. + const res = await this.http.authedRequest(undefined, Method.Get, path, params); // TODO types + if (!res.event) { + throw new Error("'event' not in '/context' result - homeserver too old?"); + } - // by the time the request completes, the event might have ended up in - // the timeline. - if (timelineSet.getTimelineForEvent(eventId)) { - return timelineSet.getTimelineForEvent(eventId); - } + // by the time the request completes, the event might have ended up in the timeline. + if (timelineSet.getTimelineForEvent(eventId)) { + return timelineSet.getTimelineForEvent(eventId); + } - // we start with the last event, since that's the point at which we - // have known state. + const mapper = this.getEventMapper(); + const event = mapper(res.event); + const events = [ + // we start with the last event, since that's the point at which we have known state. // events_after is already backwards; events_before is forwards. - res.events_after.reverse(); - const events = res.events_after - .concat([res.event]) - .concat(res.events_before); - const matrixEvents = events.map(this.getEventMapper()); + ...res.events_after.reverse().map(mapper), + event, + ...res.events_before.map(mapper), + ]; - let timeline = timelineSet.getTimelineForEvent(matrixEvents[0].getId()); - if (!timeline) { - timeline = timelineSet.addTimeline(); - timeline.initialiseState(res.state.map(this.getEventMapper())); - timeline.getState(EventTimeline.FORWARDS).paginationToken = res.end; - } else { - const stateEvents = res.state.map(this.getEventMapper()); - timeline.getState(EventTimeline.BACKWARDS).setUnknownStateEvents(stateEvents); + // 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 && event.isRelation(THREAD_RELATION_TYPE.name)) { + const [, threadedEvents] = this.partitionThreadedEvents(events); + const thread = await timelineSet.room.createThreadFetchRoot(event.threadRootId, threadedEvents, true); + + let nextBatch: string; + const response = await thread.fetchInitialEvents(); + if (response?.nextBatch) { + nextBatch = response.nextBatch; } - const [timelineEvents, threadedEvents] = this.partitionThreadedEvents(matrixEvents); + const opts: IRelationsRequestOpts = { + limit: 50, + }; - timelineSet.addEventsToTimeline(timelineEvents, true, timeline, res.start); - await this.processThreadEvents(timelineSet.room, threadedEvents, true); + // Fetch events until we find the one we were asked for + while (!thread.findEventById(eventId)) { + if (nextBatch) { + opts.from = nextBatch; + } - // there is no guarantee that the event ended up in "timeline" (we - // might have switched to a neighbouring timeline) - so check the - // room's index again. On the other hand, there's no guarantee the - // event ended up anywhere, if it was later redacted, so we just - // return the timeline we first thought of. - return timelineSet.getTimelineForEvent(eventId) || timeline; - }); - return promise; + ({ nextBatch } = await thread.fetchEvents(opts)); + } + + return thread.liveTimeline; + } + + // Here we handle non-thread timelines only, but still process any thread events to populate thread summaries. + let timeline = timelineSet.getTimelineForEvent(events[0].getId()); + if (timeline) { + timeline.getState(EventTimeline.BACKWARDS).setUnknownStateEvents(res.state.map(mapper)); + } else { + timeline = timelineSet.addTimeline(); + timeline.initialiseState(res.state.map(mapper)); + timeline.getState(EventTimeline.FORWARDS).paginationToken = res.end; + } + + const [timelineEvents, threadedEvents] = this.partitionThreadedEvents(events); + timelineSet.addEventsToTimeline(timelineEvents, true, timeline, res.start); + // The target event is not in a thread but process the contextual events, so we can show any threads around it. + await this.processThreadEvents(timelineSet.room, threadedEvents, true); + + // There is no guarantee that the event ended up in "timeline" (we might have switched to a neighbouring + // timeline) - so check the room's index again. On the other hand, there's no guarantee the event ended up + // anywhere, if it was later redacted, so we just return the timeline we first thought of. + return timelineSet.getTimelineForEvent(eventId) + ?? timelineSet.room.findThreadForEvent(event)?.liveTimeline // for Threads degraded support + ?? timeline; } /** @@ -6026,10 +6050,12 @@ export class MatrixClient extends TypedEventEmitter } } - /** - * Add an event to a thread's timeline. Will fire "Thread.update" - * @experimental - */ - public async addThreadedEvent(event: MatrixEvent, toStartOfTimeline: boolean): Promise { - this.applyRedaction(event); - let thread = this.findThreadForEvent(event); - if (thread) { - thread.addEvent(event, toStartOfTimeline); - } else { - const events = [event]; - let rootEvent = this.findEventById(event.threadRootId); - // If the rootEvent does not exist in the current sync, then look for it over the network. + public async createThreadFetchRoot( + threadId: string, + events?: MatrixEvent[], + toStartOfTimeline?: boolean, + ): Promise { + let thread = this.getThread(threadId); + + if (!thread) { + let rootEvent = this.findEventById(threadId); + // If the rootEvent does not exist in the local stores, then fetch it from the server. try { - let eventData: IMinimalEvent; - if (event.threadRootId) { - eventData = await this.client.fetchRoomEvent(this.roomId, event.threadRootId); - } + const eventData = await this.client.fetchRoomEvent(this.roomId, threadId); if (!rootEvent) { rootEvent = new MatrixEvent(eventData); @@ -1613,6 +1606,22 @@ export class Room extends TypedEventEmitter } } + return thread; + } + + /** + * Add an event to a thread's timeline. Will fire "Thread.update" + * @experimental + */ + public async addThreadedEvent(event: MatrixEvent, toStartOfTimeline: boolean): Promise { + this.applyRedaction(event); + let thread = this.findThreadForEvent(event); + if (thread) { + await thread.addEvent(event, toStartOfTimeline); + } else { + thread = await this.createThreadFetchRoot(event.threadRootId, [event], toStartOfTimeline); + } + this.emit(ThreadEvent.Update, thread); } @@ -1634,8 +1643,7 @@ export class Room extends TypedEventEmitter room: this, client: this.client, }); - // If we managed to create a thread and figure out its `id` - // then we can use it + // If we managed to create a thread and figure out its `id` then we can use it if (thread.id) { this.threads.set(thread.id, thread); this.reEmitter.reEmit(thread, [ diff --git a/src/models/search-result.ts b/src/models/search-result.ts index 1dc16ea84..99f9c5dda 100644 --- a/src/models/search-result.ts +++ b/src/models/search-result.ts @@ -33,14 +33,19 @@ export class SearchResult { public static fromJson(jsonObj: ISearchResult, eventMapper: EventMapper): SearchResult { const jsonContext = jsonObj.context || {} as IResultContext; - const eventsBefore = jsonContext.events_before || []; - const eventsAfter = jsonContext.events_after || []; + let eventsBefore = (jsonContext.events_before || []).map(eventMapper); + let eventsAfter = (jsonContext.events_after || []).map(eventMapper); const context = new EventContext(eventMapper(jsonObj.result)); + // Filter out any contextual events which do not correspond to the same timeline (thread or room) + const threadRootId = context.ourEvent.threadRootId; + eventsBefore = eventsBefore.filter(e => e.threadRootId === threadRootId); + eventsAfter = eventsAfter.filter(e => e.threadRootId === threadRootId); + context.setPaginateToken(jsonContext.start, true); - context.addEvents(eventsBefore.map(eventMapper), true); - context.addEvents(eventsAfter.map(eventMapper), false); + context.addEvents(eventsBefore, true); + context.addEvents(eventsAfter, false); context.setPaginateToken(jsonContext.end, false); return new SearchResult(jsonObj.rank, context); diff --git a/src/models/thread.ts b/src/models/thread.ts index 6ace74042..7879cc89f 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -165,12 +165,12 @@ export class Thread extends TypedEventEmitter { this.addEventToTimeline(event, toStartOfTimeline); await this.client.decryptEventIfNeeded(event, {}); - } else { + } else if (!toStartOfTimeline && + this.initialEventsFetched && + event.localTimestamp > this.lastReply().localTimestamp + ) { await this.fetchEditsWhereNeeded(event); - - if (this.initialEventsFetched && event.localTimestamp > this.lastReply().localTimestamp) { - this.addEventToTimeline(event, false); - } + this.addEventToTimeline(event, false); } if (!this._currentUserParticipated && event.getSender() === this.client.getUserId()) { diff --git a/src/timeline-window.ts b/src/timeline-window.ts index 936c910cf..24c95fbcf 100644 --- a/src/timeline-window.ts +++ b/src/timeline-window.ts @@ -99,11 +99,11 @@ export class TimelineWindow { * * @return {Promise} */ - public load(initialEventId?: string, initialWindowSize = 20): Promise { + public load(initialEventId?: string, initialWindowSize = 20): Promise { // given an EventTimeline, find the event we were looking for, and initialise our // fields so that the event in question is in the middle of the window. const initFields = (timeline: EventTimeline) => { - let eventIndex; + let eventIndex: number; const events = timeline.getEvents(); @@ -111,40 +111,31 @@ export class TimelineWindow { // we were looking for the live timeline: initialise to the end eventIndex = events.length; } else { - for (let i = 0; i < events.length; i++) { - if (events[i].getId() == initialEventId) { - eventIndex = i; - break; - } - } + eventIndex = events.findIndex(e => e.getId() === initialEventId); - if (eventIndex === undefined) { + if (eventIndex < 0) { throw new Error("getEventTimeline result didn't include requested event"); } } - const endIndex = Math.min(events.length, - eventIndex + Math.ceil(initialWindowSize / 2)); + const endIndex = Math.min(events.length, eventIndex + Math.ceil(initialWindowSize / 2)); const startIndex = Math.max(0, endIndex - initialWindowSize); this.start = new TimelineIndex(timeline, startIndex - timeline.getBaseIndex()); this.end = new TimelineIndex(timeline, endIndex - timeline.getBaseIndex()); this.eventCount = endIndex - startIndex; }; - // We avoid delaying the resolution of the promise by a reactor tick if - // we already have the data we need, which is important to keep room-switching - // feeling snappy. - // + // We avoid delaying the resolution of the promise by a reactor tick if we already have the data we need, + // which is important to keep room-switching feeling snappy. if (initialEventId) { const timeline = this.timelineSet.getTimelineForEvent(initialEventId); if (timeline) { // hot-path optimization to save a reactor tick by replicating the sync check getTimelineForEvent does. initFields(timeline); - return Promise.resolve(timeline); + return Promise.resolve(); } - const prom = this.client.getEventTimeline(this.timelineSet, initialEventId); - return prom.then(initFields); + return this.client.getEventTimeline(this.timelineSet, initialEventId).then(initFields); } else { const tl = this.timelineSet.getLiveTimeline(); initFields(tl);