From 182dd8fce722b14b26e3dddcbbc16a9763f2d86e Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Thu, 27 Aug 2026 11:46:05 -0400 Subject: [PATCH] perf(wallet): stop the login spinner waiting on three network races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wallet tab blanks itself while `isAwaitingTokens || isAwaitingActivity`, and on a fresh login neither side resolves promptly. The balance fetch idled behind `tokenUpdater.poll(startIn = 2.seconds)`. Nothing else fetches balances after login — `TokenCoordinator.onUserLoggedIn` only hydrates Room, which is empty on a new account — so that head start was a second of spinner spent deliberately idle. It could also be worse than a second: `onAppInForeground` fires twice at login, once from the transition into `AuthState.Ready` and once from `ON_RESUME`, and `NetworkUpdater.poll` restarted unconditionally, cancelling the in-flight fetch and re-serving the delay. `poll` now keys on its previously unused `key` parameter and leaves an equivalent running loop alone; `stop` still resets on background and logout. `onboardingItems` stayed null until `hasEverTipped()` could answer, which waits on the sequential per-chat backfill behind `ChatHydrationState`. The milestone combine now always emits, carrying `isTipMilestoneResolved` to say whether the tip half can be believed. Only `isNewUserTutorialComplete` reads that flag, so "Scan a Tip Card" is still never drawn as outstanding to someone who already tipped, while the balance, the card deck and the action tiles draw off state the app already has, as iOS does. The feed gate waited on a 100-row page. The count only bites on a cold cache — with anything cached `fetchSinceLatest` pages forward from the newest row — so it is really how much history a login waits for before the wallet can draw, and the wallet previews three rows. 25 still covers the history screen's first page, which pages at 20, and leaves the rest to that screen's own paging. The catch-up is guarded against the repeated foreground edge as well, so the second one joins the fetch rather than duplicating it. The per-chat backfill is untouched. It remains the long pole for an account with many conversations; the wallet just no longer waits on it. --- .../app/core/updater/NetworkUpdater.kt | 16 ++++ .../app/core/updater/NetworkUpdaterTest.kt | 86 +++++++++++++++++++ .../app/balance/internal/WalletViewModel.kt | 69 ++++++++++----- .../internal/WalletLoadingStateTest.kt | 26 ++++++ .../internal/WalletMilestoneGatingTest.kt | 58 +++++++++---- .../session/internal/RealSessionController.kt | 40 +++++++-- 6 files changed, 252 insertions(+), 43 deletions(-) create mode 100644 apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/updater/NetworkUpdaterTest.kt diff --git a/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/updater/NetworkUpdater.kt b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/updater/NetworkUpdater.kt index 47e42c57d4..35557dd811 100644 --- a/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/updater/NetworkUpdater.kt +++ b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/updater/NetworkUpdater.kt @@ -10,16 +10,31 @@ import kotlin.time.Duration.Companion.seconds abstract class NetworkUpdater { private var job: Job? = null + private var activeKey: Any? = null protected abstract suspend fun doUpdate() + /** + * Starts the polling loop, or leaves an equivalent one already running alone. + * + * The idempotence matters because callers arrive from lifecycle edges that can fire twice in + * quick succession — `SessionController.onAppInForeground` is invoked both by the transition + * into `AuthState.Ready` and by `ON_RESUME`, and at login those land together. Restarting + * unconditionally cancelled the in-flight [doUpdate] and re-served [startIn], so the first + * fetch of a fresh session was pushed further out by the very event that asked for it. + * + * Backgrounding and logout still reset the loop, through [stop]. + */ fun poll( key: Any? = null, scope: CoroutineScope, frequency: Duration, startIn: Duration = 0.seconds, ) { + if (job?.isActive == true && activeKey == key) return + stop() + activeKey = key job = scope.launch { delay(startIn) while (isActive) { @@ -32,5 +47,6 @@ abstract class NetworkUpdater { fun stop() { job?.cancel() job = null + activeKey = null } } diff --git a/apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/updater/NetworkUpdaterTest.kt b/apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/updater/NetworkUpdaterTest.kt new file mode 100644 index 0000000000..4520a366e9 --- /dev/null +++ b/apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/updater/NetworkUpdaterTest.kt @@ -0,0 +1,86 @@ +package com.flipcash.app.core.updater + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.runTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.seconds + +/** + * `SessionController.onAppInForeground` is invoked both by the transition into `AuthState.Ready` + * and by `ON_RESUME`, and at login those arrive together. A poller that restarted on every call + * cancelled the fetch the first call had started and re-served its start delay, so the second + * event pushed the first fetch of a fresh session further away. + * + * Time is advanced in bounded steps rather than with `advanceUntilIdle`, which does not run + * `backgroundScope`'s tasks — and an unbounded advance would not terminate against a poll loop. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class NetworkUpdaterTest { + + private class CountingUpdater : NetworkUpdater() { + var updates = 0 + private set + + override suspend fun doUpdate() { + updates++ + } + } + + @Test + fun `a repeated poll leaves the running loop alone`() = runTest { + val updater = CountingUpdater() + + updater.poll(scope = backgroundScope, frequency = 20.seconds) + advanceTimeBy(1.milliseconds) + assertEquals(1, updater.updates, "the first poll fetches immediately") + + updater.poll(scope = backgroundScope, frequency = 20.seconds) + advanceTimeBy(1.seconds) + assertEquals(1, updater.updates, "the duplicate must not re-fetch off-cadence") + + advanceTimeBy(20.seconds) + assertEquals(2, updater.updates, "and the original cadence is intact") + } + + @Test + fun `a repeated poll does not re-serve the start delay`() = runTest { + val updater = CountingUpdater() + + updater.poll(scope = backgroundScope, frequency = 20.seconds, startIn = 2.seconds) + advanceTimeBy(1.seconds) + updater.poll(scope = backgroundScope, frequency = 20.seconds, startIn = 2.seconds) + + // t = 2.5s. On the original schedule the fetch has landed; a restart would have pushed it + // to t = 3s. + advanceTimeBy(1500.milliseconds) + assertEquals(1, updater.updates, "the fetch lands on the original schedule, not a second late") + } + + @Test + fun `stopping releases the loop so the next foreground restarts it`() = runTest { + val updater = CountingUpdater() + + updater.poll(scope = backgroundScope, frequency = 20.seconds) + advanceTimeBy(1.milliseconds) + updater.stop() + + updater.poll(scope = backgroundScope, frequency = 20.seconds) + advanceTimeBy(1.milliseconds) + assertEquals(2, updater.updates) + } + + @Test + fun `a poll under a different key replaces the running loop`() = runTest { + val updater = CountingUpdater() + + updater.poll(key = "a", scope = backgroundScope, frequency = 20.seconds) + advanceTimeBy(1.milliseconds) + + updater.poll(key = "b", scope = backgroundScope, frequency = 20.seconds) + advanceTimeBy(1.milliseconds) + assertEquals(2, updater.updates) + } +} 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 2f1a212393..f416bd7f67 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,7 +21,6 @@ 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 @@ -49,8 +48,22 @@ internal class WalletViewModel @Inject constructor( * Onboarding milestones, or `null` while they are still unknown. The distinction matters: * an empty/incomplete checklist is what draws the new-user tutorial, and every milestone * reads as incomplete before its source has reported. + * + * Published off local reads (the feed's own cache and the held balance), so this lands + * without waiting on the network. The tip milestone inside it is the one that needs a + * server round-trip; [isTipMilestoneResolved] says whether it can be believed yet. */ val onboardingItems: List? = null, + /** + * Whether [TutorialItem.ScanTipCard]'s answer is trustworthy. + * + * The tip milestone is read off the chat cache, which reports every account as never + * having tipped until its history has been reconciled with the server — a wait that + * scales with how many conversations the account has. Only the tutorial depends on that + * answer, so only the tutorial waits for it (see [isNewUserTutorialComplete]); the rest + * of the tab draws off state it already has, as iOS does. + */ + val isTipMilestoneResolved: Boolean = false, /** * Preview of the most recent unified cross-token activity — at most [RECENT_PREVIEW_COUNT] * rows. The coordinator owns the mapping and enforces the limit; the full paged history is a @@ -64,9 +77,15 @@ internal class WalletViewModel @Inject constructor( val hasReceivedMoney: Boolean get() = onboardingItems?.find { it is TutorialItem.AddMoney }?.isCompleted == true - /** Treated as complete while unknown, so the tutorial is never the thing we guess at. */ + /** + * Treated as complete while unknown, so the tutorial is never the thing we guess at. + * + * "Unknown" covers the un-reconciled chat cache as well as absent milestones: until + * [isTipMilestoneResolved], `ScanTipCard` reads incomplete for everyone, and drawing that + * would tell an established tipper to go and scan a tip card. + */ val isNewUserTutorialComplete: Boolean - get() = onboardingItems?.all { it.isCompleted } != false + get() = !isTipMilestoneResolved || onboardingItems?.all { it.isCompleted } != false /** * Whether the activity half of the tab is still settling. @@ -79,11 +98,11 @@ internal class WalletViewModel @Inject constructor( * 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. + * Scoped to the activity feed on purpose. The *chat* cache backing the tip milestone + * settles separately and far later — its hydration waits on a per-conversation backfill — + * and the only thing that reads it is the tutorial, which withholds itself while unsure + * (see [isNewUserTutorialComplete]). Holding the whole tab for it meant the balance and + * the card deck, both long since resolved, waited on an answer neither of them uses. */ val isAwaitingActivity: Boolean get() = onboardingItems == null || @@ -94,6 +113,7 @@ internal class WalletViewModel @Inject constructor( data class OnOnboardingItemsUpdated( val items: List, val holdsBalance: Boolean, + val isTipMilestoneResolved: Boolean, ): Event data class OnTransactionsUpdated(val transactions: List) : Event data class OnPreferredOnRampProviderChanged(val provider: OnRampProvider.Defined?) : Event @@ -145,20 +165,21 @@ internal class WalletViewModel @Inject constructor( tokenCoordinator.hasAnyBalance, ) { hasReceivedMoney, hasTipped, 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, - ) - } + // un-hydrated cache reports every account as never having tipped. That is carried as + // `isTipMilestoneResolved` rather than by withholding the emission: the other + // milestone and the action tiles are answerable from local state immediately, and + // holding them back put the chat backfill on the critical path for the whole tab. + // "Scan a Tip Card" is still never drawn as outstanding to someone who already did it + // — [State.isNewUserTutorialComplete] reads complete until this resolves. + Event.OnOnboardingItemsUpdated( + items = listOf( + TutorialItem.AddMoney(isCompleted = hasReceivedMoney || holdsBalance), + TutorialItem.ScanTipCard(isCompleted = hasTipped == true), + ), + holdsBalance = holdsBalance, + isTipMilestoneResolved = hasTipped != null, + ) } - .filterNotNull() .onEach { dispatchEvent(it) } .launchIn(viewModelScope) } @@ -177,7 +198,11 @@ internal class WalletViewModel @Inject constructor( state.copy(feedSyncState = event.syncState) } is Event.OnOnboardingItemsUpdated -> { state -> - state.copy(onboardingItems = event.items, holdsBalance = event.holdsBalance) + state.copy( + onboardingItems = event.items, + holdsBalance = event.holdsBalance, + isTipMilestoneResolved = event.isTipMilestoneResolved, + ) } is Event.OnTransactionsUpdated -> { state -> state.copy(transactions = event.transactions) diff --git a/apps/flipcash/features/balance/src/test/kotlin/com/flipcash/app/balance/internal/WalletLoadingStateTest.kt b/apps/flipcash/features/balance/src/test/kotlin/com/flipcash/app/balance/internal/WalletLoadingStateTest.kt index d8afd7c23a..0a750bdbb2 100644 --- a/apps/flipcash/features/balance/src/test/kotlin/com/flipcash/app/balance/internal/WalletLoadingStateTest.kt +++ b/apps/flipcash/features/balance/src/test/kotlin/com/flipcash/app/balance/internal/WalletLoadingStateTest.kt @@ -104,6 +104,7 @@ class WalletLoadingStateTest { fun `tutorial shows once a milestone is known to be outstanding`() { val state = WalletViewModel.State( onboardingItems = milestones(addedMoney = true, tipped = false), + isTipMilestoneResolved = true, feedSyncState = FeedSyncState.Synced, ) assertFalse(state.isNewUserTutorialComplete) @@ -113,11 +114,35 @@ class WalletLoadingStateTest { fun `tutorial is complete when every milestone is`() { val state = WalletViewModel.State( onboardingItems = milestones(addedMoney = true, tipped = true), + isTipMilestoneResolved = true, feedSyncState = FeedSyncState.Synced, ) assertTrue(state.isNewUserTutorialComplete) } + /** + * The chat cache reports every account as never having tipped until it has been reconciled, + * and reconciling it waits on a per-conversation backfill. The tutorial is the only thing that + * reads the answer, so it is the only thing that waits. + */ + @Test + fun `an unresolved tip milestone withholds the tutorial without holding the tab`() { + val state = WalletViewModel.State( + onboardingItems = milestones(addedMoney = true, tipped = false), + isTipMilestoneResolved = false, + feedSyncState = FeedSyncState.Synced, + ) + assertTrue( + state.isNewUserTutorialComplete, + "an un-reconciled chat cache must not be drawn as an outstanding milestone", + ) + assertFalse( + state.isAwaitingActivity, + "the balance and the card deck do not read the tip milestone and must not wait for it", + ) + assertTrue(state.hasReceivedMoney, "the action tiles read local state and are answerable") + } + @Test fun `hasReceivedMoney is false while unknown, gating the action tiles`() { assertFalse(WalletViewModel.State().hasReceivedMoney) @@ -132,6 +157,7 @@ class WalletLoadingStateTest { fun `a held balance completes the add-money milestone without a feed row`() { val state = WalletViewModel.State( onboardingItems = milestones(addedMoney = true, tipped = true), + isTipMilestoneResolved = true, feedSyncState = FeedSyncState.Synced, holdsBalance = true, ) 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 index e902fa8dda..2c10bdcc01 100644 --- 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 @@ -22,18 +22,17 @@ import kotlinx.coroutines.test.runTest import org.junit.Rule import org.junit.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse 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. + * until its history has been reconciled — a wait that scales with how many conversations the + * account has. The wallet must not draw that answer, but it must not wait on it either: the + * milestones publish immediately off local state, carrying + * [WalletViewModel.State.isTipMilestoneResolved] to say whether the tip half can be believed, and + * only the tutorial reads that flag. */ @OptIn(ExperimentalCoroutinesApi::class) class WalletMilestoneGatingTest { @@ -70,25 +69,34 @@ class WalletMilestoneGatingTest { ) @Test - fun `milestones are withheld while the tip milestone is unknown`() = + fun `milestones publish without waiting on the tip milestone`() = 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", + val state = vm.stateFlow.value + assertNotNull( + state.onboardingItems, + "the add-money milestone is answerable from local state and must not wait", + ) + assertFalse( + state.isTipMilestoneResolved, + "an un-reconciled chat cache cannot answer whether the account has tipped", + ) + assertTrue( + state.isNewUserTutorialComplete, + "so the tutorial withholds itself rather than drawing an answer it does not have", ) assertTrue( - vm.stateFlow.value.isAwaitingActivity, - "the tab must keep loading rather than draw a milestone it cannot yet answer", + state.hasReceivedMoney, + "the milestone the chat cache says nothing about is drawn immediately", ) } @Test - fun `milestones are published once the tip milestone resolves`() = + fun `the tip milestone is believed once the chat cache reconciles`() = runTest(mainCoroutineRule.dispatcher) { dispatchers = TestDispatchers(testScheduler) @@ -97,8 +105,10 @@ class WalletMilestoneGatingTest { hasEverTipped.value = true advanceUntilIdle() - val items = vm.stateFlow.value.onboardingItems + val state = vm.stateFlow.value + val items = state.onboardingItems assertNotNull(items) + assertTrue(state.isTipMilestoneResolved) assertEquals( listOf(true, true), items.map { it.isCompleted }, @@ -106,4 +116,22 @@ class WalletMilestoneGatingTest { ) assertTrue(items.any { it is TutorialItem.ScanTipCard }) } + + @Test + fun `a reconciled cache that has never seen a tip draws the tutorial`() = + runTest(mainCoroutineRule.dispatcher) { + dispatchers = TestDispatchers(testScheduler) + + val vm = createViewModel() + advanceUntilIdle() + hasEverTipped.value = false + advanceUntilIdle() + + val state = vm.stateFlow.value + assertTrue(state.isTipMilestoneResolved) + assertFalse( + state.isNewUserTutorialComplete, + "an answered 'never tipped' is an outstanding milestone, not an unknown one", + ) + } } 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 3392bedd8d..a18357c4c1 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 @@ -45,6 +45,7 @@ import com.getcode.utils.TraceType import com.getcode.utils.network.NetworkConnectivityListener import com.getcode.utils.trace import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.distinctUntilChanged @@ -121,6 +122,9 @@ class RealSessionController @Inject constructor( private val scope = CoroutineScope(dispatchers.IO + SupervisorJob()) + /** In-flight feed catch-up, so a repeated foreground edge joins it instead of duplicating it. */ + private var feedCatchUpJob: Job? = null + override val state: StateFlow get() = stateHolder.state @@ -316,7 +320,11 @@ class RealSessionController @Inject constructor( private fun startPolling() { if (userManager.authState.canAccessAuthenticatedApis) { - tokenUpdater.poll(scope = scope, frequency = 20.seconds, startIn = 2.seconds) + // No `startIn` on the balances: this is the only thing that fetches them after login + // (TokenCoordinator.onUserLoggedIn just hydrates Room, which is empty on a fresh + // account), and the wallet tab holds its spinner until the fetch lands. A head start + // here was a second of the login spinner spent deliberately idle. + tokenUpdater.poll(scope = scope, frequency = 20.seconds) activityFeedUpdater.poll(scope = scope, frequency = 60.seconds, startIn = 60.seconds) profileUpdater.poll(scope = scope, frequency = 60.seconds, startIn = 0.seconds) } @@ -390,11 +398,26 @@ class RealSessionController @Inject constructor( } } - private fun bringActivityFeedCurrent(count: Int = 100) { - if (userManager.authState.canAccessAuthenticatedApis) { - scope.launch { - feedCoordinator.fetchSinceLatest(count) - } + /** + * Reconciles the activity feed with the server. + * + * [count] only bites on a cold cache, where [ActivityFeedCoordinator.fetchSinceLatest] seeds + * the newest [count] rows; with anything cached it pages *forward* from the newest row and + * takes whatever has happened since. So the size is really "how much history a fresh login + * waits for before the wallet can draw" — and the wallet previews three rows. The default + * covers the history screen's first page (it pages at 20) and leaves the rest to that screen's + * own paging, rather than making every login pay for a hundred rows up front. + * + * Guarded against overlap because the foreground edge can arrive twice at login — from the + * transition into [AuthState.Ready] and from `ON_RESUME` — and the second one would otherwise + * duplicate the fetch rather than wait for it. + */ + private fun bringActivityFeedCurrent(count: Int = FEED_CATCH_UP_PAGE) { + if (!userManager.authState.canAccessAuthenticatedApis) return + if (feedCatchUpJob?.isActive == true) return + + feedCatchUpJob = scope.launch { + feedCoordinator.fetchSinceLatest(count) } } @@ -405,4 +428,9 @@ class RealSessionController @Inject constructor( } } } + + private companion object { + /** Rows a fresh login seeds the activity feed with. See [bringActivityFeedCurrent]. */ + const val FEED_CATCH_UP_PAGE = 25 + } }