feat(download): Add file validation and cleanup #164
Conversation
This commit introduces several enhancements to the download functionality:
- **File Validation:** Before starting a new download, the system now checks if the file already exists. If it does, it validates the file size against the expected size. The download is skipped if the sizes match, preventing redundant downloads.
- **Post-Download Verification:** After a download completes, the size of the downloaded file is verified to ensure its integrity.
- **Automatic Cleanup:** The downloads directory is now automatically cleaned of files that do not belong to the current repository's release assets.
- **Code Refactoring:**
- A new `DownloadedFile` data class has been created.
- The `Downloader` interface and its implementations (`AndroidDownloader`, `DesktopDownloader`) have been updated to support listing downloaded files, getting file sizes, and changing the download directory from the user's public downloads to a dedicated application downloads directory.
- The download cancellation logic in `DetailsViewModel`'s `onCleared` method has been simplified.
WalkthroughThis PR introduces a new DownloadedFile data model and extends the Downloader interface with query methods (listDownloadedFiles, getLatestDownload, getFileSize, getLatestDownloadForAssets) across Android and Desktop platforms. It adds pre/post-download validation to detect existing valid files and skip redundant downloads, plus cleanup logic to remove downloads from other repositories. Changes
Sequence DiagramsequenceDiagram
participant DVM as DetailsViewModel
participant DL as Downloader
participant FS as File System
DVM->>DL: Check if file exists (getLatestDownloadForAssets)
DL->>FS: List downloaded files
FS-->>DL: Return file metadata
DL-->>DVM: Return DownloadedFile or null
alt File exists and valid
DVM->>DVM: Log Downloaded state, skip download
else File missing or invalid
DVM->>DL: Download asset
DL->>FS: Save to app downloads dir
FS-->>DL: File saved
DL-->>DVM: Return download path
DVM->>DL: Verify file size (getFileSize)
DL->>FS: Check file length
FS-->>DL: Return file size
DL-->>DVM: Return file size
DVM->>DVM: Validate size matches expected
end
DVM->>DL: Cleanup old downloads (listDownloadedFiles)
DL->>FS: List all downloads
FS-->>DL: Return all downloaded files
DL-->>DVM: Return file list
DVM->>DVM: Filter files from other repos
DVM->>FS: Delete stale files
FS-->>DVM: Deletion complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/details/presentation/DetailsViewModel.kt (1)
455-472: Missing file validation in OpenInAppManager flow.The
installAssetfunction now includes pre-download and post-download size validation, but theOpenInAppManageraction (lines 455-472) directly downloads without these checks. This could result in opening corrupted or incomplete files in AppManager.Consider extracting the validation logic into a shared helper or applying the same pattern here.
🤖 Fix all issues with AI agents
In
@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/details/presentation/DetailsViewModel.kt:
- Around line 196-215: The final log incorrectly reports all files as removed;
modify the cleanup loop in DetailsViewModel (the block using
filesToDelete.forEach and downloader.cancelDownload) to track an actual success
counter (e.g., successCount = 0), increment it only when cancelDownload returns
true, and then log "Cleanup complete - X files removed" using that successCount
instead of filesToDelete.size; keep existing per-file success/failure/error logs
unchanged so failures are still reported.
🧹 Nitpick comments (5)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/core/domain/model/DownloadedFile.kt (1)
1-8: Looks good; clarifydownloadedAtunit (epoch millis vs seconds)Since
downloadedAtis a rawLong, consider renaming todownloadedAtEpochMillis(or add a short KDoc) to prevent unit mixups across platforms.composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/core/data/services/Downloader.kt (1)
16-22: Tighten API contract forgetFileSize(filePath)+ drop inline “new method” comment
- The
// Add this new methodcomment (Line 20) reads like a temporary note—can be removed.- Consider constraining
getFileSizeto the app downloads domain (e.g., acceptfileNameor document thatfilePathmust be withinappDownloadsDir()), to avoid callers probing arbitrary filesystem paths and to keep semantics consistent withlistDownloadedFiles().composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/core/data/services/AndroidDownloader.kt (1)
179-223: Avoid redundantwithContext(IO)nesting; consider scopinggetFileSizeto app downloads dir
getLatestDownload()/getLatestDownloadForAssets()wrapwithContext(IO)but immediately calllistDownloadedFiles()which already doeswithContext(IO)—can be simplified.getFileSize(filePath)currently allows arbitrary paths; if callers can pass untrusted values, validate that the canonical path is withinfiles.appDownloadsDir().Proposed simplification (no behavior change)
- override suspend fun getLatestDownload(): DownloadedFile? = withContext(Dispatchers.IO) { - listDownloadedFiles().firstOrNull() - } + override suspend fun getLatestDownload(): DownloadedFile? = + listDownloadedFiles().firstOrNull() - override suspend fun getLatestDownloadForAssets(assetNames: List<String>): DownloadedFile? = - withContext(Dispatchers.IO) { - listDownloadedFiles() - .firstOrNull { downloadedFile -> - assetNames.any { assetName -> - downloadedFile.fileName == assetName - } - } - } + override suspend fun getLatestDownloadForAssets(assetNames: List<String>): DownloadedFile? = + listDownloadedFiles() + .firstOrNull { downloadedFile -> + assetNames.any { assetName -> downloadedFile.fileName == assetName } + }composeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/core/data/services/DesktopDownloader.kt (1)
132-177: Same follow-ups as Android: de-nestwithContext(IO)and scopegetFileSize
getLatestDownload()andgetLatestDownloadForAssets()can drop their ownwithContext(IO)sincelistDownloadedFiles()already handles IO.- Consider validating
filePath(canonicalized) is underfiles.appDownloadsDir()(or change the API to acceptfileName) to prevent arbitrary path probing.composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/details/presentation/DetailsViewModel.kt (1)
793-804: Consider adding file validation todownloadAssetfor consistency.The
downloadAssetfunction lacks the pre-download and post-download validation added toinstallAsset. While this may be intentional for non-installable assets, adding size verification would ensure download integrity across all download paths.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/core/data/services/AndroidDownloader.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/core/data/services/Downloader.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/core/domain/model/DownloadedFile.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/details/presentation/DetailsViewModel.ktcomposeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/core/data/services/DesktopDownloader.kt
🧰 Additional context used
🧬 Code graph analysis (2)
composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/core/data/services/AndroidDownloader.kt (2)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/core/data/services/Downloader.kt (1)
listDownloadedFiles(16-16)composeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/core/data/services/DesktopDownloader.kt (1)
listDownloadedFiles(132-148)
composeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/core/data/services/DesktopDownloader.kt (2)
composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/core/data/services/AndroidDownloader.kt (1)
listDownloadedFiles(179-195)composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/core/data/services/Downloader.kt (1)
listDownloadedFiles(16-16)
🔇 Additional comments (4)
composeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/core/data/services/DesktopDownloader.kt (1)
27-29: Directory change toappDownloadsDir()is consistent with the new storage modelNice alignment across
download,saveToFile,getDownloadedFilePath, andcancelDownload, and the log message now matches the new directory.Also applies to: 86-87, 105-106, 116-123
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/details/presentation/DetailsViewModel.kt (3)
550-551: LGTM!Clean ternary expressions for differentiating update vs new install log results.
855-860: LGTM!Proper cleanup: cancelling the job and nullifying the reference prevents dangling references.
563-617: Pre/post-download validation logic looks good.The implementation correctly:
- Skips redundant downloads when a valid file already exists
- Re-downloads when file size mismatches
- Verifies downloaded file integrity before proceeding to install
getFileSizeis implemented safely across all platforms (Android and Desktop): it returnsLong?(nullable), never -1. On missing files or I/O errors, it returnsnull, which correctly fails the size validation check at line 568 and triggers re-download as intended.
| if (filesToDelete.isNotEmpty()) { | ||
| Logger.d { "Cleaning up ${filesToDelete.size} files from other repositories" } | ||
|
|
||
| filesToDelete.forEach { file -> | ||
| try { | ||
| val deleted = downloader.cancelDownload(file.fileName) | ||
| if (deleted) { | ||
| Logger.d { "✓ Cleaned up file from other repo: ${file.fileName}" } | ||
| } else { | ||
| Logger.w { "✗ Failed to delete file: ${file.fileName}" } | ||
| } | ||
| } catch (e: Exception) { | ||
| Logger.e { "✗ Error deleting ${file.fileName}: ${e.message}" } | ||
| } | ||
| } | ||
|
|
||
| Logger.d { "Cleanup complete - ${filesToDelete.size} files removed" } | ||
| } else { | ||
| Logger.d { "No files from other repos to clean up" } | ||
| } |
There was a problem hiding this comment.
Misleading log message: claims all files removed even when some deletions fail.
The log at line 212 reports "Cleanup complete - ${filesToDelete.size} files removed", but previous iterations may have failed (logged at lines 205 and 208). Consider tracking actual success count:
Suggested fix
+ var successCount = 0
filesToDelete.forEach { file ->
try {
val deleted = downloader.cancelDownload(file.fileName)
if (deleted) {
Logger.d { "✓ Cleaned up file from other repo: ${file.fileName}" }
+ successCount++
} else {
Logger.w { "✗ Failed to delete file: ${file.fileName}" }
}
} catch (e: Exception) {
Logger.e { "✗ Error deleting ${file.fileName}: ${e.message}" }
}
}
- Logger.d { "Cleanup complete - ${filesToDelete.size} files removed" }
+ Logger.d { "Cleanup complete - $successCount/${filesToDelete.size} files removed" }🤖 Prompt for AI Agents
In
@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/details/presentation/DetailsViewModel.kt
around lines 196 - 215, The final log incorrectly reports all files as removed;
modify the cleanup loop in DetailsViewModel (the block using
filesToDelete.forEach and downloader.cancelDownload) to track an actual success
counter (e.g., successCount = 0), increment it only when cancelDownload returns
true, and then log "Cleanup complete - X files removed" using that successCount
instead of filesToDelete.size; keep existing per-file success/failure/error logs
unchanged so failures are still reported.
This commit introduces several enhancements to the download functionality:
DownloadedFiledata class has been created.Downloaderinterface and its implementations (AndroidDownloader,DesktopDownloader) have been updated to support listing downloaded files, getting file sizes, and changing the download directory from the user's public downloads to a dedicated application downloads directory.DetailsViewModel'sonClearedmethod has been simplified.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.