Feat/download notification#849
Closed
eddyizm wants to merge 11 commits into
Closed
Conversation
…ssing downloaded files
… in `DownloadFragment`
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/src/main/java/com/cappielloantonio/tempo/model/Download.kt (1)
10-26: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMove
queuePositioninto the primary constructor.@Parcelizeonly writes constructor properties, so this body field is dropped whenDownloadis sent through aBundle, and the queue order comes back as0.app/src/main/java/com/cappielloantonio/tempo/model/Download.kt:24-25🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/cappielloantonio/tempo/model/Download.kt` around lines 10 - 26, Move the queuePosition property into the Download primary constructor so `@Parcelize` includes it when the object is passed through a Bundle. Update the Download class definition to make queuePosition a constructor property alongside id, playlistId, playlistName, downloadState, and downloadUri, and remove the separate body field so the saved queue order is preserved.app/src/main/java/com/cappielloantonio/tempo/util/ExternalAudioWriter.java (1)
109-113: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThree
notifyFailurecalls bypass theemitNotificationsgate.
performDownloadgates most notifications onemitNotifications, but the "Invalid media URI" (Line 110), "Unsupported media URI" (Line 160), and "Failed to create file" (Line 222) failures still fire unconditionally. WhenexportDownloadByIdinvokesperformDownload(..., false)to run a silent background export, any of these paths will still surface a "Download failed" notification to the user even thoughDownloadManageralready reported the item as completed — defeating the suppression this PR introduces.Gate all three on
emitNotificationsfor consistency.🔧 Proposed gating
- if (mediaUri == null) { - notifyFailure(context, "Invalid media URI."); + if (mediaUri == null) { + if (emitNotifications) notifyFailure(context, "Invalid media URI."); ExternalDownloadMetadataStore.remove(metadataKey); return; }} else { - notifyFailure(context, "Unsupported media URI."); + if (emitNotifications) notifyFailure(context, "Unsupported media URI."); ExternalDownloadMetadataStore.remove(metadataKey); return; }targetFile = directory.createFile(mimeType, fileName); if (targetFile == null) { - notifyFailure(context, "Failed to create file."); + if (emitNotifications) notifyFailure(context, "Failed to create file."); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/cappielloantonio/tempo/util/ExternalAudioWriter.java` around lines 109 - 113, The `performDownload` flow in `ExternalAudioWriter` still sends failure notifications even when `emitNotifications` is false. Update the three unconditional `notifyFailure` paths in `performDownload`—the invalid media URI check, the unsupported media URI branch, and the file creation failure branch—to respect the same `emitNotifications` gate used elsewhere, so `exportDownloadById` can run silently. Keep the cleanup logic (`ExternalDownloadMetadataStore.remove`, file deletion, etc.) unchanged and only suppress the user-facing failure notification when notifications are disabled.app/src/main/java/com/cappielloantonio/tempo/ui/fragment/bottomsheetdialog/SongBottomSheetDialog.java (1)
351-356: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear or bind
albumLinkChiphere
albumLinkChipis still inflated, but nothing resets its defaultVISIBLEstate or click/text handlers. That letssyncAssetLinkGroupVisibility()treat it as visible, so the row can surface with an empty, inert chip when the song/artist chips are absent. Either clear it explicitly or drop the lookup if album links are no longer shown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/cappielloantonio/tempo/ui/fragment/bottomsheetdialog/SongBottomSheetDialog.java` around lines 351 - 356, The album link chip is being inflated in SongBottomSheetDialog but never bound or hidden, so it can remain visible with no behavior. In the song details setup where albumLinkChip, artistLinkChip, and bindAssetLinkChip are used, either explicitly reset albumLinkChip to a hidden/non-interactive state before calling syncAssetLinkGroupVisibility() or remove the albumLinkChip lookup entirely if album links are no longer supported. Make sure the visibility logic only considers chips that are actually bound and actionable.
🧹 Nitpick comments (2)
app/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.java (2)
156-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffHardcoded user-facing notification strings.
Titles/body/action labels ("Downloads Paused", "Waiting for WiFi connection...", "Downloading (%d of %d)", "Pause"/"Resume"/"Done", "%d tracks downloaded successfully", etc.) are inline English literals. The rest of the app uses
R.string/plurals, so these won't localize. Recommend extracting to string resources (with plurals where counts are involved).Also applies to: 205-231, 242-249
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.java` around lines 156 - 172, The notification UI in DownloaderService still uses hardcoded English literals for titles, body text, and action labels, so those strings will not be localized. Move the user-facing text in the notification-building logic (including the paused/downloading/preparing messages and the action labels) into R.string resources, and use plurals for count-based messages such as completed/downloaded totals. Update the related notification setup methods in DownloaderService so they reference the resource IDs instead of inline strings, matching the app’s existing localization pattern.
300-308: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid silently swallowing export failures.
The catch block discards all exceptions with no logging, so a broken export (e.g., store read failure,
exportDownloadByIderror) leaves no trace. Log at warn level to preserve diagnostics while still not disrupting notification flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.java` around lines 300 - 308, The catch block in DownloaderService is silently discarding all exceptions without any logging, making it impossible to diagnose export failures from ExternalDownloadMetadataStore or ExternalAudioWriter methods. Add a warn-level log statement in the catch block that includes the exception details (using the Exception e parameter) to preserve diagnostic information while maintaining the current behavior of not disrupting notification handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/cappielloantonio/tempo/repository/DownloadRepository.java`:
- Around line 212-222: Make queue-position assignment atomic in
DownloadRepository by removing the separate read-then-add flow around
getMaxQueuePositionSync(); the current MAX(queue_position) lookup in
GetMaxQueuePositionThreadSafe can race when multiple callers later add 1 and
insert. Update the download creation path so the max-read and insert happen
together in one transaction or use a single SQL statement/DAO method that
returns and persists the next queue position in one step, and route callers to
that atomic path instead of relying on getMaxQueuePositionSync().
In `@app/src/main/java/com/cappielloantonio/tempo/service/DownloaderManager.java`:
- Around line 76-104: The enqueue path in DownloaderManager.download(...) is
doing synchronous repository lookups for the max queue position on the caller
thread, which can block UI interactions. Move queue-position assignment into the
existing background insert flow used by insertDatabase(...) or the download-list
processing path, and keep only lightweight state setup in download(...). Use the
DownloaderManager.download(MediaItem, Download, Integer) and
download(List<MediaItem>, List<Download>) methods as the places to reroute this
work so the queue position is resolved off the main thread.
In `@app/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.java`:
- Around line 233-250: The “Done” action in DownloaderService’s completion
notification is a no-op because the PendingIntent uses an empty Intent with no
receiver, so tapping the action will not dismiss the notification. Update the
dismiss flow in the notification builder to either remove the action entirely or
route it through a real broadcast receiver that explicitly cancels the
completion notification via
NotificationManager.cancel(COMPLETION_NOTIFICATION_ID). Locate the notification
setup in DownloaderService around the builder/addAction usage and make the
action actually close the completion notification.
In
`@app/src/main/java/com/cappielloantonio/tempo/ui/fragment/DownloadFragment.java`:
- Around line 150-154: The count == 0 path in DownloadFragment’s refresh logic
is recursively re-entering the same refresh flow through
reconcileMissingFiles(), which can keep scheduling background work on the common
no-changes case. Remove that recursive call from reconcileMissingFiles() or
change the helper to perform its file check without calling
refreshExternalDownloads() again, and keep the no-changes toast path in the
existing refresh branch.
---
Outside diff comments:
In `@app/src/main/java/com/cappielloantonio/tempo/model/Download.kt`:
- Around line 10-26: Move the queuePosition property into the Download primary
constructor so `@Parcelize` includes it when the object is passed through a
Bundle. Update the Download class definition to make queuePosition a constructor
property alongside id, playlistId, playlistName, downloadState, and downloadUri,
and remove the separate body field so the saved queue order is preserved.
In
`@app/src/main/java/com/cappielloantonio/tempo/ui/fragment/bottomsheetdialog/SongBottomSheetDialog.java`:
- Around line 351-356: The album link chip is being inflated in
SongBottomSheetDialog but never bound or hidden, so it can remain visible with
no behavior. In the song details setup where albumLinkChip, artistLinkChip, and
bindAssetLinkChip are used, either explicitly reset albumLinkChip to a
hidden/non-interactive state before calling syncAssetLinkGroupVisibility() or
remove the albumLinkChip lookup entirely if album links are no longer supported.
Make sure the visibility logic only considers chips that are actually bound and
actionable.
In `@app/src/main/java/com/cappielloantonio/tempo/util/ExternalAudioWriter.java`:
- Around line 109-113: The `performDownload` flow in `ExternalAudioWriter` still
sends failure notifications even when `emitNotifications` is false. Update the
three unconditional `notifyFailure` paths in `performDownload`—the invalid media
URI check, the unsupported media URI branch, and the file creation failure
branch—to respect the same `emitNotifications` gate used elsewhere, so
`exportDownloadById` can run silently. Keep the cleanup logic
(`ExternalDownloadMetadataStore.remove`, file deletion, etc.) unchanged and only
suppress the user-facing failure notification when notifications are disabled.
---
Nitpick comments:
In `@app/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.java`:
- Around line 156-172: The notification UI in DownloaderService still uses
hardcoded English literals for titles, body text, and action labels, so those
strings will not be localized. Move the user-facing text in the
notification-building logic (including the paused/downloading/preparing messages
and the action labels) into R.string resources, and use plurals for count-based
messages such as completed/downloaded totals. Update the related notification
setup methods in DownloaderService so they reference the resource IDs instead of
inline strings, matching the app’s existing localization pattern.
- Around line 300-308: The catch block in DownloaderService is silently
discarding all exceptions without any logging, making it impossible to diagnose
export failures from ExternalDownloadMetadataStore or ExternalAudioWriter
methods. Add a warn-level log statement in the catch block that includes the
exception details (using the Exception e parameter) to preserve diagnostic
information while maintaining the current behavior of not disrupting
notification handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f206388-e22a-4100-ac88-88326f79c9fc
📒 Files selected for processing (21)
.gitignoreapp/schemas/com.cappielloantonio.tempo.database.AppDatabase/20.jsonapp/schemas/com.cappielloantonio.tempo.database.AppDatabase/21.jsonapp/src/main/java/com/cappielloantonio/tempo/database/dao/DownloadDao.javaapp/src/main/java/com/cappielloantonio/tempo/model/Download.ktapp/src/main/java/com/cappielloantonio/tempo/repository/DownloadRepository.javaapp/src/main/java/com/cappielloantonio/tempo/service/DownloaderManager.javaapp/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.javaapp/src/main/java/com/cappielloantonio/tempo/ui/activity/MainActivity.javaapp/src/main/java/com/cappielloantonio/tempo/ui/fragment/AlbumPageFragment.javaapp/src/main/java/com/cappielloantonio/tempo/ui/fragment/DirectoryFragment.javaapp/src/main/java/com/cappielloantonio/tempo/ui/fragment/DownloadFragment.javaapp/src/main/java/com/cappielloantonio/tempo/ui/fragment/HomeTabMusicFragment.javaapp/src/main/java/com/cappielloantonio/tempo/ui/fragment/PlayerCoverFragment.javaapp/src/main/java/com/cappielloantonio/tempo/ui/fragment/PlayerQueueFragment.javaapp/src/main/java/com/cappielloantonio/tempo/ui/fragment/PlaylistPageFragment.javaapp/src/main/java/com/cappielloantonio/tempo/ui/fragment/bottomsheetdialog/AlbumBottomSheetDialog.javaapp/src/main/java/com/cappielloantonio/tempo/ui/fragment/bottomsheetdialog/SongBottomSheetDialog.javaapp/src/main/java/com/cappielloantonio/tempo/util/Constants.ktapp/src/main/java/com/cappielloantonio/tempo/util/ExternalAudioWriter.javaapp/src/main/java/com/cappielloantonio/tempo/util/ExternalDownloadMetadataStore.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes