feat: offline playlist & album downloader#191
Open
tbrackbill wants to merge 9 commits into
Open
Conversation
OfflineService: - Add downloadedSongIds ValueNotifier (Set<String>) seeded from SharedPrefs on initialize(), updated on download success/delete so UI reacts without polling - Add downloadLog ValueNotifier (List<DownloadLogEntry>) with per-song queued/downloading/done/failed status, reset each batch - Extend DownloadState with currentSong and failedSongs for active-download panel - Add getPlaylistDownloadStatus() helper returning (downloaded, total) - Add DownloadStatus enum and DownloadLogEntry model SongTile: - _buildTrailing() wrapped in ValueListenableBuilder on downloadedSongIds; shows 14px green check_circle badge when song is locally downloaded PlaylistScreen / AlbumScreen: - Artwork wrapped in ValueListenableBuilder on downloadedSongIds; shows green check_circle badge when every song in the collection is downloaded Settings (Offline Downloads section): - Active Downloads row: shows current song artist-title and X/Y progress while a batch runs, otherwise "No downloads in progress" - Playlist Downloads row: shows total downloaded-song count from reactive set; navigation to per-playlist breakdown wired in feature/download-detail-screens Build: - android/gradle.properties: add android.newDsl=false (AGP 8.11 compat) - Dockerfile.release + build-release-apk.sh: retained for reference but native build via /rw/flutter is the preferred approach going forward Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add green check badge to _PlaylistTile when all songs downloaded - Create DownloadPlaylistStatusScreen showing downloaded/total per playlist - Wire Settings > Playlist Downloads row to navigate to new screen Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Switch downloadSong() from /stream to /download so files are always the original, non-transcoded version and match song.size exactly - Persist expected file sizes to SharedPrefs before each download so reconciliation works even if the app is killed mid-batch - initialize() now scans the offline dir and recovers any valid files that landed on disk but weren't indexed (handles interrupted downloads) - isSongDownloaded() uses stored expected size when available, 64 KB floor as fallback for songs where size is unknown - One automatic retry pass for failed songs at end of each batch - deleteSong() and deleteAllDownloads() clean up the expected sizes map Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- queuePlaylistDownload() replaces direct startBackgroundDownload calls; multiple playlists/albums stack in a sequential download queue processed one batch at a time - queuedPlaylistIds ValueNotifier drives an outline check_circle_outline badge in playlists list and Library view when a playlist is queued but not yet fully downloaded; fills to check_circle when complete - Queued state (including full song list JSON) is persisted to SharedPrefs so the badge survives app restarts - initialize() loads queued playlist data and unmarks any that are now fully on disk after the reconciliation scan - PlayerProvider resumes incomplete queued downloads automatically on startup via resumeIncompleteDownloads() - deleteAllDownloads() clears the queue and queued playlist data Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Playlist/album download spinner now driven by queuedPlaylistIds
ValueNotifier instead of local _isDownloading state, so the spinner
stays visible for the full duration of the download — including when
additional playlists are queued while one is already in progress
- Also save cover art indexed by song.coverArt ID (art_{id}.jpg) so
AlbumArtwork can resolve it offline for album/playlist grid views;
previously art was only stored by song ID and never found by the widget
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace fragile nested ValueListenableBuilder with explicit listeners: - _allDownloaded and _isQueued driven by addListener on OfflineService notifiers; state is updated via _updateDownloadState() called on load and on every notifier change, ensuring correct initial state - Button has 3 clean states: cloud_download (idle) → spinner (queued/ downloading, tap cancels) → cloud_done green (complete, tap removes) - Cancel: removes playlist from queue and stops active download - Remove: confirmation dialog then deletes all songs in playlist/album - Added cancelPlaylistDownload() and deletePlaylistDownloads() to OfflineService Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- cancelPlaylistDownload: only kill the active download when the target
playlist is currently running; cancelling a queued-but-not-started
entry no longer stops an unrelated active download
- _updateDownloadState: disk-check fallback now guarded by _isQueued
(previous state) so it only fires on the queued→not-queued transition,
not on every song-download event for every open screen
- deletePlaylistDownloads: accepts List<Song> and deletes art_{id}.jpg
files alongside song files so stale offline art doesn't persist after
removing downloads
- AlbumArtwork: skip File.existsSync() when downloadedSongIds is empty,
avoiding synchronous disk stats on every build for non-downloading users
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace file-presence checks with a dedicated downloadedPlaylistIds notifier so cross-playlist songs don't produce false solid-check badges. The solid check now reflects explicit download intent rather than whether individual audio files happen to exist on disk. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…badges - Move WakelockPlus.disable() outside retry block so it always fires - cancelBackgroundDownload() before wiping index in deleteAllDownloads - Remove shared art_* deletion from deletePlaylistDownloads (cross-playlist safe) - Swap downloadedSongIds listeners to downloadedPlaylistIds in playlist/album screens - Simplify _updateDownloadState to intent-based contains() lookup - Version 1.0.13+4 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces a stateful, queue-driven offline download system with reactive UI updates and adds release build infrastructure. The offline service now validates downloads by expected file size, tracks playlist-level completion intent, and resumes incomplete batches on app startup. Screens transition from polling/flag-based UI to listener-driven state, displaying download progress and status badges throughout the app. Release Docker and Bash tooling automate APK production. ChangesOffline Download State Management & UI
Release Build Infrastructure
🎯 4 (Complex) | ⏱️ ~75 minutes Possibly Related PRs
🚥 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. 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: 13
🧹 Nitpick comments (4)
lib/screens/download_playlist_status_screen.dart (1)
14-14: 💤 Low valueConsider
listen: falsefor LibraryProvider if playlists are stable.The screen uses
Provider.of<LibraryProvider>(context)withoutlisten: false, which rebuilds the entire screen whenever LibraryProvider notifies listeners. If playlists rarely change while this screen is visible, addinglisten: falsewould prevent unnecessary rebuilds since theValueListenableBuilderalready handles download state reactively.⚡ Optional performance tweak
- final libraryProvider = Provider.of<LibraryProvider>(context); + final libraryProvider = Provider.of<LibraryProvider>(context, listen: false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/screens/download_playlist_status_screen.dart` at line 14, The screen calls Provider.of<LibraryProvider>(context) which causes the widget to rebuild on provider updates; change this to Provider.of<LibraryProvider>(context, listen: false) (or otherwise obtain the provider with listen: false) so that the top-level download_playlist_status_screen widget does not rebuild unnecessarily—update the usage where the variable libraryProvider is assigned in download_playlist_status_screen.dart to pass listen: false.lib/services/offline_service.dart (2)
162-168: 💤 Low valueUnsafe cast may throw if persisted data is malformed.
If
_keyQueuedPlaylistDatacontains corrupted or unexpected JSON structure, the cast(v as List).cast<Map<String, dynamic>>()will throw aCastErrorat runtime. Consider wrapping this in a try-catch like the outer block or validating structure before casting.Proposed safer parsing
try { final raw = json.decode(queuedDataJson) as Map<String, dynamic>; - _queuedPlaylistData = raw.map( - (k, v) => MapEntry(k, (v as List).cast<Map<String, dynamic>>()), - ); + _queuedPlaylistData = {}; + for (final entry in raw.entries) { + if (entry.value is List) { + final songs = <Map<String, dynamic>>[]; + for (final item in entry.value as List) { + if (item is Map<String, dynamic>) songs.add(item); + } + _queuedPlaylistData[entry.key] = songs; + } + } } catch (_) {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/services/offline_service.dart` around lines 162 - 168, The current deserialization of queuedDataJson naively casts entries which can throw a CastError; update the parsing for json.decode(queuedDataJson) so you validate types before casting: ensure the decoded value is a Map<String, dynamic>, iterate its entries, check each value is a List, and for each list item verify it's a Map<String, dynamic> (skip or drop invalid items) before building _queuedPlaylistData; wrap the whole operation in a try-catch to preserve the existing failure-silent behavior and fall back to an empty map or previous state if parsing fails.
752-752: 💤 Low valueRedundant
_downloadQueue.clear()call.
_downloadQueue.clear()is called twice: once at line 733 and again at line 752. The second call is unnecessary.Remove duplicate clear
_expectedSizes = {}; _queuedPlaylistData = {}; - _downloadQueue.clear(); queuedPlaylistIds.value = {};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/services/offline_service.dart` at line 752, The second redundant call to _downloadQueue.clear() should be removed: locate the method that manipulates _downloadQueue (the block calling _downloadQueue.clear() twice) and delete the later invocation of _downloadQueue.clear() so the queue is only cleared once (keep the first clear where it currently executes and ensure no other logic depends on a second clear).lib/providers/player_provider.dart (1)
1127-1132: 💤 Low valueFire-and-forget download resumption has no error handling.
The
.then()callback invokesresumeIncompleteDownloadsbut any exceptions are silently swallowed. If_subsonicServiceis not yet configured or the resume fails, there's no feedback. Consider adding.catchError()for logging.Proposed fix with error handling
// Resume any playlists that were queued for download but interrupted _offlineService.initialize().then((_) { _offlineService.resumeIncompleteDownloads(_subsonicService); - }); - + }).catchError((e) { + debugPrint('Failed to resume incomplete downloads: $e'); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/providers/player_provider.dart` around lines 1127 - 1132, The fire-and-forget call to _offlineService.initialize().then((_) { _offlineService.resumeIncompleteDownloads(_subsonicService); }) lacks error handling and can swallow exceptions (including cases where _subsonicService is null); change this to await or to a promise chain that catches errors (e.g., use .then(...).catchError(...) or an async function with try/catch) and log any errors using the module's logger so failures in _offlineService.initialize() or _offlineService.resumeIncompleteDownloads(_subsonicService) are reported; also validate _subsonicService before calling resumeIncompleteDownloads and log a clear message if it is not configured.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build-release-apk.sh`:
- Around line 9-23: The script uses a fixed CONTAINER_NAME ("musly-apk-build")
which can collide across concurrent runs; change CONTAINER_NAME in
build-release-apk.sh to a unique value (e.g., include $$, a timestamp, or a
generated UUID) and use that variable consistently for docker create, docker cp
and docker rm; also add a trap cleanup that removes the container name on exit
to avoid leftover containers (references: CONTAINER_NAME, docker create --name,
docker cp, docker rm -f).
In `@Dockerfile.release`:
- Around line 41-43: Replace the non-deterministic clone of Flutter that uses
--branch stable with a pinned SDK tag or commit so releases are reproducible:
update the RUN that clones into /opt/flutter to fetch a specific release (e.g.,
use --branch vX.Y.Z or clone then git checkout <tag_or_commit>) and keep the git
config --global --add safe.directory /opt/flutter; choose and document the exact
Flutter tag/commit to pin (replace "stable" with the chosen tag in the RUN
command or add an explicit git checkout after clone).
In `@lib/screens/album_screen.dart`:
- Around line 111-178: Replace hardcoded English UI strings in _downloadAlbum,
_removeDownloads, and _buildDownloadButton with localized values from
AppLocalizations (e.g. call AppLocalizations.of(context) inside these
methods/builders and use resource keys instead of literal text); specifically
localize the SnackBar message in _downloadAlbum, the AlertDialog title/content
and button labels in _removeDownloads, and all IconButton tooltip texts in
_buildDownloadButton, adding new localization keys
(queuedSongs/removeDownloadsTitle/removeDownloadsContent/cancelLabel/removeLabel/downloadedTooltip/downloadingTooltip/downloadAlbumTooltip
or similar) to your ARB/locale files and use those keys via AppLocalizations in
the referenced methods.
In `@lib/screens/library_screen.dart`:
- Around line 649-650: The call to
OfflineService().cancelPlaylistDownload(item.id) can throw and prevent
libraryProvider.deletePlaylist(item.id) from running; wrap the
cancelPlaylistDownload call in a try/catch that catches and logs the error
(including context and item.id) but does not rethrow, then always call
libraryProvider.deletePlaylist(item.id) so playlist deletion proceeds regardless
of offline cleanup failure.
In `@lib/screens/playlist_screen.dart`:
- Around line 303-371: Replace hardcoded English UI strings in
_downloadPlaylist, _removeDownloads, _cancelDownload (SnackBar text, AlertDialog
title/content/button labels) and the tooltips in _buildDownloadButton with
localized values from AppLocalizations (e.g., final loc =
AppLocalizations.of(context)!). Use loc.* getters for the snackbar message
("Queued X songs for download…"), the dialog title/content and button labels
("Remove downloads?", "Cancel", "Remove"), and the three tooltips ("Downloaded —
tap to remove", "Downloading — tap to cancel", "Download playlist"); ensure
pluralization or interpolation uses loc methods if available and pass
songs.length into the localized string where needed.
- Around line 420-463: The primary header (SliverAppBar) artwork lacks the
downloaded badge because the badge rendering is only inside the reorder-only
branch; update the SliverAppBar artwork builder to use the same download-state
logic as the ValueListenableBuilder currently referencing
OfflineService().downloadedPlaylistIds (and the allDownloaded boolean) so the
check icon is shown in both places: reuse the same condition
(downloaded.contains(widget.playlistId) / allDownloaded) and render the
Positioned check badge around the artwork (same styling used where
_playlist!.coverArt / AlbumArtwork is rendered) so the header shows the
completed-download badge outside of reorder mode as well.
In `@lib/screens/playlists_screen.dart`:
- Around line 160-161: The cancelPlaylistDownload call can throw and prevent
deletion; wrap the call to OfflineService().cancelPlaylistDownload(playlist.id)
in its own try/catch so failures are logged/handled but do not stop execution,
then always call libraryProvider.deletePlaylist(playlist.id) (or rethrow only
after deletion if needed); reference the symbols cancelPlaylistDownload,
deletePlaylist and playlist.id and ensure the catch does not swallow important
errors (log or report them) before proceeding to delete.
In `@lib/screens/settings_storage_tab.dart`:
- Around line 718-756: Localize the hardcoded strings in
_buildPlaylistStatusRow: replace the title 'Playlist Downloads' with
AppLocalizations.of(context).playlistDownloads and the subtitle '${ids.length}
songs downloaded' with the pluralized/localized call
AppLocalizations.of(context).songsDownloaded(ids.length) (or your project's
plural API), and add the two keys to AppLocalizations: playlistDownloads and
songsDownloaded(int count); keep the rest of the ListTile (leading, trailing,
navigation to DownloadPlaylistStatusScreen, and usage of
_offlineService.downloadedSongIds) unchanged.
- Around line 670-716: The ListTile title and fallback subtitle in
_buildActiveDownloadsRow are hardcoded; replace the strings with localized
values from AppLocalizations (use AppLocalizations.of(context)!.activeDownloads
for the title and AppLocalizations.of(context)!.noDownloadsInProgress for the
'No downloads in progress' fallback used when state.isDownloading is false), and
add the corresponding keys activeDownloads and noDownloadsInProgress to your
localization resource files and generated AppLocalizations class so the compiler
picks them up.
In `@lib/services/offline_service.dart`:
- Around line 606-619: The retry loop updates downloadState.failedSongs but
never updates per-song entries in downloadLog; modify the retry block around
downloadSong(song, subsonicService) in the same method so that after each
attempt you call _updateLogEntry (or the appropriate helper) for that song to
set its status to "retrying" before the attempt and then to "downloaded" on
success or "failed" on final failure, and ensure downloadState.value.failedSongs
and downloadLog stay in sync (use Song identity or song.id to locate the log
entry and update progress/timestamps accordingly); keep respecting
_isBackgroundDownloadActive and only update the log when the attempt completes.
- Around line 404-406: getDownloadUrl currently always builds a Subsonic
/rest/download URL which breaks Jellyfin/YouTube backends; update
SubsonicService.getDownloadUrl to mirror getStreamUrl by returning
_jellyfin!.getDownloadUrl(songId) if _jellyfin != null, returning
_youtube!.getDownloadUrl(songId) if _youtube != null, and only then falling back
to _buildUrl('download', {'id': songId}); also add corresponding getDownloadUrl
implementations to JellyfinService and YoutubeService so those backends return
proper download URLs.
In `@lib/services/subsonic_service.dart`:
- Around line 423-427: getDownloadUrl currently always returns a Subsonic
download URL via _buildUrl('download', ...) and omits backend checks; update
getDownloadUrl to mirror the pattern used in getStreamUrl/getCoverArtUrl: if
_jellyfin is present delegate to _jellyfin (e.g., call its stream/download
helper or fallback to getStreamUrl(songId)), if _youtube is present delegate to
_youtube (or fallback to getStreamUrl(songId)), otherwise return
_buildUrl('download', {'id': songId}); keep references to getDownloadUrl,
getStreamUrl, getCoverArtUrl, _jellyfin, _youtube and _buildUrl to locate the
change.
In `@lib/widgets/album_artwork.dart`:
- Around line 192-202: The widget currently reads
OfflineService().downloadedSongIds.value directly, so _buildImageNatural and
_buildImage won't rebuild when downloads complete; wrap each offline-art check
(the block that calls
OfflineService().getLocalCoverArtPathByCoverArtId(coverArt) and returns
Image.file with ValueKey('offline_natural_$coverArt') / similar key) in a
ValueListenableBuilder<Set<String>> that listens to
OfflineService().downloadedSongIds and returns the same Image.file (or
placeholder) based on the latest value so the widget rebuilds automatically when
downloadedSongIds updates.
---
Nitpick comments:
In `@lib/providers/player_provider.dart`:
- Around line 1127-1132: The fire-and-forget call to
_offlineService.initialize().then((_) {
_offlineService.resumeIncompleteDownloads(_subsonicService); }) lacks error
handling and can swallow exceptions (including cases where _subsonicService is
null); change this to await or to a promise chain that catches errors (e.g., use
.then(...).catchError(...) or an async function with try/catch) and log any
errors using the module's logger so failures in _offlineService.initialize() or
_offlineService.resumeIncompleteDownloads(_subsonicService) are reported; also
validate _subsonicService before calling resumeIncompleteDownloads and log a
clear message if it is not configured.
In `@lib/screens/download_playlist_status_screen.dart`:
- Line 14: The screen calls Provider.of<LibraryProvider>(context) which causes
the widget to rebuild on provider updates; change this to
Provider.of<LibraryProvider>(context, listen: false) (or otherwise obtain the
provider with listen: false) so that the top-level
download_playlist_status_screen widget does not rebuild unnecessarily—update the
usage where the variable libraryProvider is assigned in
download_playlist_status_screen.dart to pass listen: false.
In `@lib/services/offline_service.dart`:
- Around line 162-168: The current deserialization of queuedDataJson naively
casts entries which can throw a CastError; update the parsing for
json.decode(queuedDataJson) so you validate types before casting: ensure the
decoded value is a Map<String, dynamic>, iterate its entries, check each value
is a List, and for each list item verify it's a Map<String, dynamic> (skip or
drop invalid items) before building _queuedPlaylistData; wrap the whole
operation in a try-catch to preserve the existing failure-silent behavior and
fall back to an empty map or previous state if parsing fails.
- Line 752: The second redundant call to _downloadQueue.clear() should be
removed: locate the method that manipulates _downloadQueue (the block calling
_downloadQueue.clear() twice) and delete the later invocation of
_downloadQueue.clear() so the queue is only cleared once (keep the first clear
where it currently executes and ensure no other logic depends on a second
clear).
🪄 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: f87b418d-46a2-4a4f-b199-221a4ca2d3ad
📒 Files selected for processing (15)
Dockerfile.releaseandroid/gradle.propertiesbuild-release-apk.shlib/providers/player_provider.dartlib/screens/album_screen.dartlib/screens/download_playlist_status_screen.dartlib/screens/library_screen.dartlib/screens/playlist_screen.dartlib/screens/playlists_screen.dartlib/screens/settings_storage_tab.dartlib/services/offline_service.dartlib/services/subsonic_service.dartlib/widgets/album_artwork.dartlib/widgets/song_tile.dartpubspec.yaml
Comment on lines
+9
to
+23
| CONTAINER_NAME="musly-apk-build" | ||
|
|
||
| cd "$(dirname "$0")" | ||
|
|
||
| echo "==> Building Docker image: $IMAGE_TAG" | ||
| sudo docker build -f Dockerfile.release -t "$IMAGE_TAG" . | ||
|
|
||
| echo "==> Extracting APK from image" | ||
| sudo docker rm -f "$CONTAINER_NAME" 2>/dev/null || true | ||
| sudo docker create --name "$CONTAINER_NAME" "$IMAGE_TAG" echo done | ||
| mkdir -p "$OUTPUT_DIR" | ||
| sudo docker cp \ | ||
| "$CONTAINER_NAME:/app/build/app/outputs/flutter-apk/app-release.apk" \ | ||
| "$OUTPUT_DIR/musly-release.apk" | ||
| sudo docker rm -f "$CONTAINER_NAME" |
There was a problem hiding this comment.
Use a unique container name to avoid cross-run collisions.
A fixed CONTAINER_NAME can break concurrent builds and may delete another run’s container.
Proposed change
-CONTAINER_NAME="musly-apk-build"
+CONTAINER_NAME="musly-apk-build-$$-$(date +%s)"
+cleanup() { sudo docker rm -f "$CONTAINER_NAME" >/dev/null 2>&1 || true; }
+trap cleanup EXIT
...
-sudo docker rm -f "$CONTAINER_NAME" 2>/dev/null || true
sudo docker create --name "$CONTAINER_NAME" "$IMAGE_TAG" echo done
...
-sudo docker rm -f "$CONTAINER_NAME"🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 18-18: Use semicolon or linefeed before 'done' (or quote to make it literal).
(SC1010)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build-release-apk.sh` around lines 9 - 23, The script uses a fixed
CONTAINER_NAME ("musly-apk-build") which can collide across concurrent runs;
change CONTAINER_NAME in build-release-apk.sh to a unique value (e.g., include
$$, a timestamp, or a generated UUID) and use that variable consistently for
docker create, docker cp and docker rm; also add a trap cleanup that removes the
container name on exit to avoid leftover containers (references: CONTAINER_NAME,
docker create --name, docker cp, docker rm -f).
Comment on lines
+41
to
+43
| RUN git clone --depth 1 --branch stable \ | ||
| https://github.com/flutter/flutter.git /opt/flutter \ | ||
| && git config --global --add safe.directory /opt/flutter |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What do Flutter release docs recommend for reproducible CI builds: pinning a specific Flutter version/tag vs using the moving "stable" branch?
💡 Result:
For reproducible CI builds, Flutter best practices strongly recommend pinning a specific Flutter version or tag rather than using the moving "stable" branch [1][2]. While the "stable" channel is the recommended choice for production app releases [3], it is a moving target that receives updates. Relying on it in CI can lead to non-deterministic builds where your environment changes unexpectedly between pipeline runs, potentially causing build failures or regressions due to underlying SDK changes [1][2]. To ensure reproducibility and stability in CI/CD pipelines, the following approaches are recommended: 1. Pinning a Specific Version: Explicitly define the Flutter version (e.g., 3.41.5) in your CI configuration or a version file [2][4]. This ensures that every build uses the exact same SDK environment, regardless of when the pipeline runs [1]. 2. Using Version Management Tools: Tools like FVM (Flutter Version Manager) are widely used to manage Flutter SDK versions per project [5][6]. You can pin your project to a specific version in a configuration file (such as.fvmrc), which your CI pipeline can then read to install the correct SDK version automatically [2]. 3. Matrix Testing: If you need to ensure compatibility across different versions, use a CI matrix strategy [2]. This allows you to test your application against multiple specific versions (e.g., current stable, next stable, and beta) in a single pipeline run, rather than relying on a single moving branch [2]. By pinning your version, you gain control over when to upgrade your SDK, allowing you to test changes in a controlled manner before updating your production pipeline [7][1].
Citations:
- 1: https://medium.com/@govindavaramsrikanth/unlocking-the-secrets-of-flutter-versioning-the-ultimate-guide-for-developers-770e60210391
- 2: https://startdebugging.net/2026/05/how-to-target-multiple-flutter-versions-from-one-ci-pipeline/
- 3: https://flutter.googlesource.com/mirrors/flutter/+/HEAD/docs/releases/Flutter-build-release-channels.md
- 4: https://www.freecodecamp.org/news/how-to-build-a-production-ready-flutter-ci-cd-pipeline-with-github-actions-quality-gates-environments-and-store-deployment/
- 5: https://pub.dev/packages/fvm
- 6: https://fvm.app/
- 7: https://docs.flutterflow.io/resources/projects/settings/flutterflow-version-management
Pin Flutter to an explicit version for reproducible release artifacts.
Using --branch stable creates non-deterministic builds since the stable branch updates over time. Flutter best practices explicitly recommend pinning to a specific version in CI/CD to ensure reproducibility and gain control over when to upgrade the SDK.
Proposed change
+ARG FLUTTER_VERSION=3.29.3
-RUN git clone --depth 1 --branch stable \
- https://github.com/flutter/flutter.git /opt/flutter \
+RUN git clone --depth 1 --branch ${FLUTTER_VERSION} \
+ https://github.com/flutter/flutter.git /opt/flutter \
&& git config --global --add safe.directory /opt/flutter📝 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
| RUN git clone --depth 1 --branch stable \ | |
| https://github.com/flutter/flutter.git /opt/flutter \ | |
| && git config --global --add safe.directory /opt/flutter | |
| ARG FLUTTER_VERSION=3.29.3 | |
| RUN git clone --depth 1 --branch ${FLUTTER_VERSION} \ | |
| https://github.com/flutter/flutter.git /opt/flutter \ | |
| && git config --global --add safe.directory /opt/flutter |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile.release` around lines 41 - 43, Replace the non-deterministic clone
of Flutter that uses --branch stable with a pinned SDK tag or commit so releases
are reproducible: update the RUN that clones into /opt/flutter to fetch a
specific release (e.g., use --branch vX.Y.Z or clone then git checkout
<tag_or_commit>) and keep the git config --global --add safe.directory
/opt/flutter; choose and document the exact Flutter tag/commit to pin (replace
"stable" with the chosen tag in the RUN command or add an explicit git checkout
after clone).
Comment on lines
111
to
+178
| Future<void> _downloadAlbum() async { | ||
| if (_songs.isEmpty) return; | ||
|
|
||
| final subsonicService = Provider.of<SubsonicService>( | ||
| context, | ||
| listen: false, | ||
| ); | ||
| if (_songs.isEmpty || _album == null) return; | ||
| final offlineService = OfflineService(); | ||
| final subsonicService = Provider.of<SubsonicService>(context, listen: false); | ||
| await offlineService.initialize(); | ||
|
|
||
| setState(() => _isDownloading = true); | ||
|
|
||
| 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.queuePlaylistDownload(_album!.id, _songs, subsonicService); | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text('Downloading ${_songs.length} songs in background…'), | ||
| content: Text('Queued ${_songs.length} songs for download…'), | ||
| duration: const Duration(seconds: 2), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| Future<void> _cancelDownload() async { | ||
| if (_album == null) return; | ||
| await OfflineService().cancelPlaylistDownload(_album!.id); | ||
| } | ||
|
|
||
| Future<void> _removeDownloads() async { | ||
| if (_songs.isEmpty || _album == null) return; | ||
| final confirmed = await showDialog<bool>( | ||
| context: context, | ||
| builder: (ctx) => AlertDialog( | ||
| title: const Text('Remove downloads?'), | ||
| content: Text('Remove all ${_songs.length} downloaded songs from "${_album!.name}"?'), | ||
| actions: [ | ||
| TextButton(onPressed: () => Navigator.pop(ctx, false), child: const Text('Cancel')), | ||
| TextButton( | ||
| onPressed: () => Navigator.pop(ctx, true), | ||
| child: const Text('Remove', style: TextStyle(color: Colors.red)), | ||
| ), | ||
| ], | ||
| ), | ||
| ); | ||
| if (confirmed == true && mounted) { | ||
| await OfflineService().cancelPlaylistDownload(_album!.id); | ||
| await OfflineService().deletePlaylistDownloads(_songs); | ||
| } | ||
| } | ||
|
|
||
| Widget _buildDownloadButton(BuildContext context) { | ||
| if (_allDownloaded) { | ||
| return IconButton( | ||
| tooltip: 'Downloaded — tap to remove', | ||
| onPressed: _removeDownloads, | ||
| icon: const Icon(Icons.cloud_done, color: Colors.green), | ||
| ); | ||
| } | ||
| if (_isQueued) { | ||
| return IconButton( | ||
| tooltip: 'Downloading — tap to cancel', | ||
| onPressed: _cancelDownload, | ||
| icon: const SizedBox( | ||
| width: 20, | ||
| height: 20, | ||
| child: CircularProgressIndicator(strokeWidth: 2), | ||
| ), | ||
| ); | ||
| } | ||
| return IconButton( | ||
| tooltip: 'Download album', | ||
| onPressed: _downloadAlbum, | ||
| icon: const Icon(CupertinoIcons.cloud_download), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Localize the new album download strings.
The queued snackbar, remove-downloads dialog, and tooltips are all hardcoded English. Since this screen already depends on AppLocalizations, these new states should use localized strings too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/screens/album_screen.dart` around lines 111 - 178, Replace hardcoded
English UI strings in _downloadAlbum, _removeDownloads, and _buildDownloadButton
with localized values from AppLocalizations (e.g. call
AppLocalizations.of(context) inside these methods/builders and use resource keys
instead of literal text); specifically localize the SnackBar message in
_downloadAlbum, the AlertDialog title/content and button labels in
_removeDownloads, and all IconButton tooltip texts in _buildDownloadButton,
adding new localization keys
(queuedSongs/removeDownloadsTitle/removeDownloadsContent/cancelLabel/removeLabel/downloadedTooltip/downloadingTooltip/downloadAlbumTooltip
or similar) to your ARB/locale files and use those keys via AppLocalizations in
the referenced methods.
Comment on lines
+649
to
650
| await OfflineService().cancelPlaylistDownload(item.id); | ||
| await libraryProvider.deletePlaylist(item.id); |
There was a problem hiding this comment.
Do not block playlist deletion when download cancel fails.
Line 649 can fail independently, and then Line 650 never runs. That makes playlist deletion fragile if offline cleanup errors.
Proposed fix
- try {
- await OfflineService().cancelPlaylistDownload(item.id);
- await libraryProvider.deletePlaylist(item.id);
+ try {
+ try {
+ await OfflineService().cancelPlaylistDownload(item.id);
+ } catch (_) {
+ // Best-effort cancel; continue deleting playlist.
+ }
+ await libraryProvider.deletePlaylist(item.id);
if (context.mounted) {
ScaffoldMessenger.of(context).showSnackBar(📝 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
| await OfflineService().cancelPlaylistDownload(item.id); | |
| await libraryProvider.deletePlaylist(item.id); | |
| try { | |
| try { | |
| await OfflineService().cancelPlaylistDownload(item.id); | |
| } catch (_) { | |
| // Best-effort cancel; continue deleting playlist. | |
| } | |
| await libraryProvider.deletePlaylist(item.id); | |
| if (context.mounted) { | |
| ScaffoldMessenger.of(context).showSnackBar( |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/screens/library_screen.dart` around lines 649 - 650, The call to
OfflineService().cancelPlaylistDownload(item.id) can throw and prevent
libraryProvider.deletePlaylist(item.id) from running; wrap the
cancelPlaylistDownload call in a try/catch that catches and logs the error
(including context and item.id) but does not rethrow, then always call
libraryProvider.deletePlaylist(item.id) so playlist deletion proceeds regardless
of offline cleanup failure.
Comment on lines
303
to
+371
| Future<void> _downloadPlaylist() async { | ||
| final songs = _playlist?.songs; | ||
| if (songs == null || songs.isEmpty) return; | ||
|
|
||
| final subsonicService = Provider.of<SubsonicService>( | ||
| context, | ||
| listen: false, | ||
| ); | ||
| final offlineService = OfflineService(); | ||
| final subsonicService = Provider.of<SubsonicService>(context, listen: false); | ||
| await offlineService.initialize(); | ||
|
|
||
| setState(() => _isDownloading = true); | ||
|
|
||
| offlineService.startBackgroundDownload(songs, subsonicService).then((_) { | ||
| if (mounted) { | ||
| setState(() => _isDownloading = false); | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text( | ||
| 'Downloaded ${songs.length} songs from ${_playlist!.name}', | ||
| ), | ||
| duration: const Duration(seconds: 3), | ||
| ), | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| offlineService.queuePlaylistDownload(widget.playlistId, songs, subsonicService); | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text('Downloading ${songs.length} songs in background…'), | ||
| content: Text('Queued ${songs.length} songs for download…'), | ||
| duration: const Duration(seconds: 2), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| Future<void> _cancelDownload() async { | ||
| await OfflineService().cancelPlaylistDownload(widget.playlistId); | ||
| } | ||
|
|
||
| Future<void> _removeDownloads() async { | ||
| final songs = _playlist?.songs ?? []; | ||
| if (songs.isEmpty) return; | ||
| final confirmed = await showDialog<bool>( | ||
| context: context, | ||
| builder: (ctx) => AlertDialog( | ||
| title: const Text('Remove downloads?'), | ||
| content: Text('Remove all ${songs.length} downloaded songs from "${_playlist!.name}"?'), | ||
| actions: [ | ||
| TextButton(onPressed: () => Navigator.pop(ctx, false), child: const Text('Cancel')), | ||
| TextButton( | ||
| onPressed: () => Navigator.pop(ctx, true), | ||
| child: const Text('Remove', style: TextStyle(color: Colors.red)), | ||
| ), | ||
| ], | ||
| ), | ||
| ); | ||
| if (confirmed == true && mounted) { | ||
| await OfflineService().cancelPlaylistDownload(widget.playlistId); | ||
| await OfflineService().deletePlaylistDownloads(songs); | ||
| } | ||
| } | ||
|
|
||
| Widget _buildDownloadButton(BuildContext context) { | ||
| if (_allDownloaded) { | ||
| return IconButton( | ||
| tooltip: 'Downloaded — tap to remove', | ||
| onPressed: _removeDownloads, | ||
| icon: const Icon(Icons.cloud_done, color: Colors.green), | ||
| ); | ||
| } | ||
| if (_isQueued) { | ||
| return IconButton( | ||
| tooltip: 'Downloading — tap to cancel', | ||
| onPressed: _cancelDownload, | ||
| icon: const SizedBox( | ||
| width: 20, | ||
| height: 20, | ||
| child: CircularProgressIndicator(strokeWidth: 2), | ||
| ), | ||
| ); | ||
| } | ||
| return IconButton( | ||
| tooltip: 'Download playlist', | ||
| onPressed: _downloadPlaylist, | ||
| icon: const Icon(CupertinoIcons.cloud_download), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Localize the new download UI copy.
These snackbars, dialog labels, and tooltips are hardcoded in English even though this screen already uses AppLocalizations. They will stay untranslated in non-English locales.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/screens/playlist_screen.dart` around lines 303 - 371, Replace hardcoded
English UI strings in _downloadPlaylist, _removeDownloads, _cancelDownload
(SnackBar text, AlertDialog title/content/button labels) and the tooltips in
_buildDownloadButton with localized values from AppLocalizations (e.g., final
loc = AppLocalizations.of(context)!). Use loc.* getters for the snackbar message
("Queued X songs for download…"), the dialog title/content and button labels
("Remove downloads?", "Cancel", "Remove"), and the three tooltips ("Downloaded —
tap to remove", "Downloading — tap to cancel", "Download playlist"); ensure
pluralization or interpolation uses loc methods if available and pass
songs.length into the localized string where needed.
Comment on lines
+718
to
+756
| Widget _buildPlaylistStatusRow() { | ||
| return ValueListenableBuilder<Set<String>>( | ||
| valueListenable: _offlineService.downloadedSongIds, | ||
| builder: (context, ids, _) { | ||
| return ListTile( | ||
| contentPadding: const EdgeInsets.symmetric(horizontal: 16, vertical: 4), | ||
| leading: Container( | ||
| width: 32, | ||
| height: 32, | ||
| decoration: BoxDecoration( | ||
| gradient: const LinearGradient( | ||
| colors: [Color(0xFF5856D6), Color(0xFF7B68EE)], | ||
| ), | ||
| borderRadius: BorderRadius.circular(8), | ||
| ), | ||
| child: const Icon( | ||
| CupertinoIcons.music_note_list, | ||
| color: Colors.white, | ||
| size: 18, | ||
| ), | ||
| ), | ||
| title: const Text('Playlist Downloads', style: TextStyle(fontSize: 16)), | ||
| subtitle: Text( | ||
| '${ids.length} songs downloaded', | ||
| style: TextStyle( | ||
| fontSize: 12, | ||
| color: _isDark ? AppTheme.darkSecondaryText : AppTheme.lightSecondaryText, | ||
| ), | ||
| ), | ||
| trailing: const Icon(CupertinoIcons.chevron_right, size: 16), | ||
| onTap: () => Navigator.of(context).push( | ||
| MaterialPageRoute( | ||
| builder: (_) => const DownloadPlaylistStatusScreen(), | ||
| ), | ||
| ), | ||
| ); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Localize user-facing strings for i18n consistency.
The "Playlist Downloads" title (line 739) and "songs downloaded" text (line 741) are hardcoded in English, inconsistent with the AppLocalizations pattern used throughout this file.
🌐 Proposed fix to add localization
Add the following keys to AppLocalizations:
playlistDownloads→ "Playlist Downloads"songsDownloaded(int count)→ returns e.g., "5 songs downloaded"
Then update the code:
- title: const Text('Playlist Downloads', style: TextStyle(fontSize: 16)),
+ title: Text(AppLocalizations.of(context)!.playlistDownloads, style: const TextStyle(fontSize: 16)),
subtitle: Text(
- '${ids.length} songs downloaded',
+ AppLocalizations.of(context)!.songsDownloaded(ids.length),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/screens/settings_storage_tab.dart` around lines 718 - 756, Localize the
hardcoded strings in _buildPlaylistStatusRow: replace the title 'Playlist
Downloads' with AppLocalizations.of(context).playlistDownloads and the subtitle
'${ids.length} songs downloaded' with the pluralized/localized call
AppLocalizations.of(context).songsDownloaded(ids.length) (or your project's
plural API), and add the two keys to AppLocalizations: playlistDownloads and
songsDownloaded(int count); keep the rest of the ListTile (leading, trailing,
navigation to DownloadPlaylistStatusScreen, and usage of
_offlineService.downloadedSongIds) unchanged.
Comment on lines
+404
to
406
| // Use /download (original file, no transcoding) so size matches song.size | ||
| final url = subsonicService.getDownloadUrl(song.id); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if getDownloadUrl has Jellyfin/YouTube handling like other URL methods
rg -n "getDownloadUrl|getStreamUrl" --type=dart lib/services/subsonic_service.dart -A 5Repository: dddevid/Musly
Length of output: 807
🏁 Script executed:
rg -n "getDownloadUrl" --type=dartRepository: dddevid/Musly
Length of output: 234
🏁 Script executed:
# Check the downloadSong method and its calling context
rg -n "downloadSong|startBackgroundDownload|_processQueue" --type=dart lib/services/offline_service.dart -B 3 -A 10Repository: dddevid/Musly
Length of output: 4302
🏁 Script executed:
# Find where queuePlaylistDownload is called
rg -n "queuePlaylistDownload" --type=dart -B 3 -A 3Repository: dddevid/Musly
Length of output: 1758
🏁 Script executed:
# Check if SubsonicService has _jellyfin and _youtube properties and how they're set
rg -n "_jellyfin|_youtube" --type=dart lib/services/subsonic_service.dart -B 1 -A 1 | head -60Repository: dddevid/Musly
Length of output: 2296
Add backend-specific handling to getDownloadUrl to match getStreamUrl pattern.
getDownloadUrl at line 425 in subsonic_service.dart builds a Subsonic /rest/download URL unconditionally, even when _jellyfin or _youtube is non-null. Other URL-building methods like getStreamUrl (lines 430–433) check for Jellyfin and YouTube backends first. Downloads will fail for Jellyfin and YouTube users without this fix.
Apply the same backend-specific pattern:
String getDownloadUrl(String songId) {
if (_jellyfin != null) return _jellyfin!.getDownloadUrl(songId);
if (_youtube != null) return _youtube!.getDownloadUrl(songId);
return _buildUrl('download', {'id': songId});
}Then implement getDownloadUrl on JellyfinService and YoutubeService.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/services/offline_service.dart` around lines 404 - 406, getDownloadUrl
currently always builds a Subsonic /rest/download URL which breaks
Jellyfin/YouTube backends; update SubsonicService.getDownloadUrl to mirror
getStreamUrl by returning _jellyfin!.getDownloadUrl(songId) if _jellyfin !=
null, returning _youtube!.getDownloadUrl(songId) if _youtube != null, and only
then falling back to _buildUrl('download', {'id': songId}); also add
corresponding getDownloadUrl implementations to JellyfinService and
YoutubeService so those backends return proper download URLs.
Comment on lines
+606
to
+619
| // One automatic retry pass for any failed songs | ||
| final toRetry = List<Song>.from(downloadState.value.failedSongs); | ||
| if (toRetry.isNotEmpty && _isBackgroundDownloadActive) { | ||
| debugPrint('Retrying ${toRetry.length} failed song(s)...'); | ||
| final retryFailed = <Song>[]; | ||
| for (final song in toRetry) { | ||
| if (!_isBackgroundDownloadActive) break; | ||
| final success = await downloadSong(song, subsonicService); | ||
| if (!success) retryFailed.add(song); | ||
| } | ||
| downloadState.value = downloadState.value.copyWith( | ||
| failedSongs: retryFailed, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Retry pass does not update downloadLog status.
The retry loop updates downloadState.failedSongs but doesn't call _updateLogEntry to reflect retry status in the download log. The UI showing per-song progress (via downloadLog) will still show these songs as "failed" even if they succeed on retry.
Proposed fix to update log entries on retry
final toRetry = List<Song>.from(downloadState.value.failedSongs);
if (toRetry.isNotEmpty && _isBackgroundDownloadActive) {
debugPrint('Retrying ${toRetry.length} failed song(s)...');
final retryFailed = <Song>[];
for (final song in toRetry) {
if (!_isBackgroundDownloadActive) break;
+ // Find the log entry index for this song
+ final logIdx = downloadLog.value.indexWhere((e) => e.song.id == song.id);
+ if (logIdx != -1) _updateLogEntry(logIdx, DownloadStatus.downloading);
final success = await downloadSong(song, subsonicService);
- if (!success) retryFailed.add(song);
+ if (!success) {
+ retryFailed.add(song);
+ } else if (logIdx != -1) {
+ _updateLogEntry(logIdx, DownloadStatus.done);
+ }
}
downloadState.value = downloadState.value.copyWith(
failedSongs: retryFailed,
);
}📝 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
| // One automatic retry pass for any failed songs | |
| final toRetry = List<Song>.from(downloadState.value.failedSongs); | |
| if (toRetry.isNotEmpty && _isBackgroundDownloadActive) { | |
| debugPrint('Retrying ${toRetry.length} failed song(s)...'); | |
| final retryFailed = <Song>[]; | |
| for (final song in toRetry) { | |
| if (!_isBackgroundDownloadActive) break; | |
| final success = await downloadSong(song, subsonicService); | |
| if (!success) retryFailed.add(song); | |
| } | |
| downloadState.value = downloadState.value.copyWith( | |
| failedSongs: retryFailed, | |
| ); | |
| } | |
| // One automatic retry pass for any failed songs | |
| final toRetry = List<Song>.from(downloadState.value.failedSongs); | |
| if (toRetry.isNotEmpty && _isBackgroundDownloadActive) { | |
| debugPrint('Retrying ${toRetry.length} failed song(s)...'); | |
| final retryFailed = <Song>[]; | |
| for (final song in toRetry) { | |
| if (!_isBackgroundDownloadActive) break; | |
| // Find the log entry index for this song | |
| final logIdx = downloadLog.value.indexWhere((e) => e.song.id == song.id); | |
| if (logIdx != -1) _updateLogEntry(logIdx, DownloadStatus.downloading); | |
| final success = await downloadSong(song, subsonicService); | |
| if (!success) { | |
| retryFailed.add(song); | |
| } else if (logIdx != -1) { | |
| _updateLogEntry(logIdx, DownloadStatus.done); | |
| } | |
| } | |
| downloadState.value = downloadState.value.copyWith( | |
| failedSongs: retryFailed, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/services/offline_service.dart` around lines 606 - 619, The retry loop
updates downloadState.failedSongs but never updates per-song entries in
downloadLog; modify the retry block around downloadSong(song, subsonicService)
in the same method so that after each attempt you call _updateLogEntry (or the
appropriate helper) for that song to set its status to "retrying" before the
attempt and then to "downloaded" on success or "failed" on final failure, and
ensure downloadState.value.failedSongs and downloadLog stay in sync (use Song
identity or song.id to locate the log entry and update progress/timestamps
accordingly); keep respecting _isBackgroundDownloadActive and only update the
log when the attempt completes.
Comment on lines
+423
to
+427
| /// Returns the /download URL for a song — always original file, no transcoding. | ||
| /// Use this for offline downloads so file size matches song.size exactly. | ||
| String getDownloadUrl(String songId) { | ||
| return _buildUrl('download', {'id': songId}); | ||
| } |
There was a problem hiding this comment.
Missing Jellyfin/YouTube backend handling.
Other URL methods like getStreamUrl (line 429-442) and getCoverArtUrl (line 392-421) check for _jellyfin and _youtube backends and delegate appropriately. This method unconditionally builds a Subsonic URL, which will fail for non-Subsonic backends.
Proposed fix to add backend checks
/// Returns the /download URL for a song — always original file, no transcoding.
/// Use this for offline downloads so file size matches song.size exactly.
String getDownloadUrl(String songId) {
+ if (_jellyfin != null) return _jellyfin!.getStreamUrl(songId);
+ if (_youtube != null) return _youtube!.getStreamUrl(songId);
return _buildUrl('download', {'id': songId});
}Note: Jellyfin and YouTube may not have a distinct "download" vs "stream" concept, so falling back to getStreamUrl preserves functionality. Consider adding dedicated download methods to those services if they have different download endpoints.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/services/subsonic_service.dart` around lines 423 - 427, getDownloadUrl
currently always returns a Subsonic download URL via _buildUrl('download', ...)
and omits backend checks; update getDownloadUrl to mirror the pattern used in
getStreamUrl/getCoverArtUrl: if _jellyfin is present delegate to _jellyfin
(e.g., call its stream/download helper or fallback to getStreamUrl(songId)), if
_youtube is present delegate to _youtube (or fallback to getStreamUrl(songId)),
otherwise return _buildUrl('download', {'id': songId}); keep references to
getDownloadUrl, getStreamUrl, getCoverArtUrl, _jellyfin, _youtube and _buildUrl
to locate the change.
Comment on lines
+192
to
+202
| if (OfflineService().downloadedSongIds.value.isNotEmpty) { | ||
| final offlinePath = OfflineService().getLocalCoverArtPathByCoverArtId(coverArt); | ||
| if (offlinePath != null) { | ||
| return Image.file( | ||
| File(offlinePath), | ||
| key: ValueKey('offline_natural_$coverArt'), | ||
| fit: BoxFit.contain, | ||
| errorBuilder: (ctx, err, stack) => _buildPlaceholder(isDark), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd lib/widgets && wc -l album_artwork.dartRepository: dddevid/Musly
Length of output: 80
🏁 Script executed:
cat -n lib/widgets/album_artwork.dart | sed -n '190,210p'Repository: dddevid/Musly
Length of output: 844
🏁 Script executed:
cat -n lib/widgets/album_artwork.dart | sed -n '243,260p'Repository: dddevid/Musly
Length of output: 716
🏁 Script executed:
find . -name "*.dart" -type f | xargs grep -l "class OfflineService"Repository: dddevid/Musly
Length of output: 93
🏁 Script executed:
find . -name "*.dart" -type f | xargs grep -l "downloadedSongIds"Repository: dddevid/Musly
Length of output: 246
🏁 Script executed:
cat -n lib/services/offline_service.dart | head -100Repository: dddevid/Musly
Length of output: 4201
🏁 Script executed:
cat -n lib/services/offline_service.dart | grep -A 5 -B 5 "downloadedSongIds"Repository: dddevid/Musly
Length of output: 4142
🏁 Script executed:
cat -n lib/widgets/album_artwork.dart | sed -n '180,225p'Repository: dddevid/Musly
Length of output: 2050
🏁 Script executed:
cat -n lib/widgets/album_artwork.dart | sed -n '230,280p'Repository: dddevid/Musly
Length of output: 2223
Widget won't refresh when offline artwork becomes available after downloads complete.
Lines 192 and 245 read downloadedSongIds.value directly without listening. When a download finishes and downloadedSongIds updates, this widget won't rebuild to display the newly-available local artwork unless forced by an external rebuild.
Wrap both blocks in ValueListenableBuilder<Set<String>> listening to OfflineService().downloadedSongIds:
Fix pattern
- if (OfflineService().downloadedSongIds.value.isNotEmpty) {
- final offlinePath = OfflineService().getLocalCoverArtPathByCoverArtId(coverArt);
+ return ValueListenableBuilder<Set<String>>(
+ valueListenable: OfflineService().downloadedSongIds,
+ builder: (context, _, __) {
+ final offlinePath = OfflineService()
+ .getLocalCoverArtPathByCoverArtId(coverArt);
if (offlinePath != null) {
return Image.file(
File(offlinePath),
...
);
}
- }
+ // fallback to network
+ },
+ );Apply to both _buildImageNatural and _buildImage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/widgets/album_artwork.dart` around lines 192 - 202, The widget currently
reads OfflineService().downloadedSongIds.value directly, so _buildImageNatural
and _buildImage won't rebuild when downloads complete; wrap each offline-art
check (the block that calls
OfflineService().getLocalCoverArtPathByCoverArtId(coverArt) and returns
Image.file with ValueKey('offline_natural_$coverArt') / similar key) in a
ValueListenableBuilder<Set<String>> that listens to
OfflineService().downloadedSongIds and returns the same Image.file (or
placeholder) based on the latest value so the widget rebuilds automatically when
downloadedSongIds updates.
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
Adds a full offline download system for playlists and albums, built on top of the existing
OfflineServiceskeleton.ValueNotifier<Set<String>>fields (queuedPlaylistIds,downloadedPlaylistIds) drive badges independently of individual song file presence, so cross-playlist songs don't create false positivescoverArtIdso album/playlist views can load artwork offlineSharedPreferencesand re-queued at startup if interruptedFiles changed
offline_service.dartplaylist_screen.dart/album_screen.dartplaylists_screen.dart/library_screen.dartsong_tile.dartalbum_artwork.dartdownload_playlist_status_screen.dartsettings_storage_tab.dartsubsonic_service.dartgetDownloadUrl()for original-file endpointSummary by CodeRabbit
New Features
Chores