Skip to content

Commit

Permalink
Fix #23916: Prevent edits of the last message in a thread getting lost (
Browse files Browse the repository at this point in the history 
#2951)

* Fix issue where the root event of a thread had to be loaded in a complicated way
* Fix issue where edits to the last event of a thread would get lost
* Fix issue where thread reply count would desync
* Refactor relations pagination mocking for tests
  • Loading branch information
@justjanne
justjanne authored Dec 12, 2022
1 parent 4c5f416 commit 8293011
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 126 deletions.
114 changes: 29 additions & 85 deletions spec/integ/matrix-client-event-timeline.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,11 @@ import {
MatrixEvent,
PendingEventOrdering,
Room,
RoomEvent,
} from "../../src/matrix";
import { logger } from "../../src/logger";
import { encodeUri } from "../../src/utils";
import { encodeParams, encodeUri, QueryDict, replaceParam } from "../../src/utils";
import { TestClient } from "../TestClient";
import { FeatureSupport, Thread, THREAD_RELATION_TYPE } from "../../src/models/thread";
import { FeatureSupport, Thread, THREAD_RELATION_TYPE, ThreadEvent } from "../../src/models/thread";
import { emitPromise } from "../test-utils/test-utils";

const userId = "@alice:localhost";
Expand All @@ -47,6 +46,18 @@ const withoutRoomId = (e: Partial<IEvent>): Partial<IEvent> => {
return copy;
};

/**
* Our httpBackend only allows matching calls if we have the exact same query, in the exact same order
* This method allows building queries with the exact same parameter order as the fetchRelations method in client
* @param params query parameters
*/
const buildRelationPaginationQuery = (params: QueryDict): string => {
if (Thread.hasServerSideFwdPaginationSupport === FeatureSupport.Experimental) {
params = replaceParam("dir", "org.matrix.msc3715.dir", params);
}
return "?" + encodeParams(params).toString();
};

const USER_MEMBERSHIP_EVENT = utils.mkMembership({
room: roomId,
mship: "join",
Expand Down Expand Up @@ -595,42 +606,6 @@ describe("MatrixClient event timelines", function () {
.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) +
"?dir=b&limit=1",
)
.respond(200, function () {
return {
original_event: THREAD_ROOT,
chunk: [THREAD_REPLY],
// no next batch as this is the oldest end of the timeline
};
});

const thread = room.createThread(THREAD_ROOT.event_id!, undefined, [], false);
await httpBackend.flushAllExpected();
const timelineSet = thread.timelineSet;

const timelinePromise = client.getEventTimeline(timelineSet, THREAD_REPLY.event_id!);
const timeline = await timelinePromise;

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 handle thread replies with server support by fetching a contiguous thread timeline", async () => {
// @ts-ignore
client.clientOpts.experimentalThreadSupport = true;
Thread.setServerSideSupport(FeatureSupport.Experimental);
await client.stopClient(); // we don't need the client to be syncing at this time
const room = client.getRoom(roomId)!;

httpBackend
.when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!))
.respond(200, function () {
Expand All @@ -644,7 +619,7 @@ describe("MatrixClient event timelines", function () {
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
"?dir=b&limit=1",
buildRelationPaginationQuery({ dir: Direction.Backward, limit: 1 }),
)
.respond(200, function () {
return {
Expand All @@ -656,15 +631,14 @@ describe("MatrixClient event timelines", function () {
const thread = room.createThread(THREAD_ROOT.event_id!, undefined, [], false);
await httpBackend.flushAllExpected();
const timelineSet = thread.timelineSet;

httpBackend
.when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!))
.respond(200, function () {
return THREAD_ROOT;
});
await flushHttp(emitPromise(thread, ThreadEvent.Update));

const timelinePromise = client.getEventTimeline(timelineSet, THREAD_REPLY.event_id!);
const [timeline] = await Promise.all([timelinePromise, httpBackend.flushAllExpected()]);
const timeline = await client.getEventTimeline(timelineSet, THREAD_REPLY.event_id!);

const eventIds = timeline!.getEvents().map((it) => it.getId());
expect(eventIds).toContain(THREAD_ROOT.event_id);
Expand Down Expand Up @@ -1273,7 +1247,7 @@ describe("MatrixClient event timelines", function () {
event_id: THREAD_ROOT.event_id,
},
},
event: false,
event: true,
});

// Test data for the first thread, with the second reply
Expand All @@ -1286,7 +1260,7 @@ describe("MatrixClient event timelines", function () {
"io.element.thread": {
...THREAD_ROOT.unsigned!["m.relations"]!["io.element.thread"],
count: 2,
latest_event: THREAD_REPLY2,
latest_event: THREAD_REPLY2.event,
},
},
},
Expand Down Expand Up @@ -1314,12 +1288,13 @@ describe("MatrixClient event timelines", function () {
respondToThreads(threadsResponse);
respondToThreads(threadsResponse);
respondToEvent(THREAD_ROOT);
respondToEvent(THREAD_ROOT);
respondToEvent(THREAD2_ROOT);
respondToEvent(THREAD2_ROOT);
respondToThread(THREAD_ROOT, [THREAD_REPLY]);
respondToThread(THREAD2_ROOT, [THREAD2_REPLY]);
await flushHttp(room.fetchRoomThreads());
const threadIds = room.getThreads().map((thread) => thread.id);
expect(threadIds).toContain(THREAD_ROOT.event_id);
expect(threadIds).toContain(THREAD2_ROOT.event_id);
const [allThreads] = timelineSets!;
const timeline = allThreads.getLiveTimeline()!;
// Test threads are in chronological order
Expand All @@ -1330,12 +1305,15 @@ describe("MatrixClient event timelines", function () {

// Test adding a second event to the first thread
const thread = room.getThread(THREAD_ROOT.event_id!)!;
const prom = emitPromise(allThreads!, RoomEvent.Timeline);
await thread.addEvent(client.getEventMapper()(THREAD_REPLY2), false);
const prom = emitPromise(room, ThreadEvent.NewReply);
respondToEvent(THREAD_ROOT_UPDATED);
respondToEvent(THREAD_ROOT_UPDATED);
respondToEvent(THREAD_ROOT_UPDATED);
respondToEvent(THREAD2_ROOT);
room.addLiveEvents([THREAD_REPLY2]);
await httpBackend.flushAllExpected();
await prom;
expect(thread.length).toBe(2);
// Test threads are in chronological order
expect(timeline!.getEvents().map((it) => it.event.event_id)).toEqual([
THREAD2_ROOT.event_id,
Expand Down Expand Up @@ -1652,24 +1630,6 @@ describe("MatrixClient event timelines", function () {
const thread = room.getThread(THREAD_ROOT.event_id!)!;
const timelineSet = thread.timelineSet;

const buildParams = (direction: Direction, token: string): string => {
if (Thread.hasServerSideFwdPaginationSupport === FeatureSupport.Experimental) {
return `?from=${token}&org.matrix.msc3715.dir=${direction}`;
} else {
return `?dir=${direction}&from=${token}`;
}
};

httpBackend
.when("GET", "/rooms/!foo%3Abar/context/" + encodeURIComponent(THREAD_ROOT.event_id!))
.respond(200, {
start: "start_token",
events_before: [],
event: THREAD_ROOT,
events_after: [],
state: [],
end: "end_token",
});
httpBackend
.when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!))
.respond(200, function () {
Expand All @@ -1692,27 +1652,11 @@ describe("MatrixClient event timelines", function () {
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
buildParams(Direction.Backward, "start_token"),
buildRelationPaginationQuery({ dir: Direction.Backward, limit: 1 }),
)
.respond(200, function () {
return {
original_event: THREAD_ROOT,
chunk: [],
};
});
httpBackend
.when(
"GET",
"/_matrix/client/v1/rooms/!foo%3Abar/relations/" +
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
buildParams(Direction.Forward, "end_token"),
)
.respond(200, function () {
return {
original_event: THREAD_ROOT,
chunk: [THREAD_REPLY],
chunk: [THREAD_ROOT],
};
});
const timeline = await flushHttp(client.getEventTimeline(timelineSet, THREAD_ROOT.event_id!));
Expand Down
3 changes: 0 additions & 3 deletions spec/test-utils/thread.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,6 @@ export const mkThread = ({
}

const thread = room.createThread(rootEvent.getId() ?? "", rootEvent, events, true);
// So that we do not have to mock the thread loading
thread.initialEventsFetched = true;
thread.addEvents(events, true);

return { thread, rootEvent, events };
};
17 changes: 9 additions & 8 deletions spec/unit/room.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2475,11 +2475,11 @@ describe("Room", function () {

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

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

room.client.fetchRoomEvent = (eventId: string) =>
Promise.resolve({
Expand All @@ -2490,7 +2490,7 @@ describe("Room", function () {
[THREAD_RELATION_TYPE.name]: {
latest_event: {
...threadResponse.event,
content: threadResponseEdit.event.content,
content: threadResponseEdit.getContent()["m.new_content"],
},
count: 2,
current_user_participated: true,
Expand All @@ -2502,7 +2502,7 @@ describe("Room", function () {
prom = emitPromise(room, ThreadEvent.Update);
room.addLiveEvents([threadResponseEdit]);
await prom;
expect(thread.replyToEvent.getContent().body).toBe(threadResponseEdit.getContent()["m.new_content"].body);
expect(thread.replyToEvent!.getContent().body).toBe(threadResponseEdit.getContent()["m.new_content"].body);
});

it("Redactions to thread responses decrement the length", async () => {
Expand Down Expand Up @@ -2847,7 +2847,7 @@ describe("Room", function () {
expect(room.eventShouldLiveIn(reply2, events, roots).shouldLiveInThread).toBeFalsy();
});

it("should aggregate relations in thread event timeline set", () => {
it("should aggregate relations in thread event timeline set", async () => {
Thread.setServerSideSupport(FeatureSupport.Stable);
const threadRoot = mkMessage();
const rootReaction = mkReaction(threadRoot);
Expand All @@ -2856,9 +2856,10 @@ describe("Room", function () {

const events = [threadRoot, rootReaction, threadResponse, threadReaction];

const prom = emitPromise(room, ThreadEvent.New);
room.addLiveEvents(events);

const thread = threadRoot.getThread()!;
const thread = await prom;
expect(thread).toBe(threadRoot.getThread());
expect(thread.rootEvent).toBe(threadRoot);

const rootRelations = thread.timelineSet.relations
Expand Down
4 changes: 1 addition & 3 deletions src/models/room.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2090,9 +2090,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
// If we jump to an event in a thread where neither the event, nor the root,
// nor any thread event are loaded yet, we'll load the event as well as the thread root, create the thread,
// and pass the event through this.
for (const event of events) {
thread.setEventMetadata(event);
}
thread.addEvents(events, false);

// If we managed to create a thread and figure out its `id` then we can use it
this.threads.set(thread.id, thread);
Expand Down
Loading

0 comments on commit 8293011

Please sign in to comment.