From 57b24f8c745c1252cabd9a3c30c86599825bc68c Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Fri, 24 Jul 2026 21:42:12 +0200 Subject: [PATCH] fix(offline): Downloaded browse groups by container and loads on large libraries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs on the Downloaded browse surface (UR-055/UR-056): 1. Grouping — browsing a downloaded *library* listed individual leaves (songs, episodes) instead of their containers. The library-level match in `get_downloaded_items` selected every downloaded item on the server; add a NOT EXISTS clause so the top level shows only albums/series/movies, with leaves still reachable by drilling in. Regression tests for music + TV. 2. "Loading your downloads…" hung on large libraries. The disk-usage partiality query did an OR-based self-join over the entire synced catalog (O(items^2), unindexable). Narrow it to downloaded containers first via a CTE, and add the missing idx_items_season index (migration 020 + base schema) — parent_id/album_id/series_id were already indexed. Also annotate the existing backend tests that cover the IT-016/IT-017 end-to-end offline-listing scenarios with their trace IDs. --- src-tauri/src/commands/catalog.rs | 6 ++ src-tauri/src/repository/offline.rs | 160 ++++++++++++++++++++++++++-- src-tauri/src/storage/schema.rs | 14 +++ 3 files changed, 172 insertions(+), 8 deletions(-) diff --git a/src-tauri/src/commands/catalog.rs b/src-tauri/src/commands/catalog.rs index 664dbc35..d9b43077 100644 --- a/src-tauri/src/commands/catalog.rs +++ b/src-tauri/src/commands/catalog.rs @@ -433,6 +433,12 @@ mod tests { .unwrap() } + /// IT-017: a download queued from a greyed-out offline catalog entry + /// (pending, `stream_url IS NULL`) persists, and on reconnect its URL is + /// resolved and the row is healed (URL + target dir) so the pump can start + /// it — while already-resolved rows are left untouched. + /// + /// TRACES: UR-052, UR-011 | IT-017 #[tokio::test] async fn resolves_offline_queued_row_and_leaves_resolved_rows_untouched() { let db = test_db(); diff --git a/src-tauri/src/repository/offline.rs b/src-tauri/src/repository/offline.rs index 9f0a6b0c..208d4a0c 100644 --- a/src-tauri/src/repository/offline.rs +++ b/src-tauri/src/repository/offline.rs @@ -587,6 +587,18 @@ impl OfflineRepository { String::new() }; + // When the parent is a LIBRARY, cached items carry no link back to it + // (library_id/parent_id are NULL), so the `libraries` EXISTS clause below + // matches every downloaded item on the server — both containers + // (MusicAlbum/Series/…) AND their leaves (Audio/Episode). Listing the + // leaves alongside the containers is the "I see individual songs, not + // albums" bug: a library landing page must show only *top-level* items. + // So at the library level we exclude any leaf whose own container + // (album/season/series/parent) is itself present in `downloaded_items` — + // that container represents it in the grid. Items with no downloaded + // container (e.g. a downloaded Movie, or a stray track whose album isn't + // cached) still surface. This mirrors the online music library, which + // routes to a dedicated albums view. See [[offline-libraries-never-cached]]. let sql = format!( "{cte} SELECT i.id, i.name, i.item_type, i.server_id, i.parent_id, i.library_id, i.overview, i.genres, @@ -599,9 +611,19 @@ impl OfflineRepository { WHERE i.server_id = ? AND ( i.parent_id = ? OR i.album_id = ? OR i.season_id = ? OR i.series_id = ? - OR EXISTS ( - SELECT 1 FROM libraries l - WHERE l.id = ? AND l.server_id = i.server_id + OR ( + EXISTS ( + SELECT 1 FROM libraries l + WHERE l.id = ? AND l.server_id = i.server_id + ) + -- Top-level only: hide leaves whose container is downloaded. + AND NOT EXISTS ( + SELECT 1 FROM downloaded_items parent + WHERE parent.id = i.album_id + OR parent.id = i.season_id + OR parent.id = i.series_id + OR parent.id = i.parent_id + ) ) ){type_filter} ORDER BY i.sort_name ASC, i.name ASC @@ -736,17 +758,32 @@ impl OfflineRepository { // descendants that are NOT downloaded. We compare downloaded-descendant // count against total-cached-descendant count (the offline cache holds // the synced full catalog, so this is meaningful). + // + // Perf: restrict `c` to containers that actually have a completed + // download *first* (the CTE), so the OR-based self-join runs over that + // handful of rows instead of the entire synced catalog. Without this the + // join is an unindexable O(items²) scan and the Downloaded page hangs on + // a large library ("Loading your downloads…" forever). let partial_query = Query::with_params( - "SELECT c.id, + "WITH downloaded_containers AS ( + SELECT DISTINCT c.id + FROM items c + INNER JOIN items children + ON (children.parent_id = c.id OR children.album_id = c.id OR children.season_id = c.id OR children.series_id = c.id) + INNER JOIN downloads d ON children.id = d.item_id + WHERE d.status = 'completed' + AND c.server_id = ? + AND c.item_type IN ('MusicAlbum', 'Series', 'Season', 'BoxSet', 'Folder', 'CollectionFolder') + ) + SELECT c.id, COUNT(children.id) AS total_children, SUM(CASE WHEN d.status = 'completed' THEN 1 ELSE 0 END) AS downloaded_children FROM items c + INNER JOIN downloaded_containers dc ON dc.id = c.id INNER JOIN items children ON (children.parent_id = c.id OR children.album_id = c.id OR children.season_id = c.id OR children.series_id = c.id) LEFT JOIN downloads d ON children.id = d.item_id AND d.status = 'completed' - WHERE c.server_id = ? - AND c.item_type IN ('MusicAlbum', 'Series', 'Season', 'BoxSet', 'Folder', 'CollectionFolder') - AND children.item_type IN ('Audio', 'Movie', 'Episode', 'Season') + WHERE children.item_type IN ('Audio', 'Movie', 'Episode', 'Season') GROUP BY c.id", vec![QueryParam::String(self.server_id.clone())], ); @@ -2340,7 +2377,12 @@ mod tests { /// (synced-but-not-downloaded) catalog is revealed. Fixes the bug where /// offline library pages showed every server item regardless of the toggle. /// - /// TRACES: UR-052 | DR-078 | UT-067 + /// This is also the backend half of the end-to-end offline-listing scenario + /// IT-016: toggle off ⇒ downloaded media only; toggle on ⇒ the cached server + /// catalog is additionally revealed (greyed-out in the UI, distinguished by + /// the absence of a `downloads` row — see `MediaCard.isServerOnly`). + /// + /// TRACES: UR-052 | DR-078 | UT-067, IT-016 #[tokio::test] async fn test_get_items_toggle_gates_synced_catalog() { use crate::storage::db_service::DatabaseService; @@ -2659,6 +2701,108 @@ mod tests { assert_eq!(ids, vec!["track-1"], "only the downloaded track is listed"); } + /// Regression: browsing a downloaded *library* (top level) lists containers, + /// not their leaves — a music library shows the album, not the individual + /// downloaded songs. The leaf is still reachable by drilling into the album. + /// + /// TRACES: UR-055 | DR-082, DR-083 + #[tokio::test] + async fn test_get_downloaded_items_library_lists_albums_not_tracks() { + let db = create_test_db(); + seed_library(&db, "music-lib", "music").await; + insert_item(&db, "album-1", "MusicAlbum", None, None, None).await; + // Tracks link to the album via album_id (parent_id NULL in the cache). + insert_item(&db, "track-1", "Audio", Some("album-1"), None, None).await; + insert_item(&db, "track-2", "Audio", Some("album-1"), None, None).await; + seed_completed_download(&db, "track-1", 1000).await; + seed_completed_download(&db, "track-2", 1000).await; + + let repo = make_repo(&db); + + // Library level: only the album shows, not the two tracks. + let at_library = repo.get_downloaded_items("music-lib", None).await.unwrap(); + let ids: Vec<&str> = at_library.items.iter().map(|i| i.id.as_str()).collect(); + assert_eq!( + ids, + vec!["album-1"], + "library browse lists the album container, not its tracks" + ); + + // Drilling into the album still returns the downloaded tracks. + let in_album = repo.get_downloaded_items("album-1", None).await.unwrap(); + let mut track_ids: Vec<&str> = in_album.items.iter().map(|i| i.id.as_str()).collect(); + track_ids.sort(); + assert_eq!(track_ids, vec!["track-1", "track-2"]); + } + + /// Regression: a downloaded TV library lists the Series, not its Seasons or + /// Episodes — the same "individual songs" bug seen for music, for TV. The + /// season and episode are still reachable by drilling into the series. + /// + /// TRACES: UR-055 | DR-082, DR-083 + #[tokio::test] + async fn test_get_downloaded_items_library_lists_series_not_episodes() { + let db = create_test_db(); + seed_library(&db, "tv-lib", "tvshows").await; + insert_item(&db, "series-1", "Series", None, None, None).await; + // Season links to its series; episode links to both season and series. + insert_item(&db, "season-1", "Season", None, Some("series-1"), None).await; + insert_item( + &db, + "ep-1", + "Episode", + None, + Some("series-1"), + Some("season-1"), + ) + .await; + seed_completed_download(&db, "ep-1", 4000).await; + + let repo = make_repo(&db); + + // Library level: only the series shows. + let at_library = repo.get_downloaded_items("tv-lib", None).await.unwrap(); + let ids: Vec<&str> = at_library.items.iter().map(|i| i.id.as_str()).collect(); + assert_eq!( + ids, + vec!["series-1"], + "TV library browse lists the series, not seasons/episodes" + ); + + // Drilling into the series returns its season; into the season, the episode. + let in_series = repo.get_downloaded_items("series-1", None).await.unwrap(); + assert!( + in_series.items.iter().any(|i| i.id == "season-1"), + "series drill returns the season" + ); + let in_season = repo.get_downloaded_items("season-1", None).await.unwrap(); + assert!( + in_season.items.iter().any(|i| i.id == "ep-1"), + "season drill returns the episode" + ); + } + + /// A downloaded leaf with no cached container (e.g. a Movie, or a track whose + /// album isn't in the cache) still surfaces at the library level. + /// + /// TRACES: UR-055 | DR-082, DR-083 + #[tokio::test] + async fn test_get_downloaded_items_library_keeps_orphan_leaves() { + let db = create_test_db(); + seed_library(&db, "movie-lib", "movies").await; + insert_item(&db, "movie-1", "Movie", None, None, None).await; + seed_completed_download(&db, "movie-1", 5000).await; + + let repo = make_repo(&db); + let at_library = repo.get_downloaded_items("movie-lib", None).await.unwrap(); + let ids: Vec<&str> = at_library.items.iter().map(|i| i.id.as_str()).collect(); + assert_eq!( + ids, + vec!["movie-1"], + "a downloaded movie with no container shows" + ); + } + /// UT: an empty downloaded-only browse is authoritative — no rows, no error, /// regardless of the catalog-browse flag (which the DR-080 fallthrough uses). /// diff --git a/src-tauri/src/storage/schema.rs b/src-tauri/src/storage/schema.rs index 5be635e3..22f1cfa5 100644 --- a/src-tauri/src/storage/schema.rs +++ b/src-tauri/src/storage/schema.rs @@ -24,6 +24,7 @@ pub const MIGRATIONS: &[(&str, &str)] = &[ ("017_downloads_resume_url", MIGRATION_017), ("018_items_is_folder", MIGRATION_018), ("019_genres_cache", MIGRATION_019), + ("020_items_season_index", MIGRATION_020), ]; /// Initial schema migration @@ -281,6 +282,7 @@ CREATE INDEX IF NOT EXISTS idx_items_parent ON items(parent_id); CREATE INDEX IF NOT EXISTS idx_items_type ON items(item_type); CREATE INDEX IF NOT EXISTS idx_items_album ON items(album_id); CREATE INDEX IF NOT EXISTS idx_items_series ON items(series_id); +CREATE INDEX IF NOT EXISTS idx_items_season ON items(season_id); CREATE INDEX IF NOT EXISTS idx_user_data_user ON user_data(user_id); CREATE INDEX IF NOT EXISTS idx_user_data_item ON user_data(item_id); CREATE INDEX IF NOT EXISTS idx_downloads_status ON downloads(status); @@ -714,3 +716,15 @@ CREATE TABLE IF NOT EXISTS genres ( CREATE INDEX IF NOT EXISTS idx_genres_scope ON genres(server_id, library_id); "#; + +/// Migration to index `items.season_id`. +/// +/// Episodes link to their season via `season_id` (parent_id is NULL in the +/// cache). The container-rollup queries used by the Downloaded browse and the +/// disk-usage aggregation join `children.season_id = c.id`, which without this +/// index degrades to an unindexable scan — a large synced catalog then makes +/// the Downloaded page hang ("Loading your downloads…"). `parent_id`, +/// `album_id`, and `series_id` were already indexed; this closes the gap. +const MIGRATION_020: &str = r#" +CREATE INDEX IF NOT EXISTS idx_items_season ON items(season_id); +"#;