-
-
Notifications
You must be signed in to change notification settings - Fork 552
refactor(jellyfin-scanner): extend BaseScanner for jellyfin scanner #2226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(jellyfin-scanner): extend BaseScanner for jellyfin scanner #2226
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the Jellyfin scanner to extend the BaseScanner class, aligning its architecture with the Plex scanner implementation and reducing code duplication between the two scanner implementations.
Key changes:
- Refactored
JellyfinScannerto extendBaseScannerand utilize its shared media processing logic - Extracted movie ID resolution logic into a separate
extractMovieIdsmethod - Added Jellyfin-specific media ID tracking to
BaseScannerfor proper Jellyfin media server integration
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
server/lib/scanners/jellyfin/index.ts |
Refactored to extend BaseScanner, moved duplicate logic to base class, renamed methods for clarity, and delegated media processing to base scanner methods |
server/lib/scanners/baseScanner.ts |
Added support for Jellyfin media IDs in both movie and show processing methods, made tvdbId optional for broader compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactors JellyfinScanner to extend BaseScanner class to align the jellyfin scanner architecture with the plex scanner and reduce code duplication.
c7c86fe to
f3786ce
Compare
…rom original behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…pisodes (regression)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you take #2092 into account? Or should we add the same logic in another PR after?
gauthier-th
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Not tested though.
M0NsTeRRR
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as Gauthier :)
|
I'm merging this. So far from my testings, it seems to work as intended |
Description
This PR refactors the Jellyfin scanner to extend the
BaseScannerclass, aligning its architecture with the Plex scanner implementation.This refactor should reduce the code duplication between Plex and Jellyfin scanner implementations and make our codebase more maintainable.
This should also make it easier to implement #2066 as then it would not require any changes to that PR and can be merged after this.
How Has This Been Tested?
Ran full Jellyfin library scan with mixed movie/TV show library and verified all media items were correctly identified and processed.
Ran a Recently Added Scan and tested recently added items and verified incremental updates worked correctly
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extract