feat(discover): implement multi-region discovery and filtering#2260
feat(discover): implement multi-region discovery and filtering#2260Valtora wants to merge 3 commits intoseerr-team:developfrom
Conversation
Valtora
force-pushed
the
feature-multi-region-discovery
branch
from
fabb4b5 to
d1a7105
Compare
|
Please review and let me know if I need to do anything else, thank you for taking the time to do so. |
Seerr is on a feature freeze until next release. New features will be reviewed after release. |
|
@fallenbagel Understood, thank you. I'll leave the PR here for now and come back after release. Good luck to you and team in the meantime, thank you for putting in the work to bring us seerr. |
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
github-actions
bot
removed
the
merge conflict
📝 WalkthroughWalkthroughAdds multi-region support: RegionSelector changed to multi-select and emits pipe-delimited regions, TMDB API updated to request and aggregate multi-region discovery/trending results using with_origin_country + post-fetch Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as RegionSelector UI
participant State as Component State
participant Form as Settings/Form
participant API as TMDB API Layer
participant TMDB as TMDB Service
participant Filter as filterResults
User->>UI: Select multiple regions
UI->>State: Update selectedRegionsList
UI->>Form: onChange(pipe-delimited regions)
Form->>API: Request discover/trending (isMultiRegion)
API->>TMDB: Parallel fetches using with_origin_country (per region)
TMDB-->>API: Return results per region
API->>Filter: Merge, dedupe (uniqBy), filterResults by origin_language/country
Filter-->>API: Return combined paginated results
API-->>Form: Return filtered data
Form->>UI: Render filtered content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/RegionSelector/index.tsx (1)
160-234:⚠️ Potential issue | 🔴 CriticalRemove stale
selectedRegionrendering block.Line 225-233 still references
selectedRegion, which no longer exists after the refactor. This will not compile and also duplicates label rendering.🧹 Proposed fix
- {selectedRegion && selectedRegion.iso_3166_1 !== 'all' - ? regionName(selectedRegion.iso_3166_1) - : isUserSetting && selectedRegion?.iso_3166_1 !== 'all' - ? intl.formatMessage(messages.regionServerDefault, { - region: regionValue - ? regionName(regionValue) - : intl.formatMessage(messages.regionDefault), - }) - : intl.formatMessage(messages.regionDefault)}server/api/themoviedb/index.ts (1)
724-729:⚠️ Potential issue | 🟠 MajorGuard
regionwhen multi-region is selected.
/movie/upcomingexpects an ISO 3166-1 country code and will not accept pipe-delimited values./trending/alldoes not support theregionparameter and ignores it. Apply the same multi-region guard already used in/discover/movieand/discover/tvto maintain consistency and ensure proper behavior:💡 Suggested guard for multi-region
public getUpcomingMovies = async ({ page = 1, language = this.locale, }: { page: number; language: string; }): Promise<TmdbUpcomingMoviesResponse> => { try { + const isMultiRegion = this.discoverRegion?.includes('|'); const data = await this.get<TmdbUpcomingMoviesResponse>( '/movie/upcoming', { params: { page, language, - region: this.discoverRegion, + region: isMultiRegion ? undefined : this.discoverRegion, originalLanguage: this.originalLanguage, }, } );public getAllTrending = async ({ page = 1, timeWindow = 'day', language = this.locale, }: { page?: number; timeWindow?: 'day' | 'week'; language?: string; } = {}): Promise<TmdbSearchMultiResponse> => { try { + const isMultiRegion = this.discoverRegion?.includes('|'); const data = await this.get<TmdbSearchMultiResponse>( `/trending/all/${timeWindow}`, { params: { page, language, - region: this.discoverRegion, + region: isMultiRegion ? undefined : this.discoverRegion, }, } );Also applies to: 754-758
🤖 Fix all issues with AI agents
In `@src/components/Settings/SettingsMain/index.tsx`:
- Around line 171-176: The initialValues object contains duplicate and
misspelled keys: remove the duplicate streamingRegion entry and keep a single
assignment using nullish coalescing (streamingRegion: data?.streamingRegion ??
'US') so empty-string values are preserved, and standardize the tag keys to
match the form (replace blacklistedTags and blacklistedTagsLimit with
blocklistedTags and blocklistedTagsLimit), using nullish coalescing for limits
as well (e.g., blocklistedTagsLimit: data?.blocklistedTagsLimit ?? 50); update
the initialValues in the SettingsMain component accordingly.
| streamingRegion: data?.streamingRegion ?? 'US', | ||
| blacklistedTags: data?.blacklistedTags, | ||
| blacklistedTagsLimit: data?.blacklistedTagsLimit || 50, | ||
| streamingRegion: data?.streamingRegion || 'US', | ||
| blocklistedTags: data?.blocklistedTags, | ||
| blocklistedTagsLimit: data?.blocklistedTagsLimit || 50, |
There was a problem hiding this comment.
Duplicate initialValues keys override the intended nullish default.
Line 171-176 defines streamingRegion twice; the later || overwrites the earlier ??, so empty-string selections still collapse to 'US'. Also, blacklistedTags* appears to be a typo and diverges from the blocklistedTags* fields actually used by the form. Please keep a single, correct set of keys.
🛠️ Proposed fix
- streamingRegion: data?.streamingRegion ?? 'US',
- blacklistedTags: data?.blacklistedTags,
- blacklistedTagsLimit: data?.blacklistedTagsLimit || 50,
- streamingRegion: data?.streamingRegion || 'US',
- blocklistedTags: data?.blocklistedTags,
- blocklistedTagsLimit: data?.blocklistedTagsLimit || 50,
+ streamingRegion: data?.streamingRegion ?? 'US',
+ blocklistedTags: data?.blocklistedTags,
+ blocklistedTagsLimit: data?.blocklistedTagsLimit || 50,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| streamingRegion: data?.streamingRegion ?? 'US', | |
| blacklistedTags: data?.blacklistedTags, | |
| blacklistedTagsLimit: data?.blacklistedTagsLimit || 50, | |
| streamingRegion: data?.streamingRegion || 'US', | |
| blocklistedTags: data?.blocklistedTags, | |
| blocklistedTagsLimit: data?.blocklistedTagsLimit || 50, | |
| streamingRegion: data?.streamingRegion ?? 'US', | |
| blocklistedTags: data?.blocklistedTags, | |
| blocklistedTagsLimit: data?.blocklistedTagsLimit || 50, |
🧰 Tools
🪛 Biome (2.3.14)
[error] 171-171: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
🪛 GitHub Check: CodeQL
[failure] 171-171: Overwritten property
This property is overwritten by another property in the same object literal.
🤖 Prompt for AI Agents
In `@src/components/Settings/SettingsMain/index.tsx` around lines 171 - 176, The
initialValues object contains duplicate and misspelled keys: remove the
duplicate streamingRegion entry and keep a single assignment using nullish
coalescing (streamingRegion: data?.streamingRegion ?? 'US') so empty-string
values are preserved, and standardize the tag keys to match the form (replace
blacklistedTags and blacklistedTagsLimit with blocklistedTags and
blocklistedTagsLimit), using nullish coalescing for limits as well (e.g.,
blocklistedTagsLimit: data?.blocklistedTagsLimit ?? 50); update the
initialValues in the SettingsMain component accordingly.
|
Did you rebase ? There are still backlist leftovers. We renamed blacklist to blocklist as part of seerr release. |
|
Hi, I'm currently in the process of bringing everything up to date so please review again in a day or two. |
…selection for discovery
Valtora
force-pushed
the
feature-multi-region-discovery
branch
from
1142db1 to
5e01596
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/RegionSelector/index.tsx (1)
1-12:⚠️ Potential issue | 🟡 MinorPrettier check failed for this file.
Pipeline reports a formatting failure; please run Prettier on this file before merge.
server/api/themoviedb/index.ts (1)
721-734:⚠️ Potential issue | 🟠 MajorOmit the
regionparameter whendiscoverRegioncontains multiple regions.Lines 727 and 757 pass
region: this.discoverRegiondirectly to TMDB endpoints. TMDB'sregionparameter only accepts a single ISO 3166-1 country code—not multi-region strings likeUS|GB. WhendiscoverRegioncontains|, passing it violates the API contract and may cause errors or empty results.Other methods in this file (e.g.,
discoverat line 572,searchMultiat line 661) already handle this correctly: they checkdiscoverRegion?.includes('|')and omit theregionparameter when multi-region is detected, relying onfilterResultsfor post-filtering instead.Apply the same pattern to
getUpcomingMoviesandgetAllTrending:Suggested changes
public getUpcomingMovies = async ({ page = 1, language = this.locale, }: { page: number; language: string; }): Promise<TmdbUpcomingMoviesResponse> => { try { + const isMultiRegion = this.discoverRegion?.includes('|'); const data = await this.get<TmdbUpcomingMoviesResponse>( '/movie/upcoming', { params: { page, language, - region: this.discoverRegion, + region: isMultiRegion ? undefined : this.discoverRegion, originalLanguage: this.originalLanguage, }, } );public getAllTrending = async ({ page = 1, timeWindow = 'day', language = this.locale, }: { page?: number; timeWindow?: 'day' | 'week'; language?: string; } = {}): Promise<TmdbSearchMultiResponse> => { try { + const isMultiRegion = this.discoverRegion?.includes('|'); const data = await this.get<TmdbSearchMultiResponse>( `/trending/all/${timeWindow}`, { params: { page, language, - region: this.discoverRegion, + region: isMultiRegion ? undefined : this.discoverRegion, }, } );
🤖 Fix all issues with AI agents
In `@server/api/themoviedb/index.ts`:
- Around line 572-583: The code currently passes pipe-delimited
this.discoverRegion into the TMDB with_origin_country param which only accepts a
single ISO country; update the logic where
get<TmdbSearchMovieResponse>('/discover/movie', { params: { ..., region:
isMultiRegion ? undefined : this.discoverRegion, with_origin_country:
isMultiRegion ? this.discoverRegion : undefined } }) is built: detect if
this.discoverRegion.includes('|') and, for multi-region, split on '|' and
perform one get(...) request per country with with_origin_country set to a
single code, then merge and dedupe results (e.g., by movie id) before returning;
otherwise keep the single request path; ensure pagination/total handling is
reconciled when merging or fall back to using region or post-filtering if
merging pages is unacceptable.
- Fixes 'with_origin_country' pipe-delimiter limitation by fetching regions in parallel. - Fixes formatting in RegionSelector.
Valtora
force-pushed
the
feature-multi-region-discovery
branch
from
b66cc3a to
cc00b36
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api/themoviedb/index.ts (1)
1-5:⚠️ Potential issue | 🟡 MinorResolve the Prettier warning before merge.
CI reports Prettier issues; please re-run the formatter to keep this file consistent.
🧹 Nitpick comments (1)
server/api/themoviedb/index.ts (1)
572-667: Consider bounding multi-region fan‑out to protect TMDB rate limits.
Promise.allscales linearly with selected regions; large selections can spike concurrency and fail the whole request. Consider capping region count or batching/limiting concurrency.Also applies to: 712-804
|
This needs to be implemented properly as its slightly more complicated in scope than I initially realised. The API rate limit needs to be handled carefully and the changes are not only to the frontend but also the backend. I may revisit manually at a later time if there is enough demand. |
3 participants
feat(discover): implement multi-region discovery and filtering
with_origin_countrywhen multiple regions are selectedfilterResultshelper to manually filter Trending/Upcoming results by both Origin Country and Original LanguageDescription
This PR implements Multi-Region Discovery and Filtering, allowing users to select multiple countries in the "Discover Region" setting. This ensures that content from all selected regions is visible in Discover, Trending, and Popular feeds.
AI Disclosure
AI was used (Gemini 3 Pro, High, via Antigravity) for code generation and refactoring with heavy human supervision throughout and manual verification of all logic.
Q: Why do I think this was okay in this context?
A: The changes made are relatively simple to the frontend and API logic without interfering with other systems.
Key Changes
Frontend
RegionSelector:US|GB).Backend
TheMovieDb:getDiscoverMoviesandgetDiscoverTvto use thewith_origin_countryparameter when multiple regions are selected.filterResultshelper for endpoints that don't support simultaneous region/language filtering (like Trending).originalLanguagesetting (e.g., hiding Chinese content when English is selected).Fixes:
How Has This Been Tested?
I have tested this manually in a local development environment and verified with static analysis tools.
Manual Verification
US|GB).Automated Checks
pnpm typecheck(Client & Server)pnpm lintScreenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
New Features
Bug Fixes
Documentation