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)) + } +}