fix: correct track ordering when sorting playlists by album (#3177) #4657
Conversation
Signed-off-by: Deluan <deluan@navidrome.org>
Signed-off-by: Deluan <deluan@navidrome.org>
Fixed issue #3177 where tracks within multi-disc albums were displayed out of order when sorting playlists by album. The playlist track repository was using an incomplete sort mapping that only sorted by album name and artist, missing the critical disc_number and track_number fields. Changed the album sort mapping in playlist_track_repository from: order_album_name, order_album_artist_name to: order_album_name, order_album_artist_name, disc_number, track_number, order_artist_name, title This now matches the sorting used in the media file repository, ensuring tracks are sorted by: 1. Album name (groups by album) 2. Album artist (handles compilations) 3. Disc number (multi-disc album discs in order) 4. Track number (tracks within disc in order) 5. Artist name and title (edge cases with missing metadata) Added comprehensive tests with a multi-disc test album to verify correct sorting behavior.
Summary of ChangesHello @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! This pull request resolves a long-standing bug affecting how tracks from multi-disc albums are ordered within playlists when sorted by album. The core change involves refining the sorting criteria in the playlist track repository to properly account for disc and track numbers, thereby providing users with an accurate and intuitive track listing. The update also includes new test cases to validate the improved sorting logic. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the incorrect track ordering for multi-disc albums in playlists. The solution, which involves updating the sort mapping to include disc_number and track_number, is direct and correct. I appreciate the comprehensive tests you've added, which not only verify the fix but also improve the test suite with multi-disc album data. The changes are clear and well-documented in the pull request description. I have one minor suggestion regarding an unrelated dependency change, but overall, this is a solid contribution.
There was a problem hiding this comment.
Pull Request Overview
This PR improves playlist track sorting when ordered by album, ensuring tracks from multi-disc albums are sorted correctly by disc number and track number. It also includes a dependency update to fix an issue with the wazero library.
- Enhanced album sorting to include disc number, track number, artist name, and title for proper multi-disc album ordering
- Added comprehensive test coverage for multi-disc album sorting in playlists
- Updated wazero dependency to a fork version to address issue #4396
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| persistence/playlist_track_repository.go | Updated album sort mapping to include disc_number, track_number, order_artist_name, and title |
| persistence/playlist_repository_test.go | Added new test case to verify correct sorting of multi-disc album tracks |
| persistence/persistence_suite_test.go | Added test fixtures for multi-disc album with tracks on different discs |
| persistence/mediafile_repository_test.go | Updated count assertion to account for new test fixtures |
| persistence/album_repository_test.go | Updated test expectations to include new multi-disc test album |
| go.mod | Added replace directive for wazero fork and reformatted existing replace |
| go.sum | Updated checksums for new wazero fork dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Description
Fix issue where tracks within multi-disc albums were displayed in incorrect order when sorting playlists by album.
Problem
When users sorted playlist tracks by album, tracks from multi-disc albums appeared in the wrong order, ignoring disc numbers and track numbers. For example, track 11 of disc 2 would appear before track 1 of disc 1 instead of appearing after it.
Root Cause
The playlist track repository had an incomplete sort mapping for the 'album' field. It only sorted by album name and album artist, completely missing the critical disc_number and track_number fields.
Solution
Updated the album sort mapping in
playlist_track_repository.goto match the sorting pattern used in the media file repository:Before:
After:
This ensures consistent sorting across the application, with tracks ordered by:
Changes
How to Test
Checklist
Please review and check all that apply:
Relates to issue #3177