From 253d07c062ec847aa32629da79453bd8af65190d Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Wed, 26 Aug 2026 09:17:49 -0400 Subject: [PATCH 1/2] fix(chat): keep chat history across logout so onboarding milestones survive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An account that had already sent a tip was shown the new-user tutorial again after signing back in, with "Scan a Tip Card" unticked. The milestone is a read of chat history — ChatMessageDao.hasEverTipped looks for an outgoing message with verb TIPPED — and two things conspired to leave that history empty. Logout wiped the cache. RealSessionController routed AuthState.LoggedOut into ChatCoordinator.reset, which deleted every metadata, message, and member row. Nothing needed it to: FlipcashDatabase names the file from the account entropy, so accounts are already separated by sitting in different databases, and the activity feed has never wiped itself on logout for exactly that reason. Split the coordinator into teardown (connections, jobs, in-memory state) and clearCache (the rows), point logout at teardown, and give deleteAndLogout the explicit wipe — deletion is the one caller that wants the data gone. Account switching goes through logout too, so it stops wiping as a side effect. Login could not rebuild what logout removed. performFeedSync decides which chats need catching up by asking whether a chat has cached messages and whether its stored sequence trails the server's — but it asked after upserting the server's sequence and each chat's lastMessage, so the first read was always true and the second always equal. Both branches were unreachable, leaving each chat holding the single message the feed carried. In the reported case that message was the tip received back, not the tip sent, so hasEverTipped stayed false for good. Snapshot both values before the writes. The delta path had the same shape one level down: performDeltaSync re-read chat_metadata for its starting sequence, which by then held the value the sync had just written, so it asked the server for everything after the server's own latest event. DeltaSyncNeeded now carries the pre-sync sequence. The live gap-fill caller has no write in front of it and keeps reading the row. One gap remains: WalletViewModel.isAwaitingActivity gates on the activity feed's sync state, not the chat cache's, so a cold start can still flash the milestone as incomplete before chat history lands. Closing that needs a "chat history hydrated at least once" signal that doesn't exist yet. --- .../shared/authentication/build.gradle.kts | 1 + .../com/flipcash/app/auth/AuthManager.kt | 7 + .../com/flipcash/app/auth/AuthManagerTest.kt | 24 +++ .../flipcash/shared/chat/ChatCoordinator.kt | 23 ++- .../chat/internal/RealChatCoordinator.kt | 12 +- .../internal/delegates/EventStreamDelegate.kt | 13 +- .../internal/delegates/FeedSyncDelegate.kt | 43 +++- .../chat/ChatCoordinatorEagerBalanceTest.kt | 10 +- .../shared/chat/ChatCoordinatorEventsTest.kt | 24 +-- .../chat/ChatCoordinatorTeardownTest.kt | 135 +++++++++++++ .../shared/chat/FeedSyncCatchUpTest.kt | 191 ++++++++++++++++++ .../shared/chat/ReceivedCounterTest.kt | 14 +- .../session/internal/RealSessionController.kt | 2 +- 13 files changed, 461 insertions(+), 38 deletions(-) create mode 100644 apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatCoordinatorTeardownTest.kt create mode 100644 apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/FeedSyncCatchUpTest.kt diff --git a/apps/flipcash/shared/authentication/build.gradle.kts b/apps/flipcash/shared/authentication/build.gradle.kts index 8f80b903dc..0362d2ab58 100644 --- a/apps/flipcash/shared/authentication/build.gradle.kts +++ b/apps/flipcash/shared/authentication/build.gradle.kts @@ -12,6 +12,7 @@ dependencies { implementation(libs.androidx.datastore) implementation(project(":apps:flipcash:shared:appsettings")) + implementation(project(":apps:flipcash:shared:chat")) implementation(project(":apps:flipcash:shared:contacts")) implementation(project(":apps:flipcash:shared:persistence:provider")) implementation(project(":apps:flipcash:shared:push")) diff --git a/apps/flipcash/shared/authentication/src/main/kotlin/com/flipcash/app/auth/AuthManager.kt b/apps/flipcash/shared/authentication/src/main/kotlin/com/flipcash/app/auth/AuthManager.kt index f07255df63..b725f009f7 100644 --- a/apps/flipcash/shared/authentication/src/main/kotlin/com/flipcash/app/auth/AuthManager.kt +++ b/apps/flipcash/shared/authentication/src/main/kotlin/com/flipcash/app/auth/AuthManager.kt @@ -7,6 +7,7 @@ import com.flipcash.app.auth.internal.credentials.PassphraseCredentialManager import com.flipcash.app.contacts.ContactCoordinator import com.flipcash.app.featureflags.FeatureFlagController import com.flipcash.app.persistence.PersistenceProvider +import com.flipcash.shared.chat.ChatCoordinator import com.flipcash.app.push.PushTokenProvider import com.flipcash.app.tokens.TokenCoordinator import com.flipcash.app.userflags.UserFlagsCoordinator @@ -54,6 +55,7 @@ class AuthManager @Inject constructor( private val userFlags: UserFlagsCoordinator, private val profileCoordinator: ProfileCoordinator, private val contactCoordinator: ContactCoordinator, + private val chatCoordinator: ChatCoordinator, private val networkObserver: NetworkConnectivityListener, private val dispatchers: DispatcherProvider, // private val analytics: AnalyticsService, @@ -315,6 +317,11 @@ class AuthManager @Inject constructor( //todo: add account deletion // Wipe server contact set before logout while the session can still authenticate. contactCoordinator.clearServerContactSet() + // Deletion is the only path that erases the local chat cache. Plain logout and account + // switching leave it — the database is per-account, so it never has to be emptied to keep + // accounts apart, and keeping it lets a re-login reconcile rather than refetch. That makes + // this the one caller responsible for the data actually going away. + chatCoordinator.clearCache() return logout() } diff --git a/apps/flipcash/shared/authentication/src/test/kotlin/com/flipcash/app/auth/AuthManagerTest.kt b/apps/flipcash/shared/authentication/src/test/kotlin/com/flipcash/app/auth/AuthManagerTest.kt index d9dfee698a..e883733d94 100644 --- a/apps/flipcash/shared/authentication/src/test/kotlin/com/flipcash/app/auth/AuthManagerTest.kt +++ b/apps/flipcash/shared/authentication/src/test/kotlin/com/flipcash/app/auth/AuthManagerTest.kt @@ -11,6 +11,7 @@ import com.flipcash.app.persistence.PersistenceProvider import com.flipcash.app.push.PushTokenProvider import com.flipcash.app.tokens.TokenCoordinator import com.flipcash.app.userflags.UserFlagsCoordinator +import com.flipcash.shared.chat.ChatCoordinator import com.flipcash.shared.profile.ProfileCoordinator import com.flipcash.services.controllers.AccountController import com.flipcash.services.controllers.ProfileController @@ -65,6 +66,7 @@ class AuthManagerTest { private val userFlags: UserFlagsCoordinator = mockk(relaxed = true) private val profileCoordinator: ProfileCoordinator = mockk(relaxed = true) private val contactCoordinator: ContactCoordinator = mockk(relaxed = true) + private val chatCoordinator: ChatCoordinator = mockk(relaxed = true) private val userManagerState = MutableStateFlow(UserManager.State()) private val networkConnectivityListener = mockk(relaxed = true) @@ -108,6 +110,7 @@ class AuthManagerTest { userFlags = userFlags, profileCoordinator = profileCoordinator, contactCoordinator = contactCoordinator, + chatCoordinator = chatCoordinator, dispatchers = dispatchers, networkObserver = networkConnectivityListener, ) @@ -164,6 +167,27 @@ class AuthManagerTest { verify { userFlags.clearAll() } } + @Test + fun `logout leaves the chat cache in place`() = runTest { + coEvery { credentialManager.logout() } returns Result.success(Unit) + + authManager.logout() + + // The Room file is named from the account entropy, so signing out never has to erase chat + // history to keep accounts apart — and keeping it lets a re-login reconcile what it already + // has instead of rebuilding from one message per chat. + coVerify(exactly = 0) { chatCoordinator.clearCache() } + } + + @Test + fun `deleteAndLogout erases the chat cache`() = runTest { + coEvery { credentialManager.logout() } returns Result.success(Unit) + + authManager.deleteAndLogout() + + coVerify(exactly = 1) { chatCoordinator.clearCache() } + } + @Test fun `non-soft login sets softLoginDisabled flag`() = runTest { val entropy = "dGVzdGVudHJvcHkxMjM0NQ==" 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 9ac375aa14..d8b8482106 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 @@ -166,8 +166,27 @@ interface ChatCoordinator : FeedOperations, EventStreamOperations, DmChatResolve /** Full observable snapshot of chat state (feed, typing, reactions, active chat). */ val state: StateFlow - /** Tears down all connections, clears persisted data, and resets in-memory state. */ - suspend fun reset() + /** + * Closes connections, cancels jobs, and drops in-memory state. + * + * Deliberately leaves the persisted cache alone. The Room database is per-account + * (`FlipcashDatabase.init` names the file from the account entropy), so signing out does not + * have to erase anything to keep the next account's data separate — logging in swaps to a + * different file. Keeping the cache lets a re-login reconcile what it already has via the + * feed sync's catch-up instead of rebuilding from nothing, which is what the "send a tip" + * onboarding milestone was silently losing. + * + * @see clearCache for the one caller that does want the data gone. + */ + suspend fun teardown() + + /** + * Erases this account's persisted chat history — metadata, messages, and members. + * + * Account deletion only. Ordinary logout and account switching go through [teardown] and + * keep the cache. + */ + suspend fun clearCache() } class NoDmChatInitializedException(e164: String) : Exception("No DM chat for $e164") 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 b25f995535..5d1083827b 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 @@ -144,7 +144,7 @@ class RealChatCoordinator @Inject constructor( is FeedSyncDelegate.Event.LoadMessages -> messagingDelegate.loadMessages(event.chatId) is FeedSyncDelegate.Event.DeltaSyncNeeded -> - eventStreamDelegate.performDeltaSync(event.chatId) + eventStreamDelegate.performDeltaSync(event.chatId, event.afterSequence) } }.launchIn(scope) @@ -197,7 +197,7 @@ class RealChatCoordinator @Inject constructor( // region ChatCoordinator - override suspend fun reset() { + override suspend fun teardown() { eventStreamDelegate.stopHeartbeat() eventStreamDelegate.close() feedDelegate.cancelJobs() @@ -205,9 +205,13 @@ class RealChatCoordinator @Inject constructor( stateHolder.reset() eventStreamDelegate.clearAll() cluster.value = null - messagingDelegate.clear() supervisorJob.cancel() - trace(tag = TAG, message = "reset complete", type = TraceType.Process) + trace(tag = TAG, message = "teardown complete", type = TraceType.Process) + } + + override suspend fun clearCache() { + messagingDelegate.clear() + trace(tag = TAG, message = "cache cleared", type = TraceType.Process) } // endregion diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/EventStreamDelegate.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/EventStreamDelegate.kt index 7377693d5a..e740da77d9 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/EventStreamDelegate.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/EventStreamDelegate.kt @@ -185,8 +185,17 @@ class EventStreamDelegate @Inject constructor( heartbeatJob = null } - internal suspend fun performDeltaSync(chatId: ChatId) { - val afterSequence = metadataDataSource.getLatestEventSequence(chatId) + /** + * Backfills a chat from [afterSequence] onward. + * + * Callers that have already written the server's sequence to `chat_metadata` before asking for + * a delta must pass the sequence the cache held *beforehand* — reading it here would return + * the value they just wrote and ask the server for everything after its own latest event, + * which is always nothing. The live gap-fill path has no such write in front of it and omits + * the argument. + */ + internal suspend fun performDeltaSync(chatId: ChatId, afterSequence: Long? = null) { + val afterSequence = afterSequence ?: metadataDataSource.getLatestEventSequence(chatId) trace(tag = TAG, message = "Delta sync for $chatId from sequence $afterSequence", type = TraceType.Process) try { diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/FeedSyncDelegate.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/FeedSyncDelegate.kt index da7cb2951b..a30019af8f 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/FeedSyncDelegate.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/FeedSyncDelegate.kt @@ -62,7 +62,11 @@ class FeedSyncDelegate @Inject constructor( sealed interface Event { data class LoadMessages(val chatId: ChatId) : Event - data class DeltaSyncNeeded(val chatId: ChatId) : Event + /** + * @param afterSequence the sequence the local cache held *before* this sync overwrote it; + * the delta must be requested from there, not from the row we just wrote. + */ + data class DeltaSyncNeeded(val chatId: ChatId, val afterSequence: Long) : Event } private val _events = Channel(Channel.UNLIMITED) @@ -188,10 +192,35 @@ class FeedSyncDelegate @Inject constructor( Result.success(contact.chats + (tip?.chats ?: emptyList())) } + /** What the local cache held for a chat before a sync wrote to it. */ + private data class CachedChatState( + val hasMessages: Boolean, + val latestEventSequence: Long, + ) + private suspend fun performFeedSync() { stateHolder.update { it.copy(feedSyncState = FeedSyncState.Syncing) } fetchCombinedFeed() .onSuccess { chats -> + // Snapshot the cache before writing to it. The writes below stamp the server's + // latestEventSequence onto every metadata row and give every chat at least one + // message, so the catch-up checks at the end of this sync — which are reads of + // exactly those two things — would otherwise always find the chat current and + // never fire. + // + // Both branches being inert had the same consequence: after a re-login, where + // logout has cleared the chat cache (ChatCoordinator.reset), a chat is left + // holding only the one message the feed carried. Anything derived from chat + // history then reads as if the rest never happened — the wallet's "send a tip" + // milestone looks for an outgoing TIPPED message and re-shows the new-user + // tutorial to an account that has already tipped. + val cached = chats.associate { chat -> + chat.chatId to CachedChatState( + hasMessages = messageDataSource.hasMessages(chat.chatId), + latestEventSequence = metadataDataSource.getLatestEventSequence(chat.chatId), + ) + } + metadataDataSource.upsert(chats) for (chat in chats) { @@ -205,14 +234,18 @@ class FeedSyncDelegate @Inject constructor( trace(tag = TAG, message = "Feed synced: ${chats.size} chats", type = TraceType.Process) for (chat in chats) { + val before = cached[chat.chatId] ?: continue if (chat.latestEventSequence > 0) { - val localSeq = metadataDataSource.getLatestEventSequence(chat.chatId) - if (localSeq > 0 && localSeq < chat.latestEventSequence) { - _events.send(Event.DeltaSyncNeeded(chat.chatId)) + if (before.latestEventSequence > 0 && + before.latestEventSequence < chat.latestEventSequence + ) { + _events.send( + Event.DeltaSyncNeeded(chat.chatId, before.latestEventSequence) + ) continue } } - if (!messageDataSource.hasMessages(chat.chatId)) { + if (!before.hasMessages) { _events.send(Event.LoadMessages(chat.chatId)) } } diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatCoordinatorEagerBalanceTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatCoordinatorEagerBalanceTest.kt index 2a6025e0ee..33542d4316 100644 --- a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatCoordinatorEagerBalanceTest.kt +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatCoordinatorEagerBalanceTest.kt @@ -177,7 +177,7 @@ class ChatCoordinatorEagerBalanceTest { runCurrent() coVerify(exactly = 1) { tokenCoordinator.add(mint, amount) } - coordinator.reset() + coordinator.teardown() } @Test @@ -188,7 +188,7 @@ class ChatCoordinatorEagerBalanceTest { runCurrent() coVerify(exactly = 0) { tokenCoordinator.add(any(), any()) } - coordinator.reset() + coordinator.teardown() } @Test @@ -199,7 +199,7 @@ class ChatCoordinatorEagerBalanceTest { runCurrent() coVerify(exactly = 0) { tokenCoordinator.add(any(), any()) } - coordinator.reset() + coordinator.teardown() } @Test @@ -217,7 +217,7 @@ class ChatCoordinatorEagerBalanceTest { coVerify(exactly = 1) { tokenCoordinator.add(mint, amount1) } coVerify(exactly = 1) { tokenCoordinator.add(mintB, amount2) } - coordinator.reset() + coordinator.teardown() } @Test @@ -231,6 +231,6 @@ class ChatCoordinatorEagerBalanceTest { runCurrent() coVerify(exactly = 1) { tokenCoordinator.add(any(), any()) } - coordinator.reset() + coordinator.teardown() } } diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatCoordinatorEventsTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatCoordinatorEventsTest.kt index 1a83953299..3a44e72b1b 100644 --- a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatCoordinatorEventsTest.kt +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatCoordinatorEventsTest.kt @@ -186,7 +186,7 @@ class ChatCoordinatorEventsTest { messages.size == 1 && messages[0].messageId == 1L }) } - coordinator.reset() + coordinator.teardown() } @Test @@ -209,7 +209,7 @@ class ChatCoordinatorEventsTest { messages.size == 1 && messages[0].messageId == 42L }) } - coordinator.reset() + coordinator.teardown() } @Test @@ -238,7 +238,7 @@ class ChatCoordinatorEventsTest { messages.size == 2 }) } - coordinator.reset() + coordinator.teardown() } // endregion @@ -262,7 +262,7 @@ class ChatCoordinatorEventsTest { runCurrent() coVerify { metadataDataSource.updateLatestEventSequence(chatId, 2L) } - coordinator.reset() + coordinator.teardown() } @Test @@ -290,7 +290,7 @@ class ChatCoordinatorEventsTest { // Cursor should advance to 1 (contiguous), not 3 coVerify { metadataDataSource.updateLatestEventSequence(chatId, 1L) } coVerify(exactly = 0) { metadataDataSource.updateLatestEventSequence(chatId, 3L) } - coordinator.reset() + coordinator.teardown() } @Test @@ -322,7 +322,7 @@ class ChatCoordinatorEventsTest { // After filling the gap, cursor should advance to 3 coVerify { metadataDataSource.updateLatestEventSequence(chatId, 3L) } - coordinator.reset() + coordinator.teardown() } // endregion @@ -359,7 +359,7 @@ class ChatCoordinatorEventsTest { assertEquals(1, summary.reactions.size) assertEquals("\uD83D\uDE00", summary.reactions[0].emoji.value) assertEquals(1L, summary.reactions[0].count) - coordinator.reset() + coordinator.teardown() } @Test @@ -407,7 +407,7 @@ class ChatCoordinatorEventsTest { assertEquals(1, reactions.size) assertEquals(3L, reactions[0].count) // stayed at 3, stale update rejected assertEquals(5L, reactions[0].sequence) - coordinator.reset() + coordinator.teardown() } @Test @@ -453,7 +453,7 @@ class ChatCoordinatorEventsTest { val reactions = coordinator.state.value.reactionOverlays[chatId]?.get(1L)?.reactions assertNotNull(reactions) assertTrue(reactions.isEmpty()) - coordinator.reset() + coordinator.teardown() } @Test @@ -489,7 +489,7 @@ class ChatCoordinatorEventsTest { val reactions = coordinator.state.value.reactionOverlays[chatId]?.get(1L)?.reactions assertNotNull(reactions) assertEquals(2, reactions.size) - coordinator.reset() + coordinator.teardown() } // endregion @@ -503,7 +503,7 @@ class ChatCoordinatorEventsTest { runCurrent() // Logout cancels the coordinator's supervisor job (and thus its scope). - coordinator.reset() + coordinator.teardown() runCurrent() // Re-login in the same process. Before the fix, the scope stayed cancelled, @@ -522,7 +522,7 @@ class ChatCoordinatorEventsTest { messages.size == 1 && messages[0].messageId == 7L }) } - coordinator.reset() + coordinator.teardown() } // endregion diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatCoordinatorTeardownTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatCoordinatorTeardownTest.kt new file mode 100644 index 0000000000..b8c8fc21c9 --- /dev/null +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatCoordinatorTeardownTest.kt @@ -0,0 +1,135 @@ +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.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.coEvery +import io.mockk.coVerify +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.flow.receiveAsFlow +import kotlinx.coroutines.test.TestCoroutineScheduler +import kotlinx.coroutines.test.runTest +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Signing out and erasing the account are separate acts, and only the second one should cost the + * user their cached chat history. + * + * The Room database is per-account — [com.flipcash.app.persistence.FlipcashDatabase.init] names the + * file from the account entropy — so a logout never has to empty a table to keep the next account's + * data separate; the next login opens a different file. Emptying it anyway is what left a re-login + * unable to see that the user had ever sent a tip. + */ +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +class ChatCoordinatorTeardownTest { + + private lateinit var metadataDataSource: ChatMetadataDataSource + private lateinit var messageDataSource: ChatMessageDataSource + private lateinit var memberDataSource: ChatMemberDataSource + 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() + + val chatController = mockk(relaxed = true) + coEvery { chatController.getDmChatFeed(any(), any()) } returns + Result.failure(RuntimeException("not needed")) + + metadataDataSource = mockk(relaxed = true) + messageDataSource = mockk(relaxed = true) + 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), + ), + stateHolder = stateHolder, + userManager = userManager, + networkObserver = mockk(relaxed = true), + dispatchers = TestDispatchers(TestCoroutineScheduler()), + ) + } + + @Test + fun `teardown leaves the persisted chat cache intact`() = runTest { + coordinator.teardown() + + coVerify(exactly = 0) { messageDataSource.clear() } + coVerify(exactly = 0) { metadataDataSource.clear() } + coVerify(exactly = 0) { memberDataSource.clear() } + } + + @Test + fun `clearCache erases metadata, messages, and members`() = runTest { + coordinator.clearCache() + + coVerify(exactly = 1) { metadataDataSource.clear() } + coVerify(exactly = 1) { messageDataSource.clear() } + coVerify(exactly = 1) { memberDataSource.clear() } + } +} diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/FeedSyncCatchUpTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/FeedSyncCatchUpTest.kt new file mode 100644 index 0000000000..4710bd270a --- /dev/null +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/FeedSyncCatchUpTest.kt @@ -0,0 +1,191 @@ +package com.flipcash.shared.chat + +import com.flipcash.app.persistence.sources.ChatMemberDataSource +import com.flipcash.app.persistence.sources.ChatMessageDataSource +import com.flipcash.app.persistence.sources.ChatMetadataDataSource +import com.flipcash.services.controllers.ChatController +import com.flipcash.services.models.chat.ChatFeedPage +import com.flipcash.services.models.chat.ChatId +import com.flipcash.services.models.chat.ChatMessage +import com.flipcash.services.models.chat.ChatMetadata +import com.flipcash.services.models.chat.ChatType +import com.flipcash.services.models.chat.MessageContent +import com.flipcash.services.user.UserManager +import com.flipcash.shared.chat.internal.ChatStateHolder +import com.flipcash.shared.chat.internal.delegates.FeedSyncDelegate +import io.mockk.coEvery +import io.mockk.mockk +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.junit.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue +import kotlin.time.Instant + +/** + * A feed sync writes each chat's `lastMessage` and the server's `latestEventSequence` before it + * decides whether that chat needs catching up. These tests hold the decision to what the cache held + * *before* the sync wrote to it. + * + * The user-visible failure: after a re-login the chat cache is empty (logout clears it via + * `ChatCoordinator.reset`), the sync repopulates one message per chat, and anything derived from + * chat history — the "send a tip" onboarding milestone reads an outgoing TIPPED message out of it — + * silently reads as if it never happened. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class FeedSyncCatchUpTest { + + private val selfId = listOf(1, 2, 3) + private val otherId = listOf(4, 5, 6) + private val chatId = ChatId("aabbccdd") + + private fun message( + messageId: Long, + senderId: List, + eventSequence: Long = messageId, + ) = ChatMessage( + messageId = messageId, + senderId = senderId, + content = listOf(MessageContent.Text("hi")), + timestamp = Instant.fromEpochSeconds(1_000 + messageId), + unreadSeq = messageId, + eventSequence = eventSequence, + ) + + private fun metadata( + lastMessage: ChatMessage?, + latestEventSequence: Long = 0, + ) = ChatMetadata( + chatId = chatId, + type = ChatType.TIP_DM, + members = emptyList(), + lastMessage = lastMessage, + lastActivity = Instant.fromEpochSeconds(1_000), + latestEventSequence = latestEventSequence, + ) + + /** + * Stands in for the per-user Room database: what the sync writes is what a later read sees. + * Relaxed mocks would answer `false`/`0` regardless of the writes, which is exactly the coupling + * under test. + */ + private class Harness(chats: List, seededChatsWithMessages: Set = emptySet()) { + val chatsWithMessages = seededChatsWithMessages.toMutableSet() + val storedSequences = mutableMapOf() + + val messageDataSource = mockk(relaxed = true).also { source -> + coEvery { source.upsert(any(), any()) } answers { + chatsWithMessages += firstArg() + } + coEvery { source.hasMessages(any()) } answers { firstArg() in chatsWithMessages } + } + + val metadataDataSource = mockk(relaxed = true).also { source -> + coEvery { source.upsert(any>()) } answers { + firstArg>().forEach { storedSequences[it.chatId] = it.latestEventSequence } + } + coEvery { source.getLatestEventSequence(any()) } answers { + storedSequences[firstArg()] ?: 0L + } + } + + val chatController = mockk(relaxed = true).also { controller -> + coEvery { controller.getDmChatFeed(ChatType.CONTACT_DM, any()) } returns + Result.success(ChatFeedPage(emptyList(), null, false)) + coEvery { controller.getDmChatFeed(ChatType.TIP_DM, any()) } returns + Result.success(ChatFeedPage(chats, null, false)) + } + + val delegate = FeedSyncDelegate( + chatController = chatController, + metadataDataSource = metadataDataSource, + messageDataSource = messageDataSource, + memberDataSource = mockk(relaxed = true), + stateHolder = ChatStateHolder(), + userManager = mockk(relaxed = true), + ) + + fun sync(scope: TestScope): List { + val events = mutableListOf() + delegate.events.onEach { events += it }.launchIn(scope.backgroundScope) + delegate.initialize(scope.backgroundScope) + delegate.syncFeed() + scope.runCurrent() + return events + } + } + + @Test + fun `cold cache loads a chat's history even when the feed carried its last message`() = runTest { + // The tip the user sent is not the newest message in the chat — the other party replied + // after it — so the feed's lastMessage alone can never prove the tip happened. + val harness = Harness(chats = listOf(metadata(lastMessage = message(20, otherId)))) + + val events = harness.sync(this) + + assertEquals( + listOf(FeedSyncDelegate.Event.LoadMessages(chatId)), + events, + "a chat with no cached history must be caught up, not judged by the row the sync just wrote", + ) + } + + @Test + fun `warm cache does not refetch history`() = runTest { + val harness = Harness( + chats = listOf(metadata(lastMessage = message(20, otherId))), + seededChatsWithMessages = setOf(chatId), + ) + + val events = harness.sync(this) + + assertTrue(events.isEmpty(), "a chat already backed by cached history needs no catch-up, got $events") + } + + @Test + fun `a local sequence behind the server's triggers a delta sync`() = runTest { + val harness = Harness( + chats = listOf(metadata(lastMessage = message(20, otherId), latestEventSequence = 99)), + seededChatsWithMessages = setOf(chatId), + ).apply { storedSequences[chatId] = 42 } + + val events = harness.sync(this) + + assertEquals( + listOf(FeedSyncDelegate.Event.DeltaSyncNeeded(chatId, afterSequence = 42)), + events, + "the gap must be measured against the sequence the cache held before the sync overwrote it", + ) + } + + /** + * The event carrying its own `afterSequence` is the whole point: the delta consumer reads + * `chat_metadata` when not given one, and by then this sync has already stamped the server's + * sequence onto that row — so a re-read would ask the server for everything after its own + * latest event and get nothing back. + */ + @Test + fun `the delta request starts from the pre-sync sequence, not the row the sync wrote`() = runTest { + val harness = Harness( + chats = listOf(metadata(lastMessage = message(20, otherId), latestEventSequence = 99)), + seededChatsWithMessages = setOf(chatId), + ).apply { storedSequences[chatId] = 42 } + + val event = harness.sync(this).single() as FeedSyncDelegate.Event.DeltaSyncNeeded + + assertEquals( + 99L, + harness.metadataDataSource.getLatestEventSequence(chatId), + "precondition: the sync has overwritten the stored sequence with the server's", + ) + assertEquals( + 42L, + event.afterSequence, + "the delta must be requested from 42, or the backfill silently fetches nothing", + ) + } +} diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ReceivedCounterTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ReceivedCounterTest.kt index 0fed87915e..218ee64734 100644 --- a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ReceivedCounterTest.kt +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ReceivedCounterTest.kt @@ -205,7 +205,7 @@ class ReceivedCounterTest { coVerify(exactly = 1) { analytics.incrementReceivedCounter(Analytics.ReceivedCounter.Tips, 1.0) } coVerify(exactly = 1) { analytics.incrementReceivedCounter(Analytics.ReceivedCounter.Messages, 1.0) } coVerify(exactly = 1) { analytics.incrementReceivedCounter(Analytics.ReceivedCounter.TipsValue, 5.0) } - coordinator.reset() + coordinator.teardown() } @Test @@ -215,7 +215,7 @@ class ReceivedCounterTest { coVerify(exactly = 1) { analytics.incrementReceivedCounter(Analytics.ReceivedCounter.Messages, 1.0) } coVerify(exactly = 0) { analytics.incrementReceivedCounter(Analytics.ReceivedCounter.Tips, any()) } - coordinator.reset() + coordinator.teardown() } @Test @@ -224,7 +224,7 @@ class ReceivedCounterTest { deliver(tipMessage(messageId = 1L, senderId = selfId)) coVerify(exactly = 0) { analytics.incrementReceivedCounter(any(), any()) } - coordinator.reset() + coordinator.teardown() } @Test @@ -236,7 +236,7 @@ class ReceivedCounterTest { deliver(msg) // gap fill / reconnect replays the same message coVerify(exactly = 1) { analytics.incrementReceivedCounter(Analytics.ReceivedCounter.Tips, 1.0) } - coordinator.reset() + coordinator.teardown() } @Test @@ -246,7 +246,7 @@ class ReceivedCounterTest { deliver(textMessage(messageId = 10L, senderId = otherId)) coVerify(exactly = 0) { analytics.incrementReceivedCounter(any(), any()) } - coordinator.reset() + coordinator.teardown() } @Test @@ -256,7 +256,7 @@ class ReceivedCounterTest { deliver(tipMessage(messageId = 1L, senderId = otherId, amount = cad)) coVerify(exactly = 1) { analytics.incrementReceivedCounter(Analytics.ReceivedCounter.TipsValue, 5.0) } - coordinator.reset() + coordinator.teardown() } @Test @@ -268,6 +268,6 @@ class ReceivedCounterTest { coVerify(exactly = 1) { analytics.incrementReceivedCounter(Analytics.ReceivedCounter.Tips, 1.0) } coVerify(exactly = 0) { analytics.incrementReceivedCounter(Analytics.ReceivedCounter.TipsValue, any()) } - coordinator.reset() + coordinator.teardown() } } diff --git a/apps/flipcash/shared/session/src/main/kotlin/com/flipcash/app/session/internal/RealSessionController.kt b/apps/flipcash/shared/session/src/main/kotlin/com/flipcash/app/session/internal/RealSessionController.kt index 5d72de19be..b32fcc16cc 100644 --- a/apps/flipcash/shared/session/src/main/kotlin/com/flipcash/app/session/internal/RealSessionController.kt +++ b/apps/flipcash/shared/session/src/main/kotlin/com/flipcash/app/session/internal/RealSessionController.kt @@ -185,7 +185,7 @@ class RealSessionController @Inject constructor( stopPolling() depositDelegate.cancelSweep() scope.launch { contactCoordinator.reset() } - scope.launch { chatCoordinator.reset() } + scope.launch { chatCoordinator.teardown() } stateHolder.reset() } From cc3f4c9d756e8903eed616e30858925847bb42d2 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Wed, 26 Aug 2026 10:11:32 -0400 Subject: [PATCH 2/2] fix(chat): withhold the tip milestone until chat history has hydrated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wallet tab drew "Scan a Tip Card" as outstanding before the chat cache had been reconciled, so an account that had already tipped saw the new-user tutorial for as long as its history took to arrive. `isAwaitingActivity` covered the activity feed's sync state; nothing covered the chat cache. The feed sync cannot serve as that signal. It reports itself `Synced` once the conversation list is written, which is before the per-chat backfill it schedules has run — and that backfill is what surfaces a tip older than a chat's last message. `FeedSyncDelegate` now sends a terminal `Event.CatchUpComplete` after the catch-up loop; because the delegate's events are a FIFO channel drained by one sequential collector in `RealChatCoordinator`, every catch-up item ahead of the marker has finished its suspend call by the time it is routed. Routing it sets `ChatState.historyHydration` to `Hydrated`. A failed sync moves `Unknown` to `Unavailable` instead, so an unreachable server ends the wait rather than extending it; it never downgrades a hydration that already succeeded. `hasEverTipped()` returns `Flow`, withholding an answer while hydration is `Unknown`. A cached tip still answers immediately — a TIPPED message in the cache is proof whatever the hydration state — so the warm cache path, now the normal one, does not wait on a round-trip. The flow re-subscribes to the Room query on the hydration change rather than combining the two: a combine would emit the pre-backfill answer alongside the "hydrated" flip and reintroduce the flash it is meant to remove. `WalletViewModel` emits nothing while the milestone is null, leaving `onboardingItems` null and the tab on its loading state. A held balance deliberately does not short-circuit this one, unlike the activity-feed clause: it is evidence about money, not about tipping. The cost is that a funded, never-tipped user with a cold cache now waits one chat round-trip before the tab draws. --- .../app/balance/internal/WalletViewModel.kt | 28 +- .../internal/WalletMilestoneGatingTest.kt | 109 ++++++++ .../flipcash/shared/chat/ChatCoordinator.kt | 15 +- .../com/flipcash/shared/chat/ChatState.kt | 22 ++ .../chat/internal/RealChatCoordinator.kt | 6 + .../internal/delegates/FeedSyncDelegate.kt | 42 ++- .../internal/delegates/MessagingDelegate.kt | 32 ++- .../shared/chat/ChatHistoryHydrationTest.kt | 254 ++++++++++++++++++ .../shared/chat/FeedSyncCatchUpTest.kt | 47 +++- 9 files changed, 538 insertions(+), 17 deletions(-) create mode 100644 apps/flipcash/features/balance/src/test/kotlin/com/flipcash/app/balance/internal/WalletMilestoneGatingTest.kt create mode 100644 apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatHistoryHydrationTest.kt diff --git a/apps/flipcash/features/balance/src/main/kotlin/com/flipcash/app/balance/internal/WalletViewModel.kt b/apps/flipcash/features/balance/src/main/kotlin/com/flipcash/app/balance/internal/WalletViewModel.kt index d471933e78..2f1a212393 100644 --- a/apps/flipcash/features/balance/src/main/kotlin/com/flipcash/app/balance/internal/WalletViewModel.kt +++ b/apps/flipcash/features/balance/src/main/kotlin/com/flipcash/app/balance/internal/WalletViewModel.kt @@ -21,6 +21,7 @@ import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.filterIsInstance +import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.mapNotNull @@ -77,6 +78,12 @@ internal class WalletViewModel @Inject constructor( * short-circuit the wait: if there is already activity to draw, there is nothing to * mistake for a new account — and neither is a held balance, which is a live read of the * account rather than of the cache. + * + * The two caches are separate and settle separately. The `feedSyncState` clause covers the + * activity feed; the *chat* cache backing the tip milestone is covered by + * [onboardingItems] staying null, because the milestone flow withholds an answer until its + * own history has hydrated. A held balance deliberately does not short-circuit that one: + * it says nothing about whether the account has tipped. */ val isAwaitingActivity: Boolean get() = onboardingItems == null || @@ -137,14 +144,21 @@ internal class WalletViewModel @Inject constructor( chatCoordinator.hasEverTipped(), tokenCoordinator.hasAnyBalance, ) { hasReceivedMoney, hasTipped, holdsBalance -> - Event.OnOnboardingItemsUpdated( - items = listOf( - TutorialItem.AddMoney(isCompleted = hasReceivedMoney || holdsBalance), - TutorialItem.ScanTipCard(isCompleted = hasTipped), - ), - holdsBalance = holdsBalance, - ) + // A null tip milestone means the chat cache has not been reconciled yet, and an + // un-hydrated cache reports every account as never having tipped. Emitting nothing + // keeps [State.onboardingItems] null, which holds the whole tab on its spinner rather + // than drawing "Scan a Tip Card" as outstanding to someone who already did it. + hasTipped?.let { + Event.OnOnboardingItemsUpdated( + items = listOf( + TutorialItem.AddMoney(isCompleted = hasReceivedMoney || holdsBalance), + TutorialItem.ScanTipCard(isCompleted = it), + ), + holdsBalance = holdsBalance, + ) + } } + .filterNotNull() .onEach { dispatchEvent(it) } .launchIn(viewModelScope) } diff --git a/apps/flipcash/features/balance/src/test/kotlin/com/flipcash/app/balance/internal/WalletMilestoneGatingTest.kt b/apps/flipcash/features/balance/src/test/kotlin/com/flipcash/app/balance/internal/WalletMilestoneGatingTest.kt new file mode 100644 index 0000000000..e902fa8dda --- /dev/null +++ b/apps/flipcash/features/balance/src/test/kotlin/com/flipcash/app/balance/internal/WalletMilestoneGatingTest.kt @@ -0,0 +1,109 @@ +package com.flipcash.app.balance.internal + +import androidx.arch.core.executor.testing.InstantTaskExecutorRule +import com.flipcash.app.analytics.StubFlipcashAnalytics +import com.flipcash.app.balance.internal.components.TutorialItem +import com.flipcash.app.core.MainCoroutineRule +import com.flipcash.app.core.dispatchers.TestDispatchers +import com.flipcash.app.funding.PurchaseMethodController +import com.flipcash.app.tokens.TokenCoordinator +import com.flipcash.app.userflags.UserFlagsCoordinator +import com.flipcash.services.user.UserManager +import com.flipcash.shared.chat.ChatCoordinator +import com.flipcash.shared.transactionhistory.ActivityFeedCoordinator +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import org.junit.Rule +import org.junit.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotNull +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * The tip milestone is read off the chat cache, which reports every account as never having tipped + * until its history has been reconciled. The wallet must not draw that: [WalletViewModel.State. + * onboardingItems] stays null — holding the tab on its loading state — until the chat coordinator + * commits to an answer. + * + * A held balance deliberately does not release the wait here, unlike for the activity feed: it is + * evidence about money, not about tipping. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class WalletMilestoneGatingTest { + + @get:Rule + val instantExecutorRule = InstantTaskExecutorRule() + + @get:Rule + var mainCoroutineRule = MainCoroutineRule(UnconfinedTestDispatcher()) + + private val hasEverTipped = MutableStateFlow(null) + + private val chatCoordinator: ChatCoordinator = mockk(relaxed = true) { + every { hasEverTipped() } returns hasEverTipped + } + private val feedCoordinator: ActivityFeedCoordinator = mockk(relaxed = true) { + every { hasEverReceivedMoney() } returns flowOf(true) + } + private val tokenCoordinator: TokenCoordinator = mockk(relaxed = true) { + every { hasAnyBalance } returns flowOf(true) + } + + private lateinit var dispatchers: TestDispatchers + + private fun createViewModel() = WalletViewModel( + userManager = mockk(relaxed = true), + userFlags = mockk(relaxed = true), + dispatchers = dispatchers, + purchaseMethodController = mockk(relaxed = true), + analytics = StubFlipcashAnalytics(), + chatCoordinator = chatCoordinator, + feedCoordinator = feedCoordinator, + tokenCoordinator = tokenCoordinator, + ) + + @Test + fun `milestones are withheld while the tip milestone is unknown`() = + runTest(mainCoroutineRule.dispatcher) { + dispatchers = TestDispatchers(testScheduler) + + val vm = createViewModel() + advanceUntilIdle() + + assertNull( + vm.stateFlow.value.onboardingItems, + "an un-reconciled chat cache reports every account as never having tipped", + ) + assertTrue( + vm.stateFlow.value.isAwaitingActivity, + "the tab must keep loading rather than draw a milestone it cannot yet answer", + ) + } + + @Test + fun `milestones are published once the tip milestone resolves`() = + runTest(mainCoroutineRule.dispatcher) { + dispatchers = TestDispatchers(testScheduler) + + val vm = createViewModel() + advanceUntilIdle() + hasEverTipped.value = true + advanceUntilIdle() + + val items = vm.stateFlow.value.onboardingItems + assertNotNull(items) + assertEquals( + listOf(true, true), + items.map { it.isCompleted }, + "both milestones read complete: the account holds a balance and has tipped", + ) + assertTrue(items.any { it is TutorialItem.ScanTipCard }) + } +} 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 d8b8482106..5af1d838b8 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 @@ -120,8 +120,19 @@ interface MessagingOperations { /** Observes all messages in [chatId] as a flat list. */ fun observeMessages(chatId: ChatId): Flow> - /** True once the user has ever sent a tip (a Cash message with verb TIPPED) — onboarding milestone. */ - fun hasEverTipped(): Flow + /** + * Whether the user has ever sent a tip — a Cash message with verb TIPPED — or `null` while the + * answer is not yet trustworthy. + * + * The read is of a local cache, so an absent TIPPED message means "never tipped" only once that + * cache is known to be complete; before then it is indistinguishable from history that has not + * arrived. Onboarding is the caller, and it draws a tutorial off the answer, so it needs the + * third state rather than a `false` it has to guess about. Resolves to `true`/`false` once + * [ChatHydrationState] leaves [ChatHydrationState.Unknown] — including on + * [ChatHydrationState.Unavailable], so an unreachable server ends the wait instead of extending + * it forever. + */ + fun hasEverTipped(): Flow /** Observes messages in [chatId] via Paging 3, with remote-mediated page loads. */ fun observeMessagesPaged(chatId: ChatId): Flow> diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatState.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatState.kt index d9c021b875..f4a010780d 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatState.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatState.kt @@ -11,6 +11,7 @@ data class ChatState( val typingIndicators: Map> = emptyMap(), val reactionOverlays: Map> = emptyMap(), val feedSyncState: FeedSyncState = FeedSyncState.Idle, + val historyHydration: ChatHydrationState = ChatHydrationState.Unknown, val activeChat: ChatId? = null, ) @@ -30,3 +31,24 @@ enum class FeedSyncState { Synced, Error, } + +/** + * How far the *message* cache has got in reconciling itself with the server for the signed-in user. + * + * Distinct from [FeedSyncState], which reports only the conversation-list fetch. That fetch marks + * itself [FeedSyncState.Synced] as soon as the metadata lands, before the per-chat history backfill + * it schedules has run — so it cannot answer "is this account's chat history here yet". Callers that + * read chat history as evidence need that second question: the wallet's "send a tip" milestone looks + * for an outgoing TIPPED message, and an absent one only means the user has never tipped once the + * cache is known to be complete. + */ +enum class ChatHydrationState { + /** No sync has finished a full pass this session — an absent message proves nothing. */ + Unknown, + + /** A sync succeeded and every catch-up it scheduled has run: an absent message really is absent. */ + Hydrated, + + /** A sync completed without success. Callers should stop waiting; the next sync will retry. */ + Unavailable, +} 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 5d1083827b..3ba5b7d263 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 @@ -145,6 +145,12 @@ class RealChatCoordinator @Inject constructor( messagingDelegate.loadMessages(event.chatId) is FeedSyncDelegate.Event.DeltaSyncNeeded -> eventStreamDelegate.performDeltaSync(event.chatId, event.afterSequence) + // Arrives after every catch-up item above it, because this is one sequential + // collector over a FIFO channel. Anything reading chat history as evidence — + // the wallet's "send a tip" milestone — waits for this rather than for the + // feed sync, which reports itself synced before the backfill is scheduled. + FeedSyncDelegate.Event.CatchUpComplete -> + feedDelegate.markHistoryHydrated() } }.launchIn(scope) diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/FeedSyncDelegate.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/FeedSyncDelegate.kt index a30019af8f..b4c1e6e6e2 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/FeedSyncDelegate.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/FeedSyncDelegate.kt @@ -10,6 +10,7 @@ import com.flipcash.services.models.chat.ChatMember import com.flipcash.services.models.chat.ChatMetadata import com.flipcash.services.models.chat.ChatType import com.flipcash.services.models.chat.PointerType +import com.flipcash.shared.chat.ChatHydrationState import com.flipcash.shared.chat.ChatSummary import com.flipcash.shared.chat.FeedOperations import com.flipcash.shared.chat.FeedSyncState @@ -67,6 +68,19 @@ class FeedSyncDelegate @Inject constructor( * the delta must be requested from there, not from the row we just wrote. */ data class DeltaSyncNeeded(val chatId: ChatId, val afterSequence: Long) : Event + + /** + * Emitted last by every successful sync, after any catch-up above it. + * + * [events] is a FIFO channel routed by a single sequential collector in + * [RealChatCoordinator][com.flipcash.shared.chat.internal.RealChatCoordinator], so by the + * time this is handled every catch-up item ahead of it has finished its suspend call and + * committed its writes. That ordering is the whole reason it exists: it is what makes + * [markHistoryHydrated] safe to call, and it cannot be replaced by watching + * [FeedSyncState][com.flipcash.shared.chat.FeedSyncState], which flips to `Synced` before + * the catch-up is even scheduled. + */ + data object CatchUpComplete : Event } private val _events = Channel(Channel.UNLIMITED) @@ -157,6 +171,16 @@ class FeedSyncDelegate @Inject constructor( syncJob = scope.launch { performFeedSync() } } + /** + * Marks the message cache reconciled with the server. + * + * Called by the coordinator when it routes [Event.CatchUpComplete], not by the sync itself — + * the sync only *schedules* the backfill, and hydration is about that backfill having run. + */ + internal fun markHistoryHydrated() { + stateHolder.update { it.copy(historyHydration = ChatHydrationState.Hydrated) } + } + internal fun cancelJobs() { syncJob?.cancel() feedObserverJob?.cancel() @@ -249,9 +273,25 @@ class FeedSyncDelegate @Inject constructor( _events.send(Event.LoadMessages(chat.chatId)) } } + + _events.send(Event.CatchUpComplete) } .onFailure { error -> - stateHolder.update { it.copy(feedSyncState = FeedSyncState.Error) } + stateHolder.update { state -> + state.copy( + feedSyncState = FeedSyncState.Error, + // Don't downgrade a hydration that already succeeded: a later failure means + // this sync missed, not that the cache stopped being trustworthy. Moving off + // Unknown at all matters though — callers waiting on hydration have to stop + // waiting when the server is unreachable, or the wallet spins forever + // offline. + historyHydration = if (state.historyHydration == ChatHydrationState.Unknown) { + ChatHydrationState.Unavailable + } else { + state.historyHydration + }, + ) + } trace(tag = TAG, message = "Feed sync failed: ${error.message}", type = TraceType.Error) } } 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 c75d48d142..fcf3b26197 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 @@ -1,4 +1,4 @@ -@file:OptIn(ExperimentalPagingApi::class) +@file:OptIn(ExperimentalPagingApi::class, ExperimentalCoroutinesApi::class) package com.flipcash.shared.chat.internal.delegates @@ -23,12 +23,15 @@ import com.flipcash.services.models.chat.MessageContent import com.flipcash.services.models.chat.MessagePointer import com.flipcash.services.models.chat.PointerType import com.flipcash.services.models.chat.TypingState +import com.flipcash.shared.chat.ChatHydrationState import com.flipcash.shared.chat.MessagingOperations import com.flipcash.shared.chat.internal.ChatStateHolder import com.flipcash.services.user.UserManager import com.getcode.opencode.model.core.ID +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.map import javax.inject.Inject import javax.inject.Singleton @@ -94,7 +97,32 @@ class MessagingDelegate @Inject constructor( return messageDataSource.observeMessages(chatId) } - override fun hasEverTipped(): Flow = messageDataSource.hasEverTipped() + override fun hasEverTipped(): Flow = + stateHolder.state + .map { it.historyHydration } + .distinctUntilChanged() + .flatMapLatest { hydration -> + // Re-subscribing on the hydration change re-runs the query, so the first value a + // caller sees after a backfill is a fresh read. `combine`-ing the two flows instead + // would race: the emission carrying "hydrated" would carry the *pre*-backfill answer + // with it, and the caller would act on `false` a beat before Room's invalidation + // published `true` — the same flash of wrong state, just narrower. + messageDataSource.hasEverTipped().map { tipped -> + when { + // A TIPPED message in the cache is proof regardless of hydration, and + // answering straight away is what keeps a warm cache — the normal case now + // that logout no longer wipes it — from waiting on a round-trip. + tipped -> true + // Absence proves nothing yet. Null rather than false: the caller has to be + // able to tell "has not tipped" from "we have not looked". + hydration == ChatHydrationState.Unknown -> null + else -> false + } + } + } + // The re-subscribe repeats the current answer whenever hydration moves, which for a + // cache that already held the tip is the same `true` twice over. + .distinctUntilChanged() override fun observeMessagesPaged(chatId: ChatId): Flow> { return Pager( diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatHistoryHydrationTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatHistoryHydrationTest.kt new file mode 100644 index 0000000000..d8255c5c91 --- /dev/null +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/ChatHistoryHydrationTest.kt @@ -0,0 +1,254 @@ +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.ChatFeedPage +import com.flipcash.services.models.chat.ChatId +import com.flipcash.services.models.chat.ChatMessage +import com.flipcash.services.models.chat.ChatMetadata +import com.flipcash.services.models.chat.ChatType +import com.flipcash.services.models.chat.ChatUpdate +import com.flipcash.services.models.chat.MessageContent +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.opencode.model.accounts.AccountCluster +import com.getcode.utils.network.NetworkConnectivityListener +import io.mockk.coEvery +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.flow.receiveAsFlow +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import kotlin.test.assertEquals +import kotlin.time.Instant + +/** + * The "send a tip" onboarding milestone is read off the local chat cache, so an absent TIPPED + * message is only evidence once that cache is known to be complete. These tests hold the milestone + * to that: it withholds an answer until the sync's catch-up has actually run, rather than reporting + * the cold-start `false` that re-shows the tutorial to an account which has already tipped. + * + * The feed sync alone cannot be that signal — it reports itself `Synced` after writing the + * conversation list, before the per-chat backfill it schedules has run. + */ +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +class ChatHistoryHydrationTest { + + private val chatId = ChatId("aabbccdd") + private val selfId = listOf(1, 2, 3) + private val otherId = listOf(4, 5, 6) + + /** Stands in for the Room query behind the milestone; the backfill flips it. */ + private val tippedInCache = MutableStateFlow(false) + + private val messagingController = mockk(relaxed = true) + private val chatController = mockk(relaxed = true) + + private fun message(messageId: Long) = ChatMessage( + messageId = messageId, + senderId = otherId, + content = listOf(MessageContent.Text("hi")), + timestamp = Instant.fromEpochSeconds(1_000 + messageId), + unreadSeq = messageId, + eventSequence = messageId, + ) + + private fun metadata() = ChatMetadata( + chatId = chatId, + type = ChatType.TIP_DM, + members = emptyList(), + lastMessage = message(20), + lastActivity = Instant.fromEpochSeconds(1_000), + latestEventSequence = 0, + ) + + private fun TestScope.buildCoordinator(): RealChatCoordinator { + val userManager = mockk(relaxed = true) + every { userManager.accountId } returns selfId + + val eventStreamingController = mockk(relaxed = true) + every { eventStreamingController.chatUpdates } returns + Channel(Channel.UNLIMITED).receiveAsFlow() + + val messageDataSource = mockk(relaxed = true) + every { messageDataSource.hasEverTipped() } returns tippedInCache + // A cold cache: nothing has messages, so the sync schedules a catch-up for every chat. + coEvery { messageDataSource.hasMessages(any()) } returns false + + val metadataDataSource = mockk(relaxed = true) + val memberDataSource = mockk(relaxed = true) + val stateHolder = ChatStateHolder() + + return 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), + ), + stateHolder = stateHolder, + userManager = userManager, + networkObserver = mockk(relaxed = true), + dispatchers = TestDispatchers(testScheduler), + ) + } + + private fun feedReturns(chats: List) { + coEvery { chatController.getDmChatFeed(ChatType.CONTACT_DM, any()) } returns + Result.success(ChatFeedPage(emptyList(), null, false)) + coEvery { chatController.getDmChatFeed(ChatType.TIP_DM, any()) } returns + Result.success(ChatFeedPage(chats, null, false)) + } + + private fun TestScope.collectMilestone(coordinator: RealChatCoordinator): List { + val values = mutableListOf() + coordinator.hasEverTipped().onEach { values += it }.launchIn(backgroundScope) + runCurrent() + return values + } + + /** + * Signs in and lets the session's work run to completion, then tears it down. + * + * The teardown is not incidental: signing in starts a heartbeat that delays forever, and + * [runTest] drains the scheduler once the body returns — on the coordinator's own scope, which + * is not [TestScope.backgroundScope] and so outlives the body. Left running, that loop spins + * virtual time indefinitely and the test never ends. + */ + private suspend fun TestScope.session(coordinator: RealChatCoordinator, body: () -> Unit) { + coordinator.onUserLoggedIn(mockk(relaxed = true)) + runCurrent() + // Unconditional: a failing assertion that skipped the teardown would leave the heartbeat + // running, and the resulting spin would bury the assertion under a test that never returns. + try { + body() + } finally { + coordinator.teardown() + } + } + + /** + * The reported bug, at the read side: an account that has tipped signs in, the cache is cold, + * and the milestone must never say `false` on the way to `true`. Anything that reports `false` + * first draws the tutorial for a frame. + */ + @Test + fun `the milestone withholds an answer until the backfill has run`() = runTest { + feedReturns(listOf(metadata())) + // The backfill is what surfaces the tip: it is older than the chat's last message, so the + // feed sync's own write could never have revealed it. + coEvery { messagingController.getMessages(chatId) } coAnswers { + tippedInCache.value = true + Result.success(listOf(message(20))) + } + val coordinator = buildCoordinator() + val values = collectMilestone(coordinator) + + assertEquals(listOf(null), values, "nothing has been reconciled yet") + + session(coordinator) { + assertEquals( + listOf(null, true), + values, + "the milestone must resolve straight from unknown to tipped, never through false", + ) + } + } + + /** Absence is real once the catch-up has run; the caller has to be released to draw. */ + @Test + fun `the milestone reports false once the cache is reconciled`() = runTest { + feedReturns(listOf(metadata())) + coEvery { messagingController.getMessages(chatId) } returns Result.success(emptyList()) + val coordinator = buildCoordinator() + val values = collectMilestone(coordinator) + + session(coordinator) { + assertEquals(listOf(null, false), values) + } + } + + /** + * A warm cache is the normal case now that signing out keeps chat history, and proof of a tip + * cannot go stale — so it answers without waiting for a round-trip. + */ + @Test + fun `a cached tip answers before any sync`() = runTest { + tippedInCache.value = true + feedReturns(emptyList()) + val coordinator = buildCoordinator() + + assertEquals(listOf(true), collectMilestone(coordinator)) + coordinator.teardown() + } + + /** + * Offline, the answer is still not trustworthy — but waiting on a server that cannot be reached + * would hold the wallet on its spinner indefinitely, which is worse than a stale milestone. + */ + @Test + fun `an unreachable server ends the wait rather than extending it`() = runTest { + coEvery { chatController.getDmChatFeed(any(), any()) } returns + Result.failure(RuntimeException("offline")) + val coordinator = buildCoordinator() + val values = collectMilestone(coordinator) + + session(coordinator) { + assertEquals(listOf(null, false), values) + } + } +} diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/FeedSyncCatchUpTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/FeedSyncCatchUpTest.kt index 4710bd270a..741b1b6ce7 100644 --- a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/FeedSyncCatchUpTest.kt +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/FeedSyncCatchUpTest.kt @@ -23,7 +23,6 @@ import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import org.junit.Test import kotlin.test.assertEquals -import kotlin.test.assertTrue import kotlin.time.Instant /** @@ -128,7 +127,7 @@ class FeedSyncCatchUpTest { val events = harness.sync(this) assertEquals( - listOf(FeedSyncDelegate.Event.LoadMessages(chatId)), + listOf(FeedSyncDelegate.Event.LoadMessages(chatId), FeedSyncDelegate.Event.CatchUpComplete), events, "a chat with no cached history must be caught up, not judged by the row the sync just wrote", ) @@ -143,7 +142,11 @@ class FeedSyncCatchUpTest { val events = harness.sync(this) - assertTrue(events.isEmpty(), "a chat already backed by cached history needs no catch-up, got $events") + assertEquals( + listOf(FeedSyncDelegate.Event.CatchUpComplete), + events, + "a chat already backed by cached history needs no catch-up, only the terminal marker", + ) } @Test @@ -156,7 +159,10 @@ class FeedSyncCatchUpTest { val events = harness.sync(this) assertEquals( - listOf(FeedSyncDelegate.Event.DeltaSyncNeeded(chatId, afterSequence = 42)), + listOf( + FeedSyncDelegate.Event.DeltaSyncNeeded(chatId, afterSequence = 42), + FeedSyncDelegate.Event.CatchUpComplete, + ), events, "the gap must be measured against the sequence the cache held before the sync overwrote it", ) @@ -175,7 +181,9 @@ class FeedSyncCatchUpTest { seededChatsWithMessages = setOf(chatId), ).apply { storedSequences[chatId] = 42 } - val event = harness.sync(this).single() as FeedSyncDelegate.Event.DeltaSyncNeeded + val event = harness.sync(this) + .filterIsInstance() + .single() assertEquals( 99L, @@ -188,4 +196,33 @@ class FeedSyncCatchUpTest { "the delta must be requested from 42, or the backfill silently fetches nothing", ) } + + /** + * The marker has to be last, not merely present: hydration is declared when it is routed, and a + * marker that overtook a pending catch-up would declare the cache complete while the backfill + * that completes it is still outstanding. + */ + @Test + fun `the catch-up marker is the last event of a successful sync`() = runTest { + val harness = Harness(chats = listOf(metadata(lastMessage = message(20, otherId)))) + + val events = harness.sync(this) + + assertEquals(FeedSyncDelegate.Event.CatchUpComplete, events.last()) + } + + @Test + fun `a failed sync emits no marker`() = runTest { + val harness = Harness(chats = emptyList()) + coEvery { harness.chatController.getDmChatFeed(ChatType.CONTACT_DM, any()) } returns + Result.failure(RuntimeException("offline")) + + val events = harness.sync(this) + + assertEquals( + emptyList(), + events, + "nothing was reconciled, so nothing may report itself reconciled", + ) + } }