feat(music): add ListenBrainz provider #3123
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces ListenBrainz music metadata provider support to Seerr. It adds an API client with metadata/statistics queries, backend settings routes with SSRF-safe validation, a React settings component with configuration forms, and complete user documentation and localization. ChangesMusic Metadata Provider Integration
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsUI as Settings UI
participant API as Backend Routes
participant Settings as Settings Store
participant LB as ListenBrainzAPI
User->>SettingsUI: Load settings page
SettingsUI->>API: GET /settings/music-metadata
API->>Settings: get musicMetadata
Settings-->>API: {listenbrainz: {...}}
API-->>SettingsUI: 200 + settings
SettingsUI-->>User: Show form with current config
User->>SettingsUI: Enter API base URL<br/>and click Test
SettingsUI->>API: POST /settings/music-metadata/test<br/>{apiBaseUrl: "...", ...}
API->>API: validateBaseUrl(apiBaseUrl)
API->>LB: new ListenBrainzAPI(candidateSettings)
LB->>LB: getFreshReleases()
alt test success
LB-->>API: response
API-->>SettingsUI: 200 + {listenbrainz: success}
else test failure
LB-->>API: error
API-->>SettingsUI: 500 + {listenbrainz: failed}
end
SettingsUI-->>User: Update status badge
User->>SettingsUI: Click Save
SettingsUI->>API: PUT /settings/music-metadata<br/>{apiBaseUrl, webBaseUrl, userToken}
API->>API: validateBaseUrl + merge with saved
API->>Settings: set musicMetadata
Settings-->>API: persisted
API-->>SettingsUI: 200 + updated settings
SettingsUI-->>User: Show success toast
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds ListenBrainz as a configurable music metadata provider, including UI configuration, server-side settings storage/routes, and API client support.
Changes:
- Introduces ListenBrainz API client + cache entry and exposes music-metadata settings in server settings.
- Adds Settings UI for configuring/testing ListenBrainz (API base URL, web base URL, user token) and expands provider status badges/toasts.
- Documents and specifies the new endpoints in docs and OpenAPI, plus adds route and API client tests.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/i18n/locale/en.json | Adds i18n strings for ListenBrainz/music metadata settings UI. |
| src/components/Settings/SettingsMetadata.tsx | Adds ListenBrainz status + a new “music metadata” configuration form and test logic. |
| server/routes/settings/musicMetadata.ts | New Express routes for reading/updating/testing music metadata settings with URL sanitization. |
| server/routes/settings/musicMetadata.test.ts | Tests for the new music metadata settings routes (GET/PUT/test). |
| server/routes/settings/index.ts | Wires the new /settings/music-metadata router. |
| server/lib/settings/index.ts | Adds persisted musicMetadata settings (ListenBrainz config) with defaults and accessors. |
| server/lib/cache.ts | Adds a dedicated cache bucket for ListenBrainz. |
| server/api/listenbrainz/* | Implements ListenBrainz client + interfaces + tests. |
| seerr-api.yml | Adds OpenAPI schemas and endpoints for music-metadata settings and test. |
| docs/using-seerr/settings/metadata.md | Documents the new metadata providers page behavior and ListenBrainz configuration. |
| package.json | Formatting-only change (final brace alignment). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function sanitizeProviderBaseUrl(rawUrl: string, field: string): string { | ||
| let parsed: URL; | ||
| try { | ||
| parsed = new URL(rawUrl); | ||
| } catch { | ||
| throw new Error(`Invalid URL for ${field}`); | ||
| } | ||
| if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { | ||
| throw new Error(`Unsupported protocol for ${field}: ${parsed.protocol}`); | ||
| } | ||
| if (!parsed.hostname) { | ||
| throw new Error(`Missing hostname for ${field}`); | ||
| } | ||
| if (parsed.username || parsed.password) { | ||
| throw new Error(`Embedded credentials are not allowed for ${field}`); | ||
| } | ||
| return parsed.toString().replace(/\/+$/, ''); | ||
| } |
| /** | ||
| * Validate a user-supplied base URL before it is used to construct an | ||
| * outbound HTTP client. This is the boundary check that prevents SSRF via | ||
| * exotic protocol handlers, embedded credentials, or malformed inputs. | ||
| * Returns the canonical URL string on success and throws an Error with a | ||
| * human-readable message on failure. | ||
| */ |
| const { data: musicData, mutate: mutateMusic } = | ||
| useSWR<MusicMetadataSettings>('/api/v1/settings/music-metadata'); | ||
|
|
| {!musicInitialValues && <LoadingSpinner />} | ||
| {musicInitialValues && ( |
| put: | ||
| summary: Update music metadata provider settings | ||
| description: Updates ListenBrainz settings. Partial updates are merged with the current values. | ||
| tags: | ||
| - settings | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/MusicMetadataSettings' |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
server/routes/settings/musicMetadata.test.ts (1)
108-171: ⚡ Quick winAdd negative tests for the URL validation boundary.
The suite validates happy paths and upstream failures, but it doesn’t lock the SSRF guard behavior (malformed URL, unsupported protocol, embedded credentials) for
PUT /andPOST /test. Adding these cases will prevent silent regressions in the boundary checks.Suggested test additions
+it('rejects unsupported protocols on PUT / with 400', async () => { + const res = await request(app).put('/').send({ + listenbrainz: { + apiBaseUrl: 'file:///etc/passwd', + webBaseUrl: 'https://listenbrainz.org', + userToken: '', + }, + }); + assert.equal(res.status, 400); +}); + +it('rejects embedded credentials on POST /test with 400 and not tested', async () => { + const res = await request(app).post('/test').send({ + listenbrainz: { + apiBaseUrl: 'https://user:pass@example.com', + webBaseUrl: 'https://listenbrainz.org', + userToken: '', + }, + }); + assert.equal(res.status, 400); + assert.equal(res.body.tests.listenbrainz, 'not tested'); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/settings/musicMetadata.test.ts` around lines 108 - 171, Add negative tests that assert the SSRF/URL validation for both POST /test and PUT / rejects malformed URLs, unsupported protocols (e.g. ftp://), and URLs with embedded credentials; for each case send candidate config bodies with the bad base URLs and verify the server returns a validation error status (4xx) and marks the provider as failed, and also assert no external requests were made by checking captureExternalAPI's getCalls is empty for those requests. Locate the test suite in musicMetadata.test.ts (the describe 'POST /test' block) and add new it() cases that reuse captureExternalAPI/getCalls/getImpl patterns and the same endpoints ('/test' and the PUT '/') to ensure the SSRF guard behavior is enforced and prevents outbound calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@seerr-api.yml`:
- Around line 544-549: The OpenAPI schemas MusicMetadataSettings and
MusicMetadataTestResult currently only reference ListenBrainzSettings; add
support for MusicBrainz by adding a musicbrainz property that $ref's the
existing (or new) MusicBrainzSettings schema and ensure both schemas reflect the
multi-provider design (e.g., include both listenbrainz and musicbrainz
properties in MusicMetadataSettings, and include corresponding result fields or
a provider-specific object in MusicMetadataTestResult). Update occurrences
mentioned (around lines 555-561 and 2695-2709) to match this pattern and
reference the unique schema names MusicMetadataSettings,
MusicMetadataTestResult, ListenBrainzSettings, and MusicBrainzSettings so
generated clients see both providers.
In `@server/api/listenbrainz/index.ts`:
- Around line 19-23: The resolver functions (e.g., resolveListenBrainzApiUrl)
currently check apiBaseUrl truthiness without trimming, so strings like " "
bypass the default; update resolveListenBrainzApiUrl (and the similar resolver
around lines 35-36) to trim whitespace from apiBaseUrl first (e.g., const input
= (apiBaseUrl || '').trim()), then use the trimmed value when deciding to fall
back to 'https://api.listenbrainz.org' and when stripping trailing slashes;
ensure all subsequent normalization operates on the trimmed variable.
In `@server/routes/settings/musicMetadata.ts`:
- Around line 38-47: validateMusicMetadataUrls currently calls
sanitizeProviderBaseUrl but throws away the returned canonicalized strings;
change it to capture and assign the sanitized results back into
settings.listenbrainz.apiBaseUrl and settings.listenbrainz.webBaseUrl so the
normalized values are the ones persisted. Specifically, replace the two calls
inside validateMusicMetadataUrls to set settings.listenbrainz.apiBaseUrl =
sanitizeProviderBaseUrl(... ) and settings.listenbrainz.webBaseUrl =
sanitizeProviderBaseUrl(...), ensuring the saved configuration uses these
canonicalized values.
In `@src/components/Settings/SettingsMetadata.tsx`:
- Around line 119-121: The component is only reading musicData from useSWR and
shows <LoadingSpinner /> while missing, causing an endless spinner on fetch
failure; update the useSWR call in SettingsMetadata.tsx to also destructure the
error (e.g., const { data: musicData, error: musicError, mutate: mutateMusic } =
useSWR(...)) and then change the render logic around where <LoadingSpinner /> is
returned (symbols: musicData, musicError, mutateMusic, LoadingSpinner) to show a
user-visible error state (message + retry action that calls mutateMusic) when
musicError exists instead of the spinner; keep showing LoadingSpinner only when
no error and no data.
---
Nitpick comments:
In `@server/routes/settings/musicMetadata.test.ts`:
- Around line 108-171: Add negative tests that assert the SSRF/URL validation
for both POST /test and PUT / rejects malformed URLs, unsupported protocols
(e.g. ftp://), and URLs with embedded credentials; for each case send candidate
config bodies with the bad base URLs and verify the server returns a validation
error status (4xx) and marks the provider as failed, and also assert no external
requests were made by checking captureExternalAPI's getCalls is empty for those
requests. Locate the test suite in musicMetadata.test.ts (the describe 'POST
/test' block) and add new it() cases that reuse
captureExternalAPI/getCalls/getImpl patterns and the same endpoints ('/test' and
the PUT '/') to ensure the SSRF guard behavior is enforced and prevents outbound
calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f7355ab-8f30-42f5-9f19-4fc5cbbf054d
📒 Files selected for processing (13)
docs/using-seerr/settings/metadata.mdpackage.jsonseerr-api.ymlserver/api/listenbrainz/index.tsserver/api/listenbrainz/interfaces.tsserver/api/listenbrainz/listenbrainz.test.tsserver/lib/cache.tsserver/lib/settings/index.tsserver/routes/settings/index.tsserver/routes/settings/musicMetadata.test.tsserver/routes/settings/musicMetadata.tssrc/components/Settings/SettingsMetadata.tsxsrc/i18n/locale/en.json
| MusicMetadataSettings: | ||
| type: object | ||
| properties: | ||
| listenbrainz: | ||
| $ref: '#/components/schemas/ListenBrainzSettings' | ||
| MusicMetadataTestResult: |
There was a problem hiding this comment.
OpenAPI contract is missing MusicBrainz in music metadata schemas.
MusicMetadataSettings and MusicMetadataTestResult currently expose only listenbrainz, but this feature set is documented and implemented as multi-provider (MusicBrainz + ListenBrainz). This creates a contract break for API consumers and generated clients.
Proposed schema adjustment
+ MusicBrainzSettings:
+ type: object
+ properties:
+ baseUrl:
+ type: string
+ example: 'https://musicbrainz.org'
+ authToken:
+ type: string
+ example: ''
+ maxRPS:
+ type: number
+ example: 1
MusicMetadataSettings:
type: object
properties:
+ musicbrainz:
+ $ref: '`#/components/schemas/MusicBrainzSettings`'
listenbrainz:
$ref: '`#/components/schemas/ListenBrainzSettings`'
MusicMetadataTestResult:
type: object
properties:
success:
type: boolean
example: true
tests:
type: object
properties:
+ musicbrainz:
+ type: string
+ enum: [ok, failed, 'not tested']
+ example: 'ok'
listenbrainz:
type: string
enum: [ok, failed, 'not tested']
example: 'ok'Also applies to: 555-561, 2695-2709
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@seerr-api.yml` around lines 544 - 549, The OpenAPI schemas
MusicMetadataSettings and MusicMetadataTestResult currently only reference
ListenBrainzSettings; add support for MusicBrainz by adding a musicbrainz
property that $ref's the existing (or new) MusicBrainzSettings schema and ensure
both schemas reflect the multi-provider design (e.g., include both listenbrainz
and musicbrainz properties in MusicMetadataSettings, and include corresponding
result fields or a provider-specific object in MusicMetadataTestResult). Update
occurrences mentioned (around lines 555-561 and 2695-2709) to match this pattern
and reference the unique schema names MusicMetadataSettings,
MusicMetadataTestResult, ListenBrainzSettings, and MusicBrainzSettings so
generated clients see both providers.
| export const resolveListenBrainzApiUrl = (apiBaseUrl: string): string => { | ||
| const trimmed = (apiBaseUrl || 'https://api.listenbrainz.org').replace( | ||
| /\/+$/, | ||
| '' | ||
| ); |
There was a problem hiding this comment.
Trim whitespace before URL fallback/normalization.
' ' is truthy, so these resolvers currently produce invalid URLs (e.g. ' /1') instead of falling back to defaults.
Proposed fix
export const resolveListenBrainzApiUrl = (apiBaseUrl: string): string => {
- const trimmed = (apiBaseUrl || 'https://api.listenbrainz.org').replace(
+ const normalized = (apiBaseUrl ?? '').trim();
+ const trimmed = (normalized || 'https://api.listenbrainz.org').replace(
/\/+$/,
''
);
@@
export const resolveListenBrainzWebUrl = (webBaseUrl: string): string => {
- return (webBaseUrl || 'https://listenbrainz.org').replace(/\/+$/, '');
+ const normalized = (webBaseUrl ?? '').trim();
+ return (normalized || 'https://listenbrainz.org').replace(/\/+$/, '');
};Also applies to: 35-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/api/listenbrainz/index.ts` around lines 19 - 23, The resolver
functions (e.g., resolveListenBrainzApiUrl) currently check apiBaseUrl
truthiness without trimming, so strings like " " bypass the default; update
resolveListenBrainzApiUrl (and the similar resolver around lines 35-36) to trim
whitespace from apiBaseUrl first (e.g., const input = (apiBaseUrl ||
'').trim()), then use the trimmed value when deciding to fall back to
'https://api.listenbrainz.org' and when stripping trailing slashes; ensure all
subsequent normalization operates on the trimmed variable.
| function validateMusicMetadataUrls(settings: MusicMetadataSettings): void { | ||
| sanitizeProviderBaseUrl( | ||
| settings.listenbrainz.apiBaseUrl, | ||
| 'listenbrainz.apiBaseUrl' | ||
| ); | ||
| sanitizeProviderBaseUrl( | ||
| settings.listenbrainz.webBaseUrl, | ||
| 'listenbrainz.webBaseUrl' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Persist canonicalized URL values after validation.
Line 90 validates URLs, but the canonicalized values returned by sanitizeProviderBaseUrl() are discarded, and Line 98 saves unsanitized input. That defeats the normalization step (e.g., trailing-slash trimming) and can leave stored config non-canonical.
Suggested fix
-function validateMusicMetadataUrls(settings: MusicMetadataSettings): void {
- sanitizeProviderBaseUrl(
- settings.listenbrainz.apiBaseUrl,
- 'listenbrainz.apiBaseUrl'
- );
- sanitizeProviderBaseUrl(
- settings.listenbrainz.webBaseUrl,
- 'listenbrainz.webBaseUrl'
- );
+function sanitizeMusicMetadataUrls(
+ settings: MusicMetadataSettings
+): MusicMetadataSettings {
+ return {
+ listenbrainz: {
+ ...settings.listenbrainz,
+ apiBaseUrl: sanitizeProviderBaseUrl(
+ settings.listenbrainz.apiBaseUrl,
+ 'listenbrainz.apiBaseUrl'
+ ),
+ webBaseUrl: sanitizeProviderBaseUrl(
+ settings.listenbrainz.webBaseUrl,
+ 'listenbrainz.webBaseUrl'
+ ),
+ },
+ };
}
@@
- const updated = mergeCandidateSettings(settings.musicMetadata, body);
+ const candidate = mergeCandidateSettings(settings.musicMetadata, body);
+ let updated: MusicMetadataSettings;
try {
- validateMusicMetadataUrls(updated);
+ updated = sanitizeMusicMetadataUrls(candidate);
} catch (e) {
return res.status(400).json({
success: false,
message: e instanceof Error ? e.message : 'Invalid music metadata URL',
});
}Also applies to: 87-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/routes/settings/musicMetadata.ts` around lines 38 - 47,
validateMusicMetadataUrls currently calls sanitizeProviderBaseUrl but throws
away the returned canonicalized strings; change it to capture and assign the
sanitized results back into settings.listenbrainz.apiBaseUrl and
settings.listenbrainz.webBaseUrl so the normalized values are the ones
persisted. Specifically, replace the two calls inside validateMusicMetadataUrls
to set settings.listenbrainz.apiBaseUrl = sanitizeProviderBaseUrl(... ) and
settings.listenbrainz.webBaseUrl = sanitizeProviderBaseUrl(...), ensuring the
saved configuration uses these canonicalized values.
| const { data: musicData, mutate: mutateMusic } = | ||
| useSWR<MusicMetadataSettings>('/api/v1/settings/music-metadata'); | ||
|
|
There was a problem hiding this comment.
Handle music settings fetch errors instead of showing an endless spinner.
Line 119 only reads musicData, and Line 521 shows <LoadingSpinner /> whenever it’s missing. If /api/v1/settings/music-metadata fails, this section can stay in a perpetual loading state with no user-visible error.
Suggested fix
- const { data: musicData, mutate: mutateMusic } =
+ const { data: musicData, error: musicError, mutate: mutateMusic } =
useSWR<MusicMetadataSettings>('/api/v1/settings/music-metadata');
@@
- {!musicInitialValues && <LoadingSpinner />}
- {musicInitialValues && (
+ {!musicError && !musicInitialValues && <LoadingSpinner />}
+ {musicError && (
+ <p className="text-sm text-red-400">
+ {intl.formatMessage(messages.connectionTestFailed)}
+ </p>
+ )}
+ {!musicError && musicInitialValues && (
<FormikAlso applies to: 521-523
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Settings/SettingsMetadata.tsx` around lines 119 - 121, The
component is only reading musicData from useSWR and shows <LoadingSpinner />
while missing, causing an endless spinner on fetch failure; update the useSWR
call in SettingsMetadata.tsx to also destructure the error (e.g., const { data:
musicData, error: musicError, mutate: mutateMusic } = useSWR(...)) and then
change the render logic around where <LoadingSpinner /> is returned (symbols:
musicData, musicError, mutateMusic, LoadingSpinner) to show a user-visible error
state (message + retry action that calls mutateMusic) when musicError exists
instead of the spinner; keep showing LoadingSpinner only when no error and no
data.
| days: days.toString(), | ||
| sort, | ||
| offset: offset.toString(), | ||
| count: count.toString(), |
There was a problem hiding this comment.
It looks like getFreshReleases() exposes parameters that the upstream endpoint does not support. I tested the API directly: count=1&offset=999999 still returned the full result set, while sort=confidence returned a 400 because that sort value is not valid for this endpoint. Could we restrict these options to the documented parameters and handle any result limiting locally if needed?
| const trimmed = (apiBaseUrl || 'https://api.listenbrainz.org').replace( | ||
| /\/+$/, | ||
| '' | ||
| ); |
| * the public site when no value is configured). | ||
| */ | ||
| export const resolveListenBrainzWebUrl = (webBaseUrl: string): string => { | ||
| return (webBaseUrl || 'https://listenbrainz.org').replace(/\/+$/, ''); |
| `expected a request against the candidate ListenBrainz baseURL, got ${JSON.stringify(baseURLs)}` | ||
| ); | ||
| assert.ok( | ||
| !baseURLs.includes('https://saved-api.example/1'), |
|
|
||
| assert.equal(res.status, 200); | ||
| const baseURLs = getCalls.map((c) => c.baseURL); | ||
| assert.ok(baseURLs.includes('https://saved-api.example/1')); |
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Adds first-class support for music metadata providers: - New MusicBrainz and ListenBrainz API clients with caching and per-provider URL sanitization (http(s) only, hostname required, no embedded credentials). - Persisted musicMetadata settings (baseUrl, authToken, maxRPS for MusicBrainz; apiBaseUrl, webBaseUrl, userToken for ListenBrainz). userAgent is stripped from GET/PUT responses. - New /settings/music-metadata GET/PUT routes and POST /test endpoint that merges candidate body with persisted settings + defaults before testing connectivity. - Settings UI with combined connectivity test and shared toastProviderFailures helper. maxRPS uses codebase numeric input idiom (text + inputMode + .short class) and is coerced to an integer before submit/test. - Unit tests for both clients (including override-settings behavior verified against axios baseURL) and route tests for sanitization, candidate-config merging, and per-provider failure reporting. - OpenAPI spec, end-user docs (Metadata Providers), and i18n strings.
019bfc2 to
d243845
Compare
Description
Adds configurable ListenBrainz metadata provider clients along with the admin settings routes, UI and OpenAPI documentation so their API surface, rate limiting and configuration can be validated before any music features consume them.
These are explicitly chosen, because they needed significant work I terms of GUI exposure and featureset, compare to the original "music support" PR. combined with the fact that these metadata providers are fully stand-alone capable, so dont rely on extensive database migrations and intregrations.
Yet Musicbrains builds the core of any future music feature
Added:
ListenBrainzMetadata ProviderDependencies:
dompurify,jsdomand@types/jsdomfor HTML sanitization of the Wikipedia extract from MusicBrainsAI used:
(design of expanded feature-set and GUI structure done manually and iteratively)
How Has This Been Tested?
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
Release Notes
New Features
Documentation