From aaa0f1e829ce3be9268b247de07f95bbefa4f108 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 17 May 2023 15:16:01 -0500 Subject: [PATCH 1/3] Add more context for why threaded vs unthreaded read receipts See https://github.com/matrix-org/matrix-js-sdk/pull/3339#discussion_r1195364120 --- src/receipt-accumulator.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/receipt-accumulator.ts b/src/receipt-accumulator.ts index f1037233239..1ce54dedc7d 100644 --- a/src/receipt-accumulator.ts +++ b/src/receipt-accumulator.ts @@ -118,6 +118,21 @@ export class ReceiptAccumulator { eventId, }; + // In a world that supports threads, all read receipts have a + // `thread_id` which is either the thread they belong in or + // `MAIN_ROOM_TIMELINE` and we should always use + // `setThreaded(...)` here. The `MAIN_ROOM_TIMELINE` is just + // treated as another thread. + // + // We still may encounter read receipts from clients that don't + // support threads (unthreaded, without the `thread_id` + // property) which we use `setUnthreaded(...)` for. Or even in a + // threaded-world, a client may choose to use an unthreaded read + // receipt to completely mark the room as read. + // + // Using the wrong method will cause undefined behavior like + // messages re-appearing as "new" when you already read them + // previously. if (!data.thread_id) { this.setUnthreaded(userId, receipt); } else { From ab90ca0360c17f01a6c008d04a938c2b2c26e500 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 18 May 2023 11:52:26 -0500 Subject: [PATCH 2/3] Language updates from Andy See https://github.com/matrix-org/matrix-js-sdk/pull/3378#discussion_r1197622113 --- src/receipt-accumulator.ts | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/receipt-accumulator.ts b/src/receipt-accumulator.ts index 1ce54dedc7d..a24b49f6cf3 100644 --- a/src/receipt-accumulator.ts +++ b/src/receipt-accumulator.ts @@ -118,19 +118,24 @@ export class ReceiptAccumulator { eventId, }; - // In a world that supports threads, all read receipts have a - // `thread_id` which is either the thread they belong in or - // `MAIN_ROOM_TIMELINE` and we should always use - // `setThreaded(...)` here. The `MAIN_ROOM_TIMELINE` is just - // treated as another thread. - // - // We still may encounter read receipts from clients that don't - // support threads (unthreaded, without the `thread_id` - // property) which we use `setUnthreaded(...)` for. Or even in a - // threaded-world, a client may choose to use an unthreaded read - // receipt to completely mark the room as read. - // - // Using the wrong method will cause undefined behavior like + // In a world that supports threads, read receipts normally have + // a `thread_id` which is either the thread they belong in or + // `MAIN_ROOM_TIMELINE`, so we normally use `setThreaded(...)` + // here. The `MAIN_ROOM_TIMELINE` is just treated as another + // thread. + // + // We still encounter read receipts that are "unthreaded" + // (missing the `thread_id` property). These come from clients + // that don't support threads, and from threaded clients that + // are doing a "Mark room as read" operation. Unthreaded + // receipts mark everything "before" them as read, in all + // threads, where "before" means in Sync Order i.e. the order + // the events were received from the homeserver in a sync. + // [Note: we have some bugs where we use timestamp order instead + // of Sync Order, because we don't correctly remember the Sync + // Order. See #3325.] + // + // Calling the wrong method will cause incorrect behavior like // messages re-appearing as "new" when you already read them // previously. if (!data.thread_id) { From 4f1b83869a8a3b19a021eda090dccde6161abe28 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 18 May 2023 13:26:35 -0500 Subject: [PATCH 3/3] Fix lints --- src/receipt-accumulator.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/receipt-accumulator.ts b/src/receipt-accumulator.ts index a24b49f6cf3..ded358ad9a1 100644 --- a/src/receipt-accumulator.ts +++ b/src/receipt-accumulator.ts @@ -123,7 +123,7 @@ export class ReceiptAccumulator { // `MAIN_ROOM_TIMELINE`, so we normally use `setThreaded(...)` // here. The `MAIN_ROOM_TIMELINE` is just treated as another // thread. - // + // // We still encounter read receipts that are "unthreaded" // (missing the `thread_id` property). These come from clients // that don't support threads, and from threaded clients that @@ -134,7 +134,7 @@ export class ReceiptAccumulator { // [Note: we have some bugs where we use timestamp order instead // of Sync Order, because we don't correctly remember the Sync // Order. See #3325.] - // + // // Calling the wrong method will cause incorrect behavior like // messages re-appearing as "new" when you already read them // previously.