Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds editable-playlist metadata to queue types and persisted queues. Playlist playback passes this metadata to queues and menus. Removal actions schedule synchronization and update matching local playlist state. ChangesEditable Playlist Removal
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PlayerMenu
participant LocalSyncUtils
participant OnlinePlaylistViewModel
User->>PlayerMenu: select remove from playlist
PlayerMenu->>LocalSyncUtils: scheduleRemoveFromPlaylist(playlistId, setVideoId)
PlayerMenu->>OnlinePlaylistViewModel: remove matching local song when applicable
PlayerMenu->>User: show toast and dismiss menu
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description includes the required Problem, Cause, Solution, Testing, and Related Issues sections. It provides detailed context, implementation changes, regression coverage, and test results. The Related Issues placeholders remain empty, but this is non-critical because the rest of the description is complete.
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/kotlin/com/metrolist/music/playback/queues/YouTubePlaylistQueue.kt`:
- Around line 28-50: `YouTubePlaylistQueue` is accessing `setVideoIds` from
multiple dispatchers, which can cause the recorded `setVideoId` to be missed
when `PlayerMenu` reads it on Main while `toPlaylistItem` writes it on IO. Make
`setVideoIds` thread-safe or confine all reads/writes to the same dispatcher,
and ensure both `getSetVideoId` and `SongItem.toPlaylistItem` use the same
concurrency strategy so the mapping remains visible across
`YouTubePlaylistQueue` and `PlayerMenu`.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/PlayerMenu.kt`:
- Around line 158-189: The ListQueue removal target logic in PlayerMenu’s
produceState block is missing the same editability gate used for
YouTubePlaylistQueue, so non-editable local playlists can still show “Remove
from playlist.” Add an isEditable property to ListQueue, pass it through where
LocalPlaylistScreen constructs ListQueue, and then require queue.isEditable ==
true before creating the PlaylistRemovalTarget for the ListQueue branch in
PlayerMenu.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40361d10-714d-4745-ac5b-ad7eb96c9ba0
📒 Files selected for processing (8)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktapp/src/main/kotlin/com/metrolist/music/playback/queues/ListQueue.ktapp/src/main/kotlin/com/metrolist/music/playback/queues/YouTubePlaylistQueue.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/PlayerMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/playlist/LocalPlaylistScreen.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/playlist/OnlinePlaylistScreen.ktapp/src/main/kotlin/com/metrolist/music/viewmodels/OnlinePlaylistViewModel.kt
nyxiereal
left a comment
nyxiereal
left a comment
There was a problem hiding this comment.
Song ID maps collapse duplicate playlist occurrences and can remove the wrong item. Please carry occurrence identity, gate removal on playlist editability, publish metadata safely across threads, and test duplicate songs.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
4296-4296: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate playlist context through every playlist queue producer.
The new metadata is passed only in the uncached online branch at Line [4296]. The cached online branch at Lines [4289-4292], the local widget branch at Lines [4278-4281], and alarm playback at Lines [4410-4415] still create
ListQueuewithoutplaylistBrowseId,playlistId, orplaylistIsEditable.Songs started through these paths reach
PlayerMenuwithout editable-playlist context.Remove from playlistremains unavailable. Populate the metadata in every playlist queue producer.Based on the PR objectives and the supplied queue-construction call sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` at line 4296, Update every playlist-related ListQueue construction in MusicService, including the cached online branch, local widget branch, and alarm playback path, to populate playlistBrowseId, playlistId, and playlistIsEditable consistently with the already-updated uncached online branch. Ensure all playlist queue producers propagate the available playlist context so PlayerMenu receives editable-playlist metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Line 4296: Update every playlist-related ListQueue construction in
MusicService, including the cached online branch, local widget branch, and alarm
playback path, to populate playlistBrowseId, playlistId, and playlistIsEditable
consistently with the already-updated uncached online branch. Ensure all
playlist queue producers propagate the available playlist context so PlayerMenu
receives editable-playlist metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d66549e8-01aa-4659-a28c-2f63542b9225
📒 Files selected for processing (9)
app/src/main/kotlin/com/metrolist/music/extensions/QueueExt.ktapp/src/main/kotlin/com/metrolist/music/models/PersistQueue.ktapp/src/main/kotlin/com/metrolist/music/playback/MusicService.ktapp/src/main/kotlin/com/metrolist/music/playback/queues/ListQueue.ktapp/src/main/kotlin/com/metrolist/music/playback/queues/YouTubePlaylistQueue.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/PlayerMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/playlist/LocalPlaylistScreen.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/playlist/OnlinePlaylistScreen.ktapp/src/main/kotlin/com/metrolist/music/viewmodels/OnlinePlaylistViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/kotlin/com/metrolist/music/ui/screens/playlist/OnlinePlaylistScreen.kt
- app/src/main/kotlin/com/metrolist/music/ui/menu/PlayerMenu.kt
- app/src/main/kotlin/com/metrolist/music/ui/screens/playlist/LocalPlaylistScreen.kt
|
The final occurrence/editability handling looks improved, but this is a destructive and failure-sensitive path without a current build or regression coverage. Please rebase and add tests for duplicate song occurrences, |
Removing a song from a playlist is destructive and irreversible from the UI, so the code that decides *which* row to delete now lives in pure functions that can be tested directly, instead of inside composables and a fire-and-forget sync coroutine. - Extract resolvePlaylistRemovalTarget() and selectLocalPlaylistRowToRemove() out of PlayerMenu, so occurrence identity and editability rules are testable and shared. - Extract resolveSetVideoIdForRemoval() out of SyncUtils, making the remote lookup budget injectable and pinning the rule that a removal is abandoned - never guessed - when no setVideoId can be resolved. - Deduplicate the three playlist queue builders in LocalPlaylistScreen into Playlist.toListQueue(). - Drop the SongItem.toPlaylistItem() indirection, which just called toMediaItem(). Tests cover duplicate occurrences of one song, setVideoId identity, queues restored from disk, non-editable playlists, and remote lookup failure and retry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cf7319678
ℹ️ 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".
| val playlistRemovalTarget = remember(currentQueue, mediaMetadata.setVideoId) { | ||
| resolvePlaylistRemovalTarget(currentQueue, mediaMetadata.setVideoId) | ||
| } |
There was a problem hiding this comment.
When a song from playlist B is added via Play next/Add to queue while playlist A is playing, currentQueue remains A but the appended item's setVideoId belongs to B. This resolves a removal target for A, so the player menu exposes an action that sends B's occurrence ID to the wrong playlist and normally fails to remove anything. Associate the playlist ID with the media item, or suppress the action when the item cannot be proven to belong to the queue's playlist.
Useful? React with 👍 / 👎.
| playlistBrowseId = playlist?.browseId, | ||
| playlistId = playlist?.id, | ||
| playlistIsEditable = playlist?.isEditable == true, |
There was a problem hiding this comment.
For an editable playlist started from a widget, the new playlist context is attached but items = songs.map { it.song.toMediaItem() } discards PlaylistSong.map.setVideoId; resolvePlaylistRemovalTarget therefore always returns null and the player menu still lacks Remove from playlist. The cached-online widget path and alarm path construct items the same way, so they need the occurrence-aware conversion used by Playlist.toListQueue.
Useful? React with 👍 / 👎.
| onSongRemovedFromPlaylist = { | ||
| songItem.setVideoId?.let { setVideoId -> | ||
| viewModel.removeSongFromLocalList(songItem.id, setVideoId) |
There was a problem hiding this comment.
When an online playlist contains the same video more than once, this commit now retains both occurrences and removes one by setVideoId, but OnlinePlaylistScreen still tracks selection, active state, anchors, and click behavior solely by songItem.id. Selecting either occurrence consequently checks both, and tapping the second while the first is active executes togglePlayPause() instead of playing that occurrence. Use a compound occurrence identity such as video ID plus setVideoId throughout this screen.
Useful? React with 👍 / 👎.
| val match = Regex("""\d+""").find(countText) | ||
| if (match != null) { | ||
| val count = match.value.toIntOrNull() ?: return@let countText | ||
| countText.replace(match.value, (count - 1).coerceAtLeast(0).toString()) |
There was a problem hiding this comment.
When YouTube supplies a formatted count such as 1,234 songs or 1.2K songs, Regex("""\d+""") matches only the initial 1, producing values such as 0,234 songs after one removal. Avoid editing the server-formatted label this way, or parse the complete localized/abbreviated count before decrementing it.
Useful? React with 👍 / 👎.
This branch has not been deployed
2 participants
Problem
The
Remove from playlistaction was missing from the player menu whenselecting a currently playing song from a playlist.
Cause
The player menu could not reliably identify the playlist context for the
currently playing item. Playlist browse IDs and set video IDs were
available in some queue paths, but the local playlist ID was not preserved,
Solution
Remove from playlistshould be shown in the player menu.
direct removal calls from the menu.
items.
Review follow-up
Addressing the request for a rebase and regression coverage on this
destructive path:
main; the two merge commits are gone and thebranch is now a clean series on top of
chore(deps): bump InnerTubeX to 0.4.1.adoptQueue()(addedupstream in Changed the logic of the queue created by AA search #4214) assigned to
currentQueue, which this PR turns intoa read-only
StateFlow. It now writes_currentQueue.value.and out of the fire-and-forget sync coroutine into pure functions, so it
can be tested directly:
resolvePlaylistRemovalTarget()/selectLocalPlaylistRowToRemove()(extracted from
PlayerMenu)resolveSetVideoIdForRemoval()(extracted fromSyncUtils), whichmakes the remote lookup budget injectable and pins the rule that a
removal is abandoned, never guessed, when no setVideoId resolves.
LocalPlaylistScreeninto
Playlist.toListQueue(), and dropped theSongItem.toPlaylistItem()indirection that just called
toMediaItem().Regression coverage
23 new unit tests, covering each case raised in review:
duplicate occurrences of one song resolve to different targets,local row lookup picks the occurrence matching the set video idmissing or blank set video id yields no target,local row lookup ignores the same song in other playlists,local row lookup returns null when no occurrence matchesQueuePlaylistContextTest(persist/restore round trip, Java serialization round trip, per-occurrence targets after restore)non editable youtube playlist yields no target,non editable list queue yields no target,list queue without browse id yields no targetSetVideoIdResolverTest(retry until success, retry after a thrown lookup, give up after the attempt budget instead of removing an unknown occurrence)Known migration cost
PersistQueuegains three fields, which changes its JavaserialVersionUID. A queue persisted by an older build therefore fails todeserialize once on upgrade. The failure is already caught and the file is
cleared, so there is no crash — users lose the restored queue a single
time, and it is repopulated on the next playback. Flagging it rather than
pinning the previous UID; happy to do that instead if you would prefer to
keep old queues readable.
Testing
./gradlew :app:assembleFossDebug :app:testFossDebugUnitTest— build successful,new tests pass (23 tests, 0 failures, 0 errors).
Related Issues
Summary by CodeRabbit
New Features
Bug Fixes
Chores