From d1db4dbac8c046b7cb702b18e19c07c47ca54e20 Mon Sep 17 00:00:00 2001 From: Germain Date: Thu, 5 Jan 2023 09:25:29 +0000 Subject: [PATCH 01/24] Add failing test scenario when multiple receipts are in the same event --- spec/integ/matrix-client-syncing.spec.ts | 46 ++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/spec/integ/matrix-client-syncing.spec.ts b/spec/integ/matrix-client-syncing.spec.ts index 0f92dc60d95..75487eb81dd 100644 --- a/spec/integ/matrix-client-syncing.spec.ts +++ b/spec/integ/matrix-client-syncing.spec.ts @@ -1543,6 +1543,52 @@ describe("MatrixClient syncing", () => { }); }); }); + + it("only replays receipts relevant to the current context", async () => { + const THREAD_ID = "$unknownthread:localhost"; + + const receipt = { + type: "m.receipt", + room_id: "!foo:bar", + content: { + "$event1:localhost": { + [ReceiptType.Read]: { + "@alice:localhost": { ts: 666, thread_id: THREAD_ID }, + }, + }, + "$otherevent:localhost": { + [ReceiptType.Read]: { + "@alice:localhost": { ts: 999, thread_id: "$otherthread:localhost" }, + }, + }, + }, + }; + syncData.rooms.join[roomOne].ephemeral.events = [receipt]; + + httpBackend!.when("GET", "/sync").respond(200, syncData); + client!.startClient(); + + return Promise.all([httpBackend!.flushAllExpected(), awaitSyncEvent()]).then(() => { + const room = client?.getRoom(roomOne); + expect(room).toBeInstanceOf(Room); + + expect(room?.cachedThreadReadReceipts.has(THREAD_ID)).toBe(true); + + const thread = room!.createThread(THREAD_ID, undefined, [], true); + + expect(room?.cachedThreadReadReceipts.has(THREAD_ID)).toBe(false); + + const receipt = thread.getReadReceiptForUserId("@alice:localhost"); + + expect(receipt).toStrictEqual({ + data: { + thread_id: "$unknownthread:localhost", + ts: 666, + }, + eventId: "$event1:localhost", + }); + }); + }); }); describe("of a room", () => { From 03968c32dcc93b0375acabd54a3093570ab66a98 Mon Sep 17 00:00:00 2001 From: Germain Date: Wed, 4 Jan 2023 18:14:54 +0000 Subject: [PATCH 02/24] Fix cached read receipts --- src/@types/read_receipts.ts | 8 ++++++++ src/models/room.ts | 12 +++++++++--- src/models/thread.ts | 18 +++++------------- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/@types/read_receipts.ts b/src/@types/read_receipts.ts index 34f1c67fad7..3032c5934df 100644 --- a/src/@types/read_receipts.ts +++ b/src/@types/read_receipts.ts @@ -54,3 +54,11 @@ export type Receipts = { [userId: string]: [WrappedReceipt | null, WrappedReceipt | null]; // Pair (both nullable) }; }; + +export type CachedReceiptStructure = { + eventId: string; + receiptType: string | ReceiptType; + userId: string; + receipt: Receipt; + synthetic: boolean; +}; diff --git a/src/models/room.ts b/src/models/room.ts index 0faea02c371..15d54b3e0cd 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -54,7 +54,13 @@ import { FILTER_RELATED_BY_SENDERS, ThreadFilterType, } from "./thread"; -import { MAIN_ROOM_TIMELINE, Receipt, ReceiptContent, ReceiptType } from "../@types/read_receipts"; +import { + CachedReceiptStructure, + MAIN_ROOM_TIMELINE, + Receipt, + ReceiptContent, + ReceiptType, +} from "../@types/read_receipts"; import { IStateEventWithRoomId } from "../@types/search"; import { RelationsContainer } from "./relations-container"; import { ReadReceipt, synthesizeReceipt } from "./read-receipt"; @@ -302,7 +308,7 @@ export class Room extends ReadReceipt { private txnToEvent: Record = {}; // Pending in-flight requests { string: MatrixEvent } private notificationCounts: NotificationCount = {}; private readonly threadNotifications = new Map(); - public readonly cachedThreadReadReceipts = new Map(); + public readonly cachedThreadReadReceipts = new Map(); private readonly timelineSets: EventTimelineSet[]; public readonly threadsTimelineSets: EventTimelineSet[] = []; // any filtered timeline sets we're maintaining for this room @@ -2721,7 +2727,7 @@ export class Room extends ReadReceipt { // when the thread is created this.cachedThreadReadReceipts.set(receipt.thread_id!, [ ...(this.cachedThreadReadReceipts.get(receipt.thread_id!) ?? []), - { event, synthetic }, + { eventId, receiptType, userId, receipt, synthetic }, ]); } }); diff --git a/src/models/thread.ts b/src/models/thread.ts index da8ddf0b1f8..25e454c32e2 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -27,7 +27,7 @@ import { RoomState } from "./room-state"; import { ServerControlledNamespacedValue } from "../NamespacedValue"; import { logger } from "../logger"; import { ReadReceipt } from "./read-receipt"; -import { Receipt, ReceiptContent, ReceiptType } from "../@types/read_receipts"; +import { CachedReceiptStructure, ReceiptType } from "../@types/read_receipts"; export enum ThreadEvent { New = "Thread.new", @@ -50,7 +50,7 @@ interface IThreadOpts { room: Room; client: MatrixClient; pendingEventOrdering?: PendingEventOrdering; - receipts?: { event: MatrixEvent; synthetic: boolean }[]; + receipts?: CachedReceiptStructure[]; } export enum FeatureSupport { @@ -317,17 +317,9 @@ export class Thread extends ReadReceipt { * and apply them to the current thread * @param receipts - A collection of the receipts cached from initial sync */ - private processReceipts(receipts: { event: MatrixEvent; synthetic: boolean }[] = []): void { - for (const { event, synthetic } of receipts) { - const content = event.getContent(); - Object.keys(content).forEach((eventId: string) => { - Object.keys(content[eventId]).forEach((receiptType: ReceiptType | string) => { - Object.keys(content[eventId][receiptType]).forEach((userId: string) => { - const receipt = content[eventId][receiptType][userId] as Receipt; - this.addReceiptToStructure(eventId, receiptType as ReceiptType, userId, receipt, synthetic); - }); - }); - }); + private processReceipts(receipts: CachedReceiptStructure[] = []): void { + for (const { eventId, receiptType, userId, receipt, synthetic } of receipts) { + this.addReceiptToStructure(eventId, receiptType as ReceiptType, userId, receipt, synthetic); } } From a776ff8b1016dbb90add8f430dbeb49c9b1c2174 Mon Sep 17 00:00:00 2001 From: Germain Date: Thu, 5 Jan 2023 16:06:32 +0000 Subject: [PATCH 03/24] Improve hasUserReadEvent and getUserReadUpTo realibility with threads --- src/client.ts | 9 ++------- src/models/room.ts | 13 +++++++++++++ src/models/thread.ts | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/src/client.ts b/src/client.ts index ae42103f26e..22515a9d5b2 100644 --- a/src/client.ts +++ b/src/client.ts @@ -4840,13 +4840,8 @@ export class MatrixClient extends TypedEventEmitter(Method.Post, path, undefined, body || {}); diff --git a/src/models/room.ts b/src/models/room.ts index 15d54b3e0cd..ac6027535f7 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -309,6 +309,10 @@ export class Room extends ReadReceipt { private notificationCounts: NotificationCount = {}; private readonly threadNotifications = new Map(); public readonly cachedThreadReadReceipts = new Map(); + // Useful to know at what point the current user has started using thread in this room + public oldestThreadedReceiptTs = Infinity; + // Important to compute compute `hasUserReadEvent` and similar method correctly + public unthreadedReceipts = new Map(); private readonly timelineSets: EventTimelineSet[]; public readonly threadsTimelineSets: EventTimelineSet[] = []; // any filtered timeline sets we're maintaining for this room @@ -2730,6 +2734,15 @@ export class Room extends ReadReceipt { { eventId, receiptType, userId, receipt, synthetic }, ]); } + + const me = this.client.getSafeUserId(); + if (userId === me && !receiptForMainTimeline && receipt.ts < this.oldestThreadedReceiptTs) { + this.oldestThreadedReceiptTs = receipt.ts; + } + + if (!receipt.thread_id && receipt.ts > (this.unthreadedReceipts.get(userId)?.ts ?? 0)) { + this.unthreadedReceipts.set(userId, receipt); + } }); }); }); diff --git a/src/models/thread.ts b/src/models/thread.ts index 25e454c32e2..1b9988bc910 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -504,8 +504,42 @@ export class Thread extends ReadReceipt { throw new Error("Unsupported function on the thread model"); } + public getEventReadUpTo(userId: string, ignoreSynthesized?: boolean): string | null { + const isCurrentUser = userId === this.client.getUserId(); + if (isCurrentUser && this.lastReply()) { + // If a thread last activity is prior the first read receipt sent in a thread + // we want to consider that this thread has been read + const beforeFirstThreadedReceipt = (this.lastReply()?.getTs() ?? 0) < this.room.oldestThreadedReceiptTs; + if (beforeFirstThreadedReceipt) { + return this.timeline.at(-1)?.getId() ?? null; + } + } + + const readUpToId = super.getEventReadUpTo(userId, ignoreSynthesized); + + // Checking whether the unthreaded read receipt for that user is more recent + // than the read receipt inside that thread. + if (isCurrentUser && this.lastReply()) { + const unthreadedReceiptTs = this.room.unthreadedReceipts.get(userId)?.ts ?? Infinity; + for (let i = this.timeline?.length - 1; i >= 0; --i) { + const ev = this.timeline[i]; + if (ev.getTs() > unthreadedReceiptTs) return ev.getId() ?? readUpToId; + if (ev.getId() === readUpToId) return readUpToId; + } + } + + return readUpToId; + } + public hasUserReadEvent(userId: string, eventId: string): boolean { if (userId === this.client.getUserId()) { + const beforeFirstThreadedReceipt = (this.lastReply()?.getTs() ?? 0) < this.room.oldestThreadedReceiptTs; + const unthreadedReceiptTs = this.room.unthreadedReceipts.get(userId)?.ts ?? 0; + const beforeLastUnthreadedReceipt = (this?.lastReply()?.getTs() ?? 0) < unthreadedReceiptTs; + if (beforeFirstThreadedReceipt || beforeLastUnthreadedReceipt) { + return true; + } + const publicReadReceipt = this.getReadReceiptForUserId(userId, false, ReceiptType.Read); const privateReadReceipt = this.getReadReceiptForUserId(userId, false, ReceiptType.ReadPrivate); const hasUnreads = this.room.getThreadUnreadNotificationCount(this.id, NotificationCountType.Total) > 0; From 23cd62089edacf9f007b89dda1517858a53489fa Mon Sep 17 00:00:00 2001 From: Germain Date: Thu, 5 Jan 2023 17:24:50 +0000 Subject: [PATCH 04/24] Reword code comments and improve readibility Co-authored-by: Patrick Cloke --- src/models/room.ts | 6 ++++-- src/models/thread.ts | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/models/room.ts b/src/models/room.ts index ac6027535f7..227fc2ba328 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -309,9 +309,9 @@ export class Room extends ReadReceipt { private notificationCounts: NotificationCount = {}; private readonly threadNotifications = new Map(); public readonly cachedThreadReadReceipts = new Map(); - // Useful to know at what point the current user has started using thread in this room + // Useful to know at what point the current user has started using threads in this room public oldestThreadedReceiptTs = Infinity; - // Important to compute compute `hasUserReadEvent` and similar method correctly + // Important to compute `hasUserReadEvent` and similar methods correctly. public unthreadedReceipts = new Map(); private readonly timelineSets: EventTimelineSet[]; public readonly threadsTimelineSets: EventTimelineSet[] = []; @@ -2736,10 +2736,12 @@ export class Room extends ReadReceipt { } const me = this.client.getSafeUserId(); + // Track the time of the current user's oldest threaded receipt in the room. if (userId === me && !receiptForMainTimeline && receipt.ts < this.oldestThreadedReceiptTs) { this.oldestThreadedReceiptTs = receipt.ts; } + // Track each user's unthreaded read receipt. if (!receipt.thread_id && receipt.ts > (this.unthreadedReceipts.get(userId)?.ts ?? 0)) { this.unthreadedReceipts.set(userId, receipt); } diff --git a/src/models/thread.ts b/src/models/thread.ts index 1b9988bc910..3f1508b034c 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -507,8 +507,8 @@ export class Thread extends ReadReceipt { public getEventReadUpTo(userId: string, ignoreSynthesized?: boolean): string | null { const isCurrentUser = userId === this.client.getUserId(); if (isCurrentUser && this.lastReply()) { - // If a thread last activity is prior the first read receipt sent in a thread - // we want to consider that this thread has been read + // If the last activity in a thread is prior to the first threaded read receipt + // sent in the room we want to consider this thread as read. const beforeFirstThreadedReceipt = (this.lastReply()?.getTs() ?? 0) < this.room.oldestThreadedReceiptTs; if (beforeFirstThreadedReceipt) { return this.timeline.at(-1)?.getId() ?? null; From 1f3760ff2b8cdaa7b06660210b66cdcf357c33aa Mon Sep 17 00:00:00 2001 From: Germain Date: Thu, 5 Jan 2023 17:25:33 +0000 Subject: [PATCH 05/24] Optimise code paths Co-authored-by: Patrick Cloke --- src/models/thread.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/models/thread.ts b/src/models/thread.ts index 3f1508b034c..197243f357a 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -509,7 +509,7 @@ export class Thread extends ReadReceipt { if (isCurrentUser && this.lastReply()) { // If the last activity in a thread is prior to the first threaded read receipt // sent in the room we want to consider this thread as read. - const beforeFirstThreadedReceipt = (this.lastReply()?.getTs() ?? 0) < this.room.oldestThreadedReceiptTs; + const beforeFirstThreadedReceipt = (this.lastReply().getTs() ?? 0) < this.room.oldestThreadedReceiptTs; if (beforeFirstThreadedReceipt) { return this.timeline.at(-1)?.getId() ?? null; } @@ -520,7 +520,10 @@ export class Thread extends ReadReceipt { // Checking whether the unthreaded read receipt for that user is more recent // than the read receipt inside that thread. if (isCurrentUser && this.lastReply()) { - const unthreadedReceiptTs = this.room.unthreadedReceipts.get(userId)?.ts ?? Infinity; + if (!this.room.unthreadedReceipts.has(userId)) { + return readUpToId; + } + const unthreadedReceiptTs = this.room.unthreadedReceipts.get(userId).ts; for (let i = this.timeline?.length - 1; i >= 0; --i) { const ev = this.timeline[i]; if (ev.getTs() > unthreadedReceiptTs) return ev.getId() ?? readUpToId; From be7fb616dfaf3cbd865c5da8fe1577ebede2f5f9 Mon Sep 17 00:00:00 2001 From: Germain Date: Thu, 5 Jan 2023 17:32:39 +0000 Subject: [PATCH 06/24] fix getEventReadUpTo logic with unthreaded receipts --- src/models/thread.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/models/thread.ts b/src/models/thread.ts index 197243f357a..ffde9bffd96 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -523,11 +523,12 @@ export class Thread extends ReadReceipt { if (!this.room.unthreadedReceipts.has(userId)) { return readUpToId; } - const unthreadedReceiptTs = this.room.unthreadedReceipts.get(userId).ts; + + const unthreadedReceiptTs = this.room.unthreadedReceipts.get(userId)!.ts; for (let i = this.timeline?.length - 1; i >= 0; --i) { const ev = this.timeline[i]; - if (ev.getTs() > unthreadedReceiptTs) return ev.getId() ?? readUpToId; if (ev.getId() === readUpToId) return readUpToId; + if (ev.getTs() < unthreadedReceiptTs) return ev.getId() ?? readUpToId; } } From 0d4da28148acd1f97cd76dad95dd9e72b404fe42 Mon Sep 17 00:00:00 2001 From: Germain Date: Thu, 5 Jan 2023 17:34:36 +0000 Subject: [PATCH 07/24] Re-introduce optional chaining --- src/models/thread.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/models/thread.ts b/src/models/thread.ts index ffde9bffd96..eeccdc8abae 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -509,7 +509,7 @@ export class Thread extends ReadReceipt { if (isCurrentUser && this.lastReply()) { // If the last activity in a thread is prior to the first threaded read receipt // sent in the room we want to consider this thread as read. - const beforeFirstThreadedReceipt = (this.lastReply().getTs() ?? 0) < this.room.oldestThreadedReceiptTs; + const beforeFirstThreadedReceipt = (this.lastReply()?.getTs() ?? 0) < this.room.oldestThreadedReceiptTs; if (beforeFirstThreadedReceipt) { return this.timeline.at(-1)?.getId() ?? null; } @@ -519,7 +519,7 @@ export class Thread extends ReadReceipt { // Checking whether the unthreaded read receipt for that user is more recent // than the read receipt inside that thread. - if (isCurrentUser && this.lastReply()) { + if (this.lastReply()) { if (!this.room.unthreadedReceipts.has(userId)) { return readUpToId; } From 1ccf9544a9135a438069aa051fd397262dfa999b Mon Sep 17 00:00:00 2001 From: Germain Date: Thu, 5 Jan 2023 17:40:41 +0000 Subject: [PATCH 08/24] fixes --- src/client.ts | 9 +++++++-- src/models/room.ts | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/client.ts b/src/client.ts index 22515a9d5b2..ae42103f26e 100644 --- a/src/client.ts +++ b/src/client.ts @@ -4840,8 +4840,13 @@ export class MatrixClient extends TypedEventEmitter(Method.Post, path, undefined, body || {}); diff --git a/src/models/room.ts b/src/models/room.ts index 227fc2ba328..8567a4681da 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -2735,7 +2735,7 @@ export class Room extends ReadReceipt { ]); } - const me = this.client.getSafeUserId(); + const me = this.client.getUserId(); // Track the time of the current user's oldest threaded receipt in the room. if (userId === me && !receiptForMainTimeline && receipt.ts < this.oldestThreadedReceiptTs) { this.oldestThreadedReceiptTs = receipt.ts; From c24f027d443f11dab92abc2e11f40bc5e5795c73 Mon Sep 17 00:00:00 2001 From: Germain Date: Fri, 6 Jan 2023 09:31:15 +0000 Subject: [PATCH 09/24] mend --- spec/unit/notifications.spec.ts | 28 ++++++++++++++++++++++++++++ src/client.ts | 4 ++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/spec/unit/notifications.spec.ts b/spec/unit/notifications.spec.ts index 144afb70f12..e929b507d5c 100644 --- a/spec/unit/notifications.spec.ts +++ b/spec/unit/notifications.spec.ts @@ -14,6 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ +import { ReceiptType } from "../../src/@types/read_receipts"; import { Feature, ServerSupport } from "../../src/feature"; import { EventType, @@ -64,6 +65,30 @@ describe("fixNotificationCountOnDecryption", () => { }); room = new Room(ROOM_ID, mockClient, mockClient.getUserId() ?? ""); + + const receipt = new MatrixEvent({ + type: "m.receipt", + room_id: "!foo:bar", + content: { + "$event0:localhost": { + [ReceiptType.Read]: { + [mockClient.getUserId()!]: { ts: 123 }, + }, + }, + "$event1:localhost": { + [ReceiptType.Read]: { + [mockClient.getUserId()!]: { ts: 666, thread_id: THREAD_ID }, + }, + }, + "$otherevent:localhost": { + [ReceiptType.Read]: { + [mockClient.getUserId()!]: { ts: 999, thread_id: "$otherthread:localhost" }, + }, + }, + }, + }); + room.addReceipt(receipt); + room.setUnreadNotificationCount(NotificationCountType.Total, 1); room.setUnreadNotificationCount(NotificationCountType.Highlight, 0); @@ -75,6 +100,7 @@ describe("fixNotificationCountOnDecryption", () => { body: "Hello world!", }, event: true, + ts: 1234, }, mockClient, ); @@ -90,6 +116,7 @@ describe("fixNotificationCountOnDecryption", () => { "msgtype": MsgType.Text, "body": "Thread reply", }, + ts: 5678, event: true, }); room.createThread(THREAD_ID, event, [threadEvent], false); @@ -155,6 +182,7 @@ describe("fixNotificationCountOnDecryption", () => { "msgtype": MsgType.Text, "body": "Thread reply", }, + ts: 8901, event: true, }); diff --git a/src/client.ts b/src/client.ts index ae42103f26e..096335d8253 100644 --- a/src/client.ts +++ b/src/client.ts @@ -601,9 +601,9 @@ interface IJoinRequestBody { third_party_signed?: IThirdPartySigned; } -interface ITagMetadata { +export interface ITagMetadata { [key: string]: any; - order: number; + order?: number; } interface IMessagesResponse { From 435f4857aa683e30e4882656f2f6bb8dfb73888f Mon Sep 17 00:00:00 2001 From: Germain Date: Fri, 6 Jan 2023 10:09:08 +0000 Subject: [PATCH 10/24] Add tests for getEventReadUpTo and hasUserReadEvent --- spec/unit/models/thread.spec.ts | 187 ++++++++++++++++++++++++++++++-- spec/unit/notifications.spec.ts | 2 +- src/models/room.ts | 2 +- src/models/thread.ts | 16 +-- 4 files changed, 183 insertions(+), 24 deletions(-) diff --git a/spec/unit/models/thread.spec.ts b/spec/unit/models/thread.spec.ts index 42b976cd6ac..1525e3554bd 100644 --- a/spec/unit/models/thread.spec.ts +++ b/spec/unit/models/thread.spec.ts @@ -1,5 +1,5 @@ /* -Copyright 2022 The Matrix.org Foundation C.I.C. +Copyright 2022 - 2023 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -19,8 +19,12 @@ import { Room } from "../../../src/models/room"; import { Thread, THREAD_RELATION_TYPE, ThreadEvent } from "../../../src/models/thread"; import { mkThread } from "../../test-utils/thread"; import { TestClient } from "../../TestClient"; -import { emitPromise, mkMessage } from "../../test-utils/test-utils"; -import { EventStatus } from "../../../src"; +import { emitPromise, mkMessage, mock } from "../../test-utils/test-utils"; +import { EventStatus, MatrixEvent } from "../../../src"; +import { ReceiptType } from "../../../src/@types/read_receipts"; +import { getMockClientWithEventEmitter, mockClientMethodsUser } from "../../test-utils/client"; +import { ReEmitter } from "../../../src/ReEmitter"; +import { Feature, ServerSupport } from "../../../src/feature"; describe("Thread", () => { describe("constructor", () => { @@ -71,17 +75,54 @@ describe("Thread", () => { }); describe("hasUserReadEvent", () => { - const myUserId = "@bob:example.org"; + let myUserId: string; let client: MatrixClient; let room: Room; beforeEach(() => { - const testClient = new TestClient(myUserId, "DEVICE", "ACCESS_TOKEN", undefined, { - timelineSupport: false, + client = getMockClientWithEventEmitter({ + ...mockClientMethodsUser(), + getRoom: jest.fn().mockImplementation(() => room), + decryptEventIfNeeded: jest.fn().mockResolvedValue(void 0), + supportsExperimentalThreads: jest.fn().mockReturnValue(true), + }); + client.reEmitter = mock(ReEmitter, "ReEmitter"); + client.canSupport = new Map(); + Object.keys(Feature).forEach((feature) => { + client.canSupport.set(feature as Feature, ServerSupport.Stable); }); - client = testClient.client; + + myUserId = client.getUserId()!; + room = new Room("123", client, myUserId); + const receipt = new MatrixEvent({ + type: "m.receipt", + room_id: "!foo:bar", + content: { + // first threaded receipt + "$event0:localhost": { + [ReceiptType.Read]: { + [client.getUserId()!]: { ts: 100, thread_id: "$threadId:localhost" }, + }, + }, + // last unthreaded receipt + "$event1:localhost": { + [ReceiptType.Read]: { + [client.getUserId()!]: { ts: 200 }, + ["@alice:example.org"]: { ts: 200 }, + }, + }, + // last threaded receipt + "$event2:localhost": { + [ReceiptType.Read]: { + [client.getUserId()!]: { ts: 300 }, + }, + }, + }, + }); + room.addReceipt(receipt); + jest.spyOn(client, "getRoom").mockReturnValue(room); }); @@ -96,21 +137,151 @@ describe("Thread", () => { authorId: myUserId, participantUserIds: [myUserId], length: 2, + ts: 201, }); expect(thread.hasUserReadEvent(myUserId, events.at(-1)!.getId() ?? "")).toBeTruthy(); }); it("considers other events with no RR as unread", () => { + const { thread, events } = mkThread({ + room, + client, + authorId: myUserId, + participantUserIds: [myUserId], + length: 25, + ts: 190, + }); + + // Before alice's last unthreaded receipt + expect(thread.hasUserReadEvent("@alice:example.org", events.at(1)!.getId() ?? "")).toBeTruthy(); + + // After alice's last unthreaded receipt + expect(thread.hasUserReadEvent("@alice:example.org", events.at(-1)!.getId() ?? "")).toBeFalsy(); + }); + + it("considers event as read if there's a more recent unthreaded receipt", () => { const { thread, events } = mkThread({ room, client, authorId: myUserId, participantUserIds: ["@alice:example.org"], length: 2, + ts: 150, // before the latest unthreaded receipt }); + expect(thread.hasUserReadEvent(client.getUserId()!, events.at(-1)!.getId() ?? "")).toBe(true); + }); - expect(thread.hasUserReadEvent("@alice:example.org", events.at(-1)!.getId() ?? "")).toBeFalsy(); + it("considers event as unread if there's no more recent unthreaded receipt", () => { + const { thread, events } = mkThread({ + room, + client, + authorId: myUserId, + participantUserIds: ["@alice:example.org"], + length: 2, + ts: 1000, + }); + expect(thread.hasUserReadEvent(client.getUserId()!, events.at(-1)!.getId() ?? "")).toBe(false); + }); + }); + + describe("getEventReadUpTo", () => { + let myUserId: string; + let client: MatrixClient; + let room: Room; + + beforeEach(() => { + client = getMockClientWithEventEmitter({ + ...mockClientMethodsUser(), + getRoom: jest.fn().mockImplementation(() => room), + decryptEventIfNeeded: jest.fn().mockResolvedValue(void 0), + supportsExperimentalThreads: jest.fn().mockReturnValue(true), + }); + client.reEmitter = mock(ReEmitter, "ReEmitter"); + client.canSupport = new Map(); + Object.keys(Feature).forEach((feature) => { + client.canSupport.set(feature as Feature, ServerSupport.Stable); + }); + + myUserId = client.getUserId()!; + + room = new Room("123", client, myUserId); + + jest.spyOn(client, "getRoom").mockReturnValue(room); + }); + + afterAll(() => { + jest.resetAllMocks(); + }); + + it("uses unthreaded receipt to figure out read up to", () => { + const receipt = new MatrixEvent({ + type: "m.receipt", + room_id: "!foo:bar", + content: { + // last unthreaded receipt + "$event1:localhost": { + [ReceiptType.Read]: { + ["@alice:example.org"]: { ts: 200 }, + }, + }, + }, + }); + room.addReceipt(receipt); + + const { thread, events } = mkThread({ + room, + client, + authorId: myUserId, + participantUserIds: [myUserId], + length: 25, + ts: 190, + }); + + // The 10th event has been read, as alice's last unthreaded receipt is at ts 200 + // and `mkThread` increment every thread response by 1ms. + expect(thread.getEventReadUpTo("@alice:example.org")).toBe(events.at(9)!.getId()); + }); + + it("considers thread created before the first threaded receipt to be read", () => { + const receipt = new MatrixEvent({ + type: "m.receipt", + room_id: "!foo:bar", + content: { + // last unthreaded receipt + "$event1:localhost": { + [ReceiptType.Read]: { + [myUserId]: { ts: 200, thread_id: "$threadId" }, + }, + }, + }, + }); + room.addReceipt(receipt); + + const { thread, events } = mkThread({ + room, + client, + authorId: "@alice:example.org", + participantUserIds: ["@alice:example.org"], + length: 2, + ts: 10, + }); + + // The 10th event has been read, as alice's last unthreaded receipt is at ts 200 + // and `mkThread` increment every thread response by 1ms. + expect(thread.getEventReadUpTo(myUserId)).toBe(events.at(-1)!.getId()); + + const { thread: thread2 } = mkThread({ + room, + client, + authorId: "@alice:example.org", + participantUserIds: ["@alice:example.org"], + length: 2, + ts: 1000, + }); + + // Nothing has been read, this thread is after the first threaded receipt... + expect(thread2.getEventReadUpTo(myUserId)).toBe(null); }); }); }); diff --git a/spec/unit/notifications.spec.ts b/spec/unit/notifications.spec.ts index e929b507d5c..594740e285a 100644 --- a/spec/unit/notifications.spec.ts +++ b/spec/unit/notifications.spec.ts @@ -1,5 +1,5 @@ /* -Copyright 2022 The Matrix.org Foundation C.I.C. +Copyright 2022 - 2023 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/src/models/room.ts b/src/models/room.ts index 8567a4681da..7882e3a0dbf 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -1,5 +1,5 @@ /* -Copyright 2015 - 2022 The Matrix.org Foundation C.I.C. +Copyright 2015 - 2023 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/src/models/thread.ts b/src/models/thread.ts index eeccdc8abae..8f5c9dc16a4 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -1,5 +1,5 @@ /* -Copyright 2021-2022 The Matrix.org Foundation C.I.C. +Copyright 2021 - 2023 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -22,7 +22,7 @@ import { RelationType } from "../@types/event"; import { IThreadBundledRelationship, MatrixEvent, MatrixEventEvent } from "./event"; import { Direction, EventTimeline } from "./event-timeline"; import { EventTimelineSet, EventTimelineSetHandlerMap } from "./event-timeline-set"; -import { NotificationCountType, Room, RoomEvent } from "./room"; +import { Room, RoomEvent } from "./room"; import { RoomState } from "./room-state"; import { ServerControlledNamespacedValue } from "../NamespacedValue"; import { logger } from "../logger"; @@ -543,18 +543,6 @@ export class Thread extends ReadReceipt { if (beforeFirstThreadedReceipt || beforeLastUnthreadedReceipt) { return true; } - - const publicReadReceipt = this.getReadReceiptForUserId(userId, false, ReceiptType.Read); - const privateReadReceipt = this.getReadReceiptForUserId(userId, false, ReceiptType.ReadPrivate); - const hasUnreads = this.room.getThreadUnreadNotificationCount(this.id, NotificationCountType.Total) > 0; - - if (!publicReadReceipt && !privateReadReceipt && !hasUnreads) { - // Consider an event read if it's part of a thread that has no - // read receipts and has no notifications. It is likely that it is - // part of a thread that was created before read receipts for threads - // were supported (via MSC3771) - return true; - } } return super.hasUserReadEvent(userId, eventId); From 2f8328d32bf5f61698272c054911d980e5d29124 Mon Sep 17 00:00:00 2001 From: Germain Date: Fri, 6 Jan 2023 13:20:34 +0000 Subject: [PATCH 11/24] Reword code comments and improve readibility Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/models/thread.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/models/thread.ts b/src/models/thread.ts index 8f5c9dc16a4..4aeec246eaa 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -508,7 +508,9 @@ export class Thread extends ReadReceipt { const isCurrentUser = userId === this.client.getUserId(); if (isCurrentUser && this.lastReply()) { // If the last activity in a thread is prior to the first threaded read receipt - // sent in the room we want to consider this thread as read. + // sent in the room (suggesting that it was sent before the user started + // using a client that supported threaded read receipts), we want to + // consider this thread as read. const beforeFirstThreadedReceipt = (this.lastReply()?.getTs() ?? 0) < this.room.oldestThreadedReceiptTs; if (beforeFirstThreadedReceipt) { return this.timeline.at(-1)?.getId() ?? null; @@ -517,7 +519,7 @@ export class Thread extends ReadReceipt { const readUpToId = super.getEventReadUpTo(userId, ignoreSynthesized); - // Checking whether the unthreaded read receipt for that user is more recent + // Check whether the unthreaded read receipt for that user is more recent // than the read receipt inside that thread. if (this.lastReply()) { if (!this.room.unthreadedReceipts.has(userId)) { From e6348079bc8062a686f2bd769a86540c1ec043cc Mon Sep 17 00:00:00 2001 From: Germain Date: Fri, 6 Jan 2023 13:36:24 +0000 Subject: [PATCH 12/24] Add comments to methods --- src/models/thread.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/models/thread.ts b/src/models/thread.ts index 4aeec246eaa..3bce91454da 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -504,6 +504,15 @@ export class Thread extends ReadReceipt { throw new Error("Unsupported function on the thread model"); } + /** + * 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. + * @param userId - The user ID to get read receipt event ID for + * @param ignoreSynthesized - If true, return only receipts that have been + * sent by the server, not implicit ones generated + * by the JS SDK. + * @returns ID of the latest event that the given user has read, or null. + */ public getEventReadUpTo(userId: string, ignoreSynthesized?: boolean): string | null { const isCurrentUser = userId === this.client.getUserId(); if (isCurrentUser && this.lastReply()) { @@ -537,6 +546,14 @@ export class Thread extends ReadReceipt { return readUpToId; } + /** + * Determines if the given user has read a particular event ID with the known + * history of the room. This is not a definitive check as it relies only on + * what is available to the thread at the time of execution. + * @param userId - The user ID to check the read state of. + * @param eventId - The event ID to check if the user read. + * @returns True if the user has read the event, false otherwise. + */ public hasUserReadEvent(userId: string, eventId: string): boolean { if (userId === this.client.getUserId()) { const beforeFirstThreadedReceipt = (this.lastReply()?.getTs() ?? 0) < this.room.oldestThreadedReceiptTs; From 2d147d43064eb6d84aeed9067ac35395680edee8 Mon Sep 17 00:00:00 2001 From: Germain Date: Fri, 6 Jan 2023 13:56:57 +0000 Subject: [PATCH 13/24] Make properties private and provide accessors --- spec/unit/models/thread.spec.ts | 3 +-- src/models/room.ts | 27 ++++++++++++++++++++++++--- src/models/thread.ts | 16 +++++++++------- 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/spec/unit/models/thread.spec.ts b/spec/unit/models/thread.spec.ts index 1525e3554bd..0f718a2f1e0 100644 --- a/spec/unit/models/thread.spec.ts +++ b/spec/unit/models/thread.spec.ts @@ -267,8 +267,7 @@ describe("Thread", () => { ts: 10, }); - // The 10th event has been read, as alice's last unthreaded receipt is at ts 200 - // and `mkThread` increment every thread response by 1ms. + // This is marked as read as it is before alice's first threaded receipt... expect(thread.getEventReadUpTo(myUserId)).toBe(events.at(-1)!.getId()); const { thread: thread2 } = mkThread({ diff --git a/src/models/room.ts b/src/models/room.ts index c8745ec0db7..7be21f5c953 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -310,9 +310,12 @@ export class Room extends ReadReceipt { private readonly threadNotifications = new Map(); public readonly cachedThreadReadReceipts = new Map(); // Useful to know at what point the current user has started using threads in this room - public oldestThreadedReceiptTs = Infinity; - // Important to compute `hasUserReadEvent` and similar methods correctly. - public unthreadedReceipts = new Map(); + private oldestThreadedReceiptTs = Infinity; + /** + * Keeping a record of the lastest unthread receipts per user + * This is useful in determining whether a user has read a thread or not + */ + private unthreadedReceipts = new Map(); private readonly timelineSets: EventTimelineSet[]; public readonly threadsTimelineSets: EventTimelineSet[] = []; // any filtered timeline sets we're maintaining for this room @@ -3292,6 +3295,24 @@ export class Room extends ReadReceipt { } event.applyVisibilityEvent(visibilityChange); } + + /** + * Find when a client has gained thread capabilities by inspecting the oldest + * threaded receipt + * @returns the timestamp of the oldest threaded receipt + */ + public getOldestThreadedReceiptTs(): number { + return this.oldestThreadedReceiptTs; + } + + /** + * Returns the most receipt unthreaded receipt for a given user + * @param userId - the MxID of the User + * @returns an unthreaded Receipt + */ + public getLastUnthreadedReceiptFor(userId: string): Receipt | undefined { + return this.unthreadedReceipts.get(userId); + } } // a map from current event status to a list of allowed next statuses diff --git a/src/models/thread.ts b/src/models/thread.ts index 3bce91454da..9bbbf3c8cb5 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -515,12 +515,13 @@ export class Thread extends ReadReceipt { */ public getEventReadUpTo(userId: string, ignoreSynthesized?: boolean): string | null { const isCurrentUser = userId === this.client.getUserId(); - if (isCurrentUser && this.lastReply()) { + const lastReply = this.lastReply(); + if (isCurrentUser && lastReply) { // If the last activity in a thread is prior to the first threaded read receipt // sent in the room (suggesting that it was sent before the user started // using a client that supported threaded read receipts), we want to // consider this thread as read. - const beforeFirstThreadedReceipt = (this.lastReply()?.getTs() ?? 0) < this.room.oldestThreadedReceiptTs; + const beforeFirstThreadedReceipt = (lastReply.getTs() ?? 0) < this.room.getOldestThreadedReceiptTs(); if (beforeFirstThreadedReceipt) { return this.timeline.at(-1)?.getId() ?? null; } @@ -531,15 +532,15 @@ export class Thread extends ReadReceipt { // Check whether the unthreaded read receipt for that user is more recent // than the read receipt inside that thread. if (this.lastReply()) { - if (!this.room.unthreadedReceipts.has(userId)) { + const unthreadedReceipt = this.room.getLastUnthreadedReceiptFor(userId); + if (!unthreadedReceipt) { return readUpToId; } - const unthreadedReceiptTs = this.room.unthreadedReceipts.get(userId)!.ts; for (let i = this.timeline?.length - 1; i >= 0; --i) { const ev = this.timeline[i]; if (ev.getId() === readUpToId) return readUpToId; - if (ev.getTs() < unthreadedReceiptTs) return ev.getId() ?? readUpToId; + if (ev.getTs() < unthreadedReceipt.ts) return ev.getId() ?? readUpToId; } } @@ -556,8 +557,9 @@ export class Thread extends ReadReceipt { */ public hasUserReadEvent(userId: string, eventId: string): boolean { if (userId === this.client.getUserId()) { - const beforeFirstThreadedReceipt = (this.lastReply()?.getTs() ?? 0) < this.room.oldestThreadedReceiptTs; - const unthreadedReceiptTs = this.room.unthreadedReceipts.get(userId)?.ts ?? 0; + const beforeFirstThreadedReceipt = + (this.lastReply()?.getTs() ?? 0) < this.room.getOldestThreadedReceiptTs(); + const unthreadedReceiptTs = this.room.getLastUnthreadedReceiptFor(userId)?.ts ?? 0; const beforeLastUnthreadedReceipt = (this?.lastReply()?.getTs() ?? 0) < unthreadedReceiptTs; if (beforeFirstThreadedReceipt || beforeLastUnthreadedReceipt) { return true; From c9827deb274226e0f1ca7e9caa1baaf733f39029 Mon Sep 17 00:00:00 2001 From: Germain Date: Fri, 6 Jan 2023 14:00:00 +0000 Subject: [PATCH 14/24] Remove unwanted change --- src/client.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client.ts b/src/client.ts index 096335d8253..ae42103f26e 100644 --- a/src/client.ts +++ b/src/client.ts @@ -601,9 +601,9 @@ interface IJoinRequestBody { third_party_signed?: IThirdPartySigned; } -export interface ITagMetadata { +interface ITagMetadata { [key: string]: any; - order?: number; + order: number; } interface IMessagesResponse { From d1ab2def013570970557ebe5d7e6bee6318d1a56 Mon Sep 17 00:00:00 2001 From: Germain Date: Fri, 6 Jan 2023 14:32:41 +0000 Subject: [PATCH 15/24] Improve thread spec --- spec/unit/models/thread.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/unit/models/thread.spec.ts b/spec/unit/models/thread.spec.ts index 0f718a2f1e0..99f090a53b7 100644 --- a/spec/unit/models/thread.spec.ts +++ b/spec/unit/models/thread.spec.ts @@ -116,7 +116,7 @@ describe("Thread", () => { // last threaded receipt "$event2:localhost": { [ReceiptType.Read]: { - [client.getUserId()!]: { ts: 300 }, + [client.getUserId()!]: { ts: 300, thread_id: "$threadId" }, }, }, }, @@ -137,9 +137,9 @@ describe("Thread", () => { authorId: myUserId, participantUserIds: [myUserId], length: 2, - ts: 201, }); + // The event is automatically considered read as the current user is the sender expect(thread.hasUserReadEvent(myUserId, events.at(-1)!.getId() ?? "")).toBeTruthy(); }); From 02f5a6c4a1144f20c7c0ea6a47c38dd6685d0a73 Mon Sep 17 00:00:00 2001 From: Germain Date: Fri, 6 Jan 2023 14:38:04 +0000 Subject: [PATCH 16/24] Explain the unthreaded receipt logic in comments --- src/models/thread.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/models/thread.ts b/src/models/thread.ts index 9bbbf3c8cb5..a73914e20ab 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -539,7 +539,14 @@ export class Thread extends ReadReceipt { for (let i = this.timeline?.length - 1; i >= 0; --i) { const ev = this.timeline[i]; + // If we encounter the `readUpToId` we do not need to look further + // there is no "more recent" unthreaded read receipt if (ev.getId() === readUpToId) return readUpToId; + + // Inspecting events from most recent to oldest, we're checking + // whether an unthreaded read receipt is more recent that the current event. + // We usually prefer relying on the order of the DAG but in this scenario + // it is not possible and we have to rely on timestamp if (ev.getTs() < unthreadedReceipt.ts) return ev.getId() ?? readUpToId; } } From feb8f5eeae27d19cf31f00d0339756b7519bdec0 Mon Sep 17 00:00:00 2001 From: Germain Date: Mon, 9 Jan 2023 12:26:02 +0000 Subject: [PATCH 17/24] Apply comments readibility suggestions Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/models/room.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/models/room.ts b/src/models/room.ts index 7be21f5c953..aed31425d88 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -312,7 +312,7 @@ export class Room extends ReadReceipt { // Useful to know at what point the current user has started using threads in this room private oldestThreadedReceiptTs = Infinity; /** - * Keeping a record of the lastest unthread receipts per user + * A record of the latest unthread receipts per user * This is useful in determining whether a user has read a thread or not */ private unthreadedReceipts = new Map(); @@ -3306,7 +3306,7 @@ export class Room extends ReadReceipt { } /** - * Returns the most receipt unthreaded receipt for a given user + * Returns the most recent unthreaded receipt for a given user * @param userId - the MxID of the User * @returns an unthreaded Receipt */ From 5c5cbbf39a6107f8bd53a9734babd6a2643384ce Mon Sep 17 00:00:00 2001 From: Germain Date: Mon, 9 Jan 2023 12:30:28 +0000 Subject: [PATCH 18/24] Clarify code comments based on PR feedback --- src/models/thread.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/models/thread.ts b/src/models/thread.ts index a73914e20ab..30fd4a448ff 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -505,8 +505,8 @@ export class Thread extends ReadReceipt { } /** - * 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. + * Get the ID of the event that a given user has read up to within that thread, + * or null if we have received no read receipts from them. * @param userId - The user ID to get read receipt event ID for * @param ignoreSynthesized - If true, return only receipts that have been * sent by the server, not implicit ones generated @@ -556,7 +556,7 @@ export class Thread extends ReadReceipt { /** * Determines if the given user has read a particular event ID with the known - * history of the room. This is not a definitive check as it relies only on + * history of the thread. This is not a definitive check as it relies only on * what is available to the thread at the time of execution. * @param userId - The user ID to check the read state of. * @param eventId - The event ID to check if the user read. @@ -564,6 +564,10 @@ export class Thread extends ReadReceipt { */ public hasUserReadEvent(userId: string, eventId: string): boolean { if (userId === this.client.getUserId()) { + // Consider an event read if it's part of a thread that has no + // read receipts and has no notifications. It is likely that it is + // part of a thread that was created before read receipts for threads + // were supported (via MSC3771) const beforeFirstThreadedReceipt = (this.lastReply()?.getTs() ?? 0) < this.room.getOldestThreadedReceiptTs(); const unthreadedReceiptTs = this.room.getLastUnthreadedReceiptFor(userId)?.ts ?? 0; From 0ee16d39e78aac614688e90c8db56636314a7a7c Mon Sep 17 00:00:00 2001 From: Germain Date: Mon, 9 Jan 2023 12:30:44 +0000 Subject: [PATCH 19/24] Remove unneeded nullish coalescing check --- src/models/thread.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/models/thread.ts b/src/models/thread.ts index 30fd4a448ff..a0c4ebcf801 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -521,7 +521,7 @@ export class Thread extends ReadReceipt { // sent in the room (suggesting that it was sent before the user started // using a client that supported threaded read receipts), we want to // consider this thread as read. - const beforeFirstThreadedReceipt = (lastReply.getTs() ?? 0) < this.room.getOldestThreadedReceiptTs(); + const beforeFirstThreadedReceipt = lastReply.getTs() < this.room.getOldestThreadedReceiptTs(); if (beforeFirstThreadedReceipt) { return this.timeline.at(-1)?.getId() ?? null; } From 253427d1260eab1fa7d67870edb45e182c149bc8 Mon Sep 17 00:00:00 2001 From: Germain Date: Tue, 10 Jan 2023 13:51:32 +0000 Subject: [PATCH 20/24] Improve comments wording Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/models/thread.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/models/thread.ts b/src/models/thread.ts index a0c4ebcf801..9118dcce657 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -505,8 +505,8 @@ export class Thread extends ReadReceipt { } /** - * Get the ID of the event that a given user has read up to within that thread, - * or null if we have received no read receipts from them. + * Get the ID of the event that a given user has read up to within this thread, + * or null if we have received no read receipt (at all) from them. * @param userId - The user ID to get read receipt event ID for * @param ignoreSynthesized - If true, return only receipts that have been * sent by the server, not implicit ones generated From 4efb717ed9f42bcd090846ec581049af74e55980 Mon Sep 17 00:00:00 2001 From: Germain Date: Tue, 10 Jan 2023 14:02:45 +0000 Subject: [PATCH 21/24] Clarify comments --- src/models/room.ts | 3 ++- src/models/thread.ts | 18 ++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/models/room.ts b/src/models/room.ts index 8de505a1264..9aa35e1441c 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -3331,7 +3331,8 @@ export class Room extends ReadReceipt { /** * Returns the most recent unthreaded receipt for a given user * @param userId - the MxID of the User - * @returns an unthreaded Receipt + * @returns an unthreaded Receipt, can be undefined if receipts have been disabled + * or a user chooses to use private read receipts. */ public getLastUnthreadedReceiptFor(userId: string): Receipt | undefined { return this.unthreadedReceipts.get(userId); diff --git a/src/models/thread.ts b/src/models/thread.ts index 9118dcce657..ad31a1ddfa6 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -515,15 +515,17 @@ export class Thread extends ReadReceipt { */ public getEventReadUpTo(userId: string, ignoreSynthesized?: boolean): string | null { const isCurrentUser = userId === this.client.getUserId(); - const lastReply = this.lastReply(); + const lastReply = this.timeline.at(-1); if (isCurrentUser && lastReply) { // If the last activity in a thread is prior to the first threaded read receipt // sent in the room (suggesting that it was sent before the user started // using a client that supported threaded read receipts), we want to // consider this thread as read. const beforeFirstThreadedReceipt = lastReply.getTs() < this.room.getOldestThreadedReceiptTs(); - if (beforeFirstThreadedReceipt) { - return this.timeline.at(-1)?.getId() ?? null; + const lastReplyId = lastReply.getId(); + // Some unsent events do not have an ID, we do not want to consider them read + if (beforeFirstThreadedReceipt && lastReplyId) { + return lastReplyId; } } @@ -531,7 +533,7 @@ export class Thread extends ReadReceipt { // Check whether the unthreaded read receipt for that user is more recent // than the read receipt inside that thread. - if (this.lastReply()) { + if (lastReply) { const unthreadedReceipt = this.room.getLastUnthreadedReceiptFor(userId); if (!unthreadedReceipt) { return readUpToId; @@ -564,10 +566,10 @@ export class Thread extends ReadReceipt { */ public hasUserReadEvent(userId: string, eventId: string): boolean { if (userId === this.client.getUserId()) { - // Consider an event read if it's part of a thread that has no - // read receipts and has no notifications. It is likely that it is - // part of a thread that was created before read receipts for threads - // were supported (via MSC3771) + // Consider an event read if it's part of a thread that is before the + // first threaded receipt sent in that room. It is likely that it is + // part of a thread that was created before MSC3771 was implemented. + // Or before the last unthreaded receipt for the logged in user const beforeFirstThreadedReceipt = (this.lastReply()?.getTs() ?? 0) < this.room.getOldestThreadedReceiptTs(); const unthreadedReceiptTs = this.room.getLastUnthreadedReceiptFor(userId)?.ts ?? 0; From d2935d820e3099cb31a1a737f392d3f70af1067d Mon Sep 17 00:00:00 2001 From: Germain Date: Tue, 10 Jan 2023 14:31:15 +0000 Subject: [PATCH 22/24] fix tests --- spec/unit/room.spec.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index 5f6f355a315..aa3b709cd7f 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -50,6 +50,7 @@ import { ReceiptType, WrappedReceipt } from "../../src/@types/read_receipts"; import { FeatureSupport, Thread, THREAD_RELATION_TYPE, ThreadEvent } from "../../src/models/thread"; import { Crypto } from "../../src/crypto"; import { mkThread } from "../test-utils/thread"; +import { getMockClientWithEventEmitter, mockClientMethodsEvents, mockClientMethodsUser } from "../test-utils/client"; describe("Room", function () { const roomId = "!foo:bar"; @@ -3228,6 +3229,14 @@ describe("Room", function () { }); describe("findPredecessorRoomId", () => { + let client: MatrixClient | null = null; + beforeEach(() => { + client = getMockClientWithEventEmitter({ + ...mockClientMethodsUser(), + supportsExperimentalThreads: jest.fn().mockReturnValue(true), + }); + }); + function roomCreateEvent(newRoomId: string, predecessorRoomId: string | null): MatrixEvent { const content: { creator: string; @@ -3258,18 +3267,18 @@ describe("Room", function () { } it("Returns null if there is no create event", () => { - const room = new Room("roomid", null as unknown as MatrixClient, "@u:example.com"); + const room = new Room("roomid", client!, "@u:example.com"); expect(room.findPredecessorRoomId()).toBeNull(); }); it("Returns null if the create event has no predecessor", () => { - const room = new Room("roomid", null as unknown as MatrixClient, "@u:example.com"); + const room = new Room("roomid", client!, "@u:example.com"); room.addLiveEvents([roomCreateEvent("roomid", null)]); expect(room.findPredecessorRoomId()).toBeNull(); }); it("Returns the predecessor ID if one is provided via create event", () => { - const room = new Room("roomid", null as unknown as MatrixClient, "@u:example.com"); + const room = new Room("roomid", client!, "@u:example.com"); room.addLiveEvents([roomCreateEvent("roomid", "replacedroomid")]); expect(room.findPredecessorRoomId()).toBe("replacedroomid"); }); From 1d8891038089563c5432f934fbb23879bcbb5b97 Mon Sep 17 00:00:00 2001 From: Germain Date: Tue, 10 Jan 2023 14:47:49 +0000 Subject: [PATCH 23/24] lint fix --- spec/unit/room.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index aa3b709cd7f..38fc2cdc42a 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -50,7 +50,7 @@ import { ReceiptType, WrappedReceipt } from "../../src/@types/read_receipts"; import { FeatureSupport, Thread, THREAD_RELATION_TYPE, ThreadEvent } from "../../src/models/thread"; import { Crypto } from "../../src/crypto"; import { mkThread } from "../test-utils/thread"; -import { getMockClientWithEventEmitter, mockClientMethodsEvents, mockClientMethodsUser } from "../test-utils/client"; +import { getMockClientWithEventEmitter, mockClientMethodsUser } from "../test-utils/client"; describe("Room", function () { const roomId = "!foo:bar"; From dae36b719bd132dd30b85ecb38dd22fe3cc42aa0 Mon Sep 17 00:00:00 2001 From: Germain Date: Wed, 11 Jan 2023 09:42:08 +0000 Subject: [PATCH 24/24] Final comment wording updates Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/models/room.ts | 5 +++-- src/models/thread.ts | 9 ++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/models/room.ts b/src/models/room.ts index 9aa35e1441c..a363ef0dfa3 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -3331,8 +3331,9 @@ export class Room extends ReadReceipt { /** * Returns the most recent unthreaded receipt for a given user * @param userId - the MxID of the User - * @returns an unthreaded Receipt, can be undefined if receipts have been disabled - * or a user chooses to use private read receipts. + * @returns an unthreaded Receipt. Can be undefined if receipts have been disabled + * or a user chooses to use private read receipts (or we have simply not received + * a receipt from this user yet). */ public getLastUnthreadedReceiptFor(userId: string): Receipt | undefined { return this.unthreadedReceipts.get(userId); diff --git a/src/models/thread.ts b/src/models/thread.ts index ad31a1ddfa6..2ffee8038c2 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -557,9 +557,12 @@ export class Thread extends ReadReceipt { } /** - * Determines if the given user has read a particular event ID with the known - * history of the thread. This is not a definitive check as it relies only on - * what is available to the thread at the time of execution. + * Determine if the given user has read a particular event. + * + * It is invalid to call this method with an event that is not part of this thread. + * + * This is not a definitive check as it only checks the events that have been + * loaded client-side at the time of execution. * @param userId - The user ID to check the read state of. * @param eventId - The event ID to check if the user read. * @returns True if the user has read the event, false otherwise.