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/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..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 @@ -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,30 +86,38 @@ 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.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 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 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 +129,22 @@ 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. 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 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 +// 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 ─────────────────────────────────────────────────────────── data class HomeSections( @@ -184,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 @@ -197,38 +215,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 @@ -241,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}") } @@ -278,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)}", ) } @@ -303,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)}", ) } @@ -328,52 +321,66 @@ class HomeViewModel @Inject constructor( suspend fun playPlaylist(playlist: PlaylistRef) { viewModelScope.launch { playPlaylistShuffled(playlist, playlistsRepository, player) { - poolMessages.trySend(it) + snackbarMessages.trySend(it) } }.join() } /** - * 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 and reports the + * outcome, so this must report failure rather than swallow it. */ - 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() } /** - * 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. + * 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. */ - private fun refreshBehindVeil() { - viewModelScope.launch { - updatingInternal.value = true - try { - refresh().join() - delay(VEIL_SETTLE_MS) - } finally { - updatingInternal.value = false - } + 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 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() { + 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 } } } @@ -425,6 +432,124 @@ 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() + }, + onSessionEnd = ::reportRefreshOutcome, + work = ::runRefresh, + ) + + /** + * 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 + + 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 +579,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) } } @@ -499,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 new file mode 100644 index 00000000..0e7161a0 --- /dev/null +++ b/android/app/src/main/java/com/fabledsword/minstrel/shared/UpdateVeilController.kt @@ -0,0 +1,315 @@ +package com.fabledsword.minstrel.shared + +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.FlowPreview +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.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. +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, +) + +/** 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, +} + +/** + * 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 + * 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: 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 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) + + /** 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. + 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. + * + * @param userInitiated true when a person explicitly asked (pull to + * refresh, a Retry button), which is what [VeilSessionResult] carries + * through to [onSessionEnd]. + */ + 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()) { + // 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 + } + val raised = CompletableDeferred() + 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 { + raised.await() + delay(timings.minHoldMs) + } + val ceiling = scope.launch { + raised.await() + delay(timings.maxHoldMs) + visibleInternal.value = false + } + var succeeded = false + try { + 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. 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 + // 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() + floor.cancel() + visibleInternal.value = false + finish( + succeeded = succeeded, + changed = raised.isCompleted, + userInitiated = askedAtStart, + ) + } + } + + /** + * 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 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 true + if (attempt < timings.attempts - 1) { + delay(timings.retryBackoffMs * (attempt + 1)) + } + } + return false + } + + /** + * 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..58c98cf6 --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/shared/UpdateVeilControllerTest.kt @@ -0,0 +1,314 @@ +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.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 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 + +// 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 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 + * `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 { + + 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) + } + + /** Content visibly changed — what the veil exists to cover. */ + 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 }, + 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 an extra raise 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() + // 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. + // 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") + } + + advanceTimeBy(DRAIN_MS) + 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() + 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) + assertTrue(controller.visible.value, "veil must wait on in-flight art") + + screen.artLoading(0) + advanceTimeBy(DRAIN_MS) + assertFalse(controller.visible.value, "veil lowers once art has landed") + } + + @Test + fun `retries quietly, then veils the churn the successful attempt produces`() = runTest { + val screen = FakeScreen() + var attempts = 0 + val controller = controllerOn(screen) { + attempts++ + 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() + // 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, + "the veil went up once, over the churn the retry finally produced", + ) + } + + @Test + 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, onSessionEnd = { results += it }) { + attempts++ + succeed + } + + controller.request(userInitiated = true) + advanceTimeBy(DRAIN_MS) + assertEquals(timings.attempts, attempts, "exhausts its attempts") + 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") + + // Giving up must not latch anything off — the reconnect-driven + // recovery still gets to try again later. + succeed = true + controller.request() + advanceTimeBy(DRAIN_MS) + 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) + screen.churn() + 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") + + 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") + } + + @Test + fun `a never-settling screen still releases the veil at the ceiling`() = runTest { + val screen = FakeScreen() + val controller = controllerOn(screen) { + screen.churn() + 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 `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(userInitiated = true) + advanceTimeBy(DRAIN_MS) + + assertTrue(ran, "the refresh still happens") + assertFalse(controller.visible.value, "but no veil over a skeleton") + // It went from nothing to something — that IS a change. + assertEquals(VeilOutcome.CHANGED, results.single().outcome) + } +}