Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

194 changes: 194 additions & 0 deletions spec/integ/matrix-client-event-timeline.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,200 @@ describe("MatrixClient event timelines", function() {
});
});

describe("fetchLatestLiveTimeline", function() {
beforeEach(() => {
// @ts-ignore
client.clientOpts.experimentalThreadSupport = true;
});

it("timeline support must be enabled to work", async function() {
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();
Comment on lines +920 to +935
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

it("timeline support works when enabled", async function() {
await client.stopClient();

const testClient = new TestClient(
userId,
"DEVICE",
accessToken,
undefined,
{ timelineSupport: true },
);
client = testClient.client;
httpBackend = testClient.httpBackend;

return startClient(httpBackend, client).then(() => {
const room = client.getRoom(roomId)!;
const timelineSet = room.getTimelineSets()[0];
expect(client.fetchLatestLiveTimeline(timelineSet)).rejects.toBeFalsy();
});
});

it("only works with room timelines", async function() {
await client.stopClient();

const testClient = new TestClient(
userId,
"DEVICE",
accessToken,
undefined,
{ timelineSupport: true },
);
client = testClient.client;
httpBackend = testClient.httpBackend;
await startClient(httpBackend, client);

const timelineSet = new EventTimelineSet(undefined);
await expect(client.fetchLatestLiveTimeline(timelineSet)).rejects.toBeTruthy();
});

it("should create a new timeline for new events", function() {
const room = client.getRoom(roomId)!;
const timelineSet = room.getTimelineSets()[0];

const latestMessageId = EVENTS[2].event_id!;

httpBackend.when("GET", "/rooms/!foo%3Abar/messages")
.respond(200, function() {
return {
chunk: [{
event_id: latestMessageId,
}],
};
});

httpBackend.when("GET", `/rooms/!foo%3Abar/context/${encodeURIComponent(latestMessageId)}`)
.respond(200, function() {
return {
start: "start_token",
events_before: [EVENTS[1], EVENTS[0]],
event: EVENTS[2],
events_after: [EVENTS[3]],
state: [
ROOM_NAME_EVENT,
USER_MEMBERSHIP_EVENT,
],
end: "end_token",
};
});

return Promise.all([
client.fetchLatestLiveTimeline(timelineSet).then(function(tl) {
// Instead of this assertion logic, we could just add a spy
// for `getEventTimeline` and make sure it's called with the
// correct parameters. This doesn't feel too bad to make sure
// `fetchLatestLiveTimeline` is doing the right thing though.
expect(tl!.getEvents().length).toEqual(4);
for (let i = 0; i < 4; i++) {
expect(tl!.getEvents()[i].event).toEqual(EVENTS[i]);
expect(tl!.getEvents()[i]?.sender?.name).toEqual(userName);
}
expect(tl!.getPaginationToken(EventTimeline.BACKWARDS))
.toEqual("start_token");
expect(tl!.getPaginationToken(EventTimeline.FORWARDS))
.toEqual("end_token");
}),
httpBackend.flushAllExpected(),
]);
});

it("should successfully create a new timeline even when the latest event is a threaded reply", function() {
const room = client.getRoom(roomId);
const timelineSet = room!.getTimelineSets()[0];
expect(timelineSet.thread).toBeUndefined();

const latestMessageId = THREAD_REPLY.event_id;
if (!latestMessageId) {
throw new Error('Expected THREAD_REPLY to have an event_id to reference in the test');
}

httpBackend.when("GET", "/rooms/!foo%3Abar/messages")
.respond(200, function() {
return {
chunk: [{
event_id: latestMessageId,
}],
};
});

httpBackend.when("GET", `/rooms/!foo%3Abar/context/${encodeURIComponent(latestMessageId)}`)
.respond(200, function() {
return {
start: "start_token",
events_before: [THREAD_ROOT, EVENTS[0]],
event: THREAD_REPLY,
events_after: [],
state: [
ROOM_NAME_EVENT,
USER_MEMBERSHIP_EVENT,
],
end: "end_token",
};
});

// Make it easy to debug when there is a mismatch of events. We care
// about the event ID for direct comparison and the content for a
// human readable description.
const eventPropertiesToCompare = (event) => {
return {
eventId: event.event_id || event.getId(),
contentBody: event.content?.body || event.getContent()?.body,
};
};
return Promise.all([
client.fetchLatestLiveTimeline(timelineSet).then(function(tl) {
const events = tl!.getEvents();
const expectedEvents = [EVENTS[0], THREAD_ROOT];
expect(events.map(event => eventPropertiesToCompare(event)))
.toEqual(expectedEvents.map(event => eventPropertiesToCompare(event)));
// Sanity check: The threaded reply should not be in the timeline
expect(events.find(e => e.getId() === THREAD_REPLY.event_id)).toBeFalsy();

expect(tl!.getPaginationToken(EventTimeline.BACKWARDS))
.toEqual("start_token");
expect(tl!.getPaginationToken(EventTimeline.FORWARDS))
.toEqual("end_token");
}),
httpBackend.flushAllExpected(),
]);
});

it("should throw error when /messages does not return a message", () => {
const room = client.getRoom(roomId)!;
const timelineSet = room.getTimelineSets()[0];

httpBackend.when("GET", "/rooms/!foo%3Abar/messages")
.respond(200, () => {
return {
chunk: [
// No messages to return
],
};
});

return Promise.all([
expect(client.fetchLatestLiveTimeline(timelineSet)).rejects.toThrow(),
httpBackend.flushAllExpected(),
]);
});
});

describe("paginateEventTimeline", function() {
it("should allow you to paginate backwards", function() {
const room = client.getRoom(roomId)!;
Expand Down
4 changes: 2 additions & 2 deletions spec/integ/matrix-client-room-timeline.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -829,7 +829,7 @@ describe("MatrixClient room timelines", function() {
expect(room.timeline.length).toEqual(0);

// `/messages` request for `refreshLiveTimeline()` ->
// `getLatestTimeline()` to construct a new timeline from.
// `fetchLatestLiveTimeline()` to construct a new timeline from.
httpBackend!.when("GET", `/rooms/${encodeURIComponent(roomId)}/messages`)
.respond(200, function() {
return {
Expand All @@ -840,7 +840,7 @@ describe("MatrixClient room timelines", function() {
};
});
// `/context` request for `refreshLiveTimeline()` ->
// `getLatestTimeline()` -> `getEventTimeline()` to construct a new
// `fetchLatestLiveTimeline()` -> `getEventTimeline()` to construct a new
// timeline from.
httpBackend!.when("GET", contextUrl)
.respond(200, function() {
Expand Down
134 changes: 134 additions & 0 deletions src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5564,6 +5564,140 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
return this.getEventTimeline(timelineSet, event.event_id);
}

/**
* Get an EventTimeline for the latest events in the room. This will just
* call `/messages` to get the latest message in the room, then use
* `client.getEventTimeline(...)` to construct a new timeline from it.
* Always returns timeline in the given `timelineSet`.
*
* @param {EventTimelineSet} timelineSet The timelineSet to find or add the timeline to
*
* @return {Promise} Resolves:
* {@link module:models/event-timeline~EventTimeline} timeline with the latest events in the room
*/
public async fetchLatestLiveTimeline(timelineSet: EventTimelineSet): Promise<EventTimeline> {
// 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.");
}

if (!timelineSet.room) {
throw new Error("fetchLatestLiveTimeline only supports room timelines");
}

if (timelineSet.threadListType !== null || timelineSet.thread && Thread.hasServerSideSupport) {
throw new Error("fetchLatestLiveTimeline only supports live timelines");
}

const messagesPath = utils.encodeUri(
"/rooms/$roomId/messages", {
$roomId: timelineSet.room.roomId,
},
);
const messageRequestParams: Record<string, string | string[]> = {
dir: 'b',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direction.Backward

// 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,
);
Comment on lines +5598 to +5616
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this reuse createMessagesRequest instead?

const latestEventInTimeline = messagesRes.chunk?.[0];
const latestEventIdInTimeline = latestEventInTimeline?.event_id;
if (!latestEventIdInTimeline) {
throw new Error("No message returned when trying to construct fetchLatestLiveTimeline");
}

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,
);
Comment on lines +5623 to +5637
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!contextRes.event || contextRes.event.event_id !== latestEventIdInTimeline) {
throw new Error(
`fetchLatestLiveTimeline: \`/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) {
const backwardsState = timeline.getState(EventTimeline.BACKWARDS);
if (backwardsState) {
backwardsState.setUnknownStateEvents(contextRes.state.map(mapper));
} 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.`,
);
}
Comment on lines +5671 to +5677
Copy link
Contributor Author

@MadLittleMods MadLittleMods Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} 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));
const forwardsState = timeline.getState(EventTimeline.FORWARDS);
if (forwardsState) {
forwardsState.paginationToken = contextRes.end;
} else {
throw new Error(
`fetchLatestLiveTimeline: While updating forwards state of the new ` +
` timeline=${timeline.toString()}, it unexpectedly did not have any forwards ` +
`state. Something probably broke while creating the timeline for this to happen.`,
);
}
}

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

/**
* Makes a request to /messages with the appropriate lazy loading filter set.
* XXX: if we do get rid of scrollback (as it's not used at the moment),
Expand Down
Loading