Accumulate receipts for the main thread and unthreaded separately #3339
Conversation
Fixes matrix-org/element-web#24629
andybalaam
added
the
T-Defect
| @@ -118,7 +118,7 @@ export class ReceiptAccumulator { | |||
| eventId, | |||
| }; | |||
|
|
|||
| if (!data.thread_id || data.thread_id === MAIN_ROOM_TIMELINE) { | |||
There was a problem hiding this comment.
A comment with some context would be helpful why "we shouldn't also be checking for MAIN_ROOM_TIMELINE because ... otherwise we run into bug x. We want to avoid making messages unread after ..."
There was a problem hiding this comment.
I thought about it, but basically the old code used to treat the main thread as special, and the fix is not to, so it felt like the comment should have been there before, and my change would have deleted it...
There was a problem hiding this comment.
MAIN_ROOM_TIMELINE is still a thing in the codebase that people need to reason about and I'm confused and have to think about whenever I look at it.
- A normal message is part of
thread_id = MAIN_ROOM_TIMELINE - A thread root is part of
thread_id = MAIN_ROOM_TIMELINE - A threaded reply has a
thread_id = 'xxx'
Perhaps we can add this in a comment somewhere:
In a world that supports threads, all read receipts have a
thread_idwhich is either the thread they belong in orMAIN_ROOM_TIMELINEand we should always usesetThreaded(...)here. TheMAIN_ROOM_TIMELINEis just treated as another thread.People still may send us read receipts from clients that don't support threads (unthreaded, without the
thread_idproperty) which we usesetUnthreaded(...)for.Using the wrong method will cause undefined behavior like messages re-appearing as "new" when you already read them previously.
There was a problem hiding this comment.
That's not quite it - when the user marks a room as read e.g. by pressing Esc or clicking the "x" in the top right, we do send an unthreaded receipt. I'd happily merge a PR from you with a comment like this somewhere appropriate.
There was a problem hiding this comment.
Created #3378 and incorporated those caveats 馃憤
* Add more context for why threaded vs unthreaded read receipts See #3339 (comment) * Language updates from Andy See #3378 (comment) * Fix lints
* Ensure we do not add relations to the wrong timeline ([\matrix-org#3427](matrix-org#3427)). Fixes element-hq/element-web#25450 and element-hq/element-web#25494. * Deprecate `QrCodeEvent`, `SasEvent` and `VerificationEvent` ([\matrix-org#3386](matrix-org#3386)). * Move crypto classes into a separate namespace ([\matrix-org#3385](matrix-org#3385)). * Mention deno support in the README ([\matrix-org#3417](matrix-org#3417)). Contributed by @sigmaSd. * Mark room version 10 as safe ([\matrix-org#3425](matrix-org#3425)). * Prioritise entirely supported flows for UIA ([\matrix-org#3402](matrix-org#3402)). * Add methods to terminate idb worker ([\matrix-org#3362](matrix-org#3362)). * Total summary count ([\matrix-org#3351](matrix-org#3351)). Contributed by @toger5. * Audio concealment ([\matrix-org#3349](matrix-org#3349)). Contributed by @toger5. * Correctly accumulate sync summaries. ([\matrix-org#3366](matrix-org#3366)). Fixes element-hq/element-web#23345. * Keep measuring a call feed's volume after a stream replacement ([\matrix-org#3361](matrix-org#3361)). Fixes element-hq/element-call#1051. * Element-R: Avoid uploading a new fallback key at every `/sync` ([\matrix-org#3338](matrix-org#3338)). Fixes element-hq/element-web#25215. * Accumulate receipts for the main thread and unthreaded separately ([\matrix-org#3339](matrix-org#3339)). Fixes element-hq/element-web#24629. * Remove spec non-compliant extended glob format ([\matrix-org#3423](matrix-org#3423)). Fixes element-hq/element-web#25474. * Fix bug where original event was inserted into timeline instead of the edit event ([\matrix-org#3398](matrix-org#3398)). Contributed by @andybalaam. * Only add a local receipt if it's after an existing receipt ([\matrix-org#3399](matrix-org#3399)). Contributed by @andybalaam. * Attempt a potential workaround for stuck notifs ([\matrix-org#3384](matrix-org#3384)). Fixes element-hq/element-web#25406. Contributed by @andybalaam. * Fix verification bug with `pendingEventOrdering: "chronological"` ([\matrix-org#3382](matrix-org#3382)).
Fixes element-hq/element-web#24629
Here's what your changelog entry will look like:
馃悰 Bug Fixes