From bb79d99c2d5ea82ad1506d7e880b7b99dcda3aa9 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 3 May 2026 00:26:57 -0400 Subject: [PATCH] fix(artwork): include top-level album folders in parent cover art lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Path != "." guard added in #5451 was too aggressive — it excluded any folder with Path=".", which includes top-level album folders (not just the library root). Changed to ParentID != "" which correctly excludes only the actual library root folder. Fixes #5456 --- core/artwork/e2e/album_test.go | 20 ++++++++++- core/artwork/e2e/disc_test.go | 40 +++++++++++++++++++++ core/artwork/reader_album.go | 2 +- core/artwork/reader_album_test.go | 59 ++++++++++++++++++++++++++----- 4 files changed, 111 insertions(+), 10 deletions(-) diff --git a/core/artwork/e2e/album_test.go b/core/artwork/e2e/album_test.go index 370844e34..9de974339 100644 --- a/core/artwork/e2e/album_test.go +++ b/core/artwork/e2e/album_test.go @@ -105,7 +105,7 @@ var _ = Describe("Album artwork resolution", func() { // └── Album/ // ├── disc1/ // │ └── 01 - Track.mp3 - // └── cover.jpg ← should win (parent-folder fallback, currently ignored — bug) + // └── cover.jpg ← should win (parent-folder fallback) It("uses the parent-folder cover for single-disc-subfolder albums", func() { conf.Server.CoverArtPriority = defaultCoverPriority setLayout(fstest.MapFS{ @@ -119,6 +119,24 @@ var _ = Describe("Album artwork resolution", func() { }) }) + // Reproduces https://github.com/navidrome/navidrome/issues/5456 + When("a top-level multi-disc album has cover.jpg at the album root and per-disc folder.jpg", func() { + It("prefers the album-root cover.jpg", func() { + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Album/CD1/01 - Track.mp3": trackFile(1, "Track CD1"), + "Album/CD2/01 - Track.mp3": trackFile(1, "Track CD2"), + "Album/cover.jpg": imageFile("album-root"), + "Album/CD1/folder.jpg": imageFile("disc1"), + "Album/CD2/folder.jpg": imageFile("disc2"), + }) + scan() + + al := firstAlbum() + Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root"))) + }) + }) + When("CoverArtPriority puts embedded first and the album has both embedded and external art", func() { // Artist/ // └── Album/ diff --git a/core/artwork/e2e/disc_test.go b/core/artwork/e2e/disc_test.go index 7569cbc32..d22b83a5c 100644 --- a/core/artwork/e2e/disc_test.go +++ b/core/artwork/e2e/disc_test.go @@ -1,6 +1,7 @@ package artworke2e_test import ( + "fmt" "testing/fstest" "github.com/navidrome/navidrome/conf" @@ -255,6 +256,45 @@ var _ = Describe("Disc artwork resolution", func() { }) }) + // Reproduces https://github.com/navidrome/navidrome/issues/5456 + When("a top-level multi-disc album has cover.jpg and per-disc folder.jpg", func() { + // Album/ (top-level, Path=".") + // ├── cover.jpg ← album-level cover + // ├── Disc 01/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg ← disc 1 art + // ├── Disc 02/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // └── Disc 03/ + // ├── 01 - Track.mp3 + // └── folder.jpg + It("uses album-root cover.jpg for album art and per-disc folder.jpg for each disc", func() { + conf.Server.DiscArtPriority = defaultDiscPriority + conf.Server.CoverArtPriority = defaultCoverPriority + layout := fstest.MapFS{ + "Album/cover.jpg": imageFile("album-root-cover"), + } + for i := 1; i <= 3; i++ { + prefix := fmt.Sprintf("Album/Disc %02d/", i) + layout[prefix+"01 - Track.mp3"] = trackFile(1, fmt.Sprintf("T%d", i), map[string]any{"disc": fmt.Sprintf("%d", i)}) + layout[prefix+"folder.jpg"] = imageFile(fmt.Sprintf("disc-%02d-folder", i)) + } + setLayout(layout) + scan() + + al := firstAlbum() + + Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root-cover"))) + + for i := 1; i <= 3; i++ { + discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, i), &al.UpdatedAt) + Expect(readArtwork(discID)).To(Equal(imageBytes(fmt.Sprintf("disc-%02d-folder", i))), + "disc %d should use its own folder.jpg", i) + } + }) + }) + When("discsubtitle is set but no image filename matches the subtitle", func() { // Artist/ // └── Album/ diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index cf5497641..0dbb88dab 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -131,7 +131,7 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo } else if err != nil { return nil, nil, nil, err } - if parentFolder != nil && parentFolder.Path != "." { + if parentFolder != nil && parentFolder.ParentID != "" { folders = append(folders, *parentFolder) } } diff --git a/core/artwork/reader_album_test.go b/core/artwork/reader_album_test.go index b8f4f2dfa..15558d86c 100644 --- a/core/artwork/reader_album_test.go +++ b/core/artwork/reader_album_test.go @@ -141,6 +141,7 @@ var _ = Describe("Album Artwork Reader", func() { ID: "parentFolder", Path: "Artist", Name: "Album", + ParentID: "artistFolder", ImagesUpdatedAt: expectedAt, ImageFiles: []string{"cover.jpg", "back.jpg"}, } @@ -213,14 +214,14 @@ var _ = Describe("Album Artwork Reader", func() { Expect(repo.getCallCount).To(Equal(0)) }) - It("does not include top-level parent for multi-folder albums", func() { - // Two album parts under the same artist folder — parent is artist-level + It("does not include library root parent for multi-folder albums", func() { + // Two album parts directly under the library root — parent is the root itself repo.result = []model.Folder{ { ID: "folder1", Path: ".", Name: "AlbumPart1", - ParentID: "artistFolder", + ParentID: "rootFolder", ImagesUpdatedAt: now, ImageFiles: []string{"cover.jpg"}, }, @@ -228,16 +229,17 @@ var _ = Describe("Album Artwork Reader", func() { ID: "folder2", Path: ".", Name: "AlbumPart2", - ParentID: "artistFolder", + ParentID: "rootFolder", ImagesUpdatedAt: now, ImageFiles: []string{}, }, } repo.parentResult = &model.Folder{ - ID: "artistFolder", - Path: ".", - Name: "Artist", - ImageFiles: []string{"artist.jpg"}, + ID: "rootFolder", + Path: "", + Name: ".", + ParentID: "", + ImageFiles: []string{"unrelated.jpg"}, } _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) @@ -248,6 +250,46 @@ var _ = Describe("Album Artwork Reader", func() { Expect(repo.getCallCount).To(Equal(1)) }) + It("includes top-level album folder for multi-disc albums", func() { + // Album folder directly under artist root, with disc subfolders + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Album", + Name: "Disc1", + ParentID: "albumFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{"folder.jpg"}, + }, + { + ID: "folder2", + Path: "Album", + Name: "Disc2", + ParentID: "albumFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{"folder.jpg"}, + }, + } + repo.parentResult = &model.Folder{ + ID: "albumFolder", + Path: ".", + Name: "Album", + ParentID: "rootFolder", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"cover.jpg"}, + } + + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(*imagesUpdatedAt).To(Equal(expectedAt)) + Expect(imgFiles).To(HaveLen(3)) + Expect(imgFiles[0]).To(Equal("Album/cover.jpg")) + Expect(imgFiles[1]).To(Equal("Album/Disc1/folder.jpg")) + Expect(imgFiles[2]).To(Equal("Album/Disc2/folder.jpg")) + Expect(repo.getCallCount).To(Equal(1)) + }) + It("does not query parent for single-folder albums that already have images", func() { repo.result = []model.Folder{ { @@ -283,6 +325,7 @@ var _ = Describe("Album Artwork Reader", func() { ID: "albumFolder", Path: "Artist", Name: "Album", + ParentID: "artistFolder", ImagesUpdatedAt: expectedAt, ImageFiles: []string{"cover.jpg"}, }