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

Add events to the thread even if they appear to be out of order #3787

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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