Skip to content

Commit

Permalink
Add events to the thread even if they appear to be out of order (#3787)
Browse files Browse the repository at this point in the history
  • Loading branch information
andybalaam authored Oct 17, 2023
1 parent 4ce837b commit 3baf6ec
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 16 deletions.
63 changes: 63 additions & 0 deletions spec/unit/models/thread.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,69 @@ describe("Thread", () => {
});
});
});

describe("addEvent", () => {
describe("Given server support for threads", () => {
let previousThreadHasServerSideSupport: FeatureSupport;

beforeAll(() => {
previousThreadHasServerSideSupport = Thread.hasServerSideSupport;
Thread.hasServerSideSupport = FeatureSupport.Stable;
});

afterAll(() => {
Thread.hasServerSideSupport = previousThreadHasServerSideSupport;
});

it("Adds events even if they appear out of order", async () => {
// Given a thread exists
const client = createClient();
const user = "@alice:matrix.org";
const room = "!room:z";
const thread = await createThread(client, user, room);
const prevNumEvents = thread.timeline.length;

// When two messages come in but the later one has an older timestamp
const message1 = createThreadMessage(thread.id, user, room, "message1");
const message2 = createThreadMessage(thread.id, user, room, "message2");
message2.localTimestamp -= 10000;

await thread.addEvent(message1, false);
await thread.addEvent(message2, false);

// Then both events end up in the timeline
expect(thread.timeline.length - prevNumEvents).toEqual(2);
const lastEvent = thread.timeline.at(-1)!;
const secondLastEvent = thread.timeline.at(-2)!;
expect(lastEvent).toBe(message2);
expect(secondLastEvent).toBe(message1);
});

it("Adds events to start even if they appear out of order", async () => {
// Given a thread exists
const client = createClient();
const user = "@alice:matrix.org";
const room = "!room:z";
const thread = await createThread(client, user, room);
const prevNumEvents = thread.timeline.length;

// When two messages come in but the later one has an older timestamp
const message1 = createThreadMessage(thread.id, user, room, "message1");
const message2 = createThreadMessage(thread.id, user, room, "message2");
message2.localTimestamp -= 10000;

await thread.addEvent(message1, false);
await thread.addEvent(message2, true);

// Then both events end up in the timeline
expect(thread.timeline.length - prevNumEvents).toEqual(2);
const lastEvent = thread.timeline.at(-1)!;
const firstEvent = thread.timeline.at(0)!;
expect(lastEvent).toBe(message1);
expect(firstEvent).toBe(message2);
});
});
});
});

/**
Expand Down
29 changes: 13 additions & 16 deletions src/models/thread.ts
Original file line number Diff line number Diff line change
Expand Up @@ -430,22 +430,19 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM
// If initial events have not been fetched, we are OK to throw away
// this event, because we are about to fetch all the events for this
// thread from the server.
//
// If not, this looks like a bug - we should always add the event to
// the thread. See https://github.com/vector-im/element-web/issues/26254
logger.warn(
`Not adding event ${event.getId()} to thread timeline!
isNewestReply=${isNewestReply}
event.localTimestamp=${event.localTimestamp}
!!lastReply=${!!lastReply}
lastReply?.getId()=${lastReply?.getId()}
lastReply?.localTimestamp=${lastReply?.localTimestamp}
toStartOfTimeline=${toStartOfTimeline}
Thread.hasServerSideSupport=${Thread.hasServerSideSupport}
event.isRelation(RelationType.Annotation)=${event.isRelation(RelationType.Annotation)}
event.isRelation(RelationType.Replace)=${event.isRelation(RelationType.Replace)}
`,
);

// Otherwise, we should add it, but we suspect it is out of order.
if (toStartOfTimeline) {
// If we're adding at the start of the timeline, it doesn't
// matter that it's out of order.
this.addEventToTimeline(event, toStartOfTimeline);
} else {
// We think this event might be out of order, because isNewestReply
// is false (otherwise we would have gone into the earlier if
// clause), so try to insert it in the right place based on
// timestamp.
this.insertEventIntoTimeline(event);
}
}

if (
Expand Down

0 comments on commit 3baf6ec

Please sign in to comment.