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