Merge pull request 'M9 diagnostics follow-ups: playback relabel, sort, connected fix, per-skip + track-identity' (#105) from dev into main

android/app/src/main/java/com/fabledsword/minstrel/diagnostics/DiagnosticsReporter.kt

@@ -102,6 +102,7 @@ class DiagnosticsReporter @Inject constructor(
             launch { collectServerHealth() }
             launch { collectUpnpDrops() }
             launch { collectPlayerState() }
+            launch { collectTrackChanges() }
             launch { collectRoutes() }
             launch { heartbeatLoop() }
         }
@@ -147,11 +148,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)
@@ -160,15 +163,44 @@ class DiagnosticsReporter @Inject constructor(
             }
     }
 
+    // Emit a snapshot at each track/queue-index change — captures a
+    // skip-induced local↔Sonos desync at the INSTANT it happens, which the
+    // 45s heartbeat misses. Note: uiState is a conflated StateFlow, so a
+    // very rapid skip burst may coalesce intermediate indices (we still get
+    // the boundaries + the local-vs-Sonos snapshot).
+    private suspend fun collectTrackChanges() {
+        playerController.uiState
+            .map { it.queueIndex to it.currentTrack?.id }
+            .distinctUntilChanged()
+            .collect { (index, _) ->
+                val ui = playerController.uiState.value
+                val casting = outputPicker.routesState.value.current.protocol !=
+                    OutputRoute.Protocol.SYSTEM
+                record("playback", buildJsonObject {
+                    put("event", "track_change")
+                    put("local_index", index)
+                    // Track IDENTITY, not just index — indices wobble across
+                    // re-casts, so the id + Sonos URI make a desync unambiguous.
+                    put("local_track_id", ui.currentTrack?.id ?: "")
+                    put("local_pos_ms", ui.positionMs)
+                    putSonos(this, casting)
+                    put("upnp_loading", ui.isUpnpLoading)
+                    put("server_health", networkStatus.state.value.name)
+                    put("route", outputPicker.routesState.value.current.name)
+                })
+            }
+    }
+
     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)
             })
         }
     }
@@ -186,13 +218,11 @@ class DiagnosticsReporter @Inject constructor(
                 put("is_playing", ui.isPlaying)
                 put("source", ui.currentSource ?: "")
                 put("local_index", ui.queueIndex)
+                put("local_track_id", ui.currentTrack?.id ?: "")
                 put("local_pos_ms", ui.positionMs)
                 put("route", route.name)
                 put("route_protocol", route.protocol.name)
-                // UPnP/Sonos remote-vs-local — the desync signal.
-                put("sonos_track", remoteState.trackNumber)
-                put("sonos_pos_ms", remoteState.positionMs)
-                put("sonos_playing", remoteState.isPlaying)
+                putSonos(this, routeActive)
                 addPowerFields(this)
             })
         }
@@ -257,6 +287,18 @@ class DiagnosticsReporter @Inject constructor(
         addPowerFields(this)
     }
 
+    // Sonos/UPnP remote-vs-local desync fields. Only meaningful while a
+    // remote route is active; zeroed otherwise so a stale RemotePlayerState
+    // from a just-ended cast can't masquerade as live Sonos data (the
+    // cast→phone handoff artifact). currentTrackUri is the desync ground
+    // truth — it carries the track the speaker is actually streaming.
+    private fun putSonos(builder: kotlinx.serialization.json.JsonObjectBuilder, casting: Boolean) {
+        builder.put("sonos_track", if (casting) remoteState.trackNumber else 0)
+        builder.put("sonos_pos_ms", if (casting) remoteState.positionMs else 0)
+        builder.put("sonos_playing", casting && remoteState.isPlaying)
+        if (casting) builder.put("sonos_uri", remoteState.currentTrackUri)
+    }
+
     private fun addPowerFields(builder: kotlinx.serialization.json.JsonObjectBuilder) {
         val pm = context.getSystemService(Context.POWER_SERVICE) as PowerManager
         builder.put("doze", pm.isDeviceIdleMode)

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"
 }

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,
             )
         }
     }

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":    {},

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'
+    ));

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'
+    ));

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';
@@ -36,13 +45,26 @@
     }
   }
 
+  // Default view = the most recent N events, no time window. The
+  // start/end window is an Advanced affordance.
+  const DEFAULT_LIMIT = 500;
+
   // Filter state.
   let accountId = $state('');
   let clientId = $state('');
   let kind = $state('');
   let fromLocal = $state('');
   let toLocal = $state('');
-  let limit = $state(500);
+  let limit = $state(DEFAULT_LIMIT);
+
+  // True when an explicit time window is set (drives the caption wording).
+  const hasWindow = $derived(Boolean(fromLocal || toLocal));
+
+  function resetWindow() {
+    fromLocal = '';
+    toLocal = '';
+    limit = DEFAULT_LIMIT;
+  }
 
   // datetime-local (browser-local, no tz) → RFC3339 UTC the API accepts.
   function toRfc(v: string): string | undefined {
@@ -69,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);
@@ -191,77 +220,107 @@
     </div>
   </section>
 
-  <!-- Filters -->
-  <section class="flex flex-wrap items-end gap-3">
-    <label class="block text-xs text-text-secondary">
-      Device
-      <select
-        bind:value={clientId}
-        class="mt-1 block w-48 rounded border border-border bg-surface px-2 py-1.5 text-sm text-text-primary"
-      >
-        <option value="">All devices</option>
-        {#each devices as d (d.client_id)}
-          <option value={d.client_id}>{d.client_id.slice(0, 12)} · {d.event_count}</option>
-        {/each}
-      </select>
-    </label>
-    <label class="block text-xs text-text-secondary">
-      Kind
-      <select
-        bind:value={kind}
-        class="mt-1 block w-40 rounded border border-border bg-surface px-2 py-1.5 text-sm text-text-primary"
-      >
-        <option value="">All kinds</option>
-        {#each KINDS as k (k)}
-          <option value={k}>{kindLabel(k)}</option>
-        {/each}
-      </select>
-    </label>
-    <label class="block text-xs text-text-secondary">
-      From
-      <input
-        type="datetime-local"
-        bind:value={fromLocal}
-        class="mt-1 block rounded border border-border bg-surface px-2 py-1.5 text-sm text-text-primary"
-      />
-    </label>
-    <label class="block text-xs text-text-secondary">
-      To
-      <input
-        type="datetime-local"
-        bind:value={toLocal}
-        class="mt-1 block rounded border border-border bg-surface px-2 py-1.5 text-sm text-text-primary"
-      />
-    </label>
-    <label class="block text-xs text-text-secondary">
-      Limit
-      <input
-        type="number"
-        min="1"
-        max="5000"
-        bind:value={limit}
-        class="mt-1 block w-24 rounded border border-border bg-surface px-2 py-1.5 text-sm text-text-primary"
-      />
-    </label>
+  <!-- Filters. Default view = most recent {DEFAULT_LIMIT} events, no time
+       window; the start/end window lives under Advanced. -->
+  <section class="space-y-3">
+    <div class="flex flex-wrap items-end gap-3">
+      <label class="block text-xs text-text-secondary">
+        Device
+        <select
+          bind:value={clientId}
+          class="mt-1 block w-48 rounded border border-border bg-surface px-2 py-1.5 text-sm text-text-primary"
+        >
+          <option value="">All devices</option>
+          {#each devices as d (d.client_id)}
+            <option value={d.client_id}>{d.client_id.slice(0, 12)} · {d.event_count}</option>
+          {/each}
+        </select>
+      </label>
+      <label class="block text-xs text-text-secondary">
+        Kind
+        <select
+          bind:value={kind}
+          class="mt-1 block w-40 rounded border border-border bg-surface px-2 py-1.5 text-sm text-text-primary"
+        >
+          <option value="">All kinds</option>
+          {#each KINDS as k (k)}
+            <option value={k}>{kindLabel(k)}</option>
+          {/each}
+        </select>
+      </label>
+      <label class="block text-xs text-text-secondary">
+        Sort
+        <select
+          bind:value={sortOrder}
+          class="mt-1 block w-36 rounded border border-border bg-surface px-2 py-1.5 text-sm text-text-primary"
+        >
+          <option value="newest">Newest first</option>
+          <option value="oldest">Oldest first</option>
+        </select>
+      </label>
 
-    <div class="ml-auto flex gap-2">
-      <button
-        type="button"
-        onclick={onCopyJson}
-        disabled={rows.length === 0}
-        class="inline-flex items-center gap-1.5 rounded border border-border px-3 py-2 text-sm text-text-primary hover:bg-surface-hover disabled:opacity-50"
-      >
-        <Copy size={15} /> Copy JSON
-      </button>
-      <button
-        type="button"
-        onclick={onDownloadNdjson}
-        disabled={rows.length === 0}
-        class="inline-flex items-center gap-1.5 rounded border border-border px-3 py-2 text-sm text-text-primary hover:bg-surface-hover disabled:opacity-50"
-      >
-        <Download size={15} /> Download
-      </button>
+      <div class="ml-auto flex gap-2">
+        <button
+          type="button"
+          onclick={onCopyJson}
+          disabled={rows.length === 0}
+          class="inline-flex items-center gap-1.5 rounded border border-border px-3 py-2 text-sm text-text-primary hover:bg-surface-hover disabled:opacity-50"
+        >
+          <Copy size={15} /> Copy JSON
+        </button>
+        <button
+          type="button"
+          onclick={onDownloadNdjson}
+          disabled={rows.length === 0}
+          class="inline-flex items-center gap-1.5 rounded border border-border px-3 py-2 text-sm text-text-primary hover:bg-surface-hover disabled:opacity-50"
+        >
+          <Download size={15} /> Download
+        </button>
+      </div>
     </div>
+
+    <!-- Advanced: explicit start/end window + row cap. Collapsed by
+         default so the common case (recent N) needs no interaction. -->
+    <details class="rounded-md border border-border" open={hasWindow}>
+      <summary class="cursor-pointer px-3 py-2 text-xs text-text-secondary">
+        Advanced filters{hasWindow ? ' · window active' : ''}
+      </summary>
+      <div class="flex flex-wrap items-end gap-3 border-t border-border px-3 py-3">
+        <label class="block text-xs text-text-secondary">
+          From
+          <input
+            type="datetime-local"
+            bind:value={fromLocal}
+            class="mt-1 block rounded border border-border bg-surface px-2 py-1.5 text-sm text-text-primary"
+          />
+        </label>
+        <label class="block text-xs text-text-secondary">
+          To
+          <input
+            type="datetime-local"
+            bind:value={toLocal}
+            class="mt-1 block rounded border border-border bg-surface px-2 py-1.5 text-sm text-text-primary"
+          />
+        </label>
+        <label class="block text-xs text-text-secondary">
+          Limit
+          <input
+            type="number"
+            min="1"
+            max="5000"
+            bind:value={limit}
+            class="mt-1 block w-24 rounded border border-border bg-surface px-2 py-1.5 text-sm text-text-primary"
+          />
+        </label>
+        <button
+          type="button"
+          onclick={resetWindow}
+          class="rounded border border-border px-3 py-2 text-sm text-text-secondary hover:bg-surface-hover"
+        >
+          Reset to recent {DEFAULT_LIMIT}
+        </button>
+      </div>
+    </details>
   </section>
 
   <!-- Timeline -->
@@ -276,7 +335,13 @@
     </p>
   {:else}
     <div class="flex items-center justify-between">
-      <span class="text-xs text-text-muted">{rows.length} events (oldest first)</span>
+      <span class="text-xs text-text-muted">
+        {#if hasWindow}
+          {rows.length} events in window ({sortOrder === 'newest' ? 'newest' : 'oldest'} first)
+        {:else}
+          Most recent {rows.length} events ({sortOrder === 'newest' ? 'newest' : 'oldest'} first) · set a time window under Advanced
+        {/if}
+      </span>
     </div>
     <ul class="divide-y divide-border rounded-md border border-border font-mono text-xs">
       {#each rows as r (r.id)}