From 89389fa21f0f01d39bdec45752c5936a7667db16 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 27 Jul 2026 20:03:25 -0400 Subject: [PATCH] fix(artwork): resolve artist folder for albums with disc subfolders Dropping the promoted album root from the artist reader's paths fixed the flat-album case but broke albums whose tracks live in disc subfolders: the promoted root was what kept the byte-wise longest common prefix on a directory boundary. Without it, CD1/CD2 siblings share the fragment "Album/CD", so the artist folder resolved one level too deep and album/artist.* won over artist.*. loadAlbumFoldersPaths now collapses each album to its own root before the paths are compared across albums, so a disc-split album contributes its album folder rather than each disc folder. Folders no album claims (the promoted parent) are still returned unchanged, keeping the album, disc and mediafile readers unaffected. The prefix math moves into commonDir, which appends a trailing separator so the comparison lands on segment boundaries - this also fixes sibling folders sharing a name prefix (Album/Album2). Adds e2e coverage for the case the branch fixes (artist.* with no album/ fallback, which failed before this branch), the disc-subfolder regression, and a multi-album guard that pins the scope boundary. --- core/artwork/e2e/artist_test.go | 67 +++++++++++++++++++++++++++++++++ core/artwork/reader_album.go | 28 +++++++++++++- core/artwork/reader_artist.go | 22 +++++++++-- 3 files changed, 112 insertions(+), 5 deletions(-) diff --git a/core/artwork/e2e/artist_test.go b/core/artwork/e2e/artist_test.go index d959b1d60..0ca525a17 100644 --- a/core/artwork/e2e/artist_test.go +++ b/core/artwork/e2e/artist_test.go @@ -80,6 +80,73 @@ var _ = Describe("Artist artwork resolution", func() { }) }) + When("ArtistArtPriority has no album/ fallback", func() { + // Artist/ + // ├── artist.jpg ← must resolve via the artist folder itself + // └── Album/ + // └── 01 - Track.mp3 + It("still resolves the artist folder and returns artist.*", func() { + conf.Server.ArtistArtPriority = "artist.*" + setLayout(fstest.MapFS{ + "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}), + "Artist/artist.jpg": imageFile("artist-folder"), + }) + scan() + + ar := soleArtist() + artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) + Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) + }) + }) + + When("the artist's only album has its tracks in disc subfolders", func() { + // Artist/ + // ├── artist.jpg ← wins (artist.* before album/artist.*) + // └── Album/ + // ├── artist.jpg + // ├── CD1/01 - Track.mp3 + // └── CD2/02 - Track.mp3 + It("prefers the artist-folder image over the album-folder one", func() { + conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external" + setLayout(fstest.MapFS{ + "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album"}), + "Artist/Album/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album"}), + "Artist/artist.jpg": imageFile("artist-folder"), + "Artist/Album/artist.jpg": imageFile("album-artist"), + }) + scan() + + ar := soleArtist() + artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) + Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) + }) + }) + + When("one album has disc subfolders and another sits at artist level", func() { + // Artist/ + // ├── artist.jpg ← wins + // ├── Album1/ + // │ ├── artist.jpg + // │ ├── CD1/01 - Track.mp3 + // │ └── CD2/02 - Track.mp3 + // └── Album2/03 - Track.mp3 + It("prefers the artist-folder image over the album-folder one", func() { + conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external" + setLayout(fstest.MapFS{ + "Artist/Album1/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album1"}), + "Artist/Album1/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album1"}), + "Artist/Album2/03 - Track.mp3": trackFile(3, "Track 3", map[string]any{"albumartist": "Artist", "album": "Album2"}), + "Artist/artist.jpg": imageFile("artist-folder"), + "Artist/Album1/artist.jpg": imageFile("album-artist"), + }) + scan() + + ar := soleArtist() + artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) + Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) + }) + }) + When("an artist has an uploaded image and a matching artist.* file", func() { // / // └── artwork/ diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index c34dc0c96..7ad099bc2 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -20,6 +20,7 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/natural" + "github.com/navidrome/navidrome/utils/slice" ) type albumArtworkReader struct { @@ -123,11 +124,36 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, includeParen } } + // Collapse each album to its own root so an album split into disc + // subfolders can't pull a shared prefix below the artist folder. + pathByID := slice.ToMap(folders, func(f model.Folder) (string, string) { + return f.ID, f.AbsolutePath() + }) var paths []string + var claimedIDs []string + for _, album := range albums { + var albumPaths []string + for _, fid := range album.FolderIDs { + if p, ok := pathByID[fid]; ok { + albumPaths = append(albumPaths, p) + claimedIDs = append(claimedIDs, fid) + } + } + if len(albumPaths) > 0 { + paths = append(paths, commonDir(albumPaths)) + } + } + // Folders no album claims (e.g. the promoted parent) stay as-is. + claimed := slice.ToSet(claimedIDs) + for _, f := range folders { + if _, ok := claimed[f.ID]; !ok { + paths = append(paths, f.AbsolutePath()) + } + } + var imgFiles []string var updatedAt time.Time for _, f := range folders { - paths = append(paths, f.AbsolutePath()) if f.ImagesUpdatedAt.After(updatedAt) { updatedAt = f.ImagesUpdatedAt } diff --git a/core/artwork/reader_artist.go b/core/artwork/reader_artist.go index 5226e7ed2..68e2e78d5 100644 --- a/core/artwork/reader_artist.go +++ b/core/artwork/reader_artist.go @@ -19,6 +19,7 @@ import ( "github.com/navidrome/navidrome/core/external" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/slice" "github.com/navidrome/navidrome/utils/str" ) @@ -232,17 +233,30 @@ func escapeGlobLiteral(s string) string { return b.String() } +// commonDir returns the deepest directory containing all paths. Trailing +// separators keep the comparison on segment boundaries, so a shared name +// fragment (".../Album" and ".../Album2") is never read as a shared directory. +func commonDir(paths []string) string { + sep := string(filepath.Separator) + common := str.LongestCommonPrefix(slice.Map(paths, func(p string) string { return p + sep })) + if !strings.HasSuffix(common, sep) { + common, _ = filepath.Split(common) + } + return filepath.Clean(common) +} + func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albums, paths []string) (string, time.Time, error) { if len(albums) == 0 { return "", time.Time{}, nil } libID := albums[0].LibraryID // Just need one of the albums, as they should all be in the same Library - for now! TODO: Support multiple libraries - folderPath := str.LongestCommonPrefix(paths) - if !strings.HasSuffix(folderPath, string(filepath.Separator)) { - folderPath, _ = filepath.Split(folderPath) + // paths holds one root per album, so their common directory is already the + // artist folder; a single root is the album itself, so climb one level. + folderPath := commonDir(paths) + if len(paths) < 2 { + folderPath = filepath.Dir(folderPath) } - folderPath = filepath.Dir(folderPath) // Manipulate the path to get the folder ID // TODO: This is a bit hacky, but it's the easiest way to get the folder ID, ATM