From 7ed14dc74978f2fbd479524984ac498100c4721e Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 25 May 2023 14:59:14 +0100 Subject: [PATCH] Only add a local receipt if it's after an existing receipt (#3399) * Add a test for creating local echo receipts in threads * Only add local receipt if it's after existing receipt * Refactor local receipt tests to be shorter * Tests for local receipts where we DO have recursive relations support --- spec/unit/models/thread.spec.ts | 139 ++++++++++++++++++++++++++++++-- src/models/thread.ts | 24 +++++- 2 files changed, 156 insertions(+), 7 deletions(-) diff --git a/spec/unit/models/thread.spec.ts b/spec/unit/models/thread.spec.ts index cd0340cb199..9a36ca56163 100644 --- a/spec/unit/models/thread.spec.ts +++ b/spec/unit/models/thread.spec.ts @@ -19,7 +19,7 @@ import { mocked } from "jest-mock"; import { MatrixClient, PendingEventOrdering } from "../../../src/client"; import { Room, RoomEvent } from "../../../src/models/room"; import { Thread, THREAD_RELATION_TYPE, ThreadEvent, FeatureSupport } from "../../../src/models/thread"; -import { mkThread } from "../../test-utils/thread"; +import { makeThreadEvent, mkThread } from "../../test-utils/thread"; import { TestClient } from "../../TestClient"; import { emitPromise, mkEvent, mkMessage, mkReaction, mock } from "../../test-utils/test-utils"; import { Direction, EventStatus, EventType, MatrixEvent, RelationType } from "../../../src"; @@ -429,7 +429,7 @@ describe("Thread", () => { }); describe("insertEventIntoTimeline", () => { - it("Inserts a reply in timestamp order", () => { + it("Inserts a reaction in timestamp order", () => { // Assumption: no server side support because if we have it, events // can only be added to the timeline after the thread has been // initialised, and we are not properly initialising it here. @@ -449,11 +449,11 @@ describe("Thread", () => { ts: 100, // Events will be at ts 100, 101, 102, 103, 104 and 105 }); - // When we insert a reply to the second thread message + // When we insert a reaction to the second thread message const replyEvent = mkReaction(events[2], client, userId, room.roomId, 104); thread.insertEventIntoTimeline(replyEvent); - // Then the reply is inserted based on its timestamp + // Then the reaction is inserted based on its timestamp expect(thread.events.map((ev) => ev.getId())).toEqual([ events[0].getId(), events[1].getId(), @@ -465,10 +465,137 @@ describe("Thread", () => { ]); }); - function createClientWithEventMapper(): MatrixClient { + describe("Without relations recursion support", () => { + it("Creates a local echo receipt for new events", async () => { + // Assumption: no server side support because if we have it, events + // can only be added to the timeline after the thread has been + // initialised, and we are not properly initialising it here. + expect(Thread.hasServerSideSupport).toBe(FeatureSupport.None); + + // Given a client without relations recursion support + const client = createClientWithEventMapper(); + + // And a thread with an added event (with later timestamp) + const userId = "user1"; + const { thread, message } = await createThreadAndEvent(client, 1, 100, userId); + + // Then a receipt was added to the thread + const receipt = thread.getReadReceiptForUserId(userId); + expect(receipt).toBeTruthy(); + expect(receipt?.eventId).toEqual(message.getId()); + expect(receipt?.data.ts).toEqual(100); + expect(receipt?.data.thread_id).toEqual(thread.id); + + // (And the receipt was synthetic) + expect(thread.getReadReceiptForUserId(userId, true)).toBeNull(); + }); + + it("Doesn't create a local echo receipt for events before an existing receipt", async () => { + // Assumption: no server side support because if we have it, events + // can only be added to the timeline after the thread has been + // initialised, and we are not properly initialising it here. + expect(Thread.hasServerSideSupport).toBe(FeatureSupport.None); + + // Given a client without relations recursion support + const client = createClientWithEventMapper(); + + // And a thread with an added event with a lower timestamp than its other events + const userId = "user1"; + const { thread } = await createThreadAndEvent(client, 200, 100, userId); + + // Then no receipt was added to the thread (the receipt is still + // for the thread root). This happens because since we have no + // recursive relations support, we know that sometimes events + // appear out of order, so we have to check their timestamps as + // a guess of the correct order. + expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(thread.rootEvent?.getId()); + }); + }); + + describe("With relations recursion support", () => { + it("Creates a local echo receipt for new events", async () => { + // Assumption: no server side support because if we have it, events + // can only be added to the timeline after the thread has been + // initialised, and we are not properly initialising it here. + expect(Thread.hasServerSideSupport).toBe(FeatureSupport.None); + + // Given a client WITH relations recursion support + const client = createClientWithEventMapper( + new Map([[Feature.RelationsRecursion, ServerSupport.Stable]]), + ); + + // And a thread with an added event (with later timestamp) + const userId = "user1"; + const { thread, message } = await createThreadAndEvent(client, 1, 100, userId); + + // Then a receipt was added to the thread + const receipt = thread.getReadReceiptForUserId(userId); + expect(receipt?.eventId).toEqual(message.getId()); + }); + + it("Creates a local echo receipt even for events BEFORE an existing receipt", async () => { + // Assumption: no server side support because if we have it, events + // can only be added to the timeline after the thread has been + // initialised, and we are not properly initialising it here. + expect(Thread.hasServerSideSupport).toBe(FeatureSupport.None); + + // Given a client WITH relations recursion support + const client = createClientWithEventMapper( + new Map([[Feature.RelationsRecursion, ServerSupport.Stable]]), + ); + + // And a thread with an added event with a lower timestamp than its other events + const userId = "user1"; + const { thread, message } = await createThreadAndEvent(client, 200, 100, userId); + + // Then a receipt was added to the thread, because relations + // recursion is available, so we trust the server to have + // provided us with events in the right order. + const receipt = thread.getReadReceiptForUserId(userId); + expect(receipt?.eventId).toEqual(message.getId()); + }); + }); + + async function createThreadAndEvent( + client: MatrixClient, + rootTs: number, + eventTs: number, + userId: string, + ): Promise<{ thread: Thread; message: MatrixEvent }> { + const room = new Room("room1", client, userId); + + // Given a thread + const { thread } = mkThread({ + room, + client, + authorId: userId, + participantUserIds: [], + ts: rootTs, + }); + // Sanity: the current receipt is for the thread root + expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(thread.rootEvent?.getId()); + + const awaitTimelineEvent = new Promise((res) => thread.on(RoomEvent.Timeline, () => res())); + + // When we add a message that is before the latest receipt + const message = makeThreadEvent({ + event: true, + rootEventId: thread.id, + replyToEventId: thread.id, + user: userId, + room: room.roomId, + ts: eventTs, + }); + await thread.addEvent(message, false, true); + await awaitTimelineEvent; + + return { thread, message }; + } + + function createClientWithEventMapper(canSupport: Map = new Map()): MatrixClient { const client = mock(MatrixClient, "MatrixClient"); client.reEmitter = mock(ReEmitter, "ReEmitter"); - client.canSupport = new Map(); + client.canSupport = canSupport; jest.spyOn(client, "getEventMapper").mockReturnValue(eventMapperFor(client, {})); mocked(client.supportsThreads).mockReturnValue(true); return client; diff --git a/src/models/thread.ts b/src/models/thread.ts index 05e847594af..c44f01c9547 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -203,11 +203,33 @@ export class Thread extends ReadReceipt { ): void => { // Add a synthesized receipt when paginating forward in the timeline if (!toStartOfTimeline) { - room!.addLocalEchoReceipt(event.getSender()!, event, ReceiptType.Read); + const sender = event.getSender(); + if (sender && room && this.shouldSendLocalEchoReceipt(sender, event)) { + room.addLocalEchoReceipt(sender, event, ReceiptType.Read); + } } this.onEcho(event, toStartOfTimeline ?? false); }; + private shouldSendLocalEchoReceipt(sender: string, event: MatrixEvent): boolean { + const recursionSupport = this.client.canSupport.get(Feature.RelationsRecursion) ?? ServerSupport.Unsupported; + + if (recursionSupport === ServerSupport.Unsupported) { + // Normally we add a local receipt, but if we don't have + // recursion support, then events may arrive out of order, so we + // only create a receipt if it's after our existing receipt. + const oldReceiptEventId = this.getReadReceiptForUserId(sender)?.eventId; + if (oldReceiptEventId) { + const receiptEvent = this.findEventById(oldReceiptEventId); + if (receiptEvent && receiptEvent.getTs() > event.getTs()) { + return false; + } + } + } + + return true; + } + private onLocalEcho = (event: MatrixEvent): void => { this.onEcho(event, false); };