From 6e3c668fb9da332042c6fd60531e761d6667ebfd Mon Sep 17 00:00:00 2001 From: Narasimha-sc <166327228+Narasimha-sc@users.noreply.github.com> Date: Sat, 5 Sep 2026 14:28:00 +0000 Subject: [PATCH] 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]> --- .../Shared/Views/Chat/ChatItemsLoader.swift | 18 ++++- .../common/views/chat/ChatItemsLoader.kt | 18 ++++- .../chat/simplex/app/ChatItemsLoaderTest.kt | 65 +++++++++++++++++++ 3 files changed, 95 insertions(+), 6 deletions(-) create mode 100644 apps/multiplatform/common/src/desktopTest/kotlin/chat/simplex/app/ChatItemsLoaderTest.kt diff --git a/apps/ios/Shared/Views/Chat/ChatItemsLoader.swift b/apps/ios/Shared/Views/Chat/ChatItemsLoader.swift index 9987fb4697..5213b5509b 100644 --- a/apps/ios/Shared/Views/Chat/ChatItemsLoader.swift +++ b/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 diff --git a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/ChatItemsLoader.kt b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/ChatItemsLoader.kt index 6562d40cec..fd912a8c9e 100644 --- a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/ChatItemsLoader.kt +++ b/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 diff --git a/apps/multiplatform/common/src/desktopTest/kotlin/chat/simplex/app/ChatItemsLoaderTest.kt b/apps/multiplatform/common/src/desktopTest/kotlin/chat/simplex/app/ChatItemsLoaderTest.kt new file mode 100644 index 0000000000..3337839ff0 --- /dev/null +++ b/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): 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): Pair { + 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 = 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)) + } +}