Skip to content

Conversation

stefanceriu
Copy link
Member

This patch updates the Timeline Controller's handle_explicit_read_receipts method to also consider unthreaded read receipts on threaded main timelines when calculating timeline items states and adds a test for it.

This picks up from #5442 and fixes #5440

…:Main` timelines when computing timeline items states.
Copy link

codecov bot commented Aug 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.57%. Comparing base (a9ce1c6) to head (d8daecf).
⚠️ Report is 20 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5527      +/-   ##
==========================================
- Coverage   88.57%   88.57%   -0.01%     
==========================================
  Files         340      340              
  Lines       93694    93694              
  Branches    93694    93694              
==========================================
- Hits        82989    82988       -1     
- Misses       6573     6574       +1     
  Partials     4132     4132              

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

Copy link

codspeed-hq bot commented Aug 13, 2025

CodSpeed Performance Report

Merging #5527 will not alter performance

Comparing unthreadedReadReceiptsOnThreadedClientsTheRevenge (d8daecf) with main (b3c53dd)

Summary

✅ 31 untouched benchmarks

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, thanks! Small proposal to change the test (or if you prefer, add a new one), and that shouldn't require any changes in the timeline's code, if I understand correctly

Comment on lines +840 to +843
// Alice sends 2 messages
let f = &timeline.factory;
timeline.handle_live_event(f.text_msg("A").sender(*ALICE)).await;
timeline.handle_live_event(f.text_msg("B").sender(*ALICE).event_id(event_b)).await;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious to propose a small change in this test, that I think wouldn't require a change in the rest of the code, in theory :)

Let's add a third event with id event_c, that's sent by Alice in a thread.

This event should be filtered out, because the timeline is set to hide_threaded_events.

Now, make Bob's read receipt point to event_c.

I think the outcome should remain the same, and the read receipt on event_b should be set to Bob's.

(This enhanced version proves that, even if the unthreaded read receipt is on an in-thread event, we can put it "at the right position" in the no-threaded-events timeline.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! Added it in 875ec25, fits right in 👍

…eipts test case with a threaded message and read receipt.
@bnjbvr bnjbvr enabled auto-merge (squash) August 14, 2025 05:12
@bnjbvr bnjbvr merged commit 140e751 into main Aug 14, 2025
48 checks passed
@bnjbvr bnjbvr deleted the unthreadedReadReceiptsOnThreadedClientsTheRevenge branch August 14, 2025 05:24
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.

The timeline doesn't consider unthreaded read receipts on thread aware instances
2 participants