feat(discover): add a new Discover page Slider for Available Media#2600
Open
ryanmfransen wants to merge 2 commits intoseerr-team:developfrom
Open
feat(discover): add a new Discover page Slider for Available Media#2600ryanmfransen wants to merge 2 commits intoseerr-team:developfrom
ryanmfransen wants to merge 2 commits intoseerr-team:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces an "Available Media" feature to the discover page, enabling users to view media currently available on their server. The changes include a new enum member and slider configuration, new React components for displaying available media with infinite scrolling, a dedicated library page, frontend integration across discover components, translation strings, and E2E test coverage. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as Browser
participant API as API Server
participant DB as Media Database
User->>Browser: Click Available Media slider header
Browser->>API: GET /api/v1/media?filter=allavailable&take=20&sort=mediaAdded
API->>DB: Query available media
DB-->>API: Return paginated results
API-->>Browser: Return media items + pageInfo
Browser->>Browser: Navigate to /discover/library
Browser->>API: GET /api/v1/media?filter=allavailable&take=PAGE_SIZE&sort=mediaAdded&skip=0
API->>DB: Query page 1
DB-->>API: Return results
API-->>Browser: Return first page
Browser->>Browser: Render available media list
User->>Browser: Scroll down to end of list
Browser->>API: GET /api/v1/media?filter=allavailable&...&skip=PAGE_SIZE
API->>DB: Query next page
DB-->>API: Return results
API-->>Browser: Return next page
Browser->>Browser: Append media items
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cypress/e2e/discover.cy.ts (1)
215-224: Add API interception for test consistency and stability.The
MyMediaLibrarySlidercomponent fetches data from/api/v1/media?filter=allavailable&take=20&sort=mediaAddedusinguseSWRon component mount. This test should intercept that API call before visiting the page, consistent with other tests in this file (trending, popular movies, upcoming movies, etc.). Relying solely on the 15s timeout can lead to flakiness.♻️ Suggested improvement
it('navigates from My Media Library slider to the library page', () => { + cy.intercept('/api/v1/media?filter=allavailable*').as('getLibraryMedia'); cy.visit('/'); + cy.wait('@getLibraryMedia'); cy.contains('.slider-header .slider-title', 'My Media Library', { timeout: 15000, }).click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/discover.cy.ts` around lines 215 - 224, This test "navigates from My Media Library slider to the library page" is missing an API interception for the MyMediaLibrarySlider data fetch; before calling cy.visit('/'), intercept the GET to '/api/v1/media?filter=allavailable&take=20&sort=mediaAdded' (or the equivalent query) and stub a deterministic response (use the same fixture pattern as other tests) so useSWR returns immediately; update the test to add cy.intercept(...) for that endpoint prior to cy.visit('/') and then proceed with the existing assertions (cy.contains(...).click(), cy.url().should(...), cy.get(...).should(...)).src/components/Discover/MyMediaLibrary/index.tsx (1)
45-50: Consider memoizing the scroll callback.The inline arrow function
() => setSize(size + 1)creates a new function reference on each render. Depending on howuseVerticalScrollhandles its callback dependency, this could potentially cause unnecessary effect re-runs. Wrapping it inuseCallbackwould prevent this.♻️ Proposed refactor
+import { useCallback } from 'react'; + const MyMediaLibrary = () => { const intl = useIntl(); const { data, error, size, setSize, isValidating } = useSWRInfinite<MediaResultsResponse>( // ... key function ); const lastPage = data?.[data.length - 1]; + const loadMore = useCallback(() => { + setSize((prevSize) => prevSize + 1); + }, [setSize]); + useVerticalScroll( - () => setSize(size + 1), + loadMore, !isValidating && !!data && (lastPage?.pageInfo.page ?? 0) < (lastPage?.pageInfo.pages ?? 0) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Discover/MyMediaLibrary/index.tsx` around lines 45 - 50, Memoize the scroll callback passed to useVerticalScroll to avoid recreating the function each render: wrap the inline arrow () => setSize(size + 1) in React.useCallback and pass that memoized callback to useVerticalScroll, with a dependency array that includes size (and setSize if not stable) so the callback updates correctly when size changes; keep the existing second argument (the boolean conditional using isValidating, data, lastPage) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cypress/e2e/discover.cy.ts`:
- Around line 215-224: This test "navigates from My Media Library slider to the
library page" is missing an API interception for the MyMediaLibrarySlider data
fetch; before calling cy.visit('/'), intercept the GET to
'/api/v1/media?filter=allavailable&take=20&sort=mediaAdded' (or the equivalent
query) and stub a deterministic response (use the same fixture pattern as other
tests) so useSWR returns immediately; update the test to add cy.intercept(...)
for that endpoint prior to cy.visit('/') and then proceed with the existing
assertions (cy.contains(...).click(), cy.url().should(...),
cy.get(...).should(...)).
In `@src/components/Discover/MyMediaLibrary/index.tsx`:
- Around line 45-50: Memoize the scroll callback passed to useVerticalScroll to
avoid recreating the function each render: wrap the inline arrow () =>
setSize(size + 1) in React.useCallback and pass that memoized callback to
useVerticalScroll, with a dependency array that includes size (and setSize if
not stable) so the callback updates correctly when size changes; keep the
existing second argument (the boolean conditional using isValidating, data,
lastPage) unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cypress/e2e/discover.cy.tsserver/constants/discover.tsserver/routes/discover.tssrc/components/Discover/DiscoverSliderEdit/index.tsxsrc/components/Discover/MyMediaLibrary/index.tsxsrc/components/Discover/MyMediaLibrarySlider/index.tsxsrc/components/Discover/constants.tssrc/components/Discover/index.tsxsrc/i18n/locale/en.jsonsrc/pages/discover/library.tsx
…ary (seerr-team#658) Created a new Slider on the Discover page with links to the full local library view.
ryanmfransen
force-pushed
the
feature-media-library-filter
branch
from
3da1ee9 to
b16fcee
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/Discover/MyMediaLibrary/index.tsx (1)
56-62: Consider more specific error handling.The generic
statusCode={500}error may not accurately represent all failure modes (e.g., network errors, 404s, rate limiting). If the API provides error details, consider passing them through for more informative error pages.However, this is consistent with common patterns in similar components, so acceptable as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Discover/MyMediaLibrary/index.tsx` around lines 56 - 62, The current error branch in MyMediaLibrary returns a generic <Error statusCode={500} />; update error handling to surface API-provided details by passing the actual status and/or message from the fetched error object (e.g., use error.status or error.response?.status and error.message) into the Error component so pages reflect specific failure modes (network/404/rate-limit) while keeping the same fallback when details are missing; locate this change around the conditional using the error and data locals in the MyMediaLibrary component and adjust the props passed to Error accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/Discover/MyMediaLibrary/index.tsx`:
- Around line 56-62: The current error branch in MyMediaLibrary returns a
generic <Error statusCode={500} />; update error handling to surface
API-provided details by passing the actual status and/or message from the
fetched error object (e.g., use error.status or error.response?.status and
error.message) into the Error component so pages reflect specific failure modes
(network/404/rate-limit) while keeping the same fallback when details are
missing; locate this change around the conditional using the error and data
locals in the MyMediaLibrary component and adjust the props passed to Error
accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cypress/e2e/discover.cy.tsserver/constants/discover.tsserver/routes/discover.tssrc/components/Discover/DiscoverSliderEdit/index.tsxsrc/components/Discover/MyMediaLibrary/index.tsxsrc/components/Discover/MyMediaLibrarySlider/index.tsxsrc/components/Discover/constants.tssrc/components/Discover/index.tsxsrc/i18n/locale/en.jsonsrc/pages/discover/library.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- server/constants/discover.ts
- src/components/Discover/index.tsx
- cypress/e2e/discover.cy.ts
- src/components/Discover/DiscoverSliderEdit/index.tsx
- src/components/Discover/constants.ts
- src/i18n/locale/en.json
- server/routes/discover.ts
|
This looks good, looking forward to seeing it merged 👌 |
ryanmfransen
force-pushed
the
feature-media-library-filter
branch
from
1eedaa8 to
dd8b52b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Discover/MyMediaLibrarySlider/index.tsx (1)
13-16: Consider sharing the media-library query config.This endpoint shape is now duplicated here and in
src/components/Discover/MyMediaLibrary/index.tsx. Pulling the shared filter/sort/page-size bits into a small helper or constant would make it harder for the slider and full page to drift later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Discover/MyMediaLibrarySlider/index.tsx` around lines 13 - 16, The media query string '/api/v1/media?filter=allavailable&take=20&sort=mediaAdded' is duplicated between MyMediaLibrarySlider and MyMediaLibrary; extract it into a shared constant or small helper (e.g., export const MEDIA_LIBRARY_QUERY or a buildMediaQuery(filter, take, sort) function) and import/use that in both components instead of inlining the string where useSWR is called, keeping the existing useSWR invocation intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Discover/MyMediaLibrary/index.tsx`:
- Line 9: The import statement currently reads "import Error from
'@app/pages/_error';" which shadows the global Error constructor; update the
import to use a local alias (for example, rename the imported symbol to
ErrorPage or AppError) wherever it's referenced in this module (the import line
in src/components/Discover/MyMediaLibrary/index.tsx and any usage of Error), and
update those usages to the new name so the global Error constructor is no longer
shadowed.
---
Nitpick comments:
In `@src/components/Discover/MyMediaLibrarySlider/index.tsx`:
- Around line 13-16: The media query string
'/api/v1/media?filter=allavailable&take=20&sort=mediaAdded' is duplicated
between MyMediaLibrarySlider and MyMediaLibrary; extract it into a shared
constant or small helper (e.g., export const MEDIA_LIBRARY_QUERY or a
buildMediaQuery(filter, take, sort) function) and import/use that in both
components instead of inlining the string where useSWR is called, keeping the
existing useSWR invocation intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91a76af8-9165-4bd3-aeb0-ad293ab89bb6
📒 Files selected for processing (10)
cypress/e2e/discover.cy.tsserver/constants/discover.tsserver/routes/discover.tssrc/components/Discover/DiscoverSliderEdit/index.tsxsrc/components/Discover/MyMediaLibrary/index.tsxsrc/components/Discover/MyMediaLibrarySlider/index.tsxsrc/components/Discover/constants.tssrc/components/Discover/index.tsxsrc/i18n/locale/en.jsonsrc/pages/discover/library.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- cypress/e2e/discover.cy.ts
- src/i18n/locale/en.json
- server/routes/discover.ts
- src/components/Discover/DiscoverSliderEdit/index.tsx
- src/pages/discover/library.tsx
- src/components/Discover/constants.ts
| import TmdbTitleCard from '@app/components/TitleCard/TmdbTitleCard'; | ||
| import useVerticalScroll from '@app/hooks/useVerticalScroll'; | ||
| import globalMessages from '@app/i18n/globalMessages'; | ||
| import Error from '@app/pages/_error'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/Discover/MyMediaLibrary/index.tsx | head -70Repository: seerr-team/seerr
Length of output: 2624
🏁 Script executed:
rg -n "Error" src/components/Discover/MyMediaLibrary/index.tsxRepository: seerr-team/seerr
Length of output: 142
Rename the _error import to avoid shadowing Error.
import Error ... shadows the global Error constructor and matches the Biome warning on this file. A local alias keeps the lint clean and avoids future confusion.
Suggested fix
-import Error from '@app/pages/_error';
+import ErrorPage from '@app/pages/_error';
@@
- return <Error statusCode={500} />;
+ return <ErrorPage statusCode={500} />;🧰 Tools
🪛 Biome (2.4.6)
[error] 9-9: Do not shadow the global "Error" property.
(lint/suspicious/noShadowRestrictedNames)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Discover/MyMediaLibrary/index.tsx` at line 9, The import
statement currently reads "import Error from '@app/pages/_error';" which shadows
the global Error constructor; update the import to use a local alias (for
example, rename the imported symbol to ErrorPage or AppError) wherever it's
referenced in this module (the import line in
src/components/Discover/MyMediaLibrary/index.tsx and any usage of Error), and
update those usages to the new name so the global Error constructor is no longer
shadowed.
gauthier-th
changed the title
Member
|
I'm not sure about the "My Media Library" naming. What do you think of something like "Available Content" instead? |
Collaborator
Available Media? |
ryanmfransen
force-pushed
the
feature-media-library-filter
branch
from
dd8b52b to
681f617
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/Discover/AvailableMedia/index.tsx (1)
9-9:⚠️ Potential issue | 🟡 MinorRename the
Errorimport to avoid shadowing the globalErrorconstructor.This import shadows the global
Errorconstructor, which is flagged by Biome. Use an alias likeErrorPageto avoid confusion and keep the lint clean.Proposed fix
-import Error from '@app/pages/_error'; +import ErrorPage from '@app/pages/_error';And update the usage at line 57:
- return <Error statusCode={500} />; + return <ErrorPage statusCode={500} />;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Discover/AvailableMedia/index.tsx` at line 9, The imported default export "Error" from '@app/pages/_error' shadows the global Error constructor; rename the import to a non-conflicting alias (e.g., import ErrorPage from '@app/pages/_error') and update all usages of the "Error" symbol in this module (for example the component/render call that currently uses <Error ... />) to use "ErrorPage" instead so the global constructor is not shadowed and the Biome lint error is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/Discover/AvailableMedia/index.tsx`:
- Line 9: The imported default export "Error" from '@app/pages/_error' shadows
the global Error constructor; rename the import to a non-conflicting alias
(e.g., import ErrorPage from '@app/pages/_error') and update all usages of the
"Error" symbol in this module (for example the component/render call that
currently uses <Error ... />) to use "ErrorPage" instead so the global
constructor is not shadowed and the Biome lint error is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf80ee48-23d2-4370-b826-097f34049eb1
📒 Files selected for processing (10)
cypress/e2e/discover.cy.tsserver/constants/discover.tsserver/routes/discover.tssrc/components/Discover/AvailableMedia/index.tsxsrc/components/Discover/AvailableMediaSlider/index.tsxsrc/components/Discover/DiscoverSliderEdit/index.tsxsrc/components/Discover/constants.tssrc/components/Discover/index.tsxsrc/i18n/locale/en.jsonsrc/pages/discover/library.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/i18n/locale/en.json
- cypress/e2e/discover.cy.ts
- server/routes/discover.ts
- src/components/Discover/index.tsx
Author
|
Applied the suggested naming 'Available Media' Great idea! |
gauthier-th
changed the title
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
4 participants
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.
…brary (#658)
Created a new Slider on the Discover page with links to the full local library view.
Description
I created a new 'My Media Library' Slider at the bottom of the Discover page. This lists 20 of your local media in the standard slider form, and if more than 20 media is found in your library, a 'See More' card is display.
You can also go to the full page My Media Library using the links beside the heading of the Slider.
My Media Library is all your local media, movies and shows with status|status4k of available, and partiallyavailable.
Allows users to quickly see the contents of their (Partially)|Available library.
How Has This Been Tested?
Tested on my local machine using my Jellyfin media library, containing a partially available series and available movies, totalling > 20.
Add Jellyfin library.
Load Discover page, view new Slider.
Click link to library in header.
Scroll slider all the way to the right, click 'See More'
Load '/discover/library' observer full page view of library, available and partiallyavailable.
Manual tests for local (containerized server) Jellyfin library.
Added cypress test for basic navigation of new components.
All testing done with build running locally on my Macbook using Chrome.
I tried to introduce a minimal footprint of these changes, avoiding as many touchpoints in existing codebase as possible.
I heavily relied on AI for the following:
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit