Skip to content

feat(ui): consider unthreaded read receipts when computing the user's latest on thread aware timeline instances #5442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
+283 −116
Closed

feat(ui): consider unthreaded read receipts when computing the user's latest on thread aware timeline instances #5442

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6069e67
chore(ui): add failing tests when computing read receipts from unthre…
stefanceriu Jul 24, 2025
d32c187
feat(ui): consider unthreaded read receipts when computing the user's…
stefanceriu Jul 24, 2025
64857e9
Address PR comments, keep `latest_user_read_receipt` only on `Receipt…
stefanceriu Jul 25, 2025
8fb6356
feat(timeline): consider unthreaded read receipts for `ReceiptThread:…
stefanceriu Jul 25, 2025
7af3bbf
chore(ui): make `latest_user_read_receipt` rely on the timeline for f…
stefanceriu Jul 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 53 additions & 3 deletions crates/matrix-sdk-ui/src/timeline/controller/read_receipts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,7 @@ impl<P: RoomDataProvider> TimelineState<P> {
) -> Option<(OwnedEventId, Receipt)> {
let all_remote_events = self.items.all_remote_events();

let public_read_receipt = self
let threaded_public_read_receipt = self
.meta
.user_receipt(
user_id,
Expand All @@ -721,17 +721,67 @@ impl<P: RoomDataProvider> TimelineState<P> {
)
.await;

let private_read_receipt = self
// If the current client is thread aware, we maintain maximal
// compatibility with other clients using either unthreaded or
// main-thread read receipts by allowing both here and taking the most
// recent one.
let unthreaded_public_read_receipt = self
.meta
.user_receipt(
user_id,
ReceiptType::Read,
ReceiptThread::Unthreaded,
Copy link
Member

Choose a reason for hiding this comment

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

I think this is wrong, because the bound receipt_thread variable can be Unthreaded as well, in which case we're computing the same thing twice. Also, a thread-focused timeline should not take into account the unthreaded receipt at all, as it's not even certain that the event pointed to by the unthreaded receipt belongs to the thread or not. (So there are more chances it can't be ordered relative to the correct thread receipt at all, since the unthreaded receipt more likely points to another event of another thread.)

Copy link
Member

Choose a reason for hiding this comment

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

If the meaning of an unthreaded receipt is "I have read everything before this" then I argue an unthreaded receipt should be considered when deciding the unread state of a thread. Otherwise, if I switch from an unthreaded client to a threaded one, all threads from all time will be unread.

On the other hand, in practice, if you flip back to an unthreaded client, read the latest messages and then go back to a threaded one, all your threads will be read, so this might need us to be more pragmatic and follow what you are suggesting Benji - I don't know.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, the read receipts in the timeline are computed locally, using only the current timeline's events. To know if an event is read, we compare the position of the event timeline item pointed to by the receipt, and the positions of event timeline items. It's more likely that the unthreaded receipt points to an event that's outside a threaded timeline, so it won't be found by the timeline code. As a result, it's not possible, in the current form of the code, to figure that a thread is fully read or not read, based on the threaded timeline. All the information is around, though, so we could compute this information (on a user basis), with some extra effort.

room_data_provider,
all_remote_events,
)
.await;

let public_read_receipt = match self.meta.compare_optional_receipts(
threaded_public_read_receipt.as_ref(),
unthreaded_public_read_receipt.as_ref(),
all_remote_events,
) {
Ordering::Greater => threaded_public_read_receipt,
Ordering::Less => unthreaded_public_read_receipt,
_ => unreachable!(),
};

let threaded_private_read_receipt = self
.meta
.user_receipt(
user_id,
ReceiptType::ReadPrivate,
receipt_thread,
receipt_thread.clone(),
room_data_provider,
all_remote_events,
)
.await;

// If the current client is thread aware, we maintain maximal
// compatibility with other clients using either unthreaded or
// main-thread read receipts by allowing both here and taking the most
// recent one.
let unthreaded_private_read_receipt = self
.meta
.user_receipt(
user_id,
ReceiptType::ReadPrivate,
ReceiptThread::Unthreaded,
room_data_provider,
all_remote_events,
)
.await;

let private_read_receipt = match self.meta.compare_optional_receipts(
threaded_private_read_receipt.as_ref(),
unthreaded_private_read_receipt.as_ref(),
all_remote_events,
) {
Ordering::Greater => threaded_private_read_receipt,
Ordering::Less => unthreaded_private_read_receipt,
_ => unreachable!(),
};

// Let's assume that a private read receipt should be more recent than a public
// read receipt (otherwise there's no point in the private read receipt),
// and use it as the default.
Expand Down
104 changes: 104 additions & 0 deletions crates/matrix-sdk-ui/src/timeline/tests/read_receipts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,58 @@ async fn test_initial_public_main_thread_receipt() {
assert_eq!(receipt_event_id, event_id);
}

#[async_test]
async fn test_initial_public_unthreaded_receipt_main_threaded_timeline() {
let event_id = owned_event_id!("$event_with_receipt");

// Add initial private receipt on the main thread.
let mut initial_user_receipts = ReadReceiptMap::new();
initial_user_receipts
.entry(ReceiptType::Read)
.or_default()
.entry(ReceiptThread::Unthreaded)
.or_default()
.insert(
ALICE.to_owned(),
(event_id.clone(), Receipt::new(ruma::MilliSecondsSinceUnixEpoch(uint!(10)))),
);

let timeline = TestTimelineBuilder::new()
.focus(TimelineFocus::Live { hide_threaded_events: true })
.provider(TestRoomDataProvider::default().with_initial_user_receipts(initial_user_receipts))
.settings(TimelineSettings { track_read_receipts: true, ..Default::default() })
.build();

let (receipt_event_id, _) = timeline.controller.latest_user_read_receipt(*ALICE).await.unwrap();
assert_eq!(receipt_event_id, event_id);
}

#[async_test]
async fn test_initial_public_unthreaded_receipt_other_threaded_timeline() {
let event_id = owned_event_id!("$event_with_receipt");

// Add initial private receipt on the main thread.
let mut initial_user_receipts = ReadReceiptMap::new();
initial_user_receipts
.entry(ReceiptType::Read)
.or_default()
.entry(ReceiptThread::Unthreaded)
.or_default()
.insert(
ALICE.to_owned(),
(event_id.clone(), Receipt::new(ruma::MilliSecondsSinceUnixEpoch(uint!(10)))),
);

let timeline = TestTimelineBuilder::new()
.focus(TimelineFocus::Thread { root_event_id: owned_event_id!("$some_thread_root") })
.provider(TestRoomDataProvider::default().with_initial_user_receipts(initial_user_receipts))
.settings(TimelineSettings { track_read_receipts: true, ..Default::default() })
.build();

let (receipt_event_id, _) = timeline.controller.latest_user_read_receipt(*ALICE).await.unwrap();
assert_eq!(receipt_event_id, event_id);
}

#[async_test]
async fn test_initial_private_unthreaded_receipt() {
let event_id = owned_event_id!("$event_with_receipt");
Expand Down Expand Up @@ -610,6 +662,58 @@ async fn test_initial_private_main_thread_receipt() {
assert_eq!(receipt_event_id, event_id);
}

#[async_test]
async fn test_initial_private_unthreaded_receipt_main_threaded_timeline() {
let event_id = owned_event_id!("$event_with_receipt");

// Add initial private receipt on the main thread.
let mut initial_user_receipts = ReadReceiptMap::new();
initial_user_receipts
.entry(ReceiptType::ReadPrivate)
.or_default()
.entry(ReceiptThread::Unthreaded)
.or_default()
.insert(
ALICE.to_owned(),
(event_id.clone(), Receipt::new(ruma::MilliSecondsSinceUnixEpoch(uint!(10)))),
);

let timeline = TestTimelineBuilder::new()
.focus(TimelineFocus::Live { hide_threaded_events: true })
.provider(TestRoomDataProvider::default().with_initial_user_receipts(initial_user_receipts))
.settings(TimelineSettings { track_read_receipts: true, ..Default::default() })
.build();

let (receipt_event_id, _) = timeline.controller.latest_user_read_receipt(*ALICE).await.unwrap();
assert_eq!(receipt_event_id, event_id);
}

#[async_test]
async fn test_initial_private_unthreaded_receipt_other_threaded_timeline() {
let event_id = owned_event_id!("$event_with_receipt");

// Add initial private receipt on the main thread.
let mut initial_user_receipts = ReadReceiptMap::new();
initial_user_receipts
.entry(ReceiptType::ReadPrivate)
.or_default()
.entry(ReceiptThread::Unthreaded)
.or_default()
.insert(
ALICE.to_owned(),
(event_id.clone(), Receipt::new(ruma::MilliSecondsSinceUnixEpoch(uint!(10)))),
);

let timeline = TestTimelineBuilder::new()
.focus(TimelineFocus::Thread { root_event_id: owned_event_id!("$some_thread_root") })
.provider(TestRoomDataProvider::default().with_initial_user_receipts(initial_user_receipts))
.settings(TimelineSettings { track_read_receipts: true, ..Default::default() })
.build();

let (receipt_event_id, _) = timeline.controller.latest_user_read_receipt(*ALICE).await.unwrap();
assert_eq!(receipt_event_id, event_id);
}

#[async_test]
async fn test_clear_read_receipts() {
let room_id = room_id!("!room:localhost");
Expand Down
Loading