Conversation
📝 WalkthroughWalkthroughThis PR adds album download functionality to the album screen. It introduces background download capability using Changes
Sequence DiagramsequenceDiagram
actor User
participant AlbumScreen
participant AuthProvider
participant OfflineService
participant DownloadBackend as Background Download
User->>AlbumScreen: Tap download button
AlbumScreen->>AuthProvider: Check isOffline state
AuthProvider-->>AlbumScreen: Not in offline mode
AlbumScreen->>AlbumScreen: Set _isDownloading = true
AlbumScreen->>OfflineService: Initialize service
OfflineService-->>AlbumScreen: Ready
AlbumScreen->>DownloadBackend: startBackgroundDownload(_songs, subsonicService)
AlbumScreen->>AlbumScreen: Show "downloading in background" SnackBar
DownloadBackend->>DownloadBackend: Download in progress...
DownloadBackend-->>AlbumScreen: Download complete (Future resolves)
AlbumScreen->>AlbumScreen: Set _isDownloading = false
AlbumScreen->>User: Show success SnackBar
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 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)
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
🤖 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/album_screen.dart`:
- Around line 104-115: Replace the hardcoded English SnackBar and Text strings
with localized strings from AppLocalizations: use AppLocalizations.of(context)!
to build the messages that include dynamic values (_songs.length and
_album!.name) and update the SnackBar content in the ScaffoldMessenger/ SnackBar
code path (the block that constructs 'Downloaded ${_songs.length} songs from
${_album!.name}' and the 'Downloading ${_songs.length} songs in background…'
message). Ensure you add or use appropriate keys in AppLocalizations (e.g.,
downloadedSongs and downloadingInBackground) and format them with the dynamic
values so both SnackBars and any other hardcoded string at the referenced
location (line ~253) are localized.
- Around line 98-108: The success snackbar is unconditional because
offlineService.startBackgroundDownload(_songs, subsonicService) currently
completes even when some downloads fail; change startBackgroundDownload to
return a result (e.g., numberDownloaded, list of failed items, or a success
flag) or throw on fatal error, then await that result in album_screen and use it
to set _isDownloading and craft the SnackBar message: reference
offlineService.startBackgroundDownload, _songs and _album to compute the actual
downloaded count and/or report failures, and only show the "Downloaded X songs"
message when the returned result confirms X > 0 (otherwise show a
partial/failure message).
- Around line 201-203: The current build uses Provider.of<AuthProvider>(context,
listen: false) to set isOffline which prevents UI updates when auth changes and
only checks for AuthState.offlineMode; change both usages (the isOffline
declaration and the other check around the download button) to use
context.watch<AuthProvider>() so the widget rebuilds reactively and replace the
gating logic so the download UI/action is allowed only when
watch<AuthProvider>().state == AuthState.authenticated, denying it for
unauthenticated/error/serverUnreachable/offline states.
🪄 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: 9850fc47-d2de-4d93-af3a-0e92139a821e
📒 Files selected for processing (1)
lib/screens/album_screen.dart
| offlineService.startBackgroundDownload(_songs, subsonicService).then((_) { | ||
| if (mounted) { | ||
| setState(() => _isDownloading = false); | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text( | ||
| 'Downloaded ${_songs.length} songs from ${_album!.name}', | ||
| ), | ||
| duration: const Duration(seconds: 3), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Avoid unconditional success messaging after completion.
At Line 104, the snackbar claims all songs were downloaded, but OfflineService.startBackgroundDownload() absorbs failures and still completes normally. This can report success on partial/failed downloads.
Suggested adjustment in this file (safe interim)
- content: Text(
- 'Downloaded ${_songs.length} songs from ${_album!.name}',
- ),
+ content: Text(
+ 'Album download finished. Some songs may have failed.',
+ ),📝 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.
| offlineService.startBackgroundDownload(_songs, subsonicService).then((_) { | |
| if (mounted) { | |
| setState(() => _isDownloading = false); | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar( | |
| content: Text( | |
| 'Downloaded ${_songs.length} songs from ${_album!.name}', | |
| ), | |
| duration: const Duration(seconds: 3), | |
| ), | |
| ); | |
| offlineService.startBackgroundDownload(_songs, subsonicService).then((_) { | |
| if (mounted) { | |
| setState(() => _isDownloading = false); | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar( | |
| content: Text( | |
| 'Album download finished. Some songs may have failed.', | |
| ), | |
| duration: const Duration(seconds: 3), | |
| ), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/screens/album_screen.dart` around lines 98 - 108, The success snackbar is
unconditional because offlineService.startBackgroundDownload(_songs,
subsonicService) currently completes even when some downloads fail; change
startBackgroundDownload to return a result (e.g., numberDownloaded, list of
failed items, or a success flag) or throw on fatal error, then await that result
in album_screen and use it to set _isDownloading and craft the SnackBar message:
reference offlineService.startBackgroundDownload, _songs and _album to compute
the actual downloaded count and/or report failures, and only show the
"Downloaded X songs" message when the returned result confirms X > 0 (otherwise
show a partial/failure message).
| 'Downloaded ${_songs.length} songs from ${_album!.name}', | ||
| ), | ||
| duration: const Duration(seconds: 3), | ||
| ), | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text('Downloading ${_songs.length} songs in background…'), |
There was a problem hiding this comment.
Localize newly added UI strings.
At Lines 104, 115, and 253, strings are hardcoded in English. This bypasses AppLocalizations used elsewhere in this screen.
Also applies to: 253-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/screens/album_screen.dart` around lines 104 - 115, Replace the hardcoded
English SnackBar and Text strings with localized strings from AppLocalizations:
use AppLocalizations.of(context)! to build the messages that include dynamic
values (_songs.length and _album!.name) and update the SnackBar content in the
ScaffoldMessenger/ SnackBar code path (the block that constructs 'Downloaded
${_songs.length} songs from ${_album!.name}' and the 'Downloading
${_songs.length} songs in background…' message). Ensure you add or use
appropriate keys in AppLocalizations (e.g., downloadedSongs and
downloadingInBackground) and format them with the dynamic values so both
SnackBars and any other hardcoded string at the referenced location (line ~253)
are localized.
| final isOffline = | ||
| Provider.of<AuthProvider>(context, listen: false).state == | ||
| AuthState.offlineMode; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "album_screen.dart" | head -5Repository: dddevid/Musly
Length of output: 88
🏁 Script executed:
find . -type f -name "auth_provider.dart" | head -5Repository: dddevid/Musly
Length of output: 91
🏁 Script executed:
cat -n lib/screens/album_screen.dart | sed -n '195,260p'Repository: dddevid/Musly
Length of output: 2968
🏁 Script executed:
cat -n lib/providers/auth_provider.dartRepository: dddevid/Musly
Length of output: 11420
Make auth gating reactive and restrict download to authenticated state.
At line 202, using listen: false in build makes this value stale if auth state changes. Also, hiding only for offlineMode is too permissive—the download button currently shows in unsafe states like unauthenticated, error, and serverUnreachable. Only authenticated users should be able to download.
Replace with reactive pattern using context.watch<AuthProvider>() and gate the button to AuthState.authenticated only.
Also applies to: 251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/screens/album_screen.dart` around lines 201 - 203, The current build uses
Provider.of<AuthProvider>(context, listen: false) to set isOffline which
prevents UI updates when auth changes and only checks for AuthState.offlineMode;
change both usages (the isOffline declaration and the other check around the
download button) to use context.watch<AuthProvider>() so the widget rebuilds
reactively and replace the gating logic so the download UI/action is allowed
only when watch<AuthProvider>().state == AuthState.authenticated, denying it for
unauthenticated/error/serverUnreachable/offline states.
1 participant
Implements #105
The implementation was largely copied from the playlist screen.
Musly/lib/screens/playlist_screen.dart
Line 211 in 4e99337