Add swipe-right-to-queue across all list screens#129
Merged
Conversation
phanan
force-pushed
the
fix/always-allow-swipe-to-queue
branch
from
689f7a1 to
edf1617
Compare
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughSliver list now always builds a Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (Dismissible)
participant Provider as PlayableProvider
participant Audio as audioHandler
participant Overlay as Overlay
UI->>UI: User swipes item (start-to-end)
UI->>Provider: fetchForAlbum/Artist(id)
Provider-->>UI: List<Song>
alt songs returned
UI->>Audio: queueToBottom(song) [for each song]
Audio-->>UI: ack
UI->>Overlay: showOverlay("Queued")
Overlay-->>UI: shown
else no songs
UI-->>UI: no overlay
end
UI-->>UI: confirmDismiss returns false (row retained)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
phanan
force-pushed
the
fix/always-allow-swipe-to-queue
branch
from
edf1617 to
b54fbd6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/ui/widgets/sliver_playable_list.dart (1)
53-56: Nitpick: Padding widget can be fully const.Since both
paddingandchildare const, the entirePaddingwidget can be const-constructed for a minor optimization.♻️ Suggested change
- child: Padding( - padding: const EdgeInsets.only(left: 28), - child: const Icon(CupertinoIcons.text_badge_plus), - ), + child: const Padding( + padding: EdgeInsets.only(left: 28), + child: Icon(CupertinoIcons.text_badge_plus), + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/sliver_playable_list.dart` around lines 53 - 56, The Padding widget wrapping the Icon can be const-constructed for a minor optimization: update the Padding instantiation (inside the SliverPlayableList build where you have Padding(padding: const EdgeInsets.only(left: 28), child: const Icon(...))) to use a const constructor (const Padding(...)) so the entire widget subtree becomes compile-time constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/ui/widgets/sliver_playable_list.dart`:
- Around line 53-56: The Padding widget wrapping the Icon can be
const-constructed for a minor optimization: update the Padding instantiation
(inside the SliverPlayableList build where you have Padding(padding: const
EdgeInsets.only(left: 28), child: const Icon(...))) to use a const constructor
(const Padding(...)) so the entire widget subtree becomes compile-time constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b99118eb-be8e-47f4-936e-416639614750
📒 Files selected for processing (1)
lib/ui/widgets/sliver_playable_list.dart
phanan
force-pushed
the
fix/always-allow-swipe-to-queue
branch
2 times, most recently
from
6b231d9 to
ac83fff
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/ui/screens/artists.dart (1)
151-178: Consider extracting the swipe-to-queue Dismissible into a reusable widget.The
Dismissiblewrapper with the fetch-and-queue logic is nearly identical betweenalbums.dartandartists.dart(and presumably could be used elsewhere). Extracting this into a shared widget (e.g.,SwipeToQueueDismissible) would reduce duplication and centralize the error handling fix.Example signature:
class SwipeToQueueDismissible extends StatelessWidget { final Key dismissibleKey; final Future<List<Song>> Function() fetchSongs; final Widget child; // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/artists.dart` around lines 151 - 178, Extract the repeated swipe-to-queue Dismissible into a reusable widget (e.g., SwipeToQueueDismissible) and replace the inline Dismissible in artists.dart and albums.dart with it; the new widget should accept a Key (dismissibleKey), a fetchSongs callback (Future<List<Song>> Function()), and a child Widget, perform the same confirmDismiss logic (call context.read<PlayableProvider>().fetchForArtist or the passed fetchSongs), iterate and call audioHandler.queueToBottom for each song, call showOverlay(context, caption: 'Queued') when songs were queued, and return false; move all related UI config (direction, background alignment/icon/color) into the new widget so ArtistRow and AlbumRow are wrapped cleanly by SwipeToQueueDismissible.
🤖 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/albums.dart`:
- Around line 153-164: Wrap the async work in confirmDismiss (the call to
PlayableProvider.fetchForAlbum and the loop calling audioHandler.queueToBottom)
in a try/catch and handle errors (log or show a user-facing message) so
exceptions don't propagate; after each await (or before calling showOverlay)
verify the widget is still mounted (use the State.mounted flag or bail out)
before using context to call showOverlay, and ensure confirmDismiss returns a
safe boolean in all code paths (e.g., return false on error or unmounted).
Target symbols: confirmDismiss, PlayableProvider.fetchForAlbum,
audioHandler.queueToBottom, showOverlay, and mounted/context checks.
In `@lib/ui/screens/artists.dart`:
- Around line 153-163: In the confirmDismiss async closure, wrap the await calls
to PlayableProvider.fetchForArtist and audioHandler.queueToBottom in a try-catch
to handle thrown exceptions, and after each await sequence check that the widget
is still mounted before using context (e.g., before calling showOverlay or
returning); specifically update the confirmDismiss implementation (the closure
that calls fetchForArtist, iterates songs calling queueToBottom, and calls
showOverlay) to catch and handle errors (log or show an error overlay) and to
guard any context usage with a mounted check so you never use context after the
async operations if the widget was disposed.
---
Nitpick comments:
In `@lib/ui/screens/artists.dart`:
- Around line 151-178: Extract the repeated swipe-to-queue Dismissible into a
reusable widget (e.g., SwipeToQueueDismissible) and replace the inline
Dismissible in artists.dart and albums.dart with it; the new widget should
accept a Key (dismissibleKey), a fetchSongs callback (Future<List<Song>>
Function()), and a child Widget, perform the same confirmDismiss logic (call
context.read<PlayableProvider>().fetchForArtist or the passed fetchSongs),
iterate and call audioHandler.queueToBottom for each song, call
showOverlay(context, caption: 'Queued') when songs were queued, and return
false; move all related UI config (direction, background alignment/icon/color)
into the new widget so ArtistRow and AlbumRow are wrapped cleanly by
SwipeToQueueDismissible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f7b032d-682a-4417-9b62-39139846ccaa
📒 Files selected for processing (4)
lib/ui/screens/albums.dartlib/ui/screens/artists.dartlib/ui/screens/playlists.dartlib/ui/widgets/sliver_playable_list.dart
✅ Files skipped from review due to trivial changes (1)
- lib/ui/screens/playlists.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ui/widgets/sliver_playable_list.dart
Comment on lines
+153
to
+164
| confirmDismiss: (_) async { | ||
| final songs = await context | ||
| .read<PlayableProvider>() | ||
| .fetchForAlbum(album.id); | ||
| if (songs.isNotEmpty) { | ||
| for (final song in songs) { | ||
| await audioHandler.queueToBottom(song); | ||
| } | ||
| showOverlay(context, caption: 'Queued'); | ||
| } | ||
| return false; | ||
| }, |
There was a problem hiding this comment.
Add error handling and verify context is still valid after async operations.
The confirmDismiss callback has two concerns:
-
Missing error handling: If
fetchForAlbumorqueueToBottomthrows, the exception propagates uncaught, which could cause unexpected UI behavior or crashes. -
Context after async gap: Using
contextto show the overlay after awaiting network/queue operations is risky—if the widget unmounts during the fetch, this can throw.
🛡️ Proposed fix with error handling and mounted check
confirmDismiss: (_) async {
+ try {
final songs = await context
.read<PlayableProvider>()
.fetchForAlbum(album.id);
if (songs.isNotEmpty) {
for (final song in songs) {
await audioHandler.queueToBottom(song);
}
- showOverlay(context, caption: 'Queued');
+ if (context.mounted) {
+ showOverlay(context, caption: 'Queued');
+ }
}
+ } catch (e) {
+ // Optionally log or show error feedback
+ }
return false;
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| confirmDismiss: (_) async { | |
| final songs = await context | |
| .read<PlayableProvider>() | |
| .fetchForAlbum(album.id); | |
| if (songs.isNotEmpty) { | |
| for (final song in songs) { | |
| await audioHandler.queueToBottom(song); | |
| } | |
| showOverlay(context, caption: 'Queued'); | |
| } | |
| return false; | |
| }, | |
| confirmDismiss: (_) async { | |
| try { | |
| final songs = await context | |
| .read<PlayableProvider>() | |
| .fetchForAlbum(album.id); | |
| if (songs.isNotEmpty) { | |
| for (final song in songs) { | |
| await audioHandler.queueToBottom(song); | |
| } | |
| if (context.mounted) { | |
| showOverlay(context, caption: 'Queued'); | |
| } | |
| } | |
| } catch (e) { | |
| // Optionally log or show error feedback | |
| } | |
| return false; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/screens/albums.dart` around lines 153 - 164, Wrap the async work in
confirmDismiss (the call to PlayableProvider.fetchForAlbum and the loop calling
audioHandler.queueToBottom) in a try/catch and handle errors (log or show a
user-facing message) so exceptions don't propagate; after each await (or before
calling showOverlay) verify the widget is still mounted (use the State.mounted
flag or bail out) before using context to call showOverlay, and ensure
confirmDismiss returns a safe boolean in all code paths (e.g., return false on
error or unmounted). Target symbols: confirmDismiss,
PlayableProvider.fetchForAlbum, audioHandler.queueToBottom, showOverlay, and
mounted/context checks.
Comment on lines
+153
to
+163
| confirmDismiss: (_) async { | ||
| final songs = await context | ||
| .read<PlayableProvider>() | ||
| .fetchForArtist(artist.id); | ||
| if (songs.isNotEmpty) { | ||
| for (final song in songs) { | ||
| await audioHandler.queueToBottom(song); | ||
| } | ||
| showOverlay(context, caption: 'Queued'); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Same concerns as albums.dart: add error handling and mounted check.
This confirmDismiss implementation has the same issues flagged in albums.dart:
- No error handling if
fetchForArtistorqueueToBottomthrows - Using
contextafter async operations without verifying the widget is still mounted
🛡️ Proposed fix
confirmDismiss: (_) async {
+ try {
final songs = await context
.read<PlayableProvider>()
.fetchForArtist(artist.id);
if (songs.isNotEmpty) {
for (final song in songs) {
await audioHandler.queueToBottom(song);
}
- showOverlay(context, caption: 'Queued');
+ if (context.mounted) {
+ showOverlay(context, caption: 'Queued');
+ }
}
+ } catch (e) {
+ // Optionally log or show error feedback
+ }
return false;
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/screens/artists.dart` around lines 153 - 163, In the confirmDismiss
async closure, wrap the await calls to PlayableProvider.fetchForArtist and
audioHandler.queueToBottom in a try-catch to handle thrown exceptions, and after
each await sequence check that the widget is still mounted before using context
(e.g., before calling showOverlay or returning); specifically update the
confirmDismiss implementation (the closure that calls fetchForArtist, iterates
songs calling queueToBottom, and calls showOverlay) to catch and handle errors
(log or show an error overlay) and to guard any context usage with a mounted
check so you never use context after the async operations if the widget was
disposed.
SliverPlayableList only wrapped rows in Dismissible when onDismissed was set, making swipe-right-to-queue unavailable on screens without swipe-to-delete (e.g. artist details, album details). Now all rows are always wrapped — swipe right to queue is always available.
phanan
force-pushed
the
fix/always-allow-swipe-to-queue
branch
from
ac83fff to
00be205
Compare
phanan
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/ui/widgets/sliver_playable_list_test.dart`:
- Around line 8-30: Change the unit tests to widget tests that build a
SliverPlayableList with a non-empty playables list (use Song.fake() for test
data) so the Dismissible is actually created; in tests for queue-only mode
(PlayableListContext.other with onDismissed null) pump the widget via
testWidgets, locate the Dismissible inside SliverPlayableList and assert its
Dismissible.direction == DismissDirection.startToEnd; in tests for bidirectional
mode (PlayableListContext.favorites with a non-null onDismissed) assert
Dismissible.direction == DismissDirection.horizontal, then simulate swipes via
WidgetTester.drag: verify that an endToStart swipe triggers the onDismissed
callback and that a startToEnd swipe performs the enqueue behavior (e.g., check
queue state or related side-effect) rather than calling onDismissed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b69c922-99cf-47a1-9919-f49af92b9050
📒 Files selected for processing (5)
lib/ui/screens/albums.dartlib/ui/screens/artists.dartlib/ui/screens/playlists.dartlib/ui/widgets/sliver_playable_list.darttest/ui/widgets/sliver_playable_list_test.dart
✅ Files skipped from review due to trivial changes (1)
- lib/ui/screens/playlists.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ui/widgets/sliver_playable_list.dart
Comment on lines
+8
to
+30
| test('accepts onDismissed as null for queue-only mode', () { | ||
| // Verify that SliverPlayableList can be constructed without onDismissed | ||
| // and still supports swipe-to-queue (startToEnd direction) | ||
| final widget = SliverPlayableList( | ||
| playables: [], | ||
| listContext: PlayableListContext.other, | ||
| ); | ||
|
|
||
| expect(widget.onDismissed, isNull); | ||
| expect(widget.playables, isEmpty); | ||
| }); | ||
|
|
||
| test('accepts onDismissed for bidirectional swipe', () { | ||
| var dismissed = false; | ||
|
|
||
| final widget = SliverPlayableList( | ||
| playables: [], | ||
| listContext: PlayableListContext.favorites, | ||
| onDismissed: (_) => dismissed = true, | ||
| ); | ||
|
|
||
| expect(widget.onDismissed, isNotNull); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify implementation contract in widget code and current test coverage gaps.
# 1) Inspect SliverPlayableList dismissal wiring.
rg -n -C3 'Dismissible|DismissDirection|onDismissed|startToEnd|endToStart' lib/ui/widgets/sliver_playable_list.dart
# 2) Inspect this test file for widget tests vs constructor-only tests.
rg -n -C2 "group\\('SliverPlayableList'|testWidgets\\(|test\\(" test/ui/widgets/sliver_playable_list_test.dart
# 3) Discover existing playable test fixtures/fakes to build non-empty rows.
rg -n -C2 'Fake.*Playable|Mock.*Playable|Playable.*fixture|build.*Playable' testRepository: koel/player
Length of output: 3552
🏁 Script executed:
#!/bin/bash
# Check for Playable construction patterns and widget test examples
# 1) Search for Playable class definition and constructors
fd -e dart lib -o test | xargs grep -l "class Playable" | head -3
# 2) Look for any factory methods or builders in test files
rg -n "Playable.*=" test/ui/widgets/sliver_playable_list_test.dart
# 3) Check if there are any other widget tests in this directory for reference patterns
fd "sliver_playable_list_test.dart|*_test.dart" test/ui/widgets/ | head -5
# 4) Search for testWidgets usage in the test directory
rg -c "testWidgets" test/ui/ | grep -v ":0$" | head -5Repository: koel/player
Length of output: 773
🏁 Script executed:
#!/bin/bash
# Check the test file for imports and existing widget test patterns
head -50 test/ui/widgets/sliver_playable_list_test.dartRepository: koel/player
Length of output: 1232
🏁 Script executed:
#!/bin/bash
# Look at existing widget tests and Playable usage patterns
# Check another widget test file for testWidgets patterns
head -80 test/ui/widgets/song_cache_icon_test.dart
# Check for Playable implementation to understand construction
fd "playable.dart" lib --type f
# Look for Playable mock or fake in test files
rg -n "class.*Playable|Playable()" test/ -A 3 | head -50Repository: koel/player
Length of output: 6134
🏁 Script executed:
#!/bin/bash
# Check Playable class definition and Song relationship
cat lib/models/playable.dart
# Also check if Song extends Playable
rg "class Song|extends Playable" lib/models/ -A 2Repository: koel/player
Length of output: 1950
🏁 Script executed:
#!/bin/bash
# Check if Song has a .fake() factory/constructor
rg -n "\.fake\(|factory.*fake" lib/models/song.dart -A 2
# Also look at the actual Dismissible behavior in the widget to understand what would be tested
cat -n lib/ui/widgets/sliver_playable_list.dart | sed -n '24,45p'Repository: koel/player
Length of output: 1257
Behavioral contract is not tested; only constructor fields are asserted.
These tests use test() rather than testWidgets() and create playables: [], so the Dismissible widget is never built and swiped. This misses regressions in the conditional direction/callback logic:
- Line 30-32: direction is
DismissDirection.horizontalwhenonDismissed != null, elseDismissDirection.startToEnd - Line 34-36: callback fires only for
endToStartdirection; other directions (or queue-onlystartToEnd) trigger queuing instead
Rewrite as testWidgets() with non-empty playables (use Song.fake() for test data) and verify:
- queue-only mode:
Dismissible.direction == DismissDirection.startToEndwhenonDismissed == null - bidirectional mode:
Dismissible.direction == DismissDirection.horizontalwhenonDismissed != null - callback fires only for
endToStartswipe;startToEndswipe enqueues instead
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ui/widgets/sliver_playable_list_test.dart` around lines 8 - 30, Change
the unit tests to widget tests that build a SliverPlayableList with a non-empty
playables list (use Song.fake() for test data) so the Dismissible is actually
created; in tests for queue-only mode (PlayableListContext.other with
onDismissed null) pump the widget via testWidgets, locate the Dismissible inside
SliverPlayableList and assert its Dismissible.direction ==
DismissDirection.startToEnd; in tests for bidirectional mode
(PlayableListContext.favorites with a non-null onDismissed) assert
Dismissible.direction == DismissDirection.horizontal, then simulate swipes via
WidgetTester.drag: verify that an endToStart swipe triggers the onDismissed
callback and that a startToEnd swipe performs the enqueue behavior (e.g., check
queue state or related side-effect) rather than calling onDismissed.
This was referenced Mar 28, 2026
6 tasks
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
SwipeToQueueDismissiblewidget withfetchSongscallback, try/catch error handling, andmountedguards before context usageSliverPlayableListnow always wraps rows inDismissible— swipe right to queue is available even without swipe-to-deleteAppColors.highlighttoColors.greenLucideIcons.listPluswithCupertinoIcons.text_badge_plusTest plan