feat(media-library): added a new Discover page Slider for My Media Library#2600
Open
ryanmfransen wants to merge 2 commits intoseerr-team:developfrom
Open
feat(media-library): added a new Discover page Slider for My Media Library#2600ryanmfransen wants to merge 2 commits intoseerr-team:developfrom
ryanmfransen wants to merge 2 commits intoseerr-team:developfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a "My Media Library" slider and a full library page with infinite scrolling, registers a new Discover slider type, standardizes discover route paging defaults, adds a Cypress navigation test, and inserts an i18n key. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DiscoverPage as "Discover Page"
participant Slider as "MyMediaLibrarySlider"
participant API as "Server API"
participant LibraryPage as "MyMediaLibrary Page"
User->>DiscoverPage: Visit /discover
DiscoverPage->>Slider: Render slider component
Slider->>API: GET /api/v1/media?filter=allavailable&take=20
API-->>Slider: Return media results (pageInfo)
Slider->>DiscoverPage: Display slider items / Show More link
User->>Slider: Click slider header or Show More
Slider->>LibraryPage: Navigate to /discover/library
LibraryPage->>API: Fetch page=1 & page=2 (useSWRInfinite)
API-->>LibraryPage: Return paginated media
LibraryPage->>User: Render scrollable items
User->>LibraryPage: Scroll near bottom
LibraryPage->>API: Fetch next page
API-->>LibraryPage: Return more items
LibraryPage->>User: Append items (placeholders while loading)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 👌 |
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
2 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