fix: IndexOutOfBoundsException in Instant Mix for single-album artists - #877
fix: IndexOutOfBoundsException in Instant Mix for single-album artists#877sinful1992 wants to merge 2 commits into
Conversation
…m artists fetchNextTrack alternates between albums[0] and albums[1], so extending an Instant Mix for an artist with only one album crashed with IndexOutOfBoundsException (Index 1 out of bounds for length 1). The album index now falls back to 0 when it exceeds the list size. Also bounds the recursion with an attempt budget (4x the requested track count): when an artist has fewer unique tracks than requested, every candidate is eventually a duplicate and the previous code kept issuing network calls forever without terminating. Whatever was collected when the budget runs out is enqueued, and an empty artist no longer enqueues an empty list.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesInstant mix safety
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
🧹 Nitpick comments (1)
app/src/tempus/java/com/cappielloantonio/tempo/repository/InstantMixBuilder.java (1)
88-88: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider caching album tracks to avoid redundant network calls.
Each
fetchNextTrackinvocation re-fetches the full album viagetAlbum(album.getId()), even though the same album may be fetched dozens of times within the attempt budget. For a 20-track mix with 2 albums, this can produce up to 80 network calls fetching the same 2 albums repeatedly. Caching the songs list per album ID after the first fetch would eliminate the redundant I/O while preserving the existing shuffle/pick logic.🤖 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/tempus/java/com/cappielloantonio/tempo/repository/InstantMixBuilder.java` at line 88, Update the InstantMixBuilder flow around fetchNextTrack to cache each album’s fetched tracks by album ID after the first getAlbum call. Reuse the cached songs list on subsequent selections while preserving the existing shuffle, track-picking, and attempt-budget behavior.
🤖 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/tempus/java/com/cappielloantonio/tempo/repository/InstantMixBuilder.java`:
- Around line 82-90: Keep isRunning true until the asynchronous mix-building
operation completes: remove the immediate reset after fetchNextTrack in
buildAndEnqueue, and reset it in fetchNextTrack’s terminal completion path and
every early-return path, including failure or exhaustion cases. Ensure all paths
reset the flag exactly once so subsequent builds can start only after the
current work has finished.
---
Nitpick comments:
In
`@app/src/tempus/java/com/cappielloantonio/tempo/repository/InstantMixBuilder.java`:
- Line 88: Update the InstantMixBuilder flow around fetchNextTrack to cache each
album’s fetched tracks by album ID after the first getAlbum call. Reuse the
cached songs list on subsequent selections while preserving the existing
shuffle, track-picking, and attempt-budget behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc5c3008-2241-4147-8e0e-4c0249171358
📒 Files selected for processing (1)
app/src/tempus/java/com/cappielloantonio/tempo/repository/InstantMixBuilder.java
The flag was cleared right after kicking off the async fetchNextTrack chain, so it was false for the whole build and the concurrency guard in buildAndEnqueue never prevented overlapping builds. It is now cleared in the termination branch of fetchNextTrack (both the enqueue and the nothing-collected paths); every recursion path ends there because both Retrofit callbacks recurse with a decremented attempt budget.
|
What caused the crash you had? instantMixBuilder can and must only be called from Android Auto. https://github.com/eddyizm/tempus/blob/main/USAGE.md This PR adds no value. |
|
Fair question — the crash didn't come from the browse path, and you're right that I hit it while testing #868: there a voice query that resolves to an artist goes straight into Independent of the crash, the attempt budget fixes something reachable through the gated path today: Happy to also add the album-count check on the voice side in #868 if you'd prefer both layers. |
|
Okay, I understand what you've done. If you look at the The process works as follows:
I think it would be good to implement the same process in #868. edit: perhaps a fallback for an artist with only one album would be to add it to queue at random? |
2 participants
Problem
Extending an Instant Mix for an artist that has only one album crashes the app:
fetchNextTrackalternates betweenalbums.get(0)andalbums.get(1); with a single-album artist the second step indexes past the end of the list. Hit in real use on a library where the mixed artist has exactly one album.Fix
0when it exceeds the album list size, so single-album artists keep drawing tracks from their only album.getAlbumnetwork calls forever. Now it enqueues whatever it collected once the budget is spent.Summary by CodeRabbit