diff --git a/spec/unit/read-receipt.spec.ts b/spec/unit/read-receipt.spec.ts index 3ce0945c053..6f9aea92781 100644 --- a/spec/unit/read-receipt.spec.ts +++ b/spec/unit/read-receipt.spec.ts @@ -16,7 +16,7 @@ limitations under the License. import MockHttpBackend from "matrix-mock-request"; -import { MAIN_ROOM_TIMELINE, ReceiptType } from "../../src/@types/read_receipts"; +import { MAIN_ROOM_TIMELINE, ReceiptType, WrappedReceipt } from "../../src/@types/read_receipts"; import { MatrixClient } from "../../src/client"; import { EventType, MatrixEvent, Room } from "../../src/matrix"; import { synthesizeReceipt } from "../../src/models/read-receipt"; @@ -220,4 +220,42 @@ describe("Read receipt", () => { expect(content.thread_id).toEqual(destinationId); }); }); + + describe("addReceiptToStructure", () => { + it("should not allow an older unthreaded receipt to clobber a `main` threaded one", () => { + const userId = client.getSafeUserId(); + const room = new Room(ROOM_ID, client, userId); + + const unthreadedReceipt: WrappedReceipt = { + eventId: "$olderEvent", + data: { + ts: 1234567880, + }, + }; + const mainTimelineReceipt: WrappedReceipt = { + eventId: "$newerEvent", + data: { + ts: 1234567890, + }, + }; + + room.addReceiptToStructure( + mainTimelineReceipt.eventId, + ReceiptType.ReadPrivate, + userId, + mainTimelineReceipt.data, + false, + ); + expect(room.getEventReadUpTo(userId)).toBe(mainTimelineReceipt.eventId); + + room.addReceiptToStructure( + unthreadedReceipt.eventId, + ReceiptType.ReadPrivate, + userId, + unthreadedReceipt.data, + false, + ); + expect(room.getEventReadUpTo(userId)).toBe(mainTimelineReceipt.eventId); + }); + }); }); diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index a70e993ae63..b06c1bc021b 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -3112,10 +3112,10 @@ describe("Room", function () { it("should give precedence to m.read.private", () => { room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { if (receiptType === ReceiptType.ReadPrivate) { - return { eventId: "eventId1" } as WrappedReceipt; + return { eventId: "eventId1", data: { ts: 123 } }; } if (receiptType === ReceiptType.Read) { - return { eventId: "eventId2" } as WrappedReceipt; + return { eventId: "eventId2", data: { ts: 123 } }; } return null; }; diff --git a/src/models/read-receipt.ts b/src/models/read-receipt.ts index 7d30c8be95f..29eb1409a2c 100644 --- a/src/models/read-receipt.ts +++ b/src/models/read-receipt.ts @@ -57,9 +57,10 @@ export abstract class ReadReceipt< // which pass in an event ID and get back some receipts, so we also store // a pre-cached list for this purpose. // Map: receipt type → user Id → receipt - private receipts = new MapWithDefault>( - () => new Map(), - ); + private receipts = new MapWithDefault< + string, + Map + >(() => new Map()); private receiptCacheByEventId: ReceiptCache = new Map(); public abstract getUnfilteredTimelineSet(): EventTimelineSet; @@ -85,6 +86,13 @@ export abstract class ReadReceipt< return syntheticReceipt ?? realReceipt; } + private compareReceipts(a: WrappedReceipt, b: WrappedReceipt): number { + // Try compare them in our unfiltered timeline set order, falling back to receipt timestamp which should be + // relatively sane as receipts are set only by the originating homeserver so as long as its clock doesn't + // jump around then it should be valid. + return this.getUnfilteredTimelineSet().compareEventOrdering(a.eventId, b.eventId) ?? a.data.ts - b.data.ts; + } + /** * Get the ID of the event that a given user has read up to, or null if we * have received no read receipts from them. @@ -99,19 +107,13 @@ export abstract class ReadReceipt< // receipt type here again. IMHO this should be done by the server in // some more intelligent manner or the client should just use timestamps - const timelineSet = this.getUnfilteredTimelineSet(); const publicReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.Read); const privateReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.ReadPrivate); // If we have both, compare them let comparison: number | null | undefined; if (publicReadReceipt?.eventId && privateReadReceipt?.eventId) { - comparison = timelineSet.compareEventOrdering(publicReadReceipt?.eventId, privateReadReceipt?.eventId); - } - - // If we didn't get a comparison try to compare the ts of the receipts - if (!comparison && publicReadReceipt?.data?.ts && privateReadReceipt?.data?.ts) { - comparison = publicReadReceipt?.data?.ts - privateReadReceipt?.data?.ts; + comparison = this.compareReceipts(publicReadReceipt, privateReadReceipt); } // The public receipt is more likely to drift out of date so the private @@ -142,20 +144,20 @@ export abstract class ReadReceipt< existingReceipt = pair[ReceiptPairSyntheticIndex] ?? pair[ReceiptPairRealIndex]; } + const wrappedReceipt: WrappedReceipt = { + eventId, + data: receipt, + }; + if (existingReceipt) { - // we only want to add this receipt if we think it is later than the one we already have. + // We only want to add this receipt if we think it is later than the one we already have. // This is managed server-side, but because we synthesize RRs locally we have to do it here too. - const ordering = this.getUnfilteredTimelineSet().compareEventOrdering(existingReceipt.eventId, eventId); - if (ordering !== null && ordering >= 0) { + const ordering = this.compareReceipts(existingReceipt, wrappedReceipt); + if (ordering >= 0) { return; } } - const wrappedReceipt: WrappedReceipt = { - eventId, - data: receipt, - }; - const realReceipt = synthetic ? pair[ReceiptPairRealIndex] : wrappedReceipt; const syntheticReceipt = synthetic ? wrappedReceipt : pair[ReceiptPairSyntheticIndex];