Add Love and Rating functionality to playlist tracks #4134
Conversation
There was a problem hiding this comment.
Hello @deluan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini or gemini-code-assist here, providing a summary of this pull request.
This PR implements the ability to rate and 'love' (star) tracks directly from the playlist view. The core challenge addressed is ensuring that these actions target the underlying media file associated with the playlist track, rather than the playlist track entry itself, while maintaining backward compatibility for resources without a mediaFileId. The changes involve updates to the backend API to fetch individual playlist tracks, modifications to frontend hooks (useRating, useToggleLove) to correctly identify the target ID and refresh relevant data, and UI updates to display ratings and enable the love functionality in the playlist view.
Highlights
- Backend API: Adds a new
GET /api/playlist/{id}/tracks/{id}endpoint to fetch a specific track within a playlist. - Database Queries: Corrects SQL queries in
playlist_track_repository.goto properly qualify column names and return thePlaylistTrackobject instead of the nestedMediaFile. - Frontend Rating Logic: Updates the
useRatinghook andRatingFieldcomponent to use themediaFileIdfor rating actions when available, falling back to the record's own ID. - Frontend Love Logic: Updates the
useToggleLovehook to use themediaFileIdfor star/unstar actions when available, falling back to the record's own ID. - Data Refresh Strategy: Modifies the
useRatinganduseToggleLovehooks to refresh both the playlist track and the underlying song data after a rating or love action on a playlist track, ensuring UI consistency. - Playlist UI: Adds a rating column to the playlist songs view and enables the love/star functionality in the context menu for playlist tracks.
- Testing: Adds comprehensive test suites for the updated
useRatinganduseToggleLovehooks, specifically covering playlist track scenarios and the dual refresh behavior.
Changelog
Click here to see the changelog
- persistence/playlist_track_repository.go
- Corrected the
Readmethod's SQL query to qualify theidcolumn withplaylist_tracks.id(line 102). - Changed the return value of the
Readmethod fromtrk.PlaylistTrack.MediaFiletotrk.PlaylistTrack(line 105).
- Corrected the
- server/nativeapi/native_api.go
- Added a new GET route
/within the playlist track ID route (r.Route("/{id}")) to handle fetching individual playlist tracks (lines 147-149).
- Added a new GET route
- server/nativeapi/playlists.go
- Implemented the
getPlaylistTrackhandler function, including middleware to extract theplaylistIdfrom the URL and construct the correct repository (lines 48-62).
- Implemented the
- ui/src/common/RatingField.jsx
- Modified the
handleRatingcallback to userecord.mediaFileIdas the target ID for theratefunction, falling back torecord.id(line 41). - Updated the
nameprop of theRatingcomponent to also userecord.mediaFileIdorrecord.id(line 50). - Added
record.mediaFileIdandrecord.idto the dependency array of thehandleRatinguseCallback hook (line 44).
- Modified the
- ui/src/common/useRating.jsx
- Updated the
refreshRatingfunction to check forrecord.mediaFileId(line 21). - If
mediaFileIdis present, it now fetches both the 'song' resource usingmediaFileIdand the 'playlistTrack' resource usingrecord.idandplaylistIdfilter (lines 23-29). - If
mediaFileIdis not present, it falls back to fetching only the original resource usingrecord.id(lines 42-53). - Added
record.mediaFileId,record.id,record.playlistId, andresourceto the dependency array of therefreshRatinguseCallback hook (line 55).
- Updated the
- ui/src/common/useRating.test.js
- Added a new test file
useRating.test.jsto provide comprehensive test coverage for theuseRatinghook. - Includes tests for basic rating functionality, handling zero rating, and specific scenarios for playlist tracks (refreshing both resources, using playlist_id filter) and non-playlist resources.
- Also includes tests simulating component integration scenarios for ID fallback.
- Added a new test file
- ui/src/common/useToggleLove.jsx
- Updated the
refreshRecordfunction to fetch both the original resource (with playlistId filter if applicable) and the 'song' resource usingmediaFileIdifmediaFileIdis present (lines 20-32). - Added
record.mediaFileId,record.id,record.playlistId, andresourceto the dependency array of therefreshRecorduseCallback hook (line 39). - Modified the
toggleLovefunction to userecord.mediaFileIdas the target ID for the star/unstar API calls, falling back torecord.id(line 43).
- Updated the
- ui/src/common/useToggleLove.test.js
- Added a new test file
useToggleLove.test.jsto provide comprehensive test coverage for theuseToggleLovehook. - Includes tests for using
mediaFileId, falling back toid, callingunstar, and specific scenarios for playlist tracks (refreshing both resources, using playlist_id filter) and non-playlist resources.
- Added a new test file
- ui/src/playlist/PlaylistSongs.jsx
- Imported
RatingField(line 29) andconfig(line 35). - Added a new CSS class
ratingFieldand updated the hover styles to make it visible on hover (lines 79-81, 71-73). - Added a
ratingcolumn definition usingRatingField, conditionally rendered based onconfig.enableStarRating(lines 164-171). - Added
classes.ratingFieldto the dependency array of theuseMemohook for column definitions (line 173). - Added
ratingto the default selected fields for the playlist track resource (line 185). - Enabled the love functionality in the
SongContextMenuby settingshowLove={true}(line 225).
- Imported
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Playlist tunes flow,
Now rate and love each song,
Stars shine on the list.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively implements the Love and Rating functionality for playlist tracks by correctly targeting the underlying media files. The backend changes for the new endpoint and SQL query adjustments are clear, and the frontend hooks (useRating, useToggleLove) have been updated logically to support this, including the dual refresh mechanism for UI consistency.
The addition of comprehensive test suites for useRating and useToggleLove is a significant contribution and greatly enhances confidence in these changes. Well done on that!
I've identified a couple of high-severity issues related to error handling and loading state management in the React hooks, and one medium-severity suggestion for improving dependency arrays. Addressing these will further improve the robustness of the new functionality.
Summary of Findings
- Error Handling in
useRatingHook: TherefreshRatingfunction withinuseRating.jsxdoes not properly handle errors from its internal data fetching promises, potentially leaving the loading state astrueindefinitely if a refresh fails after a rating action. - Error Handling in
useToggleLoveHook: TherefreshRecordfunction withinuseToggleLove.jsxlacks a.catch()block for itsPromise.allcall. This can lead to unhandled errors and the loading state not being reset if a refresh operation fails after a love/unlove action. - Dependency Array Specificity in
useRating: TheuseCallbackforrefreshRatinginuseRating.jsxcould use a more specific dependency array to avoid unnecessary recreations of the callback, potentially improving performance and maintainability.
Merge Readiness
The pull request introduces valuable functionality and is generally well-implemented, especially with the inclusion of thorough tests. However, there are a couple of high-severity issues related to error handling in the React hooks (useRating and useToggleLove) that could lead to inconsistent UI states (e.g., stuck loading indicators).
I recommend addressing these high-severity issues before merging to ensure the robustness of the new features. Once these are resolved, the PR should be in good shape. As an AI, I am not authorized to approve pull requests; please ensure further review and approval from authorized team members.
There was a problem hiding this comment.
Pull Request Overview
Adds love/star and rating functionality to playlist tracks by targeting the underlying media file and keeping both playlist and song views in sync, plus a new backend endpoint for fetching individual playlist-track entries.
- UI
- Shows a rating column when enabled and toggles love in the playlist context menu
- Hooks (
useToggleLove,useRating) now usemediaFileIdand refresh both playlist-track and song data
- Backend
- New
GET /api/playlist/{id}/tracks/{id}endpoint withgetPlaylistTrackhandler - Fixed repository to qualify
playlist_tracks.idand return the playlist-track entity instead of the media file
- New
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/playlist/PlaylistSongs.jsx | Imported/enabled RatingField, updated styles & columns list |
| ui/src/common/useToggleLove.jsx | Refactored refresh to handle mediaFileId, dual data refresh |
| ui/src/common/useRating.jsx | Updated refreshRating to reload both song & playlist-track |
| server/nativeapi/playlists.go | Added getPlaylistTrack middleware handler |
| persistence/playlist_track_repository.go | Qualified playlist_tracks.id, return PlaylistTrack object |
Comments suppressed due to low confidence (2)
server/nativeapi/playlists.go:48
- New
getPlaylistTrackendpoint is added without any tests. Add unit or integration tests to verify correct middleware wiring, parameter handling, and response payload.
func getPlaylistTrack(ds model.DataStore) http.HandlerFunc {
ui/src/playlist/PlaylistSongs.jsx:185
- Including 'rating' in
useSelectedFieldsfor the 'playlistTrack' resource will cause an API error since playlistTrack objects do not have a 'rating' field. Consider removing 'rating' here or mapping the joined media file's rating into the playlistTrack DTO.
'rating',
Signed-off-by: Deluan <deluan@navidrome.org>
Signed-off-by: Deluan <deluan@navidrome.org>
…ting components Signed-off-by: Deluan <deluan@navidrome.org>
e3e1f31 to
d1da67a
Compare
Add Love and Rating functionality to playlist tracks
This PR implements rating and love/star functionality for tracks when viewed in playlists.
Changes Made
Backend:
GET /api/playlist/{id}/tracks/{id}endpoint for fetching individual playlist tracksplaylist_track_repository.goto properly qualify column names and return playlist track objects instead of media filesgetPlaylistTrackhandler function with proper middleware setupFrontend:
RatingFieldanduseRatingto usemediaFileIdwhen available (for playlist tracks) instead of the playlist track IDuseToggleLovewith similar logic to target the underlying media file for star/unstar operationsTesting:
useRatinganduseToggleLovecovering playlist track scenariosTechnical Details
The core issue was that playlist tracks are references to media files, so rating/starring operations need to target the
mediaFileIdrather than the playlist track ID itself. The solution maintains backward compatibility by falling back torecord.idwhenmediaFileIdis not present.When refreshing data after rating/starring playlist tracks, both the playlist view and the song library are updated to ensure consistency across the UI.