feat(downloads): add format picker and custom download path support#3122
Open
alltechdev wants to merge 25 commits intoMetrolistGroup:mainfrom
Open
feat(downloads): add format picker and custom download path support#3122alltechdev wants to merge 25 commits intoMetrolistGroup:mainfrom
alltechdev wants to merge 25 commits intoMetrolistGroup:mainfrom
Conversation
|
What about a function to migrate songs from internal to external and backwards |
Contributor
Author
Ahhhh man... I dunno. Can probably be done. |
If it's too complex, leave it; I simply suggested it so that current users can migrate without having to download everything again |
Contributor
Author
Try out the PR lemme know what you think. Back up your stuff before if you'll want to go back to official after. |
Contributor
|
Possibility of adding FLAC support? |
When you first start the app and enable the custom download patch, it asks you for the location of the folder you want. I choose "song" for example, but I see that below where it says "select download folder" it stays as "notifications", the selection failed?. This might confuse some people, I think. |
Contributor
Author
|
@moonpic screenshots plz |
Forget the previous issue, i can't make it happen again.. Maybe I clicked on the wrong folder. Well, actually, everything works fine. I can only suggest features like quick download, so I don't have to choose the quality every time. And what about when I want to move files from cache to downloaded? I see I have to choose the quality there too (double download?). I thought about why it doesn't save the cache in a separate folder, but I think that's inefficient. |
|
I think embedding lyrics in songs would also be a good feature, a UI issue you'll see in the picture is the only bug I've found so far. |
|
"Albums" that are Singles don't include cover art. (But albums that include multiple songs, even if they're listed under Singles/EPs, do include album art). https://music.youtube.com/playlist?list=OLAK5uy_kqpF97OXK6tU7yOdXDDVas3ivhK2bKwEQ 
|
|
As someone mentioned on discord, tapping the Swap Download button immediately removes the download rather than waiting for a selection to actually be made. (Tapping "Swap" then "Cancel" leads to your download being deleted instead of being left alone.) |
alltechdev
force-pushed
the
feat/downloads_path-format
branch
from
868df29 to
e303a25
Compare
📝 WalkthroughWalkthroughAdds per-song download format selection and overrides, custom download-path export with native M4A metadata embedding, CI step to install a native coverart JNI library, DB additions for download URIs, new UI dialogs/menus for format picking, and supporting utilities and helpers. Changes
Sequence DiagramsequenceDiagram
participant User
participant Menu as Menu Screen
participant Dialog as DownloadFormatDialog
participant YTUtils as YTPlayerUtils
participant DownloadUtil as DownloadUtil
participant DB as Database
participant ExportHelper as DownloadExportHelper
participant SAF as Storage Access Framework
participant Embedder as CoverArtEmbedder
User->>Menu: request download
Menu->>YTUtils: getAllAvailableAudioFormats(videoId)
YTUtils-->>Menu: formats list
Menu->>Dialog: show formats
User->>Dialog: select format (itag)
Dialog->>DownloadUtil: setTargetItag(songId, itag)
Menu->>DownloadUtil: enqueue download
DownloadUtil->>DB: persist download state
DownloadUtil->>DownloadUtil: download using targetItag / caches
DownloadUtil->>DownloadUtil: clear targetItag
alt custom path enabled
DownloadUtil->>ExportHelper: exportToCustomPath(songId, customPathUri)
ExportHelper->>SAF: create folders & write file
alt file is M4A >=128kbps
ExportHelper->>Embedder: embedMetadataIntoFile(fileUri,...)
Embedder->>Embedder: call native embedMetadata()
Embedder-->>ExportHelper: success/failure
end
ExportHelper->>DB: update song.downloadUri
end
DownloadUtil-->>User: download/export complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
- Add format picker dialog when downloading songs (shows bitrate/codec) - Add custom download path support via SAF (Storage Access Framework) - Export downloads to user-selected folder with Artist/Title structure - Integrate Bento4 native library for M4A metadata embedding (cover art, title, artist, album, year) - Add "swap download" option to re-download with different quality - Add database schema migration for downloadUri tracking - Only embed metadata for M4A files >= 128kbps (low bitrate compatibility) - Show metadata badge on format options that support embedding
- Use downloadUri directly for playback instead of ExoPlayer downloadCache to prevent format mismatch errors (MatroskaExtractor parsing M4A data) - Check targetItag before cache in DownloadUtil to respect user format selection - Clear playerCache when specific format requested to ensure fresh fetch - Fix URL cache expiry check (was using wrong comparison operator) - Reorder ExtractorsFactory: FragmentedMp4Extractor first for M4A files - Add detailed logging for download and cache resolution debugging
- Move Bento4 library and JNI wrapper to metrolist-coverart-lib repository - Replace embedded code with git submodule reference - This reduces main repository size by ~77,000 lines - Library can now be independently versioned and updated Submodule: https://github.com/MetrolistGroup/metrolist-coverart-lib
- Embed album artist (aART) and track number (trkn) in M4A files - Join all song artists for the Artist field (e.g., "Lady Gaga, Bruno Mars") - Fetch missing metadata (year, album artist, track position) from YouTube - Update native JNI and Kotlin wrapper signatures
…style - Add header icon in circular container - Add quality legend indicators (High/Medium/Low) - Use card-based format items with radio buttons - Add BEST badge for highest quality option - Add + Metadata badge for M4A formats - Improve visual hierarchy and spacing
…e build - Add download format picker dialog to YouTubeSongMenu - Add ProGuard keep rules for CoverArtNative JNI bindings
- Keep CoverArtEmbedder and DownloadExportHelper classes - Add swap download option to YouTubeSongMenu
- Add download format picker dialog when downloading albums - Add swap download option to re-download with different format - Use buildList pattern for download menu items
Songs downloaded before the custom path feature was added don't have downloadUri set. Add fallback to check ExoPlayer download cache for these legacy downloads to ensure they still play after updating.
WebM audio files now export with .ogg extension for better compatibility with offline music players. No quality loss - same Opus codec data, just different container extension that's more widely recognized.
Extract full path from document URI (e.g., "Music/Metrolist" instead of just "Metrolist") for clearer path display in storage settings.
- Update YTPlayerUtils to use new PoTokenGenerator.generateContentToken API - Fix schema 34.json to include downloadUri field
- Fix race condition in metadata embedding by making format storage synchronous - Add artist relationship creation from player response for proper folder organization - Add format picker dialog for playlist downloads (local and YouTube) - Fix format fallback to prefer same codec type when exact itag unavailable - Fix state updates happening on wrong thread in AlbumMenu
- Remove coverart submodule and cpp directory - Add setup-coverart action to download prebuilt library from nightly.link - Update all workflows to use the new action - Add setup_coverart.sh script for local development - Update .gitignore to exclude jniLibs directory
alltechdev
force-pushed
the
feat/downloads_path-format
branch
from
e303a25 to
9f4cf39
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt (1)
541-573:⚠️ Potential issue | 🟠 MajorSwap download removes existing downloads before user selects format.
Same issue as in
YouTubeSongMenu.kt: the swap download flow removes all existing song downloads (lines 549-556) before the user has selected a new format. If the user cancels the format picker dialog, all album song downloads are already deleted.Defer the removal until after the user commits to a new format in
onFormatSelected.Proposed fix approach
onClick = { if (songs.isEmpty()) return@Material3MenuItemData Timber.tag("AlbumMenu").d("Swap download clicked for album") showDownloadFormatDialog = true isLoadingFormats = true availableFormats = emptyList() coroutineScope.launch(Dispatchers.IO) { - // Remove all existing downloads first - songs.forEach { song -> - DownloadService.sendRemoveDownload( - context, - ExoDownloadService::class.java, - song.id, - false, - ) - } // Fetch formats from first song try {Then move the removal logic to
onFormatSelectedin theDownloadFormatDialog, ensuring downloads are only removed when a replacement is committed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt` around lines 541 - 573, The swap-download onClick in AlbumMenu currently removes existing downloads (DownloadService.sendRemoveDownload called for each song) before the user picks a format; move that removal logic out of the onClick coroutine and into the DownloadFormatDialog's onFormatSelected handler so removals only happen after the user commits to a new format. Specifically: stop calling DownloadService.sendRemoveDownload inside AlbumMenu.onClick (keep showing the dialog and fetching formats via YTPlayerUtils.getAllAvailableAudioFormats and setting availableFormats/isLoadingFormats as now), then implement the per-song removal loop and subsequent replacement logic inside DownloadFormatDialog.onFormatSelected so it runs only after a format is chosen/confirmed. Ensure you reference the same song ids (song.id) and use ExoDownloadService when performing removals in the new location.
🧹 Nitpick comments (7)
app/src/main/kotlin/com/metrolist/music/utils/DataStore.kt (1)
55-65: Prefer the existing generic preference delegate.
preference()already handlesPreferences.Key<T>, so these two helpers only duplicate the same lookup logic and create another place for behavior to drift. Reusing the generic helper keeps this API smaller and future changes centralized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/utils/DataStore.kt` around lines 55 - 65, The booleanPreference and stringPreference helpers duplicate lookup logic; update them to reuse the existing generic preference() delegate instead of duplicating behavior. Change booleanPreference(context, key, defaultValue) to return preference(context, key, defaultValue) and likewise for stringPreference, or remove these functions and update callers to call preference(context, key, defaultValue) directly so all preference handling is centralized in preference().app/src/main/kotlin/com/metrolist/music/ui/component/DownloadFormatDialog.kt (3)
218-224: Redundantclipmodifier withshapeparameter.The
Surfacealready applies the shape via itsshapeparameter, making the.clip(RoundedCornerShape(12.dp))modifier redundant.Suggested fix
Surface( modifier = Modifier .fillMaxWidth() - .clip(RoundedCornerShape(12.dp)) .clickable(onClick = onClick), color = MaterialTheme.colorScheme.surfaceContainerHigh, shape = RoundedCornerShape(12.dp) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/component/DownloadFormatDialog.kt` around lines 218 - 224, Remove the redundant clip modifier: inside the Surface composable that sets shape = RoundedCornerShape(12.dp) and uses Modifier.fillMaxWidth().clip(RoundedCornerShape(12.dp)).clickable(onClick = onClick), delete the .clip(RoundedCornerShape(12.dp)) call so the Modifier becomes Modifier.fillMaxWidth().clickable(onClick = onClick) and let Surface's shape parameter handle clipping for the Surface in DownloadFormatDialog.kt.
284-288: Consider extracting codec-to-container mapping.The hardcoded string comparison
format.codec == "OPUS"to determine the container type ("WebM" vs "M4A") could be fragile if codec naming conventions change. Consider moving this logic toAudioFormatOptionitself or a dedicated extension function for better maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/component/DownloadFormatDialog.kt` around lines 284 - 288, The inline codec check in DownloadFormatDialog's Text (using format.codec == "OPUS" ? "WebM" : "M4A") should be moved into the model layer—add a property or extension on AudioFormatOption (e.g., AudioFormatOption.containerLabel or fun AudioFormatOption.toContainerLabel()) that returns the correct container string for the codec, implement the mapping logic there, and then replace the inline logic in DownloadFormatDialog (where Text references format.codec and container) to use the new property/method (e.g., format.containerLabel) so the mapping is centralized and maintainable.
149-151: Fixed height may not adapt well to all screen sizes.The
LazyColumnhas a fixed height of280.dp. On smaller screens or when many formats are available, this could result in excessive scrolling or clipped content. Consider usingheightIn(max = 280.dp)combined withwrapContentHeight()to allow the list to shrink when fewer items are present.Suggested fix
// Format list LazyColumn( - modifier = Modifier.height(280.dp), + modifier = Modifier.heightIn(max = 280.dp), verticalArrangement = Arrangement.spacedBy(8.dp) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/component/DownloadFormatDialog.kt` around lines 149 - 151, In DownloadFormatDialog, the LazyColumn currently uses a fixed Modifier.height(280.dp) which can cause poor layout on small screens or when items are few; replace that fixed height with a flexible constraint by using Modifier.heightIn(max = 280.dp) combined with wrapContentHeight() (and keep any other modifiers like padding) so the list will cap at 280.dp but shrink to fit its content when fewer items are present.app/setup_coverart.sh (1)
18-28: Add error handling for extraction and document working directory requirement.The script lacks error handling for the
unzipcommand and doesn't verify the extraction succeeded before reporting success. Also, the script assumes execution from theapp/directory due to the relative pathsrc/main/jniLibs.Suggested improvements
+# Ensure we're in the app directory +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +cd "$SCRIPT_DIR" || exit 1 + # Create jniLibs directory mkdir -p src/main/jniLibs # Extract -unzip -o /tmp/libcoverart-jniLibs.zip -d src/main/jniLibs +if ! unzip -o /tmp/libcoverart-jniLibs.zip -d src/main/jniLibs; then + echo "Failed to extract library" + rm -f /tmp/libcoverart-jniLibs.zip + exit 1 +fi # Cleanup rm /tmp/libcoverart-jniLibs.zip🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/setup_coverart.sh` around lines 18 - 28, The script currently assumes a specific working directory and doesn't check unzip success; update it to first resolve the script directory (use $0) or require/validate execution from app/ so that src/main/jniLibs is correct, verify /tmp/libcoverart-jniLibs.zip exists before proceeding, run unzip and check its exit status (or use a safe fail-on-error approach) and only run rm /tmp/libcoverart-jniLibs.zip and print the success message if unzip succeeded, otherwise print an error to stderr and exit non‑zero; reference the mkdir -p src/main/jniLibs, unzip -o /tmp/libcoverart-jniLibs.zip -d src/main/jniLibs, rm /tmp/libcoverart-jniLibs.zip and the final find src/main/jniLibs -name "*.so" commands when making these changes..github/actions/setup-coverart/action.yml (1)
7-25: Add error handling to prevent silent failures.The action lacks error handling for the
curldownload. If the download fails (e.g., network issue, HTTP error), the action continues and fails later with a confusing error. Without the-fflag, curl succeeds even on HTTP errors like 404, saving an error page as the zip file—unzip then fails with a misleading message about file corruption.Suggested improvements
- name: Download coverart library run: | + set -e echo "Downloading coverart library..." # Use nightly.link to get public URL for the artifact DOWNLOAD_URL="https://nightly.link/MetrolistGroup/metrolist-coverart-lib/workflows/build-release/main/libcoverart-jniLibs.zip" - curl -L -o /tmp/libcoverart-jniLibs.zip "$DOWNLOAD_URL" + curl -fL -o /tmp/libcoverart-jniLibs.zip "$DOWNLOAD_URL"Add
set -eto exit on any failure and-fto curl so it fails with exit code 22 on HTTP errors (4xx/5xx).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/setup-coverart/action.yml around lines 7 - 25, The GitHub Action step that downloads the coverart library should stop on failures and make curl fail on HTTP errors: add a shell safety flag (e.g., invoke set -e at the top of the run block) and add the -f flag to the curl invocation that uses DOWNLOAD_URL so curl returns a non‑zero exit on 4xx/5xx; keep the existing unzip/cleanup steps but rely on set -e to abort early if curl or unzip fails and surface the real error instead of continuing with a corrupted zip.app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt (1)
91-131: Trim the narration comments.Most of these comments just restate the next statement; the non-obvious SAF/cache notes are the ones worth keeping.
As per coding guidelines, "Comments should be added only for complex logic or non-obvious code. Avoid redundant comments that simply restate what the code does."
Also applies to: 234-234, 278-278, 335-347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt` around lines 91 - 131, The comments around the Storage Access Framework file/folder creation and file handling are redundant and should be trimmed: remove simple narrations that restate nearby statements (e.g., comments before the artistFolder/albumFolder creation, existingFile deletion, and newFile creation) and keep only the non-obvious notes about SAF/cache behavior; update the block manipulating artistFolder (findFile/createDirectory), albumFolder (findFile/createDirectory), existingFile.delete(), and newFile (createFile) to retain any explanatory comments about SAF or caching quirks but delete tautological lines like "Create or get artist subfolder", "Create or get album subfolder inside artist folder", "Check if file already exists...", "Create the new file in album folder", and similar redundant comments elsewhere (including the other referenced locations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/DownloadUtil.kt`:
- Around line 71-85: Persist the user-selected itag instead of keeping it only
in the in-memory map targetItagOverride: update setTargetItag to write the
chosen itag into the download metadata/DB (e.g., the DownloadRecord for songId)
and still call invalidateUrl(songId), and at app startup rebuild
targetItagOverride from the persisted download records so resolver logic that
reads targetItag (and currently falls back when targetItag == 0) will find the
persisted value; ensure wherever targetItagOverride is read (resolver path
around the fallback at line ~172) prefers the persisted value and that
removals/clears update both the DB record and the in-memory map.
- Around line 145-148: The cache check in Factory correctly treats songUrlCache
values as absolute timestamps, but the code that populates songUrlCache still
stores only streamExpiresInSeconds * 1000L; update the population logic to store
an absolute expiry by adding the current epoch millis (e.g.,
System.currentTimeMillis() + streamExpiresInSeconds * 1000L) when inserting into
songUrlCache so the takeIf { it.second > System.currentTimeMillis() } check
works and cached URLs are reused; keep the existing songUrlCache key and value
shape so dataSpec.withUri(...) and the Factory lookup remain unchanged.
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 2885-2895: The resolver currently only uses song.song.downloadUri
when dataSpec.position == 0, which causes seeks/resumes on downloaded tracks to
bypass the local file; remove the position check so the Factory branch that
checks song?.song?.downloadUri uses the downloadUri regardless of
dataSpec.position. In the MusicService Factory block (the condition that
triggers recoverSong(mediaId) and returns dataSpec.withUri(...)), change the
gate to only test for song?.song?.downloadUri != null, keep the recoverSong
launch and dataSpec.withUri return as-is so offset handling remains correct, and
ensure this allows the later downloadCache fallback to remain reachable only
when downloadUri == null.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt`:
- Around line 464-496: Replace the hardcoded English labels in the
Material3MenuItemData title lambda with localized resources: use
stringResource(...) instead of literal "Unpin from Speed dial" / "Pin to Speed
dial" and add corresponding string keys (e.g. R.string.unpin_from_speed_dial and
R.string.pin_to_speed_dial) in resources; update the title expression that
checks isPinned and returns stringResource(if (isPinned)
R.string.unpin_from_speed_dial else R.string.pin_to_speed_dial) so the menu uses
i18n-compatible strings in AlbumMenu.kt.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/PlaylistMenu.kt`:
- Around line 498-530: Replace the hardcoded English labels inside the
Material3MenuItemData title lambda with localized resources: call
stringResource(R.string.pin_to_speed_dial) when !isPinned and
stringResource(R.string.unpin_from_speed_dial) when isPinned (import
androidx.compose.ui.res.stringResource), and add the corresponding keys to
strings.xml; update the Text(...) call to use these stringResource values so the
pin/unpin UI is internationalized.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/SongMenu.kt`:
- Around line 900-924: The current onClick handler calls
DownloadService.sendRemoveDownload(song.id) before the user selects a
replacement format and before getAllAvailableAudioFormats(...) succeeds, causing
data loss if the dialog is canceled or loading fails; instead remove the
existing download only after the user has successfully picked and confirmed a
new format. Move the call to DownloadService.sendRemoveDownload out of the
initial coroutine and into the confirmation/commit path (the code that processes
the selected format), leaving the current flow to only set
showDownloadFormatDialog = true, set isLoadingFormats = true, and populate
availableFormats from YTPlayerUtils.getAllAvailableAudioFormats(song.id); ensure
onFailure keeps the existing download intact and that the commit handler (where
the new download is created) invokes DownloadService.sendRemoveDownload(…,
song.id, false) immediately before creating the replacement download.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.kt`:
- Around line 642-664: Currently the onClick handler in YouTubeSongMenu
immediately calls DownloadService.sendRemoveDownload before the user chooses a
new format, which deletes the download even if the dialog is cancelled; remove
that call from the onClick coroutine and instead defer deletion to the format
selection flow by invoking DownloadService.sendRemoveDownload inside the
onFormatSelected callback (the place where the new download is queued), so the
old download is only removed after a successful format choice and enqueue. Also
ensure showDownloadFormatDialog and isLoadingFormats behavior is preserved (set
isLoadingFormats while loading formats, and clear it on success/failure) and
that onFormatSelected performs both queuing the new download and then removing
the old one (using song.id) only after enqueue succeeds.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/StorageSettings.kt`:
- Around line 137-159: Move the URI save and enable logic so it only runs after
a successful SAF grant: inside the folderPickerLauncher callback, call
context.contentResolver.takePersistableUriPermission(selectedUri, takeFlags) in
the try block and then call
onCustomDownloadPathUriChange(selectedUri.toString()) and set customPathValid =
true (and enable the toggle) only after that succeeds; before taking the new
permission, if there is an existing persisted URI saved (the previous custom
path), call context.contentResolver.releasePersistableUriPermission(oldUri,
takeFlags) to release the old tree grant to avoid accumulation; ensure the UI
toggle enabling is deferred until after the try succeeds rather than being
toggled when the picker is launched.
In `@app/src/main/kotlin/com/metrolist/music/utils/CoverArtEmbedder.kt`:
- Around line 113-123: The final replace step is unsafe — instead of opening the
target (SAF) output directly or deleting the original before renameTo(),
implement a temp/backup-then-commit flow: for the SAF branch, create a temporary
SAF document/URI in the same directory (use DocumentFile.createFile or a sibling
temp URI), write the result to that temp via
context.contentResolver.openOutputStream(tempUri, "wt") (replace the current
openOutputStream(fileUri, "wt") usage), and on successful write atomically
replace the original by deleting the original and renaming/moving the temp;
ensure you handle failures by leaving the original intact and deleting the temp.
For the local-file branch (where renameTo() is used), write to a same-directory
temp File, on success attempt atomic replace with Files.move(temp, target,
REPLACE_EXISTING) or fallback to renameTo(), and if the move fails restore from
a backup copy; always clean up temp/backup files on both success and error.
Update the code paths around input.copyTo(output),
contentResolver.openOutputStream(fileUri, "wt"), and any File.renameTo() calls
in CoverArtEmbedder.kt to follow this pattern.
In `@app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt`:
- Around line 406-413: The sanitizeFilename function currently truncates the
entire filename (including extension) which can strip the file extension; change
sanitizeFilename to split the input into base name and extension (preserve the
dot + extension if present), sanitize each piece (apply Regex replacements and
trim) and then truncate only the base name to a safe length (e.g., 200 minus
extension length) before rejoining base + extension; update references to the
function accordingly and ensure you handle filenames with no extension and with
multiple dots correctly.
- Around line 115-127: Replace the non-atomic replacement flow by writing the
export to a temporary file first and only swapping it into place after a
successful copy and updateDownloadUri(): instead of calling
existingFile.delete() and then albumFolder.createFile(sanitizedFilename)
directly, create a temp document (e.g., createFile with a ".tmp" suffix or use a
temp name via albumFolder.createFile), write the data to that temp file, call
updateDownloadUri() for the temp, verify success, then delete/replace the old
file (existingFile.delete()) and rename/move the temp to the final
sanitizedFilename (or create the final file and copy from temp), ensuring any
failures return null without touching the original existingFile; apply the same
pattern for the other location mentioned around updateDownloadUri() and lines
~240-245.
- Around line 395-403: In getExtensionFromFormat(format: FormatEntity?), change
the mapping for MIME type "audio/webm" to return the correct container extension
("webm") instead of "ogg" (or alternatively implement remuxing before export if
you must keep ".ogg"); specifically update the when-branch that checks
format.mimeType.contains("audio/webm") so the function returns "webm" to match
the actual WebM container being copied in the export (or document and call a
remux routine prior to writing the cached payload).
- Around line 141-163: The export currently concatenates cached spans from
downloadCache.getCachedSpans(songId) without verifying contiguity, which can
produce incomplete files; before iterating sortedSpans in DownloadExportHelper
(before the for loop that processes each span and writes to newFile), fetch
content metadata via downloadCache.getContentMetadata(songId) and validate that:
(1) sortedSpans.first().position == 0, (2) each span.position equals the
previous span.position + previous span.length (i.e., spans are contiguous), and
(3) the sum of span.length equals the expected content length from the metadata;
if any check fails, delete newFile and return null (as the existing error path
does) so you don't persist a downloadUri for a corrupt/incomplete export.
In `@app/src/main/kotlin/com/metrolist/music/utils/YTPlayerUtils.kt`:
- Around line 397-425: The current fallback in YTPlayerUtils.kt (around
variables targetItag, allFormats, requestedFormat,
isM4aRequested/isOpusRequested) always picks the highest-bitrate m4a/opus
variant (m4aFormat/opusFormat), which can silently upgrade quality; change the
logic to pick the format whose bitrate is closest to the originally requested
format: if requestedFormat != null compute requestedBitrate =
requestedFormat.bitrate and select the candidate format with minimal
abs(candidate.bitrate - requestedBitrate) for the same codec, otherwise do not
auto-upgrade—return null or bubble the miss to the caller so the caller can
decide (rather than always returning the max-bitrate variant).
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt`:
- Around line 541-573: The swap-download onClick in AlbumMenu currently removes
existing downloads (DownloadService.sendRemoveDownload called for each song)
before the user picks a format; move that removal logic out of the onClick
coroutine and into the DownloadFormatDialog's onFormatSelected handler so
removals only happen after the user commits to a new format. Specifically: stop
calling DownloadService.sendRemoveDownload inside AlbumMenu.onClick (keep
showing the dialog and fetching formats via
YTPlayerUtils.getAllAvailableAudioFormats and setting
availableFormats/isLoadingFormats as now), then implement the per-song removal
loop and subsequent replacement logic inside
DownloadFormatDialog.onFormatSelected so it runs only after a format is
chosen/confirmed. Ensure you reference the same song ids (song.id) and use
ExoDownloadService when performing removals in the new location.
---
Nitpick comments:
In @.github/actions/setup-coverart/action.yml:
- Around line 7-25: The GitHub Action step that downloads the coverart library
should stop on failures and make curl fail on HTTP errors: add a shell safety
flag (e.g., invoke set -e at the top of the run block) and add the -f flag to
the curl invocation that uses DOWNLOAD_URL so curl returns a non‑zero exit on
4xx/5xx; keep the existing unzip/cleanup steps but rely on set -e to abort early
if curl or unzip fails and surface the real error instead of continuing with a
corrupted zip.
In `@app/setup_coverart.sh`:
- Around line 18-28: The script currently assumes a specific working directory
and doesn't check unzip success; update it to first resolve the script directory
(use $0) or require/validate execution from app/ so that src/main/jniLibs is
correct, verify /tmp/libcoverart-jniLibs.zip exists before proceeding, run unzip
and check its exit status (or use a safe fail-on-error approach) and only run rm
/tmp/libcoverart-jniLibs.zip and print the success message if unzip succeeded,
otherwise print an error to stderr and exit non‑zero; reference the mkdir -p
src/main/jniLibs, unzip -o /tmp/libcoverart-jniLibs.zip -d src/main/jniLibs, rm
/tmp/libcoverart-jniLibs.zip and the final find src/main/jniLibs -name "*.so"
commands when making these changes.
In
`@app/src/main/kotlin/com/metrolist/music/ui/component/DownloadFormatDialog.kt`:
- Around line 218-224: Remove the redundant clip modifier: inside the Surface
composable that sets shape = RoundedCornerShape(12.dp) and uses
Modifier.fillMaxWidth().clip(RoundedCornerShape(12.dp)).clickable(onClick =
onClick), delete the .clip(RoundedCornerShape(12.dp)) call so the Modifier
becomes Modifier.fillMaxWidth().clickable(onClick = onClick) and let Surface's
shape parameter handle clipping for the Surface in DownloadFormatDialog.kt.
- Around line 284-288: The inline codec check in DownloadFormatDialog's Text
(using format.codec == "OPUS" ? "WebM" : "M4A") should be moved into the model
layer—add a property or extension on AudioFormatOption (e.g.,
AudioFormatOption.containerLabel or fun AudioFormatOption.toContainerLabel())
that returns the correct container string for the codec, implement the mapping
logic there, and then replace the inline logic in DownloadFormatDialog (where
Text references format.codec and container) to use the new property/method
(e.g., format.containerLabel) so the mapping is centralized and maintainable.
- Around line 149-151: In DownloadFormatDialog, the LazyColumn currently uses a
fixed Modifier.height(280.dp) which can cause poor layout on small screens or
when items are few; replace that fixed height with a flexible constraint by
using Modifier.heightIn(max = 280.dp) combined with wrapContentHeight() (and
keep any other modifiers like padding) so the list will cap at 280.dp but shrink
to fit its content when fewer items are present.
In `@app/src/main/kotlin/com/metrolist/music/utils/DataStore.kt`:
- Around line 55-65: The booleanPreference and stringPreference helpers
duplicate lookup logic; update them to reuse the existing generic preference()
delegate instead of duplicating behavior. Change booleanPreference(context, key,
defaultValue) to return preference(context, key, defaultValue) and likewise for
stringPreference, or remove these functions and update callers to call
preference(context, key, defaultValue) directly so all preference handling is
centralized in preference().
In `@app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt`:
- Around line 91-131: The comments around the Storage Access Framework
file/folder creation and file handling are redundant and should be trimmed:
remove simple narrations that restate nearby statements (e.g., comments before
the artistFolder/albumFolder creation, existingFile deletion, and newFile
creation) and keep only the non-obvious notes about SAF/cache behavior; update
the block manipulating artistFolder (findFile/createDirectory), albumFolder
(findFile/createDirectory), existingFile.delete(), and newFile (createFile) to
retain any explanatory comments about SAF or caching quirks but delete
tautological lines like "Create or get artist subfolder", "Create or get album
subfolder inside artist folder", "Check if file already exists...", "Create the
new file in album folder", and similar redundant comments elsewhere (including
the other referenced locations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abf34d3e-6c6f-4c54-9f8c-e38f7b0843c7
📒 Files selected for processing (29)
.github/actions/setup-coverart/action.yml.github/workflows/build.yml.github/workflows/build_pr.yml.github/workflows/build_quick.yml.github/workflows/release.yml.gitignoreapp/proguard-rules.proapp/schemas/com.metrolist.music.db.InternalDatabase/35.jsonapp/schemas/com.metrolist.music.db.InternalDatabase/36.jsonapp/setup_coverart.shapp/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.ktapp/src/main/kotlin/com/metrolist/music/db/DatabaseDao.ktapp/src/main/kotlin/com/metrolist/music/db/entities/SongEntity.ktapp/src/main/kotlin/com/metrolist/music/playback/DownloadUtil.ktapp/src/main/kotlin/com/metrolist/music/playback/MusicService.ktapp/src/main/kotlin/com/metrolist/music/ui/component/DownloadFormatDialog.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/PlayerMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/PlaylistMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/SongMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/YouTubePlaylistMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/StorageSettings.ktapp/src/main/kotlin/com/metrolist/music/utils/CoverArtEmbedder.ktapp/src/main/kotlin/com/metrolist/music/utils/CoverArtNative.ktapp/src/main/kotlin/com/metrolist/music/utils/DataStore.ktapp/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.ktapp/src/main/kotlin/com/metrolist/music/utils/YTPlayerUtils.ktapp/src/main/res/values/metrolist_strings.xml
Comment on lines
+71
to
+85
| // Map of songId -> itag for user-selected format downloads | ||
| private val targetItagOverride = mutableMapOf<String, Int>() | ||
|
|
||
| private val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO) | ||
|
|
||
| /** | ||
| * Set the target itag for a specific song download. | ||
| * This will be used instead of auto-selection when downloading. | ||
| */ | ||
| fun setTargetItag(songId: String, itag: Int) { | ||
| timber.log.Timber.tag("DownloadUtil").d("Setting target itag for $songId: $itag") | ||
| targetItagOverride[songId] = itag | ||
| // Invalidate cached URL to force fresh fetch with new format | ||
| invalidateUrl(songId) | ||
| } |
There was a problem hiding this comment.
Persist the selected itag outside process memory.
targetItagOverride only lives in this singleton. If the process is killed while a format-selected download is still running, the next resolver call comes back with targetItag == 0 and Line 172 falls back to AUTO. That can resume the same download in a different format. Persist the chosen itag in the download metadata or DB and rebuild it on startup.
Also applies to: 132-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/playback/DownloadUtil.kt` around
lines 71 - 85, Persist the user-selected itag instead of keeping it only in the
in-memory map targetItagOverride: update setTargetItag to write the chosen itag
into the download metadata/DB (e.g., the DownloadRecord for songId) and still
call invalidateUrl(songId), and at app startup rebuild targetItagOverride from
the persisted download records so resolver logic that reads targetItag (and
currently falls back when targetItag == 0) will find the persisted value; ensure
wherever targetItagOverride is read (resolver path around the fallback at line
~172) prefers the persisted value and that removals/clears update both the DB
record and the in-memory map.
Comment on lines
+145
to
+148
| // Fixed: use > for "not expired" (was < which meant "use if expired") | ||
| songUrlCache[mediaId]?.takeIf { it.second > System.currentTimeMillis() }?.let { | ||
| timber.log.Timber.tag("DownloadUtil").d("Using URL cache for $mediaId") | ||
| return@Factory dataSpec.withUri(it.first.toUri()) |
There was a problem hiding this comment.
Store an absolute URL-cache expiry.
This branch now treats songUrlCache as an absolute timestamp, but Line 253 still stores only streamExpiresInSeconds * 1000L. That makes every cached URL look expired immediately, so chunk resolution keeps refetching playback data instead of reusing the resolved stream URL.
🛠️ Possible fix
- songUrlCache[mediaId] = streamUrl to playbackData.streamExpiresInSeconds * 1000L
+ songUrlCache[mediaId] =
+ streamUrl to (System.currentTimeMillis() + playbackData.streamExpiresInSeconds * 1000L)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/playback/DownloadUtil.kt` around
lines 145 - 148, The cache check in Factory correctly treats songUrlCache values
as absolute timestamps, but the code that populates songUrlCache still stores
only streamExpiresInSeconds * 1000L; update the population logic to store an
absolute expiry by adding the current epoch millis (e.g.,
System.currentTimeMillis() + streamExpiresInSeconds * 1000L) when inserting into
songUrlCache so the takeIf { it.second > System.currentTimeMillis() } check
works and cached URLs are reused; keep the existing songUrlCache key and value
shape so dataSpec.withUri(...) and the Factory lookup remain unchanged.
Comment on lines
+2885
to
2895
| // Use downloaded file directly if available (bypasses ExoPlayer cache entirely) | ||
| if (song?.song?.downloadUri != null && dataSpec.position == 0L) { | ||
| Timber.tag("CacheResolver").d("Using downloaded file for $mediaId: ${song.song.downloadUri}") | ||
| scope.launch(Dispatchers.IO) { recoverSong(mediaId) } | ||
| return@Factory dataSpec.withUri(song.song.downloadUri.toUri()) | ||
| } | ||
|
|
||
| // Fallback: Check download cache for old downloads without downloadUri | ||
| // This handles songs downloaded before the custom path feature was added | ||
| if (song?.song?.downloadUri == null && downloadCache.isCached( | ||
| mediaId, |
There was a problem hiding this comment.
Don't gate downloaded-file playback on position == 0.
Line 2886 only uses downloadUri for the very first open. Any seek or restored position on a downloaded track comes back through this resolver with a non-zero offset, skips the local file, and because downloadUri != null also skips the legacy downloadCache branch. That forces a network path for custom-path downloads, so offline resume/seeks break.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around
lines 2885 - 2895, The resolver currently only uses song.song.downloadUri when
dataSpec.position == 0, which causes seeks/resumes on downloaded tracks to
bypass the local file; remove the position check so the Factory branch that
checks song?.song?.downloadUri uses the downloadUri regardless of
dataSpec.position. In the MusicService Factory block (the condition that
triggers recoverSong(mediaId) and returns dataSpec.withUri(...)), change the
gate to only test for song?.song?.downloadUri != null, keep the recoverSong
launch and dataSpec.withUri return as-is so offset handling remains correct, and
ensure this allows the later downloadCache fallback to remain reachable only
when downloadUri == null.
Comment on lines
+464
to
+496
| Material3MenuItemData( | ||
| title = { | ||
| Text( | ||
| text = if (isPinned) "Unpin from Speed dial" else "Pin to Speed dial" | ||
| ) | ||
| }, | ||
| icon = { | ||
| Icon( | ||
| painter = painterResource(if (isPinned) R.drawable.remove else R.drawable.add), | ||
| contentDescription = null, | ||
| ) | ||
| }, | ||
| onClick = { | ||
| coroutineScope.launch(Dispatchers.IO) { | ||
| if (isPinned) { | ||
| database.speedDialDao.delete(album.id) | ||
| } else { | ||
| database.speedDialDao.insert( | ||
| SpeedDialItem( | ||
| id = album.id, | ||
| secondaryId = album.album.playlistId, | ||
| title = album.album.title, | ||
| subtitle = album.artists.joinToString(", ") { it.name }, | ||
| thumbnailUrl = album.album.thumbnailUrl, | ||
| type = "ALBUM", | ||
| explicit = album.album.explicit | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
| onDismiss() | ||
| } | ||
| ) |
There was a problem hiding this comment.
Hardcoded strings should use string resources for i18n.
The speed dial pin/unpin text uses hardcoded English strings. Use stringResource for proper localization, consistent with other menu strings in this file.
Proposed fix
Material3MenuItemData(
title = {
Text(
- text = if (isPinned) "Unpin from Speed dial" else "Pin to Speed dial"
+ text = if (isPinned) stringResource(R.string.unpin_from_speed_dial) else stringResource(R.string.pin_to_speed_dial)
)
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt` around lines
464 - 496, Replace the hardcoded English labels in the Material3MenuItemData
title lambda with localized resources: use stringResource(...) instead of
literal "Unpin from Speed dial" / "Pin to Speed dial" and add corresponding
string keys (e.g. R.string.unpin_from_speed_dial and R.string.pin_to_speed_dial)
in resources; update the title expression that checks isPinned and returns
stringResource(if (isPinned) R.string.unpin_from_speed_dial else
R.string.pin_to_speed_dial) so the menu uses i18n-compatible strings in
AlbumMenu.kt.
Comment on lines
+498
to
+530
| add( | ||
| Material3MenuItemData( | ||
| title = { | ||
| Text( | ||
| text = if (isPinned) "Unpin from Speed dial" else "Pin to Speed dial" | ||
| ) | ||
| }, | ||
| icon = { | ||
| Icon( | ||
| painter = painterResource(if (isPinned) R.drawable.remove else R.drawable.add), | ||
| contentDescription = null, | ||
| ) | ||
| }, | ||
| onClick = { | ||
| coroutineScope.launch(Dispatchers.IO) { | ||
| if (isPinned) { | ||
| database.speedDialDao.delete(playlist.id) | ||
| } else { | ||
| database.speedDialDao.insert( | ||
| SpeedDialItem( | ||
| id = playlist.id, | ||
| title = playlist.playlist.name, | ||
| subtitle = null, | ||
| thumbnailUrl = playlist.thumbnails.firstOrNull(), | ||
| type = "LOCAL_PLAYLIST" | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
| onDismiss() | ||
| } | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Hardcoded strings should use string resources for i18n.
The speed dial pin/unpin text uses hardcoded English strings. Use stringResource for proper localization.
Proposed fix
add(
Material3MenuItemData(
title = {
Text(
- text = if (isPinned) "Unpin from Speed dial" else "Pin to Speed dial"
+ text = if (isPinned) stringResource(R.string.unpin_from_speed_dial) else stringResource(R.string.pin_to_speed_dial)
)
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/PlaylistMenu.kt` around lines
498 - 530, Replace the hardcoded English labels inside the Material3MenuItemData
title lambda with localized resources: call
stringResource(R.string.pin_to_speed_dial) when !isPinned and
stringResource(R.string.unpin_from_speed_dial) when isPinned (import
androidx.compose.ui.res.stringResource), and add the corresponding keys to
strings.xml; update the Text(...) call to use these stringResource values so the
pin/unpin UI is internationalized.
Comment on lines
+115
to
+127
| // Check if file already exists in album folder and delete it | ||
| val existingFile = albumFolder.findFile(sanitizedFilename) | ||
| if (existingFile != null) { | ||
| Timber.tag(TAG).d("Existing file found, deleting...") | ||
| existingFile.delete() | ||
| } | ||
|
|
||
| // Create the new file in album folder | ||
| val mimeType = format?.mimeType ?: "audio/mp4" | ||
| Timber.tag(TAG).d("Creating new file with mimeType: $mimeType") | ||
| val newFile = albumFolder.createFile(mimeType, sanitizedFilename) ?: run { | ||
| Timber.tag(TAG).e("Failed to create file: $sanitizedFilename") | ||
| return@withContext null |
There was a problem hiding this comment.
Make the replacement export atomic.
existingFile.delete() happens before the new copy is known-good, and the catch blocks just return null, so a mid-export failure can wipe the last good file or leave a partial orphan in the custom folder. Write to a temp document first, then swap it in only after the copy and updateDownloadUri() succeed.
Also applies to: 240-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt` around
lines 115 - 127, Replace the non-atomic replacement flow by writing the export
to a temporary file first and only swapping it into place after a successful
copy and updateDownloadUri(): instead of calling existingFile.delete() and then
albumFolder.createFile(sanitizedFilename) directly, create a temp document
(e.g., createFile with a ".tmp" suffix or use a temp name via
albumFolder.createFile), write the data to that temp file, call
updateDownloadUri() for the temp, verify success, then delete/replace the old
file (existingFile.delete()) and rename/move the temp to the final
sanitizedFilename (or create the final file and copy from temp), ensuring any
failures return null without touching the original existingFile; apply the same
pattern for the other location mentioned around updateDownloadUri() and lines
~240-245.
Comment on lines
+141
to
+163
| Timber.tag(TAG).d("Getting cached spans for: $songId") | ||
| val cachedSpans = downloadCache.getCachedSpans(songId) | ||
| Timber.tag(TAG).d("Found ${cachedSpans.size} cached spans") | ||
|
|
||
| if (cachedSpans.isEmpty()) { | ||
| Timber.tag(TAG).w("No cached data found for: $songId") | ||
| newFile.delete() | ||
| return@withContext null | ||
| } | ||
|
|
||
| // Sort spans by position and write them in order | ||
| val sortedSpans = cachedSpans.sortedBy { it.position } | ||
| Timber.tag(TAG).d("Spans sorted by position, starting copy...") | ||
|
|
||
| for ((index, span) in sortedSpans.withIndex()) { | ||
| Timber.tag(TAG).d("Processing span $index: position=${span.position}, length=${span.length}, file=${span.file?.name}") | ||
| span.file?.inputStream()?.use { input -> | ||
| val bytes = input.copyTo(out) | ||
| totalBytesWritten += bytes | ||
| Timber.tag(TAG).d("Copied $bytes bytes from span $index") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt | head -200Repository: MetrolistGroup/Metrolist
Length of output: 11283
🏁 Script executed:
tail -n +200 app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt | head -100Repository: MetrolistGroup/Metrolist
Length of output: 4460
🏁 Script executed:
# Search for getCachedSpans definition and usage to understand the API
rg "getCachedSpans" --type kt -B 3 -A 8Repository: MetrolistGroup/Metrolist
Length of output: 94
🏁 Script executed:
# Search for getCachedSpans definition and usage
rg "getCachedSpans" -B 3 -A 8Repository: MetrolistGroup/Metrolist
Length of output: 5906
🏁 Script executed:
# Search for span-related definitions and properties
rg "span\." app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt -B 2 -A 2Repository: MetrolistGroup/Metrolist
Length of output: 438
🏁 Script executed:
# Search for CacheSpan class or span properties definition
rg "class CacheSpan|data class.*Span" -B 2 -A 5Repository: MetrolistGroup/Metrolist
Length of output: 849
🏁 Script executed:
# Let's search for imports and understand what SimpleCache and getCachedSpans are
rg "import.*SimpleCache|import.*CacheSpan" -B 1 -A 1Repository: MetrolistGroup/Metrolist
Length of output: 2218
🏁 Script executed:
# Look at the MetrolistCacheEvictor.kt to see how it handles spans and contentLength
sed -n '1,100p' app/src/main/kotlin/com/metrolist/music/playback/MetrolistCacheEvictor.ktRepository: MetrolistGroup/Metrolist
Length of output: 3190
🏁 Script executed:
# Check if contentLength is accessible in the exportToCustomPath context
rg "contentLength|ContentMetadata" app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.ktRepository: MetrolistGroup/Metrolist
Length of output: 50
🏁 Script executed:
# Check if downloadCache has methods to get contentLength/metadata
rg "getContentMetadata|getContentLength|LENGTH_UNSET" app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.ktRepository: MetrolistGroup/Metrolist
Length of output: 50
🏁 Script executed:
# Look at the full context after writing spans to see if there's any validation
sed -n '155,170p' app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.ktRepository: MetrolistGroup/Metrolist
Length of output: 865
🏁 Script executed:
# Check if there's any validation before database update
sed -n '165,245p' app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.ktRepository: MetrolistGroup/Metrolist
Length of output: 4596
🏁 Script executed:
# Check if SimpleCache API provides access to contentLength
web_search "androidx.media3.datasource.cache SimpleCache getCachedSpans CacheSpan position"Repository: MetrolistGroup/Metrolist
Length of output: 118
🏁 Script executed:
# Search for how other parts handle span validation or contentLength
rg "position.*0|position == 0|gap|contiguous|hole" app/src/main/kotlin/com/metrolist/music/Repository: MetrolistGroup/Metrolist
Length of output: 6084
🏁 Script executed:
# Check MusicService.kt to see how they validate spans
sed -n '1,50p' app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt | grep -A 20 "cachedBytes"Repository: MetrolistGroup/Metrolist
Length of output: 50
🏁 Script executed:
# Get the MusicService validation logic
rg "cachedBytes >= contentLength" -B 5 -A 5 app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktRepository: MetrolistGroup/Metrolist
Length of output: 714
Validate span contiguity before exporting to prevent corrupt files.
The loop at lines 155–162 concatenates whatever getCachedSpans(songId) returns without verifying that spans start at offset 0 or cover the full asset without gaps. If the cache contains holes or evicted ranges, the export will succeed with an incomplete file but persist a valid downloadUri to the database. Add validation before line 155 to fail fast: ensure the first span's position is 0, spans are contiguous, and their total length matches the expected file size (available from downloadCache.getContentMetadata(songId)). This pattern is already used in MusicService.kt where cached bytes are validated against content length before marking a song as fully cached.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt` around
lines 141 - 163, The export currently concatenates cached spans from
downloadCache.getCachedSpans(songId) without verifying contiguity, which can
produce incomplete files; before iterating sortedSpans in DownloadExportHelper
(before the for loop that processes each span and writes to newFile), fetch
content metadata via downloadCache.getContentMetadata(songId) and validate that:
(1) sortedSpans.first().position == 0, (2) each span.position equals the
previous span.position + previous span.length (i.e., spans are contiguous), and
(3) the sum of span.length equals the expected content length from the metadata;
if any check fails, delete newFile and return null (as the existing error path
does) so you don't persist a downloadUri for a corrupt/incomplete export.
Comment on lines
+395
to
+403
| private fun getExtensionFromFormat(format: FormatEntity?): String { | ||
| return when { | ||
| format == null -> "m4a" | ||
| format.mimeType.contains("audio/webm") -> "ogg" | ||
| format.mimeType.contains("audio/mp4") -> "m4a" | ||
| format.mimeType.contains("audio/mpeg") -> "mp3" | ||
| format.mimeType.contains("audio/ogg") -> "ogg" | ||
| else -> "m4a" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "DownloadExportHelper.kt" -o -name "*ExportHelper*"Repository: MetrolistGroup/Metrolist
Length of output: 139
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt | head -100Repository: MetrolistGroup/Metrolist
Length of output: 5086
🏁 Script executed:
wc -l app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.ktRepository: MetrolistGroup/Metrolist
Length of output: 141
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt | sed -n '100,200p'Repository: MetrolistGroup/Metrolist
Length of output: 6285
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt | sed -n '200,300p'Repository: MetrolistGroup/Metrolist
Length of output: 5236
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt | sed -n '300,400p'Repository: MetrolistGroup/Metrolist
Length of output: 5141
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt | sed -n '395,449p'Repository: MetrolistGroup/Metrolist
Length of output: 2464
Replace audio/webm mapping with the correct container extension.
This function assigns .ogg extension to WebM-format bytes without remuxing. Since the export process copies the cached payload verbatim (line 158), the resulting file will have a .ogg extension but contain WebM container data, breaking compatibility with media players and file scanners that rely on extensions to identify formats. Either use the .webm extension or implement container remuxing before export.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt` around
lines 395 - 403, In getExtensionFromFormat(format: FormatEntity?), change the
mapping for MIME type "audio/webm" to return the correct container extension
("webm") instead of "ogg" (or alternatively implement remuxing before export if
you must keep ".ogg"); specifically update the when-branch that checks
format.mimeType.contains("audio/webm") so the function returns "webm" to match
the actual WebM container being copied in the export (or document and call a
remux routine prior to writing the cached payload).
Comment on lines
+406
to
+413
| private fun sanitizeFilename(filename: String): String { | ||
| // Remove or replace characters that are invalid in filenames | ||
| return filename | ||
| .replace(Regex("[\\\\/:*?\"<>|]"), "_") | ||
| .replace(Regex("\\s+"), " ") | ||
| .trim() | ||
| .take(200) // Limit filename length | ||
| } |
There was a problem hiding this comment.
Preserve the extension when truncating long names.
take(200) runs on the whole title.ext string, so long titles can lose the extension entirely and export an untyped file. Truncate the basename first, then append the extension.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt` around
lines 406 - 413, The sanitizeFilename function currently truncates the entire
filename (including extension) which can strip the file extension; change
sanitizeFilename to split the input into base name and extension (preserve the
dot + extension if present), sanitize each piece (apply Regex replacements and
trim) and then truncate only the base name to a safe length (e.g., 200 minus
extension length) before rejoining base + extension; update references to the
function accordingly and ensure you handle filenames with no extension and with
multiple dots correctly.
Comment on lines
+397
to
+425
| // Try to find a format of the same codec type (M4A or OPUS) with similar bitrate | ||
| // This handles cases where different songs have different available itags | ||
| val allFormats = playerResponse.streamingData?.adaptiveFormats?.filter { it.isAudio && it.isOriginal } | ||
| val requestedFormat = allFormats?.find { it.itag == targetItag } | ||
|
|
||
| // Determine if the requested format was M4A or OPUS based on common itags | ||
| // M4A itags: 139, 140, 141, 256, 258, 327, 380, 774 | ||
| // OPUS itags: 249, 250, 251, 338 | ||
| val isM4aRequested = targetItag in listOf(139, 140, 141, 256, 258, 327, 380, 774) | ||
| val isOpusRequested = targetItag in listOf(249, 250, 251, 338) | ||
|
|
||
| if (isM4aRequested) { | ||
| // Find best M4A format available | ||
| val m4aFormat = allFormats | ||
| ?.filter { it.mimeType.contains("mp4a") || it.mimeType.contains("audio/mp4") } | ||
| ?.maxByOrNull { it.bitrate } | ||
| if (m4aFormat != null) { | ||
| Timber.tag(logTag).d("Using alternative M4A format: itag=${m4aFormat.itag}, bitrate=${m4aFormat.bitrate}") | ||
| return m4aFormat | ||
| } | ||
| } else if (isOpusRequested) { | ||
| // Find best OPUS format available | ||
| val opusFormat = allFormats | ||
| ?.filter { it.mimeType.contains("opus") || it.mimeType.contains("audio/webm") } | ||
| ?.maxByOrNull { it.bitrate } | ||
| if (opusFormat != null) { | ||
| Timber.tag(logTag).d("Using alternative OPUS format: itag=${opusFormat.itag}, bitrate=${opusFormat.bitrate}") | ||
| return opusFormat | ||
| } |
There was a problem hiding this comment.
Keep the codec fallback from silently upgrading the chosen quality.
When the requested itag is missing, this fallback always picks the highest M4A/OPUS variant. That means a user who selected a lower-quality download can silently get a larger file and different metadata behavior than the picker advertised. Pick the nearest bitrate in the same codec, or surface the miss back to the caller instead of always upgrading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/utils/YTPlayerUtils.kt` around lines
397 - 425, The current fallback in YTPlayerUtils.kt (around variables
targetItag, allFormats, requestedFormat, isM4aRequested/isOpusRequested) always
picks the highest-bitrate m4a/opus variant (m4aFormat/opusFormat), which can
silently upgrade quality; change the logic to pick the format whose bitrate is
closest to the originally requested format: if requestedFormat != null compute
requestedBitrate = requestedFormat.bitrate and select the candidate format with
minimal abs(candidate.bitrate - requestedBitrate) for the same codec, otherwise
do not auto-upgrade—return null or bubble the miss to the caller so the caller
can decide (rather than always returning the max-bitrate variant).
- Insert song to database when fetching formats in YouTubeSongMenu - Fetch album info from YouTube when missing using next() API - Get full album details (year, track, album artist) for embedding
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt (1)
541-571:⚠️ Potential issue | 🟠 MajorDon't delete the current download until the swap is confirmed.
This handler removes every song before formats are loaded and before the user picks one. If format loading fails or the dialog is canceled, the album is left with no download at all. Move the remove loop into
onFormatSelectedafter a successful choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt` around lines 541 - 571, The current onClick handler removes all downloads immediately via the songs.forEach { DownloadService.sendRemoveDownload(...) } loop before formats are loaded or the user confirms; move that removal logic out of the onClick block and into the code path that handles a successful format choice (e.g., the onFormatSelected function/handler) so deletions only occur after formats are loaded, the user selects a format, and swap is confirmed; keep the existing state updates (showDownloadFormatDialog, isLoadingFormats, availableFormats) in onClick to fetch formats, and ensure onFormatSelected performs the DownloadService.sendRemoveDownload calls and then triggers the new download, while preserving behavior on cancel/failure (do not remove downloads) and logging exceptions as currently done with Timber.tag("AlbumMenu").app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubePlaylistMenu.kt (1)
125-138:⚠️ Potential issue | 🔴 CriticalUse the source playlist ID here, not the destination playlist ID.
targetPlaylistis the playlist the user picked as the destination. ReusingtargetPlaylist.idin the fallback fetch and inYouTube.addPlaylistToPlaylist(...)either fetches the wrong songs or passes a local DB ID to the API, so this flow breaks wheneversongswas not preloaded.Suggested fix
onGetSong = { targetPlaylist -> val allSongs = songs .ifEmpty { - YouTube.playlist(targetPlaylist.id).completed().getOrNull()?.songs.orEmpty() + YouTube.playlist(playlist.id).completed().getOrNull()?.songs.orEmpty() }.map { it.toMediaMetadata() } database.withTransaction { allSongs.forEach(::insert) } coroutineScope.launch(Dispatchers.IO) { targetPlaylist.playlist.browseId?.let { playlistId -> - YouTube.addPlaylistToPlaylist(playlistId, targetPlaylist.id) + YouTube.addPlaylistToPlaylist(playlistId, playlist.id) } } allSongs.map { it.id } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubePlaylistMenu.kt` around lines 125 - 138, The code is using targetPlaylist.id for both the fallback fetch and the YouTube.addPlaylistToPlaylist call, but targetPlaylist is the destination; replace those uses with the source playlist's ID (the source playlist variable from the surrounding scope) so the fallback fetch calls YouTube.playlist(sourceId) and the API call uses YouTube.addPlaylistToPlaylist(sourceId, targetPlaylist.id) (and keep using targetPlaylist.playlist.browseId only to obtain the destination browseId). Update the onGetSong block to reference the correct source playlist ID instead of targetPlaylist.id.
♻️ Duplicate comments (5)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
2886-2903:⚠️ Potential issue | 🟠 MajorDownloaded-file playback breaks on seek/resume due to
position == 0gate.The condition at Line 2886 (
dataSpec.position == 0L) only usesdownloadUrifor initial playback. When the user seeks or resumes at a non-zero position:
- The
downloadUribranch is skipped (position ≠ 0)- The
downloadCachebranch is also skipped (becausedownloadUri != null)- Falls through to network streaming
This breaks offline playback for custom-path downloads whenever the user seeks or pauses/resumes.
🐛 Proposed fix: Remove position check and add proper fallback
- // Use downloaded file directly if available (bypasses ExoPlayer cache entirely) - if (song?.song?.downloadUri != null && dataSpec.position == 0L) { + // Use downloaded file directly if available (bypasses ExoPlayer cache entirely) + if (song?.song?.downloadUri != null) { Timber.tag("CacheResolver").d("Using downloaded file for $mediaId: ${song.song.downloadUri}") scope.launch(Dispatchers.IO) { recoverSong(mediaId) } return@Factory dataSpec.withUri(song.song.downloadUri.toUri()) } // Fallback: Check download cache for old downloads without downloadUri - // This handles songs downloaded before the custom path feature was added - if (song?.song?.downloadUri == null && downloadCache.isCached( + // This handles songs downloaded before the custom path feature was added, + // or as a fallback if downloadUri file is inaccessible + if (downloadCache.isCached( mediaId, dataSpec.position, if (dataSpec.length >= 0) dataSpec.length else 1 ) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 2886 - 2903, The playback check currently only uses the local downloadUri when dataSpec.position == 0L, which prevents resumed/seeking playback from custom-path downloads; remove the position check so the branch that returns dataSpec.withUri(song.song.downloadUri.toUri()) runs whenever song?.song?.downloadUri != null (still launch recoverSong via scope.launch(Dispatchers.IO) { recoverSong(mediaId) } and keep the Timber log), and keep the subsequent legacy fallback that calls downloadCache.isCached(...) only for the case song?.song?.downloadUri == null so old-downloads without a downloadUri are still handled.app/src/main/kotlin/com/metrolist/music/ui/screens/settings/StorageSettings.kt (1)
393-401:⚠️ Potential issue | 🟠 MajorToggle enables before folder selection completes.
When the user enables the toggle with no path set (line 395-398), the folder picker launches but
onCustomDownloadPathEnabledChange(enabled)at line 400 immediately sets the preference to true. If the user cancels the picker, the feature remains enabled with no valid path.🛠️ Proposed fix: defer enabling until folder is selected
Switch( checked = customDownloadPathEnabled, onCheckedChange = { enabled -> Timber.tag(TAG).d("Custom download path toggle: $enabled") if (enabled && customDownloadPathUri.isEmpty()) { // Launch folder picker if enabling without a path set Timber.tag(TAG).d("No path set, launching folder picker") folderPickerLauncher.launch(null) + // Don't enable yet - will be enabled when folder is selected + return@Switch } onCustomDownloadPathEnabledChange(enabled) } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/StorageSettings.kt` around lines 393 - 401, The toggle's onCheckedChange currently calls onCustomDownloadPathEnabledChange(enabled) immediately even when launching folderPickerLauncher, causing the preference to be enabled if the user cancels; change the logic in the onCheckedChange lambda so that when enabled == true and customDownloadPathUri.isEmpty() you only launch folderPickerLauncher and do NOT call onCustomDownloadPathEnabledChange(true); instead, update the preference to true inside the folder picker's result handler (the folderPickerLauncher callback) only if a non-empty customDownloadPathUri is returned, and ensure cancellations leave the preference false; keep the existing behavior for disabling (call onCustomDownloadPathEnabledChange(false) immediately).app/src/main/kotlin/com/metrolist/music/ui/menu/PlaylistMenu.kt (1)
498-503:⚠️ Potential issue | 🟡 MinorUse string resources for the speed-dial label.
The new pin/unpin title is hardcoded English, so it will not localize with the rest of the menu.
Based on learnings, all string edits must be made to the
Metrolist/app/src/main/res/values/metrolist_strings.xmlfile, NOTMetrolist/app/src/main/res/values/strings.xml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/menu/PlaylistMenu.kt` around lines 498 - 503, The menu label is hardcoded in PlaylistMenu inside the Material3MenuItemData title lambda (using isPinned) so it won't localize; add two entries in metrolist_strings.xml (e.g., speed_dial_pin and speed_dial_unpin) and replace the hardcoded text with Text(text = stringResource(R.string.speed_dial_pin)) / Text(text = stringResource(R.string.speed_dial_unpin)) selecting which resource based on isPinned; ensure you import androidx.compose.ui.res.stringResource and reference the new R.string identifiers in the Material3MenuItemData title lambda.app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt (1)
464-468:⚠️ Potential issue | 🟡 MinorUse string resources for the speed-dial label.
The new pin/unpin title is hardcoded English, so it will not localize with the rest of the menu.
Based on learnings, all string edits must be made to the
Metrolist/app/src/main/res/values/metrolist_strings.xmlfile, NOTMetrolist/app/src/main/res/values/strings.xml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt` around lines 464 - 468, Replace the hardcoded English labels in AlbumMenu.kt (inside the Material3MenuItemData title lambda where Text(...) is used) with stringResource lookups and add those strings to metrolist_strings.xml; specifically create keys like pin_to_speed_dial and unpin_from_speed_dial in metrolist_strings.xml and use a conditional to choose stringResource(R.string.pin_to_speed_dial) or stringResource(R.string.unpin_from_speed_dial) instead of the literal "Pin to Speed dial"/"Unpin from Speed dial".app/src/main/kotlin/com/metrolist/music/playback/DownloadUtil.kt (1)
249-254:⚠️ Potential issue | 🟠 MajorURL cache stores relative duration instead of absolute expiry timestamp.
Line 253 stores
streamExpiresInSeconds * 1000L, which is a duration in milliseconds. However, line 146 comparesit.second > System.currentTimeMillis(), expecting an absolute timestamp. This causes all cached URLs to appear expired immediately, forcing unnecessary refetches.🛠️ Proposed fix
- songUrlCache[mediaId] = streamUrl to playbackData.streamExpiresInSeconds * 1000L + songUrlCache[mediaId] = streamUrl to (System.currentTimeMillis() + playbackData.streamExpiresInSeconds * 1000L)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/DownloadUtil.kt` around lines 249 - 254, The cache is storing a duration (playbackData.streamExpiresInSeconds * 1000L) but elsewhere code (the check on songUrlCache entries using it.second > System.currentTimeMillis()) expects an absolute expiry timestamp; update the assignment to store an absolute expiry by adding the current time (e.g., use System.currentTimeMillis() + playbackData.streamExpiresInSeconds * 1000L) when putting into songUrlCache for the streamUrl so the expiry comparison works as intended.
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/utils/DataStore.kt (1)
55-65: Consider reusing the existing genericpreferencefunction.The new
booleanPreferenceandstringPreferencefunctions duplicate the logic of the genericpreferencefunction defined at lines 43-47, which already handlesBooleanandStringtypes. Unless there's a specific type inference benefit needed at call sites, these could be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/utils/DataStore.kt` around lines 55 - 65, Duplicate booleanPreference and stringPreference logic should be removed by delegating to the existing generic preference function: replace the bodies of booleanPreference and stringPreference so they simply return preference(context, key, defaultValue) (keep the same signatures to avoid breaking callers) or remove these wrappers and update call sites to use preference(...) directly; reference the existing preference(...) function and the booleanPreference/stringPreference symbols when making 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 @.github/actions/setup-coverart/action.yml:
- Around line 14-18: The curl download step using DOWNLOAD_URL currently doesn't
check for failures before attempting to unzip /tmp/libcoverart-jniLibs.zip;
update the action to validate the download succeeded (check curl's exit status
or verify the file exists and is non-empty) and fail fast with a clear error
message if the download failed, e.g., after the curl to
/tmp/libcoverart-jniLibs.zip, check the exit code or file size and abort (do not
proceed to unzip -o) so subsequent unzip operations won't run on a missing or
invalid archive.
In
`@app/src/main/kotlin/com/metrolist/music/ui/component/DownloadFormatDialog.kt`:
- Around line 149-151: The fixed 280.dp height on the LazyColumn in
DownloadFormatDialog can clip content on small screens; change the modifier from
Modifier.height(280.dp) to a flexible constraint such as Modifier.heightIn(max =
280.dp) (or use Modifier.fillMaxHeight(fraction) / Modifier.weight inside a
Column) so the list can shrink on small screens while still capping its maximum
height.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt`:
- Around line 391-409: The share action currently uses album.album.playlistId
without null-checking, so for local albums it shares a broken link; update the
AlbumMenu UI to hide or disable the NewAction with icon/share text when
album.album.playlistId is null by gating its creation on a non-null playlistId
(e.g., check album.album.playlistId != null before composing the NewAction) and
only construct the Intent and call context.startActivity within the onClick when
playlistId is present (reference the NewAction composable, the onClick lambda,
and album.album.playlistId).
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/PlaylistMenu.kt`:
- Around line 377-395: The quick-share NewAction currently uses
dbPlaylist?.playlist?.browseId which can be null for local playlists; ensure the
action is only rendered or enabled when a real remote ID/share link exists by
guarding the UI and click handler: wrap the NewAction rendering (or its onClick)
with a condition that checks for a non-null dbPlaylist?.playlist?.browseId (or
use the already guarded playlist.playlist.shareLink) and only build/start the
share Intent when that value is non-null, otherwise omit the action or fall back
to the existing guarded share menu item.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubePlaylistMenu.kt`:
- Around line 551-556: The title lambda in Material3MenuItemData uses hardcoded
English strings (if (isPinned) "Unpin from Speed dial" else "Pin to Speed
dial"); move these into resources by creating two string resources (e.g.,
pin_to_speed_dial and unpin_from_speed_dial) in strings.xml and change the Text
call to use stringResource(...) with a conditional resource id based on
isPinned; update YouTubePlaylistMenu.kt to import
androidx.compose.ui.res.stringResource and reference those resource ids instead
of the raw literals.
- Around line 583-658: The download menu is hidden by the outer if
(songs.isNotEmpty()) which prevents lazy-loading downloads when only a
PlaylistItem is present; remove that outer guard so the download
Material3MenuItemData is always added, and inside its onClick replace the early
return (if (songs.isEmpty()) return@Material3MenuItemData) with logic to lazily
load the playlist items first (using the same lazy-fetch routine used by
play-next/add-to-queue/import branches) before proceeding to set
showDownloadFormatDialog = true, isLoadingFormats = true, availableFormats =
emptyList() and launching the coroutine that calls
com.metrolist.music.utils.YTPlayerUtils.getAllAvailableAudioFormats(firstSong.id);
keep using coroutineScope.launch(Dispatchers.IO) and the existing error handling
that sets isLoadingFormats/showDownloadFormatDialog on the Main dispatcher.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/StorageSettings.kt`:
- Around line 413-420: The catch block that swallows exceptions when computing
displayPath in StorageSettings (the block using customDownloadPathUri,
Uri.parse, DocumentsContract.getTreeDocumentId and DocumentFile.fromTreeUri)
should log the caught exception for debugging; update the catch (e: Exception)
branch to call the app logger or Android Log (e.g., Log.w or your processLogger)
with a clear message like "Failed to parse custom download path" and include the
exception (e) before returning customDownloadPathUri so the fallback remains
unchanged.
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt`:
- Around line 541-571: The current onClick handler removes all downloads
immediately via the songs.forEach { DownloadService.sendRemoveDownload(...) }
loop before formats are loaded or the user confirms; move that removal logic out
of the onClick block and into the code path that handles a successful format
choice (e.g., the onFormatSelected function/handler) so deletions only occur
after formats are loaded, the user selects a format, and swap is confirmed; keep
the existing state updates (showDownloadFormatDialog, isLoadingFormats,
availableFormats) in onClick to fetch formats, and ensure onFormatSelected
performs the DownloadService.sendRemoveDownload calls and then triggers the new
download, while preserving behavior on cancel/failure (do not remove downloads)
and logging exceptions as currently done with Timber.tag("AlbumMenu").
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubePlaylistMenu.kt`:
- Around line 125-138: The code is using targetPlaylist.id for both the fallback
fetch and the YouTube.addPlaylistToPlaylist call, but targetPlaylist is the
destination; replace those uses with the source playlist's ID (the source
playlist variable from the surrounding scope) so the fallback fetch calls
YouTube.playlist(sourceId) and the API call uses
YouTube.addPlaylistToPlaylist(sourceId, targetPlaylist.id) (and keep using
targetPlaylist.playlist.browseId only to obtain the destination browseId).
Update the onGetSong block to reference the correct source playlist ID instead
of targetPlaylist.id.
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/DownloadUtil.kt`:
- Around line 249-254: The cache is storing a duration
(playbackData.streamExpiresInSeconds * 1000L) but elsewhere code (the check on
songUrlCache entries using it.second > System.currentTimeMillis()) expects an
absolute expiry timestamp; update the assignment to store an absolute expiry by
adding the current time (e.g., use System.currentTimeMillis() +
playbackData.streamExpiresInSeconds * 1000L) when putting into songUrlCache for
the streamUrl so the expiry comparison works as intended.
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 2886-2903: The playback check currently only uses the local
downloadUri when dataSpec.position == 0L, which prevents resumed/seeking
playback from custom-path downloads; remove the position check so the branch
that returns dataSpec.withUri(song.song.downloadUri.toUri()) runs whenever
song?.song?.downloadUri != null (still launch recoverSong via
scope.launch(Dispatchers.IO) { recoverSong(mediaId) } and keep the Timber log),
and keep the subsequent legacy fallback that calls downloadCache.isCached(...)
only for the case song?.song?.downloadUri == null so old-downloads without a
downloadUri are still handled.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt`:
- Around line 464-468: Replace the hardcoded English labels in AlbumMenu.kt
(inside the Material3MenuItemData title lambda where Text(...) is used) with
stringResource lookups and add those strings to metrolist_strings.xml;
specifically create keys like pin_to_speed_dial and unpin_from_speed_dial in
metrolist_strings.xml and use a conditional to choose
stringResource(R.string.pin_to_speed_dial) or
stringResource(R.string.unpin_from_speed_dial) instead of the literal "Pin to
Speed dial"/"Unpin from Speed dial".
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/PlaylistMenu.kt`:
- Around line 498-503: The menu label is hardcoded in PlaylistMenu inside the
Material3MenuItemData title lambda (using isPinned) so it won't localize; add
two entries in metrolist_strings.xml (e.g., speed_dial_pin and speed_dial_unpin)
and replace the hardcoded text with Text(text =
stringResource(R.string.speed_dial_pin)) / Text(text =
stringResource(R.string.speed_dial_unpin)) selecting which resource based on
isPinned; ensure you import androidx.compose.ui.res.stringResource and reference
the new R.string identifiers in the Material3MenuItemData title lambda.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/StorageSettings.kt`:
- Around line 393-401: The toggle's onCheckedChange currently calls
onCustomDownloadPathEnabledChange(enabled) immediately even when launching
folderPickerLauncher, causing the preference to be enabled if the user cancels;
change the logic in the onCheckedChange lambda so that when enabled == true and
customDownloadPathUri.isEmpty() you only launch folderPickerLauncher and do NOT
call onCustomDownloadPathEnabledChange(true); instead, update the preference to
true inside the folder picker's result handler (the folderPickerLauncher
callback) only if a non-empty customDownloadPathUri is returned, and ensure
cancellations leave the preference false; keep the existing behavior for
disabling (call onCustomDownloadPathEnabledChange(false) immediately).
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/utils/DataStore.kt`:
- Around line 55-65: Duplicate booleanPreference and stringPreference logic
should be removed by delegating to the existing generic preference function:
replace the bodies of booleanPreference and stringPreference so they simply
return preference(context, key, defaultValue) (keep the same signatures to avoid
breaking callers) or remove these wrappers and update call sites to use
preference(...) directly; reference the existing preference(...) function and
the booleanPreference/stringPreference symbols when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 334c0957-b74c-43f5-bece-58aa27356b00
📒 Files selected for processing (29)
.github/actions/setup-coverart/action.yml.github/workflows/build.yml.github/workflows/build_pr.yml.github/workflows/build_quick.yml.github/workflows/release.yml.gitignoreapp/proguard-rules.proapp/schemas/com.metrolist.music.db.InternalDatabase/35.jsonapp/schemas/com.metrolist.music.db.InternalDatabase/36.jsonapp/setup_coverart.shapp/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.ktapp/src/main/kotlin/com/metrolist/music/db/DatabaseDao.ktapp/src/main/kotlin/com/metrolist/music/db/entities/SongEntity.ktapp/src/main/kotlin/com/metrolist/music/playback/DownloadUtil.ktapp/src/main/kotlin/com/metrolist/music/playback/MusicService.ktapp/src/main/kotlin/com/metrolist/music/ui/component/DownloadFormatDialog.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/PlayerMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/PlaylistMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/SongMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/YouTubePlaylistMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/StorageSettings.ktapp/src/main/kotlin/com/metrolist/music/utils/CoverArtEmbedder.ktapp/src/main/kotlin/com/metrolist/music/utils/CoverArtNative.ktapp/src/main/kotlin/com/metrolist/music/utils/DataStore.ktapp/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.ktapp/src/main/kotlin/com/metrolist/music/utils/YTPlayerUtils.ktapp/src/main/res/values/metrolist_strings.xml
🚧 Files skipped from review as they are similar to previous changes (6)
- app/src/main/res/values/metrolist_strings.xml
- .gitignore
- app/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.kt
- .github/workflows/release.yml
- app/src/main/kotlin/com/metrolist/music/db/DatabaseDao.kt
- app/src/main/kotlin/com/metrolist/music/utils/CoverArtNative.kt
Comment on lines
+14
to
+18
| curl -L -o /tmp/libcoverart-jniLibs.zip "$DOWNLOAD_URL" | ||
|
|
||
| # Extract to app/src/main/jniLibs | ||
| mkdir -p app/src/main/jniLibs | ||
| unzip -o /tmp/libcoverart-jniLibs.zip -d app/src/main/jniLibs |
There was a problem hiding this comment.
Add error handling for the curl download.
Unlike the companion script app/setup_coverart.sh which checks curl's exit code, this action proceeds even if the download fails (e.g., 404, network error). This can cause confusing unzip errors later.
🛠️ Proposed fix
- curl -L -o /tmp/libcoverart-jniLibs.zip "$DOWNLOAD_URL"
+ curl -fL -o /tmp/libcoverart-jniLibs.zip "$DOWNLOAD_URL" || {
+ echo "Failed to download coverart library from $DOWNLOAD_URL"
+ exit 1
+ }📝 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
| curl -L -o /tmp/libcoverart-jniLibs.zip "$DOWNLOAD_URL" | |
| # Extract to app/src/main/jniLibs | |
| mkdir -p app/src/main/jniLibs | |
| unzip -o /tmp/libcoverart-jniLibs.zip -d app/src/main/jniLibs | |
| curl -fL -o /tmp/libcoverart-jniLibs.zip "$DOWNLOAD_URL" || { | |
| echo "Failed to download coverart library from $DOWNLOAD_URL" | |
| exit 1 | |
| } | |
| # Extract to app/src/main/jniLibs | |
| mkdir -p app/src/main/jniLibs | |
| unzip -o /tmp/libcoverart-jniLibs.zip -d app/src/main/jniLibs |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/actions/setup-coverart/action.yml around lines 14 - 18, The curl
download step using DOWNLOAD_URL currently doesn't check for failures before
attempting to unzip /tmp/libcoverart-jniLibs.zip; update the action to validate
the download succeeded (check curl's exit status or verify the file exists and
is non-empty) and fail fast with a clear error message if the download failed,
e.g., after the curl to /tmp/libcoverart-jniLibs.zip, check the exit code or
file size and abort (do not proceed to unzip -o) so subsequent unzip operations
won't run on a missing or invalid archive.
Comment on lines
+149
to
+151
| LazyColumn( | ||
| modifier = Modifier.height(280.dp), | ||
| verticalArrangement = Arrangement.spacedBy(8.dp) |
There was a problem hiding this comment.
Fixed height may clip content on smaller screens.
The LazyColumn has a hardcoded height of 280.dp which may not accommodate all formats on devices with smaller screens or larger font sizes, potentially making some options unreachable.
🛠️ Consider using a max height constraint
// Format list
LazyColumn(
- modifier = Modifier.height(280.dp),
+ modifier = Modifier.heightIn(max = 280.dp),
verticalArrangement = Arrangement.spacedBy(8.dp)
) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/component/DownloadFormatDialog.kt`
around lines 149 - 151, The fixed 280.dp height on the LazyColumn in
DownloadFormatDialog can clip content on small screens; change the modifier from
Modifier.height(280.dp) to a flexible constraint such as Modifier.heightIn(max =
280.dp) (or use Modifier.fillMaxHeight(fraction) / Modifier.weight inside a
Column) so the list can shrink on small screens while still capping its maximum
height.
Comment on lines
+391
to
+409
| NewAction( | ||
| icon = { | ||
| Icon( | ||
| painter = painterResource(R.drawable.share), | ||
| contentDescription = null, | ||
| modifier = Modifier.size(28.dp), | ||
| tint = MaterialTheme.colorScheme.onSurfaceVariant | ||
| ) | ||
| }, | ||
| text = stringResource(R.string.share), | ||
| onClick = { | ||
| onDismiss() | ||
| val intent = Intent().apply { | ||
| action = Intent.ACTION_SEND | ||
| type = "text/plain" | ||
| putExtra(Intent.EXTRA_TEXT, "https://music.youtube.com/playlist?list=${album.album.playlistId}") | ||
| } | ||
| context.startActivity(Intent.createChooser(intent, null)) | ||
| } |
There was a problem hiding this comment.
Hide the share action when playlistId is missing.
album.album.playlistId is nullable. For local albums this branch shares a broken Music link instead of disabling share.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/AlbumMenu.kt` around lines
391 - 409, The share action currently uses album.album.playlistId without
null-checking, so for local albums it shares a broken link; update the AlbumMenu
UI to hide or disable the NewAction with icon/share text when
album.album.playlistId is null by gating its creation on a non-null playlistId
(e.g., check album.album.playlistId != null before composing the NewAction) and
only construct the Intent and call context.startActivity within the onClick when
playlistId is present (reference the NewAction composable, the onClick lambda,
and album.album.playlistId).
Comment on lines
+377
to
+395
| NewAction( | ||
| icon = { | ||
| Icon( | ||
| painter = painterResource(R.drawable.share), | ||
| contentDescription = null, | ||
| modifier = Modifier.size(28.dp), | ||
| tint = MaterialTheme.colorScheme.onSurfaceVariant | ||
| ) | ||
| }, | ||
| text = stringResource(R.string.share), | ||
| onClick = { | ||
| onDismiss() | ||
| val intent = Intent().apply { | ||
| action = Intent.ACTION_SEND | ||
| type = "text/plain" | ||
| putExtra(Intent.EXTRA_TEXT, "https://music.youtube.com/playlist?list=${dbPlaylist?.playlist?.browseId}") | ||
| } | ||
| context.startActivity(Intent.createChooser(intent, null)) | ||
| } |
There was a problem hiding this comment.
Guard the quick-share action with a real remote ID.
This action is always rendered, but dbPlaylist?.playlist?.browseId is nullable. For local playlists it ends up sharing a broken Music link, while the lower share menu item already uses the guarded playlist.playlist.shareLink.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/PlaylistMenu.kt` around lines
377 - 395, The quick-share NewAction currently uses
dbPlaylist?.playlist?.browseId which can be null for local playlists; ensure the
action is only rendered or enabled when a real remote ID/share link exists by
guarding the UI and click handler: wrap the NewAction rendering (or its onClick)
with a condition that checks for a non-null dbPlaylist?.playlist?.browseId (or
use the already guarded playlist.playlist.shareLink) and only build/start the
share Intent when that value is non-null, otherwise omit the action or fall back
to the existing guarded share menu item.
Comment on lines
+551
to
+556
| Material3MenuItemData( | ||
| title = { | ||
| Text( | ||
| text = if (isPinned) "Unpin from Speed dial" else "Pin to Speed dial" | ||
| ) | ||
| }, |
There was a problem hiding this comment.
Move the speed-dial label into resources.
Hardcoded English here bypasses localization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubePlaylistMenu.kt`
around lines 551 - 556, The title lambda in Material3MenuItemData uses hardcoded
English strings (if (isPinned) "Unpin from Speed dial" else "Pin to Speed
dial"); move these into resources by creating two string resources (e.g.,
pin_to_speed_dial and unpin_from_speed_dial) in strings.xml and change the Text
call to use stringResource(...) with a conditional resource id based on
isPinned; update YouTubePlaylistMenu.kt to import
androidx.compose.ui.res.stringResource and reference those resource ids instead
of the raw literals.
Comment on lines
+583
to
+658
| if (songs.isNotEmpty()) { | ||
| add( | ||
| when (downloadState) { | ||
| Download.STATE_COMPLETED -> { | ||
| Material3MenuItemData( | ||
| title = { | ||
| Text( | ||
| text = stringResource(R.string.remove_download) | ||
| ) | ||
| }, | ||
| icon = { | ||
| Icon( | ||
| painter = painterResource(R.drawable.offline), | ||
| contentDescription = null | ||
| ) | ||
| }, | ||
| onClick = { | ||
| showRemoveDownloadDialog = true | ||
| } | ||
| ) | ||
| } | ||
| Download.STATE_QUEUED, Download.STATE_DOWNLOADING -> { | ||
| Material3MenuItemData( | ||
| title = { Text(text = stringResource(R.string.downloading)) }, | ||
| icon = { | ||
| CircularProgressIndicator( | ||
| modifier = Modifier.size(24.dp), | ||
| strokeWidth = 2.dp | ||
| ) | ||
| }, | ||
| onClick = { | ||
| showRemoveDownloadDialog = true | ||
| } | ||
| ) | ||
| } | ||
| else -> { | ||
| Material3MenuItemData( | ||
| title = { Text(text = stringResource(R.string.action_download)) }, | ||
| description = { Text(text = stringResource(R.string.download_desc)) }, | ||
| icon = { | ||
| Icon( | ||
| painter = painterResource(R.drawable.download), | ||
| contentDescription = null, | ||
| ) | ||
| }, | ||
| onClick = { | ||
| if (songs.isEmpty()) return@Material3MenuItemData | ||
| // Show format picker - fetch formats from first song | ||
| showDownloadFormatDialog = true | ||
| isLoadingFormats = true | ||
| availableFormats = emptyList() | ||
|
|
||
| else -> { | ||
| Material3MenuItemData( | ||
| title = { Text(text = stringResource(R.string.action_download)) }, | ||
| description = { Text(text = stringResource(R.string.download_desc)) }, | ||
| icon = { | ||
| Icon( | ||
| painter = painterResource(R.drawable.download), | ||
| contentDescription = null, | ||
| ) | ||
| }, | ||
| onClick = { | ||
| songs.forEach { song -> | ||
| val downloadRequest = | ||
| DownloadRequest | ||
| .Builder(song.id, song.id.toUri()) | ||
| .setCustomCacheKey(song.id) | ||
| .setData(song.title.toByteArray()) | ||
| .build() | ||
| DownloadService.sendAddDownload( | ||
| context, | ||
| ExoDownloadService::class.java, | ||
| downloadRequest, | ||
| false, | ||
| ) | ||
| coroutineScope.launch(Dispatchers.IO) { | ||
| try { | ||
| val firstSong = songs.first() | ||
| val formats = com.metrolist.music.utils.YTPlayerUtils | ||
| .getAllAvailableAudioFormats(firstSong.id) | ||
| .getOrNull() ?: emptyList() | ||
| withContext(Dispatchers.Main) { | ||
| availableFormats = formats | ||
| isLoadingFormats = false | ||
| } | ||
| } catch (e: Exception) { | ||
| Timber.tag("YouTubePlaylistMenu").e(e, "Failed to fetch formats") | ||
| withContext(Dispatchers.Main) { | ||
| isLoadingFormats = false | ||
| showDownloadFormatDialog = false | ||
| } | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
| add( | ||
| Material3MenuItemData( | ||
| title = { Text(text = stringResource(R.string.export_playlist)) }, | ||
| icon = { | ||
| Icon( | ||
| painter = painterResource(R.drawable.share), | ||
| contentDescription = null, | ||
| } | ||
| } | ||
| ) | ||
| }, | ||
| onClick = { showExportDialog = true }, | ||
| ), | ||
| } | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
Keep downloads available when only PlaylistItem is loaded.
songs defaults to empty in this composable, and the play-next/add-to-queue/import branches already fetch the playlist lazily. Wrapping the new download block in if (songs.isNotEmpty()) removes download support entirely for those entry points instead of fetching on demand.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubePlaylistMenu.kt`
around lines 583 - 658, The download menu is hidden by the outer if
(songs.isNotEmpty()) which prevents lazy-loading downloads when only a
PlaylistItem is present; remove that outer guard so the download
Material3MenuItemData is always added, and inside its onClick replace the early
return (if (songs.isEmpty()) return@Material3MenuItemData) with logic to lazily
load the playlist items first (using the same lazy-fetch routine used by
play-next/add-to-queue/import branches) before proceeding to set
showDownloadFormatDialog = true, isLoadingFormats = true, availableFormats =
emptyList() and launching the coroutine that calls
com.metrolist.music.utils.YTPlayerUtils.getAllAvailableAudioFormats(firstSong.id);
keep using coroutineScope.launch(Dispatchers.IO) and the existing error handling
that sets isLoadingFormats/showDownloadFormatDialog on the Main dispatcher.
Comment on lines
+413
to
+420
| val displayPath = try { | ||
| val uri = Uri.parse(customDownloadPathUri) | ||
| // Extract full path from URI (e.g., "primary:Music/Metrolist" -> "Music/Metrolist") | ||
| val docId = android.provider.DocumentsContract.getTreeDocumentId(uri) | ||
| docId?.substringAfter(":")?.ifEmpty { docId } ?: DocumentFile.fromTreeUri(context, uri)?.name ?: customDownloadPathUri | ||
| } catch (e: Exception) { | ||
| customDownloadPathUri | ||
| } |
There was a problem hiding this comment.
Log the swallowed exception for debugging.
The exception caught when parsing the display path is silently ignored. While the fallback to customDownloadPathUri is reasonable, logging the exception would aid debugging path parsing issues.
🛠️ Proposed fix
val displayPath = try {
val uri = Uri.parse(customDownloadPathUri)
// Extract full path from URI (e.g., "primary:Music/Metrolist" -> "Music/Metrolist")
val docId = android.provider.DocumentsContract.getTreeDocumentId(uri)
docId?.substringAfter(":")?.ifEmpty { docId } ?: DocumentFile.fromTreeUri(context, uri)?.name ?: customDownloadPathUri
} catch (e: Exception) {
+ Timber.tag(TAG).w(e, "Failed to parse display path")
customDownloadPathUri
}🧰 Tools
🪛 detekt (1.23.8)
[warning] 418-418: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/StorageSettings.kt`
around lines 413 - 420, The catch block that swallows exceptions when computing
displayPath in StorageSettings (the block using customDownloadPathUri,
Uri.parse, DocumentsContract.getTreeDocumentId and DocumentFile.fromTreeUri)
should log the caught exception for debugging; update the catch (e: Exception)
branch to call the app logger or Android Log (e.g., Log.w or your processLogger)
with a clear message like "Failed to parse custom download path" and include the
exception (e) before returning customDownloadPathUri so the fallback remains
unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt (4)
424-431:⚠️ Potential issue | 🟡 MinorPreserve extension when truncating long filenames.
take(200)is applied to the fullname.ext, so long titles can drop the extension and produce untyped files.Safer truncation
private fun sanitizeFilename(filename: String): String { - return filename - .replace(Regex("[\\\\/:*?\"<>|]"), "_") - .replace(Regex("\\s+"), " ") - .trim() - .take(200) + val dot = filename.lastIndexOf('.') + val hasExt = dot > 0 && dot < filename.length - 1 + val base = (if (hasExt) filename.substring(0, dot) else filename) + .replace(Regex("[\\\\/:*?\"<>|]"), "_") + .replace(Regex("\\s+"), " ") + .trim() + val ext = if (hasExt) filename.substring(dot).replace(Regex("[\\\\/:*?\"<>|]"), "_") else "" + val maxBase = (200 - ext.length).coerceAtLeast(1) + return base.take(maxBase) + ext }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt` around lines 424 - 431, sanitizeFilename currently truncates the entire filename with take(200) which can drop the extension; change it to preserve extensions by splitting the sanitized input into base name and extension (use the last '.' if present), truncate only the base to (200 - extension.length - 1) characters if needed, then reassemble as base + '.' + extension (or just base if no extension), ensuring both parts are sanitized (refer to sanitizeFilename function) and that edge cases like names with no extension or extremely long extensions are handled.
416-416:⚠️ Potential issue | 🟠 MajorMap
audio/webmto.webm(or remux before.ogg).Returning
"ogg"foraudio/webmcreates extension/container mismatch for verbatim copied bytes, which hurts compatibility.Minimal fix
- format.mimeType.contains("audio/webm") -> "ogg" + format.mimeType.contains("audio/webm") -> "webm"For MIME type "audio/webm", what is the correct file extension for exported container data when no remuxing is performed?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt` at line 416, The mapping that returns "ogg" for format.mimeType "audio/webm" in DownloadExportHelper is incorrect; update the MIME-to-extension logic so "audio/webm" maps to "webm" (or perform remuxing to Ogg before returning "ogg"); locate the mapping branch (the when/if that checks format.mimeType and currently contains format.mimeType.contains("audio/webm") -> "ogg") and change it to return "webm" (or add a remux step before returning "ogg") to ensure container and extension match.
141-163:⚠️ Potential issue | 🟠 MajorValidate cache-span completeness before exporting.
The current loop concatenates spans but does not verify contiguous coverage from offset 0 to expected length. Partial/holey cache data can be exported and then persisted as a valid
downloadUri.Suggested guard before copy loop
val sortedSpans = cachedSpans.sortedBy { it.position } +if (sortedSpans.first().position != 0L) { + newFile.delete() + return@withContext null +} +for (i in 1 until sortedSpans.size) { + val prev = sortedSpans[i - 1] + val cur = sortedSpans[i] + if (cur.position != prev.position + prev.length) { + newFile.delete() + return@withContext null + } +} for ((index, span) in sortedSpans.withIndex()) { + val source = span.file ?: run { + newFile.delete() + return@withContext null + } - span.file?.inputStream()?.use { input -> + source.inputStream().use { input -> val bytes = input.copyTo(out) totalBytesWritten += bytes } }#!/bin/bash # Verify whether span-contiguity/content-length checks exist in the export path. rg -n -C4 'getCachedSpans|sortedSpans|position|contentLength|getContentMetadata|return@withContext null' app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt` around lines 141 - 163, The export currently concatenates cached spans from downloadCache.getCachedSpans without verifying they cover the full expected content, so add a pre-check in the DownloadExportHelper export flow (before iterating sortedSpans) to validate contiguity from offset 0 to the expected content length: sort spans by position (sortedSpans), iterate to ensure the first span.position == 0 and that each subsequent span.position == previousEnd (previous span.position + span.length) with no gaps and that the final end == expectedContentLength (obtain expectedContentLength from the content metadata/getContentMetadata or caller-provided value); if the check fails delete newFile and return@withContext null instead of writing partial data.
115-127:⚠️ Potential issue | 🟠 MajorMake replacement export failure-safe before touching the existing file.
Line 119 deletes the current file before the new file and DB state are known-good. If copy/embed/update fails, users can lose the previous download (matches the swap-cancel symptom), and failures after file creation can leave orphan files.
Suggested flow change
-// Check if file already exists in album folder and delete it -val existingFile = albumFolder.findFile(sanitizedFilename) -if (existingFile != null) { - Timber.tag(TAG).d("Existing file found, deleting...") - existingFile.delete() -} +// Keep current file until replacement is fully successful +val existingFile = albumFolder.findFile(sanitizedFilename) - val newFile = albumFolder.createFile(mimeType, sanitizedFilename) ?: run { + val tempName = "${sanitizedFilename}.tmp.${System.currentTimeMillis()}" + val newFile = albumFolder.createFile(mimeType, tempName) ?: run { Timber.tag(TAG).e("Failed to create file: $sanitizedFilename") return@withContext null } ... - database.updateDownloadUri(songId, exportedUri) + database.updateDownloadUri(songId, exportedUri) + existingFile?.delete()Also applies to: 252-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt` around lines 115 - 127, The code currently deletes the existing file via albumFolder.findFile(...).delete() before the new file is fully created and updated, risking data loss; change the flow in DownloadExportHelper (the block using albumFolder.findFile, albumFolder.createFile and the surrounding withContext) to create the new file first (preferably as a temp/temporary filename), perform the copy/embed/update steps to that new temp file, verify success, then atomically replace or delete the old file (delete existingFile only after new file operations succeed) and rename/move the temp file to sanitizedFilename; apply the same change to the similar block around lines 252-263 so that failure paths keep the original file and avoid orphan files.app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.kt (1)
154-170:⚠️ Potential issue | 🟠 MajorDon't delete the current download until the replacement is confirmed.
The swap path still removes the completed download before the user picks a new format. If the picker is dismissed or format loading fails, the user loses the only local copy. Fetch the formats first, then do the remove/requeue from
onFormatSelectedonce a new format has actually been chosen.🛠️ Minimal direction for the fix
onClick = { Timber.tag("YouTubeSongMenu").d("Swap download clicked for: ${song.id}") showDownloadFormatDialog = true isLoadingFormats = true + availableFormats = emptyList() coroutineScope.launch(Dispatchers.IO) { - // First remove existing download - DownloadService.sendRemoveDownload( - context, - ExoDownloadService::class.java, - song.id, - false, - ) val result = com.metrolist.music.utils.YTPlayerUtils.getAllAvailableAudioFormats(song.id) result.onSuccess { formats -> availableFormats = formats isLoadingFormats = false }.onFailure { availableFormats = emptyList() isLoadingFormats = false } } }Then move the remove/requeue step into
onFormatSelected.Also applies to: 638-659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.kt` around lines 154 - 170, The current flow removes the existing download before the user confirms a replacement; instead, fetch and display formats without deleting the current file, and move the remove/requeue logic into the onFormatSelected handler so deletion only happens after a user picks a format and the new download is enqueued. Update YouTubeSongMenu: keep format-fetching code as-is but ensure it does not call any removal/requeue routines, then inside the onFormatSelected lambda (where downloadUtil.setTargetItag, DownloadRequest.Builder, and DownloadService.sendAddDownload are used) invoke the existing remove/requeue operations (the code that deletes the completed download and requeues any metadata) immediately before or after setTargetItag so the old file is only removed once a new format is actually chosen and enqueued.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.kt`:
- Around line 145-147: showDownloadFormatDialog is saveable but availableFormats
and isLoadingFormats are ephemeral, causing the dialog to be restored without
its data/state; make the picker state consistent by either (A) converting
availableFormats and isLoadingFormats to rememberSaveable (ensure
AudioFormatOption is parcelable/serializable or provide a saver for
List<com.metrolist.music.utils.YTPlayerUtils.AudioFormatOption>) so they survive
recreation alongside showDownloadFormatDialog, or (B) make
showDownloadFormatDialog ephemeral (use remember instead of rememberSaveable) so
all three (showDownloadFormatDialog, availableFormats, isLoadingFormats) are
non-restorable; update the state declarations referencing
showDownloadFormatDialog, availableFormats, and isLoadingFormats accordingly.
---
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.kt`:
- Around line 154-170: The current flow removes the existing download before the
user confirms a replacement; instead, fetch and display formats without deleting
the current file, and move the remove/requeue logic into the onFormatSelected
handler so deletion only happens after a user picks a format and the new
download is enqueued. Update YouTubeSongMenu: keep format-fetching code as-is
but ensure it does not call any removal/requeue routines, then inside the
onFormatSelected lambda (where downloadUtil.setTargetItag,
DownloadRequest.Builder, and DownloadService.sendAddDownload are used) invoke
the existing remove/requeue operations (the code that deletes the completed
download and requeues any metadata) immediately before or after setTargetItag so
the old file is only removed once a new format is actually chosen and enqueued.
In `@app/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt`:
- Around line 424-431: sanitizeFilename currently truncates the entire filename
with take(200) which can drop the extension; change it to preserve extensions by
splitting the sanitized input into base name and extension (use the last '.' if
present), truncate only the base to (200 - extension.length - 1) characters if
needed, then reassemble as base + '.' + extension (or just base if no
extension), ensuring both parts are sanitized (refer to sanitizeFilename
function) and that edge cases like names with no extension or extremely long
extensions are handled.
- Line 416: The mapping that returns "ogg" for format.mimeType "audio/webm" in
DownloadExportHelper is incorrect; update the MIME-to-extension logic so
"audio/webm" maps to "webm" (or perform remuxing to Ogg before returning "ogg");
locate the mapping branch (the when/if that checks format.mimeType and currently
contains format.mimeType.contains("audio/webm") -> "ogg") and change it to
return "webm" (or add a remux step before returning "ogg") to ensure container
and extension match.
- Around line 141-163: The export currently concatenates cached spans from
downloadCache.getCachedSpans without verifying they cover the full expected
content, so add a pre-check in the DownloadExportHelper export flow (before
iterating sortedSpans) to validate contiguity from offset 0 to the expected
content length: sort spans by position (sortedSpans), iterate to ensure the
first span.position == 0 and that each subsequent span.position == previousEnd
(previous span.position + span.length) with no gaps and that the final end ==
expectedContentLength (obtain expectedContentLength from the content
metadata/getContentMetadata or caller-provided value); if the check fails delete
newFile and return@withContext null instead of writing partial data.
- Around line 115-127: The code currently deletes the existing file via
albumFolder.findFile(...).delete() before the new file is fully created and
updated, risking data loss; change the flow in DownloadExportHelper (the block
using albumFolder.findFile, albumFolder.createFile and the surrounding
withContext) to create the new file first (preferably as a temp/temporary
filename), perform the copy/embed/update steps to that new temp file, verify
success, then atomically replace or delete the old file (delete existingFile
only after new file operations succeed) and rename/move the temp file to
sanitizedFilename; apply the same change to the similar block around lines
252-263 so that failure paths keep the original file and avoid orphan files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3043f093-1174-4845-84f9-36730bc7e99a
📒 Files selected for processing (2)
app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.ktapp/src/main/kotlin/com/metrolist/music/utils/DownloadExportHelper.kt
Comment on lines
+145
to
+147
| var showDownloadFormatDialog by rememberSaveable { mutableStateOf(false) } | ||
| var availableFormats by remember { mutableStateOf<List<com.metrolist.music.utils.YTPlayerUtils.AudioFormatOption>>(emptyList()) } | ||
| var isLoadingFormats by remember { mutableStateOf(false) } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and the specific lines mentioned
wc -l app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.ktRepository: MetrolistGroup/Metrolist
Length of output: 138
🏁 Script executed:
# Get context around lines 145-147 and the surrounding state management
sed -n '140,175p' app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.kt | cat -nRepository: MetrolistGroup/Metrolist
Length of output: 1898
🏁 Script executed:
# Get context around the swap-download flow (lines 638-659)
sed -n '633,665p' app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.kt | cat -nRepository: MetrolistGroup/Metrolist
Length of output: 2324
🏁 Script executed:
# Search for all usages of these three state variables within the file
rg -n "showDownloadFormatDialog|availableFormats|isLoadingFormats" app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.ktRepository: MetrolistGroup/Metrolist
Length of output: 1382
🏁 Script executed:
# Look for LaunchedEffect or other effects that depend on showDownloadFormatDialog
sed -n '145,700p' app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.kt | grep -n -A5 -B5 "LaunchedEffect\|DisposableEffect" | head -80Repository: MetrolistGroup/Metrolist
Length of output: 50
Keep the picker state either fully saveable or fully ephemeral.
showDownloadFormatDialog survives recreation, but availableFormats and isLoadingFormats do not. When the dialog visibility is restored (e.g., after device rotation), it reappears empty with no loading state, creating inconsistent UX. Either make all picker state restorable together or keep the visibility flag non-saveable too.
♻️ Proposed fix
- var showDownloadFormatDialog by rememberSaveable { mutableStateOf(false) }
+ var showDownloadFormatDialog by remember { mutableStateOf(false) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var showDownloadFormatDialog by rememberSaveable { mutableStateOf(false) } | |
| var availableFormats by remember { mutableStateOf<List<com.metrolist.music.utils.YTPlayerUtils.AudioFormatOption>>(emptyList()) } | |
| var isLoadingFormats by remember { mutableStateOf(false) } | |
| var showDownloadFormatDialog by remember { mutableStateOf(false) } | |
| var availableFormats by remember { mutableStateOf<List<com.metrolist.music.utils.YTPlayerUtils.AudioFormatOption>>(emptyList()) } | |
| var isLoadingFormats by remember { mutableStateOf(false) } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/menu/YouTubeSongMenu.kt` around
lines 145 - 147, showDownloadFormatDialog is saveable but availableFormats and
isLoadingFormats are ephemeral, causing the dialog to be restored without its
data/state; make the picker state consistent by either (A) converting
availableFormats and isLoadingFormats to rememberSaveable (ensure
AudioFormatOption is parcelable/serializable or provide a saver for
List<com.metrolist.music.utils.YTPlayerUtils.AudioFormatOption>) so they survive
recreation alongside showDownloadFormatDialog, or (B) make
showDownloadFormatDialog ephemeral (use remember instead of rememberSaveable) so
all three (showDownloadFormatDialog, availableFormats, isLoadingFormats) are
non-restorable; update the state declarations referencing
showDownloadFormatDialog, availableFormats, and isLoadingFormats accordingly.
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
5 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.
Summary
New Features
Bug Fixes
Build Changes
Test plan
Screenshots
Summary by CodeRabbit
New Features
Chores
Database