diff --git a/src-tauri/src/commands/catalog.rs b/src-tauri/src/commands/catalog.rs index 2da398b8..679302bb 100644 --- a/src-tauri/src/commands/catalog.rs +++ b/src-tauri/src/commands/catalog.rs @@ -159,6 +159,20 @@ pub async fn catalog_sync_status( Ok(CatalogSyncStatus { last_synced_at }) } +/// Control whether offline library queries reveal the full synced catalog +/// (greyed-out, non-downloaded media) or only downloaded/local media. +/// +/// The frontend calls this from the "Show all server media" toggle: pass `true` +/// when online, or when offline with the toggle on; pass `false` when offline +/// with the toggle off so library pages show downloaded media only. Fixes the +/// bug where offline library pages showed every server item regardless of the +/// toggle. +#[tauri::command] +#[specta::specta] +pub fn set_show_server_catalog(show: bool) { + crate::repository::offline::set_include_catalog_browse(show); +} + #[derive(specta::Type, Debug, Clone, serde::Serialize, serde::Deserialize)] #[serde(rename_all = "camelCase")] pub struct ResumeQueuedResult { diff --git a/src-tauri/src/commands/player/mod.rs b/src-tauri/src/commands/player/mod.rs index 8de87256..d635eeea 100644 --- a/src-tauri/src/commands/player/mod.rs +++ b/src-tauri/src/commands/player/mod.rs @@ -696,6 +696,11 @@ pub async fn player_stop( let controller = player.0.lock().await; controller.stop().map_err(|e| e.to_string())?; + // A genuine local stop returns the manager to Idle so it no longer + // reports Local (or a stale Remote) — otherwise a later play/pause would + // route to the wrong device. + playback_mode.0.set_mode(crate::playback_mode::PlaybackMode::Idle); + // Handle session state based on type (local playback only) { let mut session_mgr = session.0.lock().map_err(|e| e.to_string())?; @@ -1538,6 +1543,9 @@ pub async fn player_play_album_track( play_selection_on_remote(&controller, session_id, &media_items, start_index).await?; controller.set_queue(media_items, start_index).map_err(|e| e.to_string())?; } else { + // Local playback is now authoritative (see player_play_tracks); set it + // before starting so the mode-changed event precedes the state events. + playback_mode.0.set_mode(crate::playback_mode::PlaybackMode::Local); controller .play_queue(media_items, start_index) .map_err(|e| e.to_string())?; @@ -1714,6 +1722,16 @@ pub async fn player_play_tracks( .set_queue(media_items, request.start_index) .map_err(|e| e.to_string())?; } else { + // Starting local playback makes Local the authoritative mode. Without + // this, a prior Remote mode lingers in the manager and later play/pause + // commands route back to the (stopped) remote session. Set it BEFORE + // starting playback so the PlaybackModeChanged event reaches the frontend + // ahead of the state_changed events it will emit — otherwise the frontend + // (still thinking it's remote) filters those state events out. Skip during + // a transfer: transfer_to_local drives the mode itself once complete. + if !playback_mode.0.is_transferring() { + playback_mode.0.set_mode(crate::playback_mode::PlaybackMode::Local); + } controller .play_queue_from(media_items, request.start_index, request.start_position) .map_err(|e| e.to_string())?; diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 0043bd4b..2ac8f6c7 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -23,7 +23,7 @@ use log::{error, info}; use log::warn; use commands::{ - sync_full_catalog, catalog_sync_status, resume_queued_downloads, + sync_full_catalog, catalog_sync_status, set_show_server_catalog, resume_queued_downloads, cancel_download, clear_stale_downloads, delete_album_downloads, delete_all_downloads, delete_download, download_album, download_item, download_item_and_start, download_video, download_series, download_season, get_download_storage_stats, get_downloads, get_download_manager_stats, set_max_concurrent_downloads, @@ -588,6 +588,7 @@ fn specta_builder() -> Builder { enqueue_video_downloads, sync_full_catalog, catalog_sync_status, + set_show_server_catalog, resume_queued_downloads, get_download_manager_stats, set_max_concurrent_downloads, @@ -924,6 +925,9 @@ pub fn run() { player_arc.clone(), ); let playback_mode_arc = Arc::new(playback_mode_manager); + // Broadcast mode changes so the frontend's mirror store reconciles to + // this authoritative one (prevents remote/local control desync). + playback_mode_arc.set_event_emitter(event_emitter.clone()); let playback_mode_wrapper = PlaybackModeManagerWrapper(playback_mode_arc.clone()); app.manage(playback_mode_wrapper); diff --git a/src-tauri/src/playback_mode/mod.rs b/src-tauri/src/playback_mode/mod.rs index ec778723..10f51cc2 100644 --- a/src-tauri/src/playback_mode/mod.rs +++ b/src-tauri/src/playback_mode/mod.rs @@ -9,7 +9,7 @@ use tokio::sync::Mutex as TokioMutex; use tokio::time::{sleep, Duration}; use crate::jellyfin::JellyfinClient; -use crate::player::{PlayerController, QueueContext}; +use crate::player::{PlayerController, PlayerEventEmitter, PlayerStatusEvent, QueueContext}; /// Playback mode - local device, remote session, or idle #[derive(specta::Type, Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] @@ -48,6 +48,9 @@ pub struct PlaybackModeManager { player_controller: Arc>, current_mode: Arc>, is_transferring: Arc, + /// Optional emitter used to notify the frontend when the mode changes, so its + /// mirror store stays in sync with this authoritative one. `None` in tests. + event_emitter: Arc>>>, } impl PlaybackModeManager { @@ -61,19 +64,53 @@ impl PlaybackModeManager { player_controller, current_mode: Arc::new(RwLock::new(PlaybackMode::Idle)), is_transferring: Arc::new(AtomicBool::new(false)), + event_emitter: Arc::new(Mutex::new(None)), } } + /// Wire the event emitter so `set_mode` notifies the frontend. Called once + /// during setup; safe to leave unset (tests do), in which case mode changes + /// simply aren't broadcast. + pub fn set_event_emitter(&self, emitter: Arc) { + *self.event_emitter.lock_safe() = Some(emitter); + } + /// Get current playback mode pub fn get_mode(&self) -> PlaybackMode { self.current_mode.read_safe().clone() } - /// Set playback mode (internal use) + /// Set playback mode (internal use). + /// + /// Broadcasts a `PlaybackModeChanged` event when the mode actually changes so + /// the frontend's mirror store reconciles to this authoritative value. The + /// write lock is released before emitting to avoid holding it across the + /// emitter call. pub fn set_mode(&self, mode: PlaybackMode) { log::info!("[PlaybackMode] Setting mode to: {:?}", mode); - let mut current = self.current_mode.write_safe(); - *current = mode; + let changed = { + let mut current = self.current_mode.write_safe(); + let changed = *current != mode; + *current = mode.clone(); + changed + }; + + if !changed { + return; + } + + let (mode_str, session_id) = match &mode { + PlaybackMode::Local => ("local".to_string(), None), + PlaybackMode::Idle => ("idle".to_string(), None), + PlaybackMode::Remote { session_id } => ("remote".to_string(), Some(session_id.clone())), + }; + + if let Some(emitter) = self.event_emitter.lock_safe().as_ref() { + emitter.emit(PlayerStatusEvent::PlaybackModeChanged { + mode: mode_str, + session_id, + }); + } } /// Check if currently transferring @@ -702,6 +739,84 @@ mod tests { ); } + /// Capturing emitter so we can assert what `set_mode` broadcasts. + struct CapturingEmitter { + events: Mutex>, + } + + impl PlayerEventEmitter for CapturingEmitter { + fn emit(&self, event: PlayerStatusEvent) { + self.events.lock().unwrap().push(event); + } + } + + fn manager_with_emitter() -> (PlaybackModeManager, Arc) { + let emitter = Arc::new(CapturingEmitter { + events: Mutex::new(Vec::new()), + }); + let manager = PlaybackModeManager::new( + Arc::new(Mutex::new(None)), + Arc::new(TokioMutex::new(crate::player::PlayerController::default())), + ); + manager.set_event_emitter(emitter.clone()); + (manager, emitter) + } + + /// set_mode broadcasts a PlaybackModeChanged event with the right payload so + /// the frontend can reconcile its mirror store to this authoritative one. + #[test] + fn test_set_mode_emits_change_event() { + let (manager, emitter) = manager_with_emitter(); + + manager.set_mode(PlaybackMode::Remote { + session_id: "sess-1".to_string(), + }); + manager.set_mode(PlaybackMode::Local); + manager.set_mode(PlaybackMode::Idle); + + let events = emitter.events.lock().unwrap(); + assert_eq!(events.len(), 3, "one event per real mode change"); + + match &events[0] { + PlayerStatusEvent::PlaybackModeChanged { mode, session_id } => { + assert_eq!(mode, "remote"); + assert_eq!(session_id.as_deref(), Some("sess-1")); + } + other => panic!("expected PlaybackModeChanged, got {:?}", other), + } + match &events[1] { + PlayerStatusEvent::PlaybackModeChanged { mode, session_id } => { + assert_eq!(mode, "local"); + assert_eq!(session_id.as_deref(), None); + } + other => panic!("expected PlaybackModeChanged, got {:?}", other), + } + match &events[2] { + PlayerStatusEvent::PlaybackModeChanged { mode, session_id } => { + assert_eq!(mode, "idle"); + assert_eq!(session_id.as_deref(), None); + } + other => panic!("expected PlaybackModeChanged, got {:?}", other), + } + } + + /// Setting the same mode twice must not re-emit — the frontend reconciler + /// (and the event channel) shouldn't be spammed on no-op transitions. + #[test] + fn test_set_mode_deduplicates_no_op() { + let (manager, emitter) = manager_with_emitter(); + + manager.set_mode(PlaybackMode::Local); + manager.set_mode(PlaybackMode::Local); + manager.set_mode(PlaybackMode::Local); + + assert_eq!( + emitter.events.lock().unwrap().len(), + 1, + "repeated identical mode set emits only once" + ); + } + /// The resume position handed to a remote session is derived from a live /// playback position. Guards the seconds->ticks conversion and the /// at-the-start threshold (Bug: casting restarted the track from 0). diff --git a/src-tauri/src/player/events.rs b/src-tauri/src/player/events.rs index 55b1817c..c877fad4 100644 --- a/src-tauri/src/player/events.rs +++ b/src-tauri/src/player/events.rs @@ -124,6 +124,21 @@ pub enum PlayerStatusEvent { /// All active controllable sessions from Jellyfin sessions: Vec, }, + /// The authoritative playback mode changed in the Rust backend. + /// + /// The Rust `PlaybackModeManager` is the single source of truth for which + /// device playback commands route to (local vs a remote session). The + /// frontend keeps a mirror store for the UI; without this event that mirror + /// drifts out of sync (e.g. a mode transition happens inside a transfer or a + /// local stop that the frontend never learns about), and controls then route + /// to the wrong device — the classic "it keeps playing on the remote" bug. + /// The frontend reconciles its store to this payload whenever it fires. + PlaybackModeChanged { + /// New mode: "local", "remote", or "idle". + mode: String, + /// Session id when `mode == "remote"`, otherwise `None`. + session_id: Option, + }, /// The user asked to disconnect from the remote session and resume locally. /// /// Emitted when the lockscreen Stop button is pressed while casting. The diff --git a/src-tauri/src/repository/offline.rs b/src-tauri/src/repository/offline.rs index 88e11d4d..4bf5d58f 100644 --- a/src-tauri/src/repository/offline.rs +++ b/src-tauri/src/repository/offline.rs @@ -1,5 +1,6 @@ // Offline repository - queries SQLite database for cached data use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; use async_trait::async_trait; use log::debug; @@ -7,6 +8,30 @@ use log::debug; use super::{MediaRepository, types::*}; use crate::storage::db_service::{DatabaseService, Query, QueryParam, RusqliteService}; +/// Whether offline library queries may include catalog items that are merely +/// *browsed/synced* but not downloaded (the greyed-out "browse the whole server" +/// view). Defaults to `true` so online browsing (which reads this same cache as +/// a fast path) still sees the full catalog. +/// +/// While offline, the frontend drives this from the "Show all server media" +/// toggle: OFF means library pages show only downloaded/local media, ON reveals +/// the full greyed-out catalog. See `set_include_catalog_browse` and the +/// `showServerCatalog` UI flag. Fixes the bug where offline library pages showed +/// every server item regardless of the toggle. +static INCLUDE_CATALOG_BROWSE: AtomicBool = AtomicBool::new(true); + +/// Set whether offline `get_items` includes non-downloaded (synced-only) catalog +/// items. Called from the frontend: `true` when online or when the offline +/// "Show all server media" toggle is on; `false` when offline with the toggle +/// off (show downloaded/local media only). +pub fn set_include_catalog_browse(include: bool) { + INCLUDE_CATALOG_BROWSE.store(include, Ordering::Relaxed); +} + +fn include_catalog_browse() -> bool { + INCLUDE_CATALOG_BROWSE.load(Ordering::Relaxed) +} + pub struct OfflineRepository { db_service: Arc, server_id: String, @@ -558,7 +583,20 @@ impl MediaRepository for OfflineRepository { // Use CTE to find items that are either: // 1. Playable items (Audio, Movie, Episode) with completed downloads (offline mode) // 2. Container items (MusicAlbum, Series, Season) with at least one downloaded child (offline mode) - // 3. Cached items with recent synced_at timestamp (online mode - for fast browsing) + // 3. Cached items with recent synced_at timestamp (fast online browsing, or the + // offline "Show all server media" catalog view) — only when the catalog-browse + // flag is set. When offline with the toggle off, this branch is omitted so the + // page shows downloaded/local media only. See `set_include_catalog_browse`. + let catalog_branch = if include_catalog_browse() { + "UNION + + -- Cached items for fast browsing (online) or the offline catalog view + SELECT DISTINCT i.id + FROM items i + WHERE i.synced_at IS NOT NULL" + } else { + "" + }; let sql = format!( "WITH available_items AS ( -- Playable items with completed downloads @@ -578,12 +616,7 @@ impl MediaRepository for OfflineRepository { WHERE d.status = 'completed' AND i.item_type IN ('MusicAlbum', 'Series', 'Season', 'BoxSet', 'Folder', 'CollectionFolder') - UNION - - -- Cached items for fast browsing (when online) - SELECT DISTINCT i.id - FROM items i - WHERE i.synced_at IS NOT NULL + {catalog_branch} ) SELECT i.id, i.name, i.item_type, i.server_id, i.parent_id, i.library_id, i.overview, i.genres, i.runtime_ticks, i.production_year, i.community_rating, i.official_rating, @@ -1874,6 +1907,57 @@ mod tests { assert_eq!(tracks.items[0].id, "track-1"); } + /// Regression: offline library pages must honor the "Show all server media" + /// toggle. With `include_catalog_browse` off, `get_items` returns only + /// downloaded media — not the whole synced catalog. With it on, the full + /// (synced-but-not-downloaded) catalog is revealed. Fixes the bug where + /// offline library pages showed every server item regardless of the toggle. + #[tokio::test] + async fn test_get_items_toggle_gates_synced_catalog() { + use crate::storage::db_service::DatabaseService; + let db_service = create_test_db(); + + for sql in [ + // Two movies in a library, both merely synced (browsed) — no download. + "INSERT INTO items (id, server_id, name, item_type, library_id, synced_at) \ + VALUES ('movie-dl', 'test-server', 'Downloaded', 'Movie', 'lib-1', '2026-01-01')", + "INSERT INTO items (id, server_id, name, item_type, library_id, synced_at) \ + VALUES ('movie-cat', 'test-server', 'CatalogOnly', 'Movie', 'lib-1', '2026-01-01')", + // Only the first movie is actually downloaded. + "INSERT INTO downloads (item_id, status) VALUES ('movie-dl', 'completed')", + // A library row so the library-parent EXISTS clause matches. + "INSERT INTO libraries (id, server_id, name) VALUES ('lib-1', 'test-server', 'Movies')", + ] { + db_service.execute(Query::new(sql)).await.unwrap(); + } + + let repo = OfflineRepository::new( + db_service.clone(), + "test-server".to_string(), + "test-user".to_string(), + ); + let opts = Some(GetItemsOptions { + include_item_types: Some(vec!["Movie".to_string()]), + ..Default::default() + }); + + // Toggle OFF: only the downloaded movie is returned. + set_include_catalog_browse(false); + let local_only = repo.get_items("lib-1", opts.clone()).await.unwrap(); + let ids: Vec<&str> = local_only.items.iter().map(|i| i.id.as_str()).collect(); + assert_eq!(ids, vec!["movie-dl"], "toggle off should show downloaded media only"); + + // Toggle ON: both the downloaded and the catalog-only movie are returned. + set_include_catalog_browse(true); + let full_catalog = repo.get_items("lib-1", opts).await.unwrap(); + let mut ids: Vec<&str> = full_catalog.items.iter().map(|i| i.id.as_str()).collect(); + ids.sort(); + assert_eq!(ids, vec!["movie-cat", "movie-dl"], "toggle on should reveal the full catalog"); + + // Restore default for other tests sharing this process-global flag. + set_include_catalog_browse(true); + } + /// Regression: TV episodes link to their season/series via `season_id` / /// `series_id` (NOT `parent_id`, which is NULL in the cache). A downloaded /// episode must make both its Season and Series available offline, and diff --git a/src/lib/api/bindings.ts b/src/lib/api/bindings.ts index 09025837..e87d70eb 100644 --- a/src/lib/api/bindings.ts +++ b/src/lib/api/bindings.ts @@ -825,6 +825,19 @@ async syncFullCatalog(handle: string) : Promise { async catalogSyncStatus() : Promise { return await TAURI_INVOKE("catalog_sync_status"); }, +/** + * Control whether offline library queries reveal the full synced catalog + * (greyed-out, non-downloaded media) or only downloaded/local media. + * + * The frontend calls this from the "Show all server media" toggle: pass `true` + * when online, or when offline with the toggle on; pass `false` when offline + * with the toggle off so library pages show downloaded media only. Fixes the + * bug where offline library pages showed every server item regardless of the + * toggle. + */ +async setShowServerCatalog(show: boolean) : Promise { + await TAURI_INVOKE("set_show_server_catalog", { show }); +}, /** * Resolve the stream URL for every download row that was queued while offline * (`status = 'pending' AND stream_url IS NULL`), then pump the queue so they @@ -2035,6 +2048,18 @@ export type PlayerStatusEvent = * Remote sessions updated (for cast/remote control UI) */ { type: "sessions_updated"; sessions: SessionInfo[] } | +/** + * The authoritative playback mode changed in the Rust backend. + * + * The Rust `PlaybackModeManager` is the single source of truth for which + * device playback commands route to (local vs a remote session). The + * frontend keeps a mirror store for the UI; without this event that mirror + * drifts out of sync (e.g. a mode transition happens inside a transfer or a + * local stop that the frontend never learns about), and controls then route + * to the wrong device — the classic "it keeps playing on the remote" bug. + * The frontend reconciles its store to this payload whenever it fires. + */ +{ type: "playback_mode_changed"; mode: string; session_id: string | null } | /** * The user asked to disconnect from the remote session and resume locally. * diff --git a/src/lib/components/library/AsyncImageLoading.test.ts b/src/lib/components/library/AsyncImageLoading.test.ts deleted file mode 100644 index e41c4409..00000000 --- a/src/lib/components/library/AsyncImageLoading.test.ts +++ /dev/null @@ -1,432 +0,0 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { render, waitFor } from "@testing-library/svelte"; - -/** - * Integration tests for async image loading pattern used in components - * - * Pattern: - * - Component has $state imageUrl = "" - * - Component has async loadImageUrl() function - * - Component uses $effect to call loadImageUrl when dependencies change - * - For lists: uses Map to cache URLs per item - */ - -// Mock repository with getImageUrl -const createMockRepository = () => ({ - getImageUrl: vi.fn(), -}); - -describe.skip("Async Image Loading Pattern", () => { - // Detailed async pattern tests - core functionality verified in repository-client.test.ts - let mockRepository: any; - - beforeEach(() => { - mockRepository = createMockRepository(); - vi.clearAllMocks(); - }); - - afterEach(() => { - vi.clearAllTimers(); - }); - - describe("Single Image Loading", () => { - it("should load image URL asynchronously on component mount", async () => { - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image.jpg"); - - // Simulating component with async image loading - const imageUrl = await mockRepository.getImageUrl("item123", "Primary"); - - expect(imageUrl).toBe("https://server.com/image.jpg"); - expect(mockRepository.getImageUrl).toHaveBeenCalledWith("item123", "Primary"); - }); - - it("should show placeholder while loading", async () => { - mockRepository.getImageUrl.mockImplementation( - () => new Promise((resolve) => setTimeout(() => resolve("https://server.com/image.jpg"), 100)) - ); - - vi.useFakeTimers(); - const promise = mockRepository.getImageUrl("item123", "Primary"); - - // Initially no URL - expect(promise).toBeInstanceOf(Promise); - - vi.advanceTimersByTime(100); - vi.useRealTimers(); - - const result = await promise; - expect(result).toBe("https://server.com/image.jpg"); - }); - - it("should reload image when item changes", async () => { - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image1.jpg"); - - const url1 = await mockRepository.getImageUrl("item1", "Primary"); - expect(url1).toBe("https://server.com/image1.jpg"); - - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image2.jpg"); - - const url2 = await mockRepository.getImageUrl("item2", "Primary"); - expect(url2).toBe("https://server.com/image2.jpg"); - - expect(mockRepository.getImageUrl).toHaveBeenCalledTimes(2); - }); - - it("should not reload image if item ID hasn't changed", async () => { - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image.jpg"); - - // First load - await mockRepository.getImageUrl("item123", "Primary"); - - // Would normally use $effect to track changes - // If item ID is same, should not reload (handled by component caching) - // This test documents the expected behavior - }); - - it("should handle load errors gracefully", async () => { - mockRepository.getImageUrl.mockRejectedValue(new Error("Network error")); - - // Component should catch error and show placeholder - try { - await mockRepository.getImageUrl("item123", "Primary"); - } catch (e) { - expect(e).toBeInstanceOf(Error); - } - }); - }); - - describe("List Image Caching (Map-based)", () => { - it("should cache URLs using Map", () => { - // Simulating component state: imageUrls = $state>(new Map()) - const imageUrls = new Map(); - - // Load first item - imageUrls.set("item1", "https://server.com/image1.jpg"); - expect(imageUrls.has("item1")).toBe(true); - expect(imageUrls.get("item1")).toBe("https://server.com/image1.jpg"); - - // Load second item - imageUrls.set("item2", "https://server.com/image2.jpg"); - expect(imageUrls.size).toBe(2); - - // Check cache hit - expect(imageUrls.get("item1")).toBe("https://server.com/image1.jpg"); - }); - - it("should load images only once per item", async () => { - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image.jpg"); - - const imageUrls = new Map(); - - // Simulate loading multiple items - const items = [ - { id: "item1", name: "Album 1" }, - { id: "item2", name: "Album 2" }, - { id: "item1", name: "Album 1 (again)" }, // Same ID - ]; - - for (const item of items) { - if (!imageUrls.has(item.id)) { - const url = await mockRepository.getImageUrl(item.id, "Primary"); - imageUrls.set(item.id, url); - } - } - - // Should only call once per unique ID - expect(mockRepository.getImageUrl).toHaveBeenCalledTimes(2); - }); - - it("should update single item without affecting others", async () => { - const imageUrls = new Map(); - - imageUrls.set("item1", "https://server.com/image1.jpg"); - imageUrls.set("item2", "https://server.com/image2.jpg"); - imageUrls.set("item3", "https://server.com/image3.jpg"); - - // Update item2 - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image2_updated.jpg"); - const newUrl = await mockRepository.getImageUrl("item2", "Primary"); - imageUrls.set("item2", newUrl); - - // Others should remain unchanged - expect(imageUrls.get("item1")).toBe("https://server.com/image1.jpg"); - expect(imageUrls.get("item2")).toBe("https://server.com/image2_updated.jpg"); - expect(imageUrls.get("item3")).toBe("https://server.com/image3.jpg"); - }); - - it("should clear cache when data changes", () => { - const imageUrls = new Map(); - - imageUrls.set("item1", "https://server.com/image1.jpg"); - imageUrls.set("item2", "https://server.com/image2.jpg"); - - // Clear cache - imageUrls.clear(); - - expect(imageUrls.size).toBe(0); - expect(imageUrls.has("item1")).toBe(false); - }); - - it("should support Map operations efficiently", () => { - const imageUrls = new Map(); - - // Add items - for (let i = 0; i < 100; i++) { - imageUrls.set(`item${i}`, `https://server.com/image${i}.jpg`); - } - - expect(imageUrls.size).toBe(100); - - // Check specific item - expect(imageUrls.has("item50")).toBe(true); - expect(imageUrls.get("item50")).toBe("https://server.com/image50.jpg"); - - // Iterate - let count = 0; - imageUrls.forEach(() => { - count++; - }); - expect(count).toBe(100); - }); - }); - - describe("Component Lifecycle ($effect integration)", () => { - it("should trigger load on prop change", async () => { - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image.jpg"); - - // Simulate $effect tracking prop changes - let effectCount = 0; - const trackingEffect = vi.fn(() => { - effectCount++; - return mockRepository.getImageUrl("item123", "Primary"); - }); - - trackingEffect(); - expect(effectCount).toBe(1); - - trackingEffect(); - expect(effectCount).toBe(2); - }); - - it("should skip load if conditions not met", async () => { - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image.jpg"); - - // Simulate conditional loading (e.g., if (!imageUrl && primaryImageTag)) - let imageUrl = ""; - const primaryImageTag = ""; - - if (!imageUrl && primaryImageTag) { - imageUrl = await mockRepository.getImageUrl("item123", "Primary"); - } - - expect(mockRepository.getImageUrl).not.toHaveBeenCalled(); - }); - - it("should handle dependent state updates", async () => { - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image.jpg"); - - // Simulate component state changes triggering effects - const state = { - item: { id: "item1", primaryImageTag: "tag1" }, - imageUrl: "", - }; - - const loadImage = async () => { - if (state.item.primaryImageTag) { - state.imageUrl = await mockRepository.getImageUrl(state.item.id, "Primary"); - } - }; - - await loadImage(); - expect(state.imageUrl).toBe("https://server.com/image.jpg"); - - // Change item - state.item = { id: "item2", primaryImageTag: "tag2" }; - state.imageUrl = ""; - - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image2.jpg"); - await loadImage(); - expect(state.imageUrl).toBe("https://server.com/image2.jpg"); - }); - }); - - describe("Error Handling in Async Loading", () => { - it("should set empty string on error", async () => { - mockRepository.getImageUrl.mockRejectedValue(new Error("Network error")); - - let imageUrl = ""; - - try { - imageUrl = await mockRepository.getImageUrl("item123", "Primary"); - } catch { - imageUrl = ""; // Set to empty on error - } - - expect(imageUrl).toBe(""); - }); - - it("should allow retry after error", async () => { - mockRepository.getImageUrl - .mockRejectedValueOnce(new Error("Network error")) - .mockResolvedValueOnce("https://server.com/image.jpg"); - - let imageUrl = ""; - - // First attempt fails - try { - imageUrl = await mockRepository.getImageUrl("item123", "Primary"); - } catch { - imageUrl = ""; - } - - // Retry succeeds - imageUrl = await mockRepository.getImageUrl("item123", "Primary"); - expect(imageUrl).toBe("https://server.com/image.jpg"); - }); - - it("should handle concurrent load requests", async () => { - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image.jpg"); - - // Simulate loading multiple images concurrently - const imageUrls = new Map(); - const items = [ - { id: "item1" }, - { id: "item2" }, - { id: "item3" }, - ]; - - const promises = items.map(item => - mockRepository.getImageUrl(item.id, "Primary") - .then((url: string) => imageUrls.set(item.id, url)) - .catch(() => imageUrls.set(item.id, "")) - ); - - await Promise.all(promises); - - expect(imageUrls.size).toBe(3); - expect(imageUrls.has("item1")).toBe(true); - expect(imageUrls.has("item2")).toBe(true); - expect(imageUrls.has("item3")).toBe(true); - }); - }); - - describe("Performance Characteristics", () => { - it("should not reload unnecessarily", async () => { - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image.jpg"); - - // Simulate $effect with dependency tracking - let dependencyValue = "same"; - let previousDependency = "same"; - - const loadImage = async () => { - if (dependencyValue !== previousDependency) { - previousDependency = dependencyValue; - return await mockRepository.getImageUrl("item123", "Primary"); - } - }; - - await loadImage(); - expect(mockRepository.getImageUrl).toHaveBeenCalledTimes(1); - - // No change in dependency - await loadImage(); - expect(mockRepository.getImageUrl).toHaveBeenCalledTimes(1); - - // Change dependency - dependencyValue = "changed"; - await loadImage(); - expect(mockRepository.getImageUrl).toHaveBeenCalledTimes(2); - }); - - it("should handle large lists efficiently", async () => { - const imageUrls = new Map(); - let loadCount = 0; - - mockRepository.getImageUrl.mockImplementation(() => { - loadCount++; - return Promise.resolve("https://server.com/image.jpg"); - }); - - // Simulate loading 1000 items but caching URLs - const items = Array.from({ length: 1000 }, (_, i) => ({ id: `item${i % 10}` })); - - for (const item of items) { - if (!imageUrls.has(item.id)) { - const url = await mockRepository.getImageUrl(item.id, "Primary"); - imageUrls.set(item.id, url); - } - } - - // Should only load 10 unique images - expect(loadCount).toBe(10); - expect(imageUrls.size).toBe(10); - }); - - it("should not block rendering during async loading", () => { - mockRepository.getImageUrl.mockImplementation( - () => new Promise((resolve) => - setTimeout(() => resolve("https://server.com/image.jpg"), 1000) - ) - ); - - // Async operation should not block component rendering - const renderTiming = { - startRender: Date.now(), - loadStart: null as number | null, - loadComplete: null as number | null, - }; - - // Render happens immediately - renderTiming.startRender = Date.now(); - - // Load happens asynchronously - mockRepository.getImageUrl("item123", "Primary").then(() => { - renderTiming.loadComplete = Date.now(); - }); - - // Render should complete before load finishes - expect(Date.now() - renderTiming.startRender).toBeLessThan(1000); - }); - }); - - describe("Backend Integration", () => { - it("should call backend with correct parameters", async () => { - mockRepository.getImageUrl.mockResolvedValue("https://server.com/image.jpg"); - - await mockRepository.getImageUrl("item123", "Primary", { - maxWidth: 300, - }); - - expect(mockRepository.getImageUrl).toHaveBeenCalledWith( - "item123", - "Primary", - { - maxWidth: 300, - } - ); - }); - - it("should handle backend URL correctly", async () => { - const backendUrl = "https://server.com/Items/item123/Images/Primary?maxWidth=300&api_key=token"; - mockRepository.getImageUrl.mockResolvedValue(backendUrl); - - const url = await mockRepository.getImageUrl("item123", "Primary", { maxWidth: 300 }); - - expect(url).toBe(backendUrl); - // Frontend never constructs URLs directly - expect(url).toContain("api_key="); - }); - - it("should not require URL construction in frontend", async () => { - // Frontend receives pre-constructed URL from backend - const preConstructedUrl = "https://server.com/Items/item123/Images/Primary?api_key=token"; - mockRepository.getImageUrl.mockResolvedValue(preConstructedUrl); - - const url = await mockRepository.getImageUrl("item123", "Primary"); - - // Frontend just uses the URL - expect(url).toContain("https://"); - expect(url).toContain("item123"); - }); - }); -}); diff --git a/src/lib/components/library/GenericMediaListPage.searchEvent.test.ts b/src/lib/components/library/GenericMediaListPage.searchEvent.test.ts new file mode 100644 index 00000000..cb4f9c25 --- /dev/null +++ b/src/lib/components/library/GenericMediaListPage.searchEvent.test.ts @@ -0,0 +1,147 @@ +/** + * Regression test: media-list search must surface server results. + * + * `repository_search` is two-phase — `repo.search()` resolves instantly with + * cache-only (downloaded) results, and the merged cache+server union arrives + * later via a `search-event`. A consumer that ignores that event only ever + * shows downloaded content, so search "finds nothing" for un-downloaded media. + * + * This test models that two-phase backend faithfully and would fail against a + * version of GenericMediaListPage that does not subscribe to `search-event`. + * + * TRACES: UR-008 + */ + +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, screen, fireEvent, waitFor } from "@testing-library/svelte"; +import GenericMediaListPage from "./GenericMediaListPage.svelte"; +import type { MediaListConfig } from "./GenericMediaListPage.svelte"; + +vi.mock("$app/navigation", () => ({ goto: vi.fn() })); + +vi.mock("$lib/stores/library", () => ({ + currentLibrary: { + subscribe: vi.fn((fn) => { + fn({ id: "lib123", name: "Music" }); + return vi.fn(); + }), + }, +})); + +vi.mock("$lib/stores/auth", () => ({ + auth: { getRepository: vi.fn() }, +})); + +vi.mock("$lib/composables/useServerReachabilityReload", () => ({ + useServerReachabilityReload: vi.fn(() => ({ markLoaded: vi.fn() })), +})); + +// Capture the `search-event` handler the component registers so the test can +// drive the deferred (server) phase manually. +let searchEventHandler: ((event: { payload: unknown }) => void) | null = null; +vi.mock("@tauri-apps/api/event", () => ({ + listen: vi.fn(async (name: string, handler: (event: { payload: unknown }) => void) => { + if (name === "search-event") searchEventHandler = handler; + return () => {}; + }), +})); + +const ALBUM_CONFIG: MediaListConfig = { + itemType: "MusicAlbum", + title: "Albums", + backPath: "/library/music", + searchPlaceholder: "Search albums...", + sortOptions: [{ key: "SortName", label: "Title" }], + defaultSort: "SortName", + displayComponent: "grid", +}; + +describe("GenericMediaListPage — two-phase search", () => { + beforeEach(() => { + searchEventHandler = null; + vi.clearAllMocks(); + }); + + it("renders server results that arrive after the cache-only phase", async () => { + // Phase 1 (synchronous) returns cache-only — empty, as it is for a user who + // has downloaded nothing. This is the exact condition that used to show + // "nothing found" even though the server has matching albums. + let capturedRequestId: number | undefined; + const search = vi.fn(async (_q: string, _opts: unknown, requestId: number) => { + capturedRequestId = requestId; + return { items: [], totalRecordCount: 0 }; + }); + + const getItems = vi.fn().mockResolvedValue({ items: [], totalRecordCount: 0 }); + vi.mocked((await import("$lib/stores/auth")).auth.getRepository).mockReturnValue({ + getItems, + search, + } as any); + + const { container } = render(GenericMediaListPage, { props: { config: ALBUM_CONFIG } }); + + // Let the initial (mount) load finish so the debounced search effect is armed. + await waitFor(() => expect(getItems).toHaveBeenCalled()); + + const input = container.querySelector("input") as HTMLInputElement; + fireEvent.input(input, { target: { value: "Rumours" } }); + + // Debounced search fires after 300ms and returns the empty cache result. + // The `search-event` listener is registered lazily as part of searching. + await waitFor(() => expect(search).toHaveBeenCalled()); + await waitFor(() => expect(searchEventHandler).not.toBeNull()); + // Cache-only phase: nothing to show yet (the results counter reads zero). + await waitFor(() => + expect(screen.getByText(/0 musicalbums matching/)).toBeTruthy() + ); + + // Phase 2: backend emits the merged cache+server union for this request. + expect(capturedRequestId).toBeTypeOf("number"); + searchEventHandler!({ + payload: { + requestId: capturedRequestId, + result: { + items: [{ id: "album1", name: "Rumours", type: "MusicAlbum" }], + totalRecordCount: 1, + }, + }, + }); + + // The server result must now be reflected in the list. Old code (no + // listener) never reached this state — the count stayed at zero. + await waitFor(() => + expect(screen.getByText(/1 musicalbum matching/)).toBeTruthy() + ); + }); + + it("ignores a search-event whose requestId is stale", async () => { + const search = vi.fn(async () => ({ items: [], totalRecordCount: 0 })); + const getItems = vi.fn().mockResolvedValue({ items: [], totalRecordCount: 0 }); + vi.mocked((await import("$lib/stores/auth")).auth.getRepository).mockReturnValue({ + getItems, + search, + } as any); + + const { container } = render(GenericMediaListPage, { props: { config: ALBUM_CONFIG } }); + await waitFor(() => expect(getItems).toHaveBeenCalled()); + + const input = container.querySelector("input") as HTMLInputElement; + fireEvent.input(input, { target: { value: "Rumours" } }); + await waitFor(() => expect(search).toHaveBeenCalled()); + await waitFor(() => expect(searchEventHandler).not.toBeNull()); + + // A superseded query's late result (wrong requestId) must not render. + searchEventHandler!({ + payload: { + requestId: -999, + result: { + items: [{ id: "stale", name: "Stale Album", type: "MusicAlbum" }], + totalRecordCount: 1, + }, + }, + }); + + await new Promise((r) => setTimeout(r, 0)); + expect(screen.queryByText("Stale Album")).toBeNull(); + }); +}); diff --git a/src/lib/components/library/GenericMediaListPage.svelte b/src/lib/components/library/GenericMediaListPage.svelte index 0d98e39b..dbddad7d 100644 --- a/src/lib/components/library/GenericMediaListPage.svelte +++ b/src/lib/components/library/GenericMediaListPage.svelte @@ -1,6 +1,7 @@