Add MusicBrainz integration for music management #8838
Conversation
WalkthroughThis change set introduces a comprehensive music management feature to the application, integrating MusicBrainz support, a new ORM schema for music data, web handlers, UI templates, configuration options, background schedulers, and supporting tests. It adds new endpoints, templates, JavaScript files, and database models to enable searching, adding, viewing, and managing music artists and albums, including metadata fetching and download management. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebUI
participant MusicHandler
participant MusicList
participant MusicBrainz
participant DB
User->>WebUI: Search/Add Artist
WebUI->>MusicHandler: /music/search or /music/add
MusicHandler->>MusicList: search_musicbrainz(query)
MusicList->>MusicBrainz: API search
MusicBrainz-->>MusicList: Artist data
MusicList-->>MusicHandler: Results
MusicHandler-->>WebUI: Render artist list/search results
User->>WebUI: View Artist Details
WebUI->>MusicHandler: /music/details
MusicHandler->>MusicList: artist_by_slug(slug)
MusicList->>DB: Query artist, albums
DB-->>MusicList: Artist, album data
MusicList-->>MusicHandler: Data
MusicHandler-->>WebUI: Render artist details
User->>WebUI: Snatch Album
WebUI->>MusicHandler: /music/snatch_album
MusicHandler->>MusicList: snatch_album(result)
MusicList->>DB: Update album status/history
MusicList-->>MusicHandler: Result
MusicHandler-->>WebUI: Show status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (4)
sickchill/gui/slick/views/music/remove.mako (1)
1-36: LGTM! Clean removal confirmation page.The template provides good user experience with:
- Clear success confirmation message
- Auto-redirect functionality for smooth workflow
- Manual navigation option as fallback
- Proper internationalization support
- Bootstrap styling consistency
The JavaScript redirect is simple and secure using
reverse_urlfor the destination.Consider making the 5-second redirect timeout configurable or using a slightly longer delay (7-10 seconds) to give users more time to read the confirmation message.
sickchill/gui/slick/js/musicRootDirs.js (1)
103-106: Improve robustness of ID parsing logic.The current implementation assumes the ID format is always 'rd-X' and uses a hardcoded slice position. This could break if the ID format changes.
- if ($('#whichDefaultMusicRootDir').val().length >= 4) { - directoryString = $('#whichDefaultMusicRootDir').val().slice(3); - } + const defaultId = $('#whichDefaultMusicRootDir').val(); + const match = defaultId.match(/^rd-(\d+)$/); + if (match) { + directoryString = match[1]; + }sickchill/oldbeard/databases/music.py (1)
103-103: Move import to module level.Importing modules inside functions/methods is generally discouraged unless necessary for avoiding circular imports.
Add
import shutilat the top of the file with other imports, then remove the import from line 103.sickchill/music.py (1)
600-608: Add error handling for track duration formatting.The duration formatting could fail if the duration value is not a valid integer.
if track_info["duration"]: try: ms = int(track_info["duration"]) seconds = ms // 1000 minutes = seconds // 60 seconds = seconds % 60 track_info["duration"] = f"{minutes}:{seconds:02d}" except (ValueError, TypeError): - pass + logger.debug(f"Invalid duration value: {track_info['duration']}") + track_info["duration"] = ""
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
SickChill.py(2 hunks)pyproject.toml(1 hunks)sickchill/gui/slick/js/music.js(1 hunks)sickchill/gui/slick/js/musicRootDirs.js(1 hunks)sickchill/gui/slick/views/config_general.mako(2 hunks)sickchill/gui/slick/views/inc_musicRootDirs.mako(1 hunks)sickchill/gui/slick/views/layouts/main.mako(2 hunks)sickchill/gui/slick/views/music/album_details.mako(1 hunks)sickchill/gui/slick/views/music/details.mako(1 hunks)sickchill/gui/slick/views/music/index.mako(1 hunks)sickchill/gui/slick/views/music/remove.mako(1 hunks)sickchill/gui/slick/views/music/search.mako(1 hunks)sickchill/music.py(1 hunks)sickchill/oldbeard/config.py(1 hunks)sickchill/oldbeard/databases/music.py(1 hunks)sickchill/providers/metadata/musicbrainz.py(1 hunks)sickchill/settings.py(3 hunks)sickchill/start.py(8 hunks)sickchill/views/__init__.py(1 hunks)sickchill/views/config/general.py(4 hunks)sickchill/views/music.py(1 hunks)sickchill/views/server_settings.py(2 hunks)tests/test_music.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: destotelhorus
PR: SickChill/sickchill#8740
File: sickchill/oldbeard/clients/__deluge_base.py:32-32
Timestamp: 2024-06-10T19:15:15.947Z
Learning: User destotelhorus agreed to the suggested change of combining `options.update` calls into a single call for better readability and efficiency in the `sickchill/oldbeard/clients/__deluge_base.py` file, but mentioned not having touched that part of the code themselves.
SickChill.py (1)
Learnt from: miigotu
PR: #8812
File: sickchill/helper/common.py:443-447
Timestamp: 2024-11-29T05:51:19.858Z
Learning: In the choose_data_dir function in sickchill/helper/common.py, avoid using try-except blocks to handle PermissionError or OSError when checking for path existence or access. Instead, check if the user has access to the path using os.access() to determine if the user has the necessary permissions, following the preference to avoid the EAFP pattern in modern Python.
sickchill/views/server_settings.py (1)
Learnt from: miigotu
PR: #8812
File: sickchill/helper/common.py:443-447
Timestamp: 2024-11-29T05:51:19.858Z
Learning: In the choose_data_dir function in sickchill/helper/common.py, avoid using try-except blocks to handle PermissionError or OSError when checking for path existence or access. Instead, check if the user has access to the path using os.access() to determine if the user has the necessary permissions, following the preference to avoid the EAFP pattern in modern Python.
sickchill/oldbeard/config.py (1)
Learnt from: destotelhorus
PR: #8740
File: sickchill/oldbeard/clients/__deluge_base.py:32-32
Timestamp: 2024-06-10T19:15:15.947Z
Learning: User destotelhorus agreed to the suggested change of combining options.update calls into a single call for better readability and efficiency in the sickchill/oldbeard/clients/__deluge_base.py file, but mentioned not having touched that part of the code themselves.
🪛 Ruff (0.12.2)
sickchill/views/__init__.py
25-25: sickchill.views.music.MusicHandler imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
sickchill/views/music.py
5-5: sickchill.oldbeard.config imported but unused
Remove unused import: sickchill.oldbeard.config
(F401)
tests/test_music.py
8-8: musicbrainzngs imported but unused
Remove unused import: musicbrainzngs
(F401)
sickchill/oldbeard/databases/music.py
9-9: sqlalchemy.ext.declarative.declarative_base imported but unused
Remove unused import: sqlalchemy.ext.declarative.declarative_base
(F401)
sickchill/providers/metadata/musicbrainz.py
12-12: sickchill.oldbeard.databases.music imported but unused
Remove unused import: sickchill.oldbeard.databases.music
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: GitGuardian Security Checks
🔇 Additional comments (36)
pyproject.toml (1)
300-300: Approve addition of musicbrainzngs dependencyVerified that
musicbrainzngs = ">=0.7.1"on pyproject.toml (line 300) is correct:
- 0.7.1 is the latest release as of July 2025.
- No known security vulnerabilities reported for version 0.7.1 or later.
- Version constraint
>=0.7.1balances flexibility and feature requirements.No further action required.
sickchill/settings.py (4)
13-13: LGTM!The import of
MusicListtype for type checking is correctly placed and follows the existing pattern withMovieList.
81-81: Clarify the purpose of BRANCH setting.The addition of
BRANCH = "master"appears unrelated to the music integration feature. Please clarify if this change is intentional and necessary for this PR.
588-588: LGTM!The
music_listvariable declaration follows the same pattern asmovie_listand uses proper type annotation.
590-596: Well-structured MusicBrainz configuration settings.The MusicBrainz configuration variables are comprehensive and follow good naming conventions. The settings cover all necessary aspects: feature toggle, API credentials, user agent info, and directory management.
sickchill/views/__init__.py (1)
25-25: LGTM!The import of
MusicHandlerfollows the established pattern with other handlers likeMoviesHandler. The static analysis warning about unused import can be safely ignored as this is a standard pattern for exposing imports in package__init__.pyfiles.SickChill.py (2)
30-30: LGTM!The import of
MusicListis clean and follows the existing pattern withMovieList.
220-221: Well-implemented conditional initialization.The conditional initialization of
settings.music_listfollows the same pattern asmovie_listand uses an appropriate feature flag (USE_MUSICBRAINZ). The placement in the startup sequence is logical.sickchill/gui/slick/views/layouts/main.mako (2)
183-194: LGTM! Clean integration of music navigation menu.The music menu follows the established pattern of other dropdown menus and is properly conditionally displayed based on the
USE_MUSICBRAINZsetting. The use ofreverse_urlfor routing ensures consistency with the URL structure.
418-418: LGTM! Appropriate script inclusion.The
musicRootDirs.jsscript is properly included alongside other core scripts in the authenticated user section, following the same pattern as existing functionality.sickchill/gui/slick/views/inc_musicRootDirs.mako (1)
1-37: LGTM! Well-structured music root directories management component.This template follows the established patterns from existing root directory management:
- Proper parsing of the
MUSIC_ROOT_DIRSsetting using pipe delimiter- Consistent UI structure with select box and control buttons
- Good use of Bootstrap responsive classes
- Proper internationalization with
_()function- Hidden inputs for state management following existing patterns
The implementation is clean and maintainable.
sickchill/views/server_settings.py (2)
15-15: LGTM! Proper import addition.The
MusicHandlerimport is correctly added to support the new music functionality.
168-179: LGTM! Comprehensive and well-structured music routing.The music routes are well-designed and consistent:
- Complete coverage of music operations (index, details, add, remove, search, etc.)
- URL patterns follow established conventions from movies routes
- Proper use of named parameters (
slug,pk) for resource identification- Route names follow consistent naming pattern (
music-{action})- No apparent routing conflicts or security concerns
The routing structure provides a solid foundation for the music management feature.
sickchill/gui/slick/views/music/search.mako (1)
18-32: LGTM! Well-structured search form.The search form follows good practices:
- Proper form structure with Bootstrap classes
- Meaningful labels and placeholders for accessibility
- Uses POST method and
reverse_urlfor consistent routing- Good internationalization support
sickchill/views/config/general.py (3)
33-34: LGTM - Consistent pattern implementation.The method follows the same pattern as
saveRootDirsabove, maintaining consistency in the codebase.
69-69: LGTM - Consistent error handling pattern.The music download directory handling follows the same validation and error handling pattern as the log directory above it, ensuring consistent user experience and proper directory management.
Also applies to: 92-93
120-121: LGTM - Standard checkbox setting implementation.The USE_MUSICBRAINZ setting follows the established pattern used by other checkbox settings in this method, using the standard
config.checkbox_to_valuehelper.sickchill/gui/slick/views/config_general.mako (2)
262-279: LGTM - Well-structured directory input field.The music download directory section follows the established pattern and correctly uses the field name that matches the backend expectation.
1175-1187: LGTM - Standard checkbox implementation.The MusicBrainz checkbox follows the established pattern and is appropriately placed in the advanced settings section.
sickchill/gui/slick/views/music/album_details.mako (4)
1-13: LGTM - Well-structured template setup.The imports are appropriate and the background image metadata is a nice UI enhancement. The use of
static_url()provides proper URL generation and XSS protection.
27-94: LGTM - Comprehensive album information display.The album information section is well-structured with proper handling of missing data (e.g., year fallback to "Unknown") and comprehensive status display covering all AlbumStatus enum values with appropriate styling.
96-133: LGTM - Robust track listing with defensive programming.The track list implementation properly handles missing data scenarios and uses defensive programming with
.get()method calls to prevent template errors when MusicBrainz data is incomplete.
136-138: LGTM - Standard JavaScript inclusion.The JavaScript file inclusion follows the established pattern with proper static URL generation.
sickchill/gui/slick/views/music/index.mako (4)
1-20: LGTM - Good UX pattern for feature availability.The conditional display based on
USE_MUSICBRAINZsetting provides clear user guidance when the feature is disabled, with instructions on how to enable it.
32-50: LGTM - Useful filtering feature with minor consideration.The root directory filtering is well-implemented. The
.split('|')[1:]assumes a specific format forMUSIC_ROOT_DIRS- ensure this parsing logic aligns with how the directories are stored and managed in the backend.
51-96: LGTM - Comprehensive artist table with good UX.The artist table implementation handles edge cases well (empty lists, missing country data) and provides appropriate user actions. The responsive design and proper link generation ensure good usability.
98-107: LGTM - Clean and defensive JavaScript implementation.The JavaScript provides smooth UX with automatic form submission on filter changes. The null check and proper event handling demonstrate good defensive programming practices.
sickchill/oldbeard/config.py (1)
302-302: Inconsistent None check compared to similar functions.The condition includes a check for
settings.MUSIC_DOWNLOAD_DIR is None, which differs from the pattern used in similar functions likechange_tv_download_dir(line 281). This inconsistency could lead to unexpected behavior if the setting is already initialized.Consider whether this None check is necessary. If
MUSIC_DOWNLOAD_DIRcan legitimately beNoneinitially, document why this function differs from others. Otherwise, align with the existing pattern:- if settings.MUSIC_DOWNLOAD_DIR is None or os.path.normpath(settings.MUSIC_DOWNLOAD_DIR) != os.path.normpath(music_download_dir): + if os.path.normpath(settings.MUSIC_DOWNLOAD_DIR) != os.path.normpath(music_download_dir):sickchill/start.py (8)
13-13: LGTM! Clean import addition.The music module import is properly integrated with the existing sickchill imports and supports the new MusicBrainz functionality.
186-187: LGTM! Proper conditional feature enablement.The USE_MUSICBRAINZ setting is appropriately restricted to developer mode, which is a sensible approach for a new experimental feature. The conditional logic and formatting are consistent with the existing codebase.
344-345: LGTM! Consistent configuration additions.The music directory settings follow established patterns and naming conventions. The placement after TV_DOWNLOAD_DIR is logical and the empty string defaults are appropriate for optional settings.
970-978: LGTM! Well-configured scheduler setup.The music scheduler is properly integrated with the existing scheduler framework. The timing parameters (10s initial delay, 5s cycle) are appropriate for a queue scheduler, and the silent mode logic correctly ties to the USE_MUSICBRAINZ setting.
1035-1039: LGTM! Consistent scheduler startup integration.The music scheduler startup follows the established pattern used by other feature-based schedulers like Trakt and subtitles. The conditional enablement based on USE_MUSICBRAINZ is properly implemented.
1061-1061: LGTM! Proper shutdown integration.The music scheduler is correctly added to the threads list for proper shutdown handling, ensuring clean termination of the music queue processing.
1263-1264: LGTM! Consistent configuration persistence.The music directory settings are properly persisted to the configuration file with appropriate empty string fallbacks, following the same pattern as other directory settings in the General section.
1302-1302: LGTM! Proper boolean configuration persistence.The USE_MUSICBRAINZ setting is correctly saved to the configuration file with appropriate integer conversion, consistent with other boolean settings in the General section.
| // Music page specific JavaScript | ||
| $(document).ready(function() { | ||
| // Apply background image if FANART_BACKGROUND is enabled | ||
| if (metaToBool('settings.FANART_BACKGROUND')) { | ||
| const backgroundPath = getMeta('showBackgroundImage'); | ||
| if (backgroundPath) { | ||
| $.backstretch(backgroundPath); | ||
| $('.backstretch').css('opacity', getMeta('settings.FANART_BACKGROUND_OPACITY')).fadeIn('500'); | ||
| } | ||
| } | ||
| }); No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clean implementation with good defensive programming.
The JavaScript code is well-structured and follows good practices:
- Uses feature detection with
metaToBool()before applying functionality - Defensive programming by checking
backgroundPathexists - Reasonable animation timing (500ms fade-in)
- Clean jQuery document ready pattern
Please verify that the required dependencies are available:
🏁 Script executed:
#!/bin/bash
# Description: Verify that required JavaScript functions and plugins are available
# Expected: metaToBool, getMeta functions and backstretch plugin should be defined
echo "Checking for required JavaScript dependencies..."
# Search for metaToBool function definition
echo "=== Searching for metaToBool function ==="
rg -A 3 "function metaToBool|metaToBool\s*=" --type js
# Search for getMeta function definition
echo "=== Searching for getMeta function ==="
rg -A 3 "function getMeta|getMeta\s*=" --type js
# Search for backstretch plugin
echo "=== Searching for backstretch plugin ==="
rg -A 3 "backstretch|jquery.*backstretch" --type js
# Search for backstretch script inclusion
echo "=== Searching for backstretch script tags ==="
rg "backstretch" --type htmlLength of output: 273830
Ensure Backstretch plugin is loaded before using it
The helper functions are available and the code is correct, but I didn’t find where the Backstretch plugin itself is included:
• metaToBool/getMeta are defined in sickchill/gui/slick/js/core.js
• $.backstretch(...) is invoked in both core.js and music.js
• No <script> tag or JS file defining the Backstretch plugin was found
Action required:
- Add or verify inclusion of the Backstretch plugin (e.g.
jquery.backstretch.js) in your layout ormain.makobeforemusic.jsis loaded so that$.backstretchis defined.
🤖 Prompt for AI Agents
In sickchill/gui/slick/js/music.js lines 1 to 11, the code uses the Backstretch
plugin via $.backstretch but the plugin script is not included anywhere. To fix
this, ensure the Backstretch plugin JavaScript file (e.g.,
jquery.backstretch.js) is added as a <script> tag in the main layout file such
as main.mako, and that it is loaded before music.js so that $.backstretch is
defined when used.
| method = methods[length]; | ||
|
|
||
| // Only stub undefined methods. | ||
| console[method] ||= noop; |
There was a problem hiding this comment.
Use fallback syntax for broader browser compatibility.
The logical nullish assignment operator (||=) requires ES2021 support, which may not be available in older browsers.
- console[method] ||= noop;
+ console[method] = console[method] || noop;📝 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.
| console[method] ||= noop; | |
| console[method] = console[method] || noop; |
🤖 Prompt for AI Agents
In sickchill/gui/slick/js/musicRootDirs.js at line 38, replace the logical
nullish assignment operator (||=) with a more widely supported fallback syntax.
Instead of using console[method] ||= noop;, use a conditional check to assign
noop only if console[method] is falsy, ensuring compatibility with older
browsers that do not support ES2021 features.
| function postRootDirectories() { | ||
| refreshRootDirectories(); | ||
| $.post(scRoot + '/config/general/saveMusicRootDirs', {musicRootDirString: $('#musicRootDirText').val()}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the AJAX request.
The POST request lacks error handling, which could leave users unaware of save failures.
function postRootDirectories() {
refreshRootDirectories();
- $.post(scRoot + '/config/general/saveMusicRootDirs', {musicRootDirString: $('#musicRootDirText').val()});
+ $.post(scRoot + '/config/general/saveMusicRootDirs', {musicRootDirString: $('#musicRootDirText').val()})
+ .fail(function(xhr, status, error) {
+ console.error('Failed to save music root directories:', error);
+ alert('Failed to save music root directories. Please try again.');
+ });
}📝 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.
| function postRootDirectories() { | |
| refreshRootDirectories(); | |
| $.post(scRoot + '/config/general/saveMusicRootDirs', {musicRootDirString: $('#musicRootDirText').val()}); | |
| } | |
| function postRootDirectories() { | |
| refreshRootDirectories(); | |
| $.post(scRoot + '/config/general/saveMusicRootDirs', {musicRootDirString: $('#musicRootDirText').val()}) | |
| .fail(function(xhr, status, error) { | |
| console.error('Failed to save music root directories:', error); | |
| alert('Failed to save music root directories. Please try again.'); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In sickchill/gui/slick/js/musicRootDirs.js around lines 122 to 125, the $.post
AJAX call to saveMusicRootDirs lacks error handling, which can leave users
uninformed if the save operation fails. Modify the $.post call to include
.done() and .fail() handlers or use the .ajax() method with success and error
callbacks to handle the response properly. In the error handler, provide user
feedback such as an alert or UI message indicating the failure to save the music
root directories.
| isDefault = true; | ||
| } | ||
|
|
||
| $('#musicRootDirs').append('<option value="' + path + '">' + path + '</option>'); |
There was a problem hiding this comment.
Escape user input to prevent XSS vulnerabilities.
The path is directly inserted into HTML without escaping, which could lead to XSS attacks if the path contains malicious content.
- $('#musicRootDirs').append('<option value="' + path + '">' + path + '</option>');
+ const option = $('<option></option>').attr('value', path).text(path);
+ $('#musicRootDirs').append(option);📝 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.
| $('#musicRootDirs').append('<option value="' + path + '">' + path + '</option>'); | |
| - $('#musicRootDirs').append('<option value="' + path + '">' + path + '</option>'); | |
| + const option = $('<option></option>') | |
| + .attr('value', path) | |
| + .text(path); | |
| + $('#musicRootDirs').append(option); |
🤖 Prompt for AI Agents
In sickchill/gui/slick/js/musicRootDirs.js at line 138, the user input variable
'path' is directly inserted into the HTML option element without escaping, which
can cause XSS vulnerabilities. To fix this, escape the 'path' string properly
before inserting it into the HTML, or use a safer method such as creating the
option element with jQuery and setting its text and value properties to avoid
direct HTML injection.
| refreshRootDirectories(); | ||
| $.post(scRoot + '/config/general/saveMusicRootDirs', { | ||
| musicRootDirString: $('#musicRootDirText').val(), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate AJAX calls and add error handling.
The same POST request is duplicated in multiple event handlers without error handling.
Instead of duplicating the AJAX call, reuse the postRootDirectories function which already calls refreshRootDirectories:
- refreshRootDirectories();
- $.post(scRoot + '/config/general/saveMusicRootDirs', {
- musicRootDirString: $('#musicRootDirText').val(),
- });
+ postRootDirectories();Apply this change to both the delete handler (lines 205-209) and default handler (lines 216-220).
Also applies to: 216-220
🤖 Prompt for AI Agents
In sickchill/gui/slick/js/musicRootDirs.js around lines 205 to 209 and 216 to
220, the POST request to save music root directories is duplicated without error
handling. Replace the direct $.post calls in both event handlers with calls to
the existing postRootDirectories function, which already handles refreshing and
includes error handling. This will remove duplication and improve robustness.
|
|
||
| if front_images: | ||
| # Use the first front image | ||
| image_url = front_images[0]["image"] | ||
| logger.debug(f"Found front image for release group: {image_url}") | ||
|
|
||
| # Download the image | ||
| response = requests.get(image_url) | ||
| if response.status_code == 200: | ||
| image_data = response.content | ||
| else: | ||
| logger.warning(f"Failed to download image from {image_url}") | ||
| else: | ||
| # If no front images, try to use any image | ||
| if image_list["images"]: | ||
| image_url = image_list["images"][0]["image"] | ||
| logger.debug(f"No front image found, using first available: {image_url}") | ||
|
|
||
| # Download the image | ||
| response = requests.get(image_url) | ||
| if response.status_code == 200: | ||
| image_data = response.content | ||
| else: | ||
| logger.warning(f"Failed to download image from {image_url}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicated image download logic.
The image download logic is duplicated multiple times in this method, making it harder to maintain.
Extract the image download logic into a helper method:
def _download_image(self, image_list, logger_prefix=""):
"""Download the first available image from an image list."""
image_data = None
# Check for front images first
front_images = [img for img in image_list["images"] if img.get("front", False)]
images_to_try = front_images if front_images else image_list["images"]
if images_to_try:
image_url = images_to_try[0]["image"]
logger.debug(f"{logger_prefix}Found image: {image_url}")
try:
response = requests.get(image_url)
if response.status_code == 200:
image_data = response.content
else:
logger.warning(f"Failed to download image from {image_url}")
except Exception as e:
logger.warning(f"Error downloading image: {e}")
return image_dataThen use it in both places to eliminate duplication.
Also applies to: 400-425
🤖 Prompt for AI Agents
In sickchill/providers/metadata/musicbrainz.py around lines 351 to 374, the
image download logic is duplicated when handling front images and fallback
images. Extract this logic into a helper method that accepts the image list and
an optional logger prefix, which filters front images first and attempts to
download the first available image with proper error handling and logging.
Replace the duplicated code blocks with calls to this helper method. Apply the
same refactoring to lines 400 to 425 where similar duplication occurs.
| import os | ||
|
|
||
| from sickchill import settings | ||
| from sickchill.oldbeard import config |
There was a problem hiding this comment.
Remove unused import.
The config module is imported but never used in this file.
-from sickchill.oldbeard import config📝 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.
| from sickchill.oldbeard import config |
🧰 Tools
🪛 Ruff (0.12.2)
5-5: sickchill.oldbeard.config imported but unused
Remove unused import: sickchill.oldbeard.config
(F401)
🤖 Prompt for AI Agents
In sickchill/views/music.py at line 5, the config module is imported but not
used anywhere in the file. Remove the line importing config from
sickchill.oldbeard to clean up unused imports.
| assert selected_root != "-1" | ||
| selected_root_dir = backend_dirs[int(selected_root)] | ||
| except (IndexError, ValueError, TypeError, AssertionError): | ||
| selected_root_dir = "" |
There was a problem hiding this comment.
Replace assert with proper validation.
Using assert for input validation is problematic as assertions can be disabled with Python's -O flag. This could lead to unexpected behavior in production.
- assert selected_root != "-1"
- selected_root_dir = backend_dirs[int(selected_root)]
- except (IndexError, ValueError, TypeError, AssertionError):
+ if selected_root == "-1":
+ raise ValueError("Invalid root selection")
+ selected_root_dir = backend_dirs[int(selected_root)]
+ except (IndexError, ValueError, TypeError):📝 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.
| assert selected_root != "-1" | |
| selected_root_dir = backend_dirs[int(selected_root)] | |
| except (IndexError, ValueError, TypeError, AssertionError): | |
| selected_root_dir = "" | |
| if selected_root == "-1": | |
| raise ValueError("Invalid root selection") | |
| selected_root_dir = backend_dirs[int(selected_root)] | |
| except (IndexError, ValueError, TypeError): | |
| selected_root_dir = "" |
🤖 Prompt for AI Agents
In sickchill/views/music.py around lines 32 to 35, replace the assert statement
used for validating selected_root with explicit input validation logic. Instead
of using assert, check if selected_root is not equal to "-1" using a conditional
statement, and handle the invalid case appropriately to avoid relying on
assertions which can be disabled in production. Adjust the code to raise an
exception or set selected_root_dir to an empty string if the validation fails.
| result = settings.music_list.add_from_musicbrainz(artist_id=artist_id) | ||
|
|
||
| # If result is an Artist object, use it | ||
| if hasattr(result, 'slug'): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper type checking instead of attribute checking.
Using hasattr(result, 'slug') to determine if the result is an Artist object is fragile and could break if other objects gain a slug attribute.
- if hasattr(result, 'slug'):
+ if isinstance(result, music.Artist):📝 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.
| if hasattr(result, 'slug'): | |
| if isinstance(result, music.Artist): |
🤖 Prompt for AI Agents
In sickchill/views/music.py at line 86, replace the use of hasattr(result,
'slug') with proper type checking to verify if result is an Artist object. Use
an explicit isinstance check against the Artist class instead of relying on the
presence of the 'slug' attribute, ensuring more robust and clear type
validation.
| import unittest | ||
| from unittest import mock | ||
|
|
||
| import musicbrainzngs |
There was a problem hiding this comment.
Remove unused import.
The musicbrainzngs module is imported but never used directly in the tests. All its functionality is mocked.
-import musicbrainzngs
-📝 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.
| import musicbrainzngs |
🧰 Tools
🪛 Ruff (0.12.2)
8-8: musicbrainzngs imported but unused
Remove unused import: musicbrainzngs
(F401)
🤖 Prompt for AI Agents
In tests/test_music.py at line 8, remove the import statement for the
musicbrainzngs module since it is not used directly in the tests and all its
functionality is mocked, making the import unnecessary.
|
You coded this because you couldn't get Lidarr to work? Impressive! |
|
Since SickChill doesn't have separate modules for downloading, etc, that
would mean duplicating and maintaining all the copies for the different
apps.
Since the "movies" integration was in the devel branch, I assumed that was
acceptable
Nicola
Il giorno mar 12 ago 2025 alle ore 12:22 ТΞNSΛI ***@***.***>
ha scritto:
… *Tensai75* left a comment (SickChill/sickchill#8838)
<#8838 (comment)>
You coded this because you couldn't get Lidarr to work? Impressive!
But shouldn't this rather be a separate fork just for music? One app for
one purpose...
—
Reply to this email directly, view it on GitHub
<#8838 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMGFJNCZATOMCUFDHIDYA33NHE57AVCNFSM6AAAAACCQBU4SOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCNZYHEYDIOJSG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Nicola Canepa
|
|
This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
github-actions
bot
added
the
Stale
Proposed changes in this pull request:
support music download via MusicBrainz
I couldn't find working options, including lidarr
I know it's a big change, but it's mainly adding files, and not easy to break down
PR is based on the DEVELOP branch
Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
Read contribution guide
Tested adding artists, downloading and showing images and background
Summary by CodeRabbit
New Features
User Interface
Bug Fixes
Tests
Chores