From 79f2d79a2ed02660f0cd80dc33d5b9aaa58d6e66 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 30 Jun 2026 16:33:01 -0400 Subject: [PATCH] feat(diagnostics): 'playback' kind, newest-first sort, fix active-route subtitle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Relabel (#1204): route + player_state events fire for every output route, not just UPnP — split them into a new 'playback' kind; 'upnp_sync' now means genuinely UPnP/Sonos signal (drops, resync). Migration 0037 adds 'playback' to the kind CHECK; server whitelist, Android reporter labels, and the web kind filter updated. Web sort: the diagnostics list gains a Newest/Oldest-first sort (default newest at top); export follows the displayed order. Fix (#1205): OutputRoute.isConnected was derived from RouteInfo.connectionState, which stays DISCONNECTED for local SYSTEM routes even when active — so a connected Bluetooth device showed "Available" and reported connected:false. The picker subtitle now uses isSelected (route == selected route); the dead isConnected field is removed and the misleading `connected` field dropped from the diagnostics route event (it only ever logs the active route). Refs Scribe M9 (#119), tasks #1204 #1205. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01K55iTxn95BtshocgdE1shW --- .../diagnostics/DiagnosticsReporter.kt | 9 +++-- .../player/output/OutputPickerSheet.kt | 9 +++-- .../minstrel/player/output/OutputRoute.kt | 21 +++++------ internal/api/diagnostics.go | 3 +- .../0037_diagnostics_playback_kind.down.sql | 9 +++++ .../0037_diagnostics_playback_kind.up.sql | 19 ++++++++++ web/src/routes/admin/diagnostics/+page.svelte | 36 ++++++++++++++++--- 7 files changed, 82 insertions(+), 24 deletions(-) create mode 100644 internal/db/migrations/0037_diagnostics_playback_kind.down.sql create mode 100644 internal/db/migrations/0037_diagnostics_playback_kind.up.sql diff --git a/android/app/src/main/java/com/fabledsword/minstrel/diagnostics/DiagnosticsReporter.kt b/android/app/src/main/java/com/fabledsword/minstrel/diagnostics/DiagnosticsReporter.kt index fe827015..d804b348 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/diagnostics/DiagnosticsReporter.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/diagnostics/DiagnosticsReporter.kt @@ -147,11 +147,13 @@ class DiagnosticsReporter @Inject constructor( } private suspend fun collectPlayerState() { + // 'playback', not 'upnp_sync' — player state applies to every output + // route (phone speaker, Bluetooth, UPnP), not just casting. playerController.uiState .map { Triple(it.currentSource, it.isUpnpLoading, it.playbackError) } .distinctUntilChanged() .collect { (source, upnpLoading, err) -> - record("upnp_sync", buildJsonObject { + record("playback", buildJsonObject { put("event", "player_state") put("source", source ?: "") put("upnp_loading", upnpLoading) @@ -161,14 +163,15 @@ class DiagnosticsReporter @Inject constructor( } private suspend fun collectRoutes() { + // 'playback' — route changes happen for all outputs. This only ever + // logs the ACTIVE route (routesState.current), so no "connected" flag. outputPicker.routesState.map { it.current }.distinctUntilChanged().collect { r -> - record("upnp_sync", buildJsonObject { + record("playback", buildJsonObject { put("event", "route") put("id", r.id) put("name", r.name) put("kind", r.kind.name) put("protocol", r.protocol.name) - put("connected", r.isConnected) }) } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerSheet.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerSheet.kt index 4a9039c3..5f31d464 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerSheet.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerSheet.kt @@ -133,7 +133,7 @@ private fun RouteRow( maxLines = 2, overflow = TextOverflow.Ellipsis, ) - val subtitle = route.description ?: defaultSubtitle(route) + val subtitle = route.description ?: defaultSubtitle(route, isSelected) Text( text = subtitle, style = MaterialTheme.typography.bodySmall, @@ -198,10 +198,13 @@ private fun MulticastHintRow() { } } -private fun defaultSubtitle(route: OutputRoute): String = when (route.kind) { +// isSelected (route == the active/selected route) drives the Bluetooth +// "Connected" subtitle. RouteInfo.connectionState can't — it stays +// DISCONNECTED for local SYSTEM routes even when they're in use. +private fun defaultSubtitle(route: OutputRoute, isSelected: Boolean): String = when (route.kind) { OutputRoute.Kind.BuiltIn -> "Phone speaker" OutputRoute.Kind.Wired -> "Wired" - OutputRoute.Kind.Bluetooth -> if (route.isConnected) "Connected" else "Available" + OutputRoute.Kind.Bluetooth -> if (isSelected) "Connected" else "Available" OutputRoute.Kind.Cast -> "Cast" OutputRoute.Kind.Other -> "Available" } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputRoute.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputRoute.kt index 7b7944f2..3baecd30 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputRoute.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputRoute.kt @@ -21,7 +21,6 @@ data class OutputRoute( val description: String?, val kind: Kind, val protocol: Protocol, - val isConnected: Boolean, ) { enum class Kind { BuiltIn, Wired, Bluetooth, Cast, Other } @@ -43,9 +42,11 @@ data class OutputRoute( /** * Lift a MediaRouter [route] into the domain model. Kind is * inferred from [MediaRouter.RouteInfo.getDeviceType]; unknown - * device types fall through to [Kind.Other]. The - * `connectionState` proxy is good enough for the chip's - * "Connected"/"Available" subtitle. + * device types fall through to [Kind.Other]. "Active" is NOT an + * intrinsic of the route — the caller derives it by comparing to + * the selected route (RouteSnapshot.current), because + * RouteInfo.connectionState only reflects remote-route handshakes + * and stays DISCONNECTED for local SYSTEM routes even when in use. */ fun fromRouteInfo(route: MediaRouter.RouteInfo): OutputRoute { val kind = when (route.deviceType) { @@ -58,15 +59,12 @@ data class OutputRoute( MediaRouter.RouteInfo.DEVICE_TYPE_SPEAKER -> Kind.Other else -> Kind.Other } - val connected = - route.connectionState == MediaRouter.RouteInfo.CONNECTION_STATE_CONNECTED return OutputRoute( id = route.id, name = route.name, description = route.description, kind = kind, protocol = Protocol.SYSTEM, - isConnected = connected, ) } @@ -76,10 +74,10 @@ data class OutputRoute( * network speakers into the same `OutputPickerController` * routes stream the system routes come through. * - * `isConnected = false` because UPnP devices have no - * MediaRouter connection-state concept — they're always - * "available" on the LAN, and the picker's selected-route - * rendering handles the "currently playing" indicator. + * UPnP devices have no MediaRouter connection-state concept — + * they're always "available" on the LAN, and the picker's + * selected-route rendering handles the "currently playing" + * indicator. * * Subtitle is `manufacturer modelName` joined by a single * space, falling back to "Network speaker" when both fields @@ -96,7 +94,6 @@ data class OutputRoute( description = description, kind = Kind.Other, protocol = Protocol.UPNP, - isConnected = false, ) } } diff --git a/internal/api/diagnostics.go b/internal/api/diagnostics.go index 23dbfe92..83ec7cb2 100644 --- a/internal/api/diagnostics.go +++ b/internal/api/diagnostics.go @@ -21,7 +21,8 @@ import ( // variants do NOT require a new category here. var validDiagnosticKinds = map[string]struct{}{ "connectivity": {}, - "upnp_sync": {}, + "upnp_sync": {}, // genuinely UPnP/Sonos-specific (drops, resync) + "playback": {}, // route + player-state, any output (phone/BT/UPnP) "power": {}, "lifecycle": {}, "heartbeat": {}, diff --git a/internal/db/migrations/0037_diagnostics_playback_kind.down.sql b/internal/db/migrations/0037_diagnostics_playback_kind.down.sql new file mode 100644 index 00000000..10875ec9 --- /dev/null +++ b/internal/db/migrations/0037_diagnostics_playback_kind.down.sql @@ -0,0 +1,9 @@ +-- Drop rows using the new value first so they don't violate the restored +-- (narrower) constraint, then revert the whitelist. +DELETE FROM diagnostic_events WHERE kind = 'playback'; +ALTER TABLE diagnostic_events DROP CONSTRAINT diagnostic_events_kind_check; +ALTER TABLE diagnostic_events ADD CONSTRAINT diagnostic_events_kind_check + CHECK (kind IN ( + 'connectivity', 'upnp_sync', 'power', 'lifecycle', + 'heartbeat', 'http' + )); diff --git a/internal/db/migrations/0037_diagnostics_playback_kind.up.sql b/internal/db/migrations/0037_diagnostics_playback_kind.up.sql new file mode 100644 index 00000000..722d1dd6 --- /dev/null +++ b/internal/db/migrations/0037_diagnostics_playback_kind.up.sql @@ -0,0 +1,19 @@ +-- Add 'playback' to the diagnostic_events kind whitelist (M9 follow-up). +-- +-- `route` and `player_state` events fire for EVERY output route (phone +-- speaker, Bluetooth, UPnP/Sonos), so bucketing them under 'upnp_sync' +-- was misleading — an operator on Bluetooth earbuds saw "upnp_sync" +-- rows. 'upnp_sync' now means genuinely UPnP/Sonos-specific signal +-- (drops, and future resync/desync); general playback + route telemetry +-- moves to the new 'playback' kind. +-- +-- Per the enum-CHECK-whitelist rule, the new value lands by dropping and +-- re-adding the constraint in the same change. Existing rows tagged +-- 'upnp_sync' stay as-is (debug telemetry on a 30-day retention). + +ALTER TABLE diagnostic_events DROP CONSTRAINT diagnostic_events_kind_check; +ALTER TABLE diagnostic_events ADD CONSTRAINT diagnostic_events_kind_check + CHECK (kind IN ( + 'connectivity', 'upnp_sync', 'power', 'lifecycle', + 'heartbeat', 'http', 'playback' + )); diff --git a/web/src/routes/admin/diagnostics/+page.svelte b/web/src/routes/admin/diagnostics/+page.svelte index 12379cb9..7224421b 100644 --- a/web/src/routes/admin/diagnostics/+page.svelte +++ b/web/src/routes/admin/diagnostics/+page.svelte @@ -22,11 +22,20 @@ const client = useQueryClient(); - const KINDS = ['connectivity', 'upnp_sync', 'power', 'lifecycle', 'heartbeat', 'http'] as const; + const KINDS = [ + 'connectivity', + 'playback', + 'upnp_sync', + 'power', + 'lifecycle', + 'heartbeat', + 'http' + ] as const; function kindLabel(k: string): string { switch (k) { case 'connectivity': return 'Connectivity'; + case 'playback': return 'Playback'; case 'upnp_sync': return 'UPnP sync'; case 'power': return 'Power'; case 'lifecycle': return 'Lifecycle'; @@ -82,10 +91,17 @@ const devicesQuery = $derived($devicesStore); const devices = $derived(devicesQuery.data ?? []); + // Display sort. The API returns newest-first; default the view to that + // (most recent at the top), with an Oldest-first option for reading a + // timeline top-to-bottom. Export follows whatever's displayed. + let sortOrder = $state<'newest' | 'oldest'>('newest'); + const diagStore = $derived(createAdminDiagnosticsQuery(filter)); const diagQuery = $derived($diagStore); - // API returns newest-first; reverse to chronological for a readable timeline. - const rows = $derived([...((diagQuery.data ?? []) as AdminDiagnostic[])].reverse()); + const rows = $derived.by(() => { + const data = [...((diagQuery.data ?? []) as AdminDiagnostic[])]; // newest-first + return sortOrder === 'oldest' ? data.reverse() : data; + }); function fmtTime(iso: string): string { const d = new Date(iso); @@ -232,6 +248,16 @@ {/each} +