From ab8235dd0bcef829af85ca74b3ab5e6410d262d7 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 1 May 2026 23:37:17 -0400 Subject: [PATCH] fix(admin): approve flow surfaces real errors; auto-default Lidarr picks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "I can't approve a request, the toast just says unknown" bug was four problems compounded: 1. Lidarr config let enabled=true save with default_quality_profile_id=0 and default_root_folder_path=''. Approve then sent invalid POST bodies to Lidarr, which 5xx'd. 2. lidarr.client.post() discarded Lidarr's response body on error, so we couldn't tell why Lidarr 5xx'd from server logs. 3. handleApproveRequest's error switch didn't map ErrServerError or ErrLookupFailed — both fell through to a generic 500 server_error. 4. apiFetch only parsed {error: {code, message}} envelopes, but admin endpoints write {error: 'code_string'}. Every admin error toast rendered as 'unknown'. Fixes: - internal/lidarr/client.go: capture up to 512 bytes of Lidarr's response body when it returns 4xx/5xx; include in the wrapped error so server logs show what Lidarr actually said instead of just the status bucket. - internal/lidarrrequests/service.go: new ErrDefaultsIncomplete fires before the Lidarr call when QP=0 or root_folder=''. Stops the bad POST entirely. - internal/api/admin_requests.go: handleApproveRequest now maps ErrDefaultsIncomplete -> 'lidarr_defaults_incomplete' (400), ErrServerError -> 'lidarr_server_error' (502), ErrLookupFailed -> 'lidarr_rejected' (502). - internal/api/admin_lidarr.go: handlePutLidarrConfig now requires QP + root folder to be set whenever enabled=true. - web/src/lib/api/client.ts: apiFetch handles both error envelope shapes so admin error codes propagate to toasts. - web/src/routes/admin/integrations/+page.svelte: auto-default to the first quality profile and first root folder Lidarr returns when the operator hasn't picked one yet — saves a click for typical one-profile/one-folder home setups. - web/src/routes/admin/requests/+page.svelte: friendly toast copy for the new error codes. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/api/admin_lidarr.go | 8 +++++++ internal/api/admin_requests.go | 12 ++++++++++ internal/lidarr/client.go | 22 +++++++++++++++---- internal/lidarrrequests/service.go | 9 ++++++++ web/src/lib/api/client.ts | 21 +++++++++++++----- .../routes/admin/integrations/+page.svelte | 15 +++++++++++++ web/src/routes/admin/requests/+page.svelte | 10 ++++++++- 7 files changed, 86 insertions(+), 11 deletions(-) diff --git a/internal/api/admin_lidarr.go b/internal/api/admin_lidarr.go index c21af7b0..0081b1da 100644 --- a/internal/api/admin_lidarr.go +++ b/internal/api/admin_lidarr.go @@ -94,6 +94,14 @@ func (h *handlers) handlePutLidarrConfig(w http.ResponseWriter, r *http.Request) writeAdminJSONErr(w, http.StatusBadRequest, "invalid_url") return } + // Defaults must be present so future Approve calls don't dispatch + // invalid POST bodies to Lidarr (which produces 5xx with no useful + // detail). The frontend pre-selects first values so this almost + // never fires for non-malicious traffic, but it's the right gate. + if body.DefaultQualityProfileID == 0 || body.DefaultRootFolderPath == "" { + writeAdminJSONErr(w, http.StatusBadRequest, "missing_defaults") + return + } } cfg := lidarrconfig.Config{ diff --git a/internal/api/admin_requests.go b/internal/api/admin_requests.go index 63bf97ab..e8ffa0e4 100644 --- a/internal/api/admin_requests.go +++ b/internal/api/admin_requests.go @@ -94,6 +94,8 @@ func (h *handlers) handleApproveRequest(w http.ResponseWriter, r *http.Request) switch { case errors.Is(err, lidarrrequests.ErrLidarrDisabled): writeAdminJSONErr(w, http.StatusServiceUnavailable, "lidarr_disabled") + case errors.Is(err, lidarrrequests.ErrDefaultsIncomplete): + writeAdminJSONErr(w, http.StatusBadRequest, "lidarr_defaults_incomplete") case errors.Is(err, lidarrrequests.ErrNotFound): writeAdminJSONErr(w, http.StatusNotFound, "request_not_found") case errors.Is(err, lidarrrequests.ErrNotPending): @@ -102,6 +104,16 @@ func (h *handlers) handleApproveRequest(w http.ResponseWriter, r *http.Request) writeAdminJSONErr(w, http.StatusServiceUnavailable, "lidarr_unreachable") case errors.Is(err, lidarr.ErrAuthFailed): writeAdminJSONErr(w, http.StatusServiceUnavailable, "lidarr_auth_failed") + case errors.Is(err, lidarr.ErrServerError): + // Lidarr itself 5xx'd — its response body is in err's wrapped + // message (see lidarr.readBodySnippet). Logged below for ops. + h.logger.Error("admin: approve request: lidarr 5xx", "err", err) + writeAdminJSONErr(w, http.StatusBadGateway, "lidarr_server_error") + case errors.Is(err, lidarr.ErrLookupFailed): + // Lidarr returned 4xx — usually "artist already exists" or + // "no metadata found." Log so ops can see the actual reason. + h.logger.Warn("admin: approve request: lidarr 4xx", "err", err) + writeAdminJSONErr(w, http.StatusBadGateway, "lidarr_rejected") default: h.logger.Error("admin: approve request", "err", err) writeAdminJSONErr(w, http.StatusInternalServerError, "server_error") diff --git a/internal/lidarr/client.go b/internal/lidarr/client.go index da531e97..f75399e4 100644 --- a/internal/lidarr/client.go +++ b/internal/lidarr/client.go @@ -57,16 +57,28 @@ func (c *Client) get(ctx context.Context, path string, q url.Values) (*http.Resp return nil, ErrAuthFailed } if resp.StatusCode >= 500 { + body := readBodySnippet(resp.Body) _ = resp.Body.Close() - return nil, ErrServerError + return nil, fmt.Errorf("%w: %d: %s", ErrServerError, resp.StatusCode, body) } if resp.StatusCode >= 400 { + body := readBodySnippet(resp.Body) _ = resp.Body.Close() - return nil, ErrLookupFailed + return nil, fmt.Errorf("%w: %d: %s", ErrLookupFailed, resp.StatusCode, body) } return resp, nil } +// readBodySnippet reads up to 512 bytes from an error response body so the +// wrapped error message can include Lidarr's actual complaint (e.g. +// `{"errorMessage":"Artist already exists in your library"}`). Without this, +// callers see only the status-code bucket and can't diagnose without +// cross-referencing Lidarr's own logs. +func readBodySnippet(r io.Reader) string { + buf, _ := io.ReadAll(io.LimitReader(r, 512)) + return strings.TrimSpace(string(buf)) +} + // post is the shared POST helper. body must be marshaled JSON. It maps HTTP // status codes to typed errors; the caller is responsible for closing the // returned body. @@ -91,12 +103,14 @@ func (c *Client) post(ctx context.Context, path string, body []byte) (*http.Resp return nil, ErrAuthFailed } if resp.StatusCode >= 500 { + body := readBodySnippet(resp.Body) _ = resp.Body.Close() - return nil, ErrServerError + return nil, fmt.Errorf("%w: %d: %s", ErrServerError, resp.StatusCode, body) } if resp.StatusCode >= 400 { + body := readBodySnippet(resp.Body) _ = resp.Body.Close() - return nil, ErrLookupFailed + return nil, fmt.Errorf("%w: %d: %s", ErrLookupFailed, resp.StatusCode, body) } return resp, nil } diff --git a/internal/lidarrrequests/service.go b/internal/lidarrrequests/service.go index 7a9cc0be..7f2d9b07 100644 --- a/internal/lidarrrequests/service.go +++ b/internal/lidarrrequests/service.go @@ -24,6 +24,12 @@ var ( ErrNotPending = errors.New("lidarrrequests: request is not pending") ErrNotFound = errors.New("lidarrrequests: request not found") ErrLidarrDisabled = errors.New("lidarrrequests: lidarr not configured") + // ErrDefaultsIncomplete fires when Lidarr is enabled but the operator + // hasn't picked a default quality profile or root folder. Without those, + // Lidarr's add-artist/add-album endpoints reject the payload with a + // generic 5xx — surfacing a typed error here lets the admin UI tell the + // operator exactly what to fix. + ErrDefaultsIncomplete = errors.New("lidarrrequests: default quality profile or root folder not configured") ) // CreateParams is the input for a new request from a user. @@ -168,6 +174,9 @@ func (s *Service) Approve(ctx context.Context, requestID pgtype.UUID, adminID pg if ov.RootFolderPath != "" { rf = ov.RootFolderPath } + if qp == 0 || rf == "" { + return dbq.LidarrRequest{}, ErrDefaultsIncomplete + } switch row.Kind { case dbq.LidarrRequestKindArtist: diff --git a/web/src/lib/api/client.ts b/web/src/lib/api/client.ts index 632e6ba7..2a030a03 100644 --- a/web/src/lib/api/client.ts +++ b/web/src/lib/api/client.ts @@ -30,12 +30,21 @@ export async function apiFetch(path: string, init?: RequestInit): Promise { + if (qualityId === 0 && profiles.data && profiles.data.length > 0) { + qualityId = profiles.data[0].id; + } + }); + $effect(() => { + if (rootPath === '' && folders.data && folders.data.length > 0) { + rootPath = folders.data[0].path; + } + }); + // The dropdown queries only fire once Lidarr is configured; otherwise the // backend has no client to call and would 4xx. const profilesEnabled = $derived(!!config.data?.enabled); diff --git a/web/src/routes/admin/requests/+page.svelte b/web/src/routes/admin/requests/+page.svelte index a97e94be..b7d2268b 100644 --- a/web/src/routes/admin/requests/+page.svelte +++ b/web/src/routes/admin/requests/+page.svelte @@ -70,13 +70,21 @@ function errorCopy(code: string): string { switch (code) { case 'lidarr_unreachable': - return "Lidarr is unreachable right now. Try again, or check Settings → Integrations."; + return "Lidarr is unreachable right now. Try again, or check Admin → Integrations."; case 'lidarr_disabled': return 'Lidarr integration is not enabled.'; case 'lidarr_auth_failed': return 'Lidarr authentication failed.'; + case 'lidarr_defaults_incomplete': + return 'Lidarr is missing a default quality profile or root folder. Set them in Admin → Integrations.'; + case 'lidarr_server_error': + return "Lidarr returned an error. Check Lidarr's logs for the cause."; + case 'lidarr_rejected': + return 'Lidarr rejected the request — usually means the artist or album is already in your library.'; case 'request_not_pending': return 'This request is no longer pending.'; + case 'request_not_found': + return 'That request no longer exists.'; default: return code ? code : "Couldn't reach Lidarr."; }