feat(discover): add external provider sliders#3066
Open
philipkraeutl wants to merge 5 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds end-to-end external provider discovery: OpenAPI schemas, DB entity + migration, settings CRUD + test endpoints, discover route, frontend settings modal/page, and Discover slider integration. ChangesExternal Provider Discovery System
Sequence DiagramsequenceDiagram
participant Admin as Admin User
participant Modal as Provider Modal
participant API as Settings API
participant DiscoverUI as Discover Slider UI
participant Pipeline as getExternalDiscoverItems / testExternalDiscoverProvider
participant TMDB as TMDB/TVDB API
Admin->>Modal: Build provider config and Test
Modal->>API: POST /settings/external-providers/providers/test
API->>Pipeline: testExternalDiscoverProvider(provider payload)
Pipeline->>API: { ok, status, totalParsed, sample }
API->>Modal: test response
Admin->>API: POST /settings/external-providers/providers (save)
Admin->>DiscoverUI: Add EXTERNAL_PROVIDER slider
DiscoverUI->>API: GET /settings/external-providers/providers (enabled)
DiscoverUI->>Admin: select provider -> trigger discover
DiscoverUI->>API: GET /discover/external/{providerId}?page=1&language=xx
API->>Pipeline: getExternalDiscoverItems(providerId, user, language, page)
Pipeline->>TMDB: Resolve TMDB/TVDB IDs
TMDB->>Pipeline: Movie/TV details
Pipeline->>API: ExternalDiscoverPayload
API->>DiscoverUI: MovieResult/TvResult objects (JSON)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
philipkraeutl
changed the title
philipkraeutl
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 6410-6412: The OpenAPI operation uses the tag "discover" but that
tag is not declared at the document top-level; open seerr-api.yml and add a
top-level tags entry for "discover" (include a brief description) so the
top-level tags array includes an object with name: "discover" to match the
operation's tag and resolve tooling/spec inconsistencies.
- Around line 3912-3917: The request bodies for the external-provider
create/test/update endpoints currently use a bare "type: object" which disables
validation; replace those anonymous schemas with concrete component schemas
(e.g., ExternalProviderCreate, ExternalProviderTest, ExternalProviderUpdate) and
reference them with $ref in the requestBody for the external-provider
create/test/update operations, specifying required properties (like
providerType, name, and provider-specific config fields), enum values for
providerType, and using oneOf/anyOf for provider-specific config shapes (or
separate nested schemas such as ExternalProviderConfigForX) so OpenAPI will
validate required fields and allowed enum values.
In `@server/lib/externalDiscover.ts`:
- Around line 687-704: The current hydration uses an unbounded Promise.all over
parsedItems which can burst external TMDB/TVDB requests; change the logic around
hydratedItems to process parsedItems with bounded concurrency (e.g., use a
concurrency-limited mapper like p-map or a small semaphore/worker pool) when
calling hydrateExternalItem(item, tmdb, language), preserving the existing
try/catch/logging behavior (logger.warn with label 'External Discover',
provider.id, item, getErrorMessage(e)) and still filtering out nulls into
HydratedExternalItem; pick a sensible concurrency constant (e.g., 5-10) and
apply it to the mapping so external calls are rate-friendly.
- Around line 650-657: The current cacheKey
(`external-discover:${provider.id}:${language ?? 'default'}`) is reused for
payloads that include user-specific computed data (the `relatedMedia` mapping),
which can leak one user's state to another; instead either include a user
identifier in the key (e.g., append `:user:${user.id}` when computing
`relatedMedia`) or stop caching user-specific fields by caching only the raw
provider response (e.g., `external-discover:${provider.id}:${language}:raw`) and
compute `relatedMedia` per-request using the `user` before returning; update
uses of `cache.get`, `cache.set`, and any code that builds
`ExternalDiscoverPayload`/`relatedMedia` accordingly (see symbols: cacheKey,
provider.cacheMinutes, cache.get<ExternalDiscoverPayload>,
ExternalDiscoverPayload, relatedMedia).
- Around line 627-631: The returned payload in getExternalDiscoverItems
currently hardcodes page: 1 and totalPages: 1 and doesn't accept a page
parameter; update the function signature to accept an optional page:number (or
remove page from the public contract) and use that value when calling the
provider API, then parse the provider response for its current page and
totalPages and propagate those into the ExternalDiscoverPayload instead of the
hardcoded values; also search the same file for any other response constructions
that hardcode page/totalPages (the similar block later in the file) and apply
the same fix so all discover endpoints either accept/forward page input or
remove page from the contract consistently.
In `@server/routes/discover.ts`:
- Line 992: The current validation "if (!providerId ||
Number.isNaN(providerId))" allows negative and non-positive values; update the
check around the providerId variable so it only accepts positive integers
(reject NaN, zero, negatives, and non-integers). Locate the validation where
providerId is parsed/used in the discover route and replace it with a guard that
asserts Number.isFinite(providerId) && Number.isInteger(providerId) &&
providerId > 0 (or equivalent), returning the existing error response when the
check fails so negative IDs never reach the service layer.
In `@server/routes/settings/externalProviders.ts`:
- Around line 93-103: The catch blocks in externalProviders route handlers
currently map every exception to a 404; change them to only return 404 for true
not-found errors and return 500 for unexpected failures: inside each catch (the
blocks using logger.error and next({...})), detect not-found cases by checking a
specific marker on the error (e.g., e?.status === 404 || e?.name ===
'NotFoundError' || a project-specific NotFoundError class) and only call
next({status: 404, message: 'External provider not found.'}) for those; for all
other errors call logger.error with the full error details and call
next({status: 500, message: 'Internal server error.'}); apply the same change to
the other two catch blocks mentioned (lines around 194-203 and 222-231) so all
handlers use the same 404-vs-500 distinction.
In `@src/components/Discover/CreateSlider/index.tsx`:
- Line 332: Replace the hardcoded placeholder "Select external provider" in
CreateSlider with a localized message using the component's existing react-intl
instance (e.g., intl.formatMessage or <FormattedMessage>); update the property
where dataPlaceholderText is set (and the identical instance at the other
occurrence) to call intl.formatMessage({ id:
'discover.createSlider.externalProviderPlaceholder', defaultMessage: 'Select
external provider' }) (or use the appropriate message id consistent with
existing keys) so the placeholder is localized.
In `@src/components/Discover/index.tsx`:
- Line 404: The URL is interpolating raw slider.data into the path
(url={`/api/v1/discover/external/${slider.data}`}) which can produce malformed
requests; update the code that builds this URL in the Discover component to
encode the provider id using encodeURIComponent (e.g.,
encodeURIComponent(String(slider.data))) before interpolation so the path
segment is safe—search for the url construction in the Discover component
(index.tsx) where url is set and replace the raw slider.data usage with the
encoded value.
In
`@src/components/Settings/SettingsExternalProviderModal/SettingsExternalProviderModal.tsx`:
- Around line 254-270: The submit handler in onSubmit currently calls
buildPayload and axios.put/post and always calls onSave, but it lacks error
handling; wrap the network calls in a try/catch so that onSave is only invoked
on success, and on failure catch the error, log it, and show an error toast to
the user (use the app's toast/toaster util and prefer the server message from
error.response?.data?.message or fallback to error.message) while keeping the
modal open; update the async onSubmit block around the axios.put/post and onSave
calls to implement this behavior.
In `@src/components/Settings/SettingsExternalProviders.tsx`:
- Around line 45-47: Replace all hardcoded user-facing strings in the
SettingsExternalProviders component with localized messages (use the component's
translation function, e.g., t from useTranslation) — specifically change the
setError call that currently contains "External providers could not be loaded.
Check whether /api/v1/settings/external-providers/providers is registered and
working." to a localized key (and remove the internal API path from the
user-facing text), and similarly replace the other hardcoded confirm/messages
around the same areas (the strings referenced at the other setError/confirm
locations) with t('...') keys and add the keys to the locale resource files;
ensure you update any import/useTranslation usage in SettingsExternalProviders
if missing and use descriptive i18n keys like
settings.externalProviders.loadError, settings.externalProviders.confirmDelete,
etc.
🪄 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: 0c3b43ff-5ecb-4145-a064-567bf58dc299
📒 Files selected for processing (16)
seerr-api.ymlserver/constants/discover.tsserver/entity/ExternalProvider.tsserver/lib/externalDiscover.tsserver/migration/sqlite/1779552594803-AddExternalProviders.tsserver/routes/discover.tsserver/routes/settings/externalProviders.tsserver/routes/settings/index.tssrc/components/Discover/CreateSlider/index.tsxsrc/components/Discover/DiscoverSliderEdit/index.tsxsrc/components/Discover/constants.tssrc/components/Discover/index.tsxsrc/components/Settings/SettingsExternalProviderModal/SettingsExternalProviderModal.tsxsrc/components/Settings/SettingsExternalProviders.tsxsrc/components/Settings/SettingsLayout.tsxsrc/pages/settings/external-providers.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/lib/externalDiscover.ts (1)
874-877:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest function doesn't substitute
$pageplaceholder.If the provider URL contains
$page(e.g.,http://api.example.com/items?page=$page), this test function will request the literal string$pageinstead of substituting a page number. This may cause HTTP 400/404 errors during provider setup testing, potentially confusing admins.Consider using
buildProviderRequestUrlto substitute page 1 for consistency with the discover endpoint:🔧 Proposed fix
export const testExternalDiscoverProvider = async (provider: { url: string; // ... other fields }) => { - const response = await axios.get(provider.url, { + const response = await axios.get(buildProviderRequestUrl(provider.url, 1), { headers: getRequestHeaders(provider), timeout: 15000, });🤖 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/lib/externalDiscover.ts` around lines 874 - 877, The test request currently uses provider.url directly (in the test function where axios.get is called with getRequestHeaders and timeout), so any $page placeholder is not substituted; change the request URL to use buildProviderRequestUrl(provider, 1) (or the existing helper that substitutes $page) instead of provider.url while keeping the same headers and timeout so the test requests page 1 consistently.
🧹 Nitpick comments (1)
server/routes/discover.ts (1)
999-1006: ⚡ Quick winPrefer centralized OpenAPI query validation for
page.This route now duplicates
pagequery validation in-handler. To stay consistent with the rest ofserver/routes, let the upstream OpenAPI middleware enforce query validity and keep the handler focused on orchestration.♻️ Minimal refactor
- const page = Number(req.query.page ?? 1); - - if (!Number.isInteger(page) || page <= 0) { - return next({ - status: 400, - message: 'Invalid page.', - }); - } + const page = req.query.page ? Number(req.query.page) : 1;Based on learnings: In
server/routes/**/*.ts, rely on upstream OpenAPI validator middleware for query parameters and avoid duplicating checks in route handlers.🤖 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/discover.ts` around lines 999 - 1006, Remove the in-handler page validation and let the OpenAPI middleware enforce query correctness: keep the numeric conversion (const page = Number(req.query.page ?? 1);) but delete the Number.isInteger/page <= 0 check and the early return that calls next({ status: 400, message: 'Invalid page.' }); so the handler only orchestrates using the validated page value; remove any references to that specific validation block (Number.isInteger, page <= 0, and the next(...) error object).
🤖 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 6621-6623: The response schema for results.items uses oneOf
between MovieResult and TvResult which is ambiguous because MovieResult declares
required: [id, mediaType, title] while TvResult lacks required constraints and
mediaType enum/discriminator, causing movie-shaped objects to validate against
both and break oneOf; fix by adding an explicit discriminator (e.g., add
"discriminator": { "propertyName": "mediaType", "mapping": { "movie":
"`#/components/schemas/MovieResult`", "tv": "`#/components/schemas/TvResult`" } } on
the parent or each schema) or alternatively enforce distinct required/enums on
the mediaType field in MovieResult and TvResult (e.g., set mediaType: { enum:
["movie"] } in MovieResult and mediaType: { enum: ["tv"] } in TvResult) so oneOf
can unambiguously distinguish between MovieResult and TvResult for
results.items.
In `@server/routes/settings/externalProviders.ts`:
- Around line 228-244: The code currently assigns credential fields to undefined
(provider.apiKey/provider.apiKeyHeader/provider.bearerToken) and uses patterns
like toNullableString(... ) ?? provider.<field>, which with TypeORM 0.3.x causes
repository.save() to skip undefined and preserves old secrets; change the
assignment logic to explicitly set null for clears and stop falling back to the
existing value: when handling incoming body values use provider.apiKey =
toNullableString(req.body.apiKey) (and similarly for apiKeyHeader and
bearerToken) so empty/blank inputs become null and are persisted, and when auth
type is ExternalProviderAuthType.NONE set provider.apiKey =
provider.apiKeyHeader = provider.bearerToken = null (not undefined); also remove
the `?? provider.<field>` fallback wherever toNullableString is used for mapping
so a null result clears the stored value.
---
Outside diff comments:
In `@server/lib/externalDiscover.ts`:
- Around line 874-877: The test request currently uses provider.url directly (in
the test function where axios.get is called with getRequestHeaders and timeout),
so any $page placeholder is not substituted; change the request URL to use
buildProviderRequestUrl(provider, 1) (or the existing helper that substitutes
$page) instead of provider.url while keeping the same headers and timeout so the
test requests page 1 consistently.
---
Nitpick comments:
In `@server/routes/discover.ts`:
- Around line 999-1006: Remove the in-handler page validation and let the
OpenAPI middleware enforce query correctness: keep the numeric conversion (const
page = Number(req.query.page ?? 1);) but delete the Number.isInteger/page <= 0
check and the early return that calls next({ status: 400, message: 'Invalid
page.' }); so the handler only orchestrates using the validated page value;
remove any references to that specific validation block (Number.isInteger, page
<= 0, and the next(...) error object).
🪄 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: 28adc654-a175-4cee-9d5c-c5e66e5c5164
📒 Files selected for processing (9)
seerr-api.ymlserver/lib/externalDiscover.tsserver/routes/discover.tsserver/routes/settings/externalProviders.tssrc/components/Discover/CreateSlider/index.tsxsrc/components/Discover/index.tsxsrc/components/Settings/SettingsExternalProviderModal/SettingsExternalProviderModal.tsxsrc/components/Settings/SettingsExternalProviders.tsxsrc/i18n/locale/en.json
✅ Files skipped from review due to trivial changes (2)
- src/i18n/locale/en.json
- src/components/Discover/CreateSlider/index.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/entity/ExternalProvider.ts (1)
80-81: ⚡ Quick winNarrow
defaultMediaTypeto supported literals.Use a constrained type instead of
string | nullso invalid values are caught at compile time and stay aligned with the API contract.Suggested change
- public defaultMediaType?: string | null; + public defaultMediaType?: + | ExternalProviderMediaType.MOVIE + | ExternalProviderMediaType.TV + | null;🤖 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/entity/ExternalProvider.ts` around lines 80 - 81, The field defaultMediaType on the ExternalProvider entity is currently typed as string | null; replace that with a constrained union of supported literal values (or an exported type/enum alias) plus null (e.g., type ExternalProviderMediaType = 'image' | 'video' | 'audio'; public defaultMediaType?: ExternalProviderMediaType | null) so invalid values are caught at compile time; keep the `@Column` decorator as varchar and nullable, and update any usages/assignments to use the new type or enum.
🤖 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 `@server/routes/settings/externalProviders.ts`:
- Around line 36-37: toNullableString currently accepts non-empty strings but
returns the original untrimmed value, which can preserve accidental whitespace;
change it to trim the input first (e.g., compute a trimmed version like s =
value.trim()), verify s !== '' and then return s (or null) so callers like
header/token/path handling receive trimmed strings; update the function named
toNullableString accordingly.
---
Nitpick comments:
In `@server/entity/ExternalProvider.ts`:
- Around line 80-81: The field defaultMediaType on the ExternalProvider entity
is currently typed as string | null; replace that with a constrained union of
supported literal values (or an exported type/enum alias) plus null (e.g., type
ExternalProviderMediaType = 'image' | 'video' | 'audio'; public
defaultMediaType?: ExternalProviderMediaType | null) so invalid values are
caught at compile time; keep the `@Column` decorator as varchar and nullable, and
update any usages/assignments to use the new type or enum.
🪄 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: b5411760-3fd2-44ca-ae11-20f985b233b0
📒 Files selected for processing (3)
seerr-api.ymlserver/entity/ExternalProvider.tsserver/routes/settings/externalProviders.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds support for configurable external discover providers that can be used as custom Discover sliders.
This allows admins to configure external HTTP providers that return TMDB or TVDB identifiers. Providers can be configured with optional authentication, cache duration, and advanced JSON mapping. External providers can then be selected when creating a custom Discover slider, with the selected provider ID stored as the slider data.
The implementation also supports automatic parsing of common JSON response formats, including responses with
tmdbId,tvdbId, or genericidfields. If media type information is missing, the external discover logic attempts to infer or resolve the media type where possible.Main changes:
No linked issue.
How Has This Been Tested?
Tested locally on a development setup.
Manual testing included:
tmdbIdmediaTypeidresultsarrays0for fresh requests/api/v1/discover/external/{providerId}returns hydrated media resultspnpm lint --fixScreenshots / Logs (if applicable)
Screenshots attached showing:
Checklist:
pnpm buildpnpm i18n:extractAI Assistance Notice
I used ChatGPT to help reason about the implementation approach, debug local development issues, and review code snippets during development. All generated code was manually reviewed, adjusted, and tested locally before submitting this pull request.
Summary by CodeRabbit