search

matrix-org / matrix-rust-sdk

Matrix Client-Server SDK for Rust
Apache License 2.0
1.2k stars 240 forks source link

ui: Allow to subscribe to read receipt changes in timeline metadata #4014

Closed zecakeh closed 4 days ago

zecakeh commented 1 week ago

This is to fix a bug caused by a race condition in Fractal. We used an event handler to detect changes in our own read receipts, then we called methods like Timeline::latest_user_read_receipt_timeline_event_id(). The problem is that there is no guarantee that the metadata of the timeline has already been updated when we call the method since they happen in separate threads. So instead we use this stream to react when the metadata is updated.

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.27%. Comparing base (f576c72) to head (6747a6f). Report is 16 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #4014 +/- ## ======================================= Coverage 84.27% 84.27% ======================================= Files 266 266 Lines 28336 28345 +9 ======================================= + Hits 23880 23889 +9 Misses 4456 4456 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bnjbvr commented 1 week ago

This is to fix a bug caused by a race condition in Fractal.

So should this be a fix in Fractal, instead? I'm not sure we want to pay the price of supporting a new API just for a bug that's happening in Fractal, especially since 1. the read receipts code is already quite complicated in the timeline API, 2. it should be moved over to the event cache eventually, so it'd be ideal to not support app-specific features in there. If you can make the argument why this feature would be useful in general, and not only for Fractal, then I'm all for it, otherwise I'd rather not add an extra maintenance burden onto ourselves.

zecakeh commented 1 week ago

We use it to compute if there are unread messages in the room, because Room::num_unread_messages doesn't work with /v2/sync so it might only be needed temporarily.

On the other end, I don't see methods like Timeline::latest_user_read_receipt() and Timeline::latest_user_read_receipt_timeline_event_id() being useful without this API, because imo it makes sense to be able to call these functions after the read receipts changed. There is nothing to "fix" in Fractal, the current bug we have is because of a shortcoming in the current API.

Usually a "bug in Fractal" is a bug that can be encountered by a client that uses the current stable Matrix APIs.