timeline: when deduplicating event meta, keep the most recent instead of the oldest

Not doing this leads to an invalid ordering of events, as shown in the
test: if we increase the timeline limit of a room using sliding sync,
we'll receive a duplicate event, and if we ignore it, it'll be in an
invalid position. The solution is to keep the most recent event (and
remove the old one from event_meta).

Author: bnjbvr

crates/matrix-sdk-ui/src/timeline/inner/state.rs

@@ -664,26 +664,35 @@ impl TimelineInnerStateTransaction<'_> {
         room_data_provider: &P,
         settings: &TimelineInnerSettings,
     ) -> bool {
-        // Detect if an event already exists in [`TimelineInnerMeta::all_events`]
-        fn event_already_exists(new_event_id: &EventId, all_events: &VecDeque<EventMeta>) -> bool {
-            all_events.iter().any(|EventMeta { event_id, .. }| event_id == new_event_id)
+        // Detect if an event already exists in [`TimelineInnerMeta::all_events`].
+        //
+        // Returns its position, in this case.
+        fn event_already_exists(
+            new_event_id: &EventId,
+            all_events: &VecDeque<EventMeta>,
+        ) -> Option<usize> {
+            all_events.iter().position(|EventMeta { event_id, .. }| event_id == new_event_id)
         }
 
         match position {
             TimelineItemPosition::Start { .. } => {
-                if event_already_exists(event_meta.event_id, &self.meta.all_events) {
-                    return false;
+                if let Some(pos) = event_already_exists(event_meta.event_id, &self.meta.all_events)
+                {
+                    self.meta.all_events.remove(pos);
                 }
 
                 self.meta.all_events.push_front(event_meta.base_meta())
             }
+
             TimelineItemPosition::End { .. } => {
-                if event_already_exists(event_meta.event_id, &self.meta.all_events) {
-                    return false;
+                if let Some(pos) = event_already_exists(event_meta.event_id, &self.meta.all_events)
+                {
+                    self.meta.all_events.remove(pos);
                 }
 
                 self.meta.all_events.push_back(event_meta.base_meta());
             }
+
             #[cfg(feature = "e2e-encryption")]
             TimelineItemPosition::Update(_) => {
                 if let Some(event) =

crates/matrix-sdk-ui/tests/integration/timeline/mod.rs

@@ -28,7 +28,8 @@ use matrix_sdk_test::{
 };
 use matrix_sdk_ui::timeline::{EventSendState, RoomExt, TimelineItemContent, VirtualTimelineItem};
 use ruma::{
-    events::room::message::RoomMessageEventContent, room_id, user_id, MilliSecondsSinceUnixEpoch,
+    event_id, events::room::message::RoomMessageEventContent, room_id, user_id,
+    MilliSecondsSinceUnixEpoch,
 };
 use serde_json::json;
 use wiremock::{
@@ -442,3 +443,67 @@ async fn test_sync_highlighted() {
     // `m.room.tombstone` should be highlighted by default.
     assert!(remote_event.is_highlighted());
 }
+
+#[async_test]
+async fn test_duplicate_maintains_correct_order() {
+    let room_id = room_id!("!a98sd12bjh:example.org");
+    let (client, server) = logged_in_client_with_server().await;
+    let sync_settings = SyncSettings::new().timeout(Duration::from_millis(3000));
+
+    let mut sync_builder = SyncResponseBuilder::new();
+    sync_builder.add_joined_room(JoinedRoomBuilder::new(room_id));
+
+    mock_sync(&server, sync_builder.build_json_sync_response(), None).await;
+    let _response = client.sync_once(sync_settings.clone()).await.unwrap();
+    server.reset().await;
+
+    let room = client.get_room(room_id).unwrap();
+    let timeline = room.timeline().await.unwrap();
+
+    // At the beginning, the timeline is empty.
+    assert!(timeline.items().await.is_empty());
+
+    let f = EventFactory::new().sender(user_id!("@a:b.c"));
+
+    // We receive an event F, from a sliding sync with timeline limit=1.
+    sync_builder.add_joined_room(
+        JoinedRoomBuilder::new(room_id)
+            .add_timeline_event(f.text_msg("C").event_id(event_id!("$c")).into_raw_sync()),
+    );
+
+    mock_sync(&server, sync_builder.build_json_sync_response(), None).await;
+    let _response = client.sync_once(sync_settings.clone()).await.unwrap();
+    server.reset().await;
+
+    // The timeline item represents the message we just received.
+    let items = timeline.items().await;
+    assert_eq!(items.len(), 2);
+
+    assert!(items[0].is_day_divider());
+    let content = items[1].as_event().unwrap().content().as_message().unwrap().body();
+    assert_eq!(content, "C");
+
+    // We receive multiple events, and C is now the last one (because we supposedly
+    // increased the timeline limit).
+    sync_builder.add_joined_room(
+        JoinedRoomBuilder::new(room_id)
+            .add_timeline_event(f.text_msg("A").event_id(event_id!("$a")).into_raw_sync())
+            .add_timeline_event(f.text_msg("B").event_id(event_id!("$b")).into_raw_sync())
+            .add_timeline_event(f.text_msg("C").event_id(event_id!("$c")).into_raw_sync()),
+    );
+
+    mock_sync(&server, sync_builder.build_json_sync_response(), None).await;
+    let _response = client.sync_once(sync_settings.clone()).await.unwrap();
+    server.reset().await;
+
+    let items = timeline.items().await;
+    assert_eq!(items.len(), 4, "{items:?}");
+
+    assert!(items[0].is_day_divider());
+    let content = items[1].as_event().unwrap().content().as_message().unwrap().body();
+    assert_eq!(content, "A");
+    let content = items[2].as_event().unwrap().content().as_message().unwrap().body();
+    assert_eq!(content, "B");
+    let content = items[3].as_event().unwrap().content().as_message().unwrap().body();
+    assert_eq!(content, "C");
+}

crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs

@@ -416,6 +416,10 @@ async fn test_timeline_duplicated_events() -> Result<()> {
         assert_timeline_stream! {
             [timeline_stream]
             update[3] "$x3:bar.org";
+            update[1] "$x1:bar.org";
+            remove[1];
+            append    "$x1:bar.org";
+            update[3] "$x1:bar.org";
             append    "$x4:bar.org";
         };
     }