Replace album popup menu with a full action sheet#190
Merged
Conversation
Long-pressing an AlbumCard or AlbumRow now opens an iOS-style action
sheet (modeled after PlayableActionSheet) instead of the cramped
inline popup menu. Structure:
Thumbnail
Album name
Artist name
--------------------------------------
[ Favorite | Play All | Shuffle All ]
--------------------------------------
Play Next
Play Last
--- (only when applicable)
Go to Artist
--- (only when canEdit)
Edit…
- Favorite: state-driven label (Favorite / Undo Favorite); toggles
via the new AlbumProvider.toggleFavorite (POST /favorites/toggle
with { type: 'albums', id }), optimistic with restore on failure.
- Play All / Shuffle All: fetch the album's songs via
PlayableProvider.fetchForAlbum, then audioHandler.replaceQueue
(with shuffle for Shuffle All).
- Play Next / Play Last: fetch + queueAfterCurrent / queueToBottom
per song, with the same overlay messages used by the song-level
action sheet.
- Go to Artist: hidden for Unknown / Various Artists.
- Edit…: only shown when album.canEdit; opens showEditAlbumDialog.
Model:
- Album.favorite (parsed from json['favorite'], default false).
Album.merge propagates it.
Removed lib/ui/widgets/album_actions_menu.dart (the old single-item
popup) and its widgets.dart export — no longer used.
Tests:
- New test/ui/screens/album_action_sheet_test.dart covers the sheet's
structure (thumbnail / names / quick actions / state-driven label /
Edit visibility / Go-to-Artist visibility).
- album_test.dart adds branches for the favorite field
(parsing + merge propagation).
- Drop the old album_card_test long-press group and album_row_test
— they targeted the inline menu that's gone.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR introduces album favoriting by adding a ChangesAlbum Favoriting & Action Sheet
Sequence DiagramsequenceDiagram
participant User
participant AlbumCard as Album Card/Row
participant ActionSheet as Album Action Sheet
participant AlbumProvider as Album Provider
participant API as API Endpoint
User->>AlbumCard: Long-press album
AlbumCard->>ActionSheet: Show album action sheet
ActionSheet->>User: Display actions (Favorite, Play, etc.)
User->>ActionSheet: Tap "Favorite"
ActionSheet->>AlbumProvider: toggleFavorite(album)
AlbumProvider->>AlbumProvider: Flip favorite optimistically
AlbumProvider->>ActionSheet: notifyListeners()
ActionSheet->>User: Update UI (show "Undo Favorite")
AlbumProvider->>API: POST /favorites/toggle
alt Success
API->>AlbumProvider: 200 OK
else Failure
API->>AlbumProvider: Error
AlbumProvider->>AlbumProvider: Revert favorite flag
AlbumProvider->>ActionSheet: notifyListeners()
ActionSheet->>User: Revert UI state
end
User->>ActionSheet: Tap "Play All"
ActionSheet->>AlbumProvider: fetchForAlbum()
AlbumProvider->>API: Fetch playables
API->>AlbumProvider: Tracks list
ActionSheet->>User: Replace queue & play
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
The koel server's FavoriteableType enum uses singular values (album / artist / podcast / radio-station / playable). I sent the plural 'albums', which got rejected as 422 Unprocessable.
Adds 5 new tests grouped under 'actions' that verify each tap behavior end-to-end: - Favorite → AlbumProvider.toggleFavorite(album) - Play All → PlayableProvider.fetchForAlbum + audioHandler.replaceQueue - Shuffle All → fetchForAlbum + replaceQueue with shuffle: true - Play Next → queueAfterCurrent for each song - Play Last → queueToBottom for each song Mirrors the mock setup used by playable_action_sheet_test (MockKoelAudioHandler assigned to app.audioHandler, with mediaItem stub). Note: testing the actual /favorites/toggle request body shape (which is what would have caught the album/albums 422 bug) needs HTTP-layer mocking the codebase doesn't have today; the toggleFavorite test verifies the provider method is called with the right album, which is the most we can pin without that infra.
![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
🧹 Nitpick comments (1)
test/ui/screens/album_action_sheet_test.dart (1)
176-212: ConsiderverifyInOrderto lock down song-queuing sequence forPlay Next.The loop of
verifycalls on lines 190–193 and 208–211 confirms each song is queued exactly once but does not verify call order. ForPlay Next, this matters:queueAfterCurrentinserts atcurrentQueueIndex + 1, so iterating songs forward (song1 → song2 → song3) produces reverse queue order (song3plays first). The current assertion is blind to this behavior; a future regression in iteration order would pass silently.Using
verifyInOrderdocuments the expected call sequence and prevents regressions. ForPlay Last,queueToBottomappends to the end, so forward iteration naturally produces correct order—still worth asserting for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/screens/album_action_sheet_test.dart` around lines 176 - 212, The tests currently verify each song is queued exactly once but not the call order; update the "tapping Play Next queues each song after current" test to use verifyInOrder with a list of expected calls to audioHandlerMock.queueAfterCurrent(...) in the exact sequence produced from songs (use songs[0], songs[1], songs[2]) instead of the per-song loop, and similarly replace the loop in the "tapping Play Last queues each song to the bottom" test with verifyInOrder asserting audioHandlerMock.queueToBottom(...) calls in forward order; keep the existing playableProviderMock.fetchForAlbum(album.id), mount(tester, album), and the pump calls unchanged and retain verifyNever checks for the opposite method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/screens/album_action_sheet.dart`:
- Around line 147-148: The Play Next loop uses audioHandler.queueAfterCurrent
for each item which keeps inserting at the same index and reverses the album;
change the loop in the Play Next action to iterate over songs.reversed (e.g.,
for (final song in songs.reversed) { await audioHandler.queueAfterCurrent(song);
}) so each insertion preserves the original album order; keep Play Last
(queueToBottom) unchanged.
- Around line 145-155: The async onTap handlers for the Play Next and Play Last
buttons perform awaits (e.g., calling _fetchSongs() and
audioHandler.queueAfterCurrent / queueLast) and then call showOverlay(context,
...), which can run after the widget is unmounted; add a mounted guard (if
(!mounted) return;) immediately before each showOverlay call in the
PlayableActionButton onTap handlers to avoid using a disposed context (reference
the _fetchSongs function, audioHandler.queueAfterCurrent / queueLast calls, and
the showOverlay invocation when applying the change).
---
Nitpick comments:
In `@test/ui/screens/album_action_sheet_test.dart`:
- Around line 176-212: The tests currently verify each song is queued exactly
once but not the call order; update the "tapping Play Next queues each song
after current" test to use verifyInOrder with a list of expected calls to
audioHandlerMock.queueAfterCurrent(...) in the exact sequence produced from
songs (use songs[0], songs[1], songs[2]) instead of the per-song loop, and
similarly replace the loop in the "tapping Play Last queues each song to the
bottom" test with verifyInOrder asserting audioHandlerMock.queueToBottom(...)
calls in forward order; keep the existing
playableProviderMock.fetchForAlbum(album.id), mount(tester, album), and the pump
calls unchanged and retain verifyNever checks for the opposite method.
🪄 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
Run ID: ea7d95a5-f8a8-433f-a813-6efbc9f3a336
📒 Files selected for processing (14)
lib/models/album.dartlib/providers/album_provider.dartlib/ui/screens/album_action_sheet.dartlib/ui/screens/albums.dartlib/ui/screens/screens.dartlib/ui/widgets/album_actions_menu.dartlib/ui/widgets/album_card.dartlib/ui/widgets/widgets.darttest/models/album_test.darttest/ui/screens/album_action_sheet_test.darttest/ui/screens/album_action_sheet_test.mocks.darttest/ui/widgets/album_card_test.darttest/ui/widgets/album_card_test.mocks.darttest/ui/widgets/album_row_test.dart
💤 Files with no reviewable changes (3)
- lib/ui/widgets/album_actions_menu.dart
- lib/ui/widgets/widgets.dart
- test/ui/widgets/album_row_test.dart
The favorite/unfavorite endpoint for non-song entities (album,
artist, podcast, radio station) only landed in koel 7.11.0; on older
servers the request would 404. Hide the quick action below that
version so the row collapses to Play All | Shuffle All.
- New Feature.favoriteEntities flag mapped to '7.11.0'.
- AlbumActionSheet checks Feature.favoriteEntities.isSupported() and
conditionally renders the Favorite quick action plus its trailing
divider.
- Tests:
- features_test.dart covers the new flag's at-version /
above-version / below-version / unset branches.
- album_action_sheet_test.dart sets apiVersion=7.11.0 in setUp so
the existing Favorite tests stay green, plus a new branch for
the feature-off case (Favorite hidden, Play All / Shuffle All
still present).
- Play Next iterated songs forward and called queueAfterCurrent on each. Since queueAfterCurrent inserts at a fixed 'after current' index, that landed the album in reverse order in the queue. Iterate songs.reversed so the queue ends up in album order. - Add `if (!mounted) return;` before showOverlay in both Play Next and Play Last paths. The fetchForAlbum call is network-backed so the State could be disposed by the time the await resolves. - Tighten the related tests to use verifyInOrder with the exact expected sequence — locks the reversed-iteration as the right contract for Play Next, forward order for Play Last.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lib/ui/screens/album_action_sheet.dart (2)
151-162:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
mountedguards beforeshowOverlayin both "Play Next" and "Play Last".
PlayableActionButtondismisses the sheet immediately on tap (defaulthideSheetOnTap: true), unmounting_AlbumActionSheetState. By the time theawaits complete andshowOverlay(context, …)is reached,contextis invalid. This was flagged in a previous review and remains unaddressed.🛡️ Proposed fix — add
mountedguard before eachshowOverlaycallfor (final song in songs.reversed) { await audioHandler.queueAfterCurrent(song); } + if (!mounted) return; showOverlay( context, icon: CupertinoIcons.arrow_right_circle_fill, caption: 'Queued', message: 'To be played next.', );for (final song in songs) { await audioHandler.queueToBottom(song); } + if (!mounted) return; showOverlay( context, icon: CupertinoIcons.arrow_down_right_circle_fill, caption: 'Queued', message: 'Queued to bottom.', );Also applies to: 170-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/album_action_sheet.dart` around lines 151 - 162, In _AlbumActionSheetState locate the onTap handlers for the "Play Next" and "Play Last" PlayableActionButton implementations (the blocks that await _fetchSongs() and call audioHandler.queueAfterCurrent(...) and audioHandler.queueEnd(...)), and add a mounted guard before calling showOverlay(context, ...); i.e., after the await loop finishes check if (!mounted) return; (or if (mounted) { showOverlay(...) }) so you don't call showOverlay with an invalid context after the sheet has been dismissed.
153-155:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
Play Nextqueues album songs in reverse order.Each
queueAfterCurrentinserts atcurrentQueueIndex + 1. Successive inserts push earlier songs back, reversing the intended album track order. This was flagged in a previous review and remains unaddressed.🐛 Proposed fix — iterate
songs.reversed- for (final song in songs) { + for (final song in songs.reversed) { await audioHandler.queueAfterCurrent(song); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/album_action_sheet.dart` around lines 153 - 155, The loop that calls audioHandler.queueAfterCurrent(song) is inserting each song at currentQueueIndex + 1 so iterating songs in forward order reverses the album; change the iteration to append in original order by iterating songs.reversed (or otherwise ensure you insert from last to first) when calling queueAfterCurrent on audioHandler so tracks keep their intended album order; update the loop that references songs and queueAfterCurrent accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/ui/screens/album_action_sheet.dart`:
- Around line 151-162: In _AlbumActionSheetState locate the onTap handlers for
the "Play Next" and "Play Last" PlayableActionButton implementations (the blocks
that await _fetchSongs() and call audioHandler.queueAfterCurrent(...) and
audioHandler.queueEnd(...)), and add a mounted guard before calling
showOverlay(context, ...); i.e., after the await loop finishes check if
(!mounted) return; (or if (mounted) { showOverlay(...) }) so you don't call
showOverlay with an invalid context after the sheet has been dismissed.
- Around line 153-155: The loop that calls audioHandler.queueAfterCurrent(song)
is inserting each song at currentQueueIndex + 1 so iterating songs in forward
order reverses the album; change the iteration to append in original order by
iterating songs.reversed (or otherwise ensure you insert from last to first)
when calling queueAfterCurrent on audioHandler so tracks keep their intended
album order; update the loop that references songs and queueAfterCurrent
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5831c88-88f6-42d0-85d1-445703618959
📒 Files selected for processing (4)
lib/ui/screens/album_action_sheet.dartlib/utils/features.darttest/ui/screens/album_action_sheet_test.darttest/utils/features_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ui/screens/album_action_sheet_test.dart
This was referenced May 2, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Long-pressing an
AlbumCardorAlbumRownow opens an iOS-style action sheet (modeled afterPlayableActionSheet) instead of the cramped single-item popup menu added in #184.Behavior
Favorite↔Undo Favorite); toggles via the newAlbumProvider.toggleFavorite(POST /favorites/togglewith{ type: 'albums', id }), optimistic with rollback on failure.PlayableProvider.fetchForAlbum, thenaudioHandler.replaceQueue(songs, shuffle: …).queueAfterCurrent/queueToBottomper song, same overlay copy as the song-level action sheet.album.canEdit; opens the existingshowEditAlbumDialog.Model
Album.favorite(parsed fromjson['favorite'], defaultsfalse);Album.mergepropagates it so synced instances stay in sync.Removed
lib/ui/widgets/album_actions_menu.dart(the old popup) and itswidgets.dartexport. The popup is replaced wholesale by the action sheet.Test plan
flutter test— full suite green (276/276)test/ui/screens/album_action_sheet_test.dartcovers the sheet's structure (album name / artist name / quick actions / state-driven Favorite label / Edit visibility / Go-to-Artist visibility for Unknown / Various / normal artists)album_test.dartadds branches forfavoriteparsing + merge propagationflutter analyzeclean on touched files (the 3 pre-existing warnings inalbums.dartare out of scope)Summary by CodeRabbit
New Features
Tests