Process rooms' read status client-side #2953

bnjbvr · 2023-12-15T18:36:29Z

This adds support for processing the number of unread messages and mentions (highlights) client-side. This can be used to know whether a room has unread content or not (if any count > 0), or if there are mentions (highlights > 0) for the user in a given room. Or, it can also be used as a high-level approximation of the number of unread messages/mentions for recently visited rooms.

This implementation is not final, but attempts to be useful in the short-term. It's relatively simple. It is an improvement over the status quo, in that it can handle events from encrypted rooms, and it has a short-term memory to reconcile read receipts received at a different time than the events they relate to.

Some implementation notes:

Historically, there's UnreadNotificationCounts which returns information as computed by the server. While it's reliable for non-encrypted rooms, it is not for encrypted rooms. A previous version of this PR overrode that, but it was indicated that this field was still useful to have per se, so it's untouched in this new version.
each RoomInfo gets a new field ReadReceipts. This is used to contain both the event id to which attaches the latest read receipt we've observed, and to contain the number of unread messages and highlights. This is only computed for sliding sync, but with some effort we could port it over to regular sync v2 as well. The num_unread_messages field reflects the number of unread messages, while num_unread_mentions reflects the number of highlights, according to push rules. Note: this relies on push rules being available and the push context being properly computed, and mentions won't work properly if any of these is missing.
- this is not updated during back-pagination/messages/context queries: here we're focusing on events received from sync.
to mark a room as unread, an event must have the following properties:
- be valid according to the Matrix protocol (deserializable)
- be a message-like event (not a state event)
- and even a subset of message-likes (we don't want to mark a room as unread when somebody reacts to somebody else's message)
- not be a redaction or an edit
- be sent from somebody else
After sending a read receipt, we'll receive a read receipt echo from the server. Using that information, we can then clear up the counts, and start counting unread messages and mentions from the next event.
It could be that a read receipt comes for an event that was in a previous sync batch. This implementation can handle that, by looking at the previously known events (those kept in the timeline_queue in-memory cache, that's partially persisted to disk), and reconciling a read receipt against those. It can also handle read-receipts for events that come in future syncs (which is possible because: federation 🥳), since it's possible to save the receipt as soon as it's seen, and reconcile it later.

There's an integration test that was used to ensure that the approach is viable. There are units tests for determining interesting events that would mark a room as unread.

codecov · 2023-12-15T19:19:22Z

Codecov Report

Attention: 65 lines in your changes are missing coverage. Please review.

Comparison is base (320b868) 83.50% compared to head (85ddfb0) 83.28%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
crates/matrix-sdk-base/src/read_receipts.rs	42.85%	44 Missing ⚠️
crates/matrix-sdk-base/src/sliding_sync.rs	69.23%	12 Missing ⚠️
crates/matrix-sdk-base/src/rooms/normal.rs	14.28%	6 Missing ⚠️
crates/matrix-sdk/src/sliding_sync/client.rs	72.72%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2953      +/-   ##
==========================================
- Coverage   83.50%   83.28%   -0.22%     
==========================================
  Files         221      222       +1     
  Lines       23007    23123     +116     
==========================================
+ Hits        19211    19258      +47     
- Misses       3796     3865      +69

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jmartinesp · 2023-12-18T14:51:04Z

On Android this doesn't seem to work properly:

If I open the room in another client (web) the read status doesn't seem to change in the EXA app.
Updating the read status doesn't seem to work for rooms with mentions. I have:

elementx: EX Test Room: Notification count: 7 | Highlight count: 14 | RoomSummaryDetailsFactory.kt:34

Then I tap on it, open the room, read everything (a few read receipts should have been sent). I go back to the room list, and I still see the unread notification indicator, a new log appears but the unread and highlighted count stays the same (7, 14). Restarting the app also has no effect.

jmartinesp · 2023-12-18T14:54:13Z

As soon as I sent the message above, I tried again sending a message with no mention, then opening the room in EXA and it's working fine now 🤷‍♂️ .

I have another room with the unread indicator and the values stuck, maybe it needs a new message first to be unstuck?

jmartinesp · 2023-12-19T11:42:13Z

The new implementation with num_unread_* seems to be a lot more reliable, none of the issues I mentioned above can be reproduced anymore.

…te fields of `RoomInfo`

…pt state Before this patch, we needed to clone the inner `timeline_queue` and turn it into a concrete `Vec<SyncTimelineEvent>`, just to iterate on the elements, and because returning an iterator from a trait method is impractical. This now changes it to return the actual concrete type of `timeline_queue`, so we don't need the extra allocations. Ideally, matrix-sdk and matrix-sdk-base would be merged, so we don't need to use a trait at all here.

This helps supporting cases where we want to show that a room has some activity (unread messages) but no notifications.

Hywan

I've added several comments, and I reckon their need to be addressed. The fundamental logic behind this PR looks valid to me, good job. Also thank you for writing atomic commits, it's helped a lot!

crates/matrix-sdk-base/src/read_receipts.rs

crates/matrix-sdk-base/src/sliding_sync.rs

crates/matrix-sdk/src/sliding_sync/client.rs

crates/matrix-sdk-base/src/read_receipts.rs

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

crates/matrix-sdk-base/src/read_receipts.rs

andybalaam

Looks good. I've mentioned some limitations in my comments, and here are a couple more:

In matrix-js-sdk, we generate a "synthetic" receipt when we send a message (or receive a message we sent from another client), meaning we consider everything before that message to be read. I don't see that here, but I might have missed it.
This will need more complex data structures if/when we support threads.

andybalaam · 2023-12-20T10:22:12Z

crates/matrix-sdk-base/src/read_receipts.rs

+        // Find a private or public read receipt for the current user.
+        let mut receipt_event_id = None;
+        if let Some((event_id, receipt)) =
+            receipt_event.user_receipt(user_id, ReceiptType::ReadPrivate)


This code always picks the private receipt if it is present, but the spec says:

Between m.read and m.read.private, the receipt which is more “ahead” or “recent” is used when determining the highest read-up-to mark

Pondered about this a bit, and what we get from the read receipts extension of sliding sync is a mapping of room id -> event id -> user id -> receipt. So to know which receipt is "ahead" in terms of sync order, we'd need to iterate over the events (with the same caveat that they could be from this sync, or a previous one, or a future one).

Would using the time of the receipt be good enough to compare them? (it's the read event timestamp, if I'm not mistaken, so that's forgeable since it's user-defined, but I don't see any big threat here)

As I tried to describe here it's tricky to decide the order of messages, and it does matter. If you disagree with the server over which message is first, the server will discard the receipts you send (if it think it already has a later one) and you will end up in an inconsistent state between server and client, potentially causing stuck unreads.

The ts in the receipt is the time at which it was created (I am fairly sure?), so it doesn't help us know which events are read. Even if it's the ts of the read event, it still might disagree with the server's idea of what is first.

If you disagree with the server over which message is first, the server will discard the receipts you send (if it think it already has a later one) and you will end up in an inconsistent state between server and client, potentially causing stuck unreads.

Thanks, that's new information 🥲

andybalaam · 2023-12-20T10:24:23Z

crates/matrix-sdk-base/src/read_receipts.rs

+            // about.
+
+            // First, save the event id as the latest one that has a read receipt.
+            room_info.read_receipts.latest_read_receipt_event_id = Some(receipt_event_id.clone());


This unconditionally saves a receipt we receive as the latest one. I don't know whether this can happen, but if we receive a receipt out of order, we may end up with incorrect unreadness.

Knowing which receipt is latest is particularly problematic, because we have to find the events they refer to, and then decide which of those is "latest".

(I assume this can happen, and, to be explicit, I think the correct behaviour is to figure out which is latest and keep only that one.)

In matrix-js-sdk, we hold on to "dangling" receipts for which we don't have an event, so that when/if we get the event, we can use that to decide whether the receipt is after the one we already have.

Very good point. Well it'll be for a later iteration of this PR I suppose 🥴

- copyright notice - doc comments and better doc in general - use static dispatch instead of &dyn T - other misc comments

bnjbvr · 2023-12-21T08:36:14Z

In matrix-js-sdk, we generate a "synthetic" receipt when we send a message (or receive a message we sent from another client), meaning we consider everything before that message to be read. I don't see that here, but I might have missed it.

For what it's worth, I consider this a different feature request. We do have the concept of an implicit read receipt when we send a message, but that's only client-side (no read marker/receipt is sent to the server in that case). We can tackle that later.

Hywan

All feedback have been addressed. There is room (ha. ha.) for improvements, but let's address that in another PR.

Hywan · 2023-12-21T09:08:23Z

crates/matrix-sdk-base/src/read_receipts.rs

+/// update the notification count in the room.
+///
+/// Returns a boolean indicating if it's found the event and updated the count.
+fn find_and_count_events<'a>(


crates/matrix-sdk-base/src/read_receipts.rs

bmarty · 2023-12-21T09:56:39Z

Tested with element-hq/element-x-android#2080 and it's working fine.

Sometimes the mention/badge does not vanish when I open the room and go back to the room list, but I guess it's not a blocker for today.

The test fails only in the codecov build, not in a local build or in the other integration test. Needs further investigation.

bnjbvr requested a review from a team as a code owner December 15, 2023 18:36

bnjbvr requested review from Hywan and removed request for a team December 15, 2023 18:36

bnjbvr force-pushed the wip-read-status branch from 12b2391 to afe3dbf Compare December 15, 2023 19:07

bnjbvr mentioned this pull request Dec 18, 2023

Small sliding sync refactorings #2956

Merged

bnjbvr added 2 commits December 18, 2023 18:08

sdk base: add latest_read_receipt_event_id field to RoomInfo

5b94bd4

doc: document SlidingSyncRoomInner::timeline_queue a bit better

cb956d6

bnjbvr force-pushed the wip-read-status branch from afe3dbf to 68ee9d6 Compare December 19, 2023 11:04

bnjbvr added 3 commits December 19, 2023 16:17

sync: process unread notification count client-side

a9da591

read receipts: add integration tests for read receipts

031d810

Make it clear that some functions are tests or test helpers only

2726725

bnjbvr force-pushed the wip-read-status branch from 68ee9d6 to fb763e3 Compare December 19, 2023 15:17

read receipts: move the unread messages and mentions counts to separa…

5602e8c

…te fields of `RoomInfo`

bnjbvr force-pushed the wip-read-status branch from fb763e3 to 5602e8c Compare December 19, 2023 15:31

bnjbvr added 2 commits December 19, 2023 16:52

read receipts: add an extra num_unread_notifications field

e9fe6b8

This helps supporting cases where we want to show that a room has some activity (unread messages) but no notifications.

Hywan requested changes Dec 20, 2023

View reviewed changes

andybalaam reviewed Dec 20, 2023

View reviewed changes

This was referenced Dec 20, 2023

Iterate on room list indicators element-hq/element-x-android#2079

Closed

Room list badges element-hq/element-x-android#2080

Merged

Address review comments

f989c17

- copyright notice - doc comments and better doc in general - use static dispatch instead of &dyn T - other misc comments

bnjbvr requested a review from Hywan December 21, 2023 09:00

Hywan approved these changes Dec 21, 2023

View reviewed changes

bnjbvr force-pushed the wip-read-status branch from d8bd5e4 to da1c2da Compare December 21, 2023 10:09

bnjbvr enabled auto-merge (rebase) December 21, 2023 10:11

bnjbvr mentioned this pull request Dec 21, 2023

Improve client-side read receipt support #2963

Closed

4 tasks

Disable integration test in code coverage build

85ddfb0

The test fails only in the codecov build, not in a local build or in the other integration test. Needs further investigation.

bnjbvr force-pushed the wip-read-status branch from da1c2da to 85ddfb0 Compare December 21, 2023 10:41

bnjbvr merged commit fb1ff70 into main Dec 21, 2023
35 checks passed

bnjbvr deleted the wip-read-status branch December 21, 2023 10:55

This was referenced Dec 21, 2023

Room List API: Sync between devices the state of the mentioned indicator. #2954

Closed

read receipts: don't update a RoomInfo if the read receipts haven't changed #2968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process rooms' read status client-side #2953

Process rooms' read status client-side #2953

bnjbvr commented Dec 15, 2023 •

edited

Loading

codecov bot commented Dec 15, 2023 •

edited

Loading

jmartinesp commented Dec 18, 2023

jmartinesp commented Dec 18, 2023

jmartinesp commented Dec 19, 2023

Hywan left a comment

andybalaam left a comment

andybalaam Dec 20, 2023

bnjbvr Dec 21, 2023

andybalaam Jan 5, 2024

bnjbvr Jan 5, 2024

andybalaam Dec 20, 2023

andybalaam Dec 20, 2023

bnjbvr Dec 21, 2023

bnjbvr commented Dec 21, 2023

Hywan left a comment

Hywan Dec 21, 2023

bmarty commented Dec 21, 2023

Process rooms' read status client-side #2953

Process rooms' read status client-side #2953

Conversation

bnjbvr commented Dec 15, 2023 • edited Loading

codecov bot commented Dec 15, 2023 • edited Loading

Codecov Report

jmartinesp commented Dec 18, 2023

jmartinesp commented Dec 18, 2023

jmartinesp commented Dec 19, 2023

Hywan left a comment

Choose a reason for hiding this comment

andybalaam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr commented Dec 21, 2023

Hywan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty commented Dec 21, 2023

bnjbvr commented Dec 15, 2023 •

edited

Loading

codecov bot commented Dec 15, 2023 •

edited

Loading