Fix `refreshLiveTimeline(...)` not working when the latest event in the room is a threaded message #2852

MadLittleMods · 2022-11-04T11:15:20Z

Fix refreshLiveTimeline(...) not working when the latest event in the room is a threaded message.

Follow-up to Timeline needs to refresh when we see a MSC2716 marker event #2299
matrix-react-sdk PR: Ask to refresh timeline when historical messages are imported (MSC2716) matrix-react-sdk#8354
This PR is a version 2 of Fix getLatestTimeline not working when the latest event in the room is a threaded message #2521

This works because fetchLatestLiveTimeline() will get an EventTimeline for the latest events in the room. It first calls /messages?dir=b to find the latest message, /context to get the proper pagination tokens, sets those pagination tokens on the timeline so it can paginate appropriately. It doesn't matter whether the latest event in the room is a thread because if it is, then we will just create a new EventTimeline in the EventTimelineSet, otherwise will get the existing timeline.

See matrix-org/matrix-react-sdk#8354 (comment)

We also have to keep in mind that we don't want to mix messages in the wrong timelines (main vs threaded timeline):

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
~~Sign-off given on the changes (see CONTRIBUTING.md)~~

Here's what your changelog entry will look like:

🐛 Bug Fixes

Fix refreshLiveTimeline(...) not working when the latest event in the room is a threaded message (#2852). Contributed by @MadLittleMods.

A v2 of #2521

MadLittleMods · 2022-11-04T11:18:06Z

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

@@ -1178,7 +1241,7 @@ describe("MatrixClient event timelines", function() {
                await flushHttp(client.getEventTimeline(timelineSet, THREAD_ROOT.event_id!));

                respondToMessagesRequest();
-                const timeline = await flushHttp(client.getLatestTimeline(timelineSet));


It looks like some non-refresh-timeline tests got infected with getLatestTimeline usage in the months while these PR's have sat.

But I don't really want to support getLatestTimeline for threads which these tests rely on.

Either we have to maintain the old getLatestTimeline(...) alongside getLatestLiveTimeline(...) forever or we can refactor these tests away from it.

Do we care about any external consumers that also may have started using it? Maybe should have marked it experimental to match the sole usage when it was introduced.

MadLittleMods · 2022-11-04T11:20:05Z

src/client.ts

+        const messagesPath = utils.encodeUri(
+            "/rooms/$roomId/messages", {
+                $roomId: timelineSet.room.roomId,
+            },
+        );
+        const messageRequestParams: Record<string, string | string[]> = {
+            dir: 'b',
+            // Since we only use the latest message in the response, we only need to
+            // fetch the one message here.
+            limit: "1",
+        };
+        if (this.clientOpts?.lazyLoadMembers) {
+            messageRequestParams.filter = JSON.stringify(Filter.LAZY_LOADING_MESSAGES_FILTER);
+        }
+        const messagesRes = await this.http.authedRequest<IMessagesResponse>(
+            Method.Get,
+            messagesPath,
+            messageRequestParams,
+        );
+        const latestEventInTimeline = messagesRes.chunk?.[0];
+        const latestEventIdInTimeline = latestEventInTimeline?.event_id;
+        if (!latestEventIdInTimeline) {
+            throw new Error("No message returned when trying to construct getLatestLiveTimeline");
+        }

-            const res = await this.http.authedRequest<IMessagesResponse>(Method.Get, messagesPath, params);
-            event = res.chunk?.[0];
+        const contextPath = utils.encodeUri(
+            "/rooms/$roomId/context/$eventId", {
+                $roomId: timelineSet.room.roomId,
+                $eventId: latestEventIdInTimeline,
+            },
+        );
+        let contextRequestParams: Record<string, string | string[]> | undefined = undefined;
+        if (this.clientOpts?.lazyLoadMembers) {
+            contextRequestParams = { filter: JSON.stringify(Filter.LAZY_LOADING_MESSAGES_FILTER) };
        }
-        if (!event) {
-            throw new Error("No message returned when trying to construct getLatestTimeline");
+        const contextRes = await this.http.authedRequest<IContextResponse>(
+            Method.Get,
+            contextPath,
+            contextRequestParams,
+        );
+        if (!contextRes.event || contextRes.event.event_id !== latestEventIdInTimeline) {
+            throw new Error(
+                `getLatestLiveTimeline: \`/context\` response did not include latestEventIdInTimeline=` +
+                `${latestEventIdInTimeline} which we were asking about. This is probably a bug in the ` +
+                `homeserver since we just saw the event with the other request above and now the server ` +
+                `claims it does not exist.`,
+            );
+        }
+
+        // By the time the request completes, the event might have ended up in the timeline.
+        const shortcutTimelineForEvent = timelineSet.getTimelineForEvent(latestEventIdInTimeline);
+        if (shortcutTimelineForEvent) {
+            return shortcutTimelineForEvent;
+        }
+
+        const mapper = this.getEventMapper();
+        const latestMatrixEventInTimeline = mapper(contextRes.event);
+        const events = [
+            // Order events from most recent to oldest (reverse-chronological).
+            // 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.
+            ...contextRes.events_after.reverse().map(mapper),
+            latestMatrixEventInTimeline,
+            ...contextRes.events_before.map(mapper),
+        ];
+
+        // This function handles non-thread timelines only, but we still process any
+        // thread events to populate thread summaries.
+        let timeline = timelineSet.getTimelineForEvent(events[0].getId()!);
+        if (timeline) {
+            timeline.getState(EventTimeline.BACKWARDS)!.setUnknownStateEvents(contextRes.state.map(mapper));
+        } else {
+            // If the `latestEventIdInTimeline` does not belong to this `timelineSet`
+            // then it will be ignored and not added to the `timelineSet`. We'll instead
+            // just create a new blank timeline in the `timelineSet` with the proper
+            // pagination tokens setup to continue paginating.
+            timeline = timelineSet.addTimeline();
+            timeline.initialiseState(contextRes.state.map(mapper));
+            timeline.getState(EventTimeline.FORWARDS)!.paginationToken = contextRes.end;
        }

-        return this.getEventTimeline(timelineSet, event.event_id);
+        const [timelineEvents, threadedEvents] = timelineSet.room.partitionThreadedEvents(events);
+        timelineSet.addEventsToTimeline(timelineEvents, true, timeline, contextRes.start);
+        // The target event is not in a thread but process the contextual events, so we can show any threads around it.
+        this.processThreadEvents(timelineSet.room, threadedEvents, true);
+        this.processBeaconEvents(timelineSet.room, timelineEvents);
+
+        return timelineSet.getTimelineForEvent(latestEventIdInTimeline) ?? timeline;


Opted for a new function as concluded in #2521 (comment)

This is very similar to the existing logic in getEventTimeline(...) but changed so that

// If the `latestEventIdInTimeline` does not belong to this `timelineSet` // then it will be ignored and not added to the `timelineSet`. We'll instead // just create a new blank timeline in the `timelineSet` with the proper // pagination tokens setup to continue paginating.

In getEventTimeline(...), it will return undefined which isn't useful when we want the new empty timeline. Any good way to de-dupe this logic or untangle the mess?

…msc2716-marker-events-v2

MadLittleMods · 2022-11-08T00:41:57Z

src/models/room.ts

        const mostRecentEventInTimeline = eventsBefore[eventsBefore.length - 1];
+        const mostRecentEventIdInTimeline = mostRecentEventInTimeline?.getId();


I am surprised that TypeScript didn't catch this before. I would have assumed that TypeScript would infer undefined as a possibility from an array lookup but I guess not.

Related https://stackoverflow.com/questions/59479171/typescript-does-not-infer-undefined-as-a-return-type-from-array-prototype-find which mentions that strictNullChecks needs to be enabled for it to infer undefined there.

Is strictNullChecks separate from strict? We added strict recently in #2835

Related to element-hq/element-web#21967 for adding strictNullChecks

MadLittleMods · 2022-11-08T01:15:19Z

src/client.ts

+            } else {
+                throw new Error(
+                    `fetchLatestLiveTimeline: While updating backwards state of the existing ` +
+                    ` timeline=${timeline.toString()}, it unexpectedly did not have any backwards ` +
+                    `state. Something probably broke with the timeline earlier for this to happen.`,
+                );
+            }


I assume SonarCloud is complaining that we don't have coverage for these two conditionals (this one and the one below like it).

These are just meant as catches for unexpected state and giving a better error message than the havoc the NPE and undefined behavior would cause the down the line. Seems better than assuming the reference is there with a ! which is what getEventTimeline is opting to use. I can also update it do a similar thing (just poke)

matrix-js-sdk/src/client.ts

Lines 5330 to 5338 in 0fbd0b3

// 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;

}

To be clear, I haven't run into these unexpected states before and I haven't looked into why a timeline wouldn't have these set. I'm guessing non-room timelines won't have them set.

Related to matrix-org/matrix-react-sdk#9558 (comment) about not stressing over coverage for these safety checks

See https://github.com/matrix-org/matrix-react-sdk/actions/runs/3416332427/jobs/5686363049#step:6:25

src/webrtc/call.ts

robintown

Clearing element-call-reviewers review

See #2852 (comment)

…msc2716-marker-events-v2

kerryarchibald

This PR duplicates a lot of code. I think it would be worth extracting and sharing the bulk of the getEventTimeline/fetchLatestLiveTimeline functions that is afaict identical

kerryarchibald · 2022-12-01T22:04:32Z

src/client.ts

+        const messagesPath = utils.encodeUri(
+            "/rooms/$roomId/messages", {
+                $roomId: timelineSet.room.roomId,
+            },
+        );
+        const messageRequestParams: Record<string, string | string[]> = {
+            dir: 'b',
+            // Since we only use the latest message in the response, we only need to
+            // fetch the one message here.
+            limit: "1",
+        };
+        if (this.clientOpts?.lazyLoadMembers) {
+            messageRequestParams.filter = JSON.stringify(Filter.LAZY_LOADING_MESSAGES_FILTER);
+        }
+        const messagesRes = await this.http.authedRequest<IMessagesResponse>(
+            Method.Get,
+            messagesPath,
+            messageRequestParams,
+        );


Can this reuse createMessagesRequest instead?

kerryarchibald · 2022-12-01T22:08:57Z

src/client.ts

+        const contextPath = utils.encodeUri(
+            "/rooms/$roomId/context/$eventId", {
+                $roomId: timelineSet.room.roomId,
+                $eventId: latestEventIdInTimeline,
+            },
+        );
+        let contextRequestParams: Record<string, string | string[]> | undefined = undefined;
+        if (this.clientOpts?.lazyLoadMembers) {
+            contextRequestParams = { filter: JSON.stringify(Filter.LAZY_LOADING_MESSAGES_FILTER) };
+        }
+        const contextRes = await this.http.authedRequest<IContextResponse>(
+            Method.Get,
+            contextPath,
+            contextRequestParams,
+        );


Can this be extracted into a function and reused in the other identical call sites?

kerryarchibald · 2022-12-01T22:20:26Z

src/webrtc/call.ts

@@ -411,10 +411,11 @@ export class MatrixCall extends TypedEventEmitter<CallEvent, CallEventHandlerMap
        this.invitee = opts.invitee;
        this.client = opts.client;

-        if (!this.client.deviceId) throw new Error("Client must have a device ID to start calls");
+        const deviceId = this.client.getDeviceId();


change seems unrelated? can you remove?

kerryarchibald · 2022-12-02T01:41:47Z

src/client.ts

+            },
+        );
+        const messageRequestParams: Record<string, string | string[]> = {
+            dir: 'b',


Direction.Backward

robintown · 2022-12-02T17:17:38Z

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

+            await client.stopClient();
+
+            const testClient = new TestClient(
+                userId,
+                "DEVICE",
+                accessToken,
+                undefined,
+                { timelineSupport: false },
+            );
+            client = testClient.client;
+            httpBackend = testClient.httpBackend;
+            await startClient(httpBackend, client);
+
+            const room = client.getRoom(roomId)!;
+            const timelineSet = room.getTimelineSets()[0]!;
+            await expect(client.fetchLatestLiveTimeline(timelineSet)).rejects.toBeTruthy();


I think there's a little room for deduplicating code in these first 3 tests

Add getLatestLiveTimeline

2451e10

A v2 of #2521

MadLittleMods commented Nov 4, 2022

View reviewed changes

MadLittleMods mentioned this pull request Nov 4, 2022

Fix getLatestTimeline not working when the latest event in the room is a threaded message #2521

Closed

3 tasks

MadLittleMods added 3 commits November 7, 2022 16:29

Merge branch 'develop' into madlittlemods/refresh-timeline-when-we-see-…

95fc9d7

…msc2716-marker-events-v2

Restore getLatestTimeline

6f1c821

Fix NPE bug when no events

ac3feca

MadLittleMods commented Nov 8, 2022

View reviewed changes

MadLittleMods added the T-Defect label Nov 8, 2022

MadLittleMods added 2 commits November 7, 2022 18:57

Fix lint

7dace31

Better error handling of an unexpected situation

32d3243

MadLittleMods commented Nov 8, 2022

View reviewed changes

MadLittleMods marked this pull request as ready for review November 8, 2022 02:55

MadLittleMods requested a review from a team as a code owner November 8, 2022 02:55

MadLittleMods requested review from t3chguy and kerryarchibald November 8, 2022 02:55

Fix Client must have a device ID to start calls in unrelated tests

6837f11

See https://github.com/matrix-org/matrix-react-sdk/actions/runs/3416332427/jobs/5686363049#step:6:25

MadLittleMods requested a review from a team as a code owner November 8, 2022 07:11

Log warning for weird situation

3670183

MadLittleMods commented Nov 8, 2022

View reviewed changes

src/webrtc/call.ts Outdated Show resolved Hide resolved

robintown reviewed Nov 8, 2022

View reviewed changes

MadLittleMods added 2 commits November 8, 2022 15:27

Use same reference so non-null inference can happen

4c49e68

See #2852 (comment)

Merge branch 'develop' into madlittlemods/refresh-timeline-when-we-see-…

02a439f

…msc2716-marker-events-v2

robintown self-requested a review December 1, 2022 17:26

kerryarchibald reviewed Dec 2, 2022

View reviewed changes

robintown reviewed Dec 2, 2022

View reviewed changes

MadLittleMods mentioned this pull request Dec 14, 2022

Ask to refresh timeline when historical messages are imported (MSC2716) matrix-org/matrix-react-sdk#8354

Closed

3 tasks

t3chguy removed their request for review April 27, 2023 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `refreshLiveTimeline(...)` not working when the latest event in the room is a threaded message #2852

Fix `refreshLiveTimeline(...)` not working when the latest event in the room is a threaded message #2852

MadLittleMods commented Nov 4, 2022 •

edited by github-actions bot

Loading

MadLittleMods Nov 4, 2022 •

edited

Loading

MadLittleMods Nov 4, 2022 •

edited

Loading

MadLittleMods Nov 8, 2022

MadLittleMods Nov 8, 2022 •

edited

Loading

MadLittleMods Nov 14, 2022 •

edited

Loading

robintown left a comment

kerryarchibald left a comment

kerryarchibald Dec 1, 2022

kerryarchibald Dec 1, 2022

kerryarchibald Dec 1, 2022

kerryarchibald Dec 2, 2022

robintown Dec 2, 2022

		const mostRecentEventInTimeline = eventsBefore[eventsBefore.length - 1];
		const mostRecentEventIdInTimeline = mostRecentEventInTimeline?.getId();

	// 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;
	}

Fix refreshLiveTimeline(...) not working when the latest event in the room is a threaded message #2852

Are you sure you want to change the base?

Fix refreshLiveTimeline(...) not working when the latest event in the room is a threaded message #2852

Conversation

MadLittleMods commented Nov 4, 2022 • edited by github-actions bot Loading

Checklist

🐛 Bug Fixes

MadLittleMods Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Nov 8, 2022

Choose a reason for hiding this comment

MadLittleMods Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

robintown left a comment

Choose a reason for hiding this comment

kerryarchibald left a comment

Choose a reason for hiding this comment

kerryarchibald Dec 1, 2022

Choose a reason for hiding this comment

kerryarchibald Dec 1, 2022

Choose a reason for hiding this comment

kerryarchibald Dec 1, 2022

Choose a reason for hiding this comment

kerryarchibald Dec 2, 2022

Choose a reason for hiding this comment

robintown Dec 2, 2022

Choose a reason for hiding this comment

Fix `refreshLiveTimeline(...)` not working when the latest event in the room is a threaded message #2852

Fix `refreshLiveTimeline(...)` not working when the latest event in the room is a threaded message #2852

MadLittleMods commented Nov 4, 2022 •

edited by github-actions bot

Loading

MadLittleMods Nov 4, 2022 •

edited

Loading

MadLittleMods Nov 4, 2022 •

edited

Loading

MadLittleMods Nov 8, 2022 •

edited

Loading

MadLittleMods Nov 14, 2022 •

edited

Loading