From 8500da1a42610970ad305fca01435187a6a838a9 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sun, 16 Aug 2026 23:05:13 +0200 Subject: [PATCH] chore(rust): clear the clippy backlog and finish the poison-tolerant lock sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cargo clippy --all-targets` went from 51 warnings (23 in the lib) to zero. Most were mechanical — needless borrows, `assert_eq!` against a bool literal, `vec!` where an array does, `or_insert_with(Vec::new)`, a loop index used only to index — and were applied with `clippy --fix`, then reviewed line by line. That review caught one auto-fix that was *not* semantically neutral: dropping the redundant `use hostname;` left its `#[cfg(target_os = "linux")]` orphaned directly above `SERVICE_NAME`, which would have silently cfg'd the constant out of every non-Linux build. Removed the stray attribute with the import. Where a lint asked for a risky change rather than a better one, it is suppressed with a comment saying why: - `too_many_arguments` on five `#[tauri::command]` handlers and `ThumbnailCache::save_thumbnail` — most of the arity is `State<'_, _>` injection, and a parameter struct would change the IPC contract and the generated TypeScript for no readability gain. - `large_enum_variant` on `PlayerStatusEvent` and `AutoplayDecision` — both are serde + specta wire types emitted a handful of times a second, never bulk allocated; boxing would have to stay invisible to the generated bindings while every match arm gained a deref. - `await_holding_lock` on the `hybrid`/`offline` test modules — the guard is a test-only serialisation lock for the process-global `INCLUDE_CATALOG_BROWSE` flag, and the await it spans *is* the critical section. Each `#[tokio::test]` gets its own single-threaded runtime, so this is not the production deadlock class the lint targets; restructuring would reintroduce the flag race. Real fixes elsewhere: `JellyfinItem::to_media_item` takes `self` by value, so it is now `into_media_item`; the five-tuple episode row in the download commands has a named `EpisodeRow` alias; the mpv `PropertyChange` arm matches `name: "pause"` instead of guarding on it. Also converted the last 27 raw `.lock().unwrap()` call sites to `lock_safe()`, completing the `MutexSafe`/`RwLockSafe` convention. All of them turned out to be in test modules — production code was already clean — so this is consistency rather than a fix. The two raw locks in `utils/lock.rs` stay raw on purpose: those tests deliberately poison a mutex to prove the helpers recover from it. Pure refactoring: all 698 tests still pass. --- src-tauri/src/commands/auth.rs | 4 +- src-tauri/src/commands/catalog.rs | 17 +++---- src-tauri/src/commands/connectivity.rs | 2 +- src-tauri/src/commands/download/mod.rs | 18 ++++++-- src-tauri/src/commands/favorites.rs | 3 +- src-tauri/src/commands/playback_reporting.rs | 2 +- src-tauri/src/commands/player/mod.rs | 25 +++++++---- src-tauri/src/commands/repository.rs | 5 ++- src-tauri/src/commands/sessions.rs | 2 +- src-tauri/src/commands/storage/mod.rs | 6 +-- src-tauri/src/commands/sync_drain.rs | 5 ++- src-tauri/src/credentials.rs | 10 +---- src-tauri/src/domain/search_rank.rs | 11 ++--- src-tauri/src/download/cache.rs | 22 ++++++---- src-tauri/src/lib.rs | 4 +- src-tauri/src/playback_mode/mod.rs | 20 ++++----- src-tauri/src/player/autoplay.rs | 6 +++ src-tauri/src/player/events.rs | 6 +++ src-tauri/src/player/mpv_backend.rs | 2 +- src-tauri/src/player/mpv_backend_test.rs | 27 ++++++------ src-tauri/src/player/queue.rs | 3 +- src-tauri/src/player/state.rs | 3 ++ src-tauri/src/player/webview_audio_backend.rs | 6 +-- src-tauri/src/repository/hybrid.rs | 10 +++++ src-tauri/src/repository/offline.rs | 15 +++++-- src-tauri/src/repository/online.rs | 44 +++++++++---------- src-tauri/src/thumbnail/cache.rs | 4 ++ 27 files changed, 173 insertions(+), 109 deletions(-) diff --git a/src-tauri/src/commands/auth.rs b/src-tauri/src/commands/auth.rs index cba5215e..90cd2627 100644 --- a/src-tauri/src/commands/auth.rs +++ b/src-tauri/src/commands/auth.rs @@ -418,13 +418,13 @@ mod tests { #[test] fn test_auth_manager_wrapper_structure() { // Verify wrapper type exists and has correct structure - assert_eq!(std::mem::size_of::() > 0, true); + assert!(std::mem::size_of::() > 0); } #[test] fn test_session_verifier_wrapper_structure() { // Verify wrapper type exists and has correct structure - assert_eq!(std::mem::size_of::() > 0, true); + assert!(std::mem::size_of::() > 0); } #[test] diff --git a/src-tauri/src/commands/catalog.rs b/src-tauri/src/commands/catalog.rs index 5027cbf6..182cdb6b 100644 --- a/src-tauri/src/commands/catalog.rs +++ b/src-tauri/src/commands/catalog.rs @@ -496,7 +496,7 @@ pub(crate) async fn requeue_mistyped_video_downloads( .collect::>() .join(", "); - let query = Query::new(&format!( + let query = Query::new(format!( "UPDATE downloads SET status = 'pending', stream_url = NULL, progress = 0, bytes_downloaded = 0, started_at = NULL, completed_at = NULL @@ -563,7 +563,7 @@ where ), None => String::new(), }; - let rows_query = Query::new(&format!( + let rows_query = Query::new(format!( "SELECT d.id, d.item_id, COALESCE( d.media_type, @@ -753,6 +753,7 @@ pub async fn resume_queued_downloads( mod tests { use super::*; use crate::storage::db_service::RusqliteService; + use crate::utils::lock::MutexSafe; use rusqlite::Connection; use std::sync::Mutex; @@ -1012,14 +1013,14 @@ mod tests { resolve_pending_download_urls(&db, "/data", None, move |item_id, media_type, _q| { let seen = Arc::clone(&seen_c); async move { - seen.lock().unwrap().push((item_id.clone(), media_type)); + seen.lock_safe().push((item_id.clone(), media_type)); Some(format!("http://resolved/{item_id}")) } }) .await .unwrap(); - let seen = seen.lock().unwrap().clone(); + let seen = seen.lock_safe().clone(); let of = |id: &str| { seen.iter() .find(|(i, _)| i == id) @@ -1045,14 +1046,14 @@ mod tests { resolve_pending_download_urls(&db, "/data", None, move |_id, media_type, _q| { let seen = Arc::clone(&seen_c); async move { - *seen.lock().unwrap() = media_type; + *seen.lock_safe() = media_type; Some("http://x".to_string()) } }) .await .unwrap(); - assert_eq!(*seen.lock().unwrap(), "audio"); + assert_eq!(*seen.lock_safe(), "audio"); } /// An explicit `media_type` on the row always wins over the item's type. @@ -1069,14 +1070,14 @@ mod tests { resolve_pending_download_urls(&db, "/data", None, move |_id, media_type, _q| { let seen = Arc::clone(&seen_c); async move { - *seen.lock().unwrap() = media_type; + *seen.lock_safe() = media_type; Some("http://x".to_string()) } }) .await .unwrap(); - assert_eq!(*seen.lock().unwrap(), "video"); + assert_eq!(*seen.lock_safe(), "video"); } /// Rows already downloaded under the audio default hold an audio-only diff --git a/src-tauri/src/commands/connectivity.rs b/src-tauri/src/commands/connectivity.rs index ee20dad1..50ed7552 100644 --- a/src-tauri/src/commands/connectivity.rs +++ b/src-tauri/src/commands/connectivity.rs @@ -97,6 +97,6 @@ mod tests { // due to its dependencies, so we just test the wrapper type structure // This verifies the wrapper type exists and can hold Arc - assert_eq!(std::mem::size_of::() > 0, true); + assert!(std::mem::size_of::() > 0); } } diff --git a/src-tauri/src/commands/download/mod.rs b/src-tauri/src/commands/download/mod.rs index 3f9d0158..0cd61748 100644 --- a/src-tauri/src/commands/download/mod.rs +++ b/src-tauri/src/commands/download/mod.rs @@ -19,6 +19,10 @@ mod smart_cache; pub use pinning::*; pub use smart_cache::*; +/// One row of the series episode listing used when queueing a whole series: +/// `(id, name, season_name, index_number, parent_index_number)`. +type EpisodeRow = (String, String, Option, Option, Option); + /// Wrapper for DownloadManager to be used as Tauri state pub struct DownloadManagerWrapper(pub Mutex); @@ -596,6 +600,10 @@ pub(crate) async fn queue_album_tracks( /// TRACES: UR-018, UR-055 | DR-173 | UT-170 #[tauri::command] #[specta::specta] +// Three of the eight arguments are Tauri `State<'_, _>` injections plus the +// `AppHandle`, not caller input. Folding the rest into a struct would change the +// IPC contract and the generated TypeScript for no readability gain. +#[allow(clippy::too_many_arguments)] pub async fn download_album( db: State<'_, DatabaseWrapper>, repository: State<'_, crate::commands::repository::RepositoryManagerWrapper>, @@ -807,7 +815,7 @@ pub async fn download_series( vec![QueryParam::String(series_id)], ); - let episodes: Vec<(String, String, Option, Option, Option)> = db_service + let episodes: Vec = db_service .query_many(episodes_query, |row| { Ok(( row.get(0)?, @@ -912,6 +920,10 @@ pub async fn download_series( /// Queue all episodes of a specific season for download #[tauri::command] #[specta::specta] +// One of the eight arguments is a Tauri `State<'_, _>` injection; the rest are +// the season's identifying fields. Folding them into a struct would change the +// IPC contract and the generated TypeScript for no readability gain. +#[allow(clippy::too_many_arguments)] pub async fn download_season( db: State<'_, DatabaseWrapper>, season_id: String, @@ -2307,7 +2319,7 @@ pub async fn delete_downloads_under( )"; let file_query = Query::with_params( - &format!("SELECT d.file_path FROM downloads d WHERE {SCOPE}"), + format!("SELECT d.file_path FROM downloads d WHERE {SCOPE}"), vec![ QueryParam::String(user_id.clone()), QueryParam::String(item_id.clone()), @@ -2323,7 +2335,7 @@ pub async fn delete_downloads_under( .map_err(|e| e.to_string())?; let delete_query = Query::with_params( - &format!("DELETE FROM downloads WHERE id IN (SELECT d.id FROM downloads d WHERE {SCOPE})"), + format!("DELETE FROM downloads WHERE id IN (SELECT d.id FROM downloads d WHERE {SCOPE})"), vec![ QueryParam::String(user_id), QueryParam::String(item_id.clone()), diff --git a/src-tauri/src/commands/favorites.rs b/src-tauri/src/commands/favorites.rs index eaeba618..5ec3044a 100644 --- a/src-tauri/src/commands/favorites.rs +++ b/src-tauri/src/commands/favorites.rs @@ -186,6 +186,7 @@ async fn run_drain(app: &tauri::AppHandle) -> Result<(), String> { #[cfg(test)] mod tests { use super::*; + use crate::utils::lock::MutexSafe; use rusqlite::Connection; use std::sync::Mutex; @@ -211,7 +212,7 @@ mod tests { } fn calls(&self) -> Vec<(String, bool)> { - let mut calls = self.calls.lock().unwrap().clone(); + let mut calls = self.calls.lock_safe().clone(); calls.sort(); calls } diff --git a/src-tauri/src/commands/playback_reporting.rs b/src-tauri/src/commands/playback_reporting.rs index 22698ea0..8cd45777 100644 --- a/src-tauri/src/commands/playback_reporting.rs +++ b/src-tauri/src/commands/playback_reporting.rs @@ -360,7 +360,7 @@ mod tests { #[test] fn test_playback_reporter_wrapper_structure() { // Verify wrapper type can hold Arc>> - assert_eq!(std::mem::size_of::() > 0, true); + assert!(std::mem::size_of::() > 0); } #[test] diff --git a/src-tauri/src/commands/player/mod.rs b/src-tauri/src/commands/player/mod.rs index 7cb301e9..bff42238 100644 --- a/src-tauri/src/commands/player/mod.rs +++ b/src-tauri/src/commands/player/mod.rs @@ -1480,6 +1480,10 @@ pub async fn player_seek_video( /// Note: Frontend should handle saving series preferences after this command succeeds #[tauri::command] #[specta::specta] +// Two of the eight arguments are Tauri `State<'_, _>` injections, not caller +// input. Folding the rest into a struct would change the IPC contract and the +// generated TypeScript for no readability gain. +#[allow(clippy::too_many_arguments)] pub async fn player_switch_audio_track( player: State<'_, PlayerStateWrapper>, repository_manager: State<'_, super::repository::RepositoryManagerWrapper>, @@ -1559,6 +1563,10 @@ pub async fn player_switch_audio_track( /// TRACES: UR-074 | DR-162 #[tauri::command] #[specta::specta] +// Three of the nine arguments are Tauri `State<'_, _>` injections, not caller +// input. Folding the rest into a struct would change the IPC contract and the +// generated TypeScript for no readability gain. +#[allow(clippy::too_many_arguments)] pub async fn player_set_stream_quality( player: State<'_, PlayerStateWrapper>, repository_manager: State<'_, super::repository::RepositoryManagerWrapper>, @@ -1784,7 +1792,7 @@ pub async fn player_get_status( let local_media = { let queue_arc = controller.queue(); let queue = queue_arc.lock().map_err(|e| e.to_string())?; - queue.current().map(|item| MergedMediaItem::from(item)) + queue.current().map(MergedMediaItem::from) }; let local_is_playing = status.state.is_playing(); @@ -1808,10 +1816,7 @@ pub async fn player_get_status( log::info!("[PlayerCommands] Merging remote session state"); // Merge media item - status.merged_media = session - .now_playing_item - .as_ref() - .map(|item| MergedMediaItem::from(item)); + status.merged_media = session.now_playing_item.as_ref().map(MergedMediaItem::from); // Merge isPlaying (NOT isPaused!) status.merged_is_playing = session @@ -2741,6 +2746,8 @@ pub async fn player_disable_jellyfin(player: State<'_, PlayerStateWrapper>) -> R #[cfg(test)] mod tests { + use crate::utils::lock::MutexSafe; + /// The subtitle list the frontend resolved must survive the IPC hop and end /// up on the `MediaItem` the native backend loads. /// @@ -3063,7 +3070,7 @@ mod tests { let database = Database::open_in_memory().unwrap(); { let conn = database.connection(); - let conn = conn.lock().unwrap(); + let conn = conn.lock_safe(); conn.execute_batch(&format!( r#" INSERT INTO servers (id, name, url) VALUES ('srv', 'Test', 'http://test'); @@ -3119,7 +3126,7 @@ mod tests { assert_eq!(switched, 1, "only the download whose file exists switches"); let queue = controller.queue(); - let queue_lock = queue.lock().unwrap(); + let queue_lock = queue.lock_safe(); match &queue_lock.items()[0].source { MediaSource::Local { file_path, @@ -3148,7 +3155,7 @@ mod tests { index_number: Option, } - let mut tracks = vec![ + let mut tracks = [ MockTrack { id: "track1".to_string(), name: "Song 1".to_string(), @@ -3241,7 +3248,7 @@ mod tests { } // Create tracks in random order (not sorted) - let mut tracks = vec![ + let mut tracks = [ MockTrack { id: "id5".to_string(), name: "Track 5".to_string(), diff --git a/src-tauri/src/commands/repository.rs b/src-tauri/src/commands/repository.rs index 26d5f67c..f09ec944 100644 --- a/src-tauri/src/commands/repository.rs +++ b/src-tauri/src/commands/repository.rs @@ -67,6 +67,10 @@ pub struct RepositoryManagerWrapper(pub RepositoryManager); /// Returns a handle (UUID) for accessing the repository #[tauri::command] #[specta::specta] +// Four of the eight arguments are Tauri `State<'_, _>` injections, not caller +// input. Folding the remaining four into a struct would change the IPC contract +// and the generated TypeScript for no readability gain. +#[allow(clippy::too_many_arguments)] pub async fn repository_create( manager: State<'_, RepositoryManagerWrapper>, player: State<'_, crate::commands::player::PlayerStateWrapper>, @@ -1091,7 +1095,6 @@ mod tests { let handle = format!("{}", uuid); // UUID should convert to a non-empty string assert!(!handle.is_empty()); - assert!(handle.len() > 0); } #[test] diff --git a/src-tauri/src/commands/sessions.rs b/src-tauri/src/commands/sessions.rs index 8e41a554..c7b16c47 100644 --- a/src-tauri/src/commands/sessions.rs +++ b/src-tauri/src/commands/sessions.rs @@ -89,7 +89,7 @@ mod tests { #[test] fn test_session_poller_wrapper_structure() { // Test that wrapper type structure is correct - assert_eq!(std::mem::size_of::() > 0, true); + assert!(std::mem::size_of::() > 0); } #[test] diff --git a/src-tauri/src/commands/storage/mod.rs b/src-tauri/src/commands/storage/mod.rs index 06a4af27..e5f231c1 100644 --- a/src-tauri/src/commands/storage/mod.rs +++ b/src-tauri/src/commands/storage/mod.rs @@ -1658,19 +1658,19 @@ mod tests { #[test] fn test_database_wrapper_structure() { // Verify DatabaseWrapper can be created and holds Mutex - assert_eq!(std::mem::size_of::() > 0, true); + assert!(std::mem::size_of::() > 0); } #[test] fn test_credential_store_wrapper_structure() { // Verify CredentialStoreWrapper can be created - assert_eq!(std::mem::size_of::() > 0, true); + assert!(std::mem::size_of::() > 0); } #[test] fn test_thumbnail_cache_wrapper_structure() { // Verify ThumbnailCacheWrapper holds Arc - assert_eq!(std::mem::size_of::() > 0, true); + assert!(std::mem::size_of::() > 0); } #[test] diff --git a/src-tauri/src/commands/sync_drain.rs b/src-tauri/src/commands/sync_drain.rs index 354b72dd..089d1754 100644 --- a/src-tauri/src/commands/sync_drain.rs +++ b/src-tauri/src/commands/sync_drain.rs @@ -481,6 +481,7 @@ pub async fn sync_process_pending(app: tauri::AppHandle) -> Result Vec { - self.calls.lock().unwrap().clone() + self.calls.lock_safe().clone() } } @@ -527,7 +528,7 @@ mod tests { if let Some(err) = &self.fail_with { return Err(err.clone()); } - self.calls.lock().unwrap().push(op.clone()); + self.calls.lock_safe().push(op.clone()); Ok(()) } } diff --git a/src-tauri/src/credentials.rs b/src-tauri/src/credentials.rs index 38585776..7bdd86f4 100644 --- a/src-tauri/src/credentials.rs +++ b/src-tauri/src/credentials.rs @@ -20,9 +20,6 @@ use sha2::{Digest, Sha256}; use std::fs; use std::path::PathBuf; -#[cfg(target_os = "linux")] -use hostname; - #[cfg(not(target_os = "android"))] const SERVICE_NAME: &str = "com.dtourolle.jellytau"; @@ -203,15 +200,12 @@ impl CredentialStore { // secret-tool doesn't support --version, so we test with a search command // that will succeed even if no items are found - match Command::new("secret-tool") + Command::new("secret-tool") .arg("search") .arg("service") .arg("__nonexistent_test__") .output() - { - Ok(_) => true, // If command runs (even with no results), secret-tool is available - Err(_) => false, // Command not found or can't execute - } + .is_ok() } #[cfg(all(not(target_os = "android"), not(target_os = "linux")))] diff --git a/src-tauri/src/domain/search_rank.rs b/src-tauri/src/domain/search_rank.rs index 80f7fc7b..a8e22f57 100644 --- a/src-tauri/src/domain/search_rank.rs +++ b/src-tauri/src/domain/search_rank.rs @@ -119,11 +119,12 @@ mod tests { use super::*; fn item(name: &str, kind: MediaKind) -> MediaItem { - let mut item = MediaItem::default(); - item.id = format!("id-{}-{:?}", name, kind); - item.name = name.to_string(); - item.kind = kind; - item + MediaItem { + id: format!("id-{}-{:?}", name, kind), + name: name.to_string(), + kind, + ..MediaItem::default() + } } fn names(items: &[MediaItem]) -> Vec<&str> { diff --git a/src-tauri/src/download/cache.rs b/src-tauri/src/download/cache.rs index 905344f1..9366447b 100644 --- a/src-tauri/src/download/cache.rs +++ b/src-tauri/src/download/cache.rs @@ -389,14 +389,18 @@ mod tests { #[test] fn test_queue_precache_config() { - let mut config = CacheConfig::default(); - config.queue_precache_enabled = false; + let config = CacheConfig { + queue_precache_enabled: false, + ..CacheConfig::default() + }; let cache = SmartCache::new(config); assert!(!cache.should_precache_queue()); - let mut new_config = CacheConfig::default(); - new_config.wifi_only = false; + let new_config = CacheConfig { + wifi_only: false, + ..CacheConfig::default() + }; cache.update_config(new_config); assert!(cache.should_precache_queue()); @@ -407,9 +411,11 @@ mod tests { // wifi_only must not short-circuit precaching: the network gate lives in // the download pump, which checks the *actual* transport. Enabling // WiFi-only while on WiFi should still precache. - let mut config = CacheConfig::default(); - config.queue_precache_enabled = true; - config.wifi_only = true; + let config = CacheConfig { + queue_precache_enabled: true, + wifi_only: true, + ..CacheConfig::default() + }; let cache = SmartCache::new(config); assert!(cache.should_precache_queue()); @@ -422,7 +428,7 @@ mod tests { /// TRACES: UR-071 | DR-127 | UT-120 #[tokio::test] async fn test_reclaim_expired_only_takes_expired_temporary_entries() { - use crate::storage::db_service::{DatabaseService, RusqliteService}; + use crate::storage::db_service::RusqliteService; use rusqlite::Connection; use std::sync::{Arc, Mutex}; diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 85b35c15..2fe53660 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -620,7 +620,7 @@ fn create_player_backend( match MpvBackend::new(Some(_event_emitter), playback_reporter, position_throttler) { Ok(backend) => { info!("Successfully initialized MPV backend for Linux"); - return Box::new(backend); + Box::new(backend) } Err(e) => { error!("\n========================================"); @@ -645,7 +645,7 @@ fn create_player_backend( // still browse the library and manage downloads, and the frontend // can show a "playback unavailable" notice via this event. emit_backend_init_failed(&app_handle, "mpv", e.to_string()); - return Box::new(NullBackend::new()); + Box::new(NullBackend::new()) } } } diff --git a/src-tauri/src/playback_mode/mod.rs b/src-tauri/src/playback_mode/mod.rs index 4fa91960..fbc5cd9f 100644 --- a/src-tauri/src/playback_mode/mod.rs +++ b/src-tauri/src/playback_mode/mod.rs @@ -624,7 +624,7 @@ impl PlaybackModeManager { ); // Log first few track IDs for debugging - if queue_ids.len() > 0 { + if !queue_ids.is_empty() { let preview: Vec<&str> = queue_ids.iter().take(3).map(|s| s.as_str()).collect(); debug!("[PlaybackMode] First track IDs: {:?}...", preview); } @@ -914,7 +914,7 @@ mod tests { impl PlayerEventEmitter for CapturingEmitter { fn emit(&self, event: PlayerStatusEvent) { - self.events.lock().unwrap().push(event); + self.events.lock_safe().push(event); } } @@ -942,7 +942,7 @@ mod tests { manager.set_mode(PlaybackMode::Local); manager.set_mode(PlaybackMode::Idle); - let events = emitter.events.lock().unwrap(); + let events = emitter.events.lock_safe(); assert_eq!(events.len(), 3, "one event per real mode change"); match &events[0] { @@ -975,10 +975,10 @@ mod tests { impl RemoteVolumeControl for RecordingVolumeControl { fn enable(&self, _initial_volume: i32) { - self.calls.lock().unwrap().push("enable"); + self.calls.lock_safe().push("enable"); } fn disable(&self) { - self.calls.lock().unwrap().push("disable"); + self.calls.lock_safe().push("disable"); } } @@ -1014,7 +1014,7 @@ mod tests { manager.set_mode(PlaybackMode::Idle); assert_eq!( - *volume.calls.lock().unwrap(), + *volume.calls.lock_safe(), vec!["enable", "disable"], "remote->idle must return volume control to the local speaker" ); @@ -1033,7 +1033,7 @@ mod tests { manager.set_mode(PlaybackMode::Local); assert_eq!( - *volume.calls.lock().unwrap(), + *volume.calls.lock_safe(), vec!["enable", "disable"], "remote->local must return volume control to the local speaker" ); @@ -1053,7 +1053,7 @@ mod tests { manager.set_mode(PlaybackMode::Local); assert!( - volume.calls.lock().unwrap().is_empty(), + volume.calls.lock_safe().is_empty(), "local/idle transitions must not touch remote volume routing" ); } @@ -1074,7 +1074,7 @@ mod tests { }); assert_eq!( - *volume.calls.lock().unwrap(), + *volume.calls.lock_safe(), vec!["enable", "enable"], "remote->remote re-arms control without releasing it to local" ); @@ -1091,7 +1091,7 @@ mod tests { manager.set_mode(PlaybackMode::Local); assert_eq!( - emitter.events.lock().unwrap().len(), + emitter.events.lock_safe().len(), 1, "repeated identical mode set emits only once" ); diff --git a/src-tauri/src/player/autoplay.rs b/src-tauri/src/player/autoplay.rs index aa59a8d0..04f93630 100644 --- a/src-tauri/src/player/autoplay.rs +++ b/src-tauri/src/player/autoplay.rs @@ -6,6 +6,12 @@ use serde::{Deserialize, Serialize}; /// Autoplay decision result - determines what happens after playback ends #[derive(specta::Type, Debug, Clone, Serialize)] #[serde(tag = "action", rename_all = "camelCase")] +// `ShowNextEpisodePopup` carries two `MediaItem`s, so it dwarfs the unit +// variants. Boxing them is not worth it here: this enum is constructed once per +// end-of-item (never in a hot loop or a large collection), and it is an IPC type +// — the indirection would have to stay invisible to serde/specta while every +// match arm gained a deref, for no measurable gain. +#[allow(clippy::large_enum_variant)] pub enum AutoplayDecision { /// Stop playback (no next item or timer expired) Stop, diff --git a/src-tauri/src/player/events.rs b/src-tauri/src/player/events.rs index 16eb182a..46105128 100644 --- a/src-tauri/src/player/events.rs +++ b/src-tauri/src/player/events.rs @@ -30,6 +30,12 @@ use super::{MediaSessionType, SleepTimerMode}; // queue_changed never reach the frontend, so the mini player never appears). // Keep serde and specta agreeing: snake_case fields, snake_case variant tags. #[serde(tag = "type", rename_all = "snake_case")] +// `ShowNextEpisodePopup` carries two `MediaItem`s, so it dwarfs the small +// position/state variants. Boxing them is rejected deliberately: this is a +// serde + specta wire type whose generated TypeScript must not shift, and the +// events are emitted a few times a second at most — never bulk-allocated — so +// the size difference costs nothing measurable. +#[allow(clippy::large_enum_variant)] pub enum PlayerStatusEvent { /// Playback position updated (emitted periodically during playback) PositionUpdate { diff --git a/src-tauri/src/player/mpv_backend.rs b/src-tauri/src/player/mpv_backend.rs index 78414fee..f0d068d4 100644 --- a/src-tauri/src/player/mpv_backend.rs +++ b/src-tauri/src/player/mpv_backend.rs @@ -243,7 +243,7 @@ impl MpvBackend { }); } } - libmpv::events::Event::PropertyChange { name, .. } if name == "pause" => { + libmpv::events::Event::PropertyChange { name: "pause", .. } => { // Handle pause state changes if let Ok(is_paused) = mpv.get_property::("pause") { let media_id = state diff --git a/src-tauri/src/player/mpv_backend_test.rs b/src-tauri/src/player/mpv_backend_test.rs index 6fd01c67..6ea3ff9c 100644 --- a/src-tauri/src/player/mpv_backend_test.rs +++ b/src-tauri/src/player/mpv_backend_test.rs @@ -1,14 +1,15 @@ -/// Tests for MpvBackend to prevent regressions -/// -/// These tests are designed to catch common issues like: -/// - Tokio runtime panics when spawning async tasks from std::thread -/// - Position update thread failures -/// - Event emission issues -/// -/// TRACES: UR-003, UR-004 | IR-003 | IT-003, IT-004 +//! Tests for MpvBackend to prevent regressions +//! +//! These tests are designed to catch common issues like: +//! - Tokio runtime panics when spawning async tasks from std::thread +//! - Position update thread failures +//! - Event emission issues +//! +//! TRACES: UR-003, UR-004 | IR-003 | IT-003, IT-004 #[cfg(test)] mod tests { + use crate::utils::lock::MutexSafe; use std::sync::{Arc, Mutex}; use tokio::sync::Mutex as TokioMutex; @@ -67,14 +68,14 @@ mod tests { if let Ok(handle) = tokio::runtime::Handle::try_current() { // Has runtime (shouldn't happen in this test) handle.spawn(async move { - *counter_clone.lock().unwrap() += 1; + *counter_clone.lock_safe() += 1; }); } else { // No runtime - use fallback (should happen in this test) std::thread::spawn(move || { let rt = tokio::runtime::Runtime::new().unwrap(); rt.block_on(async move { - *counter_clone.lock().unwrap() += 1; + *counter_clone.lock_safe() += 1; }); }); } @@ -85,7 +86,7 @@ mod tests { // Wait for async task to complete std::thread::sleep(std::time::Duration::from_millis(100)); - let count = *counter.lock().unwrap(); + let count = *counter.lock_safe(); assert_eq!( count, 1, "Fallback pattern should execute async code successfully" @@ -109,13 +110,13 @@ mod tests { let position = i as f64 * 0.25; // Store position (simulating event emission) - positions_clone.lock().unwrap().push(position); + positions_clone.lock_safe().push(position); } }); handle.join().unwrap(); - let recorded_positions = positions.lock().unwrap(); + let recorded_positions = positions.lock_safe(); assert_eq!( recorded_positions.len(), 5, diff --git a/src-tauri/src/player/queue.rs b/src-tauri/src/player/queue.rs index 335c847f..6cfcfdb1 100644 --- a/src-tauri/src/player/queue.rs +++ b/src-tauri/src/player/queue.rs @@ -806,11 +806,10 @@ mod tests { assert_eq!(queue.current_index(), Some(first_shuffled_index)); // Move through shuffle order - for i in 1..shuffle_order.len() { + for &expected_index in &shuffle_order[1..] { assert!(queue.has_next()); let result = queue.next(); assert!(result.is_some()); - let expected_index = shuffle_order[i]; assert_eq!(queue.current_index(), Some(expected_index)); } diff --git a/src-tauri/src/player/state.rs b/src-tauri/src/player/state.rs index 9eb61a0c..0096a46c 100644 --- a/src-tauri/src/player/state.rs +++ b/src-tauri/src/player/state.rs @@ -159,6 +159,9 @@ mod tests { #[test] fn test_end_reason_clone() { let reason = EndReason::Finished; + // Deliberately exercising the derived `Clone` impl, not a plain copy: + // `EndReason` is also `Copy`, so clippy flags the call as redundant. + #[allow(clippy::clone_on_copy)] let cloned = reason.clone(); assert_eq!(reason, cloned); } diff --git a/src-tauri/src/player/webview_audio_backend.rs b/src-tauri/src/player/webview_audio_backend.rs index edf11a12..1f0db40c 100644 --- a/src-tauri/src/player/webview_audio_backend.rs +++ b/src-tauri/src/player/webview_audio_backend.rs @@ -196,7 +196,7 @@ mod tests { impl PlayerEventEmitter for RecordingEmitter { fn emit(&self, event: PlayerStatusEvent) { - self.events.lock().unwrap().push(event); + self.events.lock_safe().push(event); } } @@ -244,7 +244,7 @@ mod tests { let (mut b, events) = backend(); b.load(&test_media()).unwrap(); - let ev = events.lock().unwrap(); + let ev = events.lock_safe(); let load = ev .iter() .find(|e| matches!(e, PlayerStatusEvent::WebviewAudioLoad { .. })) @@ -263,7 +263,7 @@ mod tests { b.pause().unwrap(); b.seek(42.0).unwrap(); - let ev = events.lock().unwrap(); + let ev = events.lock_safe(); assert!(ev.iter().any(|e| matches!( e, PlayerStatusEvent::ControlCommand { action, .. } if action == "pause" diff --git a/src-tauri/src/repository/hybrid.rs b/src-tauri/src/repository/hybrid.rs index 659e0eb1..240aeab1 100644 --- a/src-tauri/src/repository/hybrid.rs +++ b/src-tauri/src/repository/hybrid.rs @@ -1134,6 +1134,16 @@ impl MediaRepository for HybridRepository { #[cfg(test)] mod tests { + // `GATE_TEST_LOCK` below serialises the tests that flip the process-global + // `INCLUDE_CATALOG_BROWSE` flag, so its guard is deliberately held across + // the `.await` of the repository call under test — that await *is* the + // critical section. This is not the production deadlock hazard the lint + // targets: the lock is test-only, uncontended outside these tests, and each + // `#[tokio::test]` runs on its own single-threaded runtime, so a held guard + // cannot block another task on the same worker. Restructuring around it + // would reintroduce the flag race the lock exists to prevent. + #![allow(clippy::await_holding_lock)] + use super::*; use std::sync::Mutex; diff --git a/src-tauri/src/repository/offline.rs b/src-tauri/src/repository/offline.rs index 232d0d6e..9c24725c 100644 --- a/src-tauri/src/repository/offline.rs +++ b/src-tauri/src/repository/offline.rs @@ -2510,6 +2510,16 @@ impl MediaRepository for OfflineRepository { #[cfg(test)] mod tests { + // `CATALOG_BROWSE_LOCK` below serialises the tests that flip the + // process-global `INCLUDE_CATALOG_BROWSE` flag, so its guard is deliberately + // held across the `.await` of the query under test — that await *is* the + // critical section. This is not the production deadlock hazard the lint + // targets: the lock is test-only, uncontended outside these tests, and each + // `#[tokio::test]` runs on its own single-threaded runtime, so a held guard + // cannot block another task on the same worker. Restructuring around it + // would reintroduce the flag race the lock exists to prevent. + #![allow(clippy::await_holding_lock)] + use super::*; use crate::storage::db_service::RusqliteService; use rusqlite::Connection; @@ -2524,9 +2534,8 @@ mod tests { static CATALOG_BROWSE_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); fn lock_catalog_browse() -> std::sync::MutexGuard<'static, ()> { - CATALOG_BROWSE_LOCK - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) + use crate::utils::lock::MutexSafe; + CATALOG_BROWSE_LOCK.lock_safe() } /// TRACES: UR-065 | DR-108 | UT-111 diff --git a/src-tauri/src/repository/online.rs b/src-tauri/src/repository/online.rs index 963ae0e1..4657c4ea 100644 --- a/src-tauri/src/repository/online.rs +++ b/src-tauri/src/repository/online.rs @@ -984,7 +984,7 @@ struct JellyfinMediaSource { } impl JellyfinItem { - fn to_media_item(self, server_id: String) -> MediaItem { + fn into_media_item(self, server_id: String) -> MediaItem { // Extract image tags before consuming self let primary_tag = self.image_tags.as_ref().and_then(|tags| tags.primary()); let backdrop_tags = self.backdrop_image_tags; @@ -1123,7 +1123,7 @@ impl MediaRepository for OnlineRepository { items: response .items .into_iter() - .map(|item| item.to_media_item(self.user_id.clone())) + .map(|item| item.into_media_item(self.user_id.clone())) .collect(), total_record_count: response.total_record_count, }) @@ -1133,7 +1133,7 @@ impl MediaRepository for OnlineRepository { let endpoint = format!("/Users/{}/Items/{}?Fields=BackdropImageTags,ParentBackdropImageTags,People,MediaStreams,MediaSources,PremiereDate,UserData", self.user_id, item_id); let item: JellyfinItem = self.get_json(&endpoint).await?; - let media_item = item.to_media_item(self.user_id.clone()); + let media_item = item.into_media_item(self.user_id.clone()); Ok(media_item) } @@ -1148,7 +1148,7 @@ impl MediaRepository for OnlineRepository { let items: Vec = self.get_json(&endpoint).await?; Ok(items .into_iter() - .map(|item| item.to_media_item(self.user_id.clone())) + .map(|item| item.into_media_item(self.user_id.clone())) .collect()) } @@ -1171,7 +1171,7 @@ impl MediaRepository for OnlineRepository { Ok(response .items .into_iter() - .map(|item| item.to_media_item(self.user_id.clone())) + .map(|item| item.into_media_item(self.user_id.clone())) .collect()) } @@ -1186,7 +1186,7 @@ impl MediaRepository for OnlineRepository { Ok(response .items .into_iter() - .map(|item| item.to_media_item(self.user_id.clone())) + .map(|item| item.into_media_item(self.user_id.clone())) .collect()) } @@ -1206,7 +1206,7 @@ impl MediaRepository for OnlineRepository { let items: Vec = response .items .into_iter() - .map(|item| item.to_media_item(self.user_id.clone())) + .map(|item| item.into_media_item(self.user_id.clone())) .collect(); debug!("[get_recently_played_audio] Fetched {} items", items.len()); @@ -1229,7 +1229,7 @@ impl MediaRepository for OnlineRepository { "[get_recently_played_audio] Grouping item '{}' into album '{}'", item.name, key ); - album_map.entry(key).or_insert_with(Vec::new).push(item); + album_map.entry(key).or_default().push(item); } else { debug!( "[get_recently_played_audio] No album_id or album_name for item: '{}'", @@ -1344,7 +1344,7 @@ impl MediaRepository for OnlineRepository { Ok(response .items .into_iter() - .map(|item| item.to_media_item(self.user_id.clone())) + .map(|item| item.into_media_item(self.user_id.clone())) .collect()) } @@ -1359,7 +1359,7 @@ impl MediaRepository for OnlineRepository { Ok(response .items .into_iter() - .map(|item| item.to_media_item(self.user_id.clone())) + .map(|item| item.into_media_item(self.user_id.clone())) .collect()) } @@ -1461,7 +1461,7 @@ impl MediaRepository for OnlineRepository { items: response .items .into_iter() - .map(|item| item.to_media_item(self.user_id.clone())) + .map(|item| item.into_media_item(self.user_id.clone())) .collect(), total_record_count: response.total_record_count, }) @@ -1839,7 +1839,7 @@ impl MediaRepository for OnlineRepository { Ok(response .items .into_iter() - .map(|item| item.to_media_item(self.server_url.clone())) + .map(|item| item.into_media_item(self.server_url.clone())) .collect()) } @@ -1852,7 +1852,7 @@ impl MediaRepository for OnlineRepository { let items = response .items .into_iter() - .map(|item| item.to_media_item(self.server_url.clone())) + .map(|item| item.into_media_item(self.server_url.clone())) .collect(); Ok(SearchResult { items, @@ -2189,7 +2189,7 @@ impl MediaRepository for OnlineRepository { items: response .items .into_iter() - .map(|item| item.to_media_item(self.user_id.clone())) + .map(|item| item.into_media_item(self.user_id.clone())) .collect(), total_record_count: response.total_record_count, }) @@ -2319,7 +2319,7 @@ impl MediaRepository for OnlineRepository { async fn get_person(&self, person_id: &str) -> Result { let endpoint = format!("/Users/{}/Items/{}", self.user_id, person_id); let item: JellyfinItem = self.get_json(&endpoint).await?; - Ok(item.to_media_item(self.user_id.clone())) + Ok(item.into_media_item(self.user_id.clone())) } async fn get_items_by_person( @@ -2349,7 +2349,7 @@ impl MediaRepository for OnlineRepository { items: response .items .into_iter() - .map(|item| item.to_media_item(self.user_id.clone())) + .map(|item| item.into_media_item(self.user_id.clone())) .collect(), total_record_count: response.total_record_count, }) @@ -2373,7 +2373,7 @@ impl MediaRepository for OnlineRepository { items: response .items .into_iter() - .map(|item| item.to_media_item(self.user_id.clone())) + .map(|item| item.into_media_item(self.user_id.clone())) .collect(), total_record_count: response.total_record_count, }) @@ -2461,7 +2461,7 @@ impl MediaRepository for OnlineRepository { .into_iter() .map(|pi| PlaylistEntry { playlist_item_id: pi.playlist_item_id, - item: pi.item.to_media_item(self.user_id.clone()), + item: pi.item.into_media_item(self.user_id.clone()), }) .collect()) } @@ -2959,7 +2959,7 @@ mod tests { })) .expect("fixture must deserialize"); - let streams = item.to_media_item("server-1".to_string()).media_streams; + let streams = item.into_media_item("server-1".to_string()).media_streams; let streams = streams.expect("the item carries streams"); let deliverable = |index: i32| { streams @@ -3554,7 +3554,7 @@ mod tests { }"#; let item: JellyfinItem = serde_json::from_str(json).expect("item should deserialize"); - let media = item.to_media_item("server1".to_string()); + let media = item.into_media_item("server1".to_string()); let user_data = media.user_data.expect("user data should be mapped"); assert_eq!(user_data.is_favorite, Some(true)); @@ -3574,7 +3574,7 @@ mod tests { let json = r#"{"Id": "x", "Name": "No User Data", "Type": "Movie"}"#; let item: JellyfinItem = serde_json::from_str(json).expect("item should deserialize"); - let media = item.to_media_item("server1".to_string()); + let media = item.into_media_item("server1".to_string()); assert!(media.user_data.is_none()); } @@ -3618,7 +3618,7 @@ mod tests { }"#; let jellyfin_item: JellyfinItem = serde_json::from_str(json).expect("Failed to parse"); - let media_item = jellyfin_item.to_media_item("test-server-id".to_string()); + let media_item = jellyfin_item.into_media_item("test-server-id".to_string()); assert_eq!(media_item.id, "album456"); assert_eq!(media_item.name, "Love and Theft"); diff --git a/src-tauri/src/thumbnail/cache.rs b/src-tauri/src/thumbnail/cache.rs index f3738e6d..a91b320e 100644 --- a/src-tauri/src/thumbnail/cache.rs +++ b/src-tauri/src/thumbnail/cache.rs @@ -155,6 +155,10 @@ impl ThumbnailCache { } /// Save thumbnail to cache + // The arguments are the cache key (item/type/tag) plus the payload and its + // dimensions — all independent scalars borrowed from the caller. A parameter + // struct would only move the same list one level down. + #[allow(clippy::too_many_arguments)] pub async fn save_thumbnail( &self, db: Arc,