From cbcde6c3eb4e7cbe8216c0916821d2b80618a71a Mon Sep 17 00:00:00 2001 From: Carme99 Date: Wed, 7 Oct 2026 08:53:19 +0100 Subject: [PATCH 1/4] WIP(#758): slice-2 AppCaches progress checkpoint --- src-tauri/src/state.rs | 110 ++++++++++++++++++++++ src-tauri/src/tray/actions.rs | 43 ++++----- src-tauri/src/tray/cache.rs | 71 +++++++------- src-tauri/src/tray/dedup.rs | 169 ++++++++++++++++++---------------- src-tauri/src/tray/devices.rs | 10 +- src-tauri/src/tray/mod.rs | 112 ++++++++++++++-------- src-tauri/src/tray/snooze.rs | 22 +++-- 7 files changed, 355 insertions(+), 182 deletions(-) diff --git a/src-tauri/src/state.rs b/src-tauri/src/state.rs index a087e108..5f053ce1 100644 --- a/src-tauri/src/state.rs +++ b/src-tauri/src/state.rs @@ -726,6 +726,109 @@ impl TokensLoadGate { } } +/// Issue #758 slice 2: per-`AppState` tray + config caches and mirrors. +/// +/// The tray throttled caches (`devices`/`queue`), the post-action fetch +/// instants, the dedup snapshot, the window/playing/mode mirrors, the delayed +/// refresh coalescing guard, and the config quarantine/conflict flags used to +/// live as module-level statics, which forced test-wide locks and meant two +/// `AppState`s in one process shared one tray. They now live here, owned by +/// `AppState` (see `AppState::caches`): each test constructs its own +/// `AppCaches::new()`, and production paths reach them via `state.caches`. +/// +/// Genuinely process-wide handles stay statics (issue #758 step 3): the tray +/// icon handle (`tray::TRAY`), the tray write serialization lock +/// (`tray::TRAY_WRITE_LOCK`), the locale table (`i18n::CURRENT`), +/// `macos_deeplink::CLAIMED`, and the keychain/HTTP client caches. +/// +/// Lock shapes match the previous statics exactly (`parking_lot` mutexes for +/// the cache slots and dedup snapshot, atomics elsewhere), so the +/// single-session behaviour is unchanged. All fields are private; the +/// `*_slot` / `*_flag` accessors are the only path to the inner data. +pub struct AppCaches { + devices_cache: Mutex)>>, + queue_cache: Mutex>, + last_tray_action: Mutex>, + last_action_fetch: Mutex>, + last_tray_state: Mutex>, + window_visible: AtomicBool, + last_playing_state: AtomicBool, + last_shuffle_state: AtomicBool, + last_repeat_state: AtomicU8, + delayed_refresh_in_flight: AtomicBool, + config_quarantined: AtomicBool, + conflict_event_sent: AtomicBool, +} + +impl AppCaches { + pub(crate) fn new() -> Self { + Self { + devices_cache: Mutex::new(None), + queue_cache: Mutex::new(None), + last_tray_action: Mutex::new(None), + last_action_fetch: Mutex::new(None), + last_tray_state: Mutex::new(None), + window_visible: AtomicBool::new(false), + last_playing_state: AtomicBool::new(false), + last_shuffle_state: AtomicBool::new(false), + last_repeat_state: AtomicU8::new(crate::spotify::RepeatState::Off as u8), + delayed_refresh_in_flight: AtomicBool::new(false), + config_quarantined: AtomicBool::new(false), + conflict_event_sent: AtomicBool::new(false), + } + } + pub(crate) fn devices_slot( + &self, + ) -> &Mutex)>> { + &self.devices_cache + } + pub(crate) fn queue_slot( + &self, + ) -> &Mutex> { + &self.queue_cache + } + pub(crate) fn last_tray_action_slot(&self) -> &Mutex> { + &self.last_tray_action + } + pub(crate) fn last_action_fetch_slot(&self) -> &Mutex> { + &self.last_action_fetch + } + pub(crate) fn last_tray_state_slot( + &self, + ) -> &Mutex> { + &self.last_tray_state + } + pub(crate) fn window_visible_flag(&self) -> &AtomicBool { + &self.window_visible + } + pub(crate) fn playing_flag(&self) -> &AtomicBool { + &self.last_playing_state + } + pub(crate) fn shuffle_flag(&self) -> &AtomicBool { + &self.last_shuffle_state + } + pub(crate) fn repeat_flag(&self) -> &AtomicU8 { + &self.last_repeat_state + } + pub(crate) fn delayed_refresh_flag(&self) -> &AtomicBool { + &self.delayed_refresh_in_flight + } + pub(crate) fn quarantined_flag(&self) -> &AtomicBool { + &self.config_quarantined + } + pub(crate) fn conflict_sent_flag(&self) -> &AtomicBool { + &self.conflict_event_sent + } +} + +impl Default for AppCaches { + /// Required by `clippy::new_without_default`. Equivalent to + /// `AppCaches::new()`. + fn default() -> Self { + Self::new() + } +} + pub struct AppState { pub tokens: Tokens, pub polling: Polling, @@ -790,6 +893,12 @@ pub struct AppState { /// test constructs an isolated session and production session /// boundaries are per-session resets, not global resets. pub session: crate::polling::SessionState, + /// Issue #758 slice 2: per-`AppState` tray + config caches and mirrors + /// (throttled devices/queue caches, post-action fetch instants, dedup + /// snapshot, window/playing/mode mirrors, delayed-refresh guard, + /// quarantine/conflict flags). Owned here so each test constructs + /// isolated caches and two `AppState`s never share one tray. + pub caches: AppCaches, } impl AppState { @@ -817,6 +926,7 @@ impl AppState { secret_conflict: AtomicBool::new(false), last_sync_snapshot: RwLock::new(None), session: crate::polling::SessionState::new(), + caches: AppCaches::new(), } } } diff --git a/src-tauri/src/tray/actions.rs b/src-tauri/src/tray/actions.rs index f3870a2c..c4117c4e 100644 --- a/src-tauri/src/tray/actions.rs +++ b/src-tauri/src/tray/actions.rs @@ -20,13 +20,6 @@ pub fn tray_write_lock() -> &'static parking_lot::Mutex<()> { TRAY_WRITE_LOCK.get_or_init(|| parking_lot::Mutex::new(())) } -/// Coalescing guard for the delayed one-shot refresh kicked after a -/// successful tray player action: rapid next/previous clicks must not pile -/// up unbounded 2 s-sleep threads each firing blocking Spotify+Teams HTTP. -/// First claimant spawns; losers skip (their track change is covered by the -/// in-flight refresh's unconditional GET plus the polling loop). -pub static DELAYED_REFRESH_IN_FLIGHT: std::sync::atomic::AtomicBool = - std::sync::atomic::AtomicBool::new(false); /// Runs a Spotify player action from a tray click using the stored access /// token. On success the tray menu is force-refreshed and the action's @@ -59,10 +52,10 @@ pub fn run_player_action( Ok(()) => { log::info!("[TRAY] {}: success", label); if let Some(playing) = resulting_playing { - note_playing_state(playing); + note_playing_state(&state.caches, playing); } if let Some((shuffle, repeat)) = resulting_modes { - note_playback_modes(shuffle, repeat); + note_playback_modes(&state.caches, shuffle, repeat); } force_tray_refresh_from_app(app); // Immediate Teams catch-up after a successful player action: @@ -70,22 +63,28 @@ pub fn run_player_action( // a skip, then run a one-shot poll (no-op when sync is off). // Coalesced: rapid clicks skip while a delayed refresh is // already pending; its unconditional GET covers their tracks. - if DELAYED_REFRESH_IN_FLIGHT + if state + .caches + .delayed_refresh_flag() .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) .is_ok() { let app_clone = app.clone(); let label_owned = label.to_string(); + let state_for_drop = std::sync::Arc::clone(state.inner()); std::thread::spawn(move || { // RAII: a panic in run_oneshot must not wedge future // refreshes (a manual clear on each return path would). - struct ResetOnDrop; + struct ResetOnDrop(std::sync::Arc); impl Drop for ResetOnDrop { fn drop(&mut self) { - DELAYED_REFRESH_IN_FLIGHT.store(false, Ordering::Release); + self.0 + .caches + .delayed_refresh_flag() + .store(false, Ordering::Release); } } - let _reset = ResetOnDrop; + let _reset = ResetOnDrop(state_for_drop); std::thread::sleep(std::time::Duration::from_secs(2)); let state = app_clone.state::>(); if !state.polling.is_syncing() { @@ -286,14 +285,14 @@ pub fn force_tray_refresh( // on every player action; the action is recorded instead, and the rebuild // re-fetches under `TRAY_POST_ACTION_FETCH_MIN` — which coalesces a burst // into one pair of requests. The nudge below still forces the repaint. - *LAST_TRAY_ACTION.lock() = Some(Instant::now()); + *state.caches.last_tray_action_slot().lock() = Some(Instant::now()); let snooze_key = snooze.as_ref().map(|sn| snooze_dedup_key(&sn.status)); // Nudge the dedup snapshot (not clear it) with the *current* track key so // the rebuild below can't early-return while the re-seed logic stays // inert: a cleared snapshot would look like a genuine track change and // clobber the toggle state the action just recorded. Flipping the sync // bit is enough — the real snapshot is committed by that rebuild. - let (devices_bucket, queue_bucket) = cache_buckets(); + let (devices_bucket, queue_bucket) = cache_buckets(&state.caches); // Issue #869: the nudge snapshot's profile key reads the post-action // value so the rebuild the function triggers sees a fresh dedup key — // the real write happens inside `store_active_profile`. @@ -313,9 +312,11 @@ pub fn force_tray_refresh( devices_bucket, queue_bucket, nudge_profile_key, + state.caches.shuffle_flag().load(Ordering::Acquire), + last_repeat_state(&state.caches), ); nudge.is_syncing = !is_syncing; - *last_tray_state().lock() = Some(nudge); + *state.caches.last_tray_state_slot().lock() = Some(nudge); rebuild(is_syncing, current_track); } @@ -487,20 +488,20 @@ mod tests { let prod = tray_prod_source(); let force = body_of(prod, "fn force_tray_refresh("); assert!( - force.contains("LAST_TRAY_ACTION.lock() = Some(Instant::now())"), + force.contains("last_tray_action_slot().lock() = Some(Instant::now())"), "a player action must be recorded, not enforced by emptying the caches" ); assert!( - !force.contains("DEVICES_CACHE.lock() = None") - && !force.contains("QUEUE_CACHE.lock() = None"), + !force.contains("devices_slot().lock() = None") + && !force.contains("queue_slot().lock() = None"), "the caches must stay: emptying them bypassed the fetch throttle (issue #883)" ); let body = body_of(prod, "fn rebuild_tray_menu("); let mark = body - .find("LAST_ACTION_FETCH.lock() = Some(Instant::now())") + .find("last_action_fetch_slot().lock() = Some(Instant::now())") .expect("a post-action rebuild must mark its fetch in flight"); let devices = body - .find("devices_for_menu(access_token.as_deref(), fetch)") + .find("devices_for_menu(caches, access_token.as_deref(), fetch)") .expect("the rebuild must build the devices submenu from the fetch mode"); assert!( mark < devices, diff --git a/src-tauri/src/tray/cache.rs b/src-tauri/src/tray/cache.rs index 1ce50e61..2de14928 100644 --- a/src-tauri/src/tray/cache.rs +++ b/src-tauri/src/tray/cache.rs @@ -23,9 +23,9 @@ pub fn throttle_bucket(fetched_at: Option, throttle: Duration) -> u64 { /// The throttle buckets of both caches (issue #805), read under short locks — /// no HTTP, and no lock held past the read. -pub fn cache_buckets() -> (u64, u64) { - let devices_at = DEVICES_CACHE.lock().as_ref().map(|(at, _)| *at); - let queue_at = QUEUE_CACHE.lock().as_ref().map(|(at, _)| *at); +pub fn cache_buckets(caches: &crate::state::AppCaches) -> (u64, u64) { + let devices_at = caches.devices_slot().lock().as_ref().map(|(at, _)| *at); + let queue_at = caches.queue_slot().lock().as_ref().map(|(at, _)| *at); ( throttle_bucket(devices_at, TRAY_SPOTIFY_FETCH_THROTTLE), throttle_bucket(queue_at, TRAY_SPOTIFY_FETCH_THROTTLE), @@ -81,12 +81,6 @@ pub const TRAY_SPOTIFY_FETCH_THROTTLE: Duration = Duration::from_secs(60); /// live re-fetch when stale (issue #388). pub type DeviceCacheSlot = Option<(Instant, Vec)>; -pub static DEVICES_CACHE: std::sync::LazyLock> = - std::sync::LazyLock::new(|| parking_lot::Mutex::new(None)); - -pub static QUEUE_CACHE: std::sync::LazyLock< - parking_lot::Mutex>, -> = std::sync::LazyLock::new(|| parking_lot::Mutex::new(None)); /// Shortest gap between the post-action re-fetches of the Devices/Up Next lists /// (issue #883). A player action wants those submenus to mirror what just @@ -95,17 +89,7 @@ pub static QUEUE_CACHE: std::sync::LazyLock< /// matches the click the user just made. pub const TRAY_POST_ACTION_FETCH_MIN: Duration = Duration::from_secs(5); -/// The most recent tray player action that wants the Devices/Up Next lists -/// refreshed (issue #883). `force_tray_refresh` records the action here instead -/// of emptying both caches, which bypassed the fetch throttle for every click. -pub static LAST_TRAY_ACTION: std::sync::LazyLock>> = - std::sync::LazyLock::new(|| parking_lot::Mutex::new(None)); -/// When the most recent post-action re-fetch was issued (issue #883). Recorded -/// before the requests run, so a click landing while they are in flight reuses -/// them instead of paying for a pair of its own. -pub static LAST_ACTION_FETCH: std::sync::LazyLock>> = - std::sync::LazyLock::new(|| parking_lot::Mutex::new(None)); /// Whether a rebuild must re-fetch the Devices/Up Next lists on behalf of a /// player action (issue #883). Pure in its instants, so the coalescing rule — @@ -142,12 +126,13 @@ pub fn action_fetch_due( /// `min_interval` is this fetch's throttle (issue #883): the full window for an /// ordinary rebuild, `Duration::ZERO` for the one a player action asks for. pub fn cached_devices( + caches: &crate::state::AppCaches, access_token: &str, min_interval: Duration, ) -> Vec { // Snapshot under short lock, then drop before deciding staleness. let snapshot = { - let cache = DEVICES_CACHE.lock(); + let cache = caches.devices_slot().lock(); cache.clone() }; let needs_fetch = match &snapshot { @@ -157,11 +142,11 @@ pub fn cached_devices( if !needs_fetch { return snapshot.unwrap().1; } - // Throttled fetch OUTSIDE any lock — never hold DEVICES_CACHE across HTTP. + // Throttled fetch OUTSIDE any lock — never hold the devices slot across HTTP. match crate::spotify::get_devices(access_token) { Ok(devices) => { // Re-acquire only to store the fresh result. - *DEVICES_CACHE.lock() = Some((Instant::now(), devices.clone())); + *caches.devices_slot().lock() = Some((Instant::now(), devices.clone())); devices } Err(e) => { @@ -177,12 +162,13 @@ pub fn cached_devices( /// Same lock discipline as `cached_devices`: snapshot, drop, fetch outside /// lock, re-acquire to store. Benign double-fetch on a race. See issue #217. pub fn cached_queue( + caches: &crate::state::AppCaches, access_token: &str, min_interval: Duration, ) -> Option { // Snapshot under short lock, then drop before deciding staleness. let snapshot = { - let cache = QUEUE_CACHE.lock(); + let cache = caches.queue_slot().lock(); cache.clone() }; let needs_fetch = match &snapshot { @@ -192,11 +178,11 @@ pub fn cached_queue( if !needs_fetch { return snapshot.map(|(_, queue)| queue); } - // Throttled fetch OUTSIDE any lock — never hold QUEUE_CACHE across HTTP. + // Throttled fetch OUTSIDE any lock — never hold the queue slot across HTTP. match crate::spotify::get_queue(access_token) { Ok(queue) => { // Re-acquire only to store. - *QUEUE_CACHE.lock() = Some((Instant::now(), queue.clone())); + *caches.queue_slot().lock() = Some((Instant::now(), queue.clone())); Some(queue) } Err(e) => { @@ -244,13 +230,17 @@ pub fn tray_fetch_mode(snoozed: bool) -> TrayFetch { /// The Devices list for a rebuild, honouring [`TrayFetch`]. A missing access /// token is an empty submenu either way — there is nothing to fetch with. pub fn devices_for_menu( + caches: &crate::state::AppCaches, access_token: Option<&str>, fetch: TrayFetch, ) -> Vec { match (access_token, fetch) { - (Some(token), TrayFetch::Refresh) => cached_devices(token, TRAY_SPOTIFY_FETCH_THROTTLE), - (Some(token), TrayFetch::RefreshNow) => cached_devices(token, Duration::ZERO), - (_, TrayFetch::CacheOnly) => DEVICES_CACHE + (Some(token), TrayFetch::Refresh) => { + cached_devices(caches, token, TRAY_SPOTIFY_FETCH_THROTTLE) + } + (Some(token), TrayFetch::RefreshNow) => cached_devices(caches, token, Duration::ZERO), + (_, TrayFetch::CacheOnly) => caches + .devices_slot() .lock() .clone() .map(|(_, devices)| devices) @@ -262,13 +252,16 @@ pub fn devices_for_menu( /// The Up Next snapshot for a rebuild, honouring [`TrayFetch`]. Same contract /// as [`devices_for_menu`]. pub fn queue_for_menu( + caches: &crate::state::AppCaches, access_token: Option<&str>, fetch: TrayFetch, ) -> Option { match (access_token, fetch) { - (Some(token), TrayFetch::Refresh) => cached_queue(token, TRAY_SPOTIFY_FETCH_THROTTLE), - (Some(token), TrayFetch::RefreshNow) => cached_queue(token, Duration::ZERO), - (_, TrayFetch::CacheOnly) => QUEUE_CACHE.lock().clone().map(|(_, queue)| queue), + (Some(token), TrayFetch::Refresh) => { + cached_queue(caches, token, TRAY_SPOTIFY_FETCH_THROTTLE) + } + (Some(token), TrayFetch::RefreshNow) => cached_queue(caches, token, Duration::ZERO), + (_, TrayFetch::CacheOnly) => caches.queue_slot().lock().clone().map(|(_, queue)| queue), (None, _) => None, } } @@ -340,8 +333,8 @@ mod tests { 10 ); - let _guard = MODE_ATOM_LOCK.lock(); - note_playback_modes(false, RepeatState::Off); + let caches = crate::state::AppCaches::new(); + note_playback_modes(&caches, false, RepeatState::Off); let track = crate::spotify::TrackInfo { title: "Title".to_string(), artist: "Artist".to_string(), @@ -355,7 +348,17 @@ mod tests { actions: None, }; let at = |devices: u64, queue: u64| { - tray_snapshot_for(true, true, Some(&track), None, devices, queue, None) + tray_snapshot_for( + true, + true, + Some(&track), + None, + devices, + queue, + None, + caches.shuffle_flag().load(Ordering::Acquire), + last_repeat_state(&caches), + ) }; // Identical track, window, modes and caches: still a no-op. diff --git a/src-tauri/src/tray/dedup.rs b/src-tauri/src/tray/dedup.rs index cbd71f11..3a75559b 100644 --- a/src-tauri/src/tray/dedup.rs +++ b/src-tauri/src/tray/dedup.rs @@ -87,13 +87,17 @@ pub fn tray_snapshot_for( // dedup reason as the snooze — a switch changes the check mark, so the // key has to include the id or the rebuild early-returns. active_profile_key: Option, + // The two playback modes (issue #691): fed in by the caller from + // `AppCaches` for the same purity reason as the buckets above. + shuffle: bool, + repeat: RepeatState, ) -> TrayStateSnapshot { TrayStateSnapshot { is_syncing, is_window_visible, track_key: current_track.map(|t| format!("{}|{}|{}", t.artist, t.title, t.is_playing)), - shuffle: LAST_SHUFFLE_STATE.load(Ordering::Acquire), - repeat: last_repeat_state(), + shuffle, + repeat, snooze_key, devices_bucket, queue_bucket, @@ -101,36 +105,20 @@ pub fn tray_snapshot_for( } } -pub static LAST_TRAY_STATE: std::sync::OnceLock>> = - std::sync::OnceLock::new(); - -pub fn last_tray_state() -> &'static parking_lot::Mutex> { - LAST_TRAY_STATE.get_or_init(|| parking_lot::Mutex::new(None)) -} - -/// Last known main-window visibility, which is what the dedup key carries -/// (issue #886). -/// -/// The rebuild used to ask the window directly, and `is_visible()` blocks the -/// calling thread on an event-loop reply — a hop paid by every poll, including -/// the ones the dedup key discards a few lines later. [`note_window_visibility`] -/// keeps the mirror honest, and the rebuild re-reads the real window whenever it -/// paints anyway (see the self-heal in `rebuild_tray_menu`). -pub static WINDOW_VISIBLE: std::sync::atomic::AtomicBool = - std::sync::atomic::AtomicBool::new(false); - /// Records the main window's visibility (issue #886). Every path that shows or /// hides the window should report it — the tray's own Show/Hide and Open /// Settings arms do, and the window commands / close-to-tray paths are expected /// to; the poll loop then never has to ask the event loop for it. -pub fn note_window_visibility(visible: bool) { - WINDOW_VISIBLE.store(visible, Ordering::Release); +pub fn note_window_visibility(caches: &crate::state::AppCaches, visible: bool) { + caches + .window_visible_flag() + .store(visible, Ordering::Release); } /// The visibility the dedup key is built from (issue #886) — the mirror, so the /// discarded path performs no event-loop hop. -pub fn window_visible() -> bool { - WINDOW_VISIBLE.load(Ordering::Acquire) +pub fn window_visible(caches: &crate::state::AppCaches) -> bool { + caches.window_visible_flag().load(Ordering::Acquire) } /// The real main-window visibility, queried once per paint (issue #886). Only @@ -154,14 +142,12 @@ pub fn shuffle_toggle_target(current: bool) -> bool { /// poller's `playback-state-changed` event when the playing state changes /// for the same track (issue #689), and by the tray's own successful /// play/pause/transfer actions. Issue #3.0-P3. -pub static LAST_PLAYING_STATE: std::sync::LazyLock = - std::sync::LazyLock::new(|| std::sync::atomic::AtomicBool::new(false)); /// Records the playing state the Play/Pause mark and the status line render. /// Every path that learns the truth writes it here, so the mark never infers /// playback from a candidate that may already be stale. Issue #3.0-P3. -pub fn note_playing_state(is_playing: bool) { - LAST_PLAYING_STATE.store(is_playing, Ordering::Release); +pub fn note_playing_state(caches: &crate::state::AppCaches, is_playing: bool) { + caches.playing_flag().store(is_playing, Ordering::Release); } /// Consumes a `playback-state-changed` payload (issue #689, tray half) via @@ -173,9 +159,9 @@ pub fn note_playing_state(is_playing: bool) { /// already re-stored it, so recording it here keeps the Play/Pause mark and /// the status line truthful without waiting for the next track. An unparsable /// payload keeps the last known state rather than inventing one. -pub fn consume_playback_state_changed(payload: &str) { +pub fn consume_playback_state_changed(caches: &crate::state::AppCaches, payload: &str) { match serde_json::from_str::(payload) { - Ok(state) => note_playing_state(state.is_playing), + Ok(state) => note_playing_state(caches, state.is_playing), Err(e) => log::warn!( "[TRAY] playback-state-changed: unparsable payload, keeping the last playing state: {}", e @@ -190,28 +176,26 @@ pub fn consume_playback_state_changed(payload: &str) { /// module-level atomic rather than a field on the app's frozen `TrackInfo` /// because that type is the ts-rs-exported IPC shape shared with the /// Dashboard and built by exhaustive literals outside this module. -pub static LAST_SHUFFLE_STATE: std::sync::LazyLock = - std::sync::LazyLock::new(|| std::sync::atomic::AtomicBool::new(false)); -/// Last known repeat mode, encoded as [`RepeatState`]'s `u8` discriminant. -/// Same lifecycle as `LAST_SHUFFLE_STATE`. -pub static LAST_REPEAT_STATE: std::sync::LazyLock = - std::sync::LazyLock::new(|| std::sync::atomic::AtomicU8::new(RepeatState::Off as u8)); /// Records the playback modes a poll body reported. Called by the polling /// loop for every observed item (playing or paused) — the poll response is /// the source of truth for both toggles, so no extra Spotify request is /// needed to render them. See issue #582. -pub fn note_playback_modes(shuffle: bool, repeat: RepeatState) { - LAST_SHUFFLE_STATE.store(shuffle, Ordering::Release); - LAST_REPEAT_STATE.store(repeat as u8, Ordering::Release); +pub fn note_playback_modes( + caches: &crate::state::AppCaches, + shuffle: bool, + repeat: RepeatState, +) { + caches.shuffle_flag().store(shuffle, Ordering::Release); + caches.repeat_flag().store(repeat as u8, Ordering::Release); } /// The tray's view of the current repeat mode. An out-of-range byte (only /// possible if the encoder above is changed without this decoder) degrades /// to `Off` rather than panicking in a menu build. -pub fn last_repeat_state() -> RepeatState { - match LAST_REPEAT_STATE.load(Ordering::Acquire) { +pub fn last_repeat_state(caches: &crate::state::AppCaches) -> RepeatState { + match caches.repeat_flag().load(Ordering::Acquire) { 1 => RepeatState::Context, 2 => RepeatState::Track, _ => RepeatState::Off, @@ -266,25 +250,27 @@ mod tests { /// the item showing what the API was just told to adopt. #[test] fn playback_modes_feed_both_toggle_items() { - // The mode atoms are process-global and read by the rebuild path, so - // the tests that drive them must not interleave. - let _guard = MODE_ATOM_LOCK.lock(); - note_playback_modes(true, RepeatState::Track); - assert!(LAST_SHUFFLE_STATE.load(Ordering::Acquire)); - assert_eq!(last_repeat_state(), RepeatState::Track); - assert_eq!(repeat_menu_label(&EN, last_repeat_state()), "Repeat: Track"); + // Per-test caches (issue #758 slice 2): no global lock needed. + let caches = crate::state::AppCaches::new(); + note_playback_modes(&caches, true, RepeatState::Track); + assert!(caches.shuffle_flag().load(Ordering::Acquire)); + assert_eq!(last_repeat_state(&caches), RepeatState::Track); + assert_eq!( + repeat_menu_label(&EN, last_repeat_state(&caches)), + "Repeat: Track" + ); // The click targets: Repeat advances along the documented cycle, // Shuffle flips whatever was last observed. - assert_eq!(last_repeat_state().next(), RepeatState::Off); + assert_eq!(last_repeat_state(&caches).next(), RepeatState::Off); assert!(!shuffle_toggle_target( - LAST_SHUFFLE_STATE.load(Ordering::Acquire) + caches.shuffle_flag().load(Ordering::Acquire) )); // A poll that reports everything off must clear both items. - note_playback_modes(false, RepeatState::Off); - assert!(!LAST_SHUFFLE_STATE.load(Ordering::Acquire)); - assert_eq!(last_repeat_state(), RepeatState::Off); - assert!(!last_repeat_state().is_on()); + note_playback_modes(&caches, false, RepeatState::Off); + assert!(!caches.shuffle_flag().load(Ordering::Acquire)); + assert_eq!(last_repeat_state(&caches), RepeatState::Off); + assert!(!last_repeat_state(&caches).is_on()); } /// Issue #691: the dedup key is built by the same function the rebuild /// path uses, and it carries both playback modes — so a shuffle/repeat @@ -293,7 +279,7 @@ mod tests { /// while an unchanged poll still dedupes to a no-op. #[test] fn mode_change_forces_a_tray_rebuild() { - let _guard = MODE_ATOM_LOCK.lock(); + let caches = crate::state::AppCaches::new(); let track = |is_playing: bool| crate::spotify::TrackInfo { title: "Title".to_string(), artist: "Artist".to_string(), @@ -306,24 +292,34 @@ mod tests { supports_volume: None, actions: None, }; - let key = |sync: bool, visible: bool| { - tray_snapshot_for(sync, visible, Some(&track(true)), None, 0, 0, None) + let key = |caches: &crate::state::AppCaches, sync: bool, visible: bool| { + tray_snapshot_for( + sync, + visible, + Some(&track(true)), + None, + 0, + 0, + None, + caches.shuffle_flag().load(Ordering::Acquire), + last_repeat_state(caches), + ) }; - note_playback_modes(false, RepeatState::Off); - let base = key(true, true); + note_playback_modes(&caches, false, RepeatState::Off); + let base = key(&caches, true, true); // The poll body reports a shuffle the user toggled in the Spotify app. - note_playback_modes(true, RepeatState::Off); - let shuffled = key(true, true); + note_playback_modes(&caches, true, RepeatState::Off); + let shuffled = key(&caches, true, true); assert!( tray_state_changed(Some(&base), &shuffled), "an external shuffle change must repaint the tray (issue #691)" ); // …and a repeat-mode change. - note_playback_modes(true, RepeatState::Context); - let repeated = key(true, true); + note_playback_modes(&caches, true, RepeatState::Context); + let repeated = key(&caches, true, true); assert!( tray_state_changed(Some(&shuffled), &repeated), "an external repeat change must repaint the tray (issue #691)" @@ -331,7 +327,7 @@ mod tests { // An unchanged poll is still a no-op. assert!( - !tray_state_changed(Some(&repeated), &key(true, true)), + !tray_state_changed(Some(&repeated), &key(&caches, true, true)), "an unchanged poll must not rebuild the menu" ); assert!( @@ -341,23 +337,33 @@ mod tests { // The pre-existing parts of the key still repaint. assert!( - tray_state_changed(Some(&repeated), &key(false, true)), + tray_state_changed(Some(&repeated), &key(&caches, false, true)), "a sync toggle must still repaint (issue #71)" ); assert!( - tray_state_changed(Some(&repeated), &key(true, false)), + tray_state_changed(Some(&repeated), &key(&caches, true, false)), "a Show/Hide click must still repaint (issue #71)" ); // A same-track pause lives in the track half of the key (issue #229). - let paused = tray_snapshot_for(true, true, Some(&track(false)), None, 0, 0, None); + let paused = tray_snapshot_for( + true, + true, + Some(&track(false)), + None, + 0, + 0, + None, + caches.shuffle_flag().load(Ordering::Acquire), + last_repeat_state(&caches), + ); assert!( tray_state_changed(Some(&repeated), &paused), "a same-track pause must still repaint the Play/Pause mark (#229)" ); // Leave the shared atoms as a fresh poll would find them. - note_playback_modes(false, RepeatState::Off); + note_playback_modes(&caches, false, RepeatState::Off); } /// Issue #689 (D6, tray half): the poller emits `playback-state-changed` /// when a track's playing state changes without the track itself @@ -380,35 +386,42 @@ mod tests { }; let mark = |playing: bool| sync_status_line(&EN, true, playing, Some(&track)); - note_playing_state(true); + let caches = crate::state::AppCaches::new(); + note_playing_state(&caches, true); assert_eq!( - mark(LAST_PLAYING_STATE.load(Ordering::Acquire)), + mark(caches.playing_flag().load(Ordering::Acquire)), "Syncing — Artist — Track" ); // The exact payload shape the poller emits for a same-track pause. - consume_playback_state_changed(r#"{"is_playing":false,"track_key":"Artist|Title"}"#); + consume_playback_state_changed( + &caches, + r#"{"is_playing":false,"track_key":"Artist|Title"}"#, + ); assert!( - !LAST_PLAYING_STATE.load(Ordering::Acquire), + !caches.playing_flag().load(Ordering::Acquire), "the tray must believe a same-track pause (issue #689)" ); assert_eq!( - mark(LAST_PLAYING_STATE.load(Ordering::Acquire)), + mark(caches.playing_flag().load(Ordering::Acquire)), "Paused — Artist — Track" ); - consume_playback_state_changed(r#"{"is_playing":true,"track_key":"Artist|Title"}"#); - assert!(LAST_PLAYING_STATE.load(Ordering::Acquire)); + consume_playback_state_changed( + &caches, + r#"{"is_playing":true,"track_key":"Artist|Title"}"#, + ); + assert!(caches.playing_flag().load(Ordering::Acquire)); assert_eq!( - mark(LAST_PLAYING_STATE.load(Ordering::Acquire)), + mark(caches.playing_flag().load(Ordering::Acquire)), "Syncing — Artist — Track" ); // A payload the tray cannot parse must keep the last known state // rather than invent one. - consume_playback_state_changed("not json"); + consume_playback_state_changed(&caches, "not json"); assert!( - LAST_PLAYING_STATE.load(Ordering::Acquire), + caches.playing_flag().load(Ordering::Acquire), "an unparsable payload must not clobber the last playing state" ); } diff --git a/src-tauri/src/tray/devices.rs b/src-tauri/src/tray/devices.rs index 3b33350b..d61c7dcf 100644 --- a/src-tauri/src/tray/devices.rs +++ b/src-tauri/src/tray/devices.rs @@ -64,10 +64,12 @@ pub fn selected_for_log(selected: &DeviceMenuSelection) -> String { /// expired access token does not strand a transfer on "unknown device" /// (issue #586). pub fn resolve_device_id(app: &AppHandle, selected: &DeviceMenuSelection) -> Option { + let state = app.state::>(); + let caches = &state.caches; match selected { DeviceMenuSelection::DeviceId(id) => { // Fast path: still in the cached list and transferable. - let cached = DEVICES_CACHE.lock().as_ref().and_then(|(_, devices)| { + let cached = caches.devices_slot().lock().as_ref().and_then(|(_, devices)| { devices .iter() .find(|d| d.id.as_deref() == Some(id.as_str())) @@ -78,7 +80,6 @@ pub fn resolve_device_id(app: &AppHandle, selected: &DeviceMenuSelection) -> Opt } // Slow path: live re-fetch; the device may have appeared after // the submenu was built, or the cache may be stale. - let state = app.state::>(); match crate::commands::playback::player_with_refresh_typed( state.inner(), app, @@ -86,7 +87,7 @@ pub fn resolve_device_id(app: &AppHandle, selected: &DeviceMenuSelection) -> Opt crate::spotify::get_devices, ) { Ok(devices) => { - *DEVICES_CACHE.lock() = Some((Instant::now(), devices.clone())); + *caches.devices_slot().lock() = Some((Instant::now(), devices.clone())); devices .into_iter() .find(|d| d.id.as_deref() == Some(id.as_str())) @@ -98,7 +99,8 @@ pub fn resolve_device_id(app: &AppHandle, selected: &DeviceMenuSelection) -> Opt } } } - DeviceMenuSelection::LegacyIndex(i) => DEVICES_CACHE + DeviceMenuSelection::LegacyIndex(i) => caches + .devices_slot() .lock() .as_ref() .and_then(|(_, devices)| devices.get(*i).cloned()) diff --git a/src-tauri/src/tray/mod.rs b/src-tauri/src/tray/mod.rs index ccf3e43e..d2bcee6f 100644 --- a/src-tauri/src/tray/mod.rs +++ b/src-tauri/src/tray/mod.rs @@ -20,20 +20,17 @@ pub(crate) mod testkit; pub use actions::{ await_sync_toggle, force_tray_refresh, force_tray_refresh_from_app, refresh_tray_for_locale, refresh_tray_from_state, repaint_tray_from_state, run_player_action, tray_write_lock, - DELAYED_REFRESH_IN_FLIGHT, TOGGLE_SETTLE_POLL, TOGGLE_SETTLE_TIMEOUT, + TOGGLE_SETTLE_POLL, TOGGLE_SETTLE_TIMEOUT, }; pub use cache::{ action_fetch_due, cache_buckets, cached_devices, cached_queue, devices_for_menu, paint_fetch_mode, queue_for_menu, tray_fetch_mode, DeviceCacheSlot, TrayFetch, TrayPaint, - DEVICES_CACHE, LAST_ACTION_FETCH, LAST_TRAY_ACTION, QUEUE_CACHE, TRAY_POST_ACTION_FETCH_MIN, - TRAY_SPOTIFY_FETCH_THROTTLE, + TRAY_POST_ACTION_FETCH_MIN, TRAY_SPOTIFY_FETCH_THROTTLE, }; pub use dedup::{ - consume_playback_state_changed, last_repeat_state, last_tray_state, live_window_visible, - note_playback_modes, note_playing_state, note_window_visibility, repeat_menu_label, - shuffle_toggle_target, sync_status_line, tray_snapshot_for, tray_state_changed, window_visible, - TrayStateSnapshot, LAST_PLAYING_STATE, LAST_REPEAT_STATE, LAST_SHUFFLE_STATE, LAST_TRAY_STATE, - WINDOW_VISIBLE, + consume_playback_state_changed, last_repeat_state, live_window_visible, note_playback_modes, + note_playing_state, note_window_visibility, repeat_menu_label, shuffle_toggle_target, + sync_status_line, tray_snapshot_for, tray_state_changed, window_visible, TrayStateSnapshot, }; pub use devices::{ build_devices_submenu_from_devices, build_queue_submenu_from_queue, build_seek_submenu, @@ -273,10 +270,14 @@ pub fn handle_menu_event(app: &AppHandle, id: &str) { let _ = window.hide(); // Issue #886: the dedup key reads the visibility mirror, // so the tray's own window changes must report it. - note_window_visibility(false); + if let Some(state) = app.try_state::>() { + note_window_visibility(&state.caches, false); + } } else { let _ = window.show(); - note_window_visibility(true); + if let Some(state) = app.try_state::>() { + note_window_visibility(&state.caches, true); + } // Issue #483: a minimized window stays minimized // after show() -- unminimize first (mirrors the // single-instance raise in lib.rs and show_window). @@ -331,7 +332,9 @@ pub fn handle_menu_event(app: &AppHandle, id: &str) { let _ = app.emit("navigate", "settings"); if let Some(window) = app.get_webview_window("main") { let _ = window.show(); - note_window_visibility(true); + if let Some(state) = app.try_state::>() { + note_window_visibility(&state.caches, true); + } // Issue #483: mirror the unminimize in the Show arm. let _ = window.unminimize(); let _ = window.set_focus(); @@ -429,8 +432,18 @@ pub fn handle_menu_event(app: &AppHandle, id: &str) { // nothing is recorded and the item keeps showing the truth. let app_handle = app.clone(); std::thread::spawn(move || { - let target = shuffle_toggle_target(LAST_SHUFFLE_STATE.load(Ordering::Acquire)); - let repeat = last_repeat_state(); + let (target, repeat) = + if let Some(state) = app_handle.try_state::>() + { + ( + shuffle_toggle_target( + state.caches.shuffle_flag().load(Ordering::Acquire), + ), + last_repeat_state(&state.caches), + ) + } else { + (shuffle_toggle_target(false), crate::spotify::RepeatState::Off) + }; run_player_action( &app_handle, "shuffle", @@ -447,8 +460,16 @@ pub fn handle_menu_event(app: &AppHandle, id: &str) { // record-on-success discipline as Shuffle above. let app_handle = app.clone(); std::thread::spawn(move || { - let target = last_repeat_state().next(); - let shuffle = LAST_SHUFFLE_STATE.load(Ordering::Acquire); + let (target, shuffle) = + if let Some(state) = app_handle.try_state::>() + { + ( + last_repeat_state(&state.caches).next(), + state.caches.shuffle_flag().load(Ordering::Acquire), + ) + } else { + (crate::spotify::RepeatState::Off.next(), false) + }; run_player_action( &app_handle, "repeat", @@ -806,13 +827,20 @@ pub fn setup_tray(app: &tauri::App) -> Result<(), String> { // Issue #689 (D6): the polling loop emits `playback-state-changed` when // a track's playing state changes without the track itself changing. - // The Play/Pause mark and the status line read `LAST_PLAYING_STATE`, + // The Play/Pause mark and the status line read the playing mirror, // which a rebuild only re-seeds on a new track key — so a same-track // pause would leave the mark claiming "playing" until the next track. // Consuming the event here keeps the tray's belief truthful; the // poller's own re-store then drives the rebuild that paints it. app.listen("playback-state-changed", |event| { - consume_playback_state_changed(event.payload()); + let payload = event.payload().to_string(); + let app = event.app_handle().clone(); + std::thread::spawn(move || { + let Some(state) = app.try_state::>() else { + return; + }; + consume_playback_state_changed(&state.caches, &payload); + }); }); // Immediately set the real menu to reflect actual state (Bug 11 fix). @@ -902,22 +930,23 @@ pub(crate) fn rebuild_tray_menu( // Issue #886: the key reads the visibility mirror, so this path performs no // event-loop hop — `live_window_visible` below re-reads the real window on // the way to a paint, which is where the label is built. - let key_visible = window_visible(); + let state = app.state::>(); + let caches = &state.caches; + let key_visible = window_visible(caches); // 4.7.0 (S9 / issue #677): the snooze is resolved once, up front, because it // feeds three separate decisions below — the dedup key, the fetch mode and // the rendered status line. Issue #886: it comes from a scoped read of the // mounted config, so no whole-`AppConfig` clone is allocated per poll and no // read guard survives into the blocking Spotify HTTP below. `state` is the // same handle the rest of the rebuild uses. - let state = app.state::>(); let snooze = snooze_from_app_state(state.inner()); let snooze_key = snooze.as_ref().map(|sn| snooze_dedup_key(&sn.status)); // Issue #883: a player action asks for fresh Devices/Up Next lists, but only // when it is not already covered by a fetch in flight (`action_fetch_due`), // and never while snoozed (`paint_fetch_mode`). let action_refresh_due = action_fetch_due( - *LAST_TRAY_ACTION.lock(), - *LAST_ACTION_FETCH.lock(), + *caches.last_tray_action_slot().lock(), + *caches.last_action_fetch_slot().lock(), Instant::now(), TRAY_POST_ACTION_FETCH_MIN, ); @@ -925,7 +954,7 @@ pub(crate) fn rebuild_tray_menu( // Issue #805: the two cache buckets are part of the key, so a session where // nothing else moves still repaints — and therefore re-fetches — once per // throttle window instead of keeping whatever the last rebuild rendered. - let (devices_bucket, queue_bucket) = cache_buckets(); + let (devices_bucket, queue_bucket) = cache_buckets(caches); // Issue #869: the active profile id rides the dedup key so a switch — // tray click, CLI, or Settings card — repaints the submenu's check // mark. Cloned out of the short-lived read guard because the snapshot @@ -943,9 +972,11 @@ pub(crate) fn rebuild_tray_menu( devices_bucket, queue_bucket, active_profile_key, + caches.shuffle_flag().load(Ordering::Acquire), + last_repeat_state(caches), ); { - let last = last_tray_state().lock(); + let last = caches.last_tray_state_slot().lock(); if !tray_state_changed(last.as_ref(), &snapshot) { // No-op: menu state hasn't changed. return Ok(()); @@ -954,7 +985,7 @@ pub(crate) fn rebuild_tray_menu( // loop observed a genuinely new track (or a stop): its stored // `is_playing` is only fresh at track-change time. Tray-initiated // actions and the poller's `playback-state-changed` event update - // LAST_PLAYING_STATE themselves, and a forced rebuild keeps the + // the playing mirror themselves, and a forced rebuild keeps the // same track_key, so neither path re-seeds here. let track_changed = match last.as_ref() { Some(prev) => prev.track_key != snapshot.track_key, @@ -965,7 +996,7 @@ pub(crate) fn rebuild_tray_menu( .as_ref() .map(|t| t.is_playing) .unwrap_or(false); - note_playing_state(fresh_playing); + note_playing_state(caches, fresh_playing); } // Do NOT update the snapshot yet. If the rebuild below fails // (e.g., set_menu returns Err), we want the next call with the @@ -978,7 +1009,7 @@ pub(crate) fn rebuild_tray_menu( // therefore cannot leave the Show/Hide label wrong, and the mirror self-heals // for the polls that follow. let is_window_visible = live_window_visible(app); - note_window_visibility(is_window_visible); + note_window_visibility(caches, is_window_visible); // Fetch Spotify data OUTSIDE the tray write lock, and only when the fetch // mode allows it. The throttled caches are snapshotted and fetched without @@ -994,10 +1025,12 @@ pub(crate) fn rebuild_tray_menu( if fetch == TrayFetch::RefreshNow { // Recorded BEFORE the requests run: a click that lands while they are in // flight must reuse them, not start a pair of its own (issue #883). - *LAST_ACTION_FETCH.lock() = Some(Instant::now()); + *caches.last_action_fetch_slot().lock() = Some(Instant::now()); } - let devices: Vec = devices_for_menu(access_token.as_deref(), fetch); - let queue: Option = queue_for_menu(access_token.as_deref(), fetch); + let devices: Vec = + devices_for_menu(caches, access_token.as_deref(), fetch); + let queue: Option = + queue_for_menu(caches, access_token.as_deref(), fetch); // Issue #871: the active device's capability flags gate the playback // submenu — Volume disabled when `actions.setting_volume` is false, // Seek disabled when `actions.seeking` is false, Shuffle disabled when @@ -1107,7 +1140,7 @@ pub(crate) fn rebuild_tray_menu( // (track_id, is_playing) via the track_key dedup and LAST_PLAYING_STATE, // so a same-track pause flips without waiting for the next poll. See // issues #229 and #217. - let is_playing = LAST_PLAYING_STATE.load(Ordering::Acquire); + let is_playing = caches.playing_flag().load(Ordering::Acquire); // Issue #591: a disabled head-of-menu status line stating what the app // is actually doing (syncing / paused / not syncing). Everything below // is derived from state already in scope for this rebuild, so the line @@ -1177,7 +1210,7 @@ pub(crate) fn rebuild_tray_menu( // documents both as sources of truth for "can I do this?"; the // capability flag wins when the device reports it). let shuffle = CheckMenuItemBuilder::with_id(ID_SHUFFLE, s.shuffle) - .checked(LAST_SHUFFLE_STATE.load(Ordering::Acquire)) + .checked(caches.shuffle_flag().load(Ordering::Acquire)) .enabled(!active_is_restricted && active_actions.toggling_shuffle) .build(app) .map_err(|e| { @@ -1187,7 +1220,7 @@ pub(crate) fn rebuild_tray_menu( ); e.to_string() })?; - let repeat_state = last_repeat_state(); + let repeat_state = last_repeat_state(caches); let repeat = CheckMenuItemBuilder::with_id(ID_REPEAT, repeat_menu_label(s, repeat_state)) .checked(repeat_state.is_on()) .enabled( @@ -1419,7 +1452,7 @@ pub(crate) fn rebuild_tray_menu( // Issue #882: the startup paint never commits — it renders the caches // only, so recording it would dedup away the first real rebuild. if paint == TrayPaint::Deduped { - *last_tray_state().lock() = Some(snapshot); + *caches.last_tray_state_slot().lock() = Some(snapshot); } log::info!( @@ -1948,7 +1981,7 @@ mod tests { "setup_tray must subscribe to the poller's playback-state-changed event (issue #689)" ); assert!( - body.contains("consume_playback_state_changed(event.payload())"), + body.contains("consume_playback_state_changed(&state.caches, &payload)"), "the subscription must hand the payload to the tray's consumer" ); } @@ -2118,10 +2151,11 @@ mod tests { #[test] fn discarded_rebuilds_query_neither_the_window_nor_a_config_clone() { // The mirror is the key's source and round-trips. - note_window_visibility(true); - assert!(window_visible(), "the mirror must report what was recorded"); - note_window_visibility(false); - assert!(!window_visible()); + let caches = crate::state::AppCaches::new(); + note_window_visibility(&caches, true); + assert!(window_visible(&caches), "the mirror must report what was recorded"); + note_window_visibility(&caches, false); + assert!(!window_visible(&caches)); let prod = tray_prod_source(); let body = body_of(prod, "fn rebuild_tray_menu("); @@ -2129,7 +2163,7 @@ mod tests { .find("tray_state_changed(") .expect("the rebuild must keep the dedup guard"); let mirror = body - .find("window_visible()") + .find("window_visible(caches)") .expect("the dedup key must read the visibility mirror (issue #886)"); assert!( mirror < guard, diff --git a/src-tauri/src/tray/snooze.rs b/src-tauri/src/tray/snooze.rs index d736067c..d85aedcb 100644 --- a/src-tauri/src/tray/snooze.rs +++ b/src-tauri/src/tray/snooze.rs @@ -559,8 +559,8 @@ mod tests { /// (the countdown would otherwise freeze at the minute the menu was built). #[test] fn snooze_change_forces_a_tray_rebuild() { - let _guard = MODE_ATOM_LOCK.lock(); - note_playback_modes(false, RepeatState::Off); + let caches = crate::state::AppCaches::new(); + note_playback_modes(&caches, false, RepeatState::Off); let track = crate::spotify::TrackInfo { title: "Title".to_string(), artist: "Artist".to_string(), @@ -574,7 +574,17 @@ mod tests { actions: None, }; let at = |snooze: Option| { - tray_snapshot_for(true, true, Some(&track), snooze, 0, 0, None) + tray_snapshot_for( + true, + true, + Some(&track), + snooze, + 0, + 0, + None, + caches.shuffle_flag().load(Ordering::Acquire), + last_repeat_state(&caches), + ) }; let none = at(None); @@ -639,8 +649,8 @@ mod tests { "an ordinary rebuild must take its fetch mode from the snooze (issue #677)" ); for call in [ - "devices_for_menu(access_token.as_deref(), fetch)", - "queue_for_menu(access_token.as_deref(), fetch)", + "devices_for_menu(caches, access_token.as_deref(), fetch)", + "queue_for_menu(caches, access_token.as_deref(), fetch)", ] { assert!( body.contains(call), @@ -649,7 +659,7 @@ mod tests { ); } assert!( - !body.contains("cached_devices(") && !body.contains("cached_queue("), + !body.contains("cached_devices(caches,") && !body.contains("cached_queue(caches,"), "the rebuild must go through the fetch-mode helpers, not the raw fetchers" ); } From 665279ba877748e152951569e6f32db2945412a9 Mon Sep 17 00:00:00 2001 From: Carme99 Date: Thu, 8 Oct 2026 17:39:23 +0100 Subject: [PATCH 2/4] refactor(tray,config): own AppCaches from AppState, delete quarantine-era statics (#758, slice 2) Thread &AppCaches through load_config, quarantine/conflict emitters, tray rebuild/paint helpers, and every window/config/updater/snooze caller; delete CONFIG_QUARANTINED, CONFLICT_EVENT_SENT, tray cache statics and QUARANTINE_TEST_LOCK; fix the Tauri 2.11 event-listener handle capture; add test_two_app_states_do_not_share_caches. LOCALE_TEST_LOCK survives (i18n::CURRENT is an issue-blessed keep). No behaviour change in the single-session path. --- CHANGELOG.md | 1 + docs/STATE-OF-FEATURES.md | 1 + docs/architecture/storage-and-config.md | 3 +- src-tauri/src/app.rs | 14 ++- src-tauri/src/cli.rs | 7 +- src-tauri/src/commands/config.rs | 16 ++-- src-tauri/src/commands/onboarding.rs | 3 +- src-tauri/src/commands/rules.rs | 13 ++- src-tauri/src/commands/window.rs | 2 +- src-tauri/src/config/io.rs | 28 +++--- src-tauri/src/config/migrate.rs | 26 +++--- src-tauri/src/config/mod.rs | 117 +++++++++++++----------- src-tauri/src/diagnostics.rs | 13 ++- src-tauri/src/menu.rs | 4 +- src-tauri/src/polling/iteration.rs | 7 +- src-tauri/src/state.rs | 42 +++++++-- src-tauri/src/tray/actions.rs | 1 - src-tauri/src/tray/cache.rs | 3 - src-tauri/src/tray/dedup.rs | 26 +----- src-tauri/src/tray/devices.rs | 16 ++-- src-tauri/src/tray/mod.rs | 55 ++++++----- src-tauri/src/tray/snooze.rs | 4 +- src-tauri/src/updater_bg.rs | 47 +++++++--- 23 files changed, 260 insertions(+), 189 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b58ace3..7ac2d37c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ section to the released version and opens a fresh empty one (see `docs/RELEASING - **Poll config reads share one immutable snapshot instead of deep-cloning per iteration (#893).** `Config` now holds `RwLock>>` with a `Config::snapshot()` getter that clones the pointer, not the document; `run_inner` and the exit-path cleanup take their config through it and never hold the read guard across the iteration body, so a save racing an in-flight poll completes without blocking and the iteration never observes a partially updated config. Writers publish a new `Arc` under the write guard (`Arc::make_mut` for the in-place keychain-stamp path). The Spotify bundle, write-clock snapshot, and tray dedup clones are untouched. No on-disk format change. - **Config `u64` fields typed as `number`, not `bigint`, in generated types (#765).** `PollingConfig`, `LoggingConfig`, patch types, `ConfigSummary` counters, snooze minutes and stage-progress bytes carry `#[ts(type = "number")]`, matching the `serde_json` wire shape (JS numbers) and the existing `TrackInfo` override idiom. The `BIGINT_SECTIONS` normaliser, `BigInt(...)` defaults and bigint-aware stringify paths are gone; the store round-trips plain numbers. No value change. - **Polling session state moves off module statics into `AppState::session` (#758, PARTIAL — polling half).** `SessionState` owns the write-decision clocks (with the D11 generation guard), the quiet/snooze latches, the last-now-playing cache, the preferred-presence session, the exit snapshot, and the #863 failure/gate mirrors; `poll_once`/`loop`/`state`/`sync`/`diagnostics` thread `&session` instead of touching statics. `global_state_lock` is deleted and each test constructs its own `SessionState::new()` (new `test_two_sessions_do_not_share_clocks_latches_or_caches` proves isolation). Tray/config `AppCaches` (slice 2) stay static for now, so `QUARANTINE_TEST_LOCK` and `LOCALE_TEST_LOCK` survive and the issue stays open. No behaviour change in the single-session path. +- **Tray/config caches move off module statics into `AppState::caches` (#758, PARTIAL — slice 2).** `AppCaches` owns the throttled devices/queue caches, post-action fetch instants, dedup snapshot, window/playing/mode mirrors, delayed-refresh guard, and the config quarantine/conflict flags; `load_config`/`quarantine_corrupt_config`/`emit_spotify_secret_conflict_once`, the tray rebuild/paint helpers, and every window/config/updater/snooze caller thread `&state.caches` instead of touching statics. `CONFIG_QUARANTINED`, `CONFLICT_EVENT_SENT`, and the tray cache statics are deleted and each test constructs its own `AppCaches::new()` (new `test_two_app_states_do_not_share_caches` proves isolation). `QUARANTINE_TEST_LOCK` is deleted; `LOCALE_TEST_LOCK` survives because the locale table (`i18n::CURRENT`) stays process-wide per the issue's keeps, so the issue stays open. No behaviour change in the single-session path. ### Refactor - **Monolithic `lib.rs` split into `app`/`cli`/`deep_link`/`state` modules (#757).** The ~4.9k-line `lib.rs` is now a 34-line module registry + state re-export shim; the Tauri `run()`/setup wiring lives in `app.rs` (with `setup_*` helpers), CLI parsing/dispatch in `cli.rs`, the Spotify-callback exchange in `deep_link.rs`, and `AppState` + token-commit seams in `state.rs`. Six source-scanner guards retargeted to the new homes (`generate_handler!` → `app.rs`, `handle_spotify_callback` → `deep_link.rs`, `forward_launch_to_running_instance` → `app.rs`, `polling::run_oneshot` call site → `cli.rs`, log-permission setup → `setup_log_permissions` helper, `macos_deeplink` module decl → `lib.rs`), and the redaction-literal sweep now covers the four new modules. No behaviour change. - **Config split into 7-slice mod with re-exported surface (#755).** `src-tauri/src/config.rs` (8313 ln) becomes `src-tauri/src/config/{schema,clamp,snooze,patch,migrate,io,transfer}.rs` plus a 52-line re-export header in `mod.rs` that keeps every `crate::config::X` path stable. All 121 config tests remain centralized in `config/mod.rs` (identical set, 565 asserts) — the slices carry no `#[test]`; source-scan guards read the slices through one `concat!(include_str!(…))`, `redact.rs` aggregates the 8 slice sources, `LoggingConfig` lives only in `schema.rs`. `cargo check --all-targets` plus `cargo test --lib` (config 121/121, full 910/910) plus `clippy -D warnings` plus `fmt --check` are all clean. diff --git a/docs/STATE-OF-FEATURES.md b/docs/STATE-OF-FEATURES.md index c452672c..0e4fec59 100644 --- a/docs/STATE-OF-FEATURES.md +++ b/docs/STATE-OF-FEATURES.md @@ -129,6 +129,7 @@ end-to-end; the few rows that can't be sourced inline are explicitly flagged | Polish CLDR few/many plurals (#1154) | ✅ | Both plural keys (`logs.count`, `dashboard.snoozeStatusStart`) carry a `{key}_few` entry in all eight dictionaries — real Polish nominative plurals ("2 wpisy", "2 minuty"; model-written, human review pending per #984) with `_other`-mirroring `_few` in the seven locales whose CLDR never selects `few`. No `_many`: covered-noun `many` ("5 wpisów", "5 minut") is the genitive plural `_other` already carries. `tests/i18n.test.ts` pins the real few/many forms (1/2–4/5/0/12/22/25) plus the snooze trio (1/2/5); key-coverage + placeholder gates cover the `_one`/`_other`/`_few` trio. | | Five more UI locales + follow-system language (#984) | ✅ | Webview ships es/it/pl/pt(BR)/nl beside en/de/fr (`src/lib/i18n/*.ts`, `Dict`-typed parity), the Rust tray/menu tables carry the same eight (`i18n.rs::ES/IT/PL/PT/NL`), detection prefix-matches every code longest-tag-first (`pt-BR` → `pt`), and Settings carries the follow-system toggle. Frontend suite 427/427 green (incl. `tests/i18n.test.ts` 26 tests); Rust `cargo test --lib i18n` 10/10, `config::` 168/168, `commands::config` 19/19. The five new locales are model-written with human review pending (see CONTRIBUTING). | | Polling session state owned by `AppState` (#758, PARTIAL — polling half) | ⚠ Partial | `polling/state.rs::SessionState` owns the write-decision clocks (D11 generation guard), quiet/snooze latches, last-now-playing cache, preferred-presence session, exit snapshot, and #863 failure/gate mirrors; `poll_once`/`loop`/`state`/`sync`/`diagnostics` thread `&session`. `global_state_lock` deleted; each test builds `SessionState::new()` and `test_two_sessions_do_not_share_clocks_latches_or_caches` proves isolation. Remainder: tray/config `AppCaches` (slice 2) still static, so `QUARANTINE_TEST_LOCK` + `LOCALE_TEST_LOCK` survive. `cargo test --lib` 910/910, clippy `-D warnings` clean. | +| Tray/config caches owned by `AppState` (#758, PARTIAL — slice 2) | ⚠ Partial | `state.rs::AppCaches` owns the throttled devices/queue caches, post-action fetch instants, dedup snapshot, window/playing/mode mirrors, delayed-refresh guard, and quarantine/conflict flags; `load_config`/tray rebuild/paint callers thread `&state.caches`. `CONFIG_QUARANTINED`/`CONFLICT_EVENT_SENT`/tray cache statics deleted, `QUARANTINE_TEST_LOCK` deleted, `test_two_app_states_do_not_share_caches` proves isolation. Remainder: `LOCALE_TEST_LOCK` survives (`i18n::CURRENT` is an issue-blessed keep). `cargo test --lib` 913/913, clippy `-D warnings` clean. | | Monolithic `lib.rs` split into `app`/`cli`/`deep_link`/`state` (#757) | ✅ | `lib.rs` is a 34-line module registry + state re-export shim; `run()`/setup wiring in `app.rs` (`setup_*` helpers), CLI parse/dispatch in `cli.rs`, Spotify-callback exchange in `deep_link.rs`, `AppState` + token-commit seams in `state.rs`. Six scanner guards retargeted (`generate_handler!` → `app.rs`, `handle_spotify_callback` → `deep_link.rs`, `forward_launch_to_running_instance` → `app.rs`, `polling::run_oneshot` call site → `cli.rs`, log-permission setup → `setup_log_permissions`, `macos_deeplink` decl → `lib.rs`); redaction sweep covers the four new modules. `cargo test --lib` 910/910, clippy `-D warnings` clean. | | Desktop notification classes (v4.7.0) | ✅ | `config.rs::NotificationsConfig` (`AppConfig.notifications`): `track_change` (dispatched from `Dashboard.svelte`, 5 s throttle + replace-in-place id), `sync_stopped` (from the always-mounted `routes/+layout.svelte`, and only for `payload?.self_terminated === true`, so a Pause Sync you clicked stays quiet), `auth_required` (from `teams-reconnect-required` unless `payload?.user_initiated === true` — the one user-initiated emitter is `commands/onboarding.rs::reconnect_teams`) and `update_staged` (from `update-stage-complete { version }`, emitted once per successful stage by `updater_bg.rs`). All four default on and each has its own Settings toggle; `stores/notifications.ts` also mirrors preferences across windows. | diff --git a/docs/architecture/storage-and-config.md b/docs/architecture/storage-and-config.md index 5dae3f40..99c2d82c 100644 --- a/docs/architecture/storage-and-config.md +++ b/docs/architecture/storage-and-config.md @@ -27,7 +27,8 @@ settings: - **Corrupt-file quarantine (#379):** a `config.json` that fails `serde_json::from_str` is renamed beside itself to `config.json.bak` (fixed name, never timestamped), a `[CFG] corrupt config … quarantined to …` warning is - logged, `CONFIG_QUARANTINED` is raised, and the app boots on + logged, the per-`AppState` quarantine flag (`AppCaches`, issue #758 slice 2) + is raised, and the app boots on `AppConfig::default()`. The rename is best-effort: a failure is logged and swallowed, and the flag is raised either way, so the original file is never truncated. A *schema-version* mismatch is **not** a quarantine — it goes through diff --git a/src-tauri/src/app.rs b/src-tauri/src/app.rs index 9c49a0ee..b9e0efd1 100644 --- a/src-tauri/src/app.rs +++ b/src-tauri/src/app.rs @@ -329,7 +329,9 @@ pub(crate) fn forward_launch_to_running_instance(app: &AppHandle, argv: Vec>() { + crate::tray::note_window_visibility(&state.caches, true); + } let _ = window.unminimize(); let _ = window.set_focus(); } @@ -528,7 +530,7 @@ fn setup_keychain_cache() { /// Load config into `AppState`, honour start-minimized. fn setup_config(app: &mut tauri::App, state: &Arc, cli_mode: bool) { // Load config into AppState - match config::load_config() { + match config::load_config(&state.caches) { Ok(cfg) => { // #226: wire logging.enabled / log_level into the logger after // config load. The mapping lives in `config::apply_log_level` @@ -567,7 +569,7 @@ fn setup_config(app: &mut tauri::App, state: &Arc, cli_mode: bool) { if let Some(window) = app.get_webview_window("main") { let _ = window.hide(); // Issue #886: report the hide to the tray's mirror. - crate::tray::note_window_visibility(false); + crate::tray::note_window_visibility(&state.caches, false); } #[cfg(target_os = "macos")] { @@ -597,7 +599,7 @@ fn setup_secret_migration(app: &tauri::App, state: &Arc) { // the setup hook runs before any webview has mounted, so the event // can never reach Settings' `onMount` listener — `get_sync_status` // replays the flag for late mounters instead. - if config::migrate_legacy_client_secret_with_app(app.handle()) + if config::migrate_legacy_client_secret_with_app(&state.caches, app.handle()) == config::LegacySecretOutcome::ConflictKeychainDiffers { state @@ -1242,7 +1244,9 @@ pub fn run() { let _ = window.hide(); // Issue #886: the hide has to reach the tray's visibility mirror, // which is what the dedup key is built from. - crate::tray::note_window_visibility(false); + if let Some(state) = window.app_handle().try_state::>() { + crate::tray::note_window_visibility(&state.caches, false); + } api.prevent_close(); } }) diff --git a/src-tauri/src/cli.rs b/src-tauri/src/cli.rs index 532a1b97..215445dc 100644 --- a/src-tauri/src/cli.rs +++ b/src-tauri/src/cli.rs @@ -279,7 +279,7 @@ pub(crate) fn cli_read_tokens() -> Result { pub(crate) fn cli_headless_state() -> (Arc, Vec) { let state = Arc::new(AppState::new()); let mut failures = Vec::new(); - match config::load_config() { + match config::load_config(&state.caches) { Ok(cfg) => *state.config.get_mut() = Some(Arc::new(cfg)), Err(e) => failures.push(format!( "no config loaded ({e}); reporting the built-in defaults" @@ -530,11 +530,10 @@ pub(crate) fn cli_sync_once_preflight( Ok(()) } -/// Load the files [`cli_sync_once_preflight`] decides on, turning a load -/// failure into the reason the CLI prints. pub(crate) fn cli_sync_once_preflight_from_disk() -> Result<(), String> { + let caches = crate::state::AppCaches::new(); let config = - config::load_config().map_err(|e| format!("cannot read the stored config: {e}"))?; + config::load_config(&caches).map_err(|e| format!("cannot read the stored config: {e}"))?; let tokens = cli_read_tokens().map_err(|e| format!("cannot read the stored tokens: {e}"))?; cli_sync_once_preflight(&config, &tokens) } diff --git a/src-tauri/src/commands/config.rs b/src-tauri/src/commands/config.rs index 9099aeb0..093fb78b 100644 --- a/src-tauri/src/commands/config.rs +++ b/src-tauri/src/commands/config.rs @@ -27,9 +27,10 @@ const CMD: &str = "[CMD.CONFIG]"; /// D-Bus timeout, so simply launching the app froze the window, the tray /// menu and window events for seconds. Both halves now run on the blocking /// pool (the same seam `get_recent_logs` uses). -pub async fn load_config() -> Result { +pub async fn load_config(state: tauri::State<'_, Arc>) -> Result { log::debug!("{CMD} load_config: ENTRY"); - match load_config_offloaded(config::load_config).await { + let caches = Arc::clone(state.inner()); + match load_config_offloaded(move || config::load_config(&caches.caches)).await { Ok(cfg) => { log::info!( "{CMD} load_config: SUCCESS - spotify.client_id.len={}", @@ -48,7 +49,8 @@ pub async fn load_config() -> Result { /// /// The loader is injected so the unit test can observe *which thread the /// read ran on* — an assertion no source inspection can make. Production -/// passes [`config::load_config`]; a join failure is folded into the same +/// passes a closure over [`config::load_config`] bound to the caller's +/// `AppCaches` (issue #758 slice 2); a join failure is folded into the same /// `String` error channel the command already reports. async fn load_config_offloaded(load: F) -> Result where @@ -146,7 +148,7 @@ pub async fn update_config( let mut config_guard = state_clone.config.get_mut(); let base = match config_guard.as_ref() { Some(current) => (**current).clone(), - None => config::load_config()?, + None => config::load_config(&state_clone.caches)?, }; let mut merged = base; @@ -602,7 +604,7 @@ pub async fn export_config( let guard = state.config.get(); match guard.as_ref() { Some(cfg) => (**cfg).clone(), - None => config::load_config()?, + None => config::load_config(&state.caches)?, } }; let json = config::export_document(¤t)?; @@ -979,7 +981,7 @@ pub async fn import_config( &destination, &prepared.document, || {}, - config::load_config, + || config::load_config(&state_clone.caches), ) }) .await @@ -1056,7 +1058,7 @@ pub async fn set_locale( let mut config_guard = state_clone.config.get_mut(); let mut merged = match config_guard.as_ref() { Some(current) => (**current).clone(), - None => config::load_config()?, + None => config::load_config(&state_clone.caches)?, }; merged.locale = Some(tag.to_string()); diff --git a/src-tauri/src/commands/onboarding.rs b/src-tauri/src/commands/onboarding.rs index 01430a36..870f809b 100644 --- a/src-tauri/src/commands/onboarding.rs +++ b/src-tauri/src/commands/onboarding.rs @@ -443,9 +443,8 @@ fn teams_session_verdict( /// from `is_onboarding_complete` so the async runtime can keep serving other /// commands while the refresh round-trips complete. fn is_onboarding_complete_impl(state: &Arc, app: &AppHandle) -> Result { - let config = config::load_config()?; + let config = config::load_config(&state.caches)?; let spotify_configured = !config.spotify.client_id.is_empty(); - // Clone out of the token locks BEFORE any network call: a read guard held // across a 10 s HTTPS round-trip would block the polling thread's write to // the same slot for that whole window. diff --git a/src-tauri/src/commands/rules.rs b/src-tauri/src/commands/rules.rs index b7dce30a..18707c14 100644 --- a/src-tauri/src/commands/rules.rs +++ b/src-tauri/src/commands/rules.rs @@ -14,7 +14,7 @@ use crate::polling::{ track_rule_conditions_match, track_rule_hit, track_rule_schedule_matches, TrackRuleContext, }; use serde::{Deserialize, Serialize}; -use tauri::AppHandle; +use tauri::{AppHandle, Manager}; /// Log tag prefix for this submodule (issue #79 item 3). const CMD: &str = "[CMD.RULES]"; @@ -92,7 +92,7 @@ pub struct RulesExplanation { /// verdict matches what a real poll would have done. #[tauri::command] pub fn explain_rules( - _app: AppHandle, + app: AppHandle, now_minutes: u16, weekday: u8, synthetic_track: SyntheticTrack, @@ -103,7 +103,14 @@ pub fn explain_rules( synthetic_track.artist, synthetic_track.track, ); - let config = crate::config::load_config().ok().map(std::sync::Arc::new); + let config = app + .try_state::>() + .map(|s| { + crate::config::load_config(&s.caches) + .ok() + .map(std::sync::Arc::new) + }) + .unwrap_or(None); explain_rules_with_config(&config, now_minutes, weekday, &synthetic_track) } diff --git a/src-tauri/src/commands/window.rs b/src-tauri/src/commands/window.rs index 11c99350..134b7b4e 100644 --- a/src-tauri/src/commands/window.rs +++ b/src-tauri/src/commands/window.rs @@ -154,7 +154,7 @@ pub async fn set_autostart_enabled( let mut config_guard = state_clone.config.get_mut(); let mut merged = match config_guard.as_ref() { Some(current) => (**current).clone(), - None => config::load_config().map_err(|e| { + None => config::load_config(&state_clone.caches).map_err(|e| { log::error!("{CMD} set_autostart_enabled: config load FAILED - {}", e); ShortcutReason::autostart(&e) })?, diff --git a/src-tauri/src/config/io.rs b/src-tauri/src/config/io.rs index d2427dd3..070b38b9 100644 --- a/src-tauri/src/config/io.rs +++ b/src-tauri/src/config/io.rs @@ -14,7 +14,7 @@ use std::io::{Read, Write}; #[cfg(unix)] use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; use std::path::PathBuf; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::Ordering; use tauri::Emitter; pub fn config_dir() -> Result { // Maintained replacement for the unmaintained `dirs` crate (issue #418): @@ -56,13 +56,10 @@ pub fn get_config_path() -> Result { /// Set when `load_config` finds a corrupt config.json and quarantines it to /// `.bak` (issue #379). Diagnostics-visible via /// [`config_was_quarantined`]; warn-log-only otherwise — no other channel is -/// touched by this slice. -pub(crate) static CONFIG_QUARANTINED: AtomicBool = AtomicBool::new(false); - -/// Diagnostics-visible flag: true once this process has quarantined a corrupt -/// config.json to `.bak` and fallen back to defaults (issue #379). -pub fn config_was_quarantined() -> bool { - CONFIG_QUARANTINED.load(Ordering::SeqCst) +/// touched by this slice. Owned by `AppCaches` (issue #758 slice 2) so each +/// test constructs isolated flags instead of serialising on a test lock. +pub fn config_was_quarantined(caches: &crate::state::AppCaches) -> bool { + caches.quarantined_flag().load(Ordering::SeqCst) } /// Backup path alongside the original: `config.json` → `config.json.bak`. @@ -251,6 +248,7 @@ pub fn config_quarantine_backup_name() -> Option { /// rename errors are logged and swallowed so the caller falls back to /// defaults either way (issue #379). pub(crate) fn quarantine_corrupt_config( + caches: &crate::state::AppCaches, path: &std::path::Path, parse_err: impl std::fmt::Display, ) -> PathBuf { @@ -270,7 +268,7 @@ pub(crate) fn quarantine_corrupt_config( rename_err ), } - CONFIG_QUARANTINED.store(true, Ordering::SeqCst); + caches.quarantined_flag().store(true, Ordering::SeqCst); backup } @@ -432,8 +430,8 @@ pub(crate) fn tighten_config_permissions(path: &std::path::Path) { } } -pub fn load_config() -> Result { - load_config_from(&get_config_path()?).map(|config| { +pub fn load_config(caches: &crate::state::AppCaches) -> Result { + load_config_from(caches, &get_config_path()?).map(|config| { with_keychain_flags(config, || { crate::keychain::cached_spotify_client_secret_presence() }) @@ -444,7 +442,10 @@ pub fn load_config() -> Result { /// parse and the normalization, with the keychain stamping left to the public /// entry point — so this half is testable against real files with no keychain /// probe, the same shape [`import_config_document`] uses. -pub(crate) fn load_config_from(path: &std::path::Path) -> Result { +pub(crate) fn load_config_from( + caches: &crate::state::AppCaches, + path: &std::path::Path, +) -> Result { if !path.exists() { log::info!( "[CFG] Config file not found at '{}', using defaults", @@ -476,6 +477,7 @@ pub(crate) fn load_config_from(path: &std::path::Path) -> Result { quarantine_corrupt_config( + caches, path, format!("expected a JSON object, found {}", json_kind(&other)), ); @@ -485,7 +487,7 @@ pub(crate) fn load_config_from(path: &std::path::Path) -> Result.bak` alongside the original and boot on // defaults. Observable via `config_was_quarantined()`. - quarantine_corrupt_config(path, &e); + quarantine_corrupt_config(caches, path, &e); return Ok(AppConfig::default()); } }; diff --git a/src-tauri/src/config/migrate.rs b/src-tauri/src/config/migrate.rs index d7241250..4be85701 100644 --- a/src-tauri/src/config/migrate.rs +++ b/src-tauri/src/config/migrate.rs @@ -2,7 +2,7 @@ use super::io::{atomic_write_json, get_config_path}; use super::schema::AppConfig; use super::transfer::{legacy_client_secret, strip_client_secret_keys}; use std::fs; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::Ordering; use tauri::Emitter; /// The config schema version THIS binary writes (CfgDiag#1, issue #536). /// Bump whenever the persisted shape gains or changes a field that needs a @@ -96,10 +96,13 @@ pub fn migrate_legacy_client_secret() { /// one-time [`SPOTIFY_SECRET_CONFLICT_EVENT`] so Settings can prompt /// Settings → Reconnect Spotify (payload carries the manual step). /// All other outcomes are silent apart from the usual `[CFG]` logs. -pub fn migrate_legacy_client_secret_with_app(app: &tauri::AppHandle) -> LegacySecretOutcome { +pub fn migrate_legacy_client_secret_with_app( + caches: &crate::state::AppCaches, + app: &tauri::AppHandle, +) -> LegacySecretOutcome { let outcome = run_legacy_secret_migration(); if outcome == LegacySecretOutcome::ConflictKeychainDiffers { - emit_spotify_secret_conflict_once(app); + emit_spotify_secret_conflict_once(caches, app); } outcome } @@ -136,15 +139,14 @@ pub(crate) fn decide_legacy_secret_outcome( Err(_) => LegacySecretOutcome::Migrated, } } -/// Process-wide guard so the conflict event fires at most once per launch, -/// no matter how often the migration entry points are called. -static CONFLICT_EVENT_SENT: AtomicBool = AtomicBool::new(false); -/// Emit [`SPOTIFY_SECRET_CONFLICT_EVENT`] unless already sent this process. -/// Follows the `let _ = app.emit(...)` pattern used in `poll_once.rs`; -/// the payload tells Settings to prompt Reconnect Spotify. Returns true -/// when this call performed the (single) emit. -fn emit_spotify_secret_conflict_once(app: &tauri::AppHandle) -> bool { - if CONFLICT_EVENT_SENT.swap(true, Ordering::AcqRel) { +/// Per-`AppState` guard so the conflict event fires at most once per launch, +/// no matter how often the migration entry points are called (issue #758 +/// slice 2: owned by `AppCaches`, not a process static). +fn emit_spotify_secret_conflict_once( + caches: &crate::state::AppCaches, + app: &tauri::AppHandle, +) -> bool { + if caches.conflict_sent_flag().swap(true, Ordering::AcqRel) { return false; } log::warn!( diff --git a/src-tauri/src/config/mod.rs b/src-tauri/src/config/mod.rs index c78926bb..aa2c4452 100644 --- a/src-tauri/src/config/mod.rs +++ b/src-tauri/src/config/mod.rs @@ -57,8 +57,7 @@ mod tests { }; use super::io::{ load_config_from, quarantine_backup_name_for, quarantine_corrupt_config, save_config_to, - staged_import_path, stamp_keychain_flags, with_keychain_flags, CONFIG_QUARANTINED, - TYPED_CONFIG_KEYS, + staged_import_path, stamp_keychain_flags, with_keychain_flags, TYPED_CONFIG_KEYS, }; use super::migrate::{ decide_legacy_secret_outcome, default_schema_version, migrate_config, @@ -71,8 +70,15 @@ mod tests { use crate::profanity; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; - use std::sync::atomic::Ordering; - static QUARANTINE_TEST_LOCK: parking_lot::Mutex<()> = parking_lot::Mutex::new(()); + + /// Serialises the tests that assert on captured log output (issue #758 + /// slice 2). This is NOT a state-isolation lock: the quarantine flag and + /// the config caches it used to guard are per-`AppCaches` now, so those + /// tests construct their own and run in parallel. What remains genuinely + /// process-wide is `log::set_boxed_logger` — one logger per process, whose + /// sink is the shared `LOG_LINES` buffer below — so a test that clears and + /// reads that buffer has to exclude the other tests that do the same. + static LOG_CAPTURE_TEST_LOCK: parking_lot::Mutex<()> = parking_lot::Mutex::new(()); #[test] fn test_default_config() { @@ -143,7 +149,8 @@ mod tests { let path = dir.join("config.json"); std::fs::write(&path, "{}\n").expect("the config seam fixture must be writable"); - let loaded = load_config_from(&path).expect("the real config file seam must load"); + let caches = crate::state::AppCaches::new(); + let loaded = load_config_from(&caches, &path).expect("the real config file seam must load"); let calls = std::sync::atomic::AtomicUsize::new(0); let config = with_keychain_flags(loaded, || { calls.fetch_add(1, std::sync::atomic::Ordering::SeqCst); @@ -459,10 +466,9 @@ mod tests { /// alongside the original and the diagnostics-visible flag is raised. #[test] fn test_corrupt_config_quarantined_to_bak() { - // Process-wide CONFIG_QUARANTINED is global: serialize the two - // quarantine tests so parallel save/restore cannot interleave. - let _guard = QUARANTINE_TEST_LOCK.lock(); - let prev = CONFIG_QUARANTINED.load(Ordering::SeqCst); + // Issue #758 slice 2: the quarantine flag is per-`AppCaches`, so this + // test owns its flag and needs no process-wide lock or save/restore. + let caches = crate::state::AppCaches::new(); let dir = std::env::temp_dir().join(format!( "pj-test-quarantine-{}-{}", std::process::id(), @@ -476,7 +482,7 @@ mod tests { serde_json::from_str::(&std::fs::read_to_string(&path).unwrap()).is_err() ); - let backup = quarantine_corrupt_config(&path, "test corrupt sentinel"); + let backup = quarantine_corrupt_config(&caches, &path, "test corrupt sentinel"); assert_eq!(backup, dir.join("config.json.bak")); assert!(!path.exists(), "corrupt original must be renamed away"); assert_eq!( @@ -485,14 +491,13 @@ mod tests { "quarantined copy must preserve the corrupt bytes" ); assert!( - config_was_quarantined(), + config_was_quarantined(&caches), "diagnostics-visible flag must be raised after quarantine" ); // Defaults remain loadable alongside the quarantine. assert_eq!(AppConfig::default().schema_version, 1); let _ = std::fs::remove_dir_all(&dir); - CONFIG_QUARANTINED.store(prev, Ordering::SeqCst); } /// Issue #379: when the quarantine rename itself fails (e.g. a @@ -501,10 +506,8 @@ mod tests { /// the diagnostics-visible flag is raised either way. #[test] fn test_corrupt_config_quarantine_rename_failure_preserves_original() { - // Process-wide CONFIG_QUARANTINED is global: serialize the two - // quarantine tests so parallel save/restore cannot interleave. - let _guard = QUARANTINE_TEST_LOCK.lock(); - let prev = CONFIG_QUARANTINED.load(Ordering::SeqCst); + // Issue #758 slice 2: per-test caches, no process-wide lock. + let caches = crate::state::AppCaches::new(); let dir = std::env::temp_dir().join(format!( "pj-test-quarantine-fail-{}-{}", std::process::id(), @@ -518,7 +521,7 @@ mod tests { // `fs::rename(file, dir)` fail on both POSIX and Windows. std::fs::create_dir_all(dir.join("config.json.bak")).unwrap(); - let backup = quarantine_corrupt_config(&path, "test rename-failure sentinel"); + let backup = quarantine_corrupt_config(&caches, &path, "test rename-failure sentinel"); assert_eq!(backup, dir.join("config.json.bak")); // Rename failed → the corrupt original must still be in place with // its bytes untouched, and the flag is raised so diagnostics still @@ -533,14 +536,13 @@ mod tests { "failed quarantine must not truncate or alter the original" ); assert!( - config_was_quarantined(), + config_was_quarantined(&caches), "diagnostics-visible flag must be raised even when the rename fails" ); // Defaults remain loadable alongside the failed quarantine. assert_eq!(AppConfig::default().schema_version, 1); let _ = std::fs::remove_dir_all(&dir); - CONFIG_QUARANTINED.store(prev, Ordering::SeqCst); } /// Issue #379: `extra` keys serialize in deterministic (sorted) order so @@ -1393,7 +1395,9 @@ mod tests { "patch-channel-refused", r#"{"updates": {"channel": "nightly", "future_channel_key": 1}, "autostart": true}"#, ); - let cfg = load_config_from(&path).expect("a bad channel must not fail the document"); + let caches = crate::state::AppCaches::new(); + let cfg = + load_config_from(&caches, &path).expect("a bad channel must not fail the document"); assert_eq!(cfg.updates.channel, UpdateChannel::Stable); assert!( cfg.updates.extra.contains_key("future_channel_key"), @@ -3363,7 +3367,7 @@ mod tests { /// setting was lost, and nothing was logged. #[test] fn test_unknown_update_channel_keeps_the_rest_of_the_config_and_warns() { - let _guard = QUARANTINE_TEST_LOCK.lock(); + let _guard = LOG_CAPTURE_TEST_LOCK.lock(); LOGGER.call_once(|| { // Best-effort: another test may have installed a logger first. let _ = log::set_boxed_logger(Box::new(CapturingLogger)); @@ -3442,15 +3446,16 @@ mod tests { /// polling tuning and everything else too. #[test] fn test_one_bad_section_keeps_every_other_section() { - let _guard = QUARANTINE_TEST_LOCK.lock(); + // Only the shared log sink needs serialising (issue #758 slice 2); the + // quarantine flag is this test's own `caches`. + let _guard = LOG_CAPTURE_TEST_LOCK.lock(); LOGGER.call_once(|| { // Best-effort: another test may have installed a logger first. let _ = log::set_boxed_logger(Box::new(CapturingLogger)); log::set_max_level(log::LevelFilter::Warn); }); LOG_LINES.lock().clear(); - let prev = CONFIG_QUARANTINED.load(Ordering::SeqCst); - CONFIG_QUARANTINED.store(false, Ordering::SeqCst); + let caches = crate::state::AppCaches::new(); let (dir, path) = temp_config_file( "sections", @@ -3467,7 +3472,8 @@ mod tests { }"#, ); - let cfg = load_config_from(&path).expect("a partially invalid document must still load"); + let cfg = + load_config_from(&caches, &path).expect("a partially invalid document must still load"); assert_eq!( cfg.teams.status_format, @@ -3494,7 +3500,7 @@ mod tests { !quarantine_backup_path(&path).exists(), "one bad section must not quarantine the whole file" ); - assert!(!config_was_quarantined()); + assert!(!config_was_quarantined(&caches)); let logged = LOG_LINES.lock().clone(); assert!( logged @@ -3503,7 +3509,6 @@ mod tests { "the fallback must name the field it replaced: {logged:?}" ); - CONFIG_QUARANTINED.store(prev, Ordering::SeqCst); let _ = std::fs::remove_dir_all(&dir); } @@ -3513,15 +3518,13 @@ mod tests { /// the section's defaults, the second as those defaults plus a warning. #[test] fn test_empty_or_null_client_id_loads_without_quarantining() { - let _guard = QUARANTINE_TEST_LOCK.lock(); - let prev = CONFIG_QUARANTINED.load(Ordering::SeqCst); - CONFIG_QUARANTINED.store(false, Ordering::SeqCst); + let caches = crate::state::AppCaches::new(); for contents in [ r#"{"spotify": {}, "autostart": true}"#, r#"{"spotify": {"client_id": null}, "autostart": true}"#, ] { let (dir, path) = temp_config_file("client-id", contents); - let cfg = load_config_from(&path).expect("must load"); + let cfg = load_config_from(&caches, &path).expect("must load"); assert_eq!(cfg.spotify.client_id, "", "{contents}"); assert_eq!( cfg.spotify.redirect_uri, @@ -3533,10 +3536,9 @@ mod tests { "the rest of the document is kept: {contents}" ); assert!(!quarantine_backup_path(&path).exists(), "{contents}"); - assert!(!config_was_quarantined(), "{contents}"); + assert!(!config_was_quarantined(&caches), "{contents}"); let _ = std::fs::remove_dir_all(&dir); } - CONFIG_QUARANTINED.store(prev, Ordering::SeqCst); } /// Issue #926: the field-by-field loader is for documents that ARE an @@ -3545,12 +3547,12 @@ mod tests { /// with the app booting on defaults. #[test] fn test_non_object_root_is_still_quarantined() { - let _guard = QUARANTINE_TEST_LOCK.lock(); - let prev = CONFIG_QUARANTINED.load(Ordering::SeqCst); for contents in ["[1, 2, 3]", "\"spotify\"", "null", "{ NOT VALID JSON !!!"] { - CONFIG_QUARANTINED.store(false, Ordering::SeqCst); + // A fresh `AppCaches` per iteration, so each case starts from an + // un-raised flag without any save/restore dance (issue #758). + let caches = crate::state::AppCaches::new(); let (dir, path) = temp_config_file("non-object", contents); - let cfg = load_config_from(&path).expect("quarantine still yields defaults"); + let cfg = load_config_from(&caches, &path).expect("quarantine still yields defaults"); assert_eq!(cfg.schema_version, default_schema_version()); assert!(cfg.spotify.client_id.is_empty()); assert!( @@ -3558,10 +3560,9 @@ mod tests { "{contents} must be quarantined" ); assert!(!path.exists(), "{contents}"); - assert!(config_was_quarantined(), "{contents}"); + assert!(config_was_quarantined(&caches), "{contents}"); let _ = std::fs::remove_dir_all(&dir); } - CONFIG_QUARANTINED.store(prev, Ordering::SeqCst); } // ----------------------------------------------------------------- @@ -3643,7 +3644,8 @@ mod tests { fn test_loose_config_is_still_tightened_on_load() { let (dir, path) = temp_config_file("tighten", r#"{"autostart": true}"#); std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); - let cfg = load_config_from(&path).expect("a readable config must load"); + let caches = crate::state::AppCaches::new(); + let cfg = load_config_from(&caches, &path).expect("a readable config must load"); assert!(cfg.autostart); assert_eq!( std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, @@ -3712,7 +3714,8 @@ mod tests { {"enabled": true, "days": [0]} ]}}"#, ); - let cfg = load_config_from(&path).expect("must load"); + let caches = crate::state::AppCaches::new(); + let cfg = load_config_from(&caches, &path).expect("must load"); let first = &cfg.status_rules.quiet_hours[0]; assert_eq!(first.days, vec![2]); assert_eq!(first.start_minutes, 1439); @@ -3749,7 +3752,8 @@ mod tests { "future_rule_flag": "r"}, "shortcuts": {"future_shortcut": "s"}}"#, ); - let cfg = load_config_from(&path).expect("must load"); + let caches = crate::state::AppCaches::new(); + let cfg = load_config_from(&caches, &path).expect("must load"); assert_eq!( cfg.teams.extra.get("future_flag"), Some(&serde_json::json!(true)) @@ -3818,7 +3822,8 @@ mod tests { fn test_save_refuses_a_newer_document_and_never_lowers_the_marker() { let contents = r#"{"schema_version": 99, "future_key": {"kept": true}, "autostart": true}"#; let (dir, path) = temp_config_file("newer-than-us", contents); - let cfg = load_config_from(&path).expect("a newer document must still load"); + let caches = crate::state::AppCaches::new(); + let cfg = load_config_from(&caches, &path).expect("a newer document must still load"); assert_eq!( cfg.schema_version, 99, "a newer document keeps its own version instead of being relabelled" @@ -3871,7 +3876,8 @@ mod tests { "availability": "Busy", "activity": "InACall", "future_pp_flag": true, "future_pp_block": {"a": [1, 2]}}}}"#, ); - let cfg = load_config_from(&path).expect("must load"); + let caches = crate::state::AppCaches::new(); + let cfg = load_config_from(&caches, &path).expect("must load"); assert_eq!( cfg.teams.preferred_presence.extra.get("future_pp_flag"), Some(&serde_json::json!(true)), @@ -3912,7 +3918,8 @@ mod tests { "teams": {"preferred_presence": {"client_secret": "PP-SENTINEL", "kept_pp": 3}}}"#, ); - let cfg = load_config_from(&path).expect("must load"); + let caches = crate::state::AppCaches::new(); + let cfg = load_config_from(&caches, &path).expect("must load"); let serialized = serde_json::to_string(&cfg).expect("must serialize"); assert!( @@ -3990,7 +3997,8 @@ mod tests { ); // And it is still a usable config: the next launch must not boot on // defaults. - let loaded = load_config_from(&path).expect("the surviving config must load"); + let caches = crate::state::AppCaches::new(); + let loaded = load_config_from(&caches, &path).expect("the surviving config must load"); assert_eq!(loaded.spotify.client_id, "LIVE"); assert_eq!(loaded.logging.keep_files, 7); @@ -4055,7 +4063,8 @@ mod tests { "client_secret": "RULES-SENTINEL"}, "shortcuts": {"client_secret": "SHORTCUT-SENTINEL"}}"#, ); - let cfg = load_config_from(&path).expect("must load"); + let caches = crate::state::AppCaches::new(); + let cfg = load_config_from(&caches, &path).expect("must load"); let serialized = serde_json::to_string(&cfg).expect("must serialize"); assert!( @@ -4247,12 +4256,13 @@ mod tests { /// and the value itself never reaches the log. #[test] fn test_legacy_secret_sidecar_survives_a_later_save() { - let _guard = QUARANTINE_TEST_LOCK.lock(); + let _guard = LOG_CAPTURE_TEST_LOCK.lock(); LOGGER.call_once(|| { let _ = log::set_boxed_logger(Box::new(CapturingLogger)); log::set_max_level(log::LevelFilter::Warn); }); LOG_LINES.lock().clear(); + let caches = crate::state::AppCaches::new(); let (dir, path) = temp_config_file( "legacy-sidecar", @@ -4277,7 +4287,7 @@ mod tests { // Any later save replaces `config.json` wholesale — the #803 premise — // and the sidecar is what keeps the user's copy. - let mut cfg = load_config_from(&path).expect("must load"); + let mut cfg = load_config_from(&caches, &path).expect("must load"); cfg.autostart = false; save_config_to(&path, &cfg).expect("save must succeed"); assert!( @@ -4320,7 +4330,8 @@ mod tests { fn test_save_rejects_a_stale_revision_and_leaves_the_file_alone() { let contents = r#"{"autostart": true, "revision": 5}"#; let (dir, path) = temp_config_file("stale-revision", contents); - let mut stale = load_config_from(&path).expect("must load"); + let caches = crate::state::AppCaches::new(); + let mut stale = load_config_from(&caches, &path).expect("must load"); assert_eq!(stale.revision, 5); stale.revision = 4; // the other window saved in between @@ -4346,7 +4357,8 @@ mod tests { #[test] fn test_save_advances_the_revision_monotonically() { let (dir, path) = temp_config_file("revision", r#"{"autostart": true}"#); - let cfg = load_config_from(&path).expect("must load"); + let caches = crate::state::AppCaches::new(); + let cfg = load_config_from(&caches, &path).expect("must load"); assert_eq!(cfg.revision, 0, "a pre-#943 file reads as revision 0"); let first = save_config_to(&path, &cfg).expect("first save"); @@ -4375,7 +4387,8 @@ mod tests { #[test] fn test_a_payload_without_a_revision_is_stamped_not_refused() { let (dir, path) = temp_config_file("no-revision", r#"{"autostart": true, "revision": 7}"#); - let mut payload = load_config_from(&path).expect("must load"); + let caches = crate::state::AppCaches::new(); + let mut payload = load_config_from(&caches, &path).expect("must load"); payload.revision = 0; payload.autostart = false; diff --git a/src-tauri/src/diagnostics.rs b/src-tauri/src/diagnostics.rs index 4f9d22d1..a02e9b85 100644 --- a/src-tauri/src/diagnostics.rs +++ b/src-tauri/src/diagnostics.rs @@ -316,11 +316,11 @@ pub struct ConfigQuarantine { } impl ConfigQuarantine { - /// Production read of the process flag plus an existence probe on the - /// `.bak` sibling of the real config path. - fn observe() -> Self { + /// Production read of the per-`AppState` flag plus an existence probe on + /// the `.bak` sibling of the real config path (issue #758 slice 2). + fn observe(caches: &crate::state::AppCaches) -> Self { Self { - quarantined: crate::config::config_was_quarantined(), + quarantined: crate::config::config_was_quarantined(caches), backup_name: crate::config::config_quarantine_backup_name(), } } @@ -1275,9 +1275,8 @@ pub async fn get_diagnostics_snapshot(app: AppHandle) -> Result Result log_dir, probe_keychain(), crate::updater_bg::read_failed_install_marker(), - ConfigQuarantine::observe(), + ConfigQuarantine::observe(&state.caches), ); let bytes = serialize_snapshot(&snapshot)?; let dir = app_clone.path().download_dir().map_err(|e| { diff --git a/src-tauri/src/menu.rs b/src-tauri/src/menu.rs index 119192c0..3e1b5c5e 100644 --- a/src-tauri/src/menu.rs +++ b/src-tauri/src/menu.rs @@ -18,7 +18,9 @@ fn show_and_focus_main_window(app: &AppHandle) { if let Some(window) = app.get_webview_window("main") { let _ = window.show(); // Issue #886: report the raise to the mirror the tray dedup key reads. - crate::tray::note_window_visibility(true); + if let Some(state) = app.try_state::>() { + crate::tray::note_window_visibility(&state.caches, true); + } // Issue #483: a minimized window stays minimized after show() -- // unminimize first (mirrors the single-instance raise in lib.rs). let _ = window.unminimize(); diff --git a/src-tauri/src/polling/iteration.rs b/src-tauri/src/polling/iteration.rs index 76500b61..b8a446a6 100644 --- a/src-tauri/src/polling/iteration.rs +++ b/src-tauri/src/polling/iteration.rs @@ -577,7 +577,11 @@ fn run_inner( // extra request, no cache. System sources do not surface // shuffle/repeat (the spec leaves them unset); the helper // is a no-op in that case. - crate::tray::note_playback_modes(now.context.shuffle, now.context.repeat); + crate::tray::note_playback_modes( + &state.caches, + now.context.shuffle, + now.context.repeat, + ); // Issue #344: debug, not info — title/artist at info // level land verbatim in the diagnostics `recent_logs` // tail (a paste-able support artifact). No raw track @@ -770,6 +774,7 @@ fn run_inner( return iteration; } crate::tray::note_playback_modes( + &state.caches, now.context.shuffle, now.context.repeat, ); diff --git a/src-tauri/src/state.rs b/src-tauri/src/state.rs index 5f053ce1..0b6cdfff 100644 --- a/src-tauri/src/state.rs +++ b/src-tauri/src/state.rs @@ -782,9 +782,7 @@ impl AppCaches { ) -> &Mutex)>> { &self.devices_cache } - pub(crate) fn queue_slot( - &self, - ) -> &Mutex> { + pub(crate) fn queue_slot(&self) -> &Mutex> { &self.queue_cache } pub(crate) fn last_tray_action_slot(&self) -> &Mutex> { @@ -793,9 +791,7 @@ impl AppCaches { pub(crate) fn last_action_fetch_slot(&self) -> &Mutex> { &self.last_action_fetch } - pub(crate) fn last_tray_state_slot( - &self, - ) -> &Mutex> { + pub(crate) fn last_tray_state_slot(&self) -> &Mutex> { &self.last_tray_state } pub(crate) fn window_visible_flag(&self) -> &AtomicBool { @@ -1228,7 +1224,7 @@ mod tests { .expect("app.rs has no #[cfg(test)] mod tests block"); // Wiring half: setup persists the conflict outcome onto process state. for marker in [ - "migrate_legacy_client_secret_with_app(app.handle())", + "migrate_legacy_client_secret_with_app(", "LegacySecretOutcome::ConflictKeychainDiffers", "secret_conflict", ] { @@ -1289,4 +1285,36 @@ mod tests { } } } + + /// Issue #758 slice 2: two `AppState`s never share one tray. Quarantining + /// through one state's caches raises only its flag; the other's stays + /// down. Fails pre-fix by construction: a single process-wide + /// `CONFIG_QUARANTINED` static is raised no matter which state loads. + #[test] + fn test_two_app_states_do_not_share_caches() { + use std::sync::atomic::Ordering; + let first = AppState::new(); + let second = AppState::new(); + first + .caches + .quarantined_flag() + .store(true, Ordering::SeqCst); + assert!( + crate::config::config_was_quarantined(&first.caches), + "the quarantined state must observe its own flag" + ); + assert!( + !crate::config::config_was_quarantined(&second.caches), + "a second AppState must not observe the first one's quarantine" + ); + crate::tray::note_window_visibility(&first.caches, true); + assert!( + crate::tray::window_visible(&first.caches), + "the first state's mirror must report what was recorded" + ); + assert!( + !crate::tray::window_visible(&second.caches), + "the second state's mirror must stay at its default" + ); + } } diff --git a/src-tauri/src/tray/actions.rs b/src-tauri/src/tray/actions.rs index c4117c4e..2beeb19b 100644 --- a/src-tauri/src/tray/actions.rs +++ b/src-tauri/src/tray/actions.rs @@ -20,7 +20,6 @@ pub fn tray_write_lock() -> &'static parking_lot::Mutex<()> { TRAY_WRITE_LOCK.get_or_init(|| parking_lot::Mutex::new(())) } - /// Runs a Spotify player action from a tray click using the stored access /// token. On success the tray menu is force-refreshed and the action's /// deterministic outcome is recorded for the items that mirror it: diff --git a/src-tauri/src/tray/cache.rs b/src-tauri/src/tray/cache.rs index 2de14928..f43984c6 100644 --- a/src-tauri/src/tray/cache.rs +++ b/src-tauri/src/tray/cache.rs @@ -81,7 +81,6 @@ pub const TRAY_SPOTIFY_FETCH_THROTTLE: Duration = Duration::from_secs(60); /// live re-fetch when stale (issue #388). pub type DeviceCacheSlot = Option<(Instant, Vec)>; - /// Shortest gap between the post-action re-fetches of the Devices/Up Next lists /// (issue #883). A player action wants those submenus to mirror what just /// happened, but a burst of clicks must share one devices+queue pair: five @@ -89,8 +88,6 @@ pub type DeviceCacheSlot = Option<(Instant, Vec)>; /// matches the click the user just made. pub const TRAY_POST_ACTION_FETCH_MIN: Duration = Duration::from_secs(5); - - /// Whether a rebuild must re-fetch the Devices/Up Next lists on behalf of a /// player action (issue #883). Pure in its instants, so the coalescing rule — /// ten rapid clicks, one pair of requests — is unit-testable without waiting. diff --git a/src-tauri/src/tray/dedup.rs b/src-tauri/src/tray/dedup.rs index 3a75559b..e29e3d5a 100644 --- a/src-tauri/src/tray/dedup.rs +++ b/src-tauri/src/tray/dedup.rs @@ -74,6 +74,10 @@ pub fn tray_state_changed(prev: Option<&TrayStateSnapshot>, next: &TrayStateSnap /// the throttle bucket of both caches (issue #805). /// Single construction site so the key can never be built from a subset of what /// the menu shows (issue #691). +// Nine inputs by design (issue #691): the key is built at one construction +// site from everything the menu renders, so it stays a pure function that +// can never be built from a subset. +#[allow(clippy::too_many_arguments)] pub fn tray_snapshot_for( is_syncing: bool, is_window_visible: bool, @@ -136,13 +140,6 @@ pub fn shuffle_toggle_target(current: bool) -> bool { !current } -/// Last known Spotify playing state, driving the Play/Pause check mark and -/// the tray status line (and the toggle's fallback dispatch). Re-seeded -/// from the polling loop's stored track on a genuine track change, from the -/// poller's `playback-state-changed` event when the playing state changes -/// for the same track (issue #689), and by the tray's own successful -/// play/pause/transfer actions. Issue #3.0-P3. - /// Records the playing state the Play/Pause mark and the status line render. /// Every path that learns the truth writes it here, so the mark never infers /// playback from a candidate that may already be stale. Issue #3.0-P3. @@ -169,24 +166,11 @@ pub fn consume_playback_state_changed(caches: &crate::state::AppCaches, payload: } } -/// Last known shuffle state, driving the Shuffle item's check mark -/// (issue #582). Written by the polling loop from the poll body -/// (`note_playback_modes`) and optimistically by the tray's own successful -/// toggle, so a same-track toggle does not wait for the next poll. It is a -/// module-level atomic rather than a field on the app's frozen `TrackInfo` -/// because that type is the ts-rs-exported IPC shape shared with the -/// Dashboard and built by exhaustive literals outside this module. - - /// Records the playback modes a poll body reported. Called by the polling /// loop for every observed item (playing or paused) — the poll response is /// the source of truth for both toggles, so no extra Spotify request is /// needed to render them. See issue #582. -pub fn note_playback_modes( - caches: &crate::state::AppCaches, - shuffle: bool, - repeat: RepeatState, -) { +pub fn note_playback_modes(caches: &crate::state::AppCaches, shuffle: bool, repeat: RepeatState) { caches.shuffle_flag().store(shuffle, Ordering::Release); caches.repeat_flag().store(repeat as u8, Ordering::Release); } diff --git a/src-tauri/src/tray/devices.rs b/src-tauri/src/tray/devices.rs index d61c7dcf..0d58d418 100644 --- a/src-tauri/src/tray/devices.rs +++ b/src-tauri/src/tray/devices.rs @@ -69,12 +69,16 @@ pub fn resolve_device_id(app: &AppHandle, selected: &DeviceMenuSelection) -> Opt match selected { DeviceMenuSelection::DeviceId(id) => { // Fast path: still in the cached list and transferable. - let cached = caches.devices_slot().lock().as_ref().and_then(|(_, devices)| { - devices - .iter() - .find(|d| d.id.as_deref() == Some(id.as_str())) - .and_then(|d| d.id.clone()) - }); + let cached = caches + .devices_slot() + .lock() + .as_ref() + .and_then(|(_, devices)| { + devices + .iter() + .find(|d| d.id.as_deref() == Some(id.as_str())) + .and_then(|d| d.id.clone()) + }); if cached.is_some() { return cached; } diff --git a/src-tauri/src/tray/mod.rs b/src-tauri/src/tray/mod.rs index d2bcee6f..03fb5632 100644 --- a/src-tauri/src/tray/mod.rs +++ b/src-tauri/src/tray/mod.rs @@ -432,18 +432,19 @@ pub fn handle_menu_event(app: &AppHandle, id: &str) { // nothing is recorded and the item keeps showing the truth. let app_handle = app.clone(); std::thread::spawn(move || { - let (target, repeat) = - if let Some(state) = app_handle.try_state::>() - { - ( - shuffle_toggle_target( - state.caches.shuffle_flag().load(Ordering::Acquire), - ), - last_repeat_state(&state.caches), - ) - } else { - (shuffle_toggle_target(false), crate::spotify::RepeatState::Off) - }; + let (target, repeat) = if let Some(state) = + app_handle.try_state::>() + { + ( + shuffle_toggle_target(state.caches.shuffle_flag().load(Ordering::Acquire)), + last_repeat_state(&state.caches), + ) + } else { + ( + shuffle_toggle_target(false), + crate::spotify::RepeatState::Off, + ) + }; run_player_action( &app_handle, "shuffle", @@ -460,16 +461,16 @@ pub fn handle_menu_event(app: &AppHandle, id: &str) { // record-on-success discipline as Shuffle above. let app_handle = app.clone(); std::thread::spawn(move || { - let (target, shuffle) = - if let Some(state) = app_handle.try_state::>() - { - ( - last_repeat_state(&state.caches).next(), - state.caches.shuffle_flag().load(Ordering::Acquire), - ) - } else { - (crate::spotify::RepeatState::Off.next(), false) - }; + let (target, shuffle) = if let Some(state) = + app_handle.try_state::>() + { + ( + last_repeat_state(&state.caches).next(), + state.caches.shuffle_flag().load(Ordering::Acquire), + ) + } else { + (crate::spotify::RepeatState::Off.next(), false) + }; run_player_action( &app_handle, "repeat", @@ -832,9 +833,10 @@ pub fn setup_tray(app: &tauri::App) -> Result<(), String> { // pause would leave the mark claiming "playing" until the next track. // Consuming the event here keeps the tray's belief truthful; the // poller's own re-store then drives the rebuild that paints it. - app.listen("playback-state-changed", |event| { + let listen_handle = app.handle().clone(); + app.listen("playback-state-changed", move |event| { let payload = event.payload().to_string(); - let app = event.app_handle().clone(); + let app = listen_handle.clone(); std::thread::spawn(move || { let Some(state) = app.try_state::>() else { return; @@ -2153,7 +2155,10 @@ mod tests { // The mirror is the key's source and round-trips. let caches = crate::state::AppCaches::new(); note_window_visibility(&caches, true); - assert!(window_visible(&caches), "the mirror must report what was recorded"); + assert!( + window_visible(&caches), + "the mirror must report what was recorded" + ); note_window_visibility(&caches, false); assert!(!window_visible(&caches)); diff --git a/src-tauri/src/tray/snooze.rs b/src-tauri/src/tray/snooze.rs index d85aedcb..ed94ff37 100644 --- a/src-tauri/src/tray/snooze.rs +++ b/src-tauri/src/tray/snooze.rs @@ -312,7 +312,7 @@ pub fn store_snooze( let mut guard = state.config.get_mut(); let base = match guard.as_ref() { Some(current) => (**current).clone(), - None => crate::config::load_config()?, + None => crate::config::load_config(&state.caches)?, }; let mut next = base; next.snooze_until = until.map(crate::config::snooze_store_form); @@ -373,7 +373,7 @@ pub fn store_active_profile(app: &AppHandle, name: Option) -> Result<(), let mut guard = state.config.get_mut(); let base = match guard.as_ref() { Some(current) => (**current).clone(), - None => crate::config::load_config()?, + None => crate::config::load_config(&state.caches)?, }; let mut next = base; next.active_profile = name; diff --git a/src-tauri/src/updater_bg.rs b/src-tauri/src/updater_bg.rs index 33fea180..a08cc713 100644 --- a/src-tauri/src/updater_bg.rs +++ b/src-tauri/src/updater_bg.rs @@ -1370,20 +1370,30 @@ fn install_method_for( #[cfg(desktop)] #[tauri::command] pub async fn check_for_update(app: AppHandle) -> Result, String> { + use tauri::Manager; + let caches = app + .try_state::>() + .map(|s| std::sync::Arc::clone(&s)); let found = tauri::async_runtime::spawn_blocking(move || { - let channel = crate::config::load_config() - .map_err(|e| { - // A read failure must never be a silent, permanent - // "no update" state: the reason lands in the log file, and the - // next check (mount / 24h tick) re-reads the config, so a - // transient failure restores itself. Parse failures do not - // reach here at all — `load_config` quarantines the file and - // boots on defaults (verified in config.rs). - log::warn!("{TAG} check_for_update: config unreadable ({e}); update check failed"); - format!("config load failed: {e}") - })? - .updates - .channel; + // Issue #758 slice 2: the config read goes through the managed + // `AppState`'s caches; a command invoked before setup managed state + // (or in a test without one) falls back to throwaway caches. + let channel = match caches.as_ref() { + Some(state) => crate::config::load_config(&state.caches), + None => crate::config::load_config(&crate::state::AppCaches::new()), + } + .map_err(|e| { + // A read failure must never be a silent, permanent + // "no update" state: the reason lands in the log file, and the + // next check (mount / 24h tick) re-reads the config, so a + // transient failure restores itself. Parse failures do not + // reach here at all — `load_config` quarantines the file and + // boots on defaults (verified in config.rs). + log::warn!("{TAG} check_for_update: config unreadable ({e}); update check failed"); + format!("config load failed: {e}") + })? + .updates + .channel; tauri::async_runtime::block_on(check_with_channel(&app, channel)) }) .await @@ -1528,11 +1538,18 @@ pub async fn stage_deferred_update( drop(guard); active }; + let state_caches = app + .try_state::>() + .map(|s| std::sync::Arc::clone(&s)); let outcome = tauri::async_runtime::spawn_blocking(move || { tauri::async_runtime::block_on(async move { let current = env!("CARGO_PKG_VERSION").to_string(); - let channel = crate::config::load_config() - .map_err(|e| { + // Issue #758 slice 2: same managed-caches read as check_for_update. + let channel = match state_caches.as_ref() { + Some(state) => crate::config::load_config(&state.caches), + None => crate::config::load_config(&crate::state::AppCaches::new()), + } + .map_err(|e| { // The channel decides which manifest is staged, so an // unreadable config is a visible failure for THIS call — // never a silent guess — with the reason in the log file. From d1e04353cd7d2eed9f496cfd3f18dff12a34ba2c Mon Sep 17 00:00:00 2001 From: Carme99 Date: Thu, 8 Oct 2026 18:14:55 +0100 Subject: [PATCH 3/4] fix(tray): consume playback-state inline + clear dead lock/static refs (#758, slice 2) - listener consumes on emitter thread (spawn raced the rebuild, could paint stale Play/Pause on same-track pause); guard pins inline shape - delete dead MODE_ATOM_LOCK imports + static (zero live uses) - repoint LAST_*_STATE refs to playing/shuffle/repeat_flag accessors --- docs/STATE-OF-FEATURES.md | 4 ++-- docs/architecture/tray-and-shell.md | 4 ++-- src-tauri/src/tray/cache.rs | 1 - src-tauri/src/tray/dedup.rs | 3 +-- src-tauri/src/tray/mod.rs | 36 ++++++++++++++++++----------- src-tauri/src/tray/snooze.rs | 1 - src-tauri/src/tray/testkit.rs | 2 -- 7 files changed, 27 insertions(+), 24 deletions(-) diff --git a/docs/STATE-OF-FEATURES.md b/docs/STATE-OF-FEATURES.md index 0e4fec59..319b3a46 100644 --- a/docs/STATE-OF-FEATURES.md +++ b/docs/STATE-OF-FEATURES.md @@ -34,7 +34,7 @@ end-to-end; the few rows that can't be sourced inline are explicitly flagged | Multi-window detach for Logs/Settings (C7) | ✅ | `src/routes/detached/[pane]/+page.svelte` renders the Logs, Settings, and unknown-pane branches; `src/lib/stores/detach.ts` calls Rust's fixed-table `detach_pane` command, and the main capability no longer grants webview-window creation. Detached labels receive minimal mirrored permissions in `src-tauri/capabilities/detached.json`, including `core:window:allow-close` for Pop back in; a refused close leaves the badge alone. `+layout.svelte` listeners remain window-label-guarded, and the app still boots single-window. | | IPC guard matrix enforced by test (#771) | ✅ | `src-tauri/src/commands/mod.rs` caller-location matrix covers all 54 `generate_handler!` commands with a guarded / main-only-by-caller-location / detached-legit justification each; `test_guard_matrix_covers_every_registered_command` brace-counts the handler list out of `lib.rs` and fails on any registered-without-entry or listed-without-registration drift. `check_for_update` + `cancel_deferred_update` stay main-only by caller location (UpdatePrompt mounts only under `{#if isMainWindow}`); adding a `window` param + guard is the recorded follow-up. | | Deep-link navigate UX (C2) | ✅ | `navigate` event emitted from `handle_deep_link` / Teams auth success (`dashboard` / `settings`); listener in `+page.svelte` defers while Onboarding owns the view. | -| Tray tooltip + native CheckMenuItems + dock badge (C4) | ✅ | `tray/mod.rs` + `tray/dedup.rs` — live tooltip `Artist — Track (▶|⏸)` on each rebuild; Play/Pause, Shuffle and Repeat as native CheckMenuItems driven by `LAST_PLAYING_STATE` / `LAST_SHUFFLE_STATE` / `LAST_REPEAT_STATE`; macOS-only (`#[cfg]`) presence-gated dock badge wired into the polling loop (`b82f515`). The menu also carries a **Pause / Resume Sync** item (IDs `ID_PAUSE_SYNC` / `ID_RESUME_SYNC`) and, since 4.7.0 (#677), a **Pause sync for…** snooze submenu plus **Resume sync now** while a snooze is active. The tray's own status line is `sync_status_line` ("Syncing — Artist — Track", "Paused — …", "Not syncing", "Syncing — nothing playing"), and the tooltip is `{status_line} · {track_tooltip}`; the words come from the `i18n::Strings` table and a snooze replaces the line with `snooze_status_line` (remaining time + local deadline). | +| Tray tooltip + native CheckMenuItems + dock badge (C4) | ✅ | `tray/mod.rs` + `tray/dedup.rs` — live tooltip `Artist — Track (▶|⏸)` on each rebuild; Play/Pause, Shuffle and Repeat as native CheckMenuItems driven by `playing_flag` / `shuffle_flag` / `repeat_flag`; macOS-only (`#[cfg]`) presence-gated dock badge wired into the polling loop (`b82f515`). The menu also carries a **Pause / Resume Sync** item (IDs `ID_PAUSE_SYNC` / `ID_RESUME_SYNC`) and, since 4.7.0 (#677), a **Pause sync for…** snooze submenu plus **Resume sync now** while a snooze is active. The tray's own status line is `sync_status_line` ("Syncing — Artist — Track", "Paused — …", "Not syncing", "Syncing — nothing playing"), and the tooltip is `{status_line} · {track_tooltip}`; the words come from the `i18n::Strings` table and a snooze replaces the line with `snooze_status_line` (remaining time + local deadline). | | Settings dirty-state + clamp feedback + reset (C9) | ✅ | `Settings.svelte` — unsaved-changes banner via BigInt-safe deep compare; per-section Reset-to-default buttons using `defaultConfig`. Clamp feedback lives in the cards (`PollingCard` min>max + pause-backoff hints, `StatusFormatCard` lexicon hint, `RulesCard` replacement-length hints) styled by the shared `.clamp-hint` rule in `src/app.css`. | | Settings per-card split (#750) | ✅ | `src/lib/components/settings/` holds `SettingsCard` shell + all thirteen cards (`RulesCard` with `$bindable()` slices + `onreset`/`onchange` and a header `actions` Undo; `PresenceCard`/`PollingCard` with `$bindable()` slice + `onreset`; `LoggingCard`/`UpdatesCard`/`ShortcutsCard` with `$bindable()` slice and no `onreset`; `StatusFormatCard` with `$bindable()` slice + `onreset` + lexicon/preview callbacks; `SpotifyCard`/`TeamsCard` with connection props + reconnect callbacks and a header `actions` badge; `ProfilesCard` with `$bindable()` slices + `saveMessage` + `onchange`; `AppearanceCard` with `$bindable()` slices + `onreset` + `onAutostartError`; `NotificationsCard`/`BackupCard` store- or callback-driven with no `$bindable()` slice); `Settings.svelte` keeps draft state, save/discard, `pendingNav`, footer. | | Config module split, one concern per file (#755) | ✅ | `src-tauri/src/config/mod.rs` re-exports `schema`/`clamp`/`snooze`/`patch`/`migrate`/`io`/`transfer` (one concern per file, every `crate::config::X` path stable); all 121 config tests remain centralized in `mod.rs` (identical set) — the slices carry no `#[test]`; `redact.rs` aggregates the 8 slice sources through one `concat!`; `LoggingConfig` lives only in `schema.rs`; `cargo check --all-targets` plus `cargo test --lib` (config 121/121, full 910/910) plus `clippy -D warnings` plus `fmt --check` all clean. | @@ -83,7 +83,7 @@ end-to-end; the few rows that can't be sourced inline are explicitly flagged | Maintained path/rand crates | ✅ | `dirs` → `directories` 6, `rand` 0.9 with `try_fill_bytes` propagation; single keyring feature set (#418). | | Status template placeholders ({device}/{playlist}/{progress}/{shuffle}/{repeat}) | ✅ | `src-tauri/src/spotify.rs::format_status_with_context` — one 13-token table (`placeholder_values`) filled per render and substituted in a single left-to-right pass (`substitute_placeholders`), so a value that arrived from Spotify is never re-scanned and re-expanded (#341; `format_status_does_not_expand_data_inserted_tokens`, #580). `{context}` is a literal alias of `{playlist}`; `{shuffle}`/`{repeat}` are icon-only (🔀/🔁 or `""`); `{progress}` is `M:SS` with no hour rollover (`90:00` for a 90-minute episode) and **empty** when `progress_ms` is `None` (#165). UI hint keys `settings.placeholdersHint` / `onboarding.placeholdersHint` list the music tokens. | | Podcast & audiobook episodes | ✅ | `src-tauri/src/spotify.rs::map_media_item` reads the item's own `type` against the documented `oneOf(track, episode)` union: an episode's `show.name` takes the `artist` slot and `show.publisher` the `album` slot, and `EpisodeInfo` carries `show_name`/`publisher`. `poll_once::process_track` selects `DEFAULT_EPISODE_STATUS_FORMAT` (`🎙️ {show} - {episode}`) whenever `now.episode.is_some()`, so a music template is never applied to an episode (#581). Adverts are unchanged: the envelope's `currently_playing_type` is gated and `Ad | Unknown` returns `None` from the mapper — "nothing playing" (#161). The queue mapper feeds episodes into tray *Up Next* and drops ads (#583). Note: the episode template is a built-in constant — there is **no** `teams.episode_status_format` config key. | -| Tray Shuffle / Repeat toggles | ✅ | `src-tauri/src/tray/dedup.rs` — both are native CheckMenuItems whose marks read `LAST_SHUFFLE_STATE` / `LAST_REPEAT_STATE`, written by `note_playback_modes` from the poll body (no extra request) and optimistically on a successful tray toggle before `force_tray_refresh`. Shuffle targets the inverse of the last known state; Repeat targets `RepeatState::next()`, i.e. `off → context → track → off`, and its label spells the mode out because a check mark cannot distinguish the last two (#582). A rejected command records nothing, so the tray never claims a state Spotify refused; the error surfaces on `playback-error` as an in-app toast. | +| Tray Shuffle / Repeat toggles | ✅ | `src-tauri/src/tray/dedup.rs` — both are native CheckMenuItems whose marks read `shuffle_flag` / `repeat_flag`, written by `note_playback_modes` from the poll body (no extra request) and optimistically on a successful tray toggle before `force_tray_refresh`. Shuffle targets the inverse of the last known state; Repeat targets `RepeatState::next()`, i.e. `off → context → track → off`, and its label spells the mode out because a check mark cannot distinguish the last two (#582). A rejected command records nothing, so the tray never claims a state Spotify refused; the error surfaces on `playback-error` as an in-app toast. | | Live stage progress + cancel for install-on-quit | ✅ | `updater_bg.rs` binds progress, completion, and cancellation to the exact stage request id and generation. Cancel invalidates an in-flight completion, removes an already-committed payload, or tombstones cancellation that arrives before staging begins; a losing download cannot stage or install, and queued terminal events are suppressed. | | Keychain-unavailable surface (locked vs absent) | ✅ | `keychain.rs::KeychainPresence` preserves `Present` / `Absent` / `Unavailable(help)`. Config loads stamp both derived fields through `with_keychain_flags` and the warm presence cache; a fresh `Present` observation is reused for 30 seconds, while a cold or expired cache falls back to the direct namespaced/legacy probe. Explicit user-action checks remain uncached. | | Log viewer on-disk backfill | ✅ | `src-tauri/src/commands/logs.rs::get_recent_logs` reads the tail on `spawn_blocking`, clamped twice — `MAX_LOG_LINES` 500 and `LOG_TAIL_MAX_BYTES` 256 KiB — dropping the partial first line; a missing file is `Ok(empty)`, not an error. `LogViewer.svelte` registers `log://log` first and then awaits the seed, prepending it and re-clamping to `MAX_BUFFER`. The seed is **raw, not redacted** by explicit decision (#595): redaction belongs to the paste-able snapshot path. **Clear** sets `seedCancelled` so history cannot reappear. | diff --git a/docs/architecture/tray-and-shell.md b/docs/architecture/tray-and-shell.md index 5c4a494e..3d119e90 100644 --- a/docs/architecture/tray-and-shell.md +++ b/docs/architecture/tray-and-shell.md @@ -198,8 +198,8 @@ matrix builds **aarch64 macOS only** — Intel Macs never receive updates `tray/mod.rs` builds the menu natively, from in-process state: - **Shuffle / Repeat are real toggles (#582).** Both are - `CheckMenuItemBuilder` items; Shuffle's mark reads `LAST_SHUFFLE_STATE` and - Repeat's reads `LAST_REPEAT_STATE` plus a mode-spelling label + `CheckMenuItemBuilder` items; Shuffle's mark reads `shuffle_flag` and + Repeat's reads `repeat_flag` plus a mode-spelling label (`Repeat: Off` / `Repeat: Context` / `Repeat: Track` — a check mark alone cannot tell the last two apart). Those atoms are written by `note_playback_modes` from the poll body itself (no extra request, no new scope) and optimistically by a diff --git a/src-tauri/src/tray/cache.rs b/src-tauri/src/tray/cache.rs index f43984c6..413bc95f 100644 --- a/src-tauri/src/tray/cache.rs +++ b/src-tauri/src/tray/cache.rs @@ -267,7 +267,6 @@ pub fn queue_for_menu( mod tests { use super::*; use crate::i18n::{DE, EN, FR}; - use crate::tray::testkit::MODE_ATOM_LOCK; /// Issue #582: a check mark can only carry on/off, while `repeat_state` /// has three documented values — so the label must name the mode, or a /// user cannot tell "repeat one" from "repeat the playlist". Issue #674: diff --git a/src-tauri/src/tray/dedup.rs b/src-tauri/src/tray/dedup.rs index e29e3d5a..89ce8f65 100644 --- a/src-tauri/src/tray/dedup.rs +++ b/src-tauri/src/tray/dedup.rs @@ -201,7 +201,7 @@ pub fn repeat_menu_label(strings: &Strings, state: RepeatState) -> &'static str /// One-line sync/status summary for the tray's status item and tooltip /// (issue #591). The Pause/Resume verb on its own left the sync state /// unstated, and the presence-gated dock badge is macOS-only. `is_playing` -/// is `LAST_PLAYING_STATE` — the same source as the Play/Pause checkmark — +/// is `playing_flag` — the same source as the Play/Pause checkmark — /// not the polling loop's copy, which goes stale on a same-track pause. /// /// Localized from the table it is handed (issue #674); the artist/title and @@ -227,7 +227,6 @@ pub fn sync_status_line( mod tests { use super::*; use crate::i18n::{DE, EN, FR}; - use crate::tray::testkit::MODE_ATOM_LOCK; /// Issue #582: the two toggles render the state the poll body reported /// (`note_playback_modes` → the atoms the menu build reads), and the /// click target is the documented cycle, so a successful toggle leaves diff --git a/src-tauri/src/tray/mod.rs b/src-tauri/src/tray/mod.rs index 03fb5632..83c02999 100644 --- a/src-tauri/src/tray/mod.rs +++ b/src-tauri/src/tray/mod.rs @@ -833,16 +833,18 @@ pub fn setup_tray(app: &tauri::App) -> Result<(), String> { // pause would leave the mark claiming "playing" until the next track. // Consuming the event here keeps the tray's belief truthful; the // poller's own re-store then drives the rebuild that paints it. + // The consume is inline (not spawned): tauri invokes `listen` callbacks + // synchronously on the emitter thread, so `note_playing_state` lands + // before `process_track` returns and before the rebuild that paints it. + // A spawn would race the rebuild and could paint a stale Play/Pause + // mark on a same-track pause (#758 slice-2 review). `try_state` is a + // lock-free map lookup, so there is no blocking cost to staying inline. let listen_handle = app.handle().clone(); app.listen("playback-state-changed", move |event| { - let payload = event.payload().to_string(); - let app = listen_handle.clone(); - std::thread::spawn(move || { - let Some(state) = app.try_state::>() else { - return; - }; - consume_playback_state_changed(&state.caches, &payload); - }); + let Some(state) = listen_handle.try_state::>() else { + return; + }; + consume_playback_state_changed(&state.caches, event.payload()); }); // Immediately set the real menu to reflect actual state (Bug 11 fix). @@ -1134,12 +1136,12 @@ pub(crate) fn rebuild_tray_menu( })?; // Spotify playback controls (issue #3.0-P3). Play/Pause is a single - // check-item whose native checked state comes from LAST_PLAYING_STATE - // (see the static's docs — the polling loop's stored track goes stale - // on a same-track pause); the Devices/Up Next submenus are built from + // check-item whose native checked state comes from `playing_flag` + // (the polling loop's stored track goes stale on a same-track pause); + // the Devices/Up Next submenus are built from // the pre-fetched throttled caches so the polling loop's per-iteration // rebuilds don't hammer the Spotify API. The checkmark is derived from - // (track_id, is_playing) via the track_key dedup and LAST_PLAYING_STATE, + // (track_id, is_playing) via the track_key dedup and `playing_flag`, // so a same-track pause flips without waiting for the next poll. See // issues #229 and #217. let is_playing = caches.playing_flag().load(Ordering::Acquire); @@ -1982,9 +1984,15 @@ mod tests { body.contains("app.listen(\"playback-state-changed\""), "setup_tray must subscribe to the poller's playback-state-changed event (issue #689)" ); + // The consume is inline on the listener thread (not spawned): a spawn + // would race the rebuild and could paint a stale Play/Pause mark. + assert!( + body.contains("consume_playback_state_changed(&state.caches, event.payload())"), + "the subscription must hand the payload to the tray's consumer inline" + ); assert!( - body.contains("consume_playback_state_changed(&state.caches, &payload)"), - "the subscription must hand the payload to the tray's consumer" + !body.contains("std::thread::spawn"), + "the playback-state consume must not hop threads (issue #689 ordering)" ); } /// Issue #882: `setup_tray` runs on the main thread inside Tauri's diff --git a/src-tauri/src/tray/snooze.rs b/src-tauri/src/tray/snooze.rs index ed94ff37..e6d288be 100644 --- a/src-tauri/src/tray/snooze.rs +++ b/src-tauri/src/tray/snooze.rs @@ -446,7 +446,6 @@ pub fn snooze_status_line(strings: &Strings, snooze: &TraySnooze) -> String { mod tests { use super::*; use crate::i18n::{DE, EN, FR}; - use crate::tray::testkit::MODE_ATOM_LOCK; use crate::tray::testkit::{body_of, tray_prod_source}; /// A config carrying `snooze_until` as stored. fn snoozed_config(stored: &str) -> std::sync::Arc { diff --git a/src-tauri/src/tray/testkit.rs b/src-tauri/src/tray/testkit.rs index cc07c15b..1821c3b2 100644 --- a/src-tauri/src/tray/testkit.rs +++ b/src-tauri/src/tray/testkit.rs @@ -1,8 +1,6 @@ //! tray/testkit.rs — shared helpers for source-scan tests (#756). #![allow(dead_code)] use std::sync::OnceLock; -#[cfg(test)] -pub static MODE_ATOM_LOCK: parking_lot::Mutex<()> = parking_lot::Mutex::new(()); static TRAY_PROD: OnceLock = OnceLock::new(); /// Production half of `src` — everything before the inline test module, /// so a scan can never match the assertions themselves. From 0a5ca127f0731ea4d0b8518ed421f03ed24c5908 Mon Sep 17 00:00:00 2001 From: Carme99 Date: Thu, 8 Oct 2026 18:22:04 +0100 Subject: [PATCH 4/4] docs(tray,config): restore two comments truncated in slice-2 move (#758) --- src-tauri/src/cli.rs | 2 ++ src-tauri/src/diagnostics.rs | 1 + 2 files changed, 3 insertions(+) diff --git a/src-tauri/src/cli.rs b/src-tauri/src/cli.rs index 215445dc..7e660a6d 100644 --- a/src-tauri/src/cli.rs +++ b/src-tauri/src/cli.rs @@ -530,6 +530,8 @@ pub(crate) fn cli_sync_once_preflight( Ok(()) } +/// Load the files [`cli_sync_once_preflight`] decides on, turning a load +/// failure into the reason the CLI prints. pub(crate) fn cli_sync_once_preflight_from_disk() -> Result<(), String> { let caches = crate::state::AppCaches::new(); let config = diff --git a/src-tauri/src/diagnostics.rs b/src-tauri/src/diagnostics.rs index a02e9b85..f4d2cd83 100644 --- a/src-tauri/src/diagnostics.rs +++ b/src-tauri/src/diagnostics.rs @@ -1275,6 +1275,7 @@ pub async fn get_diagnostics_snapshot(app: AppHandle) -> Result