Replace artist popup menu with a full action sheet#191
Merged
Conversation
Mirrors the album action sheet (#190) for artists: a Music-style sheet with thumbnail, name, the Favorite/Play All/Shuffle All quick row, and Play Next / Play Last / Edit list rows. There's no "Go to Artist" row since the user is already acting on an artist. Adds the optimistic favorite toggle on Artist + ArtistProvider with the koel >= 7.11.0 feature gate, and removes the old long-press popup menu (artist_actions_menu.dart).
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughArtist gains a persistent ChangesArtist favorite + action sheet
Sequence DiagramsequenceDiagram
participant User as User / UI
participant Sheet as ArtistActionSheet
participant Provider as ArtistProvider
participant API as API Server
participant State as AppState
User->>Sheet: Tap favorite button
Sheet->>Sheet: Flip artist.favorite (optimistic)
Sheet->>State: notifyListeners()
Sheet->>Provider: toggleFavorite(artist)
Provider->>API: POST /favorites/toggle
alt success
API-->>Provider: 200 OK
Provider-->>Sheet: completes
else failure
API-->>Provider: Error
Provider->>Sheet: revert favorite, notifyListeners(), rethrow
end
sequenceDiagram
participant User as User / UI
participant Sheet as ArtistActionSheet
participant Playable as PlayableProvider
participant Audio as KoelAudioHandler
participant Overlay as Overlay Feedback
User->>Sheet: Tap "Play All" / "Shuffle All"
Sheet->>Sheet: Dismiss sheet
Sheet->>Playable: fetchForArtist(id)
Playable-->>Sheet: songs[]
Sheet->>Audio: replaceQueue(songs, shuffle: true/false)
Sheet->>Overlay: show confirmation (auto-dismiss)
Note over Sheet,Audio: Play Next / Play Last variations
User->>Sheet: Tap "Play Next"
loop for each song in songs.reversed
Sheet->>Audio: queueAfterCurrent(song)
end
User->>Sheet: Tap "Play Last"
loop for each song in songs
Sheet->>Audio: queueToBottom(song)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 6/8 reviews remaining, refill in 11 minutes and 20 seconds.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)
test/ui/screens/artist_action_sheet_test.dart (1)
94-97: ⚡ Quick winConsider adding an action test for tapping "Edit…".
The structure tests verify the
Edit…row is conditionally visible, but there's no interaction test asserting what happens when it's tapped (e.g., navigation to an edit screen or opening an edit dialog). This is the only action row that lacks a corresponding test in theactionsgroup — Play Next/Play Last/Play All/Shuffle All/Favorite all have them.✅ Suggested addition to the `actions` group
// Requires a MockAppRouter (or equivalent) injected via the mount helper, // and a stub/verify on the navigation call triggered by tapping Edit. testWidgets('tapping Edit navigates to the artist edit screen', (tester) async { final artist = Artist.fake(name: 'Editable', canEdit: true); // stub router or dialog expectation here await mount(tester, artist); await tester.tap(find.text('Edit…')); await tester.pumpAndSettle(); // verify navigation / dialog was shown });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/screens/artist_action_sheet_test.dart` around lines 94 - 97, Add an interaction test to the existing actions group that mounts the sheet with Artist.fake(name: 'Editable', canEdit: true) using the same mount helper, stubs/injects a MockAppRouter (or equivalent) used by the widget, simulates tapping the 'Edit…' row via tester.tap(find.text('Edit…')) and tester.pumpAndSettle(), and then verifies the expected navigation or dialog call was made on the router (or the dialog was shown); reference the existing testWidgets harness and mount helper so the new test mirrors the other action tests (e.g., Play Next/Play Last) and asserts the router/navigation method was invoked for the edit flow.
🤖 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/artist_action_sheet.dart`:
- Around line 90-93: The onTap currently calls
artistProvider.toggleFavorite(artist) without awaiting or handling its Future,
which lets toggleFavorite's rethrow produce an unhandled Future error; update
the onTap handler (where Navigator.pop(context) and
artistProvider.toggleFavorite(artist) are called) to either make the onTap async
and await artistProvider.toggleFavorite(artist) inside a try/catch, or append a
.catchError(...) to the returned Future to swallow/log the error (and optionally
show user feedback), ensuring toggleFavorite's exceptions are consumed.
---
Nitpick comments:
In `@test/ui/screens/artist_action_sheet_test.dart`:
- Around line 94-97: Add an interaction test to the existing actions group that
mounts the sheet with Artist.fake(name: 'Editable', canEdit: true) using the
same mount helper, stubs/injects a MockAppRouter (or equivalent) used by the
widget, simulates tapping the 'Edit…' row via tester.tap(find.text('Edit…')) and
tester.pumpAndSettle(), and then verifies the expected navigation or dialog call
was made on the router (or the dialog was shown); reference the existing
testWidgets harness and mount helper so the new test mirrors the other action
tests (e.g., Play Next/Play Last) and asserts the router/navigation method was
invoked for the edit flow.
🪄 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: 172d2fb2-8076-44cc-a507-4737082d2601
📒 Files selected for processing (14)
lib/models/artist.dartlib/providers/artist_provider.dartlib/ui/screens/artist_action_sheet.dartlib/ui/screens/artists.dartlib/ui/screens/screens.dartlib/ui/widgets/artist_actions_menu.dartlib/ui/widgets/artist_card.dartlib/ui/widgets/widgets.darttest/models/artist_test.darttest/ui/screens/artist_action_sheet_test.darttest/ui/screens/artist_action_sheet_test.mocks.darttest/ui/widgets/artist_card_test.darttest/ui/widgets/artist_card_test.mocks.darttest/ui/widgets/artist_row_test.dart
💤 Files with no reviewable changes (3)
- lib/ui/widgets/widgets.dart
- lib/ui/widgets/artist_actions_menu.dart
- test/ui/widgets/artist_row_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
Artist.favoritetoggle with the koel >= 7.11.0 feature gate (the favorite quick action is hidden on older servers).artist_actions_menu.dartpopup.Test plan
flutter test— all 297 tests pass, including new model + sheet coverage.Summary by CodeRabbit
New Features
UI
Tests