Open
Conversation
📝 WalkthroughWalkthroughAdds an AppBar action to enqueue all songs for an artist and implements a private async method that aggregates songs per album (from local cache or remote), calls PlayerProvider.addAllToQueue, and shows localized SnackBars on success or error. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "ArtistScreen (IconButton)"
participant Cache as "Local Cache"
participant API as "subsonicService"
participant Player as "PlayerProvider"
participant Snack as "ScaffoldMessenger (SnackBar)"
UI->>UI: tap "Add Artist to Queue"
UI->>Cache: check albums list
alt albums empty
UI-->UI: return early
else albums present
loop albums
UI->>Cache: get songs for album
alt cache miss
Cache->>API: fetch album songs
API-->>Cache: return songs
end
Cache-->>UI: provide songs
end
UI->>Player: addAllToQueue(collectedSongs)
alt success
Player-->>Snack: show success SnackBar
else error
Player-->>Snack: show error SnackBar
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
📝 Coding Plan
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
🧹 Nitpick comments (1)
lib/screens/artist_screen.dart (1)
185-188: Add tooltip and disable the button when no albums are available.On Line 187, the action remains clickable even when
_albumsis empty (it becomes a silent no-op at Line 78), and there’s no tooltip for accessibility/discoverability.Proposed fix
IconButton( icon: const Icon(Icons.queue_music_rounded), - onPressed: () => _addArtistToQueue(), + tooltip: AppLocalizations.of(context)!.addToQueue, + onPressed: _albums.isEmpty ? null : () => _addArtistToQueue(), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/screens/artist_screen.dart` around lines 185 - 188, Wrap or update the IconButton so it provides a tooltip and becomes disabled when there are no albums: change the onPressed to be conditional (e.g. onPressed: _albums.isEmpty ? null : () => _addArtistToQueue()) and add a tooltip (either via the IconButton.tooltip property or a surrounding Tooltip widget) such as 'Add artist to queue' when albums exist and 'No albums' when _albums.isEmpty; this uses the existing IconButton, _addArtistToQueue, and _albums symbols to locate the change.
🤖 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/screens/artist_screen.dart`:
- Around line 106-109: Replace the SnackBar content that currently displays the
raw exception string from messenger.showSnackBar / SnackBar / Text('Error adding
to queue: $e') with a user-safe, localized message (e.g., use your localization
helper like AppLocalizations.of(context).errorAddingToQueue or a generic "Unable
to add to queue" string), and move the raw error details into a developer-facing
log or error-reporting call (e.g., logger.error or Sentry.captureException) so
end users do not see technical exception text.
- Around line 92-94: The loop calling playerProvider.addToQueue for each song
causes repeated notifyListeners and UI rebuilds; implement and use a batched
method (e.g., addAllToQueue on PlayerProvider) that takes an Iterable<Song> and
performs a single _queue.addAll(...) followed by one notifyListeners(), then
replace the for-loop over albumSongs with a single call to
playerProvider.addAllToQueue(albumSongs); ensure the new method is added to the
same class that defines addToQueue (e.g., PlayerProvider) so callers can switch
to the batched API.
---
Nitpick comments:
In `@lib/screens/artist_screen.dart`:
- Around line 185-188: Wrap or update the IconButton so it provides a tooltip
and becomes disabled when there are no albums: change the onPressed to be
conditional (e.g. onPressed: _albums.isEmpty ? null : () => _addArtistToQueue())
and add a tooltip (either via the IconButton.tooltip property or a surrounding
Tooltip widget) such as 'Add artist to queue' when albums exist and 'No albums'
when _albums.isEmpty; this uses the existing IconButton, _addArtistToQueue, and
_albums symbols to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7cf1e13-f25a-455c-8ccd-eb526851db9b
📒 Files selected for processing (1)
lib/screens/artist_screen.dart
Comment on lines
+106
to
+109
| messenger.showSnackBar( | ||
| SnackBar( | ||
| content: Text('Error adding to queue: $e'), | ||
| duration: const Duration(seconds: 2), |
There was a problem hiding this comment.
Use localized/generic error text instead of raw exception details.
On Line 108, the snackbar is hardcoded English and exposes technical exception text to end users. Prefer localized, user-safe messaging and log the raw error separately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/screens/artist_screen.dart` around lines 106 - 109, Replace the SnackBar
content that currently displays the raw exception string from
messenger.showSnackBar / SnackBar / Text('Error adding to queue: $e') with a
user-safe, localized message (e.g., use your localization helper like
AppLocalizations.of(context).errorAddingToQueue or a generic "Unable to add to
queue" string), and move the raw error details into a developer-facing log or
error-reporting call (e.g., logger.error or Sentry.captureException) so end
users do not see technical exception text.
Kreedyy
force-pushed
the
Features-testing
branch
from
ceca97f to
8cd53bd
Compare
Kreedyy
force-pushed
the
Features-testing
branch
from
8cd53bd to
4735588
Compare
Owner
|
Thanks for the PR! I noticed the error message is currently hard-coded. Could you please move it to the ARB file to ensure it's properly localized? Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/screens/artist_screen.dart (1)
195-199: Guard against repeated taps while queueing is in progress.The button can be pressed multiple times before the async action completes, which can enqueue the artist catalog repeatedly.
🛡️ Proposed in-flight guard
class _ArtistScreenState extends State<ArtistScreen> { Artist? _artist; List<Song> _topSongs = []; List<Album> _albums = []; bool _isLoading = true; + bool _isAddingArtistToQueue = false; Future<void> _addArtistToQueue() async { - if (_albums.isEmpty) return; + if (_albums.isEmpty || _isAddingArtistToQueue) return; + setState(() => _isAddingArtistToQueue = true); final playerProvider = Provider.of<PlayerProvider>(context, listen: false); @@ - } catch (e) { + } catch (e) { if (!mounted) return; @@ ); + } finally { + if (mounted) { + setState(() => _isAddingArtistToQueue = false); + } } } @@ IconButton( icon: const Icon(Icons.queue_music_rounded), tooltip: AppLocalizations.of(context)!.addToQueue, - onPressed: _albums.isEmpty ? null : () => _addArtistToQueue(), + onPressed: (_albums.isEmpty || _isAddingArtistToQueue) + ? null + : () => _addArtistToQueue(), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/screens/artist_screen.dart` around lines 195 - 199, The IconButton can trigger multiple concurrent calls to _addArtistToQueue; add an in-flight guard by introducing a boolean state field (e.g. _isQueueing) and update the onPressed condition to be disabled when _isQueueing is true (onPressed: _albums.isEmpty || _isQueueing ? null : () => _addArtistToQueue()). In _addArtistToQueue set _isQueueing = true before starting async work and clear it in a finally block after awaiting to ensure the button is re-enabled only once the operation completes; optionally update the UI (setState) to reflect the disabled/loading state.
🤖 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/l10n/app_en.arb`:
- Around line 1254-1258: The strings addedArtistToQueue and
addedArtistToQueueError in the localization file are awkwardly phrased; update
their values to natural English (e.g., change "Added artist to Queue" to "Artist
added to queue" and "Failed adding artist to Queue" to "Failed to add artist to
queue"), leaving the description metadata (`@addedArtistToQueue` and
`@addedArtistToQueueError`) unchanged so translations and tooling remain intact.
In `@lib/screens/artist_screen.dart`:
- Around line 99-111: The snackbar success message is shown even when no songs
were queued; change the flow so the success notification is only displayed when
songsToQueue.isNotEmpty (the same condition used for calling
playerProvider.addAllToQueue). Concretely, move or wrap the
messenger.showSnackBar call (and the addedToQueueMessage preparation) inside the
if (songsToQueue.isNotEmpty) block after playerProvider.addAllToQueue, keep the
mounted check before showing UI, and do not show the SnackBar when songsToQueue
is empty.
---
Nitpick comments:
In `@lib/screens/artist_screen.dart`:
- Around line 195-199: The IconButton can trigger multiple concurrent calls to
_addArtistToQueue; add an in-flight guard by introducing a boolean state field
(e.g. _isQueueing) and update the onPressed condition to be disabled when
_isQueueing is true (onPressed: _albums.isEmpty || _isQueueing ? null : () =>
_addArtistToQueue()). In _addArtistToQueue set _isQueueing = true before
starting async work and clear it in a finally block after awaiting to ensure the
button is re-enabled only once the operation completes; optionally update the UI
(setState) to reflect the disabled/loading state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12696c2a-f06d-4371-a13d-4cd1f1e87cbc
📒 Files selected for processing (3)
lib/l10n/app_en.arblib/providers/player_provider.dartlib/screens/artist_screen.dart
Comment on lines
+1254
to
+1258
| "addedArtistToQueue": "Added artist to Queue", | ||
| "@addedArtistToQueue": { "description": "Snackbar when user adds artist to queue" }, | ||
|
|
||
| "addedArtistToQueueError": "Failed adding artist to Queue", | ||
| "@addedArtistToQueueError": { "description": "Error shown in snackbar when adding artist to queue fails" }, |
There was a problem hiding this comment.
Polish snackbar copy for natural phrasing.
Current messages read a bit awkwardly. Suggested wording is clearer for users.
✏️ Proposed copy update
- "addedArtistToQueue": "Added artist to Queue",
+ "addedArtistToQueue": "Added artist to queue",
- "addedArtistToQueueError": "Failed adding artist to Queue",
+ "addedArtistToQueueError": "Failed to add artist to queue",📝 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
| "addedArtistToQueue": "Added artist to Queue", | |
| "@addedArtistToQueue": { "description": "Snackbar when user adds artist to queue" }, | |
| "addedArtistToQueueError": "Failed adding artist to Queue", | |
| "@addedArtistToQueueError": { "description": "Error shown in snackbar when adding artist to queue fails" }, | |
| "addedArtistToQueue": "Added artist to queue", | |
| "@addedArtistToQueue": { "description": "Snackbar when user adds artist to queue" }, | |
| "addedArtistToQueueError": "Failed to add artist to queue", | |
| "@addedArtistToQueueError": { "description": "Error shown in snackbar when adding artist to queue fails" }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/l10n/app_en.arb` around lines 1254 - 1258, The strings addedArtistToQueue
and addedArtistToQueueError in the localization file are awkwardly phrased;
update their values to natural English (e.g., change "Added artist to Queue" to
"Artist added to queue" and "Failed adding artist to Queue" to "Failed to add
artist to queue"), leaving the description metadata (`@addedArtistToQueue` and
`@addedArtistToQueueError`) unchanged so translations and tooling remain intact.
Comment on lines
+99
to
+111
| if (songsToQueue.isNotEmpty) { | ||
| playerProvider.addAllToQueue(songsToQueue); | ||
| } | ||
|
|
||
| if (!mounted) return; | ||
|
|
||
| final addedToQueueMessage = loc?.addedArtistToQueue ?? 'Added artist to Queue'; | ||
| messenger.showSnackBar( | ||
| SnackBar( | ||
| content: Text(addedToQueueMessage), | ||
| duration: const Duration(seconds: 2), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Don’t show success when zero songs were queued.
On Line 105, success is shown even when songsToQueue is empty. That produces false-positive feedback.
✅ Proposed fix
- if (songsToQueue.isNotEmpty) {
- playerProvider.addAllToQueue(songsToQueue);
- }
-
- if (!mounted) return;
-
- final addedToQueueMessage = loc?.addedArtistToQueue ?? 'Added artist to Queue';
- messenger.showSnackBar(
- SnackBar(
- content: Text(addedToQueueMessage),
- duration: const Duration(seconds: 2),
- ),
- );
+ if (songsToQueue.isEmpty) {
+ if (!mounted) return;
+ final addedToQueueErrorMessage =
+ loc?.addedArtistToQueueError ?? 'Failed to add artist to queue';
+ messenger.showSnackBar(
+ SnackBar(
+ content: Text(addedToQueueErrorMessage),
+ duration: const Duration(seconds: 2),
+ ),
+ );
+ return;
+ }
+
+ playerProvider.addAllToQueue(songsToQueue);
+ if (!mounted) return;
+ final addedToQueueMessage =
+ loc?.addedArtistToQueue ?? 'Added artist to queue';
+ messenger.showSnackBar(
+ SnackBar(
+ content: Text(addedToQueueMessage),
+ duration: const Duration(seconds: 2),
+ ),
+ );📝 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
| if (songsToQueue.isNotEmpty) { | |
| playerProvider.addAllToQueue(songsToQueue); | |
| } | |
| if (!mounted) return; | |
| final addedToQueueMessage = loc?.addedArtistToQueue ?? 'Added artist to Queue'; | |
| messenger.showSnackBar( | |
| SnackBar( | |
| content: Text(addedToQueueMessage), | |
| duration: const Duration(seconds: 2), | |
| ), | |
| ); | |
| if (songsToQueue.isEmpty) { | |
| if (!mounted) return; | |
| final addedToQueueErrorMessage = | |
| loc?.addedArtistToQueueError ?? 'Failed to add artist to queue'; | |
| messenger.showSnackBar( | |
| SnackBar( | |
| content: Text(addedToQueueErrorMessage), | |
| duration: const Duration(seconds: 2), | |
| ), | |
| ); | |
| return; | |
| } | |
| playerProvider.addAllToQueue(songsToQueue); | |
| if (!mounted) return; | |
| final addedToQueueMessage = | |
| loc?.addedArtistToQueue ?? 'Added artist to queue'; | |
| messenger.showSnackBar( | |
| SnackBar( | |
| content: Text(addedToQueueMessage), | |
| duration: const Duration(seconds: 2), | |
| ), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/screens/artist_screen.dart` around lines 99 - 111, The snackbar success
message is shown even when no songs were queued; change the flow so the success
notification is only displayed when songsToQueue.isNotEmpty (the same condition
used for calling playerProvider.addAllToQueue). Concretely, move or wrap the
messenger.showSnackBar call (and the addedToQueueMessage preparation) inside the
if (songsToQueue.isNotEmpty) block after playerProvider.addAllToQueue, keep the
mounted check before showing UI, and do not show the SnackBar when songsToQueue
is empty.
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
2 participants
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.
Closes #97
Add localization for error msg in 'artist_screen' on line: 112?
Summary by CodeRabbit
New Features
User Interface
Localization