fix(offline): Downloaded browse groups by container and loads on large libraries
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.
This commit is contained in:
@@ -433,6 +433,12 @@ mod tests {
|
|||||||
.unwrap()
|
.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]
|
#[tokio::test]
|
||||||
async fn resolves_offline_queued_row_and_leaves_resolved_rows_untouched() {
|
async fn resolves_offline_queued_row_and_leaves_resolved_rows_untouched() {
|
||||||
let db = test_db();
|
let db = test_db();
|
||||||
|
|||||||
@@ -587,6 +587,18 @@ impl OfflineRepository {
|
|||||||
String::new()
|
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!(
|
let sql = format!(
|
||||||
"{cte}
|
"{cte}
|
||||||
SELECT i.id, i.name, i.item_type, i.server_id, i.parent_id, i.library_id, i.overview, i.genres,
|
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 = ?
|
WHERE i.server_id = ?
|
||||||
AND (
|
AND (
|
||||||
i.parent_id = ? OR i.album_id = ? OR i.season_id = ? OR i.series_id = ?
|
i.parent_id = ? OR i.album_id = ? OR i.season_id = ? OR i.series_id = ?
|
||||||
OR EXISTS (
|
OR (
|
||||||
SELECT 1 FROM libraries l
|
EXISTS (
|
||||||
WHERE l.id = ? AND l.server_id = i.server_id
|
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}
|
){type_filter}
|
||||||
ORDER BY i.sort_name ASC, i.name ASC
|
ORDER BY i.sort_name ASC, i.name ASC
|
||||||
@@ -736,17 +758,32 @@ impl OfflineRepository {
|
|||||||
// descendants that are NOT downloaded. We compare downloaded-descendant
|
// descendants that are NOT downloaded. We compare downloaded-descendant
|
||||||
// count against total-cached-descendant count (the offline cache holds
|
// count against total-cached-descendant count (the offline cache holds
|
||||||
// the synced full catalog, so this is meaningful).
|
// 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(
|
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,
|
COUNT(children.id) AS total_children,
|
||||||
SUM(CASE WHEN d.status = 'completed' THEN 1 ELSE 0 END) AS downloaded_children
|
SUM(CASE WHEN d.status = 'completed' THEN 1 ELSE 0 END) AS downloaded_children
|
||||||
FROM items c
|
FROM items c
|
||||||
|
INNER JOIN downloaded_containers dc ON dc.id = c.id
|
||||||
INNER JOIN items children
|
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)
|
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'
|
LEFT JOIN downloads d ON children.id = d.item_id AND d.status = 'completed'
|
||||||
WHERE c.server_id = ?
|
WHERE children.item_type IN ('Audio', 'Movie', 'Episode', 'Season')
|
||||||
AND c.item_type IN ('MusicAlbum', 'Series', 'Season', 'BoxSet', 'Folder', 'CollectionFolder')
|
|
||||||
AND children.item_type IN ('Audio', 'Movie', 'Episode', 'Season')
|
|
||||||
GROUP BY c.id",
|
GROUP BY c.id",
|
||||||
vec![QueryParam::String(self.server_id.clone())],
|
vec![QueryParam::String(self.server_id.clone())],
|
||||||
);
|
);
|
||||||
@@ -2340,7 +2377,12 @@ mod tests {
|
|||||||
/// (synced-but-not-downloaded) catalog is revealed. Fixes the bug where
|
/// (synced-but-not-downloaded) catalog is revealed. Fixes the bug where
|
||||||
/// offline library pages showed every server item regardless of the toggle.
|
/// 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]
|
#[tokio::test]
|
||||||
async fn test_get_items_toggle_gates_synced_catalog() {
|
async fn test_get_items_toggle_gates_synced_catalog() {
|
||||||
use crate::storage::db_service::DatabaseService;
|
use crate::storage::db_service::DatabaseService;
|
||||||
@@ -2659,6 +2701,108 @@ mod tests {
|
|||||||
assert_eq!(ids, vec!["track-1"], "only the downloaded track is listed");
|
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,
|
/// UT: an empty downloaded-only browse is authoritative — no rows, no error,
|
||||||
/// regardless of the catalog-browse flag (which the DR-080 fallthrough uses).
|
/// regardless of the catalog-browse flag (which the DR-080 fallthrough uses).
|
||||||
///
|
///
|
||||||
|
|||||||
@@ -24,6 +24,7 @@ pub const MIGRATIONS: &[(&str, &str)] = &[
|
|||||||
("017_downloads_resume_url", MIGRATION_017),
|
("017_downloads_resume_url", MIGRATION_017),
|
||||||
("018_items_is_folder", MIGRATION_018),
|
("018_items_is_folder", MIGRATION_018),
|
||||||
("019_genres_cache", MIGRATION_019),
|
("019_genres_cache", MIGRATION_019),
|
||||||
|
("020_items_season_index", MIGRATION_020),
|
||||||
];
|
];
|
||||||
|
|
||||||
/// Initial schema migration
|
/// 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_type ON items(item_type);
|
||||||
CREATE INDEX IF NOT EXISTS idx_items_album ON items(album_id);
|
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_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_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_user_data_item ON user_data(item_id);
|
||||||
CREATE INDEX IF NOT EXISTS idx_downloads_status ON downloads(status);
|
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);
|
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);
|
||||||
|
"#;
|
||||||
|
|||||||
Reference in New Issue
Block a user