diff --git a/android/app/src/main/java/com/fabledsword/minstrel/home/ui/HomeScreen.kt b/android/app/src/main/java/com/fabledsword/minstrel/home/ui/HomeScreen.kt index 8b6b6313..c5c22095 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/home/ui/HomeScreen.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/home/ui/HomeScreen.kt @@ -87,6 +87,8 @@ import com.fabledsword.minstrel.playlists.widgets.PlaylistCard import com.fabledsword.minstrel.playlists.widgets.PlaylistPlaceholderCard import com.fabledsword.minstrel.shared.UiState import com.fabledsword.minstrel.shared.UpdateVeilController +import com.fabledsword.minstrel.shared.VeilOutcome +import com.fabledsword.minstrel.shared.VeilSessionResult import com.fabledsword.minstrel.shared.VeilSettleState import com.fabledsword.minstrel.shared.asCacheFirstStateFlow import com.fabledsword.minstrel.shared.widgets.ArtSettleTracker @@ -100,7 +102,6 @@ import com.fabledsword.minstrel.shared.widgets.SkeletonAlbumTile import com.fabledsword.minstrel.shared.widgets.SkeletonArtistTile import com.fabledsword.minstrel.shared.widgets.SkeletonSectionHeader import dagger.hilt.android.lifecycle.HiltViewModel -import kotlinx.coroutines.Job import kotlinx.coroutines.async import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.coroutineScope @@ -128,8 +129,9 @@ private const val BOTTOM_PADDING_FOR_MINIPLAYER_DP = 140 private const val RECENTLY_ADDED_GRID_ROWS = 2 private const val RECENTLY_ADDED_GRID_HEIGHT_DP = 440 -// "Updating your mixes…" veil. Its lifetime is decided by -// UpdateVeilController watching the screen settle — not by a fixed delay, +// "Updating your mixes…" veil. UpdateVeilController decides both whether it +// appears at all — only when a refresh actually changes something — and how +// long it stays, by watching the screen settle rather than by a fixed delay, // which lowered it while tiles and artwork were still landing (#2327). // Near-opaque (VEIL_ALPHA) so the section churn never bleeds through. private const val VEIL_WIPE_MS = 280 @@ -138,8 +140,9 @@ private const val VEIL_SPINNER_DP = 22 private const val VEIL_SPINNER_STROKE_DP = 2 private const val VEIL_LABEL_GAP_DP = 12 -// How long a manual pull keeps its own indicator while waiting for the -// veil to take over, so the two don't both vanish for a frame mid-handoff. +// Backstop on how long a manual pull keeps its own indicator while waiting +// for its successor — the veil, or the "already up to date" snackbar — so the +// two never both vanish for a frame mid-handoff. private const val PULL_HANDOFF_TIMEOUT_MS = 2_000L // ─── State ─────────────────────────────────────────────────────────── @@ -196,10 +199,13 @@ class HomeViewModel @Inject constructor( initialValue = false, ) - private val poolMessages = Channel(Channel.BUFFERED) + private val snackbarMessages = Channel(Channel.BUFFERED) - /** Transient snackbar messages from offline-pool taps. */ - val transientMessages: Flow = poolMessages.receiveAsFlow() + /** + * Transient snackbar messages: offline-pool taps, playback failures, and + * the outcome of a refresh the user explicitly asked for. + */ + val transientMessages: Flow = snackbarMessages.receiveAsFlow() /** * Copy for the most recent /home/index refresh failure; null once a @@ -228,7 +234,7 @@ class HomeViewModel @Inject constructor( OfflinePoolKind.LIKED -> shuffleSource.liked() }.shuffled() if (tracks.isEmpty()) { - poolMessages.trySend("No cached ${kind.label} tracks yet") + snackbarMessages.trySend("No cached ${kind.label} tracks yet") } else { player.setQueue(tracks, initialIndex = 0, source = "offline:${kind.name}") } @@ -265,14 +271,14 @@ class HomeViewModel @Inject constructor( try { val detail = libraryRepository.refreshAlbumDetail(albumId) if (detail.tracks.isEmpty()) { - poolMessages.trySend("This album has no tracks to play.") + snackbarMessages.trySend("This album has no tracks to play.") } else { player.setQueue(detail.tracks, initialIndex = 0, source = "album:$albumId") } } catch ( @Suppress("TooGenericExceptionCaught") e: Throwable, ) { - poolMessages.trySend( + snackbarMessages.trySend( "Couldn't start playback: ${ErrorCopy.fromThrowable(e)}", ) } @@ -290,14 +296,14 @@ class HomeViewModel @Inject constructor( try { val tracks = libraryRepository.fetchArtistTracks(artistId).shuffled() if (tracks.isEmpty()) { - poolMessages.trySend("This artist has no tracks to play.") + snackbarMessages.trySend("This artist has no tracks to play.") } else { player.setQueue(tracks, initialIndex = 0, source = "artist:$artistId") } } catch ( @Suppress("TooGenericExceptionCaught") e: Throwable, ) { - poolMessages.trySend( + snackbarMessages.trySend( "Couldn't start playback: ${ErrorCopy.fromThrowable(e)}", ) } @@ -315,7 +321,7 @@ class HomeViewModel @Inject constructor( suspend fun playPlaylist(playlist: PlaylistRef) { viewModelScope.launch { playPlaylistShuffled(playlist, playlistsRepository, player) { - poolMessages.trySend(it) + snackbarMessages.trySend(it) } }.join() } @@ -323,9 +329,8 @@ class HomeViewModel @Inject constructor( /** * Pulls /home/index, the playlists list and the system-playlist * status. Returns true when the load-bearing /home/index pull - * succeeded — the veil controller retries on false, so this must - * report failure rather than swallow it the way the fire-and-forget - * [refresh] entry point does. + * succeeded — the veil controller retries on false and reports the + * outcome, so this must report failure rather than swallow it. */ private suspend fun runRefresh(): Boolean = coroutineScope { // /home/index is the load-bearing pull: its failure drives the @@ -352,19 +357,31 @@ class HomeViewModel @Inject constructor( home.await() } - /** Unveiled refresh, for the Error state's explicit Retry button. */ - fun refresh(): Job = viewModelScope.launch { runRefresh() } + /** + * The Error state's explicit Retry button. User-initiated, so it gets + * the controller's retries and reports its outcome; over an empty cache + * there's no content to protect, so no veil goes up. + */ + fun retry() = veil.request(userInitiated = true) /** - * Manual pull-to-refresh. Goes behind the veil like every other - * refresh (operator call, 2026-07-31: the churn a pull causes is - * identical to the automatic paths, and a small spinner didn't hide - * it). Suspends only long enough to hand the indicator off to the - * veil; the refresh itself continues in the controller's session. + * Manual pull-to-refresh. Goes behind the veil like every other refresh + * (operator call, 2026-07-31: the churn a pull causes is identical to + * the automatic paths, and a small spinner didn't hide it). + * + * Suspends until the veil has taken over OR the session has finished, + * so the pull indicator hands off to exactly one successor: the veil if + * content changed, the "Already up to date" snackbar if it didn't. The + * timeout is only a backstop against a session that outlives it. */ suspend fun refreshFromPull() { - veil.request() - withTimeoutOrNull(PULL_HANDOFF_TIMEOUT_MS) { veil.visible.first { it } } + val before = veil.finishedSessions.value + veil.request(userInitiated = true) + withTimeoutOrNull(PULL_HANDOFF_TIMEOUT_MS) { + combine(veil.visible, veil.finishedSessions) { veiled, finished -> + veiled || finished != before + }.first { it } + } } val uiState: StateFlow> = @@ -458,14 +475,38 @@ class HomeViewModel @Inject constructor( // right before the cache emits. uiState.value is UiState.Success || homeRepository.hasCachedIndex() }, + onSessionEnd = ::reportRefreshOutcome, work = ::runRefresh, ) /** - * True while the "Updating your mixes…" veil should be raised. The - * controller holds it until Home actually settles — sections swapped, - * tiles hydrated, artwork loaded — instead of for a fixed delay after - * the network pull returns (issue #2327). + * Tells the user how a refresh *they asked for* went, in the one case + * the veil can't: when nothing changed there's no veil to see, and a + * pull that produces no visible response at all reads as broken. + * + * Only user-initiated sessions say anything. The same outcome from a + * background check — the initial load, the 03:00 rebuild, a reconnect — + * is noise, and "Already up to date" on every launch would be worse + * than silence (operator's call, 2026-07-31). + */ + private fun reportRefreshOutcome(result: VeilSessionResult) { + if (!result.userInitiated) return + when (result.outcome) { + // The veil was the feedback. + VeilOutcome.CHANGED -> return + VeilOutcome.UNCHANGED -> snackbarMessages.trySend("Already up to date") + VeilOutcome.FAILED -> snackbarMessages.trySend("Couldn't check for updates") + } + } + + /** + * True while the "Updating your mixes…" veil should be raised. + * + * Raised only when a refresh actually changes what's on screen, and then + * held until Home settles — tiles hydrated, artwork loaded — instead of + * for a fixed delay after the network pull returns (issue #2327). A + * refresh that returns what's already cached shows no veil at all; + * [reportRefreshOutcome] tells the user instead, if they asked. */ val isUpdating: StateFlow = veil.visible @@ -589,7 +630,7 @@ private fun HomeStateCrossfade( is UiState.Error -> ErrorRetry( title = "Couldn't load home", message = s.message, - onRetry = { viewModel.refresh() }, + onRetry = { viewModel.retry() }, ) is UiState.Success -> HomeSuccessContent( sections = s.data, diff --git a/android/app/src/main/java/com/fabledsword/minstrel/shared/UpdateVeilController.kt b/android/app/src/main/java/com/fabledsword/minstrel/shared/UpdateVeilController.kt index 2d1fabd8..0e7161a0 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/shared/UpdateVeilController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/shared/UpdateVeilController.kt @@ -12,8 +12,10 @@ import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import kotlinx.coroutines.withTimeoutOrNull +import java.util.concurrent.atomic.AtomicBoolean // Once raised, the veil stays up at least this long. Without a floor a // no-op refresh wipes on and straight back off, which reads as a glitch. @@ -61,9 +63,33 @@ data class VeilSettleState( val quiescent: Boolean, ) +/** What a finished session did, so callers can report it if they want. */ +enum class VeilOutcome { + /** Content changed, and the veil covered the churn. */ + CHANGED, + + /** The refresh worked, but nothing on screen moved — already current. */ + UNCHANGED, + + /** Every attempt failed. */ + FAILED, +} + /** - * Drives an "updating" overlay from *observed content settling* rather - * than from a fixed delay. + * One session's result, plus whether a user explicitly asked for it. + * + * [userInitiated] is what lets a caller tell feedback from noise: a user + * who pulled to refresh is owed an answer even when the answer is "nothing + * changed", while the same outcome from a background check is noise. + */ +data class VeilSessionResult( + val outcome: VeilOutcome, + val userInitiated: Boolean, +) + +/** + * Drives an "updating" overlay from *observed content change and settling* + * rather than from a fixed delay. * * The problem this replaces: a veil held for `refresh().join() + 500ms` * lowers while the screen is still moving, because finishing the network @@ -73,31 +99,44 @@ data class VeilSettleState( * two overlapping refreshes finishes first, wiping the veil off mid-update * (issue #2327). * - * So instead: raise, run [work], then hold until [settleSignal] reports - * the screen has stopped changing for [VeilTimings.quietMs] AND is - * quiescent — bounded below by [VeilTimings.minHoldMs] so it can never - * flash, and above by [VeilTimings.maxHoldMs] so it can never strand. + * So instead: run [work], raise only if the content actually changes, then + * hold until [settleSignal] reports the screen has stopped changing for + * [VeilTimings.quietMs] AND is quiescent — bounded below by + * [VeilTimings.minHoldMs] so it can never flash, and above by + * [VeilTimings.maxHoldMs] so it can never strand. + * + * The raise is deliberately *reactive*: a refresh that returns what's + * already on screen — the common case on a launch over a warm cache — + * raises nothing at all, because a veil over an unchanged screen hides + * nothing and only delays first paint. The cost is that the veil arrives + * one emission after the change, so a single atomic content swap shows + * through; everything messier that follows it (tile hydration, then + * artwork) still lands behind the veil. * * Overlapping triggers extend the running session instead of racing it, * so the veil stays up continuously rather than lowering and re-raising. * - * Failure is quiet by design: [work] gets [VeilTimings.attempts] tries - * behind the veil, and if they all fail the veil simply wipes off over - * the cached content with no error surfaced. Giving up here ends only - * *this* session — it sets no latch and blocks nothing, so the caller's - * own recovery paths (reconnect re-pull, freshness sweeps, the next - * event, a manual pull) keep retrying afterwards exactly as before. + * Failure is quiet at this layer: [work] gets [VeilTimings.attempts] tries + * behind the veil, and if they all fail the veil simply wipes off over the + * cached content. Giving up ends only *this* session — it sets no latch and + * blocks nothing, so the caller's own recovery paths (reconnect re-pull, + * freshness sweeps, the next event, a manual pull) keep retrying afterwards + * exactly as before. Callers that want to surface a failure can do it from + * [onSessionEnd] instead. * * @param work one refresh attempt; returns true when it succeeded. * @param shouldVeil sampled at session start — "is there cached content * this refresh is about to overwrite?". False means a cold load, where * a skeleton is the right affordance, and the work runs unveiled. + * @param onSessionEnd called once per finished session, on the controller's + * coroutine. Use it for user-facing feedback the veil itself can't give. */ class UpdateVeilController( private val scope: CoroutineScope, private val settleSignal: Flow, private val shouldVeil: suspend () -> Boolean, private val timings: VeilTimings = VeilTimings(), + private val onSessionEnd: (VeilSessionResult) -> Unit = {}, private val work: suspend () -> Boolean, ) { private val visibleInternal = MutableStateFlow(false) @@ -105,10 +144,25 @@ class UpdateVeilController( /** True while the veil should be drawn over the screen. */ val visible: StateFlow = visibleInternal.asStateFlow() + private val finishedInternal = MutableStateFlow(0) + + /** + * Increments as each session ends. Lets a caller wait for "this + * refresh is done" without knowing whether a veil ever went up — + * a pull-to-refresh indicator needs exactly that, since an unchanged + * refresh never raises one. + */ + val finishedSessions: StateFlow = finishedInternal.asStateFlow() + // Conflated: a burst of triggers (reconnect + rebuild event arriving // together) collapses into one follow-up pass, not a queue of them. private val requests = Channel(Channel.CONFLATED) + // Sticky across a conflated burst: conflation drops the older token, so + // the "a user asked for this" bit can't ride on it. If ANY coalesced + // trigger was the user's, the session still owes them an answer. + private val userAsked = AtomicBoolean(false) + init { // One consumer, so sessions are serialised by construction: two // triggers can never each own a piece of the veil's state. @@ -124,28 +178,31 @@ class UpdateVeilController( * Ask for a refresh. Safe to call from any trigger at any rate — * calls arriving during a session extend it rather than starting a * competing one. + * + * @param userInitiated true when a person explicitly asked (pull to + * refresh, a Retry button), which is what [VeilSessionResult] carries + * through to [onSessionEnd]. */ - fun request() { + fun request(userInitiated: Boolean = false) { + if (userInitiated) userAsked.set(true) requests.trySend(Unit) } private suspend fun runSession() { + // Sampled at both ends of the work: a trigger folded in mid-session + // (see [drainWork]) may have been the user's, and they're still owed + // an answer for it. + val askedAtStart = userAsked.getAndSet(false) if (!shouldVeil()) { - drainWork() + // Cold load: the skeleton is the right affordance, so no veil. + // Succeeding here did change the screen — from nothing to + // something — so it reports CHANGED, never "already up to date". + val ok = drainWork() + finish(succeeded = ok, changed = ok, userInitiated = askedAtStart) return } - // Raise only once there's content on screen to hide. Over a warm - // cache that's within a frame or two of here — long before the - // network pull lands — so the churn still gets covered. But on a - // genuinely cold load, content appears only *because* this work - // produced it, and veiling that would delay first paint to hide - // nothing. val raised = CompletableDeferred() - val raiser = scope.launch { - settleSignal.first { it.hasContent } - visibleInternal.value = true - raised.complete(Unit) - } + val raiser = scope.launch { raiseWhenContentChanges(raised) } // Floor and ceiling are measured from the raise, not the request, // so a late raise still gets its full no-flash minimum. val floor = scope.launch { @@ -157,12 +214,16 @@ class UpdateVeilController( delay(timings.maxHoldMs) visibleInternal.value = false } + var succeeded = false try { - drainWork() + succeeded = drainWork() // Always wait for the settle, never conditionally on `visible`: // work that finishes without suspending would otherwise reach // here before the raiser has been dispatched, tear the session - // down, and leave the churn uncovered. + // down, and leave the churn uncovered. This wait is also what + // makes `raised.isCompleted` below a trustworthy "did anything + // change?" — a change landing just after the pull returns still + // gets seen. withTimeoutOrNull(timings.maxHoldMs) { awaitSettled() } // Honour the no-flash minimum before lowering. Deliberately in // the try and not the finally: on cancellation the scope is @@ -175,23 +236,64 @@ class UpdateVeilController( ceiling.cancel() floor.cancel() visibleInternal.value = false + finish( + succeeded = succeeded, + changed = raised.isCompleted, + userInitiated = askedAtStart, + ) } } - private suspend fun drainWork() { - do { - runWorkWithRetries() - // A trigger that arrived mid-session gets folded into this one. - } while (requests.tryReceive().isSuccess) + /** + * Raises the veil the moment the screen's content differs from what was + * already on it — and never, if this refresh turns out to be a no-op. + * + * The baseline is the first state that HAS content, not simply the first + * state: over a warm cache the cached rows paint a moment after the + * session starts, and treating that first paint as "a change" would veil + * every launch, which is the whole thing this avoids. + */ + private suspend fun raiseWhenContentChanges(raised: CompletableDeferred) { + val baseline = settleSignal.first { it.hasContent } + settleSignal.first { it.hasContent && it.contentKey != baseline.contentKey } + visibleInternal.value = true + raised.complete(Unit) } - private suspend fun runWorkWithRetries() { + private fun finish(succeeded: Boolean, changed: Boolean, userInitiated: Boolean) { + val outcome = when { + !succeeded -> VeilOutcome.FAILED + changed -> VeilOutcome.CHANGED + else -> VeilOutcome.UNCHANGED + } + onSessionEnd( + VeilSessionResult( + outcome = outcome, + // Fold in a mid-session request from the user. + userInitiated = userInitiated || userAsked.getAndSet(false), + ), + ) + finishedInternal.update { it + 1 } + } + + /** True when the refresh eventually succeeded. */ + private suspend fun drainWork(): Boolean { + var succeeded: Boolean + do { + succeeded = runWorkWithRetries() + // A trigger that arrived mid-session gets folded into this one. + } while (requests.tryReceive().isSuccess) + return succeeded + } + + private suspend fun runWorkWithRetries(): Boolean { repeat(timings.attempts) { attempt -> - if (work()) return + if (work()) return true if (attempt < timings.attempts - 1) { delay(timings.retryBackoffMs * (attempt + 1)) } } + return false } /** diff --git a/android/app/src/test/java/com/fabledsword/minstrel/shared/UpdateVeilControllerTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/shared/UpdateVeilControllerTest.kt index e57027f9..58c98cf6 100644 --- a/android/app/src/test/java/com/fabledsword/minstrel/shared/UpdateVeilControllerTest.kt +++ b/android/app/src/test/java/com/fabledsword/minstrel/shared/UpdateVeilControllerTest.kt @@ -15,7 +15,6 @@ import kotlin.test.assertFalse import kotlin.test.assertTrue private const val WORK_MS = 1_000L -private const val SLOW_WORK_MS = 5_000L private const val CHURN_ROUNDS = 5 private const val ART_IN_FLIGHT = 3 private const val SUCCEED_ON_ATTEMPT = 3 @@ -29,9 +28,9 @@ private const val QUIET_WINDOWS_TO_OUTLAST = 3 private const val DRAIN_MS = 3_000L /** - * The veil's job is to stay up until the screen has stopped moving. Each - * test pins one of the ways the previous fixed-delay implementation - * lowered it too early (issue #2327). + * The veil's job is to go up only when content actually changes, and then to + * stay up until the screen has stopped moving. Each test pins one of the ways + * the original fixed-delay implementation got that wrong (issue #2327). * * The controller is built on `backgroundScope` throughout: its consumer * loop runs forever, so hanging it off the test's own scope would stop @@ -64,7 +63,7 @@ class UpdateVeilControllerTest { VeilSettleState(contentKey = key, hasContent = hasContent, quiescent = art == 0) } - /** Something visibly changed — re-arms the quiet window. */ + /** Content visibly changed — what the veil exists to cover. */ fun churn() { state.value = state.value.copy(first = state.value.first + 1) } @@ -81,16 +80,18 @@ class UpdateVeilControllerTest { private fun TestScope.controllerOn( screen: FakeScreen, shouldVeil: suspend () -> Boolean = { true }, + onSessionEnd: (VeilSessionResult) -> Unit = {}, work: suspend () -> Boolean, ) = UpdateVeilController( scope = backgroundScope, settleSignal = screen.signal, shouldVeil = shouldVeil, timings = timings, + onSessionEnd = onSessionEnd, work = work, ) - /** Records every visibility transition, so a blink can't hide. */ + /** Records every visibility transition, so an extra raise can't hide. */ private fun TestScope.recordVisibility(controller: UpdateVeilController): List { val seen = mutableListOf() backgroundScope.launch { controller.visible.collect { seen.add(it) } } @@ -104,6 +105,9 @@ class UpdateVeilControllerTest { controller.request() runCurrent() + // The pull's write lands: content changed, so the veil goes up. + screen.churn() + runCurrent() assertTrue(controller.visible.value, "veil is up while the screen is still moving") // Tiles hydrating one after another, each inside the quiet window. @@ -126,6 +130,9 @@ class UpdateVeilControllerTest { screen.artLoading(ART_IN_FLIGHT) controller.request() + runCurrent() + screen.churn() + runCurrent() // Well past the quiet window and the floor — but art is still in // flight, so lowering now would show the covers popping in. advanceTimeBy(timings.minHoldMs + timings.quietMs * QUIET_WINDOWS_TO_OUTLAST) @@ -137,43 +144,53 @@ class UpdateVeilControllerTest { } @Test - fun `veil holds through failed attempts and their retries`() = runTest { + fun `retries quietly, then veils the churn the successful attempt produces`() = runTest { val screen = FakeScreen() var attempts = 0 val controller = controllerOn(screen) { attempts++ - attempts >= SUCCEED_ON_ATTEMPT // fail twice, succeed on the third + val succeeded = attempts >= SUCCEED_ON_ATTEMPT // fail twice + // Only a pull that worked writes anything. + if (succeeded) screen.churn() + succeeded } val seen = recordVisibility(controller) controller.request() runCurrent() - assertTrue(controller.visible.value) - advanceTimeBy(timings.retryBackoffMs + 1) - assertTrue(controller.visible.value, "veil stays up across the backoff") + // A failed pull changes nothing, so there is nothing to hide yet — + // the retries happen with no veil at all. + assertFalse(controller.visible.value, "no veil over a pull that changed nothing") advanceTimeBy(DRAIN_MS) assertEquals(SUCCEED_ON_ATTEMPT, attempts, "retries until the pull succeeds") - assertEquals(listOf(false, true, false), seen, "raised once, lowered once") + assertEquals( + listOf(false, true, false), + seen, + "the veil went up once, over the churn the retry finally produced", + ) } @Test - fun `giving up lowers the veil quietly and does not block later requests`() = runTest { + fun `giving up is silent, reports FAILED, and does not block later requests`() = runTest { val screen = FakeScreen() + val results = mutableListOf() var attempts = 0 var succeed = false - val controller = controllerOn(screen) { + val controller = controllerOn(screen, onSessionEnd = { results += it }) { attempts++ succeed } - controller.request() + controller.request(userInitiated = true) advanceTimeBy(DRAIN_MS) assertEquals(timings.attempts, attempts, "exhausts its attempts") - assertFalse(controller.visible.value, "gives up silently") + assertFalse(controller.visible.value, "no veil — a failed pull changed nothing") + assertEquals(VeilOutcome.FAILED, results.single().outcome) + assertTrue(results.single().userInitiated, "the user asked, so they're owed an answer") - // The operator's requirement: giving up must not latch anything off - // — the reconnect-driven recovery still gets to try again later. + // Giving up must not latch anything off — the reconnect-driven + // recovery still gets to try again later. succeed = true controller.request() advanceTimeBy(DRAIN_MS) @@ -187,6 +204,7 @@ class UpdateVeilControllerTest { val controller = controllerOn(screen) { started++ delay(WORK_MS) + screen.churn() true } val seen = recordVisibility(controller) @@ -208,7 +226,10 @@ class UpdateVeilControllerTest { @Test fun `a never-settling screen still releases the veil at the ceiling`() = runTest { val screen = FakeScreen() - val controller = controllerOn(screen) { true } + val controller = controllerOn(screen) { + screen.churn() + true + } screen.artLoading(1) // an image that never completes controller.request() @@ -217,40 +238,77 @@ class UpdateVeilControllerTest { } @Test - fun `a cold load runs unveiled`() = runTest { + fun `an unchanged refresh never raises the veil and reports UNCHANGED`() = runTest { val screen = FakeScreen() + val results = mutableListOf() + // Succeeds without writing anything — the common case on a launch + // over a warm cache, where the server returns what's already cached. + val controller = controllerOn(screen, onSessionEnd = { results += it }) { true } + val seen = recordVisibility(controller) + + controller.request(userInitiated = true) + advanceTimeBy(DRAIN_MS) + + assertEquals(listOf(false), seen, "a veil over an unchanged screen would hide nothing") + assertEquals(VeilOutcome.UNCHANGED, results.single().outcome) + assertTrue(results.single().userInitiated, "so the caller can say 'already up to date'") + } + + @Test + fun `cached content painting is not mistaken for a change`() = runTest { + // Warm cache that hasn't painted yet: the rows arrive a moment after + // the session starts. Treating that first paint as churn would veil + // every single launch. + val screen = FakeScreen(hasContent = false) + val controller = controllerOn(screen) { true } + + controller.request() + runCurrent() + screen.contentAppears() + advanceTimeBy(DRAIN_MS) + + assertFalse(controller.visible.value, "first paint is not churn") + } + + @Test + fun `a background trigger coalescing with the user's does not swallow their answer`() = + runTest { + val screen = FakeScreen() + val results = mutableListOf() + val controller = controllerOn(screen, onSessionEnd = { results += it }) { true } + + // Conflation drops the older token, so the "a user asked" bit + // cannot ride on it — it's tracked separately for exactly this. + controller.request(userInitiated = true) + controller.request() + advanceTimeBy(DRAIN_MS) + + assertTrue( + results.first().userInitiated, + "the user's request must not be conflated away", + ) + } + + @Test + fun `a cold load runs unveiled and is never reported as already up to date`() = runTest { + val screen = FakeScreen() + val results = mutableListOf() var ran = false val controller = controllerOn( screen, shouldVeil = { false }, // empty cache: the skeleton owns this + onSessionEnd = { results += it }, ) { ran = true true } - controller.request() + controller.request(userInitiated = true) advanceTimeBy(DRAIN_MS) + assertTrue(ran, "the refresh still happens") assertFalse(controller.visible.value, "but no veil over a skeleton") - } - - @Test - fun `veil waits for content to paint before raising`() = runTest { - val screen = FakeScreen(hasContent = false) - val controller = controllerOn(screen) { - delay(SLOW_WORK_MS) - true - } - - controller.request() - advanceTimeBy(WORK_MS) - assertFalse(controller.visible.value, "nothing to hide until content is up") - - screen.contentAppears() - runCurrent() - assertTrue(controller.visible.value, "raises the moment cached content paints") - - advanceTimeBy(SLOW_WORK_MS + DRAIN_MS) - assertFalse(controller.visible.value) + // It went from nothing to something — that IS a change. + assertEquals(VeilOutcome.CHANGED, results.single().outcome) } }