ios, android, desktop: do not add loaded items of another chat to the opened chat (#7450)

* ios, android, desktop: do not add loaded items of another chat to the opened chat

apiLoadMessages could apply a loaded page to a chat it was not loaded for,
mixing messages of two chats in one list:

- the chat id was not checked at all for .last pagination, and the loading
  coroutine is not cancelled when the chat is closed (PreloadItems wraps it
  into NonCancellable, apiFindMessages runs in a scope of its own), so a .last
  page loaded for the previously opened chat was appended to the items of the
  chat opened while it was loading;
- for the other paginations the chat id was checked before the items were
  loaded, but they are applied on the main thread after a dispatch, so the chat
  could be switched in between.

Check the chat id (and the remote host id in kotlin) right before the items are
applied, in every pagination except .initial and opening around item that set
chat id themselves. Empty chatItems remain allowed for .last that is used for
searching.

* android, desktop: test that loaded items of another chat are not added to the opened chat

Reproduces the bug deterministically, without depending on the timing of the
race: processLoadedChat is called with a page loaded for another chat while the
opened chat has its own items.

Without the fix the items of the two chats end up in one list:

    lastPageLoadedForAnotherChatIsNotAddedToOpenedChat
      expected:<[101, 102]> but was:<[101, 102, 201, 202]>
    beforePageLoadedForAnotherChatIsNotAddedToOpenedChat
      expected:<[101, 102]> but was:<[201, 202, 101, 102]>
    aroundPageLoadedForAnotherChatIsNotAddedToOpenedChat
      expected:<[101, 102]> but was:<[101, 102, 201, 202]>

apps/ios/Shared/Views/Chat/ChatItemsLoader.swift

@@ -31,11 +31,11 @@ func apiLoadMessages(
 
     let chatModel = ChatModel.shared
 
-    // For .initial allow the chatItems to be empty as well as chatModel.chatId to not match this chat because these values become set after .initial finishes
     let paginationIsInitial = switch pagination { case .initial: true; default: false }
     let paginationIsLast = switch pagination { case .last: true; default: false }
-    // When openAroundItemId is provided, chatId can be different too
-    if ((chatModel.chatId != chat.id || chat.chatItems.isEmpty) && !paginationIsInitial && !paginationIsLast && openAroundItemId == nil) || Task.isCancelled {
+    // For .initial allow the chatItems to be empty, as well as for .last that is used for searching
+    let allowEmptyItems = paginationIsInitial || paginationIsLast || openAroundItemId != nil
+    if chatWasSwitched(chat, pagination, openAroundItemId) || (!allowEmptyItems && chat.chatItems.isEmpty) || Task.isCancelled {
         return
     }
 
@@ -79,6 +79,7 @@ func apiLoadMessages(
         newItems.insert(contentsOf: chat.chatItems, at: insertAt)
         let newReversed: [ChatItem] = newItems.reversed()
         await MainActor.run {
+            if chatWasSwitched(chat, pagination, openAroundItemId) { return }
             im.reversedChatItems = newReversed
             im.chatState.splits = modifiedSplits.newSplits
             im.chatState.moveUnreadAfterItem(modifiedSplits.oldUnreadSplitIndex, modifiedSplits.newUnreadSplitIndex, oldItems)
@@ -99,6 +100,7 @@ func apiLoadMessages(
         let new: [ChatItem] = newItems
         let newReversed: [ChatItem] = newItems.reversed()
         await MainActor.run {
+            if chatWasSwitched(chat, pagination, openAroundItemId) { return }
             im.reversedChatItems = newReversed
             im.chatState.splits = newSplits
             im.chatState.moveUnreadAfterItem(im.chatState.splits.first ?? new.last!.id, new)
@@ -122,6 +124,7 @@ func apiLoadMessages(
         let newReversed: [ChatItem] = newItems.reversed()
         let orderedSplits = newSplits
         await MainActor.run {
+            if chatWasSwitched(chat, pagination, openAroundItemId) { return }
             im.reversedChatItems = newReversed
             im.chatState.splits = orderedSplits
             im.chatState.unreadAfterItemId = chat.chatItems.last!.id
@@ -147,6 +150,7 @@ func apiLoadMessages(
         newItems.append(contentsOf: chat.chatItems)
         let items = newItems
         await MainActor.run {
+            if chatWasSwitched(chat, pagination, openAroundItemId) { return }
             im.reversedChatItems = items.reversed()
             im.chatState.splits = newSplits
             if im.secondaryIMFilter == nil {
@@ -158,6 +162,14 @@ func apiLoadMessages(
 }
 
 
+/// .initial pagination and opening around item set ChatModel.chatId themselves after the items are loaded.
+/// In other cases the chat could be switched while the items were loading, and the items of the previously opened chat
+/// must not be added to the items of the currently opened one
+private func chatWasSwitched(_ chat: Chat, _ pagination: ChatPagination, _ openAroundItemId: ChatItem.ID?) -> Bool {
+    let paginationIsInitial = switch pagination { case .initial: true; default: false }
+    return !paginationIsInitial && openAroundItemId == nil && ChatModel.shared.chatId != chat.id
+}
+
 private class ModifiedSplits {
     let oldUnreadSplitIndex: Int
     let newUnreadSplitIndex: Int

apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/ChatItemsLoader.kt

@@ -33,13 +33,21 @@ suspend fun apiLoadMessages(
   visibleItemIndexesNonReversed: () -> IntRange = { 0 .. 0 }
 ) = coroutineScope {
   val (chat, navInfo) = chatModel.controller.apiGetChat(rhId, chatType, apiId, chatsCtx.groupScopeInfo?.toChatScope(), contentTag ?: chatsCtx.contentTag, pagination, search) ?: return@coroutineScope
-  // For .initial allow the chatItems to be empty as well as chatModel.chatId to not match this chat because these values become set after .initial finishes
-  /** When [openAroundItemId] is provided, chatId can be different too */
-  if (((chatModel.chatId.value != chat.id || chat.chatItems.isEmpty()) && pagination !is ChatPagination.Initial && pagination !is ChatPagination.Last && openAroundItemId == null)
+  // For .initial allow the chatItems to be empty, as well as for .last that is used for searching
+  val allowEmptyItems = pagination is ChatPagination.Initial || pagination is ChatPagination.Last || openAroundItemId != null
+  if (chatWasSwitched(chat, pagination, openAroundItemId)
+    || (!allowEmptyItems && chat.chatItems.isEmpty())
     || !isActive) return@coroutineScope
   processLoadedChat(chatsCtx, chat, navInfo, pagination, openAroundItemId, visibleItemIndexesNonReversed)
 }
 
+/** .initial pagination and opening around item set [ChatModel.chatId] themselves after the items are loaded.
+ * In other cases the chat could be switched while the items were loading, and the items of the previously opened chat
+ * must not be added to the items of the currently opened one */
+private fun chatWasSwitched(chat: Chat, pagination: ChatPagination, openAroundItemId: Long?): Boolean =
+  pagination !is ChatPagination.Initial && openAroundItemId == null &&
+      (chatModel.chatId.value != chat.id || chatModel.remoteHostId() != chat.remoteHostId)
+
 suspend fun processLoadedChat(
   chatsCtx: ChatModel.ChatsContext,
   chat: Chat,
@@ -94,6 +102,7 @@ suspend fun processLoadedChat(
       val insertAt = (indexInCurrentItems - (wasSize - newItems.size) + trimmedIds.size).coerceAtLeast(0)
       newItems.addAll(insertAt, chat.chatItems)
       withContext(Dispatchers.Main) {
+        if (chatWasSwitched(chat, pagination, openAroundItemId)) return@withContext
         chatsCtx.chatItems.replaceAll(newItems)
         splits.value = newSplits
         chatState.moveUnreadAfterItem(oldUnreadSplitIndex, newUnreadSplitIndex, oldItems)
@@ -113,6 +122,7 @@ suspend fun processLoadedChat(
       val indexToAddIsLast = indexToAdd == newItems.size
       newItems.addAll(indexToAdd, chat.chatItems)
       withContext(Dispatchers.Main) {
+        if (chatWasSwitched(chat, pagination, openAroundItemId)) return@withContext
         chatsCtx.chatItems.replaceAll(newItems)
         splits.value = newSplits
         chatState.moveUnreadAfterItem(splits.value.firstOrNull() ?: newItems.last().id, newItems)
@@ -135,6 +145,7 @@ suspend fun processLoadedChat(
       newSplits.add(splitIndex, chat.chatItems.last().id)
 
       withContext(Dispatchers.Main) {
+        if (chatWasSwitched(chat, pagination, openAroundItemId)) return@withContext
         chatsCtx.chatItems.replaceAll(newItems)
         splits.value = newSplits
         unreadAfterItemId.value = chat.chatItems.last().id
@@ -158,6 +169,7 @@ suspend fun processLoadedChat(
       removeDuplicates(newItems, chat)
       newItems.addAll(chat.chatItems)
       withContext(Dispatchers.Main) {
+        if (chatWasSwitched(chat, pagination, openAroundItemId)) return@withContext
         chatsCtx.chatItems.replaceAll(newItems)
         chatState.splits.value = newSplits
         unreadAfterNewestLoaded.value = 0

apps/multiplatform/common/src/desktopTest/kotlin/chat/simplex/app/ChatItemsLoaderTest.kt

@@ -0,0 +1,65 @@
+package chat.simplex.app
+
+import chat.simplex.common.model.*
+import chat.simplex.common.platform.chatModel
+import chat.simplex.common.views.chat.processLoadedChat
+import chat.simplex.common.model.replaceAll
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.runBlocking
+import kotlinx.coroutines.withContext
+import kotlin.test.Test
+import kotlin.test.assertEquals
+
+class ChatItemsLoaderTest {
+
+  private fun groupChat(groupId: Long, itemIds: List<Long>): Chat =
+    Chat(
+      remoteHostId = null,
+      chatInfo = ChatInfo.Group(GroupInfo.sampleData.copy(groupId = groupId), groupChatScope = null),
+      chatItems = itemIds.map { ChatItem.getSampleData(it, CIDirection.GroupRcv(GroupMember.sampleData)) }
+    )
+
+  private suspend fun openChatWithItems(itemIds: List<Long>): Pair<ChatModel.ChatsContext, Chat> {
+    val chatsCtx = ChatModel.ChatsContext(null)
+    val opened = groupChat(groupId = 2, itemIds = itemIds)
+    withContext(Dispatchers.Main) {
+      chatsCtx.chatItems.replaceAll(opened.chatItems)
+      chatModel.chatId.value = opened.chatInfo.id
+    }
+    return chatsCtx to opened
+  }
+
+  private fun itemIds(chatsCtx: ChatModel.ChatsContext): List<Long> = chatsCtx.chatItems.value.map { it.id }
+
+  @Test
+  fun lastPageLoadedForAnotherChatIsNotAddedToOpenedChat() = runBlocking {
+    val (chatsCtx, _) = openChatWithItems(listOf(101, 102))
+    val anotherChat = groupChat(groupId = 1, itemIds = listOf(201, 202))
+    processLoadedChat(chatsCtx, anotherChat, NavigationInfo(), ChatPagination.Last(2), openAroundItemId = null)
+    assertEquals(listOf(101L, 102L), itemIds(chatsCtx))
+  }
+
+  @Test
+  fun lastPageLoadedForOpenedChatIsAdded() = runBlocking {
+    val (chatsCtx, _) = openChatWithItems(listOf(101, 102))
+    val sameChat = groupChat(groupId = 2, itemIds = listOf(103, 104))
+    processLoadedChat(chatsCtx, sameChat, NavigationInfo(), ChatPagination.Last(2), openAroundItemId = null)
+    assertEquals(listOf(101L, 102L, 103L, 104L), itemIds(chatsCtx))
+  }
+
+  @Test
+  fun beforePageLoadedForAnotherChatIsNotAddedToOpenedChat() = runBlocking {
+    val (chatsCtx, _) = openChatWithItems(listOf(101, 102))
+    val anotherChat = groupChat(groupId = 1, itemIds = listOf(201, 202))
+    processLoadedChat(chatsCtx, anotherChat, NavigationInfo(), ChatPagination.Before(101, 2), openAroundItemId = null)
+    assertEquals(listOf(101L, 102L), itemIds(chatsCtx))
+  }
+
+  @Test
+  fun aroundPageLoadedForAnotherChatIsNotAddedToOpenedChat() = runBlocking {
+    val (chatsCtx, _) = openChatWithItems(listOf(101, 102))
+    val anotherChat = groupChat(groupId = 1, itemIds = listOf(201, 202))
+    processLoadedChat(chatsCtx, anotherChat, NavigationInfo(), ChatPagination.Around(101, 2), openAroundItemId = null)
+    assertEquals(listOf(101L, 102L), itemIds(chatsCtx))
+  }
+}