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.

Sign up for GitHub

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

Jump to bottom

Fix messages loaded during initial fetch ending up out of order #2971

Merged
merged 2 commits into from
Dec 13, 2022
Merged

Fix messages loaded during initial fetch ending up out of order #2971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eea8e28
Fix messages loaded during initial fetch ending up out of order
justjanne Dec 13, 2022
b342883
Fix accidental commenting out of an assertion
justjanne Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 66 additions & 2 deletions spec/integ/matrix-client-event-timeline.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1249,6 +1249,7 @@ describe("MatrixClient event timelines", function () {
},
event: true,
});
THREAD_REPLY2.localTimestamp += 1000;

// Test data for the first thread, with the second reply
const THREAD_ROOT_UPDATED = {
Expand Down Expand Up @@ -1305,6 +1306,7 @@ describe("MatrixClient event timelines", function () {

// Test adding a second event to the first thread
const thread = room.getThread(THREAD_ROOT.event_id!)!;
thread.initialEventsFetched = true;
const prom = emitPromise(room, ThreadEvent.NewReply);
respondToEvent(THREAD_ROOT_UPDATED);
respondToEvent(THREAD_ROOT_UPDATED);
Expand Down Expand Up @@ -1624,10 +1626,35 @@ describe("MatrixClient event timelines", function () {
},
},
});
httpBackend
Copy link
Contributor

Choose a reason for hiding this comment

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

Please could you explain to me how these changes to the test check we are now behaving correctly?

I was expecting to see a test which failed before because events were out of order, but now passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with that is that the tests often omit certain parts of the way threads are loaded, to reduce complexity. Otherwise our tests would be much larger than the are. Ideally, each of these tests only tests the part of the functionality that it's concerned with, and mocks the rest.

But as this PR changes a minor part of the original functionality, the way tests mock the loading has to be adapted.

There's no single test that can easily test this issue yet, because the way we mock the loading can mask the issue itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

We talked over Matrix and agreed that @justjanne will work on a test that failed before and passes now.

The new test will either be part of this PR, or a new one as soon as possible afterwards.

.when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!))
.respond(200, function () {
return THREAD_ROOT;
});
httpBackend
.when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!))
.respond(200, function () {
return THREAD_ROOT;
});
httpBackend
.when(
"GET",
"/_matrix/client/v1/rooms/!foo%3Abar/relations/" +
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
buildRelationPaginationQuery({ dir: Direction.Backward, limit: 1 }),
)
.respond(200, function () {
return {
chunk: [THREAD_REPLY],
};
});
await Promise.all([httpBackend.flushAllExpected(), utils.syncPromise(client)]);

const room = client.getRoom(roomId)!;
const thread = room.getThread(THREAD_ROOT.event_id!)!;
expect(thread.initialEventsFetched).toBeTruthy();
const timelineSet = thread.timelineSet;

httpBackend
Expand All @@ -1645,20 +1672,57 @@ describe("MatrixClient event timelines", function () {
.respond(200, function () {
return THREAD_ROOT;
});
httpBackend
.when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!))
.respond(200, function () {
return THREAD_ROOT;
});
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(THREAD_ROOT.event_id!))
.respond(200, function () {
return {
start: "start_token",
events_before: [],
event: THREAD_ROOT,
events_after: [],
end: "end_token",
state: [],
};
});
httpBackend
.when(
"GET",
"/_matrix/client/v1/rooms/!foo%3Abar/relations/" +
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
buildRelationPaginationQuery({ dir: Direction.Backward, limit: 1 }),
buildRelationPaginationQuery({ dir: Direction.Backward, from: "start_token" }),
)
.respond(200, function () {
return {
chunk: [THREAD_ROOT],
chunk: [],
};
});
httpBackend
.when(
"GET",
"/_matrix/client/v1/rooms/!foo%3Abar/relations/" +
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
buildRelationPaginationQuery({ dir: Direction.Forward, from: "end_token" }),
)
.respond(200, function () {
return {
chunk: [THREAD_REPLY],
};
});

const timeline = await flushHttp(client.getEventTimeline(timelineSet, THREAD_ROOT.event_id!));

httpBackend.when("GET", "/sync").respond(200, {
Expand Down
39 changes: 34 additions & 5 deletions spec/unit/room.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import {
EventTimelineSet,
EventType,
IContent,
IEvent,
IRelationsRequestOpts,
IStateEventWithRoomId,
JoinRule,
MatrixEvent,
Expand Down Expand Up @@ -2489,11 +2491,24 @@ describe("Room", function () {
},
});

room.client.fetchRelations = (
roomId: string,
eventId: string,
relationType?: RelationType | string | null,
eventType?: EventType | string | null,
opts: IRelationsRequestOpts = { dir: Direction.Backward },
) =>
Promise.resolve({
chunk: [threadResponse.event] as IEvent[],
next_batch: "start_token",
});

let prom = emitPromise(room, ThreadEvent.New);
room.addLiveEvents([randomMessage, threadRoot, threadResponse]);
const thread: Thread = await prom;
await emitPromise(room, ThreadEvent.Update);

expect(thread.initialEventsFetched).toBeTruthy();
expect(thread.replyToEvent!.event).toEqual(threadResponse.event);
expect(thread.replyToEvent!.getContent().body).toBe(threadResponse.getContent().body);

Expand Down Expand Up @@ -2704,13 +2719,27 @@ describe("Room", function () {
},
});

room.client.fetchRelations = (
roomId: string,
eventId: string,
relationType?: RelationType | string | null,
eventType?: EventType | string | null,
opts: IRelationsRequestOpts = { dir: Direction.Backward },
) =>
Promise.resolve({
chunk: [threadResponse1.event] as IEvent[],
next_batch: "start_token",
});

let prom = emitPromise(room, ThreadEvent.New);
room.addLiveEvents([threadRoot, threadResponse1, threadResponse2]);
const thread = await prom;
room.addLiveEvents([threadRoot, threadResponse1]);
const thread: Thread = await prom;
await emitPromise(room, ThreadEvent.Update);

expect(thread.initialEventsFetched).toBeTruthy();
room.addLiveEvents([threadResponse2]);
expect(thread).toHaveLength(2);
expect(thread.replyToEvent.getId()).toBe(threadResponse2.getId());
expect(thread.replyToEvent!.getId()).toBe(threadResponse2.getId());

room.client.fetchRoomEvent = (eventId: string) =>
Promise.resolve({
Expand All @@ -2733,7 +2762,7 @@ describe("Room", function () {
await prom;
await emitPromise(room, ThreadEvent.Update);
expect(thread).toHaveLength(1);
expect(thread.replyToEvent.getId()).toBe(threadResponse1.getId());
expect(thread.replyToEvent!.getId()).toBe(threadResponse1.getId());

room.client.fetchRoomEvent = (eventId: string) =>
Promise.resolve({
Expand All @@ -2757,7 +2786,7 @@ describe("Room", function () {
await prom;
await prom2;
expect(thread).toHaveLength(0);
expect(thread.replyToEvent.getId()).toBe(threadRoot.getId());
expect(thread.replyToEvent!.getId()).toBe(threadRoot.getId());
});
});

Expand Down
2 changes: 1 addition & 1 deletion src/models/thread.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ export class Thread extends ReadReceipt<EmittedEvents, EventHandlerMap> {
this.addEventToTimeline(event, toStartOfTimeline);

this.client.decryptEventIfNeeded(event, {});
} else if (!toStartOfTimeline && isNewestReply) {
} else if (!toStartOfTimeline && this.initialEventsFetched && isNewestReply) {
this.addEventToTimeline(event, false);
this.fetchEditsWhereNeeded(event);
} else if (event.isRelation(RelationType.Annotation) || event.isRelation(RelationType.Replace)) {
Expand Down