fix(catalog): show a new album once in Recently Added, not once per track
Recently Added listed every newly-added track individually, so importing a 14-track album filled the whole row with that one album and buried everything else. Both code paths that build the row had the same symptom from separate causes: - Online: Jellyfin's /Items/Latest defaults to GroupItems=false, returning each new leaf on its own. Send GroupItems=true so the server collapses children into the container that was added. - Offline: the downloaded-items CTE deliberately matches leaves *and* their container (right for browsing, wrong here), so a downloaded album returned the album plus each of its tracks. Drop a leaf only when its own container is in the same result. Items with no container (movies, standalone tracks) are unaffected in both paths. The online URL is extracted into build_latest_items_endpoint so it can be asserted without an HTTP server, matching build_favorites_endpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1426,6 +1426,14 @@ impl MediaRepository for OfflineRepository {
|
|||||||
FROM items i
|
FROM items i
|
||||||
INNER JOIN downloaded_items di ON i.id = di.id
|
INNER JOIN downloaded_items di ON i.id = di.id
|
||||||
WHERE i.server_id = ? AND i.library_id = ?
|
WHERE i.server_id = ? AND i.library_id = ?
|
||||||
|
-- Collapse leaves into the container that was added: a new
|
||||||
|
-- 14-track album should read as one album, not 14 songs. Only
|
||||||
|
-- drops a leaf when its own container is present in the same
|
||||||
|
-- result, so a standalone track or movie still appears.
|
||||||
|
AND NOT EXISTS (
|
||||||
|
SELECT 1 FROM downloaded_items parent
|
||||||
|
WHERE parent.id IN (i.album_id, i.season_id, i.series_id, i.parent_id)
|
||||||
|
)
|
||||||
ORDER BY i.synced_at DESC
|
ORDER BY i.synced_at DESC
|
||||||
LIMIT {}", limit_val
|
LIMIT {}", limit_val
|
||||||
),
|
),
|
||||||
@@ -3544,6 +3552,32 @@ mod tests {
|
|||||||
.unwrap();
|
.unwrap();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Like `insert_item`, but sets `library_id` — which `get_latest_items`
|
||||||
|
/// filters on, so rows without it are invisible to that query.
|
||||||
|
async fn insert_library_item(
|
||||||
|
db: &Arc<RusqliteService>,
|
||||||
|
id: &str,
|
||||||
|
item_type: &str,
|
||||||
|
library_id: &str,
|
||||||
|
album_id: Option<&str>,
|
||||||
|
) {
|
||||||
|
db.execute(Query::with_params(
|
||||||
|
"INSERT INTO items (id, server_id, library_id, name, item_type, album_id, synced_at)
|
||||||
|
VALUES (?1, 'test-server', ?2, ?3, ?4, ?5, '2024-01-01')",
|
||||||
|
vec![
|
||||||
|
QueryParam::String(id.to_string()),
|
||||||
|
QueryParam::String(library_id.to_string()),
|
||||||
|
QueryParam::String(format!("Name {id}")),
|
||||||
|
QueryParam::String(item_type.to_string()),
|
||||||
|
album_id
|
||||||
|
.map(|s| QueryParam::String(s.to_string()))
|
||||||
|
.unwrap_or(QueryParam::Null),
|
||||||
|
],
|
||||||
|
))
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
|
}
|
||||||
|
|
||||||
async fn seed_completed_download(db: &Arc<RusqliteService>, item_id: &str, file_size: i64) {
|
async fn seed_completed_download(db: &Arc<RusqliteService>, item_id: &str, file_size: i64) {
|
||||||
db.execute(Query::with_params(
|
db.execute(Query::with_params(
|
||||||
"INSERT INTO downloads (item_id, status, file_size) VALUES (?1, 'completed', ?2)",
|
"INSERT INTO downloads (item_id, status, file_size) VALUES (?1, 'completed', ?2)",
|
||||||
@@ -3578,6 +3612,36 @@ mod tests {
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// A newly-synced album appears once in "recently added", not once per track.
|
||||||
|
///
|
||||||
|
/// The downloaded-items CTE deliberately matches both the leaves and their
|
||||||
|
/// container, which is right for browsing but wrong here: it made a 3-track
|
||||||
|
/// album occupy 4 slots in the row. Tracks whose album is itself in the
|
||||||
|
/// result are now collapsed into it.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn test_get_latest_items_collapses_tracks_into_their_album() {
|
||||||
|
let db = create_test_db();
|
||||||
|
insert_library_item(&db, "album-1", "MusicAlbum", "lib-1", None).await;
|
||||||
|
for track in ["track-1", "track-2", "track-3"] {
|
||||||
|
insert_library_item(&db, track, "Audio", "lib-1", Some("album-1")).await;
|
||||||
|
seed_completed_download(&db, track, 1000).await;
|
||||||
|
}
|
||||||
|
// A movie has no container, so it must still show up on its own.
|
||||||
|
insert_library_item(&db, "movie-1", "Movie", "lib-1", None).await;
|
||||||
|
seed_completed_download(&db, "movie-1", 2000).await;
|
||||||
|
|
||||||
|
let repo = make_repo(&db);
|
||||||
|
let latest = repo.get_latest_items("lib-1", Some(16)).await.unwrap();
|
||||||
|
let ids: Vec<&str> = latest.iter().map(|i| i.id.as_str()).collect();
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
!ids.iter().any(|id| id.starts_with("track-")),
|
||||||
|
"individual tracks must collapse into their album, got: {ids:?}"
|
||||||
|
);
|
||||||
|
assert!(ids.contains(&"album-1"), "the album itself is listed");
|
||||||
|
assert!(ids.contains(&"movie-1"), "containerless items still listed");
|
||||||
|
}
|
||||||
|
|
||||||
/// UT: downloaded-only browse returns a downloaded leaf AND its container,
|
/// UT: downloaded-only browse returns a downloaded leaf AND its container,
|
||||||
/// filtered to the requested album parent. A non-downloaded sibling is omitted.
|
/// filtered to the requested album parent. A non-downloaded sibling is omitted.
|
||||||
///
|
///
|
||||||
|
|||||||
@@ -670,6 +670,26 @@ fn build_get_items_endpoint(
|
|||||||
endpoint
|
endpoint
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Build the Jellyfin endpoint for a "recently added" listing.
|
||||||
|
///
|
||||||
|
/// `GroupItems=true` is the load-bearing parameter: Jellyfin defaults it to
|
||||||
|
/// `false`, which returns each newly-added *leaf* separately, so importing one
|
||||||
|
/// 14-track album pushed 14 rows into "recently added" and buried everything
|
||||||
|
/// else. With grouping on, the server collapses children into the container
|
||||||
|
/// that was added — an album appears once, while movies (which have no such
|
||||||
|
/// container) are unaffected.
|
||||||
|
///
|
||||||
|
/// Pulled out of `get_latest_items` so the query can be asserted without an
|
||||||
|
/// HTTP server, matching `build_favorites_endpoint`.
|
||||||
|
fn build_latest_items_endpoint(user_id: &str, parent_id: &str, limit: Option<usize>) -> String {
|
||||||
|
format!(
|
||||||
|
"/Users/{}/Items/Latest?ParentId={}&Limit={}&GroupItems=true&Fields=BackdropImageTags,ParentBackdropImageTags,UserData",
|
||||||
|
user_id,
|
||||||
|
parent_id,
|
||||||
|
limit.unwrap_or(16)
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
/// Build the Jellyfin endpoint for a favourites listing.
|
/// Build the Jellyfin endpoint for a favourites listing.
|
||||||
///
|
///
|
||||||
/// Pulled out of `get_favorites` so the query can be asserted without an HTTP
|
/// Pulled out of `get_favorites` so the query can be asserted without an HTTP
|
||||||
@@ -908,11 +928,7 @@ impl MediaRepository for OnlineRepository {
|
|||||||
parent_id: &str,
|
parent_id: &str,
|
||||||
limit: Option<usize>,
|
limit: Option<usize>,
|
||||||
) -> Result<Vec<MediaItem>, RepoError> {
|
) -> Result<Vec<MediaItem>, RepoError> {
|
||||||
let limit_str = limit.unwrap_or(16);
|
let endpoint = build_latest_items_endpoint(&self.user_id, parent_id, limit);
|
||||||
let endpoint = format!(
|
|
||||||
"/Users/{}/Items/Latest?ParentId={}&Limit={}&Fields=BackdropImageTags,ParentBackdropImageTags,UserData",
|
|
||||||
self.user_id, parent_id, limit_str
|
|
||||||
);
|
|
||||||
|
|
||||||
let items: Vec<JellyfinItem> = self.get_json(&endpoint).await?;
|
let items: Vec<JellyfinItem> = self.get_json(&endpoint).await?;
|
||||||
Ok(items
|
Ok(items
|
||||||
@@ -2706,6 +2722,25 @@ mod tests {
|
|||||||
assert!(!off.contains("Filters=IsFavorite"));
|
assert!(!off.contains("Filters=IsFavorite"));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// A newly-added album must arrive as one entry, not one per track.
|
||||||
|
///
|
||||||
|
/// Jellyfin's `/Items/Latest` defaults to `GroupItems=false`, which returns
|
||||||
|
/// every new Audio track individually — so ripping a 14-track album filled
|
||||||
|
/// the whole "recently added" row with that one album. `GroupItems=true`
|
||||||
|
/// makes the server collapse children into their parent container.
|
||||||
|
#[test]
|
||||||
|
fn test_latest_items_endpoint_groups_children_into_containers() {
|
||||||
|
let endpoint = build_latest_items_endpoint("u1", "lib-1", Some(16));
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
endpoint.contains("GroupItems=true"),
|
||||||
|
"latest items must be grouped so an album counts once, got: {}",
|
||||||
|
endpoint
|
||||||
|
);
|
||||||
|
assert!(endpoint.contains("ParentId=lib-1"));
|
||||||
|
assert!(endpoint.contains("Limit=16"));
|
||||||
|
}
|
||||||
|
|
||||||
/// UT-099 — a Jellyfin item's `UserData` reaches `MediaItem.user_data`.
|
/// UT-099 — a Jellyfin item's `UserData` reaches `MediaItem.user_data`.
|
||||||
///
|
///
|
||||||
/// Before DR-113 this mapping was hardcoded to `None`, so nothing outside
|
/// Before DR-113 this mapping was hardcoded to `None`, so nothing outside
|
||||||
|
|||||||
Reference in New Issue
Block a user