atridad/Discodrome
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

feat: use Cover Art Archive for albums with MusicBrainz IDs #27

Merged
deluan merged 9 commits from feat/cover-art-archive into main 2026-03-20 18:34:11 -06:00
Conversation 7 Commits 9 Files Changed 7
+93 -12
4 changed files with 93 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 0f7ede580e - Show all commits
coverart.go
+14 -4
View File
@@ -7,6 +7,7 @@ import (
gemini-code-assist[bot] commented 2026-03-20 18:24:36 -06:00 (Migrated from github.com)
Review
Copy Link
Copy Source

medium

To improve maintainability and avoid magic numbers, it's better to define 5000 as a constant, for example const caaRequestTimeoutMs = 5000, and use it here. You could define it with the other constants in this file.

![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg) To improve maintainability and avoid magic numbers, it's better to define `5000` as a constant, for example `const caaRequestTimeoutMs = 5000`, and use it here. You could define it with the other constants in this file.
copilot-pull-request-reviewer[bot] commented 2026-03-20 18:28:21 -06:00 (Migrated from github.com)
Review
Copy Link
Copy Source

getImageViaCoverArt caches an empty string for caaCacheTTLMiss whenever imageURL == "", including when headCoverArt failed due to transient conditions (network error, 429/5xx, etc.). This can suppress retries for 4 hours after a temporary outage or rate-limit response. Consider distinguishing “definite miss” (e.g., 404) from “request failure” and only caching the former (or using a much shorter TTL for failures).

`getImageViaCoverArt` caches an empty string for `caaCacheTTLMiss` whenever `imageURL == ""`, including when `headCoverArt` failed due to transient conditions (network error, 429/5xx, etc.). This can suppress retries for 4 hours after a temporary outage or rate-limit response. Consider distinguishing “definite miss” (e.g., 404) from “request failure” and only caching the former (or using a much shorter TTL for failures).
deluan commented 2026-03-20 18:33:03 -06:00 (Migrated from github.com)
Review
Copy Link
Copy Source

Already extracted to caaTimeOut constant in a subsequent commit.

Already extracted to `caaTimeOut` constant in a subsequent commit.
deluan commented 2026-03-20 18:33:28 -06:00 (Migrated from github.com)
Review
Copy Link
Copy Source

Fixed in 76d4074. headCoverArt now returns (url, definitive) — transient failures (network errors, unexpected status codes) return definitive=false and are not cached, allowing immediate retries on the next track event. Only 404s are cached as misses.

Fixed in 76d4074. `headCoverArt` now returns `(url, definitive)` — transient failures (network errors, unexpected status codes) return `definitive=false` and are not cached, allowing immediate retries on the next track event. Only 404s are cached as misses.
"github.com/navidrome/navidrome/plugins/pdk/go/host"
"github.com/navidrome/navidrome/plugins/pdk/go/pdk"
"github.com/navidrome/navidrome/plugins/pdk/go/scrobbler"
)
// Configuration key for uguu.se image hosting
@@ -95,13 +96,22 @@ type uguuResponse struct {
} `json:"files"`
}
// getImageURL retrieves the track artwork URL, optionally uploading to uguu.se.
func getImageURL(username, trackID string) string {
// getImageURL retrieves the track artwork URL, checking CAA first if enabled,
// then uguu.se, then direct Navidrome URL.
func getImageURL(username string, track scrobbler.TrackInfo) string {
caaEnabled, _ := pdk.GetConfig(caaEnabledKey)
if caaEnabled == "true" {
if url := getImageViaCoverArt(track.MBZAlbumID, track.MBZReleaseGroupID); url != "" {
return url
}
}
uguuEnabled, _ := pdk.GetConfig(uguuEnabledKey)
if uguuEnabled == "true" {
return getImageViaUguu(username, trackID)
return getImageViaUguu(username, track.ID)
}
return getImageDirect(trackID)
return getImageDirect(track.ID)
}
// getImageDirect returns the artwork URL directly from Navidrome (current behavior).
coverart_test.go
+76 -7
View File
@@ -5,6 +5,7 @@ import (
"github.com/navidrome/navidrome/plugins/pdk/go/host"
"github.com/navidrome/navidrome/plugins/pdk/go/pdk"
"github.com/navidrome/navidrome/plugins/pdk/go/scrobbler"
"github.com/stretchr/testify/mock"
. "github.com/onsi/ginkgo/v2"
@@ -80,40 +81,42 @@ var _ = Describe("getImageURL", func() {
Describe("uguu disabled (default)", func() {
BeforeEach(func() {
pdk.PDKMock.On("GetConfig", caaEnabledKey).Return("", false)
pdk.PDKMock.On("GetConfig", uguuEnabledKey).Return("", false)
})
It("returns artwork URL directly", func() {
host.ArtworkMock.On("GetTrackUrl", "track1", int32(300)).Return("https://example.com/art.jpg", nil)
url := getImageURL("testuser", "track1")
url := getImageURL("testuser", scrobbler.TrackInfo{ID: "track1"})
Expect(url).To(Equal("https://example.com/art.jpg"))
})
It("returns empty for localhost URL", func() {
host.ArtworkMock.On("GetTrackUrl", "track1", int32(300)).Return("http://localhost:4533/art.jpg", nil)
url := getImageURL("testuser", "track1")
url := getImageURL("testuser", scrobbler.TrackInfo{ID: "track1"})
Expect(url).To(BeEmpty())
})
It("returns empty when artwork fetch fails", func() {
host.ArtworkMock.On("GetTrackUrl", "track1", int32(300)).Return("", errors.New("not found"))
url := getImageURL("testuser", "track1")
url := getImageURL("testuser", scrobbler.TrackInfo{ID: "track1"})
Expect(url).To(BeEmpty())
})
})
Describe("uguu enabled", func() {
BeforeEach(func() {
pdk.PDKMock.On("GetConfig", caaEnabledKey).Return("", false)
pdk.PDKMock.On("GetConfig", uguuEnabledKey).Return("true", true)
})
It("returns cached URL when available", func() {
host.CacheMock.On("GetString", "uguu.artwork.track1").Return("https://a.uguu.se/cached.jpg", true, nil)
url := getImageURL("testuser", "track1")
url := getImageURL("testuser", scrobbler.TrackInfo{ID: "track1"})
Expect(url).To(Equal("https://a.uguu.se/cached.jpg"))
})
@@ -133,7 +136,7 @@ var _ = Describe("getImageURL", func() {
// Mock cache set
host.CacheMock.On("SetString", "uguu.artwork.track1", "https://a.uguu.se/uploaded.jpg", int64(9000)).Return(nil)
url := getImageURL("testuser", "track1")
url := getImageURL("testuser", scrobbler.TrackInfo{ID: "track1"})
Expect(url).To(Equal("https://a.uguu.se/uploaded.jpg"))
host.CacheMock.AssertCalled(GinkgoT(), "SetString", "uguu.artwork.track1", "https://a.uguu.se/uploaded.jpg", int64(9000))
})
@@ -143,7 +146,7 @@ var _ = Describe("getImageURL", func() {
host.SubsonicAPIMock.On("CallRaw", "/getCoverArt?u=testuser&id=track1&size=300").
Return("", []byte(nil), errors.New("fetch failed"))
url := getImageURL("testuser", "track1")
url := getImageURL("testuser", scrobbler.TrackInfo{ID: "track1"})
Expect(url).To(BeEmpty())
})
@@ -156,10 +159,76 @@ var _ = Describe("getImageURL", func() {
return req.URL == "https://uguu.se/upload"
})).Return(&host.HTTPResponse{StatusCode: 500, Body: []byte(`{"success":false}`)}, nil)
url := getImageURL("testuser", "track1")
url := getImageURL("testuser", scrobbler.TrackInfo{ID: "track1"})
Expect(url).To(BeEmpty())
})
})
Describe("CAA enabled", func() {
BeforeEach(func() {
pdk.PDKMock.ExpectedCalls = nil
pdk.PDKMock.On("Log", mock.Anything, mock.Anything).Maybe()
pdk.PDKMock.On("GetConfig", caaEnabledKey).Return("true", true)
pdk.PDKMock.On("GetConfig", uguuEnabledKey).Return("", false)
})
It("returns CAA URL when release HEAD succeeds", func() {
host.CacheMock.On("GetString", "caa.artwork.album-id").Return("", false, nil)
host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool {
return req.URL == "https://coverartarchive.org/release/album-id/front-500"
})).Return(&host.HTTPResponse{
StatusCode: 307,
Headers: map[string]string{"Location": "https://archive.org/art.jpg"},
}, nil)
host.CacheMock.On("SetString", "caa.artwork.album-id", "https://archive.org/art.jpg", int64(86400)).Return(nil)
url := getImageURL("testuser", scrobbler.TrackInfo{ID: "track1", MBZAlbumID: "album-id", MBZReleaseGroupID: "rg-id"})
Expect(url).To(Equal("https://archive.org/art.jpg"))
host.ArtworkMock.AssertNotCalled(GinkgoT(), "GetTrackUrl", mock.Anything, mock.Anything)
host.SubsonicAPIMock.AssertNotCalled(GinkgoT(), "CallRaw", mock.Anything)
})
It("falls through to direct when CAA misses and uguu is disabled", func() {
host.CacheMock.On("GetString", "caa.artwork.album-id").Return("", false, nil)
host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool {
return req.URL == "https://coverartarchive.org/release/album-id/front-500"
})).Return(&host.HTTPResponse{StatusCode: 404}, nil)
host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool {
return req.URL == "https://coverartarchive.org/release-group/rg-id/front-500"
})).Return(&host.HTTPResponse{StatusCode: 404}, nil)
host.CacheMock.On("SetString", "caa.artwork.album-id", "", int64(14400)).Return(nil)
host.ArtworkMock.On("GetTrackUrl", "track1", int32(300)).Return("https://example.com/art.jpg", nil)
url := getImageURL("testuser", scrobbler.TrackInfo{ID: "track1", MBZAlbumID: "album-id", MBZReleaseGroupID: "rg-id"})
Expect(url).To(Equal("https://example.com/art.jpg"))
})
It("falls through to uguu when CAA misses and uguu is enabled", func() {
pdk.PDKMock.ExpectedCalls = nil
pdk.PDKMock.On("Log", mock.Anything, mock.Anything).Maybe()
pdk.PDKMock.On("GetConfig", caaEnabledKey).Return("true", true)
pdk.PDKMock.On("GetConfig", uguuEnabledKey).Return("true", true)
host.CacheMock.On("GetString", "caa.artwork.rg.rg-id").Return("", false, nil)
host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool {
return req.URL == "https://coverartarchive.org/release-group/rg-id/front-500"
})).Return(&host.HTTPResponse{StatusCode: 404}, nil)
host.CacheMock.On("SetString", "caa.artwork.rg.rg-id", "", int64(14400)).Return(nil)
host.CacheMock.On("GetString", "uguu.artwork.track1").Return("https://a.uguu.se/cached.jpg", true, nil)
url := getImageURL("testuser", scrobbler.TrackInfo{ID: "track1", MBZReleaseGroupID: "rg-id"})
Expect(url).To(Equal("https://a.uguu.se/cached.jpg"))
})
It("skips CAA when no MBZ IDs are present", func() {
host.ArtworkMock.On("GetTrackUrl", "track1", int32(300)).Return("https://example.com/art.jpg", nil)
url := getImageURL("testuser", scrobbler.TrackInfo{ID: "track1"})
Expect(url).To(Equal("https://example.com/art.jpg"))
host.HTTPMock.AssertNotCalled(GinkgoT(), "Send", mock.Anything)
})
})
})
var _ = Describe("getImageViaCoverArt", func() {
main.go
+1 -1
View File
2
 @@ -193,7 +193,7 @@ func (p *discordPlugin) NowPlaying(input scrobbler.NowPlayingRequest) error {
End: endTime,
},
Assets: activityAssets{
LargeImage: getImageURL(input.Username, input.Track.ID),
LargeImage: getImageURL(input.Username, input.Track),
LargeText: input.Track.Album,
LargeURL: spotifyURL,
SmallImage: navidromeLogoURL,
main_test.go
+2
View File
@@ -122,6 +122,7 @@ var _ = Describe("discordPlugin", func() {
pdk.PDKMock.On("GetConfig", clientIDKey).Return("test-client-id", true)
pdk.PDKMock.On("GetConfig", usersKey).Return(`[{"username":"testuser","token":"test-token"}]`, true)
pdk.PDKMock.On("GetConfig", uguuEnabledKey).Return("", false)
pdk.PDKMock.On("GetConfig", caaEnabledKey).Return("", false)
pdk.PDKMock.On("GetConfig", activityNameKey).Return("", false)
pdk.PDKMock.On("GetConfig", spotifyLinksKey).Return("", false)
@@ -174,6 +175,7 @@ var _ = Describe("discordPlugin", func() {
pdk.PDKMock.On("GetConfig", clientIDKey).Return("test-client-id", true)
pdk.PDKMock.On("GetConfig", usersKey).Return(`[{"username":"testuser","token":"test-token"}]`, true)
pdk.PDKMock.On("GetConfig", uguuEnabledKey).Return("", false)
pdk.PDKMock.On("GetConfig", caaEnabledKey).Return("", false)
pdk.PDKMock.On("GetConfig", activityNameKey).Return(configValue, configExists)
pdk.PDKMock.On("GetConfig", spotifyLinksKey).Return("", false)