fix(timeline): when removing an item, re-stash its associated reactions

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

@@ -63,9 +63,9 @@ use crate::{
         },
         event_item::{PollState, RemoteEventOrigin, ResponseData},
         item::TimelineUniqueId,
-        reactions::Reactions,
+        reactions::{PendingReaction, Reactions},
         traits::RoomDataProvider,
-        Profile, TimelineItem, TimelineItemKind,
+        Profile, ReactionStatus, TimelineItem, TimelineItemKind,
     },
     unable_to_decrypt_hook::UtdHookManager,
 };
@@ -793,7 +793,39 @@ impl TimelineStateTransaction<'_> {
         if let Some(event_meta) = self.items.all_remote_events().get(event_index) {
             // Fetch the `timeline_item_index` associated to the remote event.
             if let Some(timeline_item_index) = event_meta.timeline_item_index {
-                let _removed_timeline_item = self.items.remove(timeline_item_index);
+                let removed_timeline_item = self.items.remove(timeline_item_index);
+
+                // Restash reactions, if there were any.
+                if let Some(event_item) = removed_timeline_item.as_event() {
+                    if let Some(event_id) = event_item.event_id() {
+                        let target_event = event_id.to_owned();
+
+                        for (key, user_to_reaction) in event_item.reactions().iter() {
+                            for (user_id, reaction_info) in user_to_reaction {
+                                let reaction_event = match &reaction_info.status {
+                                    ReactionStatus::LocalToLocal(_)
+                                    | ReactionStatus::LocalToRemote(_) => continue,
+                                    ReactionStatus::RemoteToRemote(event) => event,
+                                };
+
+                                let pending_reaction = PendingReaction {
+                                    key: key.to_owned(),
+                                    sender_id: user_id.to_owned(),
+                                    timestamp: reaction_info.timestamp,
+                                };
+
+                                self.meta
+                                    .reactions
+                                    .pending
+                                    .entry(target_event.clone())
+                                    .or_default()
+                                    .insert(reaction_event.to_owned(), pending_reaction);
+                            }
+                        }
+                    }
+                }
+
+                // TODO restash poll responses/ends, edits. etc.?
             }
 
             // Now we can remove the remote event.

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

@@ -17,11 +17,15 @@ use std::{sync::Mutex, time::Duration};
 use assert_matches2::{assert_let, assert_matches};
 use eyeball_im::VectorDiff;
 use futures_util::{FutureExt as _, StreamExt as _};
-use matrix_sdk::{assert_next_matches_with_timeout, test_utils::mocks::MatrixMockServer};
-use matrix_sdk_test::{async_test, event_factory::EventFactory, JoinedRoomBuilder, ALICE};
+use matrix_sdk::{
+    assert_next_matches_with_timeout,
+    test_utils::mocks::{MatrixMockServer, RoomMessagesResponseTemplate},
+};
+use matrix_sdk_test::{async_test, event_factory::EventFactory, JoinedRoomBuilder, ALICE, BOB};
 use matrix_sdk_ui::timeline::{ReactionStatus, RoomExt as _};
 use ruma::{event_id, events::room::message::RoomMessageEventContent, room_id};
 use serde_json::json;
+use stream_assert::assert_pending;
 use wiremock::ResponseTemplate;
 
 #[async_test]
@@ -338,3 +342,163 @@ async fn test_local_reaction_to_local_echo() {
     tokio::time::sleep(Duration::from_millis(150)).await;
     assert!(stream.next().now_or_never().is_none());
 }
+
+#[async_test]
+async fn test_lost_reactions_after_deduplication() {
+    // This test checks that after deduplicating events, the reactions attached to
+    // the deduplicated event are not lost.
+
+    let server = MatrixMockServer::new().await;
+    let client = server.client_builder().build().await;
+
+    client.event_cache().subscribe().unwrap();
+    client.event_cache().enable_storage().unwrap();
+
+    let room_id = room_id!("!a98sd12bjh:example.org");
+    let room = server.sync_joined_room(&client, room_id).await;
+
+    server.mock_room_state_encryption().plain().mount().await;
+
+    let timeline = room.timeline().await.unwrap();
+    let (initial_items, mut stream) = timeline.subscribe().await;
+
+    assert!(initial_items.is_empty());
+
+    // A first sync comes with an event and its reaction.
+
+    // A first sync comes, with an event and a reaction to that event.
+    let f = EventFactory::new();
+
+    const REACTION_KEY: &str = "👍";
+    let reaction_target = event_id!("$1");
+    let target_event =
+        f.text_msg("hey").room(room_id).sender(&BOB).event_id(reaction_target).into_raw_timeline();
+    let reaction_event = f
+        .reaction(reaction_target, REACTION_KEY)
+        .sender(&ALICE)
+        .event_id(event_id!("$2"))
+        .into_raw_sync();
+
+    server
+        .sync_room(
+            &client,
+            JoinedRoomBuilder::new(room_id)
+                .add_timeline_event(target_event.clone().cast())
+                .add_timeline_event(reaction_event.clone()),
+        )
+        .await;
+
+    {
+        // Get the event.
+        assert_next_matches_with_timeout!(stream, VectorDiff::PushBack { value: item } => {
+            let item = item.as_event().unwrap();
+            assert_eq!(item.content().as_message().unwrap().body(), "hey");
+            assert!(item.reactions().is_empty());
+            // Has Bob's implicit read receipt.
+            assert_eq!(item.read_receipts().len(), 1);
+        });
+
+        // Get an updated (implicit) read receipt for Alice.
+        assert_next_matches_with_timeout!(stream, VectorDiff::Set { index: 0, value: item } => {
+            let item = item.as_event().unwrap();
+            assert_eq!(item.content().as_message().unwrap().body(), "hey");
+            assert!(item.reactions().is_empty());
+            assert_eq!(item.read_receipts().len(), 2);
+        });
+
+        // Get the reaction.
+        assert_next_matches_with_timeout!(stream, VectorDiff::Set { index: 0, value: item } => {
+            let item = item.as_event().unwrap();
+            assert_eq!(item.content().as_message().unwrap().body(), "hey");
+            let reactions = item.reactions();
+            assert_eq!(reactions.len(), 1);
+            reactions.get(REACTION_KEY).unwrap().get(*ALICE).unwrap();
+        });
+
+        // Good ol' date divider.
+        assert_next_matches_with_timeout!(stream, VectorDiff::PushFront { value: date_divider } => {
+            assert!(date_divider.is_date_divider());
+        });
+
+        // And that's it for now.
+        assert_pending!(stream);
+    }
+
+    // Another sync with fewer events, marked as limited from the server.
+    //
+    // This can happen, with SSS, if the client has been shut down and resurrected,
+    // with a sync that restarted with a small timeline limit.
+    server
+        .sync_room(
+            &client,
+            JoinedRoomBuilder::new(room_id)
+                .add_timeline_event(
+                    f.text_msg("wassup").sender(&ALICE).event_id(event_id!("$1.75")),
+                )
+                .add_timeline_event(reaction_event)
+                .set_timeline_limited()
+                .set_timeline_prev_batch("prev-batch".to_owned()),
+        )
+        .await;
+
+    {
+        // We get the new event.
+        assert_next_matches_with_timeout!(stream, VectorDiff::PushBack { value: item } => {
+            let item = item.as_event().unwrap();
+            assert_eq!(item.content().as_message().unwrap().body(), "wassup");
+        });
+
+        // TODO The reaction gets added again?
+        assert_next_matches_with_timeout!(stream, VectorDiff::Set { index: 1, value: item } => {
+            let item = item.as_event().unwrap();
+            assert_eq!(item.content().as_message().unwrap().body(), "hey");
+            let reactions = item.reactions();
+            assert_eq!(reactions.len(), 1);
+            reactions.get(REACTION_KEY).unwrap().get(*ALICE).unwrap();
+        });
+
+        // No other updates.
+        assert_pending!(stream);
+    }
+
+    // Then we trigger a back-pagination with this token.
+    // It returns the target event, but a new event has been backfilled on the
+    // server in the middle.
+    server
+        .mock_room_messages()
+        .from("prev-batch")
+        .ok(RoomMessagesResponseTemplate::default().events(vec![
+            f.text_msg("hi back!").sender(&ALICE).room(room_id).event_id(event_id!("$1.5")).into(),
+            target_event,
+        ]))
+        .mock_once()
+        .mount()
+        .await;
+
+    let hit_start = timeline.paginate_backwards(20).await.unwrap();
+    assert!(hit_start);
+
+    {
+        // The duplicate event is removed…
+        assert_next_matches_with_timeout!(stream, VectorDiff::Remove { index: 1 });
+
+        // The duplicated event shows up.
+        assert_next_matches_with_timeout!(stream, VectorDiff::Insert { index: 1, value: item } => {
+            let item = item.as_event().unwrap();
+            assert_eq!(item.content().as_message().unwrap().body(), "hey");
+            // And still has a reaction from Alice.
+            let reactions = item.reactions();
+            assert_eq!(reactions.len(), 1);
+            reactions.get(REACTION_KEY).unwrap().get(*ALICE).unwrap();
+        });
+
+        // The new event shows up.
+        assert_next_matches_with_timeout!(stream, VectorDiff::Insert { index: 2, value: item } => {
+            let item = item.as_event().unwrap();
+            assert_eq!(item.content().as_message().unwrap().body(), "hi back!");
+        });
+
+        // No other updates.
+        assert_pending!(stream);
+    }
+}