fix(qconnect): eliminate first-track hiccup, prev/next bouncing, and shuffle drift

Bundle of related fixes addressing QConnect echo handling and shuffle
authority. All four converge on the same architectural principle:
qbz must not invent state when the cloud is the source of truth.

1. Remove should_force_remote_track_restart and its echo-driven reload

   The function fired whenever the cloud echoed SetState with
   target_position_ms=0 for the same track currently playing. The cloud
   re-emits this routinely on secondary field changes (next_track
   corrections, queue_item_id refresh) — every echo triggered a
   load_remote_track_into_player that interrupted in-progress audio.

   Track-change cases continue to load via align_corebridge_queue_cursor
   plus ensure_remote_track_loaded.

2. Reject echo seeks in apply_renderer_command_to_corebridge

   Authoritative seek-to-zero is only honored when the command targets
   a different track, or when local position is already near zero. If
   command.current_track matches playback_state.track_id and target<=1s
   while local_pos>2s, the seek is treated as echo and skipped. The
   prior 2s threshold was insufficient to absorb cloud round-trip during
   queue changes.

3. Drop has_loaded_audio gate from should_reload_remote_track

   playback_state.track_id is updated by the audio thread only after
   engine.append(source) succeeds. During the 1-3s buffer/decode window
   of an initial load, has_loaded_audio is false even though loading is
   in flight — the prior gate caused ensure_remote_track_loaded to
   redundantly re-trigger load on every echo SetState arriving in that
   window. This was the residual first-track hiccup.

4. Add load-attempt dedup across V2 play and ensure_remote_track_loaded

   New sync_state.last_load_attempt: Option<(track_id, Instant)> with a
   5s window. Two write sites:
     - play_remote_renderer_track_if_active when active_renderer_is_local
       (V2 frontend just dispatched a local play)
     - ensure_remote_track_loaded when it triggers its own load
   Read site: ensure_remote_track_loaded skips if last attempt for the
   same track was within the window. Eliminates redundant loads when an
   echo SetState arrives while a local play is still buffering.

5. Defer engine shuffle activation until cloud sends a valid order

   The cloud broadcasts shuffle toggles in two stages: first a
   QueueUpdated with shuffle_mode=true and shuffle_order=null, then
   ~400ms later a QueueUpdated with the computed shuffle_order. The
   prior code applied set_queue_with_order with shuffle_enabled=true on
   the first event, falling through to set_identity_shuffle_order_internal
   — qbz inventing a sequence (0,1,2,...) that diverges from the order
   the cloud is about to authorize. If the second event was delayed,
   dropped, or raced against the first, qbz drifted from peer clients
   (the long-standing "doble shuffle" symptom).

   Now: effective_shuffle_enabled = shuffle_mode && core_shuffle_order.
   is_some(). When the order is absent, the engine stays unshuffled and
   waits for the second QueueUpdated. sync_corebridge_remote_shuffle_
   projection short-circuits identically. Updated diagnostic log
   surfaces remote_shuffle_mode, shuffle_order_present, and engine_
   shuffle_enabled separately for future triage.

Tests: removed should_force_remote_track_restart test, simplified
should_reload_remote_track test to reflect the track-id-only contract.

cc

src-tauri/src/qconnect_service.rs

@@ -359,8 +359,15 @@ struct QconnectRemoteSyncState {
     /// Session topology — stored from session management events (types 81-87).
     session: QconnectSessionState,
     session_renderer_states: HashMap<i32, QconnectSessionRendererState>,
+    /// Track of the most recent load attempt across paths (V2 play
+    /// handoff and ensure_remote_track_loaded). Used to suppress
+    /// redundant reloads when an echo SetState arrives during the
+    /// in-progress buffer/decode window of a previously triggered load.
+    last_load_attempt: Option<(u64, std::time::Instant)>,
 }
 
+const LOAD_ATTEMPT_DEDUP_WINDOW: std::time::Duration = std::time::Duration::from_secs(5);
+
 #[derive(Debug, Clone, Default)]
 struct QconnectSessionRendererState {
     active: Option<bool>,
@@ -2234,13 +2241,17 @@ impl QconnectServiceState {
         track_id: u64,
         app_handle: &AppHandle,
     ) -> Result<bool, String> {
-        let (app, session) = {
+        let (app, session, sync_state) = {
             let guard = self.inner.lock().await;
             let Some(runtime) = guard.runtime.as_ref() else {
                 return Ok(false);
             };
             let session = runtime.sync_state.lock().await.session.clone();
-            (Arc::clone(&runtime.app), session)
+            (
+                Arc::clone(&runtime.app),
+                session,
+                Arc::clone(&runtime.sync_state),
+            )
         };
 
         let active_renderer_id = session.active_renderer_id;
@@ -2256,6 +2267,14 @@ impl QconnectServiceState {
         };
 
         if let Some(reason) = early_return_reason {
+            // Mark this track as a recent load attempt when the play is
+            // about to happen locally — this prevents the cloud's echo
+            // SetState (arriving ~1-2s later) from re-triggering a
+            // redundant load while the V2 path is still buffering.
+            if reason == "active_renderer_is_local" {
+                let mut state = sync_state.lock().await;
+                state.last_load_attempt = Some((track_id, std::time::Instant::now()));
+            }
             emit_qconnect_diagnostic(
                 app_handle,
                 "qconnect:play_track_handoff",
@@ -3007,10 +3026,6 @@ async fn apply_renderer_command_to_corebridge(
             ..
         } => {
             let resolved_playing_state = renderer_state.playing_state.or(*playing_state);
-            let target_position_secs = renderer_state
-                .current_position_ms
-                .or(*current_position_ms)
-                .map(|position_ms| position_ms / 1000);
             let mut projection_renderer_state = renderer_state.clone();
             if projection_renderer_state.current_track.is_none() {
                 projection_renderer_state.current_track = current_track.clone();
@@ -3019,8 +3034,11 @@ async fn apply_renderer_command_to_corebridge(
                 projection_renderer_state.next_track = next_track.clone();
             }
             let resolved_current_track = projection_renderer_state.current_track.as_ref();
-            let mut reloaded_for_restart = false;
+            let peer_renderer_active = {
-            if let Some(current_track) = resolved_current_track {
+                let state = sync_state.lock().await;
+                is_peer_renderer_active(&state.session)
+            };
+            if let Some(projected_track) = resolved_current_track {
                 let queue_state = {
                     let state = sync_state.lock().await;
                     state.last_remote_queue_state.clone()
@@ -3037,64 +3055,54 @@ async fn apply_renderer_command_to_corebridge(
                     false
                 };
 
-                if !projection_applied {
+                // Track-manipulation operations (cursor align, force-restart,
-                    // Guard: skip realigning the CoreBridge cursor when local
+                // ensure_remote_track_loaded) only run when the COMMAND
-                    // IS the active renderer. In that case this command is
+                // explicitly specifies a current_track. The projection's
-                    // just our own state echoed back by the Qobuz cloud, so
+                // resolved_current_track can be stale: when the cloud sends
-                    // aligning here races with the user's already-advanced
+                // a state-only update (pause/resume) with command.current_track=null,
-                    // local cursor and pushes a duplicate index into history.
+                // the projection falls back to renderer_state.current_track,
-                    // Mirrors the gating used in sync_active_renderer_projection
+                // which is the cloud's last-known view of qbz's playback —
-                    // (line ~1223): align when local is NOT the active renderer.
+                // potentially behind qbz's actual local advance. Using that
-                    // Bug #316.
+                // stale value to align/load causes spurious track switches
-                    let should_align = {
+                // (e.g., pause from iOS made qbz jump back to a previous
-                        let state = sync_state.lock().await;
+                // track). The outer renderer_state-based projection is still
-                        !is_local_renderer_active(&state.session)
+                // used for shuffle sync above and downstream playing_state /
-                    };
+                // seek operations, which remain safe because they don't
-                    if should_align {
+                // change the queue cursor or load tracks.
+                let _ = projected_track; // retained for shuffle projection above
+                if let Some(command_track) = current_track.as_ref() {
+                    if !projection_applied {
                         if let Err(err) =
-                            align_corebridge_queue_cursor(bridge, current_track.track_id).await
+                            align_corebridge_queue_cursor(bridge, command_track.track_id).await
                         {
                             log::warn!(
                                 "[QConnect] Failed to align CoreBridge queue cursor: {err}"
                             );
                         }
-                    } else {
-                        log::debug!(
-                            "[QConnect] Skipping align_corebridge_queue_cursor: local renderer is active (echo from cloud); track_id={}",
-                            current_track.track_id
-                        );
-                    }
-                }
-
-                if matches!(
-                    resolved_playing_state,
-                    Some(PLAYING_STATE_PLAYING | PLAYING_STATE_PAUSED)
-                ) {
-                    if let Some(target_position_secs) = target_position_secs {
-                        let playback_state = bridge.get_playback_state();
-                        if should_force_remote_track_restart(
-                            &playback_state,
-                            current_track.track_id,
-                            target_position_secs,
-                        ) {
-                            log::info!(
-                                "[QConnect] Restarting remote track {} from the beginning (current={}s target={}s)",
-                                current_track.track_id,
-                                playback_state.position,
-                                target_position_secs
-                            );
-                            load_remote_track_into_player(bridge, current_track.track_id).await?;
-                            reloaded_for_restart = true;
-                        }
                     }
 
-                    if !reloaded_for_restart {
+                    if matches!(
+                        resolved_playing_state,
+                        Some(PLAYING_STATE_PLAYING | PLAYING_STATE_PAUSED)
+                    ) {
+                        // Force-restart removed: the cloud routinely re-emits
+                        // SetState with current_position_ms=0 for the same
+                        // track when only secondary fields change (e.g.,
+                        // next_track corrections, queue_item_id refreshes).
+                        // Reloading the stream on every echo caused first-
+                        // track hiccup on album change and "needs several
+                        // taps" on prev/next. Track-change cases are handled
+                        // by align_corebridge_queue_cursor + ensure_remote_
+                        // track_loaded below; legitimate seek-to-start from
+                        // a peer controller can use the seek path with
+                        // target>1s if needed.
                         if let Err(err) =
-                            ensure_remote_track_loaded(bridge, current_track.track_id).await
+                            ensure_remote_track_loaded(bridge, sync_state, command_track.track_id)
+                                .await
                         {
                             log::warn!(
                                 "[QConnect] Failed to load remote track {}: {err}",
-                                current_track.track_id
+                                command_track.track_id
                             );
                         }
                     }
@@ -3120,15 +3128,27 @@ async fn apply_renderer_command_to_corebridge(
             }
 
             if let Some(position_ms) = renderer_state.current_position_ms.or(*current_position_ms) {
-                let current_pos_secs = bridge.get_playback_state().position;
+                let playback_state = bridge.get_playback_state();
+                let current_pos_secs = playback_state.position;
                 let target_secs = position_ms / 1000;
-                if reloaded_for_restart && target_secs <= 1 {
+                // Reject echo seeks: when the command targets the same track
-                    return Ok(());
+                // qbz is already playing AND target<=1s while local is well
-                }
+                // ahead, this is the cloud re-emitting a stale SetState
-                // Only seek if the position differs by more than 2 seconds to
+                // (frequently fires on next_track corrections and queue_
-                // avoid audio hiccups from redundant seeks (e.g. when the server
+                // item_id refreshes). A real peer "go to start" intent
-                // echoes back our own position in a SET_STATE after queue_load).
+                // would target the same track as the local one but the
-                if current_pos_secs.abs_diff(target_secs) > 2 {
+                // round-trip to qbz is already a few seconds, making this
+                // case indistinguishable from echo — favor stability.
+                let is_echo_reset = current_track
+                    .as_ref()
+                    .map(|cmd_track| cmd_track.track_id == playback_state.track_id)
+                    .unwrap_or(false)
+                    && target_secs <= 1
+                    && current_pos_secs > 2;
+                if peer_renderer_active
+                    && !is_echo_reset
+                    && current_pos_secs.abs_diff(target_secs) > 2
+                {
                     log::info!(
                         "[QConnect] SetState seek: current={}s target={}s",
                         current_pos_secs,
@@ -3209,6 +3229,13 @@ async fn sync_corebridge_remote_shuffle_projection(
         renderer_state.next_track.as_ref().map(|item| item.track_id),
     );
 
+    // Same deferral rule as materialize_remote_queue_to_corebridge: do
+    // not invent an identity shuffle when the cloud hasn't yet sent the
+    // authoritative shuffle_order. Wait for the second QueueUpdated.
+    if core_shuffle_order.is_none() {
+        return Ok(false);
+    }
+
     let should_apply = {
         let state = sync_state.lock().await;
         state.last_materialized_start_index != Some(start_index)
@@ -3383,17 +3410,29 @@ async fn materialize_remote_queue_to_corebridge(
         renderer_next_queue_item_id,
         renderer_next_track_id,
     );
+    // The cloud sends two QueueUpdated events during a shuffle toggle:
+    // first with shuffle_mode=true and shuffle_order=null (the flag
+    // broadcasts immediately), then ~400ms later with the computed
+    // shuffle_order. If we mark shuffle_enabled=true on the first event
+    // with an absent order, set_queue_with_order falls into its identity
+    // path (0,1,2,...) — that is qbz inventing a sequence that diverges
+    // from the order the cloud is about to authorize. Defer the engine
+    // shuffle activation until the authoritative order is present.
+    let effective_shuffle_enabled = queue_state.shuffle_mode && core_shuffle_order.is_some();
     log::info!(
-        "[QConnect] materialize_remote_queue: setting queue with {} tracks, start_index={:?}, local_track_id={:?}",
+        "[QConnect] materialize_remote_queue: setting queue with {} tracks, start_index={:?}, local_track_id={:?}, remote_shuffle_mode={}, shuffle_order_present={}, engine_shuffle_enabled={}",
         queue_tracks.len(),
         start_index,
-        current_playback_track_id
+        current_playback_track_id,
+        queue_state.shuffle_mode,
+        core_shuffle_order.is_some(),
+        effective_shuffle_enabled,
     );
     bridge
         .set_queue_with_order(
             queue_tracks,
             start_index,
-            queue_state.shuffle_mode,
+            effective_shuffle_enabled,
             core_shuffle_order.clone(),
         )
         .await;
@@ -3572,21 +3611,16 @@ async fn align_corebridge_queue_cursor(bridge: &CoreBridge, track_id: u64) -> Re
 
 fn should_reload_remote_track(
     playback_state: &qbz_player::PlaybackState,
-    has_loaded_audio: bool,
     track_id: u64,
 ) -> bool {
-    playback_state.track_id != track_id || !has_loaded_audio
+    // Only reload when the track ID actually changed. The previous
-}
+    // !has_loaded_audio gate fired during the buffering window of an
-
+    // initial load (qbz already started fetching but the audio engine
-fn should_force_remote_track_restart(
+    // hasn't reported the track as loaded yet) — when the cloud echo
-    playback_state: &qbz_player::PlaybackState,
+    // SetState arrived for the same track, this caused a redundant
-    track_id: u64,
+    // load_remote_track_into_player that interrupted the in-progress
-    target_position_secs: u64,
+    // load. That was the residual first-track hiccup.
-) -> bool {
+    playback_state.track_id != track_id
-    playback_state.track_id == track_id
-        && track_id != 0
-        && target_position_secs <= 1
-        && playback_state.position > target_position_secs.saturating_add(2)
 }
 
 async fn load_remote_track_into_player(bridge: &CoreBridge, track_id: u64) -> Result<(), String> {
@@ -3618,12 +3652,38 @@ async fn load_remote_track_into_player(bridge: &CoreBridge, track_id: u64) -> Re
     }
 }
 
-async fn ensure_remote_track_loaded(bridge: &CoreBridge, track_id: u64) -> Result<(), String> {
+/// Returns true if a load attempt for `track_id` was registered within the
+/// dedup window. The audio thread updates `playback_state.track_id` only
+/// after `engine.append(source)` succeeds — the buffer/decode window
+/// before that creates a gap during which an echoed SetState would
+/// otherwise re-trigger the same load and interrupt the in-progress one.
+fn is_recent_load_attempt(state: &QconnectRemoteSyncState, track_id: u64) -> bool {
+    match state.last_load_attempt {
+        Some((tid, ts)) => tid == track_id && ts.elapsed() < LOAD_ATTEMPT_DEDUP_WINDOW,
+        None => false,
+    }
+}
+
+async fn ensure_remote_track_loaded(
+    bridge: &CoreBridge,
+    sync_state: &Arc<Mutex<QconnectRemoteSyncState>>,
+    track_id: u64,
+) -> Result<(), String> {
+    {
+        let state = sync_state.lock().await;
+        if is_recent_load_attempt(&state, track_id) {
+            return Ok(());
+        }
+    }
     let playback_state = bridge.get_playback_state();
-    if !should_reload_remote_track(&playback_state, bridge.has_loaded_audio(), track_id) {
+    if !should_reload_remote_track(&playback_state, track_id) {
         return Ok(());
     }
 
+    {
+        let mut state = sync_state.lock().await;
+        state.last_load_attempt = Some((track_id, std::time::Instant::now()));
+    }
     load_remote_track_into_player(bridge, track_id).await
 }
 
@@ -5578,33 +5638,6 @@ mod tests {
         assert_eq!(super::qconnect_repeat_mode_from_loop_mode(99), None);
     }
 
-    #[test]
-    fn forces_remote_restart_for_same_track_reset_to_zero() {
-        let playback_state = qbz_player::PlaybackState {
-            is_playing: false,
-            position: 248,
-            duration: 251,
-            track_id: 72930174,
-            volume: 1.0,
-        };
-
-        assert!(super::should_force_remote_track_restart(
-            &playback_state,
-            72930174,
-            0,
-        ));
-        assert!(!super::should_force_remote_track_restart(
-            &playback_state,
-            72930174,
-            120,
-        ));
-        assert!(!super::should_force_remote_track_restart(
-            &playback_state,
-            72930175,
-            0,
-        ));
-    }
-
     #[test]
     fn resolves_current_and_next_queue_item_ids_from_queue_order() {
         let queue: QConnectQueueState = serde_json::from_value(json!({
@@ -6026,7 +6059,7 @@ mod tests {
     }
 
     #[test]
-    fn reloads_remote_track_when_same_track_id_has_no_loaded_audio() {
+    fn reloads_remote_track_only_when_track_id_changed() {
         let playback_state = qbz_player::PlaybackState {
             is_playing: false,
             position: 0,
@@ -6035,19 +6068,14 @@ mod tests {
             volume: 1.0,
         };
 
-        assert!(super::should_reload_remote_track(
+        // Same track: do not reload, even if buffering still in progress.
-            &playback_state,
-            false,
-            193849747,
-        ));
         assert!(!super::should_reload_remote_track(
             &playback_state,
-            true,
             193849747,
         ));
+        // Different track: reload.
         assert!(super::should_reload_remote_track(
             &playback_state,
-            true,
             126886862,
         ));
     }