From e158e8abc0fbc2cfc41d8f389ca1f77e9158efef Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 24 Sep 2025 11:35:36 +0200 Subject: [PATCH] refactor(timeline): in thread permalinks, avoid back-paginating if the root event is part of the /context response For thread permalinks, we start with a /context query that will load the focused event, and maybe a few other in-thread events. In fact, it can also include the thread root event, which was excluded before. Instead, we would get a previous-token for back-paginations, which would be used in /relations. When the request to /relations returns an empty previous token, that means we've reached the start of the thread, and in this case we would manually load the root with /event. We can do better, if the root event is part of the initial /context response: skip the back-paginations altogether, and make sure to include the root event in `init_focus()`. --- .../src/timeline/controller/mod.rs | 34 ++++-- .../src/tests/timeline.rs | 112 +++++++++++++++++- 2 files changed, 135 insertions(+), 11 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index 2497e0120..1213c6c4d 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -25,7 +25,7 @@ use matrix_sdk::Result; use matrix_sdk::{ deserialized_responses::TimelineEvent, event_cache::{RoomEventCache, RoomPaginationStatus}, - paginators::{PaginationResult, Paginator}, + paginators::{PaginationResult, PaginationToken, Paginator}, send_queue::{ LocalEcho, LocalEchoContent, RoomSendQueueUpdate, SendHandle, SendReactionHandle, }, @@ -474,12 +474,14 @@ impl TimelineController

{ let event_paginator = Paginator::new(self.room_data_provider.clone()); - // Start a /context request so we can know if the event is in a thread or not + // Start a /context request so we can know if the event is in a thread or not, + // and know which kind of pagination we'll be using then. let start_from_result = event_paginator .start_from(event_id, (*num_context_events).into()) .await .map_err(PaginationError::Paginator)?; + // Find the target event, and see if it's part of a thread. let thread_root_event_id = start_from_result .events .iter() @@ -492,13 +494,28 @@ impl TimelineController

{ let _ = paginator.set(match thread_root_event_id { Some(root_id) => { - let tokens = event_paginator.tokens(); + let mut tokens = event_paginator.tokens(); + + // Look if the thread root event is part of the /context response. This + // allows us to spare some backwards pagination with + // /relations. + let includes_root_event = start_from_result.events.iter().any(|event| { + if let Some(id) = event.event_id() { id == root_id } else { false } + }); + + if includes_root_event { + // If we have the root event, there's no need to do back-paginations + // with /relations, since we are at the start of the thread. + tokens.previous = PaginationToken::HitEnd; + } + AnyPaginator::Threaded(ThreadedEventsLoader::new( self.room_data_provider.clone(), root_id, tokens, )) } + None => AnyPaginator::Unthreaded { paginator: event_paginator, hide_threaded_events: *hide_threaded_events, @@ -518,12 +535,13 @@ impl TimelineController

{ } AnyPaginator::Threaded(threaded_events_loader) => { - // We filter only events that are part of the thread, since /context will - // return adjacent events without filters. + // We filter only events that are part of the thread (including the root), + // since /context will return adjacent events without filters. + let thread_root = threaded_events_loader.thread_root_event_id(); let events_in_thread = events.into_iter().filter(|event| { - extract_thread_root(event.raw()).is_some_and(|thread_root| { - thread_root == threaded_events_loader.thread_root_event_id() - }) + extract_thread_root(event.raw()) + .is_some_and(|event_thread_root| event_thread_root == thread_root) + || event.event_id().as_deref() == Some(thread_root) }); self.replace_with_initial_remote_events( diff --git a/testing/matrix-sdk-integration-testing/src/tests/timeline.rs b/testing/matrix-sdk-integration-testing/src/tests/timeline.rs index f007e9221..0d4ebbc9d 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/timeline.rs +++ b/testing/matrix-sdk-integration-testing/src/tests/timeline.rs @@ -26,22 +26,35 @@ use matrix_sdk::{ config::SyncSettings, deserialized_responses::{VerificationLevel, VerificationState}, encryption::{EncryptionSettings, backups::BackupState}, - room::edit::EditedContent, + room::{ + edit::EditedContent, + reply::{EnforceThread, Reply}, + }, ruma::{ MilliSecondsSinceUnixEpoch, OwnedEventId, RoomId, UserId, api::client::room::create_room::v3::{Request as CreateRoomRequest, RoomPreset}, events::{ InitialStateEvent, - room::{encryption::RoomEncryptionEventContent, message::RoomMessageEventContent}, + room::{ + encryption::RoomEncryptionEventContent, + message::{ + ReplyWithinThread, RoomMessageEventContent, + RoomMessageEventContentWithoutRelation, + }, + }, }, }, }; +use matrix_sdk_test::TestResult; use matrix_sdk_ui::{ Timeline, notification_client::NotificationClient, room_list_service::RoomListLoadingState, sync_service::SyncService, - timeline::{EventSendState, EventTimelineItem, ReactionStatus, RoomExt, TimelineItem}, + timeline::{ + EventSendState, EventTimelineItem, ReactionStatus, RoomExt, TimelineBuilder, TimelineFocus, + TimelineItem, + }, }; use similar_asserts::assert_eq; use stream_assert::assert_pending; @@ -910,3 +923,96 @@ async fn test_new_users_first_messages_dont_warn_about_insecure_device_if_it_is_ assert_eq!(*index, 10); } } + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn test_thread_focused_timeline() -> TestResult { + // Set up sync for user Alice, and create a room. + let alice = TestClientBuilder::new("alice").use_sqlite().build().await?; + + let alice_clone = alice.clone(); + let alice_sync = spawn(async move { + alice_clone.sync(Default::default()).await.expect("sync failed for alice!"); + }); + + // Set up sync for another user Bob. + let bob = TestClientBuilder::new("bob").use_sqlite().build().await?; + + // Enable Bob's event cache, to speed up creating the in-thread reply event. + bob.event_cache().subscribe().unwrap(); + + let bob_clone = bob.clone(); + let bob_sync = spawn(async move { + bob_clone.sync(Default::default()).await.expect("sync failed for bob!"); + }); + + debug!("Creating room…"); + let alice_room = alice + .create_room(assign!(CreateRoomRequest::new(), { + is_direct: true, + })) + .await?; + + alice_room.invite_user_by_id(bob.user_id().unwrap()).await?; + + // Bob joins the room. + + let bob_room = loop { + if let Some(room) = bob.get_room(alice_room.room_id()) { + room.join().await?; + break room; + } + sleep(Duration::from_millis(10)).await; + }; + + // Bob sends messages in a thread. + let resp = bob_room.send(RoomMessageEventContent::text_plain("Root message")).await?; + let thread_root = resp.event_id; + + let thread_reply_event_content = bob_room + .make_reply_event( + RoomMessageEventContentWithoutRelation::text_plain("First reply"), + Reply { + event_id: thread_root.clone(), + enforce_thread: EnforceThread::Threaded(ReplyWithinThread::No), + }, + ) + .await?; + + let thread_reply_event_id = bob_room.send(thread_reply_event_content).await?.event_id; + + // Alice creates a timeline focused on the in-thread event, so this will use + // /context, and the thread root will be part of the response. + let timeline = TimelineBuilder::new(&alice_room) + .with_focus(TimelineFocus::Event { + target: thread_reply_event_id.clone(), + num_context_events: 42, + hide_threaded_events: false, + }) + .build() + .await?; + + // The timeline should be focused on the thread reply, so it should be + // considered threaded. + assert!(timeline.is_threaded()); + + // Observe that the timeline contains both the root and the reply. + let (items, mut stream) = timeline.subscribe().await; + + assert_eq!(items.len(), 3); + assert!(items[0].is_date_divider()); + assert_eq!(items[1].as_event().unwrap().event_id().unwrap(), thread_root); + assert_eq!(items[2].as_event().unwrap().event_id().unwrap(), thread_reply_event_id); + + // If Alice paginates backwards, nothing happens on the timeline, as we've hit + // the start in the /context response. + let hit_start = timeline.paginate_backwards(10).await?; + assert!(hit_start); + + sleep(Duration::from_millis(100)).await; + assert_pending!(stream); + + alice_sync.abort(); + bob_sync.abort(); + + Ok(()) +}