From 5044e7a05599943a5a25db994800082e9b74503e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 31 Jul 2026 20:31:30 -0400 Subject: [PATCH 1/4] =?UTF-8?q?fix(home):=20hold=20the=20updating=20veil?= =?UTF-8?q?=20until=20Home=20actually=20settles=20=E2=80=94=20#2327?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Updating your mixes…" veil wiped on and straight back off before the update finished, and a number of churn paths never raised it at all. Three reasons it lowered early. refreshBehindVeil held it for refresh().join() + a flat 500ms, but finishing the network pull is nowhere near the end of the visible work: refreshIndex writes only the section id lists, then each tile hydrates through MetadataProvider (null → skeleton → album), and only then does the cover art load. Second, updatingInternal was a plain Boolean cleared in a finally — reconnect and playlist.system_rebuilt routinely arrive together, so whichever pull finished first wiped the veil off while the other was still running. Third, refresh() swallowed every failure in runCatching, so join() returned "fine" after a failed pull: veil off, content unchanged, no retry. So the veil's lifetime is now driven by watching the screen instead of by a guess. UpdateVeilController raises, runs the work (retrying behind the veil), then holds until the content signature has been unchanged for a quiet window AND nothing is still loading — floored by a minimum hold so it cannot flash, capped by a hard ceiling so it cannot strand, and with overlapping triggers folded into one session rather than racing it. Giving up is silent and sets no latch: the reconnect-driven recovery and the freshness sweeper keep retrying afterwards exactly as before. Cover art was the most visible pop-in and the refresh coroutine cannot see it, so the composition reports it upward: ServerImage — the single choke point behind CoverTile for every album/artist/playlist cover — counts its in-flight loads into an ArtSettleTracker the veil waits on. Art also crossfades now (set once on the ImageLoader, so it applies app-wide) with the placeholder fading out over the same window, which softens the pop everywhere the veil isn't involved. Underneath all of it, the churn is largely no longer generated. replaceSection was delete-then-insert per section, un-transacted, so observeBySection emitted emptyList() — a visible collapse — before refilling, seven times in sequence. It is now one @Transaction across all sections (Room notifies once, on commit, so the empty gap is never observed), and the index flow dedups on the id list, so a section whose contents did not move no longer tears down and rebuilds every tile's hydration flow. fetchedAt is restamped on every write, which is why the dedup compares ids rather than rows. Same fix CachedQuarantineDao already carried for the same reason. Trigger set widened per the operator's call: the initial load over a warm cache (a full re-pull that churned every section completely unveiled), manual pull-to-refresh, scan.run_finished (Home never reacted to it at all), and the playlist.created/updated/deleted/tracks_changed kinds. The veil waits for content to be on screen before raising, so a genuinely cold load still gets its skeleton rather than an opaque panel over nothing. refreshError is now cleared on success rather than at the start of each attempt — with retries, clearing it up front made a failing cold start flash the "Welcome to Minstrel" empty state between attempts. Co-Authored-By: Claude Opus 5 (1M context) --- .../minstrel/MinstrelApplication.kt | 12 + .../cache/db/dao/CachedHomeIndexDao.kt | 27 +- .../minstrel/home/data/HomeRepository.kt | 114 ++++++--- .../minstrel/home/ui/HomeScreen.kt | 222 +++++++++++----- .../minstrel/shared/UpdateVeilController.kt | 215 ++++++++++++++++ .../shared/widgets/ArtSettleTracker.kt | 60 +++++ .../minstrel/shared/widgets/ServerImage.kt | 40 ++- .../shared/UpdateVeilControllerTest.kt | 242 ++++++++++++++++++ 8 files changed, 822 insertions(+), 110 deletions(-) create mode 100644 android/app/src/main/java/com/fabledsword/minstrel/shared/UpdateVeilController.kt create mode 100644 android/app/src/main/java/com/fabledsword/minstrel/shared/widgets/ArtSettleTracker.kt create mode 100644 android/app/src/test/java/com/fabledsword/minstrel/shared/UpdateVeilControllerTest.kt diff --git a/android/app/src/main/java/com/fabledsword/minstrel/MinstrelApplication.kt b/android/app/src/main/java/com/fabledsword/minstrel/MinstrelApplication.kt index 65a34df6..7009a7df 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/MinstrelApplication.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/MinstrelApplication.kt @@ -6,6 +6,7 @@ import androidx.work.Configuration import coil3.ImageLoader import coil3.SingletonImageLoader import coil3.network.okhttp.OkHttpNetworkFetcherFactory +import coil3.request.crossfade import com.fabledsword.minstrel.cache.CacheIndexer import com.fabledsword.minstrel.cache.mutations.MutationReplayer import com.fabledsword.minstrel.cache.sync.SyncController @@ -29,6 +30,10 @@ import okhttp3.OkHttpClient import timber.log.Timber import javax.inject.Inject +// Cover-art fade-in. Coil skips the transition for memory-cache hits, so +// already-loaded art still appears instantly — only a genuine fetch fades. +private const val ART_CROSSFADE_MS = 220 + @HiltAndroidApp class MinstrelApplication : Application(), @@ -213,11 +218,18 @@ class MinstrelApplication : * OkHttp client as the network fetcher. The `callFactory` lambda * is invoked lazily so Hilt has time to inject `okHttpClient` * before Coil makes its first request. + * + * Crossfade is set here rather than per-call so every cover surface + * in the app fades its artwork in instead of snapping it. Art + * landing a beat after its tile was the most visible pop-in on Home + * (issue #2327); `ServerImage` fades its placeholder out over the + * same window so the two read as one cross-dissolve. */ override fun newImageLoader(context: android.content.Context): ImageLoader = ImageLoader.Builder(context) .components { add(OkHttpNetworkFetcherFactory(callFactory = { okHttpClient })) } + .crossfade(ART_CROSSFADE_MS) .build() } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/cache/db/dao/CachedHomeIndexDao.kt b/android/app/src/main/java/com/fabledsword/minstrel/cache/db/dao/CachedHomeIndexDao.kt index aea08ff3..35608279 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/cache/db/dao/CachedHomeIndexDao.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/cache/db/dao/CachedHomeIndexDao.kt @@ -4,6 +4,7 @@ import androidx.room.Dao import androidx.room.Insert import androidx.room.OnConflictStrategy import androidx.room.Query +import androidx.room.Transaction import com.fabledsword.minstrel.cache.db.entities.CachedHomeIndexEntity import kotlinx.coroutines.flow.Flow @@ -21,12 +22,32 @@ interface CachedHomeIndexDao { ) suspend fun getBySection(section: String): List + /** True when Home has any cached section rows to render. */ + @Query("SELECT EXISTS(SELECT 1 FROM cached_home_index)") + suspend fun hasAny(): Boolean + @Insert(onConflict = OnConflictStrategy.REPLACE) suspend fun upsertAll(rows: List) - /** Replace-all pattern; sync wipes a section then re-inserts. */ - @Query("DELETE FROM cached_home_index WHERE section = :section") - suspend fun deleteBySection(section: String) + @Query("DELETE FROM cached_home_index WHERE section IN (:sections)") + suspend fun deleteSections(sections: List) + + /** + * Swaps every listed section's rows in ONE transaction. + * + * Atomicity is the point, not just tidiness: Room's + * InvalidationTracker only notifies observers after the transaction + * commits, so [observeBySection] never sees the empty gap between the + * delete and the re-insert. Replacing sections one at a time (and + * un-transacted) made each Home row emit `emptyList()` — visibly + * collapsing — before refilling, and made the seven sections do it in + * sequence rather than as a single content swap. + */ + @Transaction + suspend fun replaceSections(sections: List, rows: List) { + deleteSections(sections) + if (rows.isNotEmpty()) upsertAll(rows) + } @Query("DELETE FROM cached_home_index") suspend fun clear() diff --git a/android/app/src/main/java/com/fabledsword/minstrel/home/data/HomeRepository.kt b/android/app/src/main/java/com/fabledsword/minstrel/home/data/HomeRepository.kt index 25b3908c..8e3db94a 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/home/data/HomeRepository.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/home/data/HomeRepository.kt @@ -14,8 +14,10 @@ import com.fabledsword.minstrel.models.TrackRef import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.flow.map import retrofit2.Retrofit import retrofit2.create import javax.inject.Inject @@ -34,9 +36,10 @@ import javax.inject.Singleton * reveals when the fetch lands and Room re-emits. Mirrors Flutter's * per-item tile providers. * - * `refreshIndex()` pulls `GET /api/home/index`, replaces each section - * in-place (delete-then-insert, so the section Flows re-fire), and - * pre-warms the top artists via [HomeArtistPrewarmer]. + * `refreshIndex()` pulls `GET /api/home/index`, swaps all sections in + * one transaction (so the rows update together in a single emission + * rather than collapsing and refilling), and pre-warms the top artists + * via [HomeArtistPrewarmer]. */ @Singleton // Per-section observe accessors (one per Home row) inflate the function @@ -85,72 +88,98 @@ class HomeRepository @Inject constructor( fun observeYouMightLikeArtists(): Flow>> = observeArtistSection(SECTION_YOU_MIGHT_LIKE_ARTISTS) + /** True when the index cache already has content on screen to protect. */ + suspend fun hasCachedIndex(): Boolean = homeIndexDao.hasAny() + /** - * Pulls /api/home/index, replaces each cached_home_index section, - * and pre-warms the top artists. The section Flows re-fire on the - * index change; missing entity rows hydrate via the on-miss path. + * Pulls /api/home/index and swaps every cached_home_index section in + * a single transaction, then pre-warms the top artists. Missing + * entity rows hydrate via the on-miss path. + * + * One transaction for all seven sections is deliberate: Room notifies + * observers once, on commit, so Home swaps from the old content to + * the new in a single emission. Per-section, un-transacted writes + * made every row visibly collapse to empty and refill, one after + * another (issue #2327). */ suspend fun refreshIndex() { val wire = api.getHomeIndex() - replaceSection(SECTION_RECENTLY_ADDED_ALBUMS, "album", wire.recentlyAddedAlbums) - replaceSection(SECTION_REDISCOVER_ALBUMS, "album", wire.rediscoverAlbums) - replaceSection(SECTION_REDISCOVER_ARTISTS, "artist", wire.rediscoverArtists) - replaceSection(SECTION_MOST_PLAYED_TRACKS, "track", wire.mostPlayedTracks) - replaceSection(SECTION_LAST_PLAYED_ARTISTS, "artist", wire.lastPlayedArtists) - replaceSection(SECTION_YOU_MIGHT_LIKE_ALBUMS, "album", wire.youMightLikeAlbums) - replaceSection(SECTION_YOU_MIGHT_LIKE_ARTISTS, "artist", wire.youMightLikeArtists) + homeIndexDao.replaceSections( + sections = ALL_SECTIONS, + rows = rowsFor(SECTION_RECENTLY_ADDED_ALBUMS, "album", wire.recentlyAddedAlbums) + + rowsFor(SECTION_REDISCOVER_ALBUMS, "album", wire.rediscoverAlbums) + + rowsFor(SECTION_REDISCOVER_ARTISTS, "artist", wire.rediscoverArtists) + + rowsFor(SECTION_MOST_PLAYED_TRACKS, "track", wire.mostPlayedTracks) + + rowsFor(SECTION_LAST_PLAYED_ARTISTS, "artist", wire.lastPlayedArtists) + + rowsFor(SECTION_YOU_MIGHT_LIKE_ALBUMS, "album", wire.youMightLikeAlbums) + + rowsFor(SECTION_YOU_MIGHT_LIKE_ARTISTS, "artist", wire.youMightLikeArtists), + ) prewarmer.warm( wire.rediscoverArtists + wire.lastPlayedArtists + wire.youMightLikeArtists, ) } - private suspend fun replaceSection(section: String, entityType: String, ids: List) { - homeIndexDao.deleteBySection(section) - if (ids.isEmpty()) return - homeIndexDao.upsertAll( - ids.mapIndexed { index, id -> - CachedHomeIndexEntity( - section = section, - position = index, - entityType = entityType, - entityId = id, - ) - }, + private fun rowsFor( + section: String, + entityType: String, + ids: List, + ): List = ids.mapIndexed { index, id -> + CachedHomeIndexEntity( + section = section, + position = index, + entityType = entityType, + entityId = id, ) } + /** + * The section's ordered entity ids, deduplicated. + * + * Room re-runs the query on every write to `cached_home_index` — and + * `CachedHomeIndexEntity.fetchedAt` is stamped fresh each time — so + * comparing whole rows would call every rewrite a change. Comparing + * the id list instead means a section whose contents didn't actually + * move never restarts the `flatMapLatest` below, which would + * otherwise tear down and rebuild all of its tiles' hydration flows + * and flicker unchanged tiles (issue #2327). + */ + private fun observeSectionIds(section: String): Flow> = + homeIndexDao.observeBySection(section) + .map { rows -> rows.map { it.entityId } } + .distinctUntilChanged() + @OptIn(ExperimentalCoroutinesApi::class) private fun observeAlbumSection(section: String): Flow>> = - homeIndexDao.observeBySection(section).flatMapLatest { rows -> - if (rows.isEmpty()) { + observeSectionIds(section).flatMapLatest { ids -> + if (ids.isEmpty()) { flowOf(emptyList()) } else { - combine(rows.map { metadataProvider.observeAlbum(it.entityId) }) { refs -> - rows.mapIndexed { i, r -> HomeTile(r.entityId, refs[i]) } + combine(ids.map { metadataProvider.observeAlbum(it) }) { refs -> + ids.mapIndexed { i, id -> HomeTile(id, refs[i]) } } } } @OptIn(ExperimentalCoroutinesApi::class) private fun observeArtistSection(section: String): Flow>> = - homeIndexDao.observeBySection(section).flatMapLatest { rows -> - if (rows.isEmpty()) { + observeSectionIds(section).flatMapLatest { ids -> + if (ids.isEmpty()) { flowOf(emptyList()) } else { - combine(rows.map { metadataProvider.observeArtist(it.entityId) }) { refs -> - rows.mapIndexed { i, r -> HomeTile(r.entityId, refs[i]) } + combine(ids.map { metadataProvider.observeArtist(it) }) { refs -> + ids.mapIndexed { i, id -> HomeTile(id, refs[i]) } } } } @OptIn(ExperimentalCoroutinesApi::class) private fun observeTrackSection(section: String): Flow>> = - homeIndexDao.observeBySection(section).flatMapLatest { rows -> - if (rows.isEmpty()) { + observeSectionIds(section).flatMapLatest { ids -> + if (ids.isEmpty()) { flowOf(emptyList()) } else { - combine(rows.map { metadataProvider.observeTrack(it.entityId) }) { refs -> - rows.mapIndexed { i, r -> HomeTile(r.entityId, refs[i]) } + combine(ids.map { metadataProvider.observeTrack(it) }) { refs -> + ids.mapIndexed { i, id -> HomeTile(id, refs[i]) } } } } @@ -163,5 +192,16 @@ class HomeRepository @Inject constructor( const val SECTION_LAST_PLAYED_ARTISTS = "last_played_artists" const val SECTION_YOU_MIGHT_LIKE_ALBUMS = "you_might_like_albums" const val SECTION_YOU_MIGHT_LIKE_ARTISTS = "you_might_like_artists" + + /** Every section [refreshIndex] owns — the unit of one atomic swap. */ + val ALL_SECTIONS = listOf( + SECTION_RECENTLY_ADDED_ALBUMS, + SECTION_REDISCOVER_ALBUMS, + SECTION_REDISCOVER_ARTISTS, + SECTION_MOST_PLAYED_TRACKS, + SECTION_LAST_PLAYED_ARTISTS, + SECTION_YOU_MIGHT_LIKE_ALBUMS, + SECTION_YOU_MIGHT_LIKE_ARTISTS, + ) } } 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 defdb766..8b6b6313 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 @@ -43,6 +43,7 @@ import androidx.compose.material3.SnackbarHost import androidx.compose.material3.SnackbarHostState import androidx.compose.material3.Text import androidx.compose.runtime.Composable +import androidx.compose.runtime.CompositionLocalProvider import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.remember @@ -85,10 +86,14 @@ import com.fabledsword.minstrel.playlists.widgets.OfflinePoolCard 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.VeilSettleState import com.fabledsword.minstrel.shared.asCacheFirstStateFlow +import com.fabledsword.minstrel.shared.widgets.ArtSettleTracker import com.fabledsword.minstrel.shared.widgets.EmptyState import com.fabledsword.minstrel.shared.widgets.ErrorRetry import com.fabledsword.minstrel.shared.widgets.HorizontalScrollRow +import com.fabledsword.minstrel.shared.widgets.LocalArtSettleTracker import com.fabledsword.minstrel.shared.widgets.MinstrelTopAppBar import com.fabledsword.minstrel.shared.widgets.PullToRefreshScaffold import com.fabledsword.minstrel.shared.widgets.SkeletonAlbumTile @@ -96,19 +101,22 @@ 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.delay +import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.receiveAsFlow import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.filter import kotlinx.coroutines.launch +import kotlinx.coroutines.withTimeoutOrNull import javax.inject.Inject private const val SHARE_STOP_TIMEOUT_MS = 5_000L @@ -120,16 +128,20 @@ 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 (automatic refresh). Held through the pull -// plus VEIL_SETTLE_MS so per-tile hydration lands behind it before it wipes -// off; near-opaque (VEIL_ALPHA) so the section churn never bleeds through. -private const val VEIL_SETTLE_MS = 500L +// "Updating your mixes…" veil. Its lifetime is decided by +// UpdateVeilController watching the screen settle — not 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 private const val VEIL_ALPHA = 0.96f 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. +private const val PULL_HANDOFF_TIMEOUT_MS = 2_000L + // ─── State ─────────────────────────────────────────────────────────── data class HomeSections( @@ -197,38 +209,13 @@ class HomeViewModel @Inject constructor( */ private val refreshError = MutableStateFlow(null) - private val updatingInternal = MutableStateFlow(false) - /** - * True while an automatic background refresh (the 03:00 daily rebuild - * or a reconnect re-pull) is repopulating Home. Drives the "Updating - * your mixes…" veil so the section churn — delete-then-insert in - * [HomeRepository.refreshIndex] plus per-tile hydration — happens - * hidden behind the veil instead of on screen. Manual pull-to-refresh - * and cold start are NOT veiled (they own the pull spinner / skeleton). + * Cover-art loads in flight on Home, reported by every [ServerImage] + * under [LocalArtSettleTracker]. The veil waits on this so artwork + * arriving a beat after its tile lands behind the veil rather than + * popping in on screen. */ - val isUpdating: StateFlow = updatingInternal.asStateFlow() - - init { - refresh() - // Screen-level auto-recovery (issue #1245): a Home that failed to - // load while the server was unreachable re-pulls itself the moment - // health returns — same idiom as SyncController, one layer up. - // Veiled: content is already on screen and would otherwise churn. - viewModelScope.launch { - networkStatus.recoveries().collect { refreshBehindVeil() } - } - // #968: the daily 03:00 rebuild (and manual refresh) emit - // playlist.system_rebuilt; re-pull Home so the system-playlist tiles - // and You-might-like rows reflect the new snapshot without a manual - // reload. Mirrors the web SSE consumer. Veiled so the multi-section - // rebuild churn hides behind "Updating your mixes…". - viewModelScope.launch { - eventsStream.events - .filter { it.kind == "playlist.system_rebuilt" } - .collect { refreshBehindVeil() } - } - } + val artTracker = ArtSettleTracker() /** * Tap an offline pool: shuffle + play its cached tracks. Empty @@ -334,47 +321,50 @@ class HomeViewModel @Inject constructor( } /** - * Pulls both /home/index and the playlists list. Returns the Job - * for the combined refresh so a pull-to-refresh wrapper can await - * actual completion before hiding the indicator. + * 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. */ - fun refresh(): Job = viewModelScope.launch { - refreshError.value = null - val home = launch { - // /home/index is the load-bearing pull: its failure drives the - // empty-cache Error state. A failure over a populated cache - // stays silent — cached sections beat a full-screen error. + private suspend fun runRefresh(): Boolean = coroutineScope { + // /home/index is the load-bearing pull: its failure drives the + // empty-cache Error state. A failure over a populated cache + // stays silent — cached sections beat a full-screen error. + // + // Cleared on success, NOT at the start of each attempt: with the + // veil's retries, clearing up front made a failing cold start + // flash the "Welcome to Minstrel" empty state (empty cache + no + // error reads as Empty) between one attempt and the next. + val home = async { runCatching { homeRepository.refreshIndex() } + .onSuccess { refreshError.value = null } .onFailure { refreshError.value = ErrorCopy.fromThrowable(it) } + .isSuccess } val lists = launch { runCatching { playlistsRepository.refreshList() } } val status = launch { runCatching { homeRepository.getSystemPlaylistsStatus() } .onSuccess { systemStatusInternal.value = it } } - home.join() lists.join() status.join() + home.await() } + /** Unveiled refresh, for the Error state's explicit Retry button. */ + fun refresh(): Job = viewModelScope.launch { runRefresh() } + /** - * Automatic background refresh with the "Updating your mixes…" veil - * raised (see [isUpdating]). Used by the daily-rebuild + reconnect - * paths where Home is already on screen. Holds the veil through the - * pull plus a short settle so per-tile hydration lands behind it, then - * lets it wipe off. Overlapping automatic refreshes are rare enough - * (once-daily rebuild, reconnect) that a plain flag beats a counter. + * 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. */ - private fun refreshBehindVeil() { - viewModelScope.launch { - updatingInternal.value = true - try { - refresh().join() - delay(VEIL_SETTLE_MS) - } finally { - updatingInternal.value = false - } - } + suspend fun refreshFromPull() { + veil.request() + withTimeoutOrNull(PULL_HANDOFF_TIMEOUT_MS) { veil.visible.first { it } } } val uiState: StateFlow> = @@ -425,6 +415,100 @@ class HomeViewModel @Inject constructor( else -> UiState.Empty } } + + // ─── Updating veil ─────────────────────────────────────────────── + // Declared after uiState: these initialisers read it, and Kotlin runs + // property initialisers and init blocks in declaration order. + + /** + * What the veil watches to decide Home has stopped moving: the whole + * rendered state, plus how many covers are still loading. + * + * [UiState.Success] wraps a [HomeSections] data class, so any visible + * change — a section swapping ids, one tile hydrating from skeleton to + * album — changes this value and re-arms the veil's quiet window. + * + * Unhydrated tiles deliberately do NOT gate `quiescent`. A tile whose + * on-miss fetch soft-fails keeps a null value indefinitely + * ([MetadataProvider] swallows those errors), so treating "no + * skeletons left" as the settle condition would pin the veil to its + * hard ceiling on every refresh. They're covered by the content key + * instead: each tile that lands re-arms the window, and once they stop + * landing the screen is genuinely still. + */ + private val settleSignal: Flow = + combine(uiState, artTracker.inFlight) { state, artInFlight -> + VeilSettleState( + contentKey = state, + hasContent = state is UiState.Success, + quiescent = artInFlight == 0, + ) + } + + private val veil = UpdateVeilController( + scope = viewModelScope, + settleSignal = settleSignal, + shouldVeil = { + // Only worth hiding churn when there's already content to + // hide. A cold load over an empty cache keeps its skeleton — + // veiling that would replace a useful affordance with an + // opaque panel. `hasCachedIndex` is the honest check: uiState + // still reads Loading until the screen subscribes, so on a + // process restore over a warm cache it would say "no content" + // right before the cache emits. + uiState.value is UiState.Success || homeRepository.hasCachedIndex() + }, + 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). + */ + val isUpdating: StateFlow = veil.visible + + init { + // Every refresh path goes through the controller, which decides + // per session whether to raise the veil. That includes the initial + // load: over a warm cache it's a full re-pull that churns every + // section, and it used to run completely unveiled. + veil.request() + // Screen-level auto-recovery (issue #1245): a Home that failed to + // load while the server was unreachable re-pulls itself the moment + // health returns — same idiom as SyncController, one layer up. + // This is also the recovery that keeps trying after the veil has + // given up and lowered; the controller sets no latch against it. + viewModelScope.launch { + networkStatus.recoveries().collect { veil.request() } + } + // Server-side changes that rewrite what Home renders (#968 and + // the 2026-07-31 widening) re-pull behind the veil. Mirrors the + // web SSE consumer. + viewModelScope.launch { + eventsStream.events + .filter { it.kind in VEILED_EVENT_KINDS } + .collect { veil.request() } + } + } + + private companion object { + /** + * Events that change what Home shows. `playlist.system_rebuilt` + * is the 03:00 daily rebuild; the other `playlist.*` kinds move + * the Playlists and Songs-like rows; `scan.run_finished` changes + * Recently added (and Home never reacted to it at all before). + */ + private val VEILED_EVENT_KINDS = setOf( + "playlist.system_rebuilt", + "playlist.created", + "playlist.updated", + "playlist.deleted", + "playlist.tracks_changed", + "scan.run_finished", + ) + } } // ─── Screen ────────────────────────────────────────────────────────── @@ -454,14 +538,20 @@ fun HomeScreen( val offline by viewModel.offline.collectAsStateWithLifecycle() val updating by viewModel.isUpdating.collectAsStateWithLifecycle() PullToRefreshScaffold( - onRefresh = { viewModel.refresh().join() }, + onRefresh = { viewModel.refreshFromPull() }, modifier = Modifier.fillMaxSize().padding(inner), ) { Box(Modifier.fillMaxSize()) { - HomeStateCrossfade(state, systemStatus, offline, navController, viewModel) - // Automatic-refresh veil: the daily rebuild / reconnect - // churn hides behind an "Updating your mixes…" wipe. Manual - // pull owns the PullToRefreshBox spinner instead. + // Every cover below reports its load state to the tracker, + // so the veil can wait for artwork instead of guessing. + CompositionLocalProvider( + LocalArtSettleTracker provides viewModel.artTracker, + ) { + HomeStateCrossfade(state, systemStatus, offline, navController, viewModel) + } + // Refresh veil: rebuild / reconnect / pull / event churn all + // hide behind an "Updating your mixes…" wipe that stays up + // until the screen has actually stopped moving. UpdatingVeil(visible = updating) } } 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 new file mode 100644 index 00000000..83166e54 --- /dev/null +++ b/android/app/src/main/java/com/fabledsword/minstrel/shared/UpdateVeilController.kt @@ -0,0 +1,215 @@ +package com.fabledsword.minstrel.shared + +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.FlowPreview +import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.debounce +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeoutOrNull + +// 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. +private const val DEFAULT_MIN_HOLD_MS = 900L + +// The screen must stop changing for this long before the veil lowers. +// Every content change re-arms it, so a refresh that lands in stages +// (index → tile hydration → artwork) holds the veil across all of them. +private const val DEFAULT_QUIET_MS = 700L + +// Hard ceiling on visibility. A refresh that never settles must not +// strand the user behind an opaque veil; the work itself is NOT capped. +private const val DEFAULT_MAX_HOLD_MS = 12_000L + +// Attempts per session. Retrying behind the veil is the point: a pull +// that fails on the first try gets another go before the user sees +// anything, instead of the veil wiping off over unchanged content. +private const val DEFAULT_ATTEMPTS = 3 +private const val DEFAULT_RETRY_BACKOFF_MS = 600L + +/** Tunables for [UpdateVeilController]; defaults are the Home values. */ +data class VeilTimings( + val minHoldMs: Long = DEFAULT_MIN_HOLD_MS, + val quietMs: Long = DEFAULT_QUIET_MS, + val maxHoldMs: Long = DEFAULT_MAX_HOLD_MS, + val attempts: Int = DEFAULT_ATTEMPTS, + val retryBackoffMs: Long = DEFAULT_RETRY_BACKOFF_MS, +) + +/** + * A snapshot of everything that visibly moves on the veiled screen. + * + * @param contentKey any value whose equality tracks what's rendered — a + * change means the screen moved, and re-arms the quiet timer. + * @param hasContent true when real content (not a skeleton or an empty + * state) is on screen. The veil waits for this before raising: there's + * nothing to hide until there's something to hide. + * @param quiescent false while something is still landing (artwork + * loading, tiles hydrating). The veil will not lower until this is + * true, up to [VeilTimings.maxHoldMs]. + */ +data class VeilSettleState( + val contentKey: Any?, + val hasContent: Boolean, + val quiescent: Boolean, +) + +/** + * Drives an "updating" overlay from *observed content 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 + * pull is nowhere near the end of the visible work — the pull writes id + * lists, then tiles hydrate one by one, then artwork loads. And a plain + * `isUpdating` Boolean set in a `finally` gets cleared by whichever of + * 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. + * + * 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. + * + * @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. + */ +class UpdateVeilController( + private val scope: CoroutineScope, + private val settleSignal: Flow, + private val shouldVeil: suspend () -> Boolean, + private val timings: VeilTimings = VeilTimings(), + private val work: suspend () -> Boolean, +) { + private val visibleInternal = MutableStateFlow(false) + + /** True while the veil should be drawn over the screen. */ + val visible: StateFlow = visibleInternal.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) + + init { + // One consumer, so sessions are serialised by construction: two + // triggers can never each own a piece of the veil's state. + scope.launch { + while (true) { + requests.receive() + runSession() + } + } + } + + /** + * 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. + */ + fun request() { + requests.trySend(Unit) + } + + private suspend fun runSession() { + if (!shouldVeil()) { + drainWork() + 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) + } + // 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 { + raised.await() + delay(timings.minHoldMs) + } + val ceiling = scope.launch { + raised.await() + delay(timings.maxHoldMs) + visibleInternal.value = false + } + try { + 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. + withTimeoutOrNull(timings.maxHoldMs) { awaitSettled() } + } finally { + raiser.cancel() + ceiling.cancel() + if (raised.isCompleted) { + // NonCancellable so the floor is honoured (and the veil + // always cleared) even while the scope is torn down; the + // floor job dies with the scope, so this cannot hang. + withContext(NonCancellable) { floor.join() } + } + floor.cancel() + visibleInternal.value = false + } + } + + private suspend fun drainWork() { + do { + runWorkWithRetries() + // A trigger that arrived mid-session gets folded into this one. + } while (requests.tryReceive().isSuccess) + } + + private suspend fun runWorkWithRetries() { + repeat(timings.attempts) { attempt -> + if (work()) return + if (attempt < timings.attempts - 1) { + delay(timings.retryBackoffMs * (attempt + 1)) + } + } + } + + /** + * Suspends until the screen has been unchanged for + * [VeilTimings.quietMs] and reports itself quiescent. + * + * `debounce` is what makes this hold across a staged update: every + * change restarts the window, so the veil lowers only once emissions + * actually stop. `first { quiescent }` then rejects a quiet-but- + * still-loading moment and waits for the next lull. + */ + @OptIn(FlowPreview::class) + private suspend fun awaitSettled() { + settleSignal + .distinctUntilChanged() + .debounce(timings.quietMs) + .first { it.quiescent } + } +} diff --git a/android/app/src/main/java/com/fabledsword/minstrel/shared/widgets/ArtSettleTracker.kt b/android/app/src/main/java/com/fabledsword/minstrel/shared/widgets/ArtSettleTracker.kt new file mode 100644 index 00000000..0e9a5316 --- /dev/null +++ b/android/app/src/main/java/com/fabledsword/minstrel/shared/widgets/ArtSettleTracker.kt @@ -0,0 +1,60 @@ +package com.fabledsword.minstrel.shared.widgets + +import androidx.compose.runtime.Stable +import androidx.compose.runtime.staticCompositionLocalOf +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.update + +/** + * Counts the cover-art loads that are currently in flight, so a + * screen-level overlay can wait for the artwork to actually land instead + * of guessing with a fixed delay. + * + * Artwork is the most visible pop-in on Home: a tile can be fully + * hydrated (title, artist, counts all present) and still snap its cover + * in a second later, which is exactly the churn the "Updating your + * mixes…" veil exists to hide. The refresh coroutine can't see that — + * it finishes long before Coil does — so the composition reports it + * upward here instead. + * + * [ServerImage] reports into whatever tracker it finds in + * [LocalArtSettleTracker], which means every art surface in the app + * participates for free. Only *composed* images are counted, so a + * LazyRow's off-screen tiles are correctly ignored — the count tracks + * the pop-in a user can actually see. + * + * Provide one per screen that needs it (typically owned by the + * screen's ViewModel so its refresh logic can read [inFlight]): + * + * CompositionLocalProvider(LocalArtSettleTracker provides vm.artTracker) { ... } + */ +@Stable +class ArtSettleTracker { + private val inFlightInternal = MutableStateFlow(0) + + /** + * How many on-screen images are still loading. Zero means the + * artwork has settled — every composed cover has either drawn or + * failed to a fallback. + */ + val inFlight: StateFlow = inFlightInternal.asStateFlow() + + fun begin() { + inFlightInternal.update { it + 1 } + } + + fun end() { + // Floor at zero: a decrement can outlive its increment when a + // tile is disposed mid-load and the count must not go negative + // and wedge "settled" off forever. + inFlightInternal.update { (it - 1).coerceAtLeast(0) } + } +} + +/** + * The tracker [ServerImage] reports load state to, or null on screens + * that don't care (the default) — reporting is then a no-op. + */ +val LocalArtSettleTracker = staticCompositionLocalOf { null } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/shared/widgets/ServerImage.kt b/android/app/src/main/java/com/fabledsword/minstrel/shared/widgets/ServerImage.kt index 0560b76e..0bd0b25e 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/shared/widgets/ServerImage.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/shared/widgets/ServerImage.kt @@ -1,25 +1,40 @@ package com.fabledsword.minstrel.shared.widgets +import androidx.compose.animation.core.animateFloatAsState +import androidx.compose.animation.core.tween import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.runtime.Composable +import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier +import androidx.compose.ui.draw.alpha import androidx.compose.ui.layout.ContentScale import coil3.compose.AsyncImage import coil3.compose.AsyncImagePainter import com.fabledsword.minstrel.shared.resolveServerUrl +// The fallback fades out as the artwork crossfades in (Coil's crossfade is +// configured globally on the ImageLoader in MinstrelApplication). Matching +// durations makes the swap read as one cross-dissolve; without the fade the +// placeholder icon vanished a frame before the cover appeared, which is the +// "art popping in" the Home veil exists to hide (issue #2327). +private const val FALLBACK_FADE_MS = 220 + /** * Renders a server-hosted image, resolving relative URLs centrally so * every cover surface loads consistently. Shows [fallback] when the URL * is blank/unresolvable, while the image is still loading, and when the * load fails — so a tile is never left blank (e.g. art not yet backfilled, * which the "You might like" row hits often). + * + * In-flight loads are reported to [LocalArtSettleTracker] when a screen + * provides one, so a screen-level overlay can wait for artwork to land + * instead of guessing with a fixed delay. */ @Composable fun ServerImage( @@ -40,6 +55,23 @@ fun ServerImage( var state by remember(resolved) { mutableStateOf(AsyncImagePainter.State.Empty) } + // Empty counts as loading: it's the pre-request state, so treating it + // as settled would let a screen overlay lower before Coil even starts. + val loading = state is AsyncImagePainter.State.Empty || + state is AsyncImagePainter.State.Loading + val tracker = LocalArtSettleTracker.current + DisposableEffect(tracker, loading) { + if (loading) tracker?.begin() + // Balanced by construction: the effect re-runs when `loading` flips + // (decrement, then no re-increment) and disposes when a tile leaves + // the composition mid-load (scrolled away). + onDispose { if (loading) tracker?.end() } + } + val fallbackAlpha by animateFloatAsState( + targetValue = if (loading || state is AsyncImagePainter.State.Error) 1f else 0f, + animationSpec = tween(FALLBACK_FADE_MS), + label = "art-fallback", + ) Box(modifier = modifier, contentAlignment = Alignment.Center) { AsyncImage( model = resolved, @@ -48,10 +80,10 @@ fun ServerImage( contentScale = contentScale, onState = { state = it }, ) - if (state is AsyncImagePainter.State.Loading || - state is AsyncImagePainter.State.Error - ) { - fallback() + if (fallbackAlpha > 0f) { + Box(Modifier.alpha(fallbackAlpha), contentAlignment = Alignment.Center) { + fallback() + } } } } 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 new file mode 100644 index 00000000..e509b53d --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/shared/UpdateVeilControllerTest.kt @@ -0,0 +1,242 @@ +package com.fabledsword.minstrel.shared + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.map +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.junit.jupiter.api.Test +import kotlin.test.assertEquals +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 +private const val QUIET_WINDOWS_TO_OUTLAST = 3 + +/** + * 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 controller is built on `backgroundScope` throughout: its consumer + * loop runs forever, so hanging it off the test's own scope would stop + * `runTest` from ever completing. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class UpdateVeilControllerTest { + + private val timings = VeilTimings( + minHoldMs = 900, + quietMs = 700, + maxHoldMs = 12_000, + attempts = 3, + retryBackoffMs = 600, + ) + + /** Drives the settle signal by hand: content key, presence, art count. */ + private class FakeScreen(hasContent: Boolean = true) { + val state = MutableStateFlow(Triple(0, hasContent, 0)) + + val signal = state.map { (key, hasContent, art) -> + VeilSettleState(contentKey = key, hasContent = hasContent, quiescent = art == 0) + } + + /** Something visibly changed — re-arms the quiet window. */ + fun churn() { + state.value = state.value.copy(first = state.value.first + 1) + } + + fun artLoading(count: Int) { + state.value = state.value.copy(third = count) + } + + fun contentAppears() { + state.value = state.value.copy(second = true) + } + } + + private fun TestScope.controllerOn( + screen: FakeScreen, + shouldVeil: suspend () -> Boolean = { true }, + work: suspend () -> Boolean, + ) = UpdateVeilController( + scope = backgroundScope, + settleSignal = screen.signal, + shouldVeil = shouldVeil, + timings = timings, + work = work, + ) + + /** Records every visibility transition, so a blink can't hide. */ + private fun TestScope.recordVisibility(controller: UpdateVeilController): List { + val seen = mutableListOf() + backgroundScope.launch { controller.visible.collect { seen.add(it) } } + return seen + } + + @Test + fun `veil outlasts content that keeps churning after the pull returns`() = runTest { + val screen = FakeScreen() + val controller = controllerOn(screen) { true } + + controller.request() + runCurrent() + assertTrue(controller.visible.value, "veil is up while the screen is still moving") + + // Tiles hydrating one after another, each inside the quiet window. + // The old implementation had already wiped off after a flat 500ms. + repeat(CHURN_ROUNDS) { + advanceTimeBy(timings.quietMs / 2) + screen.churn() + runCurrent() + assertTrue(controller.visible.value, "veil must hold across staged churn") + } + + advanceUntilIdle() + assertFalse(controller.visible.value, "veil lowers once the screen goes quiet") + } + + @Test + fun `veil waits for artwork to finish loading`() = runTest { + val screen = FakeScreen() + val controller = controllerOn(screen) { true } + + screen.artLoading(ART_IN_FLIGHT) + controller.request() + // 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) + assertTrue(controller.visible.value, "veil must wait on in-flight art") + + screen.artLoading(0) + advanceUntilIdle() + assertFalse(controller.visible.value, "veil lowers once art has landed") + } + + @Test + fun `veil holds through failed attempts and their retries`() = runTest { + val screen = FakeScreen() + var attempts = 0 + val controller = controllerOn(screen) { + attempts++ + attempts >= SUCCEED_ON_ATTEMPT // fail twice, succeed on the third + } + 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") + + advanceUntilIdle() + assertEquals(SUCCEED_ON_ATTEMPT, attempts, "retries until the pull succeeds") + assertEquals(listOf(false, true, false), seen, "raised once, lowered once") + } + + @Test + fun `giving up lowers the veil quietly and does not block later requests`() = runTest { + val screen = FakeScreen() + var attempts = 0 + var succeed = false + val controller = controllerOn(screen) { + attempts++ + succeed + } + + controller.request() + advanceUntilIdle() + assertEquals(timings.attempts, attempts, "exhausts its attempts") + assertFalse(controller.visible.value, "gives up silently") + + // The operator's requirement: giving up must not latch anything off + // — the reconnect-driven recovery still gets to try again later. + succeed = true + controller.request() + advanceUntilIdle() + assertEquals(timings.attempts + 1, attempts, "a later request still runs") + } + + @Test + fun `overlapping requests extend one veil instead of racing it`() = runTest { + val screen = FakeScreen() + var started = 0 + val controller = controllerOn(screen) { + started++ + delay(WORK_MS) + true + } + val seen = recordVisibility(controller) + + // Reconnect and the rebuild event arriving together is what made the + // old Boolean flag clear mid-update: whichever pull finished first + // wiped the veil off while the other was still running. + controller.request() + runCurrent() + controller.request() + advanceTimeBy(WORK_MS + 1) + assertTrue(controller.visible.value, "second trigger extends the same veil") + + advanceUntilIdle() + assertEquals(2, started, "the mid-session trigger still did its pull") + assertEquals(listOf(false, true, false), seen, "one veil session, not two") + } + + @Test + fun `a never-settling screen still releases the veil at the ceiling`() = runTest { + val screen = FakeScreen() + val controller = controllerOn(screen) { true } + + screen.artLoading(1) // an image that never completes + controller.request() + advanceTimeBy(timings.maxHoldMs + 1) + assertFalse(controller.visible.value, "the hard ceiling must never strand the user") + } + + @Test + fun `a cold load runs unveiled`() = runTest { + val screen = FakeScreen() + var ran = false + val controller = controllerOn( + screen, + shouldVeil = { false }, // empty cache: the skeleton owns this + ) { + ran = true + true + } + + controller.request() + advanceUntilIdle() + 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") + + advanceUntilIdle() + assertFalse(controller.visible.value) + } +} -- 2.54.0 From 4f99b4284436063d02921c5b93095394d4f0996f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 31 Jul 2026 20:39:54 -0400 Subject: [PATCH 2/4] ci(android): print full assertion messages for failing tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI run 3161 reported seven failures as bare "java.lang.AssertionError at UpdateVeilControllerTest.kt:87" — and line 87 is the test's own `fun ... = runTest {` line, not the assertion. Gradle picks the first stack frame belonging to the test class, and assertions inside a `runTest { }` lambda live in a generated suspend-lambda class that gets filtered out, so every failure in a coroutine test collapses to the function declaration. With the HTML report unreachable from CI, that leaves nothing to debug from. testLogging with exceptionFormat = FULL prints the assertion message and the whole stack trace for failures, which is what makes a coroutine-test failure diagnosable at all here. Also drop the NonCancellable floor-join from UpdateVeilController's finally. Honouring the minimum hold while the scope is being torn down is pointless — nothing is left to render the veil — and a finally that suspends is a finally that can resist cancellation. The floor is now awaited in the try instead. Co-Authored-By: Claude Opus 5 (1M context) --- android/app/build.gradle.kts | 15 ++++++++++++++- .../minstrel/shared/UpdateVeilController.kt | 14 ++++++-------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/android/app/build.gradle.kts b/android/app/build.gradle.kts index 6a283978..192ed4be 100644 --- a/android/app/build.gradle.kts +++ b/android/app/build.gradle.kts @@ -210,4 +210,17 @@ dependencies { debugImplementation(libs.compose.ui.test.manifest) } -tasks.withType { useJUnitPlatform() } +tasks.withType { + useJUnitPlatform() + // Print the assertion message + full stack trace for failures. The + // default console output gives only "AssertionError at Foo.kt:12", and + // for a failure inside a `runTest { }` lambda even that line collapses + // to the test function's own line (the assertion frames live in the + // suspend-lambda class, which Gradle filters out) — leaving nothing to + // debug from when the HTML report isn't reachable, as in CI. + testLogging { + events("failed") + exceptionFormat = org.gradle.api.tasks.testing.logging.TestExceptionFormat.FULL + showStackTraces = true + } +} 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 83166e54..2d1fabd8 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 @@ -3,7 +3,6 @@ package com.fabledsword.minstrel.shared import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.FlowPreview -import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow @@ -14,7 +13,6 @@ import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch -import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeoutOrNull // Once raised, the veil stays up at least this long. Without a floor a @@ -166,15 +164,15 @@ class UpdateVeilController( // here before the raiser has been dispatched, tear the session // down, and leave the churn uncovered. withTimeoutOrNull(timings.maxHoldMs) { awaitSettled() } + // Honour the no-flash minimum before lowering. Deliberately in + // the try and not the finally: on cancellation the scope is + // going away and nothing will render the veil, so the floor is + // pointless there — and a finally that suspends is a finally + // that can resist teardown. + if (raised.isCompleted) floor.join() } finally { raiser.cancel() ceiling.cancel() - if (raised.isCompleted) { - // NonCancellable so the floor is honoured (and the veil - // always cleared) even while the scope is torn down; the - // floor job dies with the scope, so this cannot hang. - withContext(NonCancellable) { floor.join() } - } floor.cancel() visibleInternal.value = false } -- 2.54.0 From d3b40342b4ab0ea041f67d154a08789c9cb880a9 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 31 Jul 2026 20:47:20 -0400 Subject: [PATCH 3/4] test(home): drive the veil tests' clock explicitly, not advanceUntilIdle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All seven new UpdateVeilController tests failed in CI run 3163, and the one test that passed is the tell: it was the only one that never called advanceUntilIdle(). advanceUntilIdle() advances only while *foreground* work remains. Every coroutine this controller owns lives in backgroundScope — it has to, because its consumer loop runs forever and would otherwise stop runTest from completing — so advanceUntilIdle() returned having run nothing at all, and the assertions landed on a session that never started. Hence "exhausts its attempts. Expected <3>, actual <0>" and, where an earlier advanceTimeBy had got a session partway, "retries until the pull succeeds. Expected <3>, actual <2>". Each wait is now an explicit advanceTimeBy sized for what that test still has pending, and the class KDoc says why so nobody folds them back. The drains stay deliberately under maxHoldMs. If a drain overshot the ceiling, "the veil lowered" would stop distinguishing "it settled" from "it gave up" — which is exactly what these tests exist to tell apart. Co-Authored-By: Claude Opus 5 (1M context) --- .../shared/UpdateVeilControllerTest.kt | 32 +++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) 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 e509b53d..e57027f9 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 @@ -7,7 +7,6 @@ import kotlinx.coroutines.flow.map import kotlinx.coroutines.launch import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.advanceTimeBy -import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.Test @@ -22,6 +21,13 @@ private const val ART_IN_FLIGHT = 3 private const val SUCCEED_ON_ATTEMPT = 3 private const val QUIET_WINDOWS_TO_OUTLAST = 3 +// Time to let a session finish once the screen has stopped changing: the +// retry backoffs, the quiet window and the minimum hold all fit inside it, +// while staying well under maxHoldMs. That gap matters — if a drain ran +// past the ceiling, "the veil lowered" would no longer distinguish +// "it settled" from "it gave up", which is the whole point of these tests. +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 @@ -30,6 +36,14 @@ private const val QUIET_WINDOWS_TO_OUTLAST = 3 * The controller is built on `backgroundScope` throughout: its consumer * loop runs forever, so hanging it off the test's own scope would stop * `runTest` from ever completing. + * + * Consequence, and the reason every wait below is an explicit + * `advanceTimeBy`: **`advanceUntilIdle()` is useless here.** It advances + * only while *foreground* work remains, and everything this controller + * does lives in `backgroundScope` — so it returns having run nothing, and + * assertions land on a session that never started (CI run 3163 failed all + * seven of these with "expected 3, actual 0" and friends). Drive the clock + * deliberately instead; don't "simplify" these back to advanceUntilIdle. */ @OptIn(ExperimentalCoroutinesApi::class) class UpdateVeilControllerTest { @@ -101,7 +115,7 @@ class UpdateVeilControllerTest { assertTrue(controller.visible.value, "veil must hold across staged churn") } - advanceUntilIdle() + advanceTimeBy(DRAIN_MS) assertFalse(controller.visible.value, "veil lowers once the screen goes quiet") } @@ -118,7 +132,7 @@ class UpdateVeilControllerTest { assertTrue(controller.visible.value, "veil must wait on in-flight art") screen.artLoading(0) - advanceUntilIdle() + advanceTimeBy(DRAIN_MS) assertFalse(controller.visible.value, "veil lowers once art has landed") } @@ -138,7 +152,7 @@ class UpdateVeilControllerTest { advanceTimeBy(timings.retryBackoffMs + 1) assertTrue(controller.visible.value, "veil stays up across the backoff") - advanceUntilIdle() + advanceTimeBy(DRAIN_MS) assertEquals(SUCCEED_ON_ATTEMPT, attempts, "retries until the pull succeeds") assertEquals(listOf(false, true, false), seen, "raised once, lowered once") } @@ -154,7 +168,7 @@ class UpdateVeilControllerTest { } controller.request() - advanceUntilIdle() + advanceTimeBy(DRAIN_MS) assertEquals(timings.attempts, attempts, "exhausts its attempts") assertFalse(controller.visible.value, "gives up silently") @@ -162,7 +176,7 @@ class UpdateVeilControllerTest { // — the reconnect-driven recovery still gets to try again later. succeed = true controller.request() - advanceUntilIdle() + advanceTimeBy(DRAIN_MS) assertEquals(timings.attempts + 1, attempts, "a later request still runs") } @@ -186,7 +200,7 @@ class UpdateVeilControllerTest { advanceTimeBy(WORK_MS + 1) assertTrue(controller.visible.value, "second trigger extends the same veil") - advanceUntilIdle() + advanceTimeBy(DRAIN_MS) assertEquals(2, started, "the mid-session trigger still did its pull") assertEquals(listOf(false, true, false), seen, "one veil session, not two") } @@ -215,7 +229,7 @@ class UpdateVeilControllerTest { } controller.request() - advanceUntilIdle() + advanceTimeBy(DRAIN_MS) assertTrue(ran, "the refresh still happens") assertFalse(controller.visible.value, "but no veil over a skeleton") } @@ -236,7 +250,7 @@ class UpdateVeilControllerTest { runCurrent() assertTrue(controller.visible.value, "raises the moment cached content paints") - advanceUntilIdle() + advanceTimeBy(SLOW_WORK_MS + DRAIN_MS) assertFalse(controller.visible.value) } } -- 2.54.0 From 3acac985cdb3b3d09260ac06a0b48c93a3f820f6 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 31 Jul 2026 23:16:49 -0400 Subject: [PATCH 4/4] =?UTF-8?q?feat(home):=20veil=20only=20when=20content?= =?UTF-8?q?=20changed;=20tell=20the=20user=20when=20it=20didn't=20?= =?UTF-8?q?=E2=80=94=20#2327?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The veil raised eagerly: any trigger over a warm cache put it up before knowing whether the refresh would change anything. So every launch cost ~1-2s of opaque panel even when the pull returned exactly what was already cached — which, now that the section swap is atomic and the index flow dedups on ids, produces no visible churn to hide at all. The veil was covering nothing and only delaying first paint. The raise is now reactive: it fires when the content key actually differs from what was already on screen, and never for a no-op refresh. The baseline is the first state that HAS content, not the first state at all — over a warm cache the cached rows paint a moment after the session starts, and counting that first paint as "a change" would veil every launch, which is the thing being fixed. Cost of reacting rather than anticipating: the veil arrives one emission after the change, so a single atomic swap shows through. Everything messier that follows it — tile hydration, then artwork — still lands behind it. That leaves a hole this closes too: a manual pull where nothing changed would now produce no veil, no movement, nothing whatsoever, which reads as broken. So sessions report an outcome — CHANGED / UNCHANGED / FAILED — and Home surfaces it as "Already up to date" or "Couldn't check for updates". Only for refreshes a person actually asked for. "Already up to date" on every launch, every 03:00 rebuild and every reconnect would be worse than silence, so VeilSessionResult carries a userInitiated bit and background sessions stay quiet. The bit is tracked separately from the request token because the request channel is CONFLATED: coalescing drops the older token, and a user's pull must not be swallowed by a background trigger arriving on its heels. The surfaced failure is a deliberate narrowing of the earlier "silent on give up" call, which is now read as being about background refreshes: for a pull the user deliberately triggered, silence looks broken, and staying silent while the success case speaks would be incoherent. Recovery is unaffected either way. Pull-to-refresh now waits for whichever successor actually arrives — the veil, or the snackbar — via finishedSessions, instead of only ever waiting on the veil and timing out for 2s on an unchanged pull. The Error-state Retry goes through the controller as well, so it gets the retries and reports its outcome; over an empty cache there's no content to protect, so no veil appears. HomeViewModel.refresh() is gone, replaced by retry() and refreshFromPull() — the two things that actually exist. Tests: two changed meaning and are rewritten rather than patched. A failed pull writes nothing, so the veil no longer stands over the retries — it goes up when a retry finally lands. And "waits for content to paint" became "cached content painting is not mistaken for a change", which is the baseline subtlety above. Added coverage for UNCHANGED, FAILED, the cold-load CHANGED case, and the conflation of a user request with a background one. Co-Authored-By: Claude Opus 5 (1M context) --- .../minstrel/home/ui/HomeScreen.kt | 103 +++++++---- .../minstrel/shared/UpdateVeilController.kt | 170 ++++++++++++++---- .../shared/UpdateVeilControllerTest.kt | 140 ++++++++++----- 3 files changed, 307 insertions(+), 106 deletions(-) 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) } } -- 2.54.0