fix(ui): always order album tracks by disc and track number (fixes #3720) #3975
Conversation
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
This was referenced Apr 16, 2025
Closed
3 tasks
|
@ellipsis review this PR |
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 225ce32 in 2 minutes and 1 seconds
More details
- Looked at
222lines of code in6files - Skipped
0files when reviewing. - Skipped posting
13drafted comments based on config settings.
1. persistence/mediafile_repository.go:84
- Draft comment:
The new 'album_tracks' sort mapping ("disc_number, track_number") is correct and resolves the ordering issue. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that something is correct, which is not necessary for the PR author to know.
2. ui/src/album/AlbumShow.jsx:38
- Draft comment:
Updated sort field to 'album_tracks' ensures tracks are sorted by disc and track number as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, stating what the update does without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.
3. ui/src/album/AlbumSongs.jsx:185
- Draft comment:
Removal of 'showReleaseDivider' logic cleans up legacy grouping without affecting the correct order; this appears correct. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it praises the removal of a piece of logic without suggesting any changes or asking for confirmation. It doesn't provide any actionable feedback or raise any concerns.
4. ui/src/common/ContextMenus.jsx:230
- Draft comment:
Using 'album_tracks' in songQueryParams is correct; consider if passing 'release_date' and 'disc_number' in the filter is still necessary. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment has two parts: 1) Confirming the change to 'album_tracks' is correct (which is about the diff), 2) Questioning release_date and disc_number filters (which are unchanged code). The second part violates the rule about not commenting on unchanged code. The first part is just agreeing with the change, which isn't actionable.
Maybe the filters are actually problematic and worth fixing even if they weren't changed? Maybe confirming the change is correct provides valuable validation?
Per the rules, we should not comment on unchanged code even if it could be improved. Additionally, simply confirming a change is correct doesn't require action and thus violates the rule about only making comments that require code changes.
The comment should be deleted as it combines validation of the current change (which doesn't require action) with suggestions about unchanged code (which we shouldn't comment on).
5. ui/src/common/PlayButton.jsx:24
- Draft comment:
The sort parameter updated to 'album_tracks' in the PlayButton ensures playback order is correct. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it just states that the sort parameter update ensures correct playback order. It doesn't provide any actionable feedback or suggestions for improvement.
6. ui/src/common/SongDatagrid.jsx:59
- Draft comment:
Removal of obsolete release grouping logic (ReleaseRow and firstTracksOfReleases) simplifies the UI; ordering now relies on backend 'album_tracks'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, explaining what the code change does without suggesting any action or asking for clarification. It doesn't provide any actionable feedback or raise any concerns about the code change.
7. persistence/mediafile_repository.go:84
- Draft comment:
Mapping 'album_tracks' is correctly added to sort by disc_number and track_number. Ensure that the DB column names match these identifiers. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment starts with a positive note about the mapping being correct, which is informative. However, it then asks to ensure that the DB column names match the identifiers, which is a request for confirmation. This is not allowed as per the rules.
8. ui/src/album/AlbumShow.jsx:38
- Draft comment:
Updated the sort field to 'album_tracks' on the ReferenceManyField. This should consistently order the tracks, assuming the backend columns are set correctly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, explaining what was changed and the expected outcome. It does not provide a suggestion, ask for confirmation, or point out a potential issue.
9. ui/src/album/AlbumSongs.jsx:185
- Draft comment:
Removal of the 'showReleaseDivider' prop is appropriate given that the obsolete release date grouping logic has been removed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it praises the removal of a prop without suggesting any changes or asking for confirmation. It doesn't provide any actionable feedback or raise any concerns.
10. ui/src/common/ContextMenus.jsx:230
- Draft comment:
The album context menu now sorts by 'album_tracks', but the filter still includes release_date. Since the intention is to ignore release_date differences for ordering, consider removing 'release_date' from the filter to ensure tracks aren’t inadvertently excluded in compilation albums. - Reason this comment was not posted:
Comment was on unchanged code.
11. ui/src/common/PlayButton.jsx:22
- Draft comment:
Although the sort field is updated to 'album_tracks', the filter still includes release_date. To fully ignore differences in release_date, consider removing it from the filter so that all tracks for the album (or the selected disc) are fetched. - Reason this comment was not posted:
Comment was on unchanged code.
12. ui/src/common/SongDatagrid.jsx:240
- Draft comment:
In the grouping logic (firstTracksOfDiscs) and the playSubset callback, a check on release_date is still used to separate groups and filter tracks. Since release_date differences should be ignored for ordering, update these parts to group and filter solely by discNumber. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. persistence/mediafile_repository.go:247
- Draft comment:
Typo detected: 'participnts' should be corrected to 'participants' in the comment describing GetMissingAndMatching. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_dXZnj7rYaGOF6tUn
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a4e4d13 in 54 seconds
More details
- Looked at
56lines of code in4files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. persistence/mediafile_repository.go:80
- Draft comment:
Changing the 'album' sort mapping removes release_date and adds album_id. Verify that this change meets all use cases and that ordering solely by disc_number and track_number (after album name and id) is the intended behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify their intention and ensure that the change meets all use cases, which violates the rules. It does not provide a specific code suggestion or ask for a specific test to be written.
2. ui/src/album/AlbumShow.jsx:38
- Draft comment:
Sort mapping changed from 'album_tracks' to 'album'. Confirm that this update aligns with back‑end changes and that no obsolete filtering (e.g. release_date) is required. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm that the change aligns with back-end changes and that no obsolete filtering is required. This is asking for confirmation and verification, which violates the rules.
3. ui/src/common/ContextMenus.jsx:230
- Draft comment:
Updated sort mapping from 'album_tracks' to 'album'. Double-check if filter parameters (including release_date and disc_number) should be cleaned up as well, since release_date grouping is now obsolete. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to double-check if filter parameters should be cleaned up, which violates the rule against asking the author to double-check things. It does not provide a specific suggestion or point out a specific issue that needs to be addressed.
4. ui/src/common/PlayButton.jsx:24
- Draft comment:
Sort update to use field 'album' is consistent with backend changes. However, note that the filter still includes release_date (and disc_number). Confirm this is intentional given the removed ordering by release_date. - Reason this comment was not posted:
Comment was on unchanged code.
5. ui/src/common/ContextMenus.jsx:230
- Draft comment:
The filter still includes release_date (and disc_number), which may exclude tracks with differing release_date values in compilations. Consider removing release_date from the filter to align with the new sort logic. - Reason this comment was not posted:
Comment was on unchanged code.
6. ui/src/common/PlayButton.jsx:22
- Draft comment:
The query filter still includes release_date (and disc_number) when fetching album tracks. This may omit tracks in compilations that have varying release_date tags. Verify if the filter should be simplified to use only album_id. - Reason this comment was not posted:
Comment was on unchanged code.
7. persistence/mediafile_repository.go:245
- Draft comment:
Typographical Error: In the comment on line 245, 'participnts' is incorrectly spelled. It should be changed to 'participants'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_GEzBpKGRR3mEIWoD
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1a84b90 in 16 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. persistence/mediafile_repository.go:245
- Draft comment:
Typo fixed: 'participnts' corrected to 'participants'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. persistence/mediafile_repository.go:243
- Draft comment:
Typo fixed: corrected 'participnts' to 'participants' in the comment. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_bZTj2gYdz5FAOddi
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Removes the leftover `releaseDate` check from the `firstTracksOfDiscs` calculation in `SongDatagridBody`. This check caused unnecessary `DiscSubtitleRow` insertions when tracks on the same disc had different release dates, leading to an incorrect visual grouping that resembled a multi-disc layout. This change ensures disc subtitles are only shown when the actual `discNumber` changes, correcting the UI presentation issue reported in issue #3720 after PR #3975.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b782f50 in 54 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. ui/src/common/SongDatagrid.jsx:241
- Draft comment:
Good change—removing the releaseDate check ensures grouping is only by discNumber. This aligns with the intended fix for #3720. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. ui/src/common/SongDatagrid.jsx:241
- Draft comment:
Good removal of the releaseDate condition for grouping by disc number. However, note that the playSubset function (lines 207–214) still filters by releaseDate. This inconsistency could cause issues if tracks within the same disc have different releaseDate values. Consider updating the playSubset filter to rely solely on discNumber for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_UX2H8KmTrXI23W8a
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Cleaned up leftover `releaseDate` references in `SongDatagrid.jsx`: - Removed `releaseDate` parameter and usage from `handlePlaySubset` in `DiscSubtitleRow`. - Removed `releaseDate` prop passed to `AlbumContextMenu` in `DiscSubtitleRow`. - Removed `releaseDate` from the drag item data in `SongDatagridRow`. - Removed `releaseDate` parameter and the corresponding `else` block from the `playSubset` function in `SongDatagridBody`. This ensures the component consistently uses `discNumber` for grouping and playback actions initiated from the disc subtitle, fully resolving the inconsistencies related to issue #3720.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 854023d in 34 seconds
More details
- Looked at
59lines of code in1files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. ui/src/common/SongDatagrid.jsx:66
- Draft comment:
Good change: the handler now solely uses discNumber. This simplifies the logic in light of the new sorting behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. ui/src/common/SongDatagrid.jsx:82
- Draft comment:
Ensuring the click handler now only passes discNumber seems consistent with the new sorting logic. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. ui/src/common/SongDatagrid.jsx:204
- Draft comment:
The playSubset callback now filters tracks based solely on discNumber. Confirm that tracks without a defined discNumber are handled as intended. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. ui/src/common/SongDatagrid.jsx:64
- Draft comment:
Good removal of the 'releaseDate' parameter from the handlePlaySubset function. Ensure that all tracks have a defined discNumber, as this filter now solely depends on it. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. ui/src/common/SongDatagrid.jsx:80
- Draft comment:
Updated onClick to pass only record.discNumber. Confirm that this change aligns with backend expectations since the releaseDate is now omitted. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. ui/src/common/SongDatagrid.jsx:92
- Draft comment:
Removal of the 'releaseDate' prop from AlbumContextMenu is consistent with the updated sorting logic. Verify that AlbumContextMenu no longer depends on it. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. ui/src/common/SongDatagrid.jsx:127
- Draft comment:
The discs object now omits releaseDate and only includes albumId and discNumber. Ensure downstream code consuming this object is updated accordingly. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. ui/src/common/SongDatagrid.jsx:205
- Draft comment:
Simplified playSubset now filters solely by discNumber. Confirm that tracks without a defined discNumber are either not expected or handled appropriately. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_qCMUPTZSbWqAiscp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
What this fixes
This PR resolves issue #3720, where tracks in compilation/grouped albums could appear out of order in the UI if their
ORIGINALDATEorrelease_datetags differed. Previously, the sort order includedrelease_date, which caused tracks to be grouped and ordered incorrectly.What changed
"album"field inmediafile_repository.go. It now sorts tracks byorder_album_name, album_id, disc_number, track_number, ...instead of usingrelease_date. This ensures tracks within the same album are always grouped together and ordered correctly by disc/track number, regardless of release date differences.SongDatagridandAlbumSongs. Tracks are now always displayed as a flat list ordered by disc/track.AlbumContextMenu) and the play button (PlayButton) to remove the dependency onrelease_date, ensuring all relevant tracks are fetched for the album/disc.Result
Tracks in albums are now always displayed and played in the correct order (by disc number, then track number) across the UI, regardless of their
release_dateorORIGINALDATEtags. The relevant UI code is also cleaner and more maintainable.Closes #3720.