Fix manual queue priority and FIFO order during Shuffle#3712
Fix manual queue priority and FIFO order during Shuffle#3712fra06083 wants to merge 5 commits intoMetrolistGroup:mainfrom
Conversation
… queue then do a shuffle in playlist
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a manual-queue tag and a forward-scanned ChangesManual queue accounting and insertion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 3
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)
1721-1725:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor:
playNextwas not updated, contradicting the PR description and breaking FIFO when mixed withaddToQueue.The PR description states "Updated addToQueue and playNext to insert at index currentIndex + manualQueueCount + 1 to preserve FIFO order", but
playNextstill inserts atcurrentIndex + 1and never incrementsmanualQueueCount. Two consequences:
playNextafteraddToQueueoverwrites the position of items thataddToQueueplaced atcurrentIndex + manualQueueCount + 1, producing the LIFO behavior the PR is trying to fix.- Items inserted via
playNextare invisible tomanualQueueCount, so a subsequentaddToQueue(in repeat mode) will insert into the playNext-added range instead of after it.
playNextshould also usecurrentIndex + manualQueueCount + 1and incrementmanualQueueCount, otherwise the two entry points are not coherent.🤖 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` around lines 1721 - 1725, The playNext implementation currently inserts at player.currentMediaItemIndex + 1 and does not update manualQueueCount, causing FIFO to break with addToQueue; modify playNext to compute insertIndex as player.currentMediaItemIndex + manualQueueCount + 1 (matching addToQueue) and after inserting via player.addMediaItems(...) increment manualQueueCount by the number of items inserted so manualQueueCount remains consistent; ensure this logic applies whether shuffleModeEnabled is true or false and reference the playNext method, manualQueueCount field, and player.addMediaItems/player.currentMediaItemIndex when making the change.
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
1832-1858: ⚡ Quick winRemove unused variables and use idiomatic Kotlin Random API.
allIndices(line 1837) andfixedIndices(line 1842) are created but never used.- Replace
java.util.Random()withkotlin.random.Random(already imported at line 231), which is more idiomatic and consistent with Kotlin conventions.- Remove comments that restate loop logic ("Put first indices in linear order", "Rest of playlist is shuffled after the queue"), per the guideline that comments should highlight non-obvious logic only.
♻️ Proposed cleanup
if (player.shuffleModeEnabled) { val totalItems = player.mediaItemCount - val random = java.util.Random() - - // Create a valid permutation of ALL indices - val allIndices = (0 until totalItems).toMutableList() - - // Remove indices that must have FIXED position: - // [Passed] + [Current] + [Manual Queue] val fixedPathCount = currentIndex + manualQueueCount + 1 - val fixedIndices = (0 until fixedPathCount).toList() - val flexibleIndices = (fixedPathCount until totalItems).toMutableList() - flexibleIndices.shuffle(random) + flexibleIndices.shuffle() val finalShuffleOrder = IntArray(totalItems) - // Put first indices in linear order (Absolute priority) for (i in 0 until fixedPathCount) { finalShuffleOrder[i] = i } - // Rest of playlist is shuffled after the queue for (i in flexibleIndices.indices) { finalShuffleOrder[fixedPathCount + i] = flexibleIndices[i] } - player.setShuffleOrder(DefaultShuffleOrder(finalShuffleOrder, random.nextLong())) + player.setShuffleOrder(DefaultShuffleOrder(finalShuffleOrder, Random.nextLong())) }🤖 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` around lines 1832 - 1858, Remove the unused local variables allIndices and fixedIndices from the shuffle block and switch java.util.Random() to the idiomatic kotlin.random.Random (already imported) when constructing the shuffle seed; keep building finalShuffleOrder and calling player.setShuffleOrder(DefaultShuffleOrder(...)) unchanged, and delete the redundant explanatory comments ("Put first indices..." and "Rest of playlist...") so only non-obvious logic remains documented.
🤖 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/MusicService.kt`:
- Around line 2286-2289: The manualQueueCount should not be blindly decremented
on Player.MEDIA_ITEM_TRANSITION_REASON_SEEK because seeks can cross zero or many
manual items and will drift the counter; instead, stop decrementing on
MEDIA_ITEM_TRANSITION_REASON_SEEK and implement marking of manual-inserted items
via a MediaMetadata.extras flag (e.g., IS_MANUAL_QUEUE) when adding items in
addToQueue, then on item transitions (in the transition handler that currently
references manualQueueCount and the reason) recompute manualQueueCount by
scanning the player's timeline/queue for entries with IS_MANUAL_QUEUE and use
that accurate count to drive the insert index logic in addToQueue and the
shuffle fixedPathCount logic.
- Around line 412-413: The field addedQueueSize is only ever assigned (reset in
reset logic and incremented elsewhere) and never read; either remove
addedQueueSize entirely or wire it into the queue-consumer logic: if it's
intended to track programmatic (non-manual) additions, document that distinction
and use addedQueueSize alongside manualQueueCount where queue size/metrics are
computed (e.g., in getQueueSize(), queue summary methods, or logging sent when
enqueuing/dequeuing) so callers read the value; otherwise delete the
addedQueueSize declaration and its reset/increment references to avoid dead
state.
- Around line 1805-1858: The code appends manual items to the end when repeat is
OFF, but the shuffle priority logic still assumes the manual items are placed
immediately after the current index; fix by inserting the new items at the
computed insertIndex in both cases (use player.addMediaItems(insertIndex, items)
instead of player.addMediaItems(items) when isRepeatActive is false) so the
manual items occupy the contiguous positions the shuffle logic expects; ensure
fixedPathCount is computed from currentIndex + manualQueueCount + 1 after
manualQueueCount is updated so the shuffle's fixed indices reflect the actual
positions of the newly inserted manual items (refer to insertIndex,
isRepeatActive, player.addMediaItems, manualQueueCount, and fixedPathCount).
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1721-1725: The playNext implementation currently inserts at
player.currentMediaItemIndex + 1 and does not update manualQueueCount, causing
FIFO to break with addToQueue; modify playNext to compute insertIndex as
player.currentMediaItemIndex + manualQueueCount + 1 (matching addToQueue) and
after inserting via player.addMediaItems(...) increment manualQueueCount by the
number of items inserted so manualQueueCount remains consistent; ensure this
logic applies whether shuffleModeEnabled is true or false and reference the
playNext method, manualQueueCount field, and
player.addMediaItems/player.currentMediaItemIndex when making the change.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1832-1858: Remove the unused local variables allIndices and
fixedIndices from the shuffle block and switch java.util.Random() to the
idiomatic kotlin.random.Random (already imported) when constructing the shuffle
seed; keep building finalShuffleOrder and calling
player.setShuffleOrder(DefaultShuffleOrder(...)) unchanged, and delete the
redundant explanatory comments ("Put first indices..." and "Rest of
playlist...") so only non-obvious logic remains documented.
🪄 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: 09749ddf-1fe1-470a-a54f-58df09794220
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1816-1857:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecompute
manualQueueCountafter duplicate removal.With duplicate prevention enabled, this block can delete an already-queued manual item ahead of the current song, but
manualQueueCountis still used as if that item were still present. The very nextinsertIndex/fixedPathCountcalculation can therefore misplace the new items until a later transition or seek rescans the queue.Suggested fix
indicesToRemove.sortedDescending().forEach { index -> player.removeMediaItem(index) } + manualQueueCount = recomputeManualQueueCount() }🤖 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` around lines 1816 - 1857, Duplicate removal can remove manual queue entries but manualQueueCount is not updated before computing insertIndex; after removing items (indicesToRemove.forEach { player.removeMediaItem(it) }) recompute or adjust manualQueueCount — e.g. decrement manualQueueCount by the number of removed indices that were ahead of the current item (manualQueueCount -= indicesToRemove.count { it > currentIndex }) or, if available, fully recompute by scanning player items ahead of player.currentMediaItemIndex to count manual entries; ensure this adjustment happens before using manualQueueCount to compute insertIndex and before adding stampedItems.
♻️ Duplicate comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
1841-1880:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRepeat-off still breaks manual priority under shuffle.
When
repeatMode == REPEAT_MODE_OFF, Lines 1841-1854 append the new items to the tail, but Lines 1863-1878 still build the fixed shuffle prefix as if those items were contiguous right aftercurrentIndex. In the common shuffle-on/repeat-off path, the old upcoming tracks stay ahead of the manual additions and the new items get randomized into the tail again.🤖 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` around lines 1841 - 1880, The shuffle prefix calculation assumes manual items are contiguous after currentIndex; fix it by basing the fixedPathCount on the actual insertion position (insertIndex) and the previous manualQueueCount rather than the post-incremented manualQueueCount; capture previousManualCount = manualQueueCount before adding stampedItems (or postpone manualQueueCount += stampedItems.size until after computing shuffle), then compute fixedPathCount = if (isRepeatActive use currentIndex + previousManualCount + 1 else use insertIndex + previousManualCount) and use that when building flexibleIndices and finalShuffleOrder before calling player.setShuffleOrder.
🤖 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/MusicService.kt`:
- Around line 1786-1798: recomputeManualQueueCount() only counts items with
IS_MANUAL_QUEUE set, but playNext() inserts items without that flag so they are
invisible to the counter; refactor so both addToQueue() and playNext() use a
shared insertion path (e.g., extract an insertManualQueueItem() helper) that
sets the IS_MANUAL_QUEUE metadata and updates any manual-queue bookkeeping, and
ensure recomputeManualQueueCount() continues to check the IS_MANUAL_QUEUE flag
on upcoming media items so items inserted by playNext() are counted the same as
addToQueue() items.
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1816-1857: Duplicate removal can remove manual queue entries but
manualQueueCount is not updated before computing insertIndex; after removing
items (indicesToRemove.forEach { player.removeMediaItem(it) }) recompute or
adjust manualQueueCount — e.g. decrement manualQueueCount by the number of
removed indices that were ahead of the current item (manualQueueCount -=
indicesToRemove.count { it > currentIndex }) or, if available, fully recompute
by scanning player items ahead of player.currentMediaItemIndex to count manual
entries; ensure this adjustment happens before using manualQueueCount to compute
insertIndex and before adding stampedItems.
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1841-1880: The shuffle prefix calculation assumes manual items are
contiguous after currentIndex; fix it by basing the fixedPathCount on the actual
insertion position (insertIndex) and the previous manualQueueCount rather than
the post-incremented manualQueueCount; capture previousManualCount =
manualQueueCount before adding stampedItems (or postpone manualQueueCount +=
stampedItems.size until after computing shuffle), then compute fixedPathCount =
if (isRepeatActive use currentIndex + previousManualCount + 1 else use
insertIndex + previousManualCount) and use that when building flexibleIndices
and finalShuffleOrder before calling player.setShuffleOrder.
🪄 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: 49ba8bd3-9aac-46a1-a71e-5630977dcf0a
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
I think the logic is not wrong when the button is supposed to literally "play next", it's likely intended behavior |
|
Oh... I get that the current way might be intended, but I think it would be a nice change because: Right now, 'Add to Queue' is fine for just building a long list, but 'Play Next' should really be a 'skip the line' feature. If I’m halfway through a playlist and I see a couple of tracks I want them to play immediately after the current song, not buried under everything else I added manually before. (Play next wasn't working properply) The old logic was kind of a struggle because it didn't really handle multiple songs well, and it felt like the queue was just too stiff. With this new approach, the whole thing feels way more agile and responsive. You can basically stack a few 'Play Next' songs on the fly without ruining the rest of your queue. In my opinion, it’s a massive improvement for usability.. (Or It would be a nice choice to add an option to change queue logic) |
2 participants
Problem
The manual queue ("Add to Queue" / "Play Next") suffered from two main logical issues:
LIFO behavior: Manually adding multiple songs resulted in an inverted playback order (the last song added was played first and it might be wrong).
Shuffle Priority Loss: When Shuffle mode was active, manually added songs were treated as part of the general pool, often being ignored or placed at the end of the shuffled timeline instead of being prioritized as the next tracks to play.
Cause:
The causes were:
The insertion logic was adding items at a fixed index (currentIndex + 1) without tracking previous manual additions, causing a "stack" (LIFO) effect.
The ShuffleOrder was being fully randomized or improperly rebuilt upon adding items, which broke the expected "sequential path" for manually queued songs.
Solution
Implemented a priority queue logic:
Manual Queue Tracking: Introduced manualQueueCount to track the number of priority items currently in the queue.
FIFO Logic: Updated addToQueue and playNext to calculate the insertion index as currentIndex + manualQueueCount + 1, ensuring the order of addition is preserved.
Shuffle Order Refinement: Modified the DefaultShuffleOrder reconstruction. Now, the player maintains a sequential path from the current song through the entire manual queue, while the remaining playlist items are shuffled behind them.
State Management: Integrated the counter decrement in onMediaItemTransition and reset it in playQueue to ensure consistency.
Testing:
Summary by CodeRabbit
New Features
Bug Fixes