Repository navigation
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:
Walkthrough
Pre-merge checks:
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly identifies the main change: synchronizing playlist removals initiated from menus. |
| Description check | |
The description clearly explains the problem, the implemented solution, and extensive validation results. It does not include the template headings for Cause or Related Issues, and it uses Changes and… |
Full details: Description check
-
Explanation
The description clearly explains the problem, the implemented solution, and extensive validation results. It does not include the template headings for Cause or Related Issues, and it uses Changes and Validation instead of Solution and Testing, but the required information is mostly present.
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
![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, |
|
@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 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
nyxiereal
left a comment
nyxiereal
left a comment
There was a problem hiding this comment.
Please await remote success or support rollback before deleting local/UI rows, preserve old saved queues, and make duplicate playback/selection occurrence-aware. Also fix formatted counts and carry setVideoId through widget/alarm queues. The new helper tests are useful, but add integration coverage for these failure and upgrade paths before merging.
|
Testing this build on my phone now. I'll post an update once verified. Also, I wasn't aware that pushing to my repo would auto-update the PR—sorry for the extra notifications, I'll be more careful! |
nyxiereal
left a comment
nyxiereal
left a comment
There was a problem hiding this comment.
The occurrence-target helpers and new tests improve this patch, but the destructive and upgrade paths still need changes:
PlayerMenu.kt:524-545schedules remote removal and then deletes the local mapping without awaiting remote success.YouTubeSongMenulikewise removes the visible row immediately. If the queued remote edit fails or is skipped, local/UI state no longer matches YouTube. Await a success result or implement rollback before committing those removals.- Adding fields to Java-serialized
PersistQueuechanges its automatically computed serialVersionUID. The documented one-time loss of users' saved queue/playback position is avoidable. Preserve compatibility and add an old-version serialized-queue fixture test; a new-version round trip does not test upgrades. - After removing song-ID deduplication,
OnlinePlaylistScreenstill identifies playing and selected rows by song ID. With two occurrences of a song, tapping the second can pause the first, and selecting one selects both. Carry occurrence identity through playback and selection. removeSongFromLocalListparses only the first digit run:1,000 songsbecomes0,000 songsafter one removal. Use a numeric count, or parse/format the whole localized value safely. Widget/alarm items also still omit each row's setVideoId, so the new removal action remains unavailable there.
Please add failure/rollback, upgrade, and duplicate-occurrence integration regressions before merge. The helper tests alone do not exercise these data-loss and wrong-row behaviors.
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>
|
I've rebased onto the latest main and pushed the follow-up fixes. |
Problem
The player menu could not reliably identify the playlist occurrence behind the current song, so
Remove from playlistwas missing or could act on incomplete playlist context.Changes
Review follow-up
MetrolistGroup/Metrolist:mainat876e35518. The PR contains five commits; equivalent upstream commits are excluded.Validation
./gradlew :app:assembleFossDebug :app:testFossDebugUnitTest :innertube:testDebugUnitTestcompleted successfully. App: 296 tests; innertube: 39 tests. Zero failures, errors, or skipped tests.SM-S911N), after rebase at2d4ba68f2: installed the final debug APK and repeated theggplaylist test with two copies of the same song. With Wi-Fi and mobile data disabled, a failed swipe removal restored both rows to their original positions; swiping that same row again issued a second request and restored the rows again. Confirmed two failure callbacks in device logs and no playlist row loss. Network, edit lock, and swipe settings were restored afterward.git diff --checkpasses.Summary by CodeRabbit
New Features
Bug Fixes
Chores
The PR appears safe to merge; all previous findings are addressed and no new actionable issues were found.Summary
The PR carries playlist and occurrence details into playback queues and routes menu removals through
SyncUtils.Reviews (2) · Last reviewed commit: "fix(playlists): preserve queues and reco..." · Reviewed by Greptile