fix(sdk): Respect server ordering for remote echoes

crates/matrix-sdk/src/room/timeline/event_handler.rs

@@ -531,21 +531,29 @@ impl<'a, 'i> TimelineEventHandler<'a, 'i> {
                     {
                         // TODO: Check whether anything is different about the
                         //       old and new item?
-                        self.timeline_items.set_cloned(idx, item);
-                        return;
+                        // Remove local echo, remote echo will be added below
+                        self.timeline_items.remove(idx);
                     } else {
                         warn!(
                             "Received event with transaction ID, but didn't \
                              find matching timeline item"
                         );
                     }
-                }
-
-                if let Some((idx, old_item)) = find_event_by_id(self.timeline_items, event_id) {
+                } else if let Some((idx, _old_item)) =
+                    find_local_echo_by_event_id(self.timeline_items, event_id)
+                {
                     // This occurs very often right now due to a sliding-sync
                     // bug: https://github.com/matrix-org/sliding-sync/issues/3
-                    // TODO: Use warn log level once that bug is fixed.
-                    trace!(
+                    // TODO: Remove this branch once the bug is fixed?
+                    trace!("Received remote echo without transaction ID");
+                    self.timeline_items.remove(idx);
+                } else if let Some((idx, old_item)) =
+                    find_event_by_id(self.timeline_items, event_id)
+                {
+                    // TODO: Remove for better performance? Doing another scan
+                    // over all the items if the event is not a remote echo is
+                    // slow.
+                    warn!(
                         ?item,
                         ?old_item,
                         "Received event with an ID we already have a timeline item for"
@@ -677,6 +685,20 @@ fn _update_timeline_item(
     }
 }
 
+fn find_local_echo_by_event_id<'a>(
+    items: &'a [Arc<TimelineItem>],
+    event_id: &EventId,
+) -> Option<(usize, &'a EventTimelineItem)> {
+    items
+        .iter()
+        .enumerate()
+        .filter_map(|(idx, item)| Some((idx, item.as_event()?)))
+        // Note: not using event_id() method as this is only supposed to find
+        // local echoes, where the event ID is stored in the separate field
+        // rather than the key.
+        .rfind(|(_, it)| it.event_id.as_deref() == Some(event_id))
+}
+
 /// Converts a timestamp since Unix Epoch to a local date and time.
 fn timestamp_to_local_datetime(ts: MilliSecondsSinceUnixEpoch) -> DateTime<Local> {
     Local

crates/matrix-sdk/src/room/timeline/mod.rs

@@ -415,25 +415,27 @@ impl TimelineItem {
 // FIXME: Put an upper bound on timeline size or add a separate map to look up
 // the index of a timeline item by its key, to avoid large linear scans.
 fn find_event_by_id<'a>(
-    lock: &'a [Arc<TimelineItem>],
+    items: &'a [Arc<TimelineItem>],
     event_id: &EventId,
 ) -> Option<(usize, &'a EventTimelineItem)> {
-    lock.iter()
+    items
+        .iter()
         .enumerate()
         .filter_map(|(idx, item)| Some((idx, item.as_event()?)))
         .rfind(|(_, it)| it.event_id() == Some(event_id))
 }
 
 fn find_event_by_txn_id<'a>(
-    lock: &'a [Arc<TimelineItem>],
+    items: &'a [Arc<TimelineItem>],
     txn_id: &TransactionId,
 ) -> Option<(usize, &'a EventTimelineItem)> {
-    lock.iter()
+    items
+        .iter()
         .enumerate()
         .filter_map(|(idx, item)| Some((idx, item.as_event()?)))
         .rfind(|(_, it)| it.key == *txn_id)
 }
 
-fn find_read_marker(lock: &[Arc<TimelineItem>]) -> Option<usize> {
-    lock.iter().rposition(|item| item.is_read_marker())
+fn find_read_marker(items: &[Arc<TimelineItem>]) -> Option<usize> {
+    items.iter().rposition(|item| item.is_read_marker())
 }

crates/matrix-sdk/src/room/timeline/tests.rs

@@ -412,8 +412,67 @@ async fn remote_echo_without_txn_id() {
         }))
         .await;
 
-    let item =
-        assert_matches!(stream.next().await, Some(VecDiff::UpdateAt { value, index: 1 }) => value);
+    // The local echo is removed
+    assert_matches!(stream.next().await, Some(VecDiff::Pop { .. }));
+    // This day divider shouldn't be present, or the previous one should be
+    // removed. There being a two day dividers in a row is a bug, but it's
+    // non-trivial to fix and rare enough that we can fix it later (only happens
+    // when the first message on a given day is a local echo).
+    let _day_divider = assert_matches!(stream.next().await, Some(VecDiff::Push { value }) => value);
+
+    // … and the remote echo is added.
+    let item = assert_matches!(stream.next().await, Some(VecDiff::Push { value }) => value);
+    assert_matches!(item.as_event().unwrap().key(), TimelineKey::EventId(_));
+}
+
+#[async_test]
+async fn remote_echo_new_position() {
+    let timeline = TestTimeline::new();
+    let mut stream = timeline.stream();
+
+    // Given a local event…
+    let txn_id = timeline
+        .handle_local_event(AnyMessageLikeEventContent::RoomMessage(
+            RoomMessageEventContent::text_plain("echo"),
+        ))
+        .await;
+
+    let _day_divider = assert_matches!(stream.next().await, Some(VecDiff::Push { value }) => value);
+
+    let item = assert_matches!(stream.next().await, Some(VecDiff::Push { value }) => value);
+    let txn_id_from_event = assert_matches!(
+        item.as_event().unwrap().key(),
+        TimelineKey::TransactionId(txn_id) => txn_id
+    );
+    assert_eq!(txn_id, *txn_id_from_event);
+
+    // … and another event that comes back before the remote echo
+    timeline.handle_live_message_event(&BOB, RoomMessageEventContent::text_plain("test")).await;
+    let _day_divider = assert_matches!(stream.next().await, Some(VecDiff::Push { value }) => value);
+    let _bob_message = assert_matches!(stream.next().await, Some(VecDiff::Push { value }) => value);
+
+    // When the remote echo comes in…
+    timeline
+        .handle_live_custom_event(json!({
+            "content": {
+                "body": "echo",
+                "msgtype": "m.text",
+            },
+            "sender": &*ALICE,
+            "event_id": "$eeG0HA0FAZ37wP8kXlNkxx3I",
+            "origin_server_ts": 6,
+            "type": "m.room.message",
+            "unsigned": {
+                "transaction_id": txn_id,
+            },
+        }))
+        .await;
+
+    // … the local echo should be removed
+    assert_matches!(stream.next().await, Some(VecDiff::RemoveAt { index: 1 }));
+
+    // … and the remote echo added
+    let item = assert_matches!(stream.next().await, Some(VecDiff::Push { value }) => value);
     assert_matches!(item.as_event().unwrap().key(), TimelineKey::EventId(_));
 }
 

crates/matrix-sdk/tests/integration/room/timeline.rs

@@ -227,10 +227,15 @@ async fn echo() {
     let _response = client.sync_once(sync_settings.clone()).await.unwrap();
     server.reset().await;
 
-    let remote_echo = assert_matches!(
-        timeline_stream.next().await,
-        Some(VecDiff::UpdateAt { index: 1, value }) => value
-    );
+    // Local echo is removed
+    assert_matches!(timeline_stream.next().await, Some(VecDiff::Pop { .. }));
+    // Bug, will be fixed later. See comment in remote_echo_without_txn_id test
+    // from `room::timeline::tests`.
+    let _day_divider =
+        assert_matches!(timeline_stream.next().await, Some(VecDiff::Push { value }) => value);
+
+    let remote_echo =
+        assert_matches!(timeline_stream.next().await, Some(VecDiff::Push { value }) => value);
     let item = remote_echo.as_event().unwrap();
     assert!(item.event_id().is_some());
     assert!(item.is_own());