From 8ba8e7f2d71a8346660d333012ff602d8065f45a Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 4 Sep 2026 07:29:54 -0400 Subject: [PATCH] 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": {