From 11766b4a6cb62d795011346d148ecefbf8b30be1 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Mon, 21 Sep 2026 14:29:16 -0400 Subject: [PATCH] fix(chat): release the active chat only if it is still ours MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening a chat directly from another chat left notifications firing for the chat on screen. ChatViewModel.onCleared cleared the active chat id unconditionally, and Navigation 3 keeps both entries alive across that kind of switch: the outgoing entry is disposed after the incoming one has already claimed the chat, so the outgoing screen's teardown cleared the chat the user was by then looking at. An on-device trace caught the window — `Navigating to Chat[VDbei…]` at 13:58:55.368, the outgoing `[W4rX…]` screen's dispose at 13:58:55.892. Going back to the chat list first puts the dispose ahead of the open, which is why the same chat notified correctly when reached that way and made this look chat-specific. It is not: any chat opened straight from another one hits it. Replace the unconditional clear with clearActiveChat(chatId), a compare-and- clear that releases the chat only if it is still the active one. Correct in either dispose order. RealChatCoordinator also drops the backgrounded stash on a match, so a screen destroyed while the app is away is not restored as the active chat with nothing on screen. --- .../app/messenger/internal/ChatViewModel.kt | 5 +- .../flipcash/shared/chat/ChatCoordinator.kt | 12 ++ .../chat/internal/RealChatCoordinator.kt | 11 ++ .../internal/delegates/MessagingDelegate.kt | 5 + .../shared/chat/ActiveChatHandoffTest.kt | 174 ++++++++++++++++++ 5 files changed, 206 insertions(+), 1 deletion(-) create mode 100644 apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ActiveChatHandoffTest.kt diff --git a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt index e4fba136c4..23502ab942 100644 --- a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt +++ b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt @@ -1888,7 +1888,10 @@ internal class ChatViewModel @Inject constructor( override fun onCleared() { ProcessLifecycleOwner.get().lifecycle.removeObserver(draftFlushObserver) flushDraft() - chatCoordinator.setActiveChatId(null) + // This chat's claim only, not whatever is active now: opening a chat directly from + // another one disposes this entry after the incoming one has claimed the chat, so an + // unconditional clear would silence the chat the user just opened. + chatCoordinator.clearActiveChat(stateFlow.value.chatId) linkCardResolver.dispose() } diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatCoordinator.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatCoordinator.kt index ec3a5587ff..bc76c5b11b 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatCoordinator.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatCoordinator.kt @@ -131,6 +131,18 @@ interface MessagingOperations { /** Marks [chatId] as the currently-viewed chat (used to suppress notifications). */ fun setActiveChatId(chatId: ChatId?) + /** + * Releases [chatId] as the currently-viewed chat, if it still is. A null [chatId] releases + * nothing. + * + * The conditional is the point. A chat screen claims the active chat on open and releases it + * on teardown, but the two do not interleave in that order when one chat is opened directly + * from another: both entries are alive at once and the outgoing one is disposed after the + * incoming one has already claimed the chat. Clearing unconditionally there would clear the + * chat the user is looking at, and every push for it would notify. + */ + fun clearActiveChat(chatId: ChatId?) + /** Returns `true` if [chatId] is the currently-viewed chat. */ fun isActiveChat(chatId: ChatId): Boolean diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/RealChatCoordinator.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/RealChatCoordinator.kt index 064561c741..0b143b8060 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/RealChatCoordinator.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/RealChatCoordinator.kt @@ -227,6 +227,17 @@ class RealChatCoordinator @Inject constructor( .launchIn(scope) } + /** + * Also drops the backgrounded stash, which [onStop] fills and [onStart] restores from. A chat + * screen destroyed while the app is away is gone by the time the app returns; without this it + * would be restored as the active chat with nothing on screen, silencing its notifications. + */ + override fun clearActiveChat(chatId: ChatId?) { + if (chatId == null) return + if (backgroundedActiveChat == chatId) backgroundedActiveChat = null + messagingDelegate.clearActiveChat(chatId) + } + override fun onStart(owner: LifecycleOwner) { foregrounded.value = true backgroundedActiveChat?.let { diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/MessagingDelegate.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/MessagingDelegate.kt index 3ac3f10b7c..83e02b5053 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/MessagingDelegate.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/MessagingDelegate.kt @@ -109,6 +109,11 @@ class MessagingDelegate @Inject constructor( stateHolder.update { it.copy(activeChat = chatId) } } + override fun clearActiveChat(chatId: ChatId?) { + if (chatId == null) return + stateHolder.update { if (it.activeChat == chatId) it.copy(activeChat = null) else it } + } + override fun isActiveChat(chatId: ChatId): Boolean { return stateHolder.current.activeChat == chatId } diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ActiveChatHandoffTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ActiveChatHandoffTest.kt new file mode 100644 index 0000000000..7b27df07ad --- /dev/null +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ActiveChatHandoffTest.kt @@ -0,0 +1,174 @@ +package com.flipcash.shared.chat + +import com.flipcash.app.core.dispatchers.TestDispatchers +import com.flipcash.app.persistence.sources.ChatMemberDataSource +import com.flipcash.app.persistence.sources.ChatMessageDataSource +import com.flipcash.app.persistence.sources.ChatMetadataDataSource +import com.flipcash.app.persistence.sources.ContactDataSource +import com.flipcash.app.tokens.TokenCoordinator +import com.flipcash.services.controllers.ChatController +import com.flipcash.services.controllers.ChatMessagingController +import com.flipcash.services.controllers.EventStreamingController +import com.flipcash.services.models.chat.ChatId +import com.flipcash.services.models.chat.ChatUpdate +import com.flipcash.services.user.UserManager +import com.flipcash.shared.chat.internal.ChatIdGenerator +import com.flipcash.shared.chat.internal.ChatStateHolder +import com.flipcash.shared.chat.internal.RealChatCoordinator +import com.flipcash.shared.chat.internal.delegates.DmChatResolverDelegate +import com.flipcash.shared.chat.internal.delegates.EventStreamDelegate +import com.flipcash.shared.chat.internal.delegates.FeedSyncDelegate +import com.flipcash.shared.chat.internal.delegates.MessagingDelegate +import com.getcode.utils.network.NetworkConnectivityListener +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.flow.emptyFlow +import kotlinx.coroutines.flow.receiveAsFlow +import kotlinx.coroutines.test.TestCoroutineScheduler +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * Who owns the active chat when one chat screen replaces another. + * + * The active chat suppresses notifications for the conversation on screen + * ([com.flipcash.app.notifications.NotificationService] drops a push for it). Each chat screen + * claims it on open and released it on teardown — unconditionally, which is the bug: opening a + * chat *directly from another chat* (a group mention in the feed, a tap through from a DM) keeps + * both entries alive for a moment, and Navigation 3 disposes the outgoing one after the incoming + * one has already claimed the chat. + * + * An on-device trace caught the window: `Navigating to Chat[VDbei…]` at 13:58:55.368 and the + * outgoing `[W4rX…]` screen's dispose at 13:58:55.892 — 524ms later, clearing the chat the user + * was by then looking at. Every push for it notified from that point on. Going back to the chat + * list first put the dispose ahead of the open and hid the bug, which is why it looked like it + * only happened to some chats. + */ +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +class ActiveChatHandoffTest { + + private val leftChat = ChatId("aabbccdd") + private val openedChat = ChatId("11223344") + + private lateinit var coordinator: RealChatCoordinator + + @Before + fun setUp() { + val userManager = mockk(relaxed = true) + every { userManager.accountId } returns listOf(1, 2, 3) + + val eventStreamingController = mockk(relaxed = true) + every { eventStreamingController.chatUpdates } returns + Channel(Channel.UNLIMITED).receiveAsFlow() + every { eventStreamingController.streamFailures } returns emptyFlow() + + val chatController = mockk(relaxed = true) + val metadataDataSource = mockk(relaxed = true) + val messageDataSource = mockk(relaxed = true) + val memberDataSource = mockk(relaxed = true) + val messagingController = mockk(relaxed = true) + val stateHolder = ChatStateHolder() + + coordinator = RealChatCoordinator( + feedDelegate = FeedSyncDelegate( + chatController = chatController, + metadataDataSource = metadataDataSource, + messageDataSource = messageDataSource, + memberDataSource = memberDataSource, + stateHolder = stateHolder, + userManager = userManager, + ), + eventStreamDelegate = EventStreamDelegate( + eventStreamingController = eventStreamingController, + messagingController = messagingController, + metadataDataSource = metadataDataSource, + messageDataSource = messageDataSource, + memberDataSource = memberDataSource, + tokenCoordinator = mockk(relaxed = true), + userManager = userManager, + stateHolder = stateHolder, + analytics = mockk(relaxed = true), + exchange = mockk(relaxed = true), + ), + dmChatResolverDelegate = DmChatResolverDelegate( + chatIdGenerator = ChatIdGenerator(), + userManager = userManager, + contactDataSource = mockk(relaxed = true), + memberDataSource = memberDataSource, + ), + messagingDelegate = MessagingDelegate( + chatController = chatController, + messagingController = messagingController, + metadataDataSource = metadataDataSource, + messageDataSource = messageDataSource, + memberDataSource = memberDataSource, + notificationManager = mockk(relaxed = true), + userManager = userManager, + stateHolder = stateHolder, + analytics = mockk(relaxed = true), + senderResolver = mockk(relaxed = true), + ), + stateHolder = stateHolder, + draftStore = mockk(relaxed = true), + userManager = userManager, + networkObserver = mockk(relaxed = true), + dispatchers = TestDispatchers(TestCoroutineScheduler()), + groupFeedDelegate = mockk(relaxed = true), + ) + } + + // Nothing here logs in or foregrounds the coordinator, so no heartbeat runs and these stay + // plain synchronous calls: the active chat is set and read on the caller's thread. + + @Test + fun `a chat opened from another chat survives the outgoing screen's teardown`() { + coordinator.setActiveChatId(leftChat) + + // The incoming screen claims the chat while the outgoing entry is still alive. + coordinator.setActiveChatId(openedChat) + coordinator.clearActiveChat(leftChat) + + assertTrue(coordinator.isActiveChat(openedChat)) + } + + @Test + fun `leaving a chat for the list releases it`() { + coordinator.setActiveChatId(leftChat) + + coordinator.clearActiveChat(leftChat) + + assertFalse(coordinator.isActiveChat(leftChat)) + } + + @Test + fun `a chat that never opened releases nothing`() { + coordinator.setActiveChatId(openedChat) + + // A chat screen torn down before it ever resolved a chat id has no claim to release. + coordinator.clearActiveChat(null) + + assertTrue(coordinator.isActiveChat(openedChat)) + } + + @Test + fun `a chat torn down in the background does not come back on foreground`() { + val owner = mockk(relaxed = true) + coordinator.setActiveChatId(openedChat) + + // onStop stashes the active chat so foregrounding restores it. A screen destroyed while + // the app is away has to clear the stash too, or it returns as the active chat with + // nothing on screen. + coordinator.onStop(owner) + coordinator.clearActiveChat(openedChat) + coordinator.onStart(owner) + + assertFalse(coordinator.isActiveChat(openedChat)) + } +}