Skip to content

Conversation

@jmartinesp
Copy link
Contributor

@jmartinesp jmartinesp commented Nov 13, 2025

Changes:

  • In the live timeline, don't send read receipts for threaded events if hide_thread_events is enabled. EventMeta has now a thread_root_id field so we can know if the event is part of a thread, and TimelineController::latest_event_id filters those out if hide_thread_events is enabled.
    • Why do this instead of just filtering out non visible items? Because reactions, edits, replacements, etc. are also non-visible items that have to be taken into account for the read receipts. So it seems like we have 2 types of 'not visible' items in the timeline, those that aren't visible because we don't want to display them as separate items (aggregations), and those that aren't visible because they shouldn't be displayed at all.
    • Then why not avoid adding the items at all? Because some logic in the SDK depends on the threaded events being added to the remote events in the timeline, even if they aren't displayed because of hide_threaded_events, so some tests were broken.
  • Fix test that was consistently failing locally on main - not really related to this PR, but it also made it fail in CI. It looks like a race condition of some kind.
  • Modify ffi::Timeline::latest_event_id to use ui::Timeline::latest_event_id, which will return the value from TimelineController::latest_event_id.

Fixes element-hq/element-x-android#5639.

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 13, 2025

CodSpeed Performance Report

Merging #5864 will not alter performance

Comparing fix/sending-read-receipts-in-threads (92a3d3a) with main (9ab886f)

Summary

✅ 50 untouched

@jmartinesp jmartinesp force-pushed the fix/sending-read-receipts-in-threads branch 2 times, most recently from 07141d7 to ef9278f Compare November 13, 2025 15:34
@codecov
Copy link

codecov bot commented Nov 13, 2025

Codecov Report

❌ Patch coverage is 92.30769% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.59%. Comparing base (9ab886f) to head (92a3d3a).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs 82.35% 3 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/mod.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5864   +/-   ##
=======================================
  Coverage   88.59%   88.59%           
=======================================
  Files         363      363           
  Lines      102888   102926   +38     
  Branches   102888   102926   +38     
=======================================
+ Hits        91153    91188   +35     
- Misses       7496     7498    +2     
- Partials     4239     4240    +1     

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

…n a thread

Previously, this used the latest event in the thread as the event to mark as read, while this is not right if we're in a context that hides thread events
@jmartinesp jmartinesp force-pushed the fix/sending-read-receipts-in-threads branch from ef9278f to 635313a Compare November 13, 2025 21:19
…ne::latest_event_id`, instead of `ui::Timeline::latest_event`

This is important because `latest_event` would also return local events, which won't have an event id.
@jmartinesp jmartinesp marked this pull request as ready for review November 14, 2025 10:41
@jmartinesp jmartinesp requested a review from a team as a code owner November 14, 2025 10:41
@jmartinesp jmartinesp requested review from poljar and removed request for a team November 14, 2025 10:41
@jmartinesp jmartinesp changed the title fix(ui): Sending read receipt in thread timelines fix(ui): Sending read receipt in live timeline uses threaded event id instead Nov 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mark as read from notification action doesn't work on thread roots

2 participants