From bb79d99c2d5ea82ad1506d7e880b7b99dcda3aa9 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 3 May 2026 00:26:57 -0400 Subject: [PATCH 1/7] 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"}, } From 8fd03d9d86478b708ebfb9bd29313273b9271f47 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 3 May 2026 01:52:14 -0400 Subject: [PATCH 2/7] =?UTF-8?q?fix:=20correct=20comment=20in=20test=20?= =?UTF-8?q?=E2=80=94=20album=20is=20under=20library=20root,=20not=20artist?= =?UTF-8?q?=20root?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/artwork/reader_album_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/artwork/reader_album_test.go b/core/artwork/reader_album_test.go index 15558d86c..1cf039bee 100644 --- a/core/artwork/reader_album_test.go +++ b/core/artwork/reader_album_test.go @@ -251,7 +251,7 @@ var _ = Describe("Album Artwork Reader", func() { }) It("includes top-level album folder for multi-disc albums", func() { - // Album folder directly under artist root, with disc subfolders + // Album folder directly under library root, with disc subfolders repo.result = []model.Folder{ { ID: "folder1", From d2456a326815268edd026b508674dcde571f48dc Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 3 May 2026 10:43:15 -0400 Subject: [PATCH 3/7] test: add ascii tree diagram to top-level album e2e test --- core/artwork/e2e/album_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/artwork/e2e/album_test.go b/core/artwork/e2e/album_test.go index 9de974339..02a3ca83a 100644 --- a/core/artwork/e2e/album_test.go +++ b/core/artwork/e2e/album_test.go @@ -121,6 +121,14 @@ 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() { + // Album/ (top-level folder, Path=".") + // ├── CD1/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // ├── CD2/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // └── cover.jpg ← should win (album-root) It("prefers the album-root cover.jpg", func() { conf.Server.CoverArtPriority = defaultCoverPriority setLayout(fstest.MapFS{ From 09ac8e72a9d2ea474617af4326b87aefc65e5624 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 3 May 2026 10:47:09 -0400 Subject: [PATCH 4/7] test: replace internal bug references with issue link in e2e comments Signed-off-by: Deluan --- core/artwork/e2e/album_test.go | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/core/artwork/e2e/album_test.go b/core/artwork/e2e/album_test.go index 02a3ca83a..e765e1b1b 100644 --- a/core/artwork/e2e/album_test.go +++ b/core/artwork/e2e/album_test.go @@ -37,15 +37,15 @@ var _ = Describe("Album artwork resolution", func() { }) }) - // Bug 2 variant: cover.* basenames tie across album-root and per-disc folders; - // compareImageFiles' lexicographic full-path tiebreaker ranks disc-subfolder - // files first. + // https://github.com/navidrome/navidrome/issues/5376 + // cover.* basenames tie across album-root and per-disc folders; + // compareImageFiles must prefer shallower paths. When("a multi-disc album has a cover.jpg at the album root and per-disc covers", func() { // Artist/ // └── Album/ // ├── CD1/ // │ ├── 01 - Track.mp3 - // │ └── cover.jpg ← currently wins (bug) + // │ └── cover.jpg ← should not win // ├── CD2/ // │ ├── 01 - Track.mp3 // │ └── cover.jpg @@ -68,15 +68,15 @@ var _ = Describe("Album artwork resolution", func() { }) }) - // Bug 2: folder.jpg basenames tie across album-root and per-disc folders; - // the lexicographic full-path tiebreaker in compareImageFiles ranks - // "Artist/Album/CD1/folder.jpg" ahead of "Artist/Album/folder.jpg". + // https://github.com/navidrome/navidrome/issues/5376 + // folder.jpg basenames tie across album-root and per-disc folders; + // compareImageFiles must prefer shallower paths. When("a multi-disc album has folder.jpg at the album root AND in each disc subfolder", func() { // Artist/ // └── Album/ // ├── CD1/ // │ ├── 01 - Track.mp3 - // │ └── folder.jpg ← currently wins (bug) + // │ └── folder.jpg ← should not win // ├── CD2/ // │ ├── 01 - Track.mp3 // │ └── folder.jpg @@ -97,9 +97,8 @@ var _ = Describe("Album artwork resolution", func() { }) }) - // Bug 1: commonParentFolder's `len(folders) < 2` guard skips the parent-folder - // lookup whenever an album lives entirely under a single subfolder, so an - // album-root cover is never considered. + // https://github.com/navidrome/navidrome/issues/5376 + // Single-subfolder albums must still consider the parent folder's images. When("an album lives entirely under a single disc subfolder with cover.jpg at the parent", func() { // Artist/ // └── Album/ @@ -119,7 +118,7 @@ var _ = Describe("Album artwork resolution", func() { }) }) - // Reproduces https://github.com/navidrome/navidrome/issues/5456 + // 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() { // Album/ (top-level folder, Path=".") // ├── CD1/ From efdff0e2ce101b5f743795d591f7cb52cb65e887 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 3 May 2026 13:58:02 -0400 Subject: [PATCH 5/7] test: add e2e test matching reporter's exact library layout (#5456) Adds a deeply nested test (Genre/Artist/Album/Disc) with 12 discs using the reporter's actual folder names to verify artwork resolution works for non-top-level album folders too. --- core/artwork/e2e/disc_test.go | 55 +++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/core/artwork/e2e/disc_test.go b/core/artwork/e2e/disc_test.go index d22b83a5c..667079458 100644 --- a/core/artwork/e2e/disc_test.go +++ b/core/artwork/e2e/disc_test.go @@ -257,6 +257,61 @@ var _ = Describe("Disc artwork resolution", func() { }) // Reproduces https://github.com/navidrome/navidrome/issues/5456 + // Deeply nested layout matching the reporter's actual structure. + When("a deeply nested multi-disc album has cover.jpg and per-disc folder.jpg", func() { + // Genre/Artist/Album/ ← album root with cover.jpg + // ├── cover.jpg ← album-level cover + // ├── Disc 01 (Subtitle)/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg ← disc 1 art + // ├── Disc 02 (Subtitle)/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // └── ... (12 discs) + 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 + discNames := []string{ + "Disc 01 (Birth of the Dead - The Studio Sides)", + "Disc 02 (Birth of the Dead - The Live Sides)", + "Disc 03 (The Grateful Dead)", + "Disc 04 (Anthem of the Sun)", + "Disc 05 (Aoxomoxoa)", + "Disc 06 (Live; Dead)", + "Disc 07 (Workingman's Dead)", + "Disc 08 (American Beauty)", + "Disc 09 (Grateful Dead)", + "Disc 10 (Europe '72)", + "Disc 11 (Europe '72)", + "Disc 12 (History of the Grateful Dead, Volume One (Bear's Choice))", + } + layout := fstest.MapFS{ + "Pop; Rock/Grateful Dead/(2001) The Golden Road/cover.jpg": imageFile("album-root-cover"), + } + for i, name := range discNames { + discNum := i + 1 + prefix := fmt.Sprintf("Pop; Rock/Grateful Dead/(2001) The Golden Road/%s/", name) + layout[prefix+"01 - Track.mp3"] = trackFile(1, fmt.Sprintf("T%d", discNum), map[string]any{"disc": fmt.Sprintf("%d", discNum)}) + layout[prefix+"folder.jpg"] = imageFile(fmt.Sprintf("disc-%02d-folder", discNum)) + } + setLayout(layout) + scan() + + al := firstAlbum() + + Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root-cover"))) + + for i := range discNames { + discNum := i + 1 + discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, discNum), &al.UpdatedAt) + Expect(readArtwork(discID)).To(Equal(imageBytes(fmt.Sprintf("disc-%02d-folder", discNum))), + "disc %d should use its own folder.jpg", discNum) + } + }) + }) + + // https://github.com/navidrome/navidrome/issues/5456 + // Top-level album variant — album folder at library root (Path="."). 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 From 5bb4e9091156f7e7ecb6eeb119cbece0476ae288 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 4 May 2026 07:10:27 -0400 Subject: [PATCH 6/7] fix(artwork): include ImportedAt in artwork cache key to invalidate stale cache Reverts the Phase 3 UpdatedAt bump (which would change album.UpdatedAt semantics) and instead includes album.ImportedAt in the artwork cache key computation. Since ImportedAt is bumped to time.Now() on every album Put, any Phase 3 correction naturally invalidates cached artwork that was resolved mid-scan with incomplete folder data. --- core/artwork/reader_album.go | 8 +++++--- core/artwork/reader_disc.go | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index 0dbb88dab..680ce2349 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -53,10 +53,12 @@ func newAlbumArtworkReader(ctx context.Context, artwork *artwork, artID model.Ar lib: lib, } a.cacheKey.artID = artID - if a.updatedAt != nil && a.updatedAt.After(al.UpdatedAt) { + a.cacheKey.lastUpdate = al.UpdatedAt + if a.updatedAt != nil && a.updatedAt.After(a.cacheKey.lastUpdate) { a.cacheKey.lastUpdate = *a.updatedAt - } else { - a.cacheKey.lastUpdate = al.UpdatedAt + } + if al.ImportedAt.After(a.cacheKey.lastUpdate) { + a.cacheKey.lastUpdate = al.ImportedAt } return a, nil } diff --git a/core/artwork/reader_disc.go b/core/artwork/reader_disc.go index de0a765f0..9140d6f1e 100644 --- a/core/artwork/reader_disc.go +++ b/core/artwork/reader_disc.go @@ -105,10 +105,12 @@ func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID updatedAt: imagesUpdatedAt, } r.cacheKey.artID = artID - if r.updatedAt != nil && r.updatedAt.After(al.UpdatedAt) { + r.cacheKey.lastUpdate = al.UpdatedAt + if r.updatedAt != nil && r.updatedAt.After(r.cacheKey.lastUpdate) { r.cacheKey.lastUpdate = *r.updatedAt - } else { - r.cacheKey.lastUpdate = al.UpdatedAt + } + if al.ImportedAt.After(r.cacheKey.lastUpdate) { + r.cacheKey.lastUpdate = al.ImportedAt } return r, nil } From ad04cd4e4c2fc00c914dbf50df470bace5388010 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 4 May 2026 07:23:20 -0400 Subject: [PATCH 7/7] fix(artwork): simplify lastUpdate logic using TimeNewest utility Signed-off-by: Deluan --- core/artwork/reader_album.go | 10 ++++------ core/artwork/reader_disc.go | 10 ++++------ 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index 680ce2349..73ba9b5ee 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -18,6 +18,7 @@ import ( "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/natural" ) @@ -53,12 +54,9 @@ func newAlbumArtworkReader(ctx context.Context, artwork *artwork, artID model.Ar lib: lib, } a.cacheKey.artID = artID - a.cacheKey.lastUpdate = al.UpdatedAt - if a.updatedAt != nil && a.updatedAt.After(a.cacheKey.lastUpdate) { - a.cacheKey.lastUpdate = *a.updatedAt - } - if al.ImportedAt.After(a.cacheKey.lastUpdate) { - a.cacheKey.lastUpdate = al.ImportedAt + a.cacheKey.lastUpdate = utils.TimeNewest(al.UpdatedAt, al.ImportedAt) + if imagesUpdateAt != nil { + a.cacheKey.lastUpdate = utils.TimeNewest(a.cacheKey.lastUpdate, *imagesUpdateAt) } return a, nil } diff --git a/core/artwork/reader_disc.go b/core/artwork/reader_disc.go index 9140d6f1e..0f648c987 100644 --- a/core/artwork/reader_disc.go +++ b/core/artwork/reader_disc.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils" ) type discArtworkReader struct { @@ -105,12 +106,9 @@ func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID updatedAt: imagesUpdatedAt, } r.cacheKey.artID = artID - r.cacheKey.lastUpdate = al.UpdatedAt - if r.updatedAt != nil && r.updatedAt.After(r.cacheKey.lastUpdate) { - r.cacheKey.lastUpdate = *r.updatedAt - } - if al.ImportedAt.After(r.cacheKey.lastUpdate) { - r.cacheKey.lastUpdate = al.ImportedAt + r.cacheKey.lastUpdate = utils.TimeNewest(al.UpdatedAt, al.ImportedAt) + if imagesUpdatedAt != nil { + r.cacheKey.lastUpdate = utils.TimeNewest(r.cacheKey.lastUpdate, *imagesUpdatedAt) } return r, nil }