From 3290759e625025d7626b62329b49bd59b8b086df Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 3 Sep 2026 22:48:23 -0400 Subject: [PATCH 01/13] refactor(artwork): move the explain verdict helpers into core/artwork --- cmd/artwork.go | 84 ++----------------------- cmd/artwork_test.go | 119 ----------------------------------- core/artwork/explain.go | 85 +++++++++++++++++++++++++ core/artwork/explain_test.go | 82 ++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 197 deletions(-) create mode 100644 core/artwork/explain.go create mode 100644 core/artwork/explain_test.go diff --git a/cmd/artwork.go b/cmd/artwork.go index 9a64cd8c3..0ab391425 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -746,81 +746,8 @@ func artworkKindAndID(ctx context.Context, ds model.DataStore, arg string) (mode return model.ArtworkID{Kind: kind, ID: arg}, nil } -// explainAgents accounts for every configured agent: one the CLI cannot construct (a plugin, or a -// built-in missing its credentials) never reaches the Chain, so the raw list alone overstates it. -func explainAgents(configured string, available []string) string { - if strings.TrimSpace(configured) == "" { - return "(none)" - } - var unavailable bool - names := slice.Map(strings.Split(configured, ","), func(name string) string { - name = strings.TrimSpace(name) - if slices.Contains(available, name) { - return name - } - unavailable = true - return name + "*" - }) - line := strings.Join(names, ", ") - if unavailable { - line += " (* not available to the CLI)" - } - return line -} - -// availableImageAgents names the agents that can actually supply an image for kind. -func availableImageAgents(ds model.DataStore, mgr *plugins.Manager, kind model.Kind) []string { - ag := agents.GetAgents(ds, mgr) - if kind == model.KindArtistArtwork { - return slice.Map(ag.ArtistImageAgents(), func(a agents.ArtistImageAgent) string { return a.Name }) - } - return slice.Map(ag.AlbumImageAgents(), func(a agents.AlbumImageAgent) string { return a.Name }) -} - -// explainResult states the verdict of the walk. A skipped or failed external tier, or a local -// candidate that would not open, leaves the outcome unknown: nothing observed that there is no artwork. -func explainResult(source string, steps []artwork.TraceStep) string { - if source != "" { - for _, s := range steps { - if s.Outcome == artwork.OutcomeHit { - break - } - // An external winner discards the earlier error, so the resolver settles it with no retry. - if s.Outcome == artwork.OutcomeError && strings.HasPrefix(s.Candidate, artwork.ExternalPrefix) && - !strings.HasPrefix(source, artwork.ExternalPrefix) { - return "resolved from " + source + - " (indeterminate: a higher-priority external lookup failed; this may resolve differently on a retry)" - } - } - return "resolved from " + source - } - for _, s := range steps { - switch { - case s.Outcome == artwork.OutcomeError && strings.HasPrefix(s.Candidate, artwork.ExternalPrefix): - return "indeterminate (an external lookup failed; the item may resolve on a later attempt)" - // A stage error or an unreadable candidate means a source was found but not processed; the - // worker retries rather than settling absent, so neither reads as a clean miss. - case s.Outcome == artwork.OutcomeError, s.Outcome == artwork.OutcomeUnreadable: - return "indeterminate (a candidate was found but could not be processed; the worker retries rather than settling absent)" - } - } - return "not resolved" -} - -// explainConfig names the setting that decides where a kind's artwork comes from, and its value. -func explainConfig(kind model.Kind) (name, value string) { - switch kind { - case model.KindArtistArtwork: - return "ArtistArtPriority", conf.Server.ArtistArtPriority - case model.KindAlbumArtwork: - return "CoverArtPriority", conf.Server.CoverArtPriority - case model.KindDiscArtwork: - return "DiscArtPriority", conf.Server.DiscArtPriority - case model.KindMediaFileArtwork: - return "EnableMediaFileCoverArt", strconv.FormatBool(conf.Server.EnableMediaFileCoverArt) - } - return "", "" -} +// cliUnavailableNote marks agents the CLI cannot construct; a running server loads them all. +const cliUnavailableNote = " (* not available to the CLI)" type explainReport struct { kind model.Kind @@ -912,7 +839,7 @@ func formatExplain(rep explainReport) string { } fmt.Fprintln(w, "\nConfig") - if setting, value := explainConfig(rep.kind); setting == "" { + if setting, value := artwork.ConfigFor(rep.kind); setting == "" { fmt.Fprintln(w, " (no artwork source configuration applies)") } else { fmt.Fprintf(w, " %s:\t%s\n", setting, value) @@ -948,7 +875,7 @@ func formatExplain(rep explainReport) string { case unrecorded: fmt.Fprintln(w, " not evaluated (nothing recorded; re-run with --live to walk the chain now)") default: - fmt.Fprintf(w, " %s\n", explainResult(rep.source, rep.steps)) + fmt.Fprintf(w, " %s\n", artwork.Result(rep.source, rep.steps)) } w.Flush() @@ -1002,7 +929,8 @@ func runExplain(ctx context.Context, args []string) { if kind == model.KindArtistArtwork || kind == model.KindAlbumArtwork { mgr := loadPluginAgents(ctx, explainLive) defer func() { _ = mgr.Stop() }() - rep.agents = explainAgents(conf.Server.Agents, availableImageAgents(ds, mgr, kind)) + rep.agents = artwork.FormatAgents(conf.Server.Agents, + artwork.ImageAgentNames(agents.GetAgents(ds, mgr), kind), cliUnavailableNote) } switch { case rep.walked: diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index 71b450914..dc33f9e13 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -122,106 +122,6 @@ var _ = Describe("resolveArtworkTargets", func() { }) }) -var _ = Describe("explainResult", func() { - It("reports the winning source", func() { - steps := []artwork.TraceStep{{Candidate: "folder", Outcome: "hit", Detail: "/music/a.jpg"}} - Expect(explainResult("folder", steps)).To(ContainSubstring("resolved from folder")) - }) - - It("reports not resolved when every candidate was tried and missed", func() { - steps := []artwork.TraceStep{ - {Candidate: "artist.*", Outcome: "miss"}, - {Candidate: "external:deezer", Outcome: "miss"}, - } - Expect(explainResult("", steps)).To(Equal("not resolved")) - }) - - It("reports indeterminate when a local candidate exists but could not be read", func() { - steps := []artwork.TraceStep{ - {Candidate: "cover.*", Outcome: "miss"}, - {Candidate: "embedded", Outcome: "unreadable"}, - } - Expect(explainResult("", steps)).To(ContainSubstring("indeterminate"), - "the worker retries an unreadable candidate instead of settling absent, so this is not a clean miss") - }) - - It("reports indeterminate when a processing stage errored after a candidate was found", func() { - steps := []artwork.TraceStep{ - {Candidate: "cover.*", Outcome: "hit", Detail: "/music/cover.jpg"}, - {Candidate: "store", Outcome: "error", Detail: "disk full"}, - } - Expect(explainResult("", steps)).To(ContainSubstring("indeterminate"), - "a stage error is a processing failure the worker retries, not a definitive miss") - }) - - It("does not qualify a hit that an earlier unreadable candidate preceded", func() { - // chainState.try stamps only the external error onto a hit and drops the local one, so the - // worker settles this as found; warning about it would be a false alarm. - steps := []artwork.TraceStep{ - {Candidate: "embedded", Outcome: "unreadable"}, - {Candidate: "cover.*", Outcome: "hit", Detail: "/music/cover.jpg"}, - } - Expect(explainResult("folder", steps)).To(Equal("resolved from folder")) - }) - - It("reports indeterminate when an external lookup failed transiently", func() { - steps := []artwork.TraceStep{ - {Candidate: "artist.*", Outcome: "miss"}, - {Candidate: "external:deezer", Outcome: "error", Detail: "context deadline exceeded"}, - } - Expect(explainResult("", steps)).To(ContainSubstring("indeterminate"), - "a failed network call is not evidence that the item has no artwork") - }) - - It("qualifies a win a failed higher-priority external lookup could have taken", func() { - steps := []artwork.TraceStep{ - {Candidate: "external:deezer", Outcome: "error", Detail: "context deadline exceeded"}, - {Candidate: "artist.*", Outcome: "hit", Detail: "/music/artist.jpg"}, - } - res := explainResult("artist.*", steps) - Expect(res).To(ContainSubstring("resolved from artist.*")) - Expect(res).To(ContainSubstring("indeterminate"), - "the resolver serves this hit but retries later, so the winner is provisional") - }) - - It("does not qualify an external win that followed a failed external lookup", func() { - steps := []artwork.TraceStep{ - {Candidate: "external:deezer", Outcome: "error", Detail: "context deadline exceeded"}, - {Candidate: "external:lastfm", Outcome: "hit", Detail: "http://img"}, - } - Expect(explainResult("external:lastfm", steps)).To(Equal("resolved from external:lastfm"), - "a later agent supplying the image discards the earlier error, so there is no retry to warn about") - }) - - It("does not qualify a win that outranked the failed external lookup", func() { - steps := []artwork.TraceStep{ - {Candidate: "artist.*", Outcome: "hit"}, - {Candidate: "external:deezer", Outcome: "error", Detail: "context deadline exceeded"}, - } - Expect(explainResult("artist.*", steps)).To(Equal("resolved from artist.*")) - }) -}) - -var _ = Describe("explainAgents", func() { - It("accounts for every configured agent, marking the ones the CLI could not use", func() { - out := explainAgents("artist-nfo-metadata,apple-music,deezer,lastfm", []string{"deezer"}) - for _, name := range []string{"artist-nfo-metadata", "apple-music", "deezer", "lastfm"} { - Expect(out).To(ContainSubstring(name), - "a configured agent missing from this line reads as if it had never been configured") - } - Expect(out).To(ContainSubstring("not available to the CLI")) - }) - - It("does not mark anything when every configured agent is available", func() { - out := explainAgents("deezer, lastfm", []string{"lastfm", "deezer"}) - Expect(out).To(Equal("deezer, lastfm")) - }) - - It("reports an empty configuration as none, not as an unavailable agent", func() { - Expect(explainAgents("", nil)).To(Equal("(none)")) - }) -}) - var _ = Describe("formatExplain", func() { var rep explainReport @@ -404,25 +304,6 @@ var _ = Describe("formatExplain", func() { }) }) -var _ = Describe("explainConfig", func() { - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - conf.Server.DiscArtPriority = "cover.jpg" - conf.Server.EnableMediaFileCoverArt = true - }) - - DescribeTable("names the setting that decides where a kind's artwork comes from", - func(kind model.Kind, setting, value string) { - gotSetting, gotValue := explainConfig(kind) - Expect(gotSetting).To(Equal(setting)) - Expect(gotValue).To(Equal(value)) - }, - Entry("disc", model.KindDiscArtwork, "DiscArtPriority", "cover.jpg"), - Entry("media file", model.KindMediaFileArtwork, "EnableMediaFileCoverArt", "true"), - Entry("playlist has none", model.KindPlaylistArtwork, "", ""), - ) -}) - var _ = Describe("artwork refresh command", func() { It("requires at least one argument", func() { Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{})).To(HaveOccurred()) diff --git a/core/artwork/explain.go b/core/artwork/explain.go new file mode 100644 index 000000000..4c0d465c9 --- /dev/null +++ b/core/artwork/explain.go @@ -0,0 +1,85 @@ +package artwork + +import ( + "slices" + "strconv" + "strings" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/core/agents" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/slice" +) + +// Result states the verdict of the walk. A skipped or failed external tier, or a local candidate +// that would not open, leaves the outcome unknown: nothing observed that there is no artwork. +func Result(source string, steps []TraceStep) string { + if source != "" { + for _, s := range steps { + if s.Outcome == OutcomeHit { + break + } + // An external winner discards the earlier error, so the resolver settles it with no retry. + if s.Outcome == OutcomeError && strings.HasPrefix(s.Candidate, ExternalPrefix) && + !strings.HasPrefix(source, ExternalPrefix) { + return "resolved from " + source + + " (indeterminate: a higher-priority external lookup failed; this may resolve differently on a retry)" + } + } + return "resolved from " + source + } + for _, s := range steps { + switch { + case s.Outcome == OutcomeError && strings.HasPrefix(s.Candidate, ExternalPrefix): + return "indeterminate (an external lookup failed; the item may resolve on a later attempt)" + case s.Outcome == OutcomeError, s.Outcome == OutcomeUnreadable: + return "indeterminate (a candidate was found but could not be processed; the worker retries rather than settling absent)" + } + } + return "not resolved" +} + +// ConfigFor names the setting that decides where a kind's artwork comes from, and its value. +func ConfigFor(kind model.Kind) (setting, value string) { + switch kind { + case model.KindArtistArtwork: + return "ArtistArtPriority", conf.Server.ArtistArtPriority + case model.KindAlbumArtwork: + return "CoverArtPriority", conf.Server.CoverArtPriority + case model.KindDiscArtwork: + return "DiscArtPriority", conf.Server.DiscArtPriority + case model.KindMediaFileArtwork: + return "EnableMediaFileCoverArt", strconv.FormatBool(conf.Server.EnableMediaFileCoverArt) + } + return "", "" +} + +// ImageAgentNames names the agents that can actually supply an image for kind. +func ImageAgentNames(ag *agents.Agents, kind model.Kind) []string { + if kind == model.KindArtistArtwork { + return slice.Map(ag.ArtistImageAgents(), func(a agents.ArtistImageAgent) string { return a.Name }) + } + return slice.Map(ag.AlbumImageAgents(), func(a agents.AlbumImageAgent) string { return a.Name }) +} + +// FormatAgents accounts for every configured agent: one that cannot be constructed never reaches +// the chain, so the raw list alone overstates it. unavailableNote is appended only if some are. +func FormatAgents(configured string, available []string, unavailableNote string) string { + if strings.TrimSpace(configured) == "" { + return "(none)" + } + var unavailable bool + names := slice.Map(strings.Split(configured, ","), func(name string) string { + name = strings.TrimSpace(name) + if slices.Contains(available, name) { + return name + } + unavailable = true + return name + "*" + }) + line := strings.Join(names, ", ") + if unavailable { + line += unavailableNote + } + return line +} diff --git a/core/artwork/explain_test.go b/core/artwork/explain_test.go new file mode 100644 index 000000000..14df4bbbf --- /dev/null +++ b/core/artwork/explain_test.go @@ -0,0 +1,82 @@ +package artwork_test + +import ( + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/core/artwork" + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Result", func() { + It("reports the source when one was found", func() { + steps := []artwork.TraceStep{{Candidate: "cover.*", Outcome: artwork.OutcomeHit}} + Expect(artwork.Result("folder", steps)).To(Equal("resolved from folder")) + }) + + It("flags a local win that a failed external lookup could have outranked", func() { + steps := []artwork.TraceStep{ + {Candidate: artwork.ExternalPrefix + "deezer", Outcome: artwork.OutcomeError, Detail: "timeout"}, + {Candidate: "cover.*", Outcome: artwork.OutcomeHit}, + } + Expect(artwork.Result("folder", steps)).To(ContainSubstring("indeterminate")) + }) + + It("does not flag an external winner", func() { + steps := []artwork.TraceStep{ + {Candidate: artwork.ExternalPrefix + "lastfm", Outcome: artwork.OutcomeError}, + {Candidate: artwork.ExternalPrefix + "deezer", Outcome: artwork.OutcomeHit}, + } + Expect(artwork.Result(artwork.ExternalPrefix+"deezer", steps)). + To(Equal("resolved from external:deezer")) + }) + + It("is indeterminate when an external lookup failed and nothing resolved", func() { + steps := []artwork.TraceStep{{Candidate: artwork.ExternalPrefix + "deezer", Outcome: artwork.OutcomeError}} + Expect(artwork.Result("", steps)).To(ContainSubstring("an external lookup failed")) + }) + + It("is indeterminate when a candidate could not be processed", func() { + steps := []artwork.TraceStep{{Candidate: "cover.*", Outcome: artwork.OutcomeUnreadable}} + Expect(artwork.Result("", steps)).To(ContainSubstring("could not be processed")) + }) + + It("reports a clean miss", func() { + steps := []artwork.TraceStep{{Candidate: "cover.*", Outcome: artwork.OutcomeMiss}} + Expect(artwork.Result("", steps)).To(Equal("not resolved")) + }) +}) + +var _ = Describe("ConfigFor", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.ArtistArtPriority = "external" + }) + + It("names the setting for a kind that has one", func() { + setting, value := artwork.ConfigFor(model.KindArtistArtwork) + Expect(setting).To(Equal("ArtistArtPriority")) + Expect(value).To(Equal("external")) + }) + + It("returns nothing for a kind with no source configuration", func() { + setting, _ := artwork.ConfigFor(model.KindPlaylistArtwork) + Expect(setting).To(BeEmpty()) + }) +}) + +var _ = Describe("FormatAgents", func() { + It("reports none when nothing is configured", func() { + Expect(artwork.FormatAgents(" ", nil, "")).To(Equal("(none)")) + }) + + It("keeps the configured order and marks what is unavailable", func() { + got := artwork.FormatAgents("spotify, lastfm", []string{"lastfm"}, " (* missing)") + Expect(got).To(Equal("spotify*, lastfm (* missing)")) + }) + + It("omits the note when every configured agent is available", func() { + Expect(artwork.FormatAgents("lastfm", []string{"lastfm"}, " (* missing)")).To(Equal("lastfm")) + }) +}) From 0ab549e1b2af4ecbb196088e82f6df621478a241 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 3 Sep 2026 23:01:29 -0400 Subject: [PATCH 02/13] refactor(artwork): build the explain report in core/artwork Moves report construction (stored state, queue row, chain walk or recorded trace) out of the CLI and into artwork.Explain, so a future HTTP handler can reuse it. cmd/artwork.go keeps only text formatting. --- cmd/artwork.go | 145 +++++++++++-------------------- cmd/artwork_test.go | 74 ++++++++-------- core/artwork/explain.go | 99 +++++++++++++++++++++ core/artwork/explain_test.go | 87 +++++++++++++++++++ tests/mock_artwork_queue_repo.go | 12 +++ 5 files changed, 286 insertions(+), 131 deletions(-) diff --git a/cmd/artwork.go b/cmd/artwork.go index 0ab391425..5d4c8a04b 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -749,32 +749,6 @@ func artworkKindAndID(ctx context.Context, ds model.DataStore, arg string) (mode // cliUnavailableNote marks agents the CLI cannot construct; a running server loads them all. const cliUnavailableNote = " (* not available to the CLI)" -type explainReport struct { - kind model.Kind - id string - name string - stored *model.ItemArtwork - queued *model.ArtworkQueueItem - agents string - // steps is the chain walk: recorded when the item was resolved, or performed just now when walked. - steps []artwork.TraceStep - source string - walked bool - resolveErr error -} - -// explainChainOrigin says whether the operator is reading history or a walk performed just now, -// since the two can disagree after a config change. -func explainChainOrigin(rep explainReport) string { - if rep.walked { - return "walked now" - } - if rep.stored != nil { - return "recorded " + formatTime(rep.stored.AttemptedAt) - } - return "not recorded" -} - // writeSteps prints the trace rows. An empty last cell would end tabwriter's column block and // break the alignment, so a missing detail is rendered as a dash. func writeSteps(w io.Writer, indent string, steps []artwork.TraceStep) { @@ -793,89 +767,89 @@ func writeStepTable(w io.Writer, title string, steps []artwork.TraceStep) { writeSteps(w, " ", steps) } -func formatExplain(rep explainReport) string { +func formatExplain(rep artwork.ExplainReport) string { var sb strings.Builder w := newTabWriter(&sb) - explainable := artwork.Explainable(rep.kind) - stateful := artwork.KeepsState(rep.kind) - unrecorded := !rep.walked && rep.stored == nil + explainable := artwork.Explainable(rep.Kind) + stateful := artwork.KeepsState(rep.Kind) + unrecorded := !rep.Walked && rep.Stored == nil fmt.Fprintln(w, "Item") - fmt.Fprintf(w, " Kind:\t%s (%s)\n", rep.kind, rep.kind.Prefix()) - fmt.Fprintf(w, " ID:\t%s\n", rep.id) - fmt.Fprintf(w, " Name:\t%s\n", rep.name) + fmt.Fprintf(w, " Kind:\t%s (%s)\n", rep.Kind, rep.Kind.Prefix()) + fmt.Fprintf(w, " ID:\t%s\n", rep.ID) + fmt.Fprintf(w, " Name:\t%s\n", rep.Name) fmt.Fprintln(w, "\nStored") switch { case !stateful: - fmt.Fprintf(w, " (%s artwork is resolved on every request and never recorded)\n", rep.kind) - case rep.stored == nil: + fmt.Fprintf(w, " (%s artwork is resolved on every request and never recorded)\n", rep.Kind) + case rep.Stored == nil: fmt.Fprintln(w, " (no artwork state recorded)") default: - fmt.Fprintf(w, " Source:\t%s\n", displaySource(rep.stored.Source)) - fmt.Fprintf(w, " Hash:\t%s\n", cmp.Or(rep.stored.Hash, "(absent)")) - if rep.stored.SourcePath != "" { - fmt.Fprintf(w, " Source path:\t%s\n", rep.stored.SourcePath) + fmt.Fprintf(w, " Source:\t%s\n", displaySource(rep.Stored.Source)) + fmt.Fprintf(w, " Hash:\t%s\n", cmp.Or(rep.Stored.Hash, "(absent)")) + if rep.Stored.SourcePath != "" { + fmt.Fprintf(w, " Source path:\t%s\n", rep.Stored.SourcePath) } - fmt.Fprintf(w, " Attempted at:\t%s\n", formatTime(rep.stored.AttemptedAt)) + fmt.Fprintf(w, " Attempted at:\t%s\n", formatTime(rep.Stored.AttemptedAt)) } fmt.Fprintln(w, "\nQueue") switch { case !stateful: fmt.Fprintln(w, " (never queued)") - case rep.queued == nil: + case rep.Queued == nil: fmt.Fprintln(w, " (not queued)") default: - fmt.Fprintf(w, " Priority:\t%s (%d)\n", priorityName(rep.queued.Priority), rep.queued.Priority) - fmt.Fprintf(w, " Attempts:\t%d\n", rep.queued.Attempts) - fmt.Fprintf(w, " Retry at:\t%s\n", formatTime(rep.queued.RetryAt)) + fmt.Fprintf(w, " Priority:\t%s (%d)\n", priorityName(rep.Queued.Priority), rep.Queued.Priority) + fmt.Fprintf(w, " Attempts:\t%d\n", rep.Queued.Attempts) + fmt.Fprintf(w, " Retry at:\t%s\n", formatTime(rep.Queued.RetryAt)) } - if rep.queued != nil { - writeStepTable(w, "Last attempt failed", artwork.DecodeTrace(rep.queued.Trace, "")) + if rep.Queued != nil { + writeStepTable(w, "Last attempt failed", rep.LastAttemptFailed()) } - if rep.stored != nil { - writeStepTable(w, "Gave up after", artwork.DecodeTrace(rep.stored.LastFailure, "")) + if rep.Stored != nil { + writeStepTable(w, "Gave up after", rep.GaveUpAfter()) } fmt.Fprintln(w, "\nConfig") - if setting, value := artwork.ConfigFor(rep.kind); setting == "" { + if setting, value := artwork.ConfigFor(rep.Kind); setting == "" { fmt.Fprintln(w, " (no artwork source configuration applies)") } else { fmt.Fprintf(w, " %s:\t%s\n", setting, value) - if rep.agents != "" { - fmt.Fprintf(w, " Agents:\t%s\n", rep.agents) + if rep.Agents != "" { + fmt.Fprintf(w, " Agents:\t%s\n", rep.Agents) } } - fmt.Fprintf(w, "\nChain (%s)\n", explainChainOrigin(rep)) + fmt.Fprintf(w, "\nChain (%s)\n", rep.ChainOrigin()) switch { case !explainable: - fmt.Fprintf(w, " (%s artwork does not walk a priority chain)\n", rep.kind) + fmt.Fprintf(w, " (%s artwork does not walk a priority chain)\n", rep.Kind) case unrecorded: fmt.Fprintln(w, " (no resolution recorded yet; re-run with --live to walk the chain now)") - case !rep.walked && len(rep.steps) == 0 && rep.stored.Hash != "": + case !rep.Walked && len(rep.Steps) == 0 && rep.Stored.Hash != "": // A stored image with no chain can only predate trace recording: a recorded resolution that // found an image always records its winning candidate. fmt.Fprintln(w, " (this item was resolved before traces were recorded; re-run with --live)") - case !rep.walked && len(rep.steps) == 0: + case !rep.Walked && len(rep.Steps) == 0: // Absent with no chain: an empty priority list walked nothing, or a pre-tracing absent row. fmt.Fprintln(w, " (no candidates were recorded; re-run with --live to walk the chain now)") default: fmt.Fprintln(w, " CANDIDATE\tOUTCOME\tDETAIL") - writeSteps(w, " ", rep.steps) + writeSteps(w, " ", rep.Steps) } fmt.Fprintln(w, "\nResult") switch { - case rep.resolveErr != nil: - fmt.Fprintf(w, " resolution failed: %s\n", rep.resolveErr) + case rep.ResolveErr != nil: + fmt.Fprintf(w, " resolution failed: %s\n", rep.ResolveErr) case !explainable: fmt.Fprintln(w, " not evaluated (no chain was walked; see Stored above)") case unrecorded: fmt.Fprintln(w, " not evaluated (nothing recorded; re-run with --live to walk the chain now)") default: - fmt.Fprintf(w, " %s\n", artwork.Result(rep.source, rep.steps)) + fmt.Fprintf(w, " %s\n", rep.Result()) } w.Flush() @@ -905,46 +879,29 @@ func runExplain(ctx context.Context, args []string) { } kind, id := targets[0].Kind, targets[0].ID - name, err := artwork.ItemName(ctx, ds, kind, id) - if err != nil { - log.Fatal(ctx, "Item not found", "kind", kind, "id", id, err) + opts := artwork.ExplainOptions{UnavailableNote: cliUnavailableNote} + // Only artist and album reach an agent, and the load must precede the resolver, which reads the + // same manager. Leaving ag nil elsewhere avoids handing agents.GetAgents a not-yet-loaded manager. + var ag *agents.Agents + if kind == model.KindArtistArtwork || kind == model.KindAlbumArtwork { + mgr := loadPluginAgents(ctx, explainLive) + defer func() { _ = mgr.Stop() }() + ag = agents.GetAgents(ds, mgr) } - rep := explainReport{kind: kind, id: id, name: name} - if artwork.KeepsState(kind) { - rep.stored, err = ds.Artwork(ctx).GetItemArtwork(kind, id, model.ImageTypePrimary) - if err != nil && !errors.Is(err, model.ErrNotFound) { - log.Fatal(ctx, "Failed to read artwork state", "kind", kind, "id", id, err) - } - rep.queued, err = ds.ArtworkQueue(ctx).Get(kind, id, model.ImageTypePrimary) - if err != nil && !errors.Is(err, model.ErrNotFound) { - log.Fatal(ctx, "Failed to read the artwork queue", "kind", kind, "id", id, err) + // Disc artwork keeps no row, so it has no stored trace and can only be explained by walking now. + if explainLive || !artwork.KeepsState(kind) { + opts.Walk = func(t *artwork.ChainTrace) *artwork.TracingResolver { + return CreateArtworkResolver(t, explainLive) } } + rep, err := artwork.Explain(ctx, ds, ag, kind, id, opts) + if err != nil { + log.Fatal(ctx, "Failed to explain artwork", "kind", kind, "id", id, err) + } - // Disc artwork keeps no row, so it has no stored trace and can only be explained by walking now. - rep.walked = explainLive || !artwork.KeepsState(kind) - if artwork.Explainable(kind) { - // Only artist and album reach an agent, and the load must precede the resolver, which reads - // the same manager. - if kind == model.KindArtistArtwork || kind == model.KindAlbumArtwork { - mgr := loadPluginAgents(ctx, explainLive) - defer func() { _ = mgr.Stop() }() - rep.agents = artwork.FormatAgents(conf.Server.Agents, - artwork.ImageAgentNames(agents.GetAgents(ds, mgr), kind), cliUnavailableNote) - } - switch { - case rep.walked: - trace := &artwork.ChainTrace{} - rep.source, rep.resolveErr = CreateArtworkResolver(trace, explainLive).Resolve(ctx, kind, id) - rep.steps = trace.Steps() - case rep.stored != nil: - rep.steps = artwork.DecodeTrace(rep.stored.Trace, rep.stored.SourcePath) - rep.source = rep.stored.Source - } - } fmt.Print(formatExplain(rep)) // The steps taken before a failed walk are the diagnosis, so report them before exiting. - if rep.resolveErr != nil { - log.Fatal(ctx, "Failed to resolve artwork", "kind", kind, "id", id, rep.resolveErr) + if rep.ResolveErr != nil { + log.Fatal(ctx, "Failed to resolve artwork", "kind", kind, "id", id, rep.ResolveErr) } } diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index dc33f9e13..0be9d85ec 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -123,22 +123,22 @@ var _ = Describe("resolveArtworkTargets", func() { }) var _ = Describe("formatExplain", func() { - var rep explainReport + var rep artwork.ExplainReport BeforeEach(func() { DeferCleanup(configtest.SetupConfig()) conf.Server.ArtistArtPriority = "external, artist.*" - rep = explainReport{ - kind: model.KindArtistArtwork, - id: "ar-1", - name: "Radiohead", - agents: "lastfm,spotify", - walked: true, - steps: []artwork.TraceStep{ + rep = artwork.ExplainReport{ + Kind: model.KindArtistArtwork, + ID: "ar-1", + Name: "Radiohead", + Agents: "lastfm,spotify", + Walked: true, + Steps: []artwork.TraceStep{ {Candidate: "upload", Outcome: "skipped", Detail: "no uploaded image"}, {Candidate: "external:deezer", Outcome: "error", Detail: "context deadline exceeded"}, }, - source: "", + Source: "", } }) @@ -160,11 +160,11 @@ var _ = Describe("formatExplain", func() { It("prints the stored state and the queue row when they exist", func() { attempted := time.Date(2026, 8, 13, 10, 0, 0, 0, time.UTC) - rep.stored = &model.ItemArtwork{Source: "folder", Hash: "abc123", + rep.Stored = &model.ItemArtwork{Source: "folder", Hash: "abc123", SourcePath: "/music/cover.jpg", AttemptedAt: attempted} - rep.queued = &model.ArtworkQueueItem{Priority: model.ArtworkPriorityScan, Attempts: 2, + rep.Queued = &model.ArtworkQueueItem{Priority: model.ArtworkPriorityScan, Attempts: 2, RetryAt: attempted.Add(time.Hour)} - rep.source = "folder" + rep.Source = "folder" out := formatExplain(rep) Expect(out).To(ContainSubstring("abc123")) @@ -175,12 +175,12 @@ var _ = Describe("formatExplain", func() { }) It("marks a known-absent stored state instead of printing an empty hash", func() { - rep.stored = &model.ItemArtwork{AttemptedAt: time.Now()} + rep.Stored = &model.ItemArtwork{AttemptedAt: time.Now()} Expect(formatExplain(rep)).To(ContainSubstring("absent")) }) It("reports a failed walk as failed, not as unresolved", func() { - rep.resolveErr = errors.New("no such directory") + rep.ResolveErr = errors.New("no such directory") out := formatExplain(rep) Expect(out).To(ContainSubstring("resolution failed: no such directory")) @@ -190,9 +190,9 @@ var _ = Describe("formatExplain", func() { It("says a kind that does not walk a chain has no chain, without an empty table", func() { conf.Server.CoverArtPriority = "cover.*, embedded" - rep.kind = model.KindPlaylistArtwork - rep.steps = nil - rep.agents = "" + rep.Kind = model.KindPlaylistArtwork + rep.Steps = nil + rep.Agents = "" out := formatExplain(rep) Expect(out).To(ContainSubstring("does not walk a priority chain")) @@ -206,11 +206,11 @@ var _ = Describe("formatExplain", func() { It("says disc artwork keeps no state instead of reporting it as unresolved state", func() { conf.Server.DiscArtPriority = "cover.jpg, embedded" - rep = explainReport{ - kind: model.KindDiscArtwork, id: "al-1:2", name: "OK Computer (disc 2)", - steps: []artwork.TraceStep{{Candidate: "cover.jpg", Outcome: "hit", Detail: "/music/cover.jpg"}}, - source: "folder", - walked: true, + rep = artwork.ExplainReport{ + Kind: model.KindDiscArtwork, ID: "al-1:2", Name: "OK Computer (disc 2)", + Steps: []artwork.TraceStep{{Candidate: "cover.jpg", Outcome: "hit", Detail: "/music/cover.jpg"}}, + Source: "folder", + Walked: true, } out := formatExplain(rep) @@ -225,15 +225,15 @@ var _ = Describe("formatExplain", func() { Context("stored traces", func() { BeforeEach(func() { - rep.walked = false - rep.steps = nil + rep.Walked = false + rep.Steps = nil }) It("labels a recorded chain with when it was recorded, not as a walk done now", func() { attempted := time.Date(2026, 8, 13, 10, 0, 0, 0, time.UTC) - rep.stored = &model.ItemArtwork{Source: "folder", Hash: "abc", AttemptedAt: attempted} - rep.steps = []artwork.TraceStep{{Candidate: "artist.*", Outcome: "hit", Detail: "/music/artist.jpg"}} - rep.source = "folder" + rep.Stored = &model.ItemArtwork{Source: "folder", Hash: "abc", AttemptedAt: attempted} + rep.Steps = []artwork.TraceStep{{Candidate: "artist.*", Outcome: "hit", Detail: "/music/artist.jpg"}} + rep.Source = "folder" out := formatExplain(rep) Expect(out).To(ContainSubstring("Chain (recorded 2026-08-13T10:00:00Z)")) @@ -250,7 +250,7 @@ var _ = Describe("formatExplain", func() { }) It("distinguishes a row written before traces existed from one with an empty chain", func() { - rep.stored = &model.ItemArtwork{Source: "folder", Hash: "abc", AttemptedAt: time.Now()} + rep.Stored = &model.ItemArtwork{Source: "folder", Hash: "abc", AttemptedAt: time.Now()} Expect(formatExplain(rep)).To(ContainSubstring("resolved before traces were recorded")) }) @@ -258,7 +258,7 @@ var _ = Describe("formatExplain", func() { It("does not call an absent row with an empty recorded chain a pre-tracing row", func() { // An empty priority list records a real but empty chain and resolves absent; that is not a // legacy row, so it must not be reported as resolved before tracing existed. - rep.stored = &model.ItemArtwork{Source: "", Hash: "", AttemptedAt: time.Now()} + rep.Stored = &model.ItemArtwork{Source: "", Hash: "", AttemptedAt: time.Now()} out := formatExplain(rep) Expect(out).ToNot(ContainSubstring("resolved before traces were recorded")) @@ -267,9 +267,9 @@ var _ = Describe("formatExplain", func() { }) It("prints why the last attempt failed and why it gave up", func() { - rep.queued = &model.ArtworkQueueItem{Priority: model.ArtworkPriorityScan, Attempts: 3, + rep.Queued = &model.ArtworkQueueItem{Priority: model.ArtworkPriorityScan, Attempts: 3, Trace: `[{"c":"decode","o":"error","d":"bad header"}]`} - rep.stored = &model.ItemArtwork{Source: "folder", Hash: "abc", AttemptedAt: time.Now(), + rep.Stored = &model.ItemArtwork{Source: "folder", Hash: "abc", AttemptedAt: time.Now(), LastFailure: `[{"c":"read","o":"error","d":"i/o timeout"}]`} out := formatExplain(rep) @@ -288,10 +288,10 @@ var _ = Describe("formatExplain", func() { It("reports the setting that governs media file artwork", func() { conf.Server.EnableMediaFileCoverArt = false - rep = explainReport{ - kind: model.KindMediaFileArtwork, id: "mf-1", name: "Airbag", - walked: true, - steps: []artwork.TraceStep{ + rep = artwork.ExplainReport{ + Kind: model.KindMediaFileArtwork, ID: "mf-1", Name: "Airbag", + Walked: true, + Steps: []artwork.TraceStep{ {Candidate: "embedded", Outcome: "skipped", Detail: "EnableMediaFileCoverArt is off"}, }, } @@ -376,8 +376,8 @@ var _ = Describe("explain/reprocess source round trip", func() { Expect(art.PutItemArtwork(&model.ItemArtwork{ItemKind: model.KindArtistArtwork.Prefix(), ItemID: "ar-1", ImageType: model.ImageTypePrimary})).To(Succeed()) - shown := storedSource(formatExplain(explainReport{kind: model.KindArtistArtwork, id: "ar-1", - stored: &model.ItemArtwork{AttemptedAt: time.Now()}})) + shown := storedSource(formatExplain(artwork.ExplainReport{Kind: model.KindArtistArtwork, ID: "ar-1", + Stored: &model.ItemArtwork{AttemptedAt: time.Now()}})) q := ds.ArtworkQueue(ctx) Expect(validateSources(q, repositorySources([]string{shown}))).To(Succeed(), diff --git a/core/artwork/explain.go b/core/artwork/explain.go index 4c0d465c9..c6bba8553 100644 --- a/core/artwork/explain.go +++ b/core/artwork/explain.go @@ -1,9 +1,13 @@ package artwork import ( + "context" + "errors" + "fmt" "slices" "strconv" "strings" + "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core/agents" @@ -83,3 +87,98 @@ func FormatAgents(configured string, available []string, unavailableNote string) } return line } + +// ExplainOptions configures a single explain. The zero value reads history and never +// touches the network. +type ExplainOptions struct { + // Walk builds a resolver that records into the trace; nil reads the recorded trace instead. + Walk func(*ChainTrace) *TracingResolver + // UnavailableNote is appended to the agent line when a configured agent is missing. + UnavailableNote string +} + +// ExplainReport is everything known about how one item's artwork resolved. +type ExplainReport struct { + Kind model.Kind + ID string + Name string + Stored *model.ItemArtwork + Queued *model.ArtworkQueueItem + Steps []TraceStep + Source string + Agents string + Walked bool + ResolveErr error +} + +// Explain gathers everything known about how kind/id's artwork resolved: stored state, the +// queue row, and either the recorded trace or a fresh walk, depending on opts.Walk. +func Explain(ctx context.Context, ds model.DataStore, ag *agents.Agents, kind model.Kind, id string, + opts ExplainOptions) (ExplainReport, error) { + name, err := ItemName(ctx, ds, kind, id) + if err != nil { + return ExplainReport{}, err + } + rep := ExplainReport{Kind: kind, ID: id, Name: name} + + if KeepsState(kind) { + rep.Stored, err = ds.Artwork(ctx).GetItemArtwork(kind, id, model.ImageTypePrimary) + if err != nil && !errors.Is(err, model.ErrNotFound) { + return ExplainReport{}, fmt.Errorf("reading artwork state: %w", err) + } + rep.Queued, err = ds.ArtworkQueue(ctx).Get(kind, id, model.ImageTypePrimary) + if err != nil && !errors.Is(err, model.ErrNotFound) { + return ExplainReport{}, fmt.Errorf("reading the artwork queue: %w", err) + } + } + if !Explainable(kind) { + return rep, nil + } + if ag != nil && (kind == model.KindArtistArtwork || kind == model.KindAlbumArtwork) { + rep.Agents = FormatAgents(conf.Server.Agents, ImageAgentNames(ag, kind), opts.UnavailableNote) + } + + rep.Walked = opts.Walk != nil + switch { + case rep.Walked: + trace := &ChainTrace{} + rep.Source, rep.ResolveErr = opts.Walk(trace).Resolve(ctx, kind, id) + rep.Steps = trace.Steps() + case rep.Stored != nil: + rep.Steps = DecodeTrace(rep.Stored.Trace, rep.Stored.SourcePath) + rep.Source = rep.Stored.Source + } + return rep, nil +} + +// Result reports this report's verdict; see the package-level Result for the rules. +func (r ExplainReport) Result() string { return Result(r.Source, r.Steps) } + +// ChainOrigin says whether the report reads history or a walk performed just now, since the two +// can disagree after a config change. +func (r ExplainReport) ChainOrigin() string { + if r.Walked { + return "walked now" + } + if r.Stored != nil { + return "recorded " + r.Stored.AttemptedAt.Format(time.RFC3339) + } + return "not recorded" +} + +// LastAttemptFailed decodes why the queued row's last attempt failed, if there is one queued. +func (r ExplainReport) LastAttemptFailed() []TraceStep { + if r.Queued == nil { + return nil + } + return DecodeTrace(r.Queued.Trace, "") +} + +// GaveUpAfter decodes the trace of the attempt that exhausted the retry budget, if the stored +// state recorded one. +func (r ExplainReport) GaveUpAfter() []TraceStep { + if r.Stored == nil { + return nil + } + return DecodeTrace(r.Stored.LastFailure, "") +} diff --git a/core/artwork/explain_test.go b/core/artwork/explain_test.go index 14df4bbbf..8f9abf224 100644 --- a/core/artwork/explain_test.go +++ b/core/artwork/explain_test.go @@ -1,10 +1,14 @@ package artwork_test import ( + "context" + "time" + "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -60,6 +64,13 @@ var _ = Describe("ConfigFor", func() { Expect(value).To(Equal("external")) }) + It("names the setting for the album kind", func() { + conf.Server.CoverArtPriority = "cover.*, embedded" + setting, value := artwork.ConfigFor(model.KindAlbumArtwork) + Expect(setting).To(Equal("CoverArtPriority")) + Expect(value).To(Equal("cover.*, embedded")) + }) + It("returns nothing for a kind with no source configuration", func() { setting, _ := artwork.ConfigFor(model.KindPlaylistArtwork) Expect(setting).To(BeEmpty()) @@ -80,3 +91,79 @@ var _ = Describe("FormatAgents", func() { Expect(artwork.FormatAgents("lastfm", []string{"lastfm"}, " (* missing)")).To(Equal("lastfm")) }) }) + +var _ = Describe("Explain", func() { + var ds *tests.MockDataStore + var artRepo *tests.MockArtworkRepo + var queueRepo *tests.MockArtworkQueueRepo + var ctx context.Context + + BeforeEach(func() { + ctx = context.Background() + artRepo = tests.CreateMockArtworkRepo() + queueRepo = tests.CreateMockArtworkQueueRepo() + ds = &tests.MockDataStore{MockedArtwork: artRepo, MockedArtworkQueue: queueRepo} + Expect(ds.Artist(ctx).Put(&model.Artist{ID: "ar-1", Name: "Radiohead"})).To(Succeed()) + }) + + It("returns an error when the item does not exist", func() { + _, err := artwork.Explain(ctx, ds, nil, model.KindArtistArtwork, "nope", artwork.ExplainOptions{}) + Expect(err).To(MatchError(model.ErrNotFound)) + }) + + It("reads the recorded trace when no walker is supplied", func() { + // Storage shape mirrors storedStep in trace.go: c=candidate, o=outcome, d=detail. + trace := `[{"c":"external:deezer","o":"hit","d":"https://cdn/x.jpg"}]` + Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ + ItemKind: model.KindArtistArtwork.Prefix(), ItemID: "ar-1", ImageType: model.ImageTypePrimary, + Hash: "abc", Source: "external:deezer", + Trace: trace, + AttemptedAt: time.Date(2026, 9, 1, 10, 0, 0, 0, time.UTC), + })).To(Succeed()) + + rep, err := artwork.Explain(ctx, ds, nil, model.KindArtistArtwork, "ar-1", artwork.ExplainOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(rep.Name).To(Equal("Radiohead")) + Expect(rep.Walked).To(BeFalse()) + Expect(rep.Steps).To(Equal([]artwork.TraceStep{ + {Candidate: "external:deezer", Outcome: artwork.OutcomeHit, Detail: "https://cdn/x.jpg"}, + })) + Expect(rep.Source).To(Equal("external:deezer")) + Expect(rep.Result()).To(Equal("resolved from external:deezer")) + Expect(rep.ChainOrigin()).To(ContainSubstring("recorded")) + }) + + It("reports nothing recorded when there is no stored state", func() { + rep, err := artwork.Explain(ctx, ds, nil, model.KindArtistArtwork, "ar-1", artwork.ExplainOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(rep.Stored).To(BeNil()) + Expect(rep.Steps).To(BeEmpty()) + Expect(rep.ChainOrigin()).To(Equal("not recorded")) + }) + + It("includes the queue row and its failure trace", func() { + failure := `[{"c":"external:lastfm","o":"error","d":"429"}]` + Expect(queueRepo.Enqueue(model.ArtworkQueueItem{ + ItemKind: model.KindArtistArtwork.Prefix(), ItemID: "ar-1", ImageType: model.ImageTypePrimary, + Priority: model.ArtworkPriorityScan, + })).To(Succeed()) + queueRepo.SetTrace(model.KindArtistArtwork, "ar-1", model.ImageTypePrimary, failure) + + rep, err := artwork.Explain(ctx, ds, nil, model.KindArtistArtwork, "ar-1", artwork.ExplainOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(rep.Queued).ToNot(BeNil()) + Expect(rep.Queued.Priority).To(Equal(model.ArtworkPriorityScan)) + Expect(rep.LastAttemptFailed()).To(Equal([]artwork.TraceStep{ + {Candidate: "external:lastfm", Outcome: artwork.OutcomeError, Detail: "429"}, + })) + }) + + It("records nothing for a kind that keeps no state and has no walker", func() { + Expect(ds.Album(ctx).Put(&model.Album{ID: "al-1", Name: "OK Computer"})).To(Succeed()) + + rep, err := artwork.Explain(ctx, ds, nil, model.KindDiscArtwork, "al-1:2", artwork.ExplainOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(rep.Stored).To(BeNil()) + Expect(rep.Steps).To(BeEmpty()) + }) +}) diff --git a/tests/mock_artwork_queue_repo.go b/tests/mock_artwork_queue_repo.go index c482e2150..23ec51bad 100644 --- a/tests/mock_artwork_queue_repo.go +++ b/tests/mock_artwork_queue_repo.go @@ -39,6 +39,18 @@ func (m *MockArtworkQueueRepo) Get(kind model.Kind, id, imageType string) (*mode return &it, nil } +// SetTrace stores a queue row's trace directly, bypassing the retry-token check +// MarkFailedIfUnchanged enforces; tests use it to seed a failure trace outright. +func (m *MockArtworkQueueRepo) SetTrace(kind model.Kind, id, imageType, trace string) { + m.mu.Lock() + defer m.mu.Unlock() + k := iaKey(kind.Prefix(), id, imageType) + if it, ok := m.Data[k]; ok { + it.Trace = trace + m.Data[k] = it + } +} + func (m *MockArtworkQueueRepo) Enqueue(items ...model.ArtworkQueueItem) error { m.mu.Lock() defer m.mu.Unlock() From 03743a030b7babc421369758628df2c18357063e Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 3 Sep 2026 23:10:46 -0400 Subject: [PATCH 03/13] fix(artwork): set Walked before the Explainable gate, fix zero-time ChainOrigin Explain now sets rep.Walked ahead of the !Explainable early return, matching the pre-refactor CLI's unconditional flag (e.g. --live on a playlist), and ChainOrigin renders a zero AttemptedAt as "-" like the CLI's formatTime did. --- core/artwork/explain.go | 14 ++++++++++++-- core/artwork/explain_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/core/artwork/explain.go b/core/artwork/explain.go index c6bba8553..9001c5cb6 100644 --- a/core/artwork/explain.go +++ b/core/artwork/explain.go @@ -131,6 +131,9 @@ func Explain(ctx context.Context, ds model.DataStore, ag *agents.Agents, kind mo return ExplainReport{}, fmt.Errorf("reading the artwork queue: %w", err) } } + // Set even for a kind the chain never walks, so a caller that asked to walk (e.g. --live on a + // playlist) is reported as having tried, matching the pre-refactor CLI's unconditional flag. + rep.Walked = opts.Walk != nil if !Explainable(kind) { return rep, nil } @@ -138,7 +141,6 @@ func Explain(ctx context.Context, ds model.DataStore, ag *agents.Agents, kind mo rep.Agents = FormatAgents(conf.Server.Agents, ImageAgentNames(ag, kind), opts.UnavailableNote) } - rep.Walked = opts.Walk != nil switch { case rep.Walked: trace := &ChainTrace{} @@ -161,11 +163,19 @@ func (r ExplainReport) ChainOrigin() string { return "walked now" } if r.Stored != nil { - return "recorded " + r.Stored.AttemptedAt.Format(time.RFC3339) + return "recorded " + formatAttemptedAt(r.Stored.AttemptedAt) } return "not recorded" } +// formatAttemptedAt matches the CLI's formatTime: a zero time reads as unset, not as year 1. +func formatAttemptedAt(t time.Time) string { + if t.IsZero() { + return "-" + } + return t.Format(time.RFC3339) +} + // LastAttemptFailed decodes why the queued row's last attempt failed, if there is one queued. func (r ExplainReport) LastAttemptFailed() []TraceStep { if r.Queued == nil { diff --git a/core/artwork/explain_test.go b/core/artwork/explain_test.go index 8f9abf224..676f626d0 100644 --- a/core/artwork/explain_test.go +++ b/core/artwork/explain_test.go @@ -133,6 +133,11 @@ var _ = Describe("Explain", func() { Expect(rep.ChainOrigin()).To(ContainSubstring("recorded")) }) + It("renders a zero attempted-at the same way the CLI's formatTime does", func() { + rep := artwork.ExplainReport{Stored: &model.ItemArtwork{Source: "folder"}} + Expect(rep.ChainOrigin()).To(Equal("recorded -")) + }) + It("reports nothing recorded when there is no stored state", func() { rep, err := artwork.Explain(ctx, ds, nil, model.KindArtistArtwork, "ar-1", artwork.ExplainOptions{}) Expect(err).ToNot(HaveOccurred()) @@ -166,4 +171,28 @@ var _ = Describe("Explain", func() { Expect(rep.Stored).To(BeNil()) Expect(rep.Steps).To(BeEmpty()) }) + + It("performs a fresh walk and reports its outcome, including a failed one, when a walker is supplied", func() { + // GetAll erroring mid-walk is a deterministic way to force ResolveErr without a real library. + ds.Album(ctx).(*tests.MockAlbumRepo).SetError(true) + opts := artwork.ExplainOptions{Walk: func(t *artwork.ChainTrace) *artwork.TracingResolver { + return artwork.NewTracingResolver(ds, nil, nil, t, false) + }} + + rep, err := artwork.Explain(ctx, ds, nil, model.KindArtistArtwork, "ar-1", opts) + Expect(err).ToNot(HaveOccurred()) + Expect(rep.Walked).To(BeTrue()) + Expect(rep.ResolveErr).To(HaveOccurred()) + }) + + It("still reports Walked for a kind with no chain to walk, when a walker is supplied", func() { + Expect(ds.Playlist(ctx).Put(&model.Playlist{ID: "pl-1", Name: "Favorites"})).To(Succeed()) + opts := artwork.ExplainOptions{Walk: func(*artwork.ChainTrace) *artwork.TracingResolver { + panic("a kind that cannot walk must never be handed to the walker") + }} + + rep, err := artwork.Explain(ctx, ds, nil, model.KindPlaylistArtwork, "pl-1", opts) + Expect(err).ToNot(HaveOccurred()) + Expect(rep.Walked).To(BeTrue()) + }) }) From b8c7f3cd2bda9991af3f9052bf6d6d124ffa75db Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 3 Sep 2026 23:19:49 -0400 Subject: [PATCH 04/13] feat(nativeapi): add an admin-only artwork explain endpoint Exposes core/artwork.Explain over GET /api/artwork/explain?kind=&id=, reporting stored trace and queue state without ever walking the chain live, so the response can only leak history the server already has. --- cmd/wire_gen.go | 2 +- server/nativeapi/artwork_explain.go | 146 ++++++++++++++++++ server/nativeapi/artwork_explain_test.go | 108 +++++++++++++ server/nativeapi/config_test.go | 2 +- server/nativeapi/library_test.go | 2 +- server/nativeapi/metadata_test.go | 2 +- server/nativeapi/native_api.go | 7 +- server/nativeapi/native_api_song_test.go | 2 +- server/nativeapi/playlists_test.go | 2 +- server/nativeapi/plugin_test.go | 2 +- .../user_password_token_refresh_test.go | 2 +- 11 files changed, 267 insertions(+), 10 deletions(-) create mode 100644 server/nativeapi/artwork_explain.go create mode 100644 server/nativeapi/artwork_explain_test.go diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index de4c55a1e..84bb34c9f 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -79,7 +79,7 @@ func CreateNativeAPIRouter(ctx context.Context) *nativeapi.Router { agentsAgents := agents.GetAgents(dataStore, manager) matcherMatcher := matcher.New(dataStore) provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher, broker) - router := nativeapi.New(dataStore, share, playlistsPlaylists, insights, library, user, maintenance, manager, uploader, provider) + router := nativeapi.New(dataStore, share, playlistsPlaylists, insights, library, user, maintenance, manager, uploader, provider, agentsAgents) return router } diff --git a/server/nativeapi/artwork_explain.go b/server/nativeapi/artwork_explain.go new file mode 100644 index 000000000..ef3ab3987 --- /dev/null +++ b/server/nativeapi/artwork_explain.go @@ -0,0 +1,146 @@ +package nativeapi + +import ( + "encoding/json" + "errors" + "net/http" + "slices" + "time" + + "github.com/go-chi/chi/v5" + "github.com/navidrome/navidrome/core/artwork" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" +) + +// explainableKinds is every kind the endpoint accepts, matching the CLI's explainKinds: a kind +// with no chain to walk still has stored state and config to report. +var explainableKinds = []model.Kind{ + model.KindArtistArtwork, model.KindAlbumArtwork, model.KindDiscArtwork, + model.KindMediaFileArtwork, model.KindPlaylistArtwork, model.KindRadioArtwork, +} + +type traceStepDTO struct { + Candidate string `json:"candidate"` + Outcome string `json:"outcome"` + Detail string `json:"detail,omitempty"` +} + +type storedDTO struct { + Source string `json:"source"` + Hash string `json:"hash,omitempty"` + SourcePath string `json:"sourcePath,omitempty"` + AttemptedAt string `json:"attemptedAt,omitempty"` +} + +type queuedDTO struct { + Priority int `json:"priority"` + Attempts int `json:"attempts"` + RetryAt string `json:"retryAt,omitempty"` +} + +type configDTO struct { + Setting string `json:"setting"` + Value string `json:"value"` +} + +type explainDTO struct { + Kind string `json:"kind"` + ID string `json:"id"` + Name string `json:"name"` + Result string `json:"result"` + ChainOrigin string `json:"chainOrigin"` + Steps []traceStepDTO `json:"steps"` + Stored *storedDTO `json:"stored,omitempty"` + Queued *queuedDTO `json:"queued,omitempty"` + LastAttemptFailed []traceStepDTO `json:"lastAttemptFailed,omitempty"` + GaveUpAfter []traceStepDTO `json:"gaveUpAfter,omitempty"` + Config *configDTO `json:"config,omitempty"` + Agents string `json:"agents,omitempty"` +} + +func (api *Router) addArtworkExplainRoute(r chi.Router) { + r.Get("/artwork/explain", api.explainArtwork()) +} + +func (api *Router) explainArtwork() http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + kind, ok := model.ParseKind(r.URL.Query().Get("kind")) + if !ok || !slices.Contains(explainableKinds, kind) { + http.Error(w, "invalid artwork kind", http.StatusBadRequest) + return + } + id := r.URL.Query().Get("id") + + // No Walk: the endpoint reports history only, so it can never reach the network. + rep, err := artwork.Explain(ctx, api.ds, api.agents, kind, id, artwork.ExplainOptions{}) + if err != nil { + if errors.Is(err, model.ErrNotFound) { + http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) + return + } + log.Error(ctx, "Error explaining artwork", "kind", kind, "id", id, err) + http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) + return + } + + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(toExplainDTO(rep)); err != nil { + log.Error(ctx, "Error encoding artwork explain response", "kind", kind, "id", id, err) + } + } +} + +func rfc3339(t time.Time) string { + if t.IsZero() { + return "" + } + return t.Format(time.RFC3339) +} + +func toStepDTOs(steps []artwork.TraceStep) []traceStepDTO { + // Never nil: the UI maps over this unconditionally. + out := make([]traceStepDTO, 0, len(steps)) + for _, s := range steps { + out = append(out, traceStepDTO{Candidate: s.Candidate, Outcome: string(s.Outcome), Detail: s.Detail}) + } + return out +} + +func toExplainDTO(rep artwork.ExplainReport) explainDTO { + dto := explainDTO{ + Kind: rep.Kind.Prefix(), + ID: rep.ID, + Name: rep.Name, + Result: rep.Result(), + ChainOrigin: rep.ChainOrigin(), + Steps: toStepDTOs(rep.Steps), + Agents: rep.Agents, + } + if rep.Stored != nil { + dto.Stored = &storedDTO{ + Source: rep.Stored.Source, + Hash: rep.Stored.Hash, + SourcePath: rep.Stored.SourcePath, + AttemptedAt: rfc3339(rep.Stored.AttemptedAt), + } + } + if rep.Queued != nil { + dto.Queued = &queuedDTO{ + Priority: rep.Queued.Priority, + Attempts: rep.Queued.Attempts, + RetryAt: rfc3339(rep.Queued.RetryAt), + } + } + if steps := rep.LastAttemptFailed(); len(steps) > 0 { + dto.LastAttemptFailed = toStepDTOs(steps) + } + if steps := rep.GaveUpAfter(); len(steps) > 0 { + dto.GaveUpAfter = toStepDTOs(steps) + } + if setting, value := artwork.ConfigFor(rep.Kind); setting != "" { + dto.Config = &configDTO{Setting: setting, Value: value} + } + return dto +} diff --git a/server/nativeapi/artwork_explain_test.go b/server/nativeapi/artwork_explain_test.go new file mode 100644 index 000000000..d316a3357 --- /dev/null +++ b/server/nativeapi/artwork_explain_test.go @@ -0,0 +1,108 @@ +package nativeapi + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "time" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/core/auth" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/server" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("GET /artwork/explain", func() { + var router http.Handler + var ds *tests.MockDataStore + var artRepo *tests.MockArtworkRepo + var queueRepo *tests.MockArtworkQueueRepo + var adminToken, userToken string + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.EnableSharing = false + conf.Server.ArtistArtPriority = "external" + artRepo = tests.CreateMockArtworkRepo() + queueRepo = tests.CreateMockArtworkQueueRepo() + ds = &tests.MockDataStore{MockedArtwork: artRepo, MockedArtworkQueue: queueRepo} + Expect(ds.Artist(context.Background()).Put(&model.Artist{ID: "ar-1", Name: "Radiohead"})).To(Succeed()) + auth.Init(ds) + + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil, nil) + router = server.JWTVerifier(nativeRouter) + + adminUser := model.User{ID: "admin-1", UserName: "admin", IsAdmin: true, NewPassword: "adminpass"} + regularUser := model.User{ID: "user-1", UserName: "regular", IsAdmin: false, NewPassword: "userpass"} + Expect(ds.User(context.Background()).Put(&adminUser)).To(Succeed()) + Expect(ds.User(context.Background()).Put(®ularUser)).To(Succeed()) + + var err error + adminToken, err = auth.CreateToken(&adminUser) + Expect(err).ToNot(HaveOccurred()) + userToken, err = auth.CreateToken(®ularUser) + Expect(err).ToNot(HaveOccurred()) + }) + + It("returns 403 for a non-admin", func() { + req := createAuthenticatedRequest("GET", "/artwork/explain?kind=ar&id=ar-1", nil, userToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + Expect(w.Code).To(Equal(http.StatusForbidden)) + }) + + It("returns 400 for an unknown kind", func() { + req := createAuthenticatedRequest("GET", "/artwork/explain?kind=zz&id=ar-1", nil, adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + Expect(w.Code).To(Equal(http.StatusBadRequest)) + }) + + It("returns 404 for an unknown id", func() { + req := createAuthenticatedRequest("GET", "/artwork/explain?kind=ar&id=missing", nil, adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + Expect(w.Code).To(Equal(http.StatusNotFound)) + }) + + It("returns the report for an admin", func() { + // Storage shape from core/artwork/trace.go's storedStep: single-letter keys, "d" optional. + trace := `[{"c":"external:deezer","o":"hit","d":"https://cdn/x.jpg"}]` + Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ + ItemKind: model.KindArtistArtwork.Prefix(), ItemID: "ar-1", ImageType: model.ImageTypePrimary, + Hash: "abc", Source: "external:deezer", Trace: trace, + AttemptedAt: time.Date(2026, 9, 1, 10, 0, 0, 0, time.UTC), + })).To(Succeed()) + + req := createAuthenticatedRequest("GET", "/artwork/explain?kind=ar&id=ar-1", nil, adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + Expect(w.Code).To(Equal(http.StatusOK)) + + var got map[string]any + Expect(json.Unmarshal(w.Body.Bytes(), &got)).To(Succeed()) + Expect(got["name"]).To(Equal("Radiohead")) + Expect(got["result"]).To(Equal("resolved from external:deezer")) + Expect(got["chainOrigin"]).To(ContainSubstring("recorded")) + Expect(got["steps"]).To(HaveLen(1)) + Expect(got["config"]).To(HaveKeyWithValue("setting", "ArtistArtPriority")) + }) + + It("omits empty sections", func() { + req := createAuthenticatedRequest("GET", "/artwork/explain?kind=ar&id=ar-1", nil, adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + Expect(w.Code).To(Equal(http.StatusOK)) + + var got map[string]any + Expect(json.Unmarshal(w.Body.Bytes(), &got)).To(Succeed()) + Expect(got).ToNot(HaveKey("stored")) + Expect(got).ToNot(HaveKey("queued")) + Expect(got["chainOrigin"]).To(Equal("not recorded")) + }) +}) diff --git a/server/nativeapi/config_test.go b/server/nativeapi/config_test.go index d1007f457..7c4f00fdd 100644 --- a/server/nativeapi/config_test.go +++ b/server/nativeapi/config_test.go @@ -29,7 +29,7 @@ var _ = Describe("Config API", func() { conf.Server.DevUIShowConfig = true // Enable config endpoint for tests ds = &tests.MockDataStore{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil) + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil, nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/library_test.go b/server/nativeapi/library_test.go index 13b33c238..cef2e06ad 100644 --- a/server/nativeapi/library_test.go +++ b/server/nativeapi/library_test.go @@ -31,7 +31,7 @@ var _ = Describe("Library API", func() { conf.Server.EnableSharing = false ds = &tests.MockDataStore{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil) + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil, nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/metadata_test.go b/server/nativeapi/metadata_test.go index ebc9aeb28..ae6e301a8 100644 --- a/server/nativeapi/metadata_test.go +++ b/server/nativeapi/metadata_test.go @@ -66,7 +66,7 @@ var _ = Describe("Metadata API", func() { } auth.Init(ds) provider = &fakeProvider{} - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, provider) + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, provider, nil) router = server.JWTVerifier(nativeRouter) adminUser := model.User{ID: "admin-1", UserName: "admin", IsAdmin: true, NewPassword: "adminpass"} diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index 57a712a20..b3b7374f9 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -13,6 +13,7 @@ import ( "github.com/go-chi/chi/v5/middleware" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core/agents" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/external" "github.com/navidrome/navidrome/core/metrics" @@ -48,10 +49,11 @@ type Router struct { pluginManager PluginManager imgUpload artwork.Uploader provider external.Provider + agents *agents.Agents } -func New(ds model.DataStore, share core.Share, playlists playlistsvc.Playlists, insights metrics.Insights, libraryService core.Library, userService core.User, maintenance core.Maintenance, pluginManager PluginManager, imgUpload artwork.Uploader, provider external.Provider) *Router { - r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, users: userService, maintenance: maintenance, pluginManager: pluginManager, imgUpload: imgUpload, provider: provider} +func New(ds model.DataStore, share core.Share, playlists playlistsvc.Playlists, insights metrics.Insights, libraryService core.Library, userService core.User, maintenance core.Maintenance, pluginManager PluginManager, imgUpload artwork.Uploader, provider external.Provider, ag *agents.Agents) *Router { + r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, users: userService, maintenance: maintenance, pluginManager: pluginManager, imgUpload: imgUpload, provider: provider, agents: ag} r.Handler = r.routes() return r } @@ -95,6 +97,7 @@ func (api *Router) routes() http.Handler { api.addUserLibraryRoute(r) api.addPluginRoute(r) api.addMetadataRoute(r) + api.addArtworkExplainRoute(r) api.RX(r, "/library", api.libs.NewRepository, true) }) }) diff --git a/server/nativeapi/native_api_song_test.go b/server/nativeapi/native_api_song_test.go index 203fcd4cf..954c872a7 100644 --- a/server/nativeapi/native_api_song_test.go +++ b/server/nativeapi/native_api_song_test.go @@ -95,7 +95,7 @@ var _ = Describe("Song Endpoints", func() { mfRepo.SetData(testSongs) // Create the native API router and wrap it with the JWTVerifier middleware - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil) + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil, nil) router = server.JWTVerifier(nativeRouter) w = httptest.NewRecorder() }) diff --git a/server/nativeapi/playlists_test.go b/server/nativeapi/playlists_test.go index 9abcc477f..e9ca26d7f 100644 --- a/server/nativeapi/playlists_test.go +++ b/server/nativeapi/playlists_test.go @@ -99,7 +99,7 @@ var _ = Describe("Playlist Tracks Endpoint", func() { err := userRepo.Put(&testUser) Expect(err).ToNot(HaveOccurred()) - nativeRouter := New(ds, nil, plsSvc, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil) + nativeRouter := New(ds, nil, plsSvc, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil, nil) router = server.JWTVerifier(nativeRouter) w = httptest.NewRecorder() }) diff --git a/server/nativeapi/plugin_test.go b/server/nativeapi/plugin_test.go index 1683885e7..4e45ddb92 100644 --- a/server/nativeapi/plugin_test.go +++ b/server/nativeapi/plugin_test.go @@ -34,7 +34,7 @@ var _ = Describe("Plugin API", func() { ds = &tests.MockDataStore{} mockManager = &tests.MockPluginManager{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, mockManager, nil, nil) + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, mockManager, nil, nil, nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/user_password_token_refresh_test.go b/server/nativeapi/user_password_token_refresh_test.go index 2a363980f..27454fc7d 100644 --- a/server/nativeapi/user_password_token_refresh_test.go +++ b/server/nativeapi/user_password_token_refresh_test.go @@ -45,7 +45,7 @@ var _ = Describe("PUT /user/{id}: token refresh on self password change", func() auth.Init(ds) userService := core.NewUser(ds, noopPluginUnloader{}) - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), userService, nil, nil, nil, nil) + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), userService, nil, nil, nil, nil, nil) router = server.JWTVerifier(nativeRouter) }) From 395f2f9ae53c8255097a591e5311f2df3273bf97 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 3 Sep 2026 23:25:01 -0400 Subject: [PATCH 05/13] test(nativeapi): pin the artwork explain wire format Extend the admin report spec to assert the full stored object and step fields, and add a spec covering the queued object and lastAttemptFailed, so a wrong JSON tag in toExplainDTO fails a test instead of shipping. --- server/nativeapi/artwork_explain_test.go | 46 ++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/server/nativeapi/artwork_explain_test.go b/server/nativeapi/artwork_explain_test.go index d316a3357..4a76be76d 100644 --- a/server/nativeapi/artwork_explain_test.go +++ b/server/nativeapi/artwork_explain_test.go @@ -73,10 +73,11 @@ var _ = Describe("GET /artwork/explain", func() { It("returns the report for an admin", func() { // Storage shape from core/artwork/trace.go's storedStep: single-letter keys, "d" optional. trace := `[{"c":"external:deezer","o":"hit","d":"https://cdn/x.jpg"}]` + attemptedAt := time.Date(2026, 9, 1, 10, 0, 0, 0, time.UTC) Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ ItemKind: model.KindArtistArtwork.Prefix(), ItemID: "ar-1", ImageType: model.ImageTypePrimary, - Hash: "abc", Source: "external:deezer", Trace: trace, - AttemptedAt: time.Date(2026, 9, 1, 10, 0, 0, 0, time.UTC), + Hash: "abc", Source: "external:deezer", SourcePath: "/music/Radiohead/folder.jpg", Trace: trace, + AttemptedAt: attemptedAt, })).To(Succeed()) req := createAuthenticatedRequest("GET", "/artwork/explain?kind=ar&id=ar-1", nil, adminToken) @@ -89,8 +90,47 @@ var _ = Describe("GET /artwork/explain", func() { Expect(got["name"]).To(Equal("Radiohead")) Expect(got["result"]).To(Equal("resolved from external:deezer")) Expect(got["chainOrigin"]).To(ContainSubstring("recorded")) - Expect(got["steps"]).To(HaveLen(1)) Expect(got["config"]).To(HaveKeyWithValue("setting", "ArtistArtPriority")) + + Expect(got["stored"]).To(Equal(map[string]any{ + "source": "external:deezer", + "hash": "abc", + "sourcePath": "/music/Radiohead/folder.jpg", + "attemptedAt": attemptedAt.Format(time.RFC3339), + })) + Expect(got["steps"]).To(Equal([]any{ + map[string]any{"candidate": "external:deezer", "outcome": "hit", "detail": "https://cdn/x.jpg"}, + })) + }) + + It("returns the queue state and the last-attempt trace", func() { + Expect(queueRepo.Enqueue(model.ArtworkQueueItem{ + ItemKind: model.KindArtistArtwork.Prefix(), ItemID: "ar-1", ImageType: model.ImageTypePrimary, + Priority: model.ArtworkPriorityScan, + })).To(Succeed()) + seen, err := queueRepo.Get(model.KindArtistArtwork, "ar-1", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + + retryAt := time.Date(2026, 9, 2, 8, 0, 0, 0, time.UTC) + failTrace := `[{"c":"external:deezer","o":"error","d":"timeout"}]` + Expect(queueRepo.MarkFailedIfUnchanged(model.KindArtistArtwork.Prefix(), "ar-1", model.ImageTypePrimary, + seen.RetryAt, retryAt, failTrace)).To(Succeed()) + + req := createAuthenticatedRequest("GET", "/artwork/explain?kind=ar&id=ar-1", nil, adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + Expect(w.Code).To(Equal(http.StatusOK)) + + var got map[string]any + Expect(json.Unmarshal(w.Body.Bytes(), &got)).To(Succeed()) + Expect(got["queued"]).To(Equal(map[string]any{ + "priority": float64(model.ArtworkPriorityScan), + "attempts": float64(1), + "retryAt": retryAt.Format(time.RFC3339), + })) + Expect(got["lastAttemptFailed"]).To(Equal([]any{ + map[string]any{"candidate": "external:deezer", "outcome": "error", "detail": "timeout"}, + })) }) It("omits empty sections", func() { From 0fcce273a2f636349320c3a70167b6d3bb6add9d Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 3 Sep 2026 23:27:41 -0400 Subject: [PATCH 06/13] test(nativeapi): cover gaveUpAfter and its omission GaveUpAfter decodes ItemArtwork.LastFailure, a separate path from LastAttemptFailed's queue-row trace; nothing previously exercised it. --- server/nativeapi/artwork_explain_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/server/nativeapi/artwork_explain_test.go b/server/nativeapi/artwork_explain_test.go index 4a76be76d..e830254cf 100644 --- a/server/nativeapi/artwork_explain_test.go +++ b/server/nativeapi/artwork_explain_test.go @@ -73,11 +73,12 @@ var _ = Describe("GET /artwork/explain", func() { It("returns the report for an admin", func() { // Storage shape from core/artwork/trace.go's storedStep: single-letter keys, "d" optional. trace := `[{"c":"external:deezer","o":"hit","d":"https://cdn/x.jpg"}]` + gaveUpTrace := `[{"c":"external:deezer","o":"error","d":"connection reset"}]` attemptedAt := time.Date(2026, 9, 1, 10, 0, 0, 0, time.UTC) Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ ItemKind: model.KindArtistArtwork.Prefix(), ItemID: "ar-1", ImageType: model.ImageTypePrimary, Hash: "abc", Source: "external:deezer", SourcePath: "/music/Radiohead/folder.jpg", Trace: trace, - AttemptedAt: attemptedAt, + LastFailure: gaveUpTrace, AttemptedAt: attemptedAt, })).To(Succeed()) req := createAuthenticatedRequest("GET", "/artwork/explain?kind=ar&id=ar-1", nil, adminToken) @@ -101,6 +102,9 @@ var _ = Describe("GET /artwork/explain", func() { Expect(got["steps"]).To(Equal([]any{ map[string]any{"candidate": "external:deezer", "outcome": "hit", "detail": "https://cdn/x.jpg"}, })) + Expect(got["gaveUpAfter"]).To(Equal([]any{ + map[string]any{"candidate": "external:deezer", "outcome": "error", "detail": "connection reset"}, + })) }) It("returns the queue state and the last-attempt trace", func() { @@ -143,6 +147,8 @@ var _ = Describe("GET /artwork/explain", func() { Expect(json.Unmarshal(w.Body.Bytes(), &got)).To(Succeed()) Expect(got).ToNot(HaveKey("stored")) Expect(got).ToNot(HaveKey("queued")) + Expect(got).ToNot(HaveKey("lastAttemptFailed")) + Expect(got).ToNot(HaveKey("gaveUpAfter")) Expect(got["chainOrigin"]).To(Equal("not recorded")) }) }) From d17ef212ab0a11a0991dae427b2289115658b978 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 3 Sep 2026 23:30:39 -0400 Subject: [PATCH 07/13] feat(ui): add an explainArtwork data provider method --- ui/src/dataProvider/wrapperDataProvider.js | 4 ++++ .../dataProvider/wrapperDataProvider.test.js | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/ui/src/dataProvider/wrapperDataProvider.js b/ui/src/dataProvider/wrapperDataProvider.js index e79beb787..3af601adf 100644 --- a/ui/src/dataProvider/wrapperDataProvider.js +++ b/ui/src/dataProvider/wrapperDataProvider.js @@ -229,6 +229,10 @@ const wrapperDataProvider = { httpClient(`${REST_URL}/metadata/${REFRESH_KIND[resource]}/${id}/refresh`, { method: 'POST', }).then(() => ({ data: { id } })), + explainArtwork: (resource, id) => + httpClient( + `${REST_URL}/artwork/explain?kind=${REFRESH_KIND[resource]}&id=${encodeURIComponent(id)}`, + ).then(({ json }) => ({ data: json })), } export default wrapperDataProvider diff --git a/ui/src/dataProvider/wrapperDataProvider.test.js b/ui/src/dataProvider/wrapperDataProvider.test.js index 4225a5a54..cf0e3a7c1 100644 --- a/ui/src/dataProvider/wrapperDataProvider.test.js +++ b/ui/src/dataProvider/wrapperDataProvider.test.js @@ -120,4 +120,23 @@ describe('wrapperDataProvider', () => { ).resolves.toEqual({ data: { id: 'al-1' } }) }) }) + + describe('explainArtwork', () => { + it('requests the artist kind prefix', async () => { + mockHttpClient.mockResolvedValue({ json: { name: 'Radiohead' } }) + const result = await wrapperDataProvider.explainArtwork('artist', 'ar-1') + expect(mockHttpClient).toHaveBeenCalledWith( + expect.stringContaining('/artwork/explain?kind=ar&id=ar-1'), + ) + expect(result).toEqual({ data: { name: 'Radiohead' } }) + }) + + it('requests the album kind prefix', async () => { + mockHttpClient.mockResolvedValue({ json: {} }) + await wrapperDataProvider.explainArtwork('album', 'al-1') + expect(mockHttpClient).toHaveBeenCalledWith( + expect.stringContaining('kind=al&id=al-1'), + ) + }) + }) }) From e7f7e7bde945e978748afb9762b180704475af0c Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 3 Sep 2026 23:33:14 -0400 Subject: [PATCH 08/13] feat(ui): show the artwork resolution trace in the album info dialog --- ui/src/album/AlbumInfo.jsx | 2 + ui/src/common/ArtworkInfo.jsx | 177 +++++++++++++++++++++++++++++ ui/src/common/ArtworkInfo.test.jsx | 93 +++++++++++++++ ui/src/common/index.js | 1 + ui/src/i18n/en.json | 21 ++++ 5 files changed, 294 insertions(+) create mode 100644 ui/src/common/ArtworkInfo.jsx create mode 100644 ui/src/common/ArtworkInfo.test.jsx diff --git a/ui/src/album/AlbumInfo.jsx b/ui/src/album/AlbumInfo.jsx index 075841e43..c7a9eac3f 100644 --- a/ui/src/album/AlbumInfo.jsx +++ b/ui/src/album/AlbumInfo.jsx @@ -18,6 +18,7 @@ import { import { makeStyles } from '@material-ui/core/styles' import { ArtistLinkField, + ArtworkInfo, MultiLineTextField, ParticipantsInfo, RangeField, @@ -139,6 +140,7 @@ const AlbumInfo = (props) => { ) })} + diff --git a/ui/src/common/ArtworkInfo.jsx b/ui/src/common/ArtworkInfo.jsx new file mode 100644 index 000000000..42eb0c699 --- /dev/null +++ b/ui/src/common/ArtworkInfo.jsx @@ -0,0 +1,177 @@ +import React, { useEffect, useState } from 'react' +import PropTypes from 'prop-types' +import { + Chip, + Link, + TableCell, + TableRow, +} from '@material-ui/core' +import { makeStyles } from '@material-ui/core/styles' +import { useDataProvider, usePermissions, useTranslate } from 'react-admin' + +const OUTCOME_COLORS = { + hit: '#4caf50', + miss: '#9e9e9e', + skipped: '#9e9e9e', + error: '#f44336', + unreadable: '#f44336', +} + +const useStyles = makeStyles({ + chip: { color: '#fff', height: 20 }, + toggle: { cursor: 'pointer' }, +}) + +const OutcomeChip = ({ outcome }) => { + const classes = useStyles() + return ( + + ) +} + +OutcomeChip.propTypes = { + outcome: PropTypes.string.isRequired, +} + +const StepTable = ({ title, steps }) => { + if (!steps?.length) return null + return ( + <> + + + {title} + + + {steps.map((s, i) => ( + + {s.candidate} + + {s.detail} + + + ))} + + ) +} + +StepTable.propTypes = { + title: PropTypes.string.isRequired, + steps: PropTypes.array, +} + +const Row = ({ label, children }) => ( + + + {label}: + + {children} + +) + +Row.propTypes = { + label: PropTypes.node.isRequired, + children: PropTypes.node, +} + +export const ArtworkInfo = ({ resource, id }) => { + const classes = useStyles() + const translate = useTranslate() + const dataProvider = useDataProvider() + const { permissions } = usePermissions() + const [report, setReport] = useState(null) + const [expanded, setExpanded] = useState(false) + const isAdmin = permissions === 'admin' + + useEffect(() => { + if (!isAdmin) return undefined + let live = true + dataProvider + .explainArtwork(resource, id) + .then(({ data }) => live && setReport(data)) + .catch(() => live && setReport(null)) + return () => { + live = false + } + }, [dataProvider, resource, id, isAdmin]) + + if (!isAdmin || !report) return null + + const recorded = report.chainOrigin !== 'not recorded' + return ( + <> + + + {translate('artwork.title')} + + + {report.result} + {recorded ? ( + <> + + {report.stored?.source} + + + {report.stored?.attemptedAt} + + + ) : ( + + {translate('artwork.notRecorded')} + + )} + + + setExpanded(!expanded)} + > + {translate(expanded ? 'artwork.hideDetails' : 'artwork.showDetails')} + + + + {expanded && ( + <> + + + + {report.queued && ( + <> + + {report.queued.priority} + + + {report.queued.attempts} + + + {report.queued.retryAt} + + + )} + {report.config && ( + {report.config.value} + )} + {report.agents && ( + {report.agents} + )} + + )} + + ) +} + +ArtworkInfo.propTypes = { + resource: PropTypes.oneOf(['album', 'artist']).isRequired, + id: PropTypes.string.isRequired, +} diff --git a/ui/src/common/ArtworkInfo.test.jsx b/ui/src/common/ArtworkInfo.test.jsx new file mode 100644 index 000000000..2a793911a --- /dev/null +++ b/ui/src/common/ArtworkInfo.test.jsx @@ -0,0 +1,93 @@ +import React from 'react' +import { render, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { ArtworkInfo } from './ArtworkInfo' + +const explainArtwork = vi.fn() +const { mockPermissions } = vi.hoisted(() => ({ + mockPermissions: { value: 'admin' }, +})) + +vi.mock('react-admin', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useDataProvider: () => ({ explainArtwork }), + usePermissions: () => ({ permissions: mockPermissions.value }), + useTranslate: () => (key) => key, + } +}) + +const report = { + result: 'resolved from external:deezer', + chainOrigin: 'recorded 2026-09-01T10:00:00Z', + stored: { source: 'external:deezer', attemptedAt: '2026-09-01T10:00:00Z' }, + steps: [ + { candidate: 'cover.*', outcome: 'miss' }, + { candidate: 'external:deezer', outcome: 'hit', detail: 'https://cdn/x.jpg' }, + ], + config: { setting: 'ArtistArtPriority', value: 'external' }, +} + +// ArtworkInfo renders bare TableRows to slot into the caller's TableBody, so it needs a +// table/tbody ancestor or React logs DOM-nesting warnings. +const renderInTable = (ui) => + render( + + {ui} +
, + ) + +describe('', () => { + beforeEach(() => { + mockPermissions.value = 'admin' + explainArtwork.mockReset() + explainArtwork.mockResolvedValue({ data: report }) + }) + + it('renders nothing for a non-admin', () => { + mockPermissions.value = 'regular' + const { container } = renderInTable( + , + ) + expect(container.querySelector('tbody')).toBeEmptyDOMElement() + expect(explainArtwork).not.toHaveBeenCalled() + }) + + it('shows the summary for an admin', async () => { + renderInTable() + expect( + await screen.findByText('resolved from external:deezer'), + ).toBeInTheDocument() + expect(screen.queryByText('external:deezer')).not.toBeNull() + expect(screen.queryByText('cover.*')).toBeNull() + }) + + it('reveals the step table when details are expanded', async () => { + renderInTable() + await screen.findByText('resolved from external:deezer') + await userEvent.click(screen.getByText('artwork.showDetails')) + expect(screen.getByText('cover.*')).toBeInTheDocument() + expect( + screen.getByText('ArtistArtPriority', { exact: false }), + ).toBeInTheDocument() + }) + + it('shows the not-recorded state', async () => { + explainArtwork.mockResolvedValue({ + data: { result: 'not resolved', chainOrigin: 'not recorded', steps: [] }, + }) + renderInTable() + expect(await screen.findByText('artwork.notRecorded')).toBeInTheDocument() + }) + + it('renders nothing when the fetch fails', async () => { + explainArtwork.mockRejectedValue(new Error('boom')) + const { container } = renderInTable( + , + ) + await waitFor(() => expect(explainArtwork).toHaveBeenCalled()) + expect(container.querySelector('tbody')).toBeEmptyDOMElement() + }) +}) diff --git a/ui/src/common/index.js b/ui/src/common/index.js index 0177df326..f038f7eb4 100644 --- a/ui/src/common/index.js +++ b/ui/src/common/index.js @@ -1,6 +1,7 @@ export * from './AddToPlaylistButton' export * from './artist' export * from './ArtistLinkField' +export * from './ArtworkInfo' export * from './BatchPlayButton' export * from './BitrateField' export * from './CollapsibleComment' diff --git a/ui/src/i18n/en.json b/ui/src/i18n/en.json index de96d47c0..e733dfeba 100644 --- a/ui/src/i18n/en.json +++ b/ui/src/i18n/en.json @@ -646,6 +646,27 @@ "sharedPlaylists": "Shared Playlists", "about": "About" }, + "artwork": { + "title": "Artwork", + "chain": "Chain", + "result": "Result", + "source": "Source", + "attemptedAt": "Attempted at", + "showDetails": "Show details", + "hideDetails": "Hide details", + "candidate": "Candidate", + "outcome": "Outcome", + "detail": "Detail", + "queue": "Queue", + "priority": "Priority", + "attempts": "Attempts", + "retryAt": "Retry at", + "lastAttemptFailed": "Last attempt failed", + "gaveUpAfter": "Gave up after", + "config": "Config", + "agents": "Agents", + "notRecorded": "No resolution recorded yet" + }, "player": { "playListsText": "Play Queue", "openText": "Open", From eadbb442a8c42978ec9a8795345800e3affc23cf Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 3 Sep 2026 23:38:15 -0400 Subject: [PATCH 09/13] fix(ui): format artwork files, drop unused i18n keys, harden test assertions --- ui/src/common/ArtworkInfo.jsx | 15 +++++---------- ui/src/common/ArtworkInfo.test.jsx | 16 ++++++++++++---- ui/src/i18n/en.json | 5 ----- 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/ui/src/common/ArtworkInfo.jsx b/ui/src/common/ArtworkInfo.jsx index 42eb0c699..b83fc71b6 100644 --- a/ui/src/common/ArtworkInfo.jsx +++ b/ui/src/common/ArtworkInfo.jsx @@ -1,11 +1,6 @@ import React, { useEffect, useState } from 'react' import PropTypes from 'prop-types' -import { - Chip, - Link, - TableCell, - TableRow, -} from '@material-ui/core' +import { Chip, Link, TableCell, TableRow } from '@material-ui/core' import { makeStyles } from '@material-ui/core/styles' import { useDataProvider, usePermissions, useTranslate } from 'react-admin' @@ -112,9 +107,7 @@ export const ArtworkInfo = ({ resource, id }) => { {report.result} {recorded ? ( <> - - {report.stored?.source} - + {report.stored?.source} {report.stored?.attemptedAt} @@ -131,7 +124,9 @@ export const ArtworkInfo = ({ resource, id }) => { className={classes.toggle} onClick={() => setExpanded(!expanded)} > - {translate(expanded ? 'artwork.hideDetails' : 'artwork.showDetails')} + {translate( + expanded ? 'artwork.hideDetails' : 'artwork.showDetails', + )} diff --git a/ui/src/common/ArtworkInfo.test.jsx b/ui/src/common/ArtworkInfo.test.jsx index 2a793911a..862881939 100644 --- a/ui/src/common/ArtworkInfo.test.jsx +++ b/ui/src/common/ArtworkInfo.test.jsx @@ -25,7 +25,11 @@ const report = { stored: { source: 'external:deezer', attemptedAt: '2026-09-01T10:00:00Z' }, steps: [ { candidate: 'cover.*', outcome: 'miss' }, - { candidate: 'external:deezer', outcome: 'hit', detail: 'https://cdn/x.jpg' }, + { + candidate: 'external:deezer', + outcome: 'hit', + detail: 'https://cdn/x.jpg', + }, ], config: { setting: 'ArtistArtPriority', value: 'external' }, } @@ -69,9 +73,7 @@ describe('', () => { await screen.findByText('resolved from external:deezer') await userEvent.click(screen.getByText('artwork.showDetails')) expect(screen.getByText('cover.*')).toBeInTheDocument() - expect( - screen.getByText('ArtistArtPriority', { exact: false }), - ).toBeInTheDocument() + expect(screen.getByText('ArtistArtPriority:')).toBeInTheDocument() }) it('shows the not-recorded state', async () => { @@ -80,6 +82,12 @@ describe('', () => { }) renderInTable() expect(await screen.findByText('artwork.notRecorded')).toBeInTheDocument() + + // stored/queued/config/agents are all omitted by the endpoint here, so the optional + // blocks' guards must not throw when expanded. + await userEvent.click(screen.getByText('artwork.showDetails')) + expect(screen.queryByText('artwork.priority')).toBeNull() + expect(screen.queryByText('artwork.agents')).toBeNull() }) it('renders nothing when the fetch fails', async () => { diff --git a/ui/src/i18n/en.json b/ui/src/i18n/en.json index e733dfeba..9635cadd1 100644 --- a/ui/src/i18n/en.json +++ b/ui/src/i18n/en.json @@ -654,16 +654,11 @@ "attemptedAt": "Attempted at", "showDetails": "Show details", "hideDetails": "Hide details", - "candidate": "Candidate", - "outcome": "Outcome", - "detail": "Detail", - "queue": "Queue", "priority": "Priority", "attempts": "Attempts", "retryAt": "Retry at", "lastAttemptFailed": "Last attempt failed", "gaveUpAfter": "Gave up after", - "config": "Config", "agents": "Agents", "notRecorded": "No resolution recorded yet" }, From 687646809f06236768764434b54b13e0310b392e Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 3 Sep 2026 23:46:06 -0400 Subject: [PATCH 10/13] refactor(ui): let the info dialog pick its content by resource --- ui/src/actions/dialogs.js | 3 +- ui/src/common/ContextMenus.jsx | 4 +-- ui/src/common/ContextMenus.test.jsx | 16 +++++++++ ui/src/common/SongContextMenu.jsx | 2 +- ui/src/common/SongContextMenu.test.jsx | 21 +++++++++++ ui/src/dialogs/ExpandInfoDialog.jsx | 15 ++++---- ui/src/dialogs/ExpandInfoDialog.test.jsx | 43 ++++++++++++++++++++++ ui/src/reducers/dialogReducer.js | 3 ++ ui/src/reducers/dialogReducer.test.js | 45 ++++++++++++++++++++++++ 9 files changed, 142 insertions(+), 10 deletions(-) create mode 100644 ui/src/dialogs/ExpandInfoDialog.test.jsx create mode 100644 ui/src/reducers/dialogReducer.test.js diff --git a/ui/src/actions/dialogs.js b/ui/src/actions/dialogs.js index dea4d6203..fd0fe6ea1 100644 --- a/ui/src/actions/dialogs.js +++ b/ui/src/actions/dialogs.js @@ -60,10 +60,11 @@ export const closeDuplicateSongDialog = () => ({ type: DUPLICATE_SONG_WARNING_CLOSE, }) -export const openExtendedInfoDialog = (record) => { +export const openExtendedInfoDialog = (record, resource) => { return { type: EXTENDED_INFO_OPEN, record, + resource, } } diff --git a/ui/src/common/ContextMenus.jsx b/ui/src/common/ContextMenus.jsx index 4407392c1..57e672b2d 100644 --- a/ui/src/common/ContextMenus.jsx +++ b/ui/src/common/ContextMenus.jsx @@ -146,9 +146,9 @@ const ContextMenu = ({ ...(!hideInfo && { info: { enabled: true, - needData: true, + needData: false, label: translate('resources.album.actions.info'), - action: () => dispatch(openExtendedInfoDialog(record)), + action: (record) => dispatch(openExtendedInfoDialog(record, resource)), }, }), } diff --git a/ui/src/common/ContextMenus.test.jsx b/ui/src/common/ContextMenus.test.jsx index 59a7de966..6ac43ac74 100644 --- a/ui/src/common/ContextMenus.test.jsx +++ b/ui/src/common/ContextMenus.test.jsx @@ -4,6 +4,7 @@ import { TestContext } from 'ra-test' import { describe, it, expect, vi, beforeEach } from 'vitest' import { ThemeProvider, createTheme } from '@material-ui/core/styles' import { AlbumContextMenu, ArtistContextMenu } from './ContextMenus' +import { EXTENDED_INFO_OPEN } from '../actions' const mockDispatch = vi.fn() vi.mock('react-redux', () => ({ useDispatch: () => mockDispatch })) @@ -130,4 +131,19 @@ describe('ContextMenus', () => { expect(mockRefreshMetadata).toHaveBeenCalledWith('album', 'al1') }) }) + + describe('info dialog', () => { + // ArtistContextMenu hardcodes hideInfo, so it never renders a Get Info item; Album + // uses the same action and proves resource is threaded through the dispatch. + it('dispatches the record and resource when opening the info dialog', () => { + const record = { id: 'al1', name: 'Album', songCount: 1 } + renderMenu(AlbumContextMenu, record) + fireEvent.click(screen.getByText('resources.album.actions.info')) + expect(mockDispatch).toHaveBeenCalledWith({ + type: EXTENDED_INFO_OPEN, + record, + resource: 'album', + }) + }) + }) }) diff --git a/ui/src/common/SongContextMenu.jsx b/ui/src/common/SongContextMenu.jsx index ac5b10e13..9387cb68b 100644 --- a/ui/src/common/SongContextMenu.jsx +++ b/ui/src/common/SongContextMenu.jsx @@ -165,7 +165,7 @@ export const SongContextMenu = ({ } } - dispatch(openExtendedInfoDialog(fullRecord)) + dispatch(openExtendedInfoDialog(fullRecord, 'song')) }, }, } diff --git a/ui/src/common/SongContextMenu.test.jsx b/ui/src/common/SongContextMenu.test.jsx index 7172742d3..dab882db6 100644 --- a/ui/src/common/SongContextMenu.test.jsx +++ b/ui/src/common/SongContextMenu.test.jsx @@ -4,6 +4,7 @@ import { TestContext } from 'ra-test' import { describe, it, expect, vi, beforeEach } from 'vitest' import { SongContextMenu } from './SongContextMenu' import subsonic from '../subsonic' +import { EXTENDED_INFO_OPEN } from '../actions' vi.mock('../dataProvider', () => ({ httpClient: vi.fn(), @@ -130,6 +131,26 @@ describe('SongContextMenu', () => { expect(mockOnClick).not.toHaveBeenCalled() }) + it('dispatches the record and resource when opening the info dialog', async () => { + const record = { id: 'song1', size: 1 } + render( + + + , + ) + fireEvent.click(screen.getAllByRole('button')[1]) + await waitFor(() => screen.getByText(/resources\.song\.actions\.info/)) + fireEvent.click(screen.getByText(/resources\.song\.actions\.info/)) + + await waitFor(() => + expect(mockDispatch).toHaveBeenCalledWith({ + type: EXTENDED_INFO_OPEN, + record, + resource: 'song', + }), + ) + }) + describe('Instant Mix action', () => { it('calls getSimilarSongs2 with song id and shows loading notification', async () => { render( diff --git a/ui/src/dialogs/ExpandInfoDialog.jsx b/ui/src/dialogs/ExpandInfoDialog.jsx index be84546fc..b02c1c451 100644 --- a/ui/src/dialogs/ExpandInfoDialog.jsx +++ b/ui/src/dialogs/ExpandInfoDialog.jsx @@ -12,9 +12,13 @@ import { import { closeExtendedInfoDialog } from '../actions' const ExpandInfoDialog = ({ title, content }) => { - const { open, record } = useSelector((state) => state.expandInfoDialog) + const { open, record, resource } = useSelector( + (state) => state.expandInfoDialog, + ) const dispatch = useDispatch() const translate = useTranslate() + // A node renders for any record; a map lets one page serve more than one resource. + const body = React.isValidElement(content) ? content : content?.[resource] const handleClose = (e) => { dispatch(closeExtendedInfoDialog()) @@ -33,10 +37,8 @@ const ExpandInfoDialog = ({ title, content }) => { {translate(title || 'resources.song.actions.info')} - {record && ( - - {content} - + {record && body && ( + {body} )} @@ -50,7 +52,8 @@ const ExpandInfoDialog = ({ title, content }) => { ExpandInfoDialog.propTypes = { title: PropTypes.string, - content: PropTypes.object.isRequired, + content: PropTypes.oneOfType([PropTypes.element, PropTypes.object]) + .isRequired, } export default ExpandInfoDialog diff --git a/ui/src/dialogs/ExpandInfoDialog.test.jsx b/ui/src/dialogs/ExpandInfoDialog.test.jsx new file mode 100644 index 000000000..9ce5d06e5 --- /dev/null +++ b/ui/src/dialogs/ExpandInfoDialog.test.jsx @@ -0,0 +1,43 @@ +import * as React from 'react' +import { TestContext } from 'ra-test' +import { render, screen, cleanup } from '@testing-library/react' +import { describe, afterEach, it, expect } from 'vitest' +import ExpandInfoDialog from './ExpandInfoDialog' + +const renderDialog = (content, resource) => + render( + + + , + ) + +describe('ExpandInfoDialog', () => { + afterEach(cleanup) + + it('renders a node content as-is, regardless of resource', () => { + renderDialog(
Song Info
, 'song') + expect(screen.getByText('Song Info')).toBeInTheDocument() + }) + + it('resolves the content by resource when given a map', () => { + renderDialog( + { album:
Album Info
, artist:
Artist Info
}, + 'artist', + ) + expect(screen.getByText('Artist Info')).toBeInTheDocument() + expect(screen.queryByText('Album Info')).not.toBeInTheDocument() + }) + + it('renders nothing when the map has no entry for the resource', () => { + renderDialog({ album:
Album Info
}, 'artist') + expect(screen.queryByText('Album Info')).not.toBeInTheDocument() + }) +}) diff --git a/ui/src/reducers/dialogReducer.js b/ui/src/reducers/dialogReducer.js index e1a77f100..740227ae8 100644 --- a/ui/src/reducers/dialogReducer.js +++ b/ui/src/reducers/dialogReducer.js @@ -127,6 +127,7 @@ export const expandInfoDialogReducer = ( previousState = { open: false, record: undefined, + resource: undefined, }, payload, ) => { @@ -137,12 +138,14 @@ export const expandInfoDialogReducer = ( ...previousState, open: true, record: payload.record, + resource: payload.resource, } case EXTENDED_INFO_CLOSE: return { ...previousState, open: false, record: undefined, + resource: undefined, } default: return previousState diff --git a/ui/src/reducers/dialogReducer.test.js b/ui/src/reducers/dialogReducer.test.js new file mode 100644 index 000000000..ad3c4b4cb --- /dev/null +++ b/ui/src/reducers/dialogReducer.test.js @@ -0,0 +1,45 @@ +import { describe, it, expect } from 'vitest' +import { expandInfoDialogReducer } from './dialogReducer' +import { EXTENDED_INFO_OPEN, EXTENDED_INFO_CLOSE } from '../actions' + +describe('expandInfoDialogReducer', () => { + it('stores the record and resource on EXTENDED_INFO_OPEN', () => { + const record = { id: 'al1', name: 'Album' } + const result = expandInfoDialogReducer( + { open: false, record: undefined, resource: undefined }, + { type: EXTENDED_INFO_OPEN, record, resource: 'album' }, + ) + + expect(result).toEqual({ open: true, record, resource: 'album' }) + }) + + it('clears the record and resource on EXTENDED_INFO_CLOSE', () => { + const previousState = { + open: true, + record: { id: 'al1', name: 'Album' }, + resource: 'album', + } + const result = expandInfoDialogReducer(previousState, { + type: EXTENDED_INFO_CLOSE, + }) + + expect(result).toEqual({ + open: false, + record: undefined, + resource: undefined, + }) + }) + + it('returns previous state for unknown action', () => { + const previousState = { + open: false, + record: undefined, + resource: undefined, + } + const result = expandInfoDialogReducer(previousState, { + type: 'UNKNOWN_ACTION', + }) + + expect(result).toBe(previousState) + }) +}) From fd46e594e1c5411ba1b97c7d5662e674747afc27 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 3 Sep 2026 23:55:40 -0400 Subject: [PATCH 11/13] feat(ui): add a Get Info dialog for artists --- ui/src/artist/ArtistInfo.jsx | 82 ++++++++++++++++++++++++ ui/src/artist/ArtistInfo.test.jsx | 39 +++++++++++ ui/src/artist/ArtistList.jsx | 3 + ui/src/artist/DesktopArtistDetails.jsx | 5 +- ui/src/common/ContextMenus.jsx | 1 - ui/src/common/ContextMenus.test.jsx | 13 +++- ui/src/dialogs/ExpandInfoDialog.test.jsx | 11 ++++ ui/src/i18n/en.json | 4 ++ 8 files changed, 154 insertions(+), 4 deletions(-) create mode 100644 ui/src/artist/ArtistInfo.jsx create mode 100644 ui/src/artist/ArtistInfo.test.jsx diff --git a/ui/src/artist/ArtistInfo.jsx b/ui/src/artist/ArtistInfo.jsx new file mode 100644 index 000000000..ad3f483d1 --- /dev/null +++ b/ui/src/artist/ArtistInfo.jsx @@ -0,0 +1,82 @@ +import Table from '@material-ui/core/Table' +import TableBody from '@material-ui/core/TableBody' +import { humanize, underscore } from 'inflection' +import TableCell from '@material-ui/core/TableCell' +import TableContainer from '@material-ui/core/TableContainer' +import TableRow from '@material-ui/core/TableRow' +import { + DateField, + TextField, + useRecordContext, + useTranslate, +} from 'react-admin' +import { makeStyles } from '@material-ui/core/styles' +import { ArtworkInfo, SizeField } from '../common' + +const useStyles = makeStyles({ + tableCell: { + width: '17.5%', + }, + value: { + whiteSpace: 'pre-line', + }, +}) + +const ArtistInfo = (props) => { + const classes = useStyles() + const translate = useTranslate() + const record = useRecordContext(props) + const data = { + name: , + sortArtistName: , + mbzArtistId: , + albumCount: , + songCount: , + size: , + playCount: , + externalInfoUpdatedAt: ( + + ), + updatedAt: , + } + + const optionalFields = [ + 'sortArtistName', + 'mbzArtistId', + 'externalInfoUpdatedAt', + ] + optionalFields.forEach((field) => { + !record[field] && delete data[field] + }) + + return ( + + + + {Object.keys(data).map((key) => { + return ( + + + {translate(`resources.artist.fields.${key}`, { + _: humanize(underscore(key)), + })} + : + + + {data[key]} + + + ) + })} + + +
+ + ) +} + +export default ArtistInfo diff --git a/ui/src/artist/ArtistInfo.test.jsx b/ui/src/artist/ArtistInfo.test.jsx new file mode 100644 index 000000000..b1b12bbba --- /dev/null +++ b/ui/src/artist/ArtistInfo.test.jsx @@ -0,0 +1,39 @@ +import React from 'react' +import { render, screen } from '@testing-library/react' +import { describe, it, expect, vi } from 'vitest' +import { RecordContextProvider } from 'react-admin' +import ArtistInfo from './ArtistInfo' + +vi.mock('../common', async (importOriginal) => ({ + ...(await importOriginal()), + ArtworkInfo: () => ( + + artwork-section + + ), +})) + +const renderWith = (record) => + render( + + + , + ) + +describe('', () => { + it('renders the artist fields', () => { + renderWith({ id: 'ar-1', name: 'Radiohead', albumCount: 9, songCount: 120 }) + expect(screen.getByText('Radiohead')).toBeInTheDocument() + expect(screen.getByText('9')).toBeInTheDocument() + }) + + it('drops optional fields that are empty', () => { + renderWith({ id: 'ar-1', name: 'Radiohead' }) + expect(screen.queryByText(/MusicBrainz/i)).toBeNull() + }) + + it('renders the artwork section', () => { + renderWith({ id: 'ar-1', name: 'Radiohead' }) + expect(screen.getByText('artwork-section')).toBeInTheDocument() + }) +}) diff --git a/ui/src/artist/ArtistList.jsx b/ui/src/artist/ArtistList.jsx index f4147221e..b34deaefe 100644 --- a/ui/src/artist/ArtistList.jsx +++ b/ui/src/artist/ArtistList.jsx @@ -35,6 +35,8 @@ import ArtistSimpleList from './ArtistSimpleList' import { DraggableTypes } from '../consts' import en from '../i18n/en.json' import { formatBytes } from '../utils/index.js' +import ArtistInfo from './ArtistInfo' +import ExpandInfoDialog from '../dialogs/ExpandInfoDialog' const useStyles = makeStyles({ contextHeader: { @@ -219,6 +221,7 @@ const ArtistList = (props) => { > + } /> ) } diff --git a/ui/src/artist/DesktopArtistDetails.jsx b/ui/src/artist/DesktopArtistDetails.jsx index 9269dc8d7..a0cddd258 100644 --- a/ui/src/artist/DesktopArtistDetails.jsx +++ b/ui/src/artist/DesktopArtistDetails.jsx @@ -9,6 +9,7 @@ import { LoveButton, RatingField, ImageUploadOverlay } from '../common' import Lightbox from 'react-image-lightbox' import ExpandInfoDialog from '../dialogs/ExpandInfoDialog' import AlbumInfo from '../album/AlbumInfo' +import ArtistInfo from './ArtistInfo' import subsonic from '../subsonic' import { SafeHTML } from '../common/SafeHTML' import { Artwork } from '../common/Artwork' @@ -160,7 +161,9 @@ const DesktopArtistDetails = ({ artistInfo, record, biography }) => { /> )} - } /> + , artist: }} + /> ) } diff --git a/ui/src/common/ContextMenus.jsx b/ui/src/common/ContextMenus.jsx index 57e672b2d..0ff23955d 100644 --- a/ui/src/common/ContextMenus.jsx +++ b/ui/src/common/ContextMenus.jsx @@ -272,7 +272,6 @@ export const ArtistContextMenu = (props) => props.record ? ( { }) describe('info dialog', () => { - // ArtistContextMenu hardcodes hideInfo, so it never renders a Get Info item; Album - // uses the same action and proves resource is threaded through the dispatch. it('dispatches the record and resource when opening the info dialog', () => { const record = { id: 'al1', name: 'Album', songCount: 1 } renderMenu(AlbumContextMenu, record) @@ -145,5 +143,16 @@ describe('ContextMenus', () => { resource: 'album', }) }) + + it('dispatches the record and resource when opening the artist info dialog', () => { + const record = { id: 'ar1', name: 'Artist', stats: {} } + renderMenu(ArtistContextMenu, record) + fireEvent.click(screen.getByText('resources.album.actions.info')) + expect(mockDispatch).toHaveBeenCalledWith({ + type: EXTENDED_INFO_OPEN, + record, + resource: 'artist', + }) + }) }) }) diff --git a/ui/src/dialogs/ExpandInfoDialog.test.jsx b/ui/src/dialogs/ExpandInfoDialog.test.jsx index 9ce5d06e5..d450b8e0e 100644 --- a/ui/src/dialogs/ExpandInfoDialog.test.jsx +++ b/ui/src/dialogs/ExpandInfoDialog.test.jsx @@ -36,6 +36,17 @@ describe('ExpandInfoDialog', () => { expect(screen.queryByText('Album Info')).not.toBeInTheDocument() }) + // Guards the artist detail page, which maps both resources to one dialog: an album + // card's Get Info must resolve to AlbumInfo, not the page's own ArtistInfo. + it('resolves the album entry, not artist, when the resource is album', () => { + renderDialog( + { album:
Album Info
, artist:
Artist Info
}, + 'album', + ) + expect(screen.getByText('Album Info')).toBeInTheDocument() + expect(screen.queryByText('Artist Info')).not.toBeInTheDocument() + }) + it('renders nothing when the map has no entry for the resource', () => { renderDialog({ album:
Album Info
}, 'artist') expect(screen.queryByText('Album Info')).not.toBeInTheDocument() diff --git a/ui/src/i18n/en.json b/ui/src/i18n/en.json index 9635cadd1..e141e6785 100644 --- a/ui/src/i18n/en.json +++ b/ui/src/i18n/en.json @@ -110,10 +110,14 @@ "name": "Artist |||| Artists", "fields": { "name": "Name", + "sortArtistName": "Sort Name", + "mbzArtistId": "MusicBrainz Artist Id", "albumCount": "Album Count", "songCount": "Song Count", "size": "Size", "playCount": "Plays", + "externalInfoUpdatedAt": "External Info Updated At", + "updatedAt": "Updated At", "rating": "Rating", "genre": "Genre", "role": "Role", From f283da06300555a5a467c1eb9ecd3898183a3349 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 4 Sep 2026 00:11:54 -0400 Subject: [PATCH 12/13] fix(artwork): surface priority names in the explain UI and tidy up loose ends Moves the CLI's priority-name lookup into core/artwork so the native API and UI can render "scan" instead of a bare priority number, replaces a brittle string comparison with a presence check on the stored artwork, renders the source path in the details section, drops the unused hash field, and aligns date rendering with the rest of the dialog. --- cmd/artwork.go | 36 ++++-------------------- cmd/artwork_test.go | 2 +- core/artwork/explain.go | 26 ++++++++++++++++- core/artwork/explain_test.go | 10 +++++++ server/nativeapi/artwork_explain.go | 16 +++++------ server/nativeapi/artwork_explain_test.go | 8 +++--- ui/src/common/ArtworkInfo.jsx | 14 ++++++--- ui/src/common/ArtworkInfo.test.jsx | 9 +++++- ui/src/i18n/en.json | 1 + 9 files changed, 73 insertions(+), 49 deletions(-) diff --git a/cmd/artwork.go b/cmd/artwork.go index 5d4c8a04b..34797d188 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -8,7 +8,6 @@ import ( "io" "os" "slices" - "strconv" "strings" "time" @@ -264,7 +263,7 @@ func configState(rep statusReport) string { func printQueueStats(w io.Writer, stats []model.ArtworkQueueStat, total int64, countHeader, indent string) { fmt.Fprintf(w, "%sKIND\tPRIORITY\t%s\n", indent, countHeader) for _, s := range stats { - fmt.Fprintf(w, "%s%s\t%s\t%d\n", indent, kindName(s.ItemKind), priorityName(s.Priority), s.Count) + fmt.Fprintf(w, "%s%s\t%s\t%d\n", indent, kindName(s.ItemKind), artwork.PriorityName(s.Priority), s.Count) } fmt.Fprintf(w, "%sTOTAL\t\t%d\n", indent, total) } @@ -276,37 +275,14 @@ func kindName(prefix string) string { return prefix } -type artworkPriority struct { - name string - value int -} - -// knownPriorities is the one listing behind both the name and the parse, so they cannot drift. -var knownPriorities = []artworkPriority{ - {"bump", model.ArtworkPriorityBump}, - {"scan", model.ArtworkPriorityScan}, - {"recheck", model.ArtworkPriorityRecheck}, - {"backfill", model.ArtworkPriorityBackfill}, -} - -// priorityName falls back to the number: a row written by a newer version still has to print. -func priorityName(p int) string { - for _, ap := range knownPriorities { - if ap.value == p { - return ap.name - } - } - return strconv.Itoa(p) -} - func priorityNames() string { - return strings.Join(slice.Map(knownPriorities, func(ap artworkPriority) string { return ap.name }), ", ") + return strings.Join(slice.Map(artwork.KnownPriorities, func(ap artwork.Priority) string { return ap.Name }), ", ") } func parseArtworkPriority(s string) (int, error) { - for _, ap := range knownPriorities { - if ap.name == s { - return ap.value, nil + for _, ap := range artwork.KnownPriorities { + if ap.Name == s { + return ap.Value, nil } } return 0, fmt.Errorf("invalid priority %q, expected one of: %s", s, priorityNames()) @@ -801,7 +777,7 @@ func formatExplain(rep artwork.ExplainReport) string { case rep.Queued == nil: fmt.Fprintln(w, " (not queued)") default: - fmt.Fprintf(w, " Priority:\t%s (%d)\n", priorityName(rep.Queued.Priority), rep.Queued.Priority) + fmt.Fprintf(w, " Priority:\t%s (%d)\n", artwork.PriorityName(rep.Queued.Priority), rep.Queued.Priority) fmt.Fprintf(w, " Attempts:\t%d\n", rep.Queued.Attempts) fmt.Fprintf(w, " Retry at:\t%s\n", formatTime(rep.Queued.RetryAt)) } diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index 0be9d85ec..88ec47007 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -940,7 +940,7 @@ var _ = Describe("parseArtworkPriority", func() { It("accepts every name status prints", func() { for _, p := range []int{model.ArtworkPriorityRecheck, model.ArtworkPriorityBackfill, model.ArtworkPriorityScan, model.ArtworkPriorityBump} { - Expect(parseArtworkPriority(priorityName(p))).To(Equal(p)) + Expect(parseArtworkPriority(artwork.PriorityName(p))).To(Equal(p)) } }) diff --git a/core/artwork/explain.go b/core/artwork/explain.go index 9001c5cb6..afea165da 100644 --- a/core/artwork/explain.go +++ b/core/artwork/explain.go @@ -132,7 +132,7 @@ func Explain(ctx context.Context, ds model.DataStore, ag *agents.Agents, kind mo } } // Set even for a kind the chain never walks, so a caller that asked to walk (e.g. --live on a - // playlist) is reported as having tried, matching the pre-refactor CLI's unconditional flag. + // playlist) is reported as having tried. rep.Walked = opts.Walk != nil if !Explainable(kind) { return rep, nil @@ -192,3 +192,27 @@ func (r ExplainReport) GaveUpAfter() []TraceStep { } return DecodeTrace(r.Stored.LastFailure, "") } + +// Priority names one of the queue's fixed priority levels. +type Priority struct { + Name string + Value int +} + +// KnownPriorities is the one listing behind both the name and the parse, so they cannot drift. +var KnownPriorities = []Priority{ + {"bump", model.ArtworkPriorityBump}, + {"scan", model.ArtworkPriorityScan}, + {"recheck", model.ArtworkPriorityRecheck}, + {"backfill", model.ArtworkPriorityBackfill}, +} + +// PriorityName falls back to the number: a row written by a newer version still has to print. +func PriorityName(p int) string { + for _, ap := range KnownPriorities { + if ap.Value == p { + return ap.Name + } + } + return strconv.Itoa(p) +} diff --git a/core/artwork/explain_test.go b/core/artwork/explain_test.go index 676f626d0..f9fa41f69 100644 --- a/core/artwork/explain_test.go +++ b/core/artwork/explain_test.go @@ -92,6 +92,16 @@ var _ = Describe("FormatAgents", func() { }) }) +var _ = Describe("PriorityName", func() { + It("names a known priority", func() { + Expect(artwork.PriorityName(model.ArtworkPriorityScan)).To(Equal("scan")) + }) + + It("falls back to the number for an unknown priority", func() { + Expect(artwork.PriorityName(999)).To(Equal("999")) + }) +}) + var _ = Describe("Explain", func() { var ds *tests.MockDataStore var artRepo *tests.MockArtworkRepo diff --git a/server/nativeapi/artwork_explain.go b/server/nativeapi/artwork_explain.go index ef3ab3987..3ec8e66d2 100644 --- a/server/nativeapi/artwork_explain.go +++ b/server/nativeapi/artwork_explain.go @@ -28,15 +28,15 @@ type traceStepDTO struct { type storedDTO struct { Source string `json:"source"` - Hash string `json:"hash,omitempty"` SourcePath string `json:"sourcePath,omitempty"` AttemptedAt string `json:"attemptedAt,omitempty"` } type queuedDTO struct { - Priority int `json:"priority"` - Attempts int `json:"attempts"` - RetryAt string `json:"retryAt,omitempty"` + Priority int `json:"priority"` + PriorityName string `json:"priorityName"` + Attempts int `json:"attempts"` + RetryAt string `json:"retryAt,omitempty"` } type configDTO struct { @@ -121,16 +121,16 @@ func toExplainDTO(rep artwork.ExplainReport) explainDTO { if rep.Stored != nil { dto.Stored = &storedDTO{ Source: rep.Stored.Source, - Hash: rep.Stored.Hash, SourcePath: rep.Stored.SourcePath, AttemptedAt: rfc3339(rep.Stored.AttemptedAt), } } if rep.Queued != nil { dto.Queued = &queuedDTO{ - Priority: rep.Queued.Priority, - Attempts: rep.Queued.Attempts, - RetryAt: rfc3339(rep.Queued.RetryAt), + Priority: rep.Queued.Priority, + PriorityName: artwork.PriorityName(rep.Queued.Priority), + Attempts: rep.Queued.Attempts, + RetryAt: rfc3339(rep.Queued.RetryAt), } } if steps := rep.LastAttemptFailed(); len(steps) > 0 { diff --git a/server/nativeapi/artwork_explain_test.go b/server/nativeapi/artwork_explain_test.go index e830254cf..782dc05b1 100644 --- a/server/nativeapi/artwork_explain_test.go +++ b/server/nativeapi/artwork_explain_test.go @@ -95,7 +95,6 @@ var _ = Describe("GET /artwork/explain", func() { Expect(got["stored"]).To(Equal(map[string]any{ "source": "external:deezer", - "hash": "abc", "sourcePath": "/music/Radiohead/folder.jpg", "attemptedAt": attemptedAt.Format(time.RFC3339), })) @@ -128,9 +127,10 @@ var _ = Describe("GET /artwork/explain", func() { var got map[string]any Expect(json.Unmarshal(w.Body.Bytes(), &got)).To(Succeed()) Expect(got["queued"]).To(Equal(map[string]any{ - "priority": float64(model.ArtworkPriorityScan), - "attempts": float64(1), - "retryAt": retryAt.Format(time.RFC3339), + "priority": float64(model.ArtworkPriorityScan), + "priorityName": "scan", + "attempts": float64(1), + "retryAt": retryAt.Format(time.RFC3339), })) Expect(got["lastAttemptFailed"]).To(Equal([]any{ map[string]any{"candidate": "external:deezer", "outcome": "error", "detail": "timeout"}, diff --git a/ui/src/common/ArtworkInfo.jsx b/ui/src/common/ArtworkInfo.jsx index b83fc71b6..1531b9a25 100644 --- a/ui/src/common/ArtworkInfo.jsx +++ b/ui/src/common/ArtworkInfo.jsx @@ -3,6 +3,7 @@ import PropTypes from 'prop-types' import { Chip, Link, TableCell, TableRow } from '@material-ui/core' import { makeStyles } from '@material-ui/core/styles' import { useDataProvider, usePermissions, useTranslate } from 'react-admin' +import { DateField } from './DateField' const OUTCOME_COLORS = { hit: '#4caf50', @@ -96,7 +97,7 @@ export const ArtworkInfo = ({ resource, id }) => { if (!isAdmin || !report) return null - const recorded = report.chainOrigin !== 'not recorded' + const recorded = !!report.stored return ( <> @@ -109,7 +110,7 @@ export const ArtworkInfo = ({ resource, id }) => { <> {report.stored?.source} - {report.stored?.attemptedAt} + ) : ( @@ -141,16 +142,21 @@ export const ArtworkInfo = ({ resource, id }) => { title={translate('artwork.gaveUpAfter')} steps={report.gaveUpAfter} /> + {report.stored?.sourcePath && ( + + {report.stored.sourcePath} + + )} {report.queued && ( <> - {report.queued.priority} + {report.queued.priorityName} {report.queued.attempts} - {report.queued.retryAt} + )} diff --git a/ui/src/common/ArtworkInfo.test.jsx b/ui/src/common/ArtworkInfo.test.jsx index 862881939..40064d5e0 100644 --- a/ui/src/common/ArtworkInfo.test.jsx +++ b/ui/src/common/ArtworkInfo.test.jsx @@ -22,7 +22,11 @@ vi.mock('react-admin', async (importOriginal) => { const report = { result: 'resolved from external:deezer', chainOrigin: 'recorded 2026-09-01T10:00:00Z', - stored: { source: 'external:deezer', attemptedAt: '2026-09-01T10:00:00Z' }, + stored: { + source: 'external:deezer', + sourcePath: '/music/Radiohead/folder.jpg', + attemptedAt: '2026-09-01T10:00:00Z', + }, steps: [ { candidate: 'cover.*', outcome: 'miss' }, { @@ -31,6 +35,7 @@ const report = { detail: 'https://cdn/x.jpg', }, ], + queued: { priority: 20, priorityName: 'scan', attempts: 1, retryAt: '' }, config: { setting: 'ArtistArtPriority', value: 'external' }, } @@ -74,6 +79,8 @@ describe('', () => { await userEvent.click(screen.getByText('artwork.showDetails')) expect(screen.getByText('cover.*')).toBeInTheDocument() expect(screen.getByText('ArtistArtPriority:')).toBeInTheDocument() + expect(screen.getByText('/music/Radiohead/folder.jpg')).toBeInTheDocument() + expect(screen.getByText('scan')).toBeInTheDocument() }) it('shows the not-recorded state', async () => { diff --git a/ui/src/i18n/en.json b/ui/src/i18n/en.json index e141e6785..d96067df9 100644 --- a/ui/src/i18n/en.json +++ b/ui/src/i18n/en.json @@ -656,6 +656,7 @@ "result": "Result", "source": "Source", "attemptedAt": "Attempted at", + "sourcePath": "Source path", "showDetails": "Show details", "hideDetails": "Hide details", "priority": "Priority", From 8ba8e7f2d71a8346660d333012ff602d8065f45a Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 4 Sep 2026 07:29:54 -0400 Subject: [PATCH 13/13] refactor: share the explain kind list and time format, trim the DTO The explainable-kind list and the report's time format each existed twice, kept in sync by a comment pointing at the other copy. Both now live in core/artwork beside their siblings. FormatAgents returned a CLI-worded sentence, so the web UI rendered starred agents with no legend at all. It now reports a bool and each caller words its own note. Drop kind, id, chainOrigin and the numeric priority from the explain response: all four were serialized and never read, and chainOrigin is derivable from stored.attemptedAt on an endpoint that never walks. ExpandInfoDialog takes an optional resource instead of a node-or-map content prop. A page mounts one dialog per resource, which removes the silent blank-dialog failure mode when a map was missing a key. Outcome chips read the MUI palette rather than hardcoded hex, so they follow the dark theme. --- cmd/artwork.go | 35 ++++++--------- cmd/artwork_test.go | 26 +++++++---- core/artwork/explain.go | 45 +++++++++---------- core/artwork/explain_test.go | 17 +++++--- core/artwork/housekeeping.go | 7 +++ server/nativeapi/artwork_explain.go | 27 +++--------- server/nativeapi/artwork_explain_test.go | 4 +- ui/src/artist/DesktopArtistDetails.jsx | 5 +-- ui/src/common/ArtworkInfo.jsx | 33 ++++++++------ ui/src/dataProvider/wrapperDataProvider.js | 6 +-- ui/src/dialogs/ExpandInfoDialog.jsx | 26 ++++++----- ui/src/dialogs/ExpandInfoDialog.test.jsx | 50 +++++++++++----------- ui/src/i18n/en.json | 1 + 13 files changed, 144 insertions(+), 138 deletions(-) diff --git a/cmd/artwork.go b/cmd/artwork.go index 34797d188..b36dee7bd 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -9,7 +9,6 @@ import ( "os" "slices" "strings" - "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" @@ -75,7 +74,7 @@ var artworkExplainCmd = &cobra.Command{ Short: "Explain why an item's artwork resolved the way it did", Long: "Explain why an item's artwork resolved the way it did.\n\n" + "The item can be given as a bare id, a full artwork id (e.g. al-), or a pair.\n" + - " is one of: " + kindPrefixes(explainKinds) + ".\n" + + " is one of: " + kindPrefixes(artwork.ExplainKinds) + ".\n" + "A disc artwork id is the album id and the disc number, joined by a colon: :2", Args: cobra.RangeArgs(1, 2), Run: func(cmd *cobra.Command, args []string) { @@ -654,13 +653,6 @@ func refreshItems(ctx context.Context, ds model.DataStore, targets []model.Artwo return failed } -// explainKinds is every kind explain accepts: it reports stored state and config too, so a kind -// with no chain to walk still has something to answer with. -var explainKinds = []model.Kind{ - model.KindArtistArtwork, model.KindAlbumArtwork, model.KindDiscArtwork, - model.KindMediaFileArtwork, model.KindPlaylistArtwork, model.KindRadioArtwork, -} - func kindPrefixes(kinds []model.Kind) string { return strings.Join(model.KindPrefixes(kinds), ", ") } @@ -725,6 +717,14 @@ func artworkKindAndID(ctx context.Context, ds model.DataStore, arg string) (mode // cliUnavailableNote marks agents the CLI cannot construct; a running server loads them all. const cliUnavailableNote = " (* not available to the CLI)" +// cliAgents words the CLI's own legend for the starred agents FormatAgents reports. +func cliAgents(rep artwork.ExplainReport) string { + if rep.AgentsIncomplete { + return rep.Agents + cliUnavailableNote + } + return rep.Agents +} + // writeSteps prints the trace rows. An empty last cell would end tabwriter's column block and // break the alignment, so a missing detail is rendered as a dash. func writeSteps(w io.Writer, indent string, steps []artwork.TraceStep) { @@ -767,7 +767,7 @@ func formatExplain(rep artwork.ExplainReport) string { if rep.Stored.SourcePath != "" { fmt.Fprintf(w, " Source path:\t%s\n", rep.Stored.SourcePath) } - fmt.Fprintf(w, " Attempted at:\t%s\n", formatTime(rep.Stored.AttemptedAt)) + fmt.Fprintf(w, " Attempted at:\t%s\n", artwork.FormatTime(rep.Stored.AttemptedAt)) } fmt.Fprintln(w, "\nQueue") @@ -779,7 +779,7 @@ func formatExplain(rep artwork.ExplainReport) string { default: fmt.Fprintf(w, " Priority:\t%s (%d)\n", artwork.PriorityName(rep.Queued.Priority), rep.Queued.Priority) fmt.Fprintf(w, " Attempts:\t%d\n", rep.Queued.Attempts) - fmt.Fprintf(w, " Retry at:\t%s\n", formatTime(rep.Queued.RetryAt)) + fmt.Fprintf(w, " Retry at:\t%s\n", artwork.FormatTime(rep.Queued.RetryAt)) } if rep.Queued != nil { writeStepTable(w, "Last attempt failed", rep.LastAttemptFailed()) @@ -794,7 +794,7 @@ func formatExplain(rep artwork.ExplainReport) string { } else { fmt.Fprintf(w, " %s:\t%s\n", setting, value) if rep.Agents != "" { - fmt.Fprintf(w, " Agents:\t%s\n", rep.Agents) + fmt.Fprintf(w, " Agents:\t%s\n", cliAgents(rep)) } } @@ -832,18 +832,11 @@ func formatExplain(rep artwork.ExplainReport) string { return sb.String() } -func formatTime(t time.Time) string { - if t.IsZero() { - return "-" - } - return t.Format(time.RFC3339) -} - func runExplain(ctx context.Context, args []string) { defer db.Init(ctx)() ds, ctx := getAdminContext(ctx) - targets, failures, err := resolveArtworkTargets(ctx, ds, args, explainKinds) + targets, failures, err := resolveArtworkTargets(ctx, ds, args, artwork.ExplainKinds) if err != nil { log.Fatal(ctx, err) } @@ -855,7 +848,7 @@ func runExplain(ctx context.Context, args []string) { } kind, id := targets[0].Kind, targets[0].ID - opts := artwork.ExplainOptions{UnavailableNote: cliUnavailableNote} + var opts artwork.ExplainOptions // Only artist and album reach an agent, and the load must precede the resolver, which reads the // same manager. Leaving ag nil elsewhere avoids handing agents.GetAgents a not-yet-loaded manager. var ag *agents.Agents diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index 88ec47007..d75501bbf 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -41,8 +41,8 @@ var _ = Describe("parseArtworkKind", func() { _, err := parseArtworkKind(prefix, valid) Expect(err).ToNot(HaveOccurred()) }, - Entry("explain reads disc artwork", "dc", explainKinds), - Entry("explain reads media file artwork", "mf", explainKinds), + Entry("explain reads disc artwork", "dc", artwork.ExplainKinds), + Entry("explain reads media file artwork", "mf", artwork.ExplainKinds), // Disc artwork has no state to clear and the worker cannot resolve it, so refresh must not // accept it: the queue row would be rejected on every drain. Entry("refresh re-queues media files", "mf", artwork.RefreshableKinds), @@ -65,7 +65,7 @@ var _ = Describe("resolveArtworkTargets", func() { }) It("accepts the explicit leader shared by every id", func() { - targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"al", "x", "y"}, explainKinds) + targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"al", "x", "y"}, artwork.ExplainKinds) Expect(err).ToNot(HaveOccurred()) Expect(failures).To(BeEmpty()) Expect(targets).To(Equal([]model.ArtworkID{ @@ -78,20 +78,20 @@ var _ = Describe("resolveArtworkTargets", func() { }) It("resolves a bare id by looking it up across tables", func() { - targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"artist1"}, explainKinds) + targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"artist1"}, artwork.ExplainKinds) Expect(err).ToNot(HaveOccurred()) Expect(failures).To(BeEmpty()) Expect(targets).To(Equal([]model.ArtworkID{{Kind: model.KindArtistArtwork, ID: "artist1"}})) }) It("reads the kind from a full artwork id prefix without a database lookup", func() { - targets, _, err := resolveArtworkTargets(ctx, ds, []string{"al-realalbum"}, explainKinds) + targets, _, err := resolveArtworkTargets(ctx, ds, []string{"al-realalbum"}, artwork.ExplainKinds) Expect(err).ToNot(HaveOccurred()) Expect(targets).To(Equal([]model.ArtworkID{{Kind: model.KindAlbumArtwork, ID: "realalbum"}})) }) It("strips the hash suffix from a full artwork id", func() { - targets, _, err := resolveArtworkTargets(ctx, ds, []string{"al-realalbum_0123456789abcdef"}, explainKinds) + targets, _, err := resolveArtworkTargets(ctx, ds, []string{"al-realalbum_0123456789abcdef"}, artwork.ExplainKinds) Expect(err).ToNot(HaveOccurred()) Expect(targets).To(Equal([]model.ArtworkID{{Kind: model.KindAlbumArtwork, ID: "realalbum"}})) }) @@ -105,7 +105,7 @@ var _ = Describe("resolveArtworkTargets", func() { }) It("collects an id that matches nothing and has no kind prefix", func() { - targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"nope"}, explainKinds) + targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"nope"}, artwork.ExplainKinds) Expect(err).ToNot(HaveOccurred()) Expect(targets).To(BeEmpty()) Expect(failures).To(HaveLen(1)) @@ -113,7 +113,7 @@ var _ = Describe("resolveArtworkTargets", func() { }) It("resolves the valid ids and collects the unresolvable ones", func() { - targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"artist1", "nope", "al-realalbum"}, explainKinds) + targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"artist1", "nope", "al-realalbum"}, artwork.ExplainKinds) Expect(err).ToNot(HaveOccurred()) Expect(targets).To(Equal([]model.ArtworkID{ {Kind: model.KindArtistArtwork, ID: "artist1"}, {Kind: model.KindAlbumArtwork, ID: "realalbum"}})) @@ -142,6 +142,16 @@ var _ = Describe("formatExplain", func() { } }) + It("explains the star on an agent the CLI could not construct", func() { + rep.AgentsIncomplete = true + + Expect(formatExplain(rep)).To(ContainSubstring("(* not available to the CLI)")) + }) + + It("leaves the agent line unadorned when every agent is available", func() { + Expect(formatExplain(rep)).ToNot(ContainSubstring("not available to the CLI")) + }) + It("reports the item, its config and the chain it walked", func() { out := formatExplain(rep) Expect(out).To(ContainSubstring("Radiohead")) diff --git a/core/artwork/explain.go b/core/artwork/explain.go index afea165da..22bf53257 100644 --- a/core/artwork/explain.go +++ b/core/artwork/explain.go @@ -67,10 +67,11 @@ func ImageAgentNames(ag *agents.Agents, kind model.Kind) []string { } // FormatAgents accounts for every configured agent: one that cannot be constructed never reaches -// the chain, so the raw list alone overstates it. unavailableNote is appended only if some are. -func FormatAgents(configured string, available []string, unavailableNote string) string { +// the chain, so the raw list alone overstates it. Those are starred, and the bool lets each +// caller word its own legend. +func FormatAgents(configured string, available []string) (string, bool) { if strings.TrimSpace(configured) == "" { - return "(none)" + return "(none)", false } var unavailable bool names := slice.Map(strings.Split(configured, ","), func(name string) string { @@ -81,11 +82,7 @@ func FormatAgents(configured string, available []string, unavailableNote string) unavailable = true return name + "*" }) - line := strings.Join(names, ", ") - if unavailable { - line += unavailableNote - } - return line + return strings.Join(names, ", "), unavailable } // ExplainOptions configures a single explain. The zero value reads history and never @@ -93,22 +90,22 @@ func FormatAgents(configured string, available []string, unavailableNote string) type ExplainOptions struct { // Walk builds a resolver that records into the trace; nil reads the recorded trace instead. Walk func(*ChainTrace) *TracingResolver - // UnavailableNote is appended to the agent line when a configured agent is missing. - UnavailableNote string } // ExplainReport is everything known about how one item's artwork resolved. type ExplainReport struct { - Kind model.Kind - ID string - Name string - Stored *model.ItemArtwork - Queued *model.ArtworkQueueItem - Steps []TraceStep - Source string - Agents string - Walked bool - ResolveErr error + Kind model.Kind + ID string + Name string + Stored *model.ItemArtwork + Queued *model.ArtworkQueueItem + Steps []TraceStep + Source string + Agents string + // AgentsIncomplete reports that some configured agent is starred in Agents. + AgentsIncomplete bool + Walked bool + ResolveErr error } // Explain gathers everything known about how kind/id's artwork resolved: stored state, the @@ -138,7 +135,7 @@ func Explain(ctx context.Context, ds model.DataStore, ag *agents.Agents, kind mo return rep, nil } if ag != nil && (kind == model.KindArtistArtwork || kind == model.KindAlbumArtwork) { - rep.Agents = FormatAgents(conf.Server.Agents, ImageAgentNames(ag, kind), opts.UnavailableNote) + rep.Agents, rep.AgentsIncomplete = FormatAgents(conf.Server.Agents, ImageAgentNames(ag, kind)) } switch { @@ -163,13 +160,13 @@ func (r ExplainReport) ChainOrigin() string { return "walked now" } if r.Stored != nil { - return "recorded " + formatAttemptedAt(r.Stored.AttemptedAt) + return "recorded " + FormatTime(r.Stored.AttemptedAt) } return "not recorded" } -// formatAttemptedAt matches the CLI's formatTime: a zero time reads as unset, not as year 1. -func formatAttemptedAt(t time.Time) string { +// FormatTime renders a timestamp for the report: a zero time reads as unset, not as year 1. +func FormatTime(t time.Time) string { if t.IsZero() { return "-" } diff --git a/core/artwork/explain_test.go b/core/artwork/explain_test.go index f9fa41f69..5a5709edc 100644 --- a/core/artwork/explain_test.go +++ b/core/artwork/explain_test.go @@ -79,16 +79,21 @@ var _ = Describe("ConfigFor", func() { var _ = Describe("FormatAgents", func() { It("reports none when nothing is configured", func() { - Expect(artwork.FormatAgents(" ", nil, "")).To(Equal("(none)")) + line, incomplete := artwork.FormatAgents(" ", nil) + Expect(line).To(Equal("(none)")) + Expect(incomplete).To(BeFalse()) }) It("keeps the configured order and marks what is unavailable", func() { - got := artwork.FormatAgents("spotify, lastfm", []string{"lastfm"}, " (* missing)") - Expect(got).To(Equal("spotify*, lastfm (* missing)")) + got, incomplete := artwork.FormatAgents("spotify, lastfm", []string{"lastfm"}) + Expect(incomplete).To(BeTrue()) + Expect(got).To(Equal("spotify*, lastfm")) }) - It("omits the note when every configured agent is available", func() { - Expect(artwork.FormatAgents("lastfm", []string{"lastfm"}, " (* missing)")).To(Equal("lastfm")) + It("reports complete when every configured agent is available", func() { + line, incomplete := artwork.FormatAgents("lastfm", []string{"lastfm"}) + Expect(line).To(Equal("lastfm")) + Expect(incomplete).To(BeFalse()) }) }) @@ -143,7 +148,7 @@ var _ = Describe("Explain", func() { Expect(rep.ChainOrigin()).To(ContainSubstring("recorded")) }) - It("renders a zero attempted-at the same way the CLI's formatTime does", func() { + It("renders a zero attempted-at as unset, not as year 1", func() { rep := artwork.ExplainReport{Stored: &model.ItemArtwork{Source: "folder"}} Expect(rep.ChainOrigin()).To(Equal("recorded -")) }) diff --git a/core/artwork/housekeeping.go b/core/artwork/housekeeping.go index 3ca452bdc..73cf13285 100644 --- a/core/artwork/housekeeping.go +++ b/core/artwork/housekeeping.go @@ -25,6 +25,13 @@ var ReprocessKinds = []model.Kind{ // artwork is read through on every request and cached by content key, so it has neither. func KeepsState(kind model.Kind) bool { return kind != model.KindDiscArtwork } +// ExplainKinds is every kind Explain accepts: it reports stored state and config too, so a kind +// with no chain to walk still has something to answer with. +var ExplainKinds = []model.Kind{ + model.KindArtistArtwork, model.KindAlbumArtwork, model.KindDiscArtwork, + model.KindMediaFileArtwork, model.KindPlaylistArtwork, model.KindRadioArtwork, +} + // RefreshableKinds is every kind Refresh can clear and re-queue, so it holds exactly the kinds // KeepsState admits. Media files are absent from ReprocessKinds but belong here: the worker // resolves them, it just never enumerates them in bulk. diff --git a/server/nativeapi/artwork_explain.go b/server/nativeapi/artwork_explain.go index 3ec8e66d2..92c51c3a6 100644 --- a/server/nativeapi/artwork_explain.go +++ b/server/nativeapi/artwork_explain.go @@ -13,13 +13,6 @@ import ( "github.com/navidrome/navidrome/model" ) -// explainableKinds is every kind the endpoint accepts, matching the CLI's explainKinds: a kind -// with no chain to walk still has stored state and config to report. -var explainableKinds = []model.Kind{ - model.KindArtistArtwork, model.KindAlbumArtwork, model.KindDiscArtwork, - model.KindMediaFileArtwork, model.KindPlaylistArtwork, model.KindRadioArtwork, -} - type traceStepDTO struct { Candidate string `json:"candidate"` Outcome string `json:"outcome"` @@ -33,7 +26,6 @@ type storedDTO struct { } type queuedDTO struct { - Priority int `json:"priority"` PriorityName string `json:"priorityName"` Attempts int `json:"attempts"` RetryAt string `json:"retryAt,omitempty"` @@ -45,11 +37,8 @@ type configDTO struct { } type explainDTO struct { - Kind string `json:"kind"` - ID string `json:"id"` Name string `json:"name"` Result string `json:"result"` - ChainOrigin string `json:"chainOrigin"` Steps []traceStepDTO `json:"steps"` Stored *storedDTO `json:"stored,omitempty"` Queued *queuedDTO `json:"queued,omitempty"` @@ -57,6 +46,7 @@ type explainDTO struct { GaveUpAfter []traceStepDTO `json:"gaveUpAfter,omitempty"` Config *configDTO `json:"config,omitempty"` Agents string `json:"agents,omitempty"` + AgentsIncomplete bool `json:"agentsIncomplete,omitempty"` } func (api *Router) addArtworkExplainRoute(r chi.Router) { @@ -67,7 +57,7 @@ func (api *Router) explainArtwork() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() kind, ok := model.ParseKind(r.URL.Query().Get("kind")) - if !ok || !slices.Contains(explainableKinds, kind) { + if !ok || !slices.Contains(artwork.ExplainKinds, kind) { http.Error(w, "invalid artwork kind", http.StatusBadRequest) return } @@ -110,13 +100,11 @@ func toStepDTOs(steps []artwork.TraceStep) []traceStepDTO { func toExplainDTO(rep artwork.ExplainReport) explainDTO { dto := explainDTO{ - Kind: rep.Kind.Prefix(), - ID: rep.ID, - Name: rep.Name, - Result: rep.Result(), - ChainOrigin: rep.ChainOrigin(), - Steps: toStepDTOs(rep.Steps), - Agents: rep.Agents, + Name: rep.Name, + Result: rep.Result(), + Steps: toStepDTOs(rep.Steps), + Agents: rep.Agents, + AgentsIncomplete: rep.AgentsIncomplete, } if rep.Stored != nil { dto.Stored = &storedDTO{ @@ -127,7 +115,6 @@ func toExplainDTO(rep artwork.ExplainReport) explainDTO { } if rep.Queued != nil { dto.Queued = &queuedDTO{ - Priority: rep.Queued.Priority, PriorityName: artwork.PriorityName(rep.Queued.Priority), Attempts: rep.Queued.Attempts, RetryAt: rfc3339(rep.Queued.RetryAt), diff --git a/server/nativeapi/artwork_explain_test.go b/server/nativeapi/artwork_explain_test.go index 782dc05b1..c8da5d88c 100644 --- a/server/nativeapi/artwork_explain_test.go +++ b/server/nativeapi/artwork_explain_test.go @@ -90,7 +90,6 @@ var _ = Describe("GET /artwork/explain", func() { Expect(json.Unmarshal(w.Body.Bytes(), &got)).To(Succeed()) Expect(got["name"]).To(Equal("Radiohead")) Expect(got["result"]).To(Equal("resolved from external:deezer")) - Expect(got["chainOrigin"]).To(ContainSubstring("recorded")) Expect(got["config"]).To(HaveKeyWithValue("setting", "ArtistArtPriority")) Expect(got["stored"]).To(Equal(map[string]any{ @@ -127,7 +126,6 @@ var _ = Describe("GET /artwork/explain", func() { var got map[string]any Expect(json.Unmarshal(w.Body.Bytes(), &got)).To(Succeed()) Expect(got["queued"]).To(Equal(map[string]any{ - "priority": float64(model.ArtworkPriorityScan), "priorityName": "scan", "attempts": float64(1), "retryAt": retryAt.Format(time.RFC3339), @@ -149,6 +147,6 @@ var _ = Describe("GET /artwork/explain", func() { Expect(got).ToNot(HaveKey("queued")) Expect(got).ToNot(HaveKey("lastAttemptFailed")) Expect(got).ToNot(HaveKey("gaveUpAfter")) - Expect(got["chainOrigin"]).To(Equal("not recorded")) + Expect(got).ToNot(HaveKey("chainOrigin")) }) }) diff --git a/ui/src/artist/DesktopArtistDetails.jsx b/ui/src/artist/DesktopArtistDetails.jsx index a0cddd258..affe87900 100644 --- a/ui/src/artist/DesktopArtistDetails.jsx +++ b/ui/src/artist/DesktopArtistDetails.jsx @@ -161,9 +161,8 @@ const DesktopArtistDetails = ({ artistInfo, record, biography }) => { /> )} - , artist: }} - /> + } /> + } /> ) } diff --git a/ui/src/common/ArtworkInfo.jsx b/ui/src/common/ArtworkInfo.jsx index 1531b9a25..056f39ecb 100644 --- a/ui/src/common/ArtworkInfo.jsx +++ b/ui/src/common/ArtworkInfo.jsx @@ -2,21 +2,19 @@ import React, { useEffect, useState } from 'react' import PropTypes from 'prop-types' import { Chip, Link, TableCell, TableRow } from '@material-ui/core' import { makeStyles } from '@material-ui/core/styles' +import clsx from 'clsx' import { useDataProvider, usePermissions, useTranslate } from 'react-admin' import { DateField } from './DateField' -const OUTCOME_COLORS = { - hit: '#4caf50', - miss: '#9e9e9e', - skipped: '#9e9e9e', - error: '#f44336', - unreadable: '#f44336', -} - -const useStyles = makeStyles({ - chip: { color: '#fff', height: 20 }, +const useStyles = makeStyles((theme) => ({ + chip: { color: theme.palette.common.white, height: 20 }, + hit: { backgroundColor: theme.palette.success.main }, + error: { backgroundColor: theme.palette.error.main }, + neutral: { backgroundColor: theme.palette.grey[500] }, toggle: { cursor: 'pointer' }, -}) +})) + +const OUTCOME_CLASS = { hit: 'hit', error: 'error', unreadable: 'error' } const OutcomeChip = ({ outcome }) => { const classes = useStyles() @@ -24,8 +22,10 @@ const OutcomeChip = ({ outcome }) => { ) } @@ -164,7 +164,12 @@ export const ArtworkInfo = ({ resource, id }) => { {report.config.value} )} {report.agents && ( - {report.agents} + + {report.agents} + {report.agentsIncomplete && ( +
{translate('artwork.agentsIncomplete')}
+ )} + )} )} diff --git a/ui/src/dataProvider/wrapperDataProvider.js b/ui/src/dataProvider/wrapperDataProvider.js index 3af601adf..345478e28 100644 --- a/ui/src/dataProvider/wrapperDataProvider.js +++ b/ui/src/dataProvider/wrapperDataProvider.js @@ -4,7 +4,7 @@ import { REST_URL } from '../consts' const dataProvider = jsonServerProvider(REST_URL, httpClient) -const REFRESH_KIND = { album: 'al', artist: 'ar' } +const ARTWORK_KIND = { album: 'al', artist: 'ar' } const isAdmin = () => { const role = localStorage.getItem('role') @@ -226,12 +226,12 @@ const wrapperDataProvider = { // The endpoint answers 204 with no body, but react-admin rejects any response without a // `data` key, so the id stands in for one. refreshMetadata: (resource, id) => - httpClient(`${REST_URL}/metadata/${REFRESH_KIND[resource]}/${id}/refresh`, { + httpClient(`${REST_URL}/metadata/${ARTWORK_KIND[resource]}/${id}/refresh`, { method: 'POST', }).then(() => ({ data: { id } })), explainArtwork: (resource, id) => httpClient( - `${REST_URL}/artwork/explain?kind=${REFRESH_KIND[resource]}&id=${encodeURIComponent(id)}`, + `${REST_URL}/artwork/explain?kind=${ARTWORK_KIND[resource]}&id=${encodeURIComponent(id)}`, ).then(({ json }) => ({ data: json })), } diff --git a/ui/src/dialogs/ExpandInfoDialog.jsx b/ui/src/dialogs/ExpandInfoDialog.jsx index b02c1c451..d847b386f 100644 --- a/ui/src/dialogs/ExpandInfoDialog.jsx +++ b/ui/src/dialogs/ExpandInfoDialog.jsx @@ -11,14 +11,16 @@ import { } from '@material-ui/core' import { closeExtendedInfoDialog } from '../actions' -const ExpandInfoDialog = ({ title, content }) => { - const { open, record, resource } = useSelector( - (state) => state.expandInfoDialog, - ) +const ExpandInfoDialog = ({ title, content, resource }) => { + const { + open, + record, + resource: openFor, + } = useSelector((state) => state.expandInfoDialog) const dispatch = useDispatch() const translate = useTranslate() - // A node renders for any record; a map lets one page serve more than one resource. - const body = React.isValidElement(content) ? content : content?.[resource] + // One page may mount several of these; each claims the resource it was given. + const mine = !resource || resource === openFor const handleClose = (e) => { dispatch(closeExtendedInfoDialog()) @@ -27,7 +29,7 @@ const ExpandInfoDialog = ({ title, content }) => { return ( { {translate(title || 'resources.song.actions.info')} - {record && body && ( - {body} + {record && mine && ( + + {content} + )} @@ -52,8 +56,8 @@ const ExpandInfoDialog = ({ title, content }) => { ExpandInfoDialog.propTypes = { title: PropTypes.string, - content: PropTypes.oneOfType([PropTypes.element, PropTypes.object]) - .isRequired, + content: PropTypes.element.isRequired, + resource: PropTypes.string, } export default ExpandInfoDialog diff --git a/ui/src/dialogs/ExpandInfoDialog.test.jsx b/ui/src/dialogs/ExpandInfoDialog.test.jsx index d450b8e0e..966b15975 100644 --- a/ui/src/dialogs/ExpandInfoDialog.test.jsx +++ b/ui/src/dialogs/ExpandInfoDialog.test.jsx @@ -4,51 +4,51 @@ import { render, screen, cleanup } from '@testing-library/react' import { describe, afterEach, it, expect } from 'vitest' import ExpandInfoDialog from './ExpandInfoDialog' -const renderDialog = (content, resource) => +const renderDialogs = (openFor, dialogs) => render( - + {dialogs} , ) describe('ExpandInfoDialog', () => { afterEach(cleanup) - it('renders a node content as-is, regardless of resource', () => { - renderDialog(
Song Info
, 'song') + it('renders an unclaimed dialog for any resource', () => { + renderDialogs('song', Song Info} />) expect(screen.getByText('Song Info')).toBeInTheDocument() }) - it('resolves the content by resource when given a map', () => { - renderDialog( - { album:
Album Info
, artist:
Artist Info
}, + // Guards the artist detail page, which mounts both dialogs: an album card's Get Info + // must open AlbumInfo, and the page's own ArtistInfo must stay shut. + it.each([ + ['artist', 'Artist Info', 'Album Info'], + ['album', 'Album Info', 'Artist Info'], + ])('opens only the %s dialog', (openFor, shown, hidden) => { + renderDialogs( + openFor, + <> + Album Info} /> + Artist Info} /> + , + ) + expect(screen.getByText(shown)).toBeInTheDocument() + expect(screen.queryByText(hidden)).not.toBeInTheDocument() + }) + + it('stays shut when no mounted dialog claims the resource', () => { + renderDialogs( 'artist', + Album Info} />, ) - expect(screen.getByText('Artist Info')).toBeInTheDocument() - expect(screen.queryByText('Album Info')).not.toBeInTheDocument() - }) - - // Guards the artist detail page, which maps both resources to one dialog: an album - // card's Get Info must resolve to AlbumInfo, not the page's own ArtistInfo. - it('resolves the album entry, not artist, when the resource is album', () => { - renderDialog( - { album:
Album Info
, artist:
Artist Info
}, - 'album', - ) - expect(screen.getByText('Album Info')).toBeInTheDocument() - expect(screen.queryByText('Artist Info')).not.toBeInTheDocument() - }) - - it('renders nothing when the map has no entry for the resource', () => { - renderDialog({ album:
Album Info
}, 'artist') expect(screen.queryByText('Album Info')).not.toBeInTheDocument() }) }) diff --git a/ui/src/i18n/en.json b/ui/src/i18n/en.json index d96067df9..48e111dac 100644 --- a/ui/src/i18n/en.json +++ b/ui/src/i18n/en.json @@ -665,6 +665,7 @@ "lastAttemptFailed": "Last attempt failed", "gaveUpAfter": "Gave up after", "agents": "Agents", + "agentsIncomplete": "* This agent is configured but cannot supply images", "notRecorded": "No resolution recorded yet" }, "player": {