From 57d01d70adf0898f8efd7da0018760b9e84bce0d Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 27 May 2026 22:03:32 -0400 Subject: [PATCH] =?UTF-8?q?feat(android):=20PlaybackErrorReporter=20?= =?UTF-8?q?=E2=80=94=20surface=20ExoPlayer=20skip-on-error=20(audit=20v3?= =?UTF-8?q?=20=C2=A74.1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit called this the worst kind of bug — when a track fails to load (404, decoder failure, premature EOS, network drop) ExoPlayer just silently skips to the next, hiding exactly the signal that distinguishes "this file is broken" from "the app is flaky". * PlayerController.onPlayerError now emits the failing track's title onto a CONFLATED Channel exposed as playbackErrorEvents: Flow. Conflated rather than buffered so a stuck loop can't pile up; the reporter's debounce coalesces anyway. * PlaybackErrorReporter @Singleton collects the per-error stream, buffers in a 2-second debounce window, and emits coalesced user-facing strings: 1 error → "Couldn't play \"X\" — skipping" N errors → "Skipped N unplayable tracks" Matches the Flutter reporter shape so users see one toast on a network blip that kills N tracks instead of a stack of N toasts. * PlaybackErrorViewModel bridges the reporter's Flow into Hilt's ViewModel layer so ShellScaffold can collectAsState via hiltViewModel(). * ShellScaffold adds a second LaunchedEffect that pipes the reporter messages into the existing shared snackbar — no new UI surface needed. * MinstrelApplication uses the construct-the-singleton trick to start the reporter at app launch so it's collecting from the first Media3 connect. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../minstrel/MinstrelApplication.kt | 11 +++ .../minstrel/player/PlaybackErrorReporter.kt | 68 +++++++++++++++++++ .../minstrel/player/PlayerController.kt | 22 ++++++ .../player/ui/PlaybackErrorViewModel.kt | 21 ++++++ .../minstrel/shared/widgets/ShellScaffold.kt | 7 ++ 5 files changed, 129 insertions(+) create mode 100644 android/app/src/main/java/com/fabledsword/minstrel/player/PlaybackErrorReporter.kt create mode 100644 android/app/src/main/java/com/fabledsword/minstrel/player/ui/PlaybackErrorViewModel.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 b3ebcc51..0b3b79d8 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/MinstrelApplication.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/MinstrelApplication.kt @@ -14,6 +14,7 @@ import com.fabledsword.minstrel.events.LiveEventsDispatcher import com.fabledsword.minstrel.metadata.FreshnessSweeper import com.fabledsword.minstrel.player.CoverPrefetcher import com.fabledsword.minstrel.player.PlayEventsReporter +import com.fabledsword.minstrel.player.PlaybackErrorReporter import com.fabledsword.minstrel.player.ResumeController import com.fabledsword.minstrel.update.data.VersionCheckController import dagger.hilt.android.HiltAndroidApp @@ -73,6 +74,16 @@ class MinstrelApplication : */ @Suppress("unused") @Inject lateinit var playEventsReporter: PlayEventsReporter + /** + * Same construct-the-singleton trick — PlaybackErrorReporter + * subscribes to PlayerController.playbackErrorEvents and emits + * coalesced "Couldn't play X" / "Skipped N tracks" messages on + * its own Flow, which ShellScaffold surfaces in the shared + * snackbar. Without this construct ExoPlayer skip-on-error is + * invisible to the user. + */ + @Suppress("unused") @Inject lateinit var playbackErrorReporter: PlaybackErrorReporter + /** * Same construct-the-singleton trick — CoverPrefetcher's init * block subscribes to PlayerController.uiState and warms Coil's diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/PlaybackErrorReporter.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/PlaybackErrorReporter.kt new file mode 100644 index 00000000..12a6e775 --- /dev/null +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/PlaybackErrorReporter.kt @@ -0,0 +1,68 @@ +package com.fabledsword.minstrel.player + +import com.fabledsword.minstrel.di.ApplicationScope +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.receiveAsFlow +import kotlinx.coroutines.launch +import javax.inject.Inject +import javax.inject.Singleton + +private const val DEBOUNCE_MS = 2_000L + +/** + * Surfaces ExoPlayer track-load failures (404 / decoder failure / + * premature EOS / network drop) as user-visible snackbar messages. + * + * Without this the player just silently skips the dead track — the + * worst kind of bug, because the user can't tell "this file is + * broken" from "the app is flaky". + * + * Pattern mirrors Flutter's `playback_error_reporter.dart`: collect + * per-error events from [PlayerController.playbackErrorEvents], + * debounce in a 2s window, and emit "Couldn't play 'X' — skipping" + * for a single error or "Skipped N unplayable tracks" when a burst + * lands inside the window. Stops the user from seeing a stack of N + * toasts on a network blip that fails N tracks in a row. + * + * ShellScaffold collects [messages] into its existing snackbar host + * so the reporter doesn't need its own UI surface. Constructed at + * app launch via the construct-the-singleton trick in + * [com.fabledsword.minstrel.MinstrelApplication]. + */ +@Singleton +class PlaybackErrorReporter @Inject constructor( + private val playerController: PlayerController, + @ApplicationScope private val scope: CoroutineScope, +) { + private val outChannel = Channel(Channel.BUFFERED) + + /** Coalesced user-facing snackbar text. Subscribers should show in a SnackbarHost. */ + val messages: Flow = outChannel.receiveAsFlow() + + init { + scope.launch { + val buffer = mutableListOf() + var debounceJob: kotlinx.coroutines.Job? = null + playerController.playbackErrorEvents.collect { title -> + buffer.add(title) + debounceJob?.cancel() + debounceJob = scope.launch { + delay(DEBOUNCE_MS) + if (buffer.isEmpty()) return@launch + val n = buffer.size + val first = buffer.first() + buffer.clear() + val msg = if (n == 1) { + "Couldn't play \"$first\" — skipping" + } else { + "Skipped $n unplayable tracks" + } + outChannel.trySend(msg) + } + } + } + } +} diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt index 0c5febe1..2370f92a 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt @@ -12,9 +12,12 @@ import com.fabledsword.minstrel.di.ApplicationScope import com.fabledsword.minstrel.models.TrackRef import dagger.hilt.android.qualifiers.ApplicationContext import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.receiveAsFlow import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine import javax.inject.Inject @@ -59,6 +62,16 @@ class PlayerController @Inject constructor( private val uiStateInternal = MutableStateFlow(PlayerUiState()) val uiState: StateFlow = uiStateInternal.asStateFlow() + /** + * Per-error events for [com.fabledsword.minstrel.player.PlaybackErrorReporter]. + * Emits the failing track's title (or `"Track"` when unknown) each + * time Media3's `onPlayerError` fires — not bound to the lifetime of + * any UI subscriber so a torn-down screen doesn't drop events. + * Conflated channel so backpressure can't stall the player loop. + */ + private val playbackErrorEventsChannel = Channel(Channel.CONFLATED) + val playbackErrorEvents: Flow = playbackErrorEventsChannel.receiveAsFlow() + /** * Stable queue snapshot kept in sync with the player's MediaItems — * the player's own getMediaItem(index) returns Media3 types; we keep @@ -181,6 +194,15 @@ class PlayerController @Inject constructor( mediaController = controller controller.addListener( object : Player.Listener { + override fun onPlayerError(error: androidx.media3.common.PlaybackException) { + val title = queueRefs + .getOrNull(controller.currentMediaItemIndex) + ?.title + ?.takeIf { it.isNotEmpty() } + ?: "Track" + playbackErrorEventsChannel.trySend(title) + } + override fun onEvents(player: Player, events: Player.Events) { val idx = player.currentMediaItemIndex val current = queueRefs.getOrNull(idx) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/ui/PlaybackErrorViewModel.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/ui/PlaybackErrorViewModel.kt new file mode 100644 index 00000000..2c0c479e --- /dev/null +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/ui/PlaybackErrorViewModel.kt @@ -0,0 +1,21 @@ +package com.fabledsword.minstrel.player.ui + +import androidx.lifecycle.ViewModel +import com.fabledsword.minstrel.player.PlaybackErrorReporter +import dagger.hilt.android.lifecycle.HiltViewModel +import kotlinx.coroutines.flow.Flow +import javax.inject.Inject + +/** + * Hilt-injectable wrapper exposing [PlaybackErrorReporter.messages] + * to ShellScaffold. The reporter itself is an app-scoped singleton; + * this VM just bridges its Flow into a `hiltViewModel()`-resolvable + * surface so ShellScaffold can collect it without an EntryPoint + * accessor. + */ +@HiltViewModel +class PlaybackErrorViewModel @Inject constructor( + reporter: PlaybackErrorReporter, +) : ViewModel() { + val messages: Flow = reporter.messages +} diff --git a/android/app/src/main/java/com/fabledsword/minstrel/shared/widgets/ShellScaffold.kt b/android/app/src/main/java/com/fabledsword/minstrel/shared/widgets/ShellScaffold.kt index 86e105f1..7348b6e6 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/shared/widgets/ShellScaffold.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/shared/widgets/ShellScaffold.kt @@ -16,6 +16,7 @@ import com.fabledsword.minstrel.connectivity.ui.ConnectionErrorBanner import com.fabledsword.minstrel.nav.AlbumDetail import com.fabledsword.minstrel.nav.ArtistDetail import com.fabledsword.minstrel.player.ui.MiniPlayer +import com.fabledsword.minstrel.player.ui.PlaybackErrorViewModel import com.fabledsword.minstrel.update.ui.VersionTooOldBanner import com.fabledsword.minstrel.shared.widgets.trackactions.TrackActionsViewModel @@ -45,6 +46,7 @@ fun ShellScaffold( onExpandPlayer: () -> Unit, modifier: Modifier = Modifier, trackActionsViewModel: TrackActionsViewModel = hiltViewModel(), + playbackErrorViewModel: PlaybackErrorViewModel = hiltViewModel(), content: @Composable () -> Unit, ) { val snackbarHostState = remember { SnackbarHostState() } @@ -53,6 +55,11 @@ fun ShellScaffold( snackbarHostState.showSnackbar(msg) } } + LaunchedEffect(Unit) { + playbackErrorViewModel.messages.collect { msg -> + snackbarHostState.showSnackbar(msg) + } + } Column(modifier = modifier.fillMaxSize()) { // Banner slot. VersionTooOldBanner appears when the server's // min_client_version exceeds our build; ConnectionErrorBanner