Skip to content

feat(download): Add file validation and cleanup #164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rainxchzed wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(download): Add file validation and cleanup #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions ...pp/src/androidMain/kotlin/zed/rainxch/githubstore/core/data/services/AndroidDownloader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import kotlinx.coroutines.flow.flowOn
import zed.rainxch.githubstore.feature.details.domain.model.DownloadProgress
import java.util.concurrent.ConcurrentHashMap
import androidx.core.net.toUri
import zed.rainxch.githubstore.core.domain.model.DownloadedFile

class AndroidDownloader(
private val context: Context,
Expand Down Expand Up @@ -174,4 +175,50 @@ class AndroidDownloader(

cancelled || deleted
}

override suspend fun listDownloadedFiles(): List<DownloadedFile> = withContext(Dispatchers.IO) {
val dir = File(files.appDownloadsDir())
if (!dir.exists()) return@withContext emptyList()

dir.listFiles()
?.filter { it.isFile && it.length() > 0 }
?.map { file ->
DownloadedFile(
fileName = file.name,
filePath = file.absolutePath,
fileSizeBytes = file.length(),
downloadedAt = file.lastModified()
)
}
?.sortedByDescending { it.downloadedAt }
?: emptyList()
}

override suspend fun getLatestDownload(): DownloadedFile? = withContext(Dispatchers.IO) {
listDownloadedFiles().firstOrNull()
}

override suspend fun getFileSize(filePath: String): Long? = withContext(Dispatchers.IO) {
try {
val file = File(filePath)
if (file.exists() && file.isFile) {
file.length()
} else {
null
}
} catch (e: Exception) {
Logger.e { "Failed to get file size for $filePath: ${e.message}" }
null
}
}

override suspend fun getLatestDownloadForAssets(assetNames: List<String>): DownloadedFile? =
withContext(Dispatchers.IO) {
listDownloadedFiles()
.firstOrNull { downloadedFile ->
assetNames.any { assetName ->
downloadedFile.fileName == assetName
}
}
}
}
7 changes: 7 additions & 0 deletions composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/core/data/services/Downloader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package zed.rainxch.githubstore.core.data.services

import kotlinx.coroutines.flow.Flow
import zed.rainxch.githubstore.core.domain.model.DownloadedFile
import zed.rainxch.githubstore.feature.details.domain.model.DownloadProgress

interface Downloader {
Expand All @@ -12,4 +13,10 @@ interface Downloader {
suspend fun getDownloadedFilePath(fileName: String): String?

suspend fun cancelDownload(fileName: String): Boolean
suspend fun listDownloadedFiles(): List<DownloadedFile>
suspend fun getLatestDownload(): DownloadedFile?
suspend fun getLatestDownloadForAssets(assetNames: List<String>): DownloadedFile?

// Add this new method
suspend fun getFileSize(filePath: String): Long?
}
8 changes: 8 additions & 0 deletions composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/core/domain/model/DownloadedFile.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package zed.rainxch.githubstore.core.domain.model

data class DownloadedFile(
val fileName: String,
val filePath: String,
val fileSizeBytes: Long,
val downloadedAt: Long
)
115 changes: 90 additions & 25 deletions ...ommonMain/kotlin/zed/rainxch/githubstore/feature/details/presentation/DetailsViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import githubstore.composeapp.generated.resources.Res
import githubstore.composeapp.generated.resources.added_to_favourites
import githubstore.composeapp.generated.resources.installer_saved_downloads
import githubstore.composeapp.generated.resources.removed_from_favourites
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.async
import kotlinx.coroutines.channels.Channel
Expand Down Expand Up @@ -184,6 +185,39 @@ class DetailsViewModel(

val primary = installer.choosePrimaryAsset(installable)

launch(Dispatchers.IO) {
try {
val allFiles = downloader.listDownloadedFiles()
val currentRepoAssetNames = installable.map { it.name }.toSet()
val filesToDelete = allFiles.filter { file ->
file.fileName !in currentRepoAssetNames
}

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" }
}
Comment on lines +196 to +215
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

} catch (t: Throwable) {
Logger.e { "Failed to cleanup files from other repos: ${t.message}" }
}
}

val isObtainiumAvailable = installer.isObtainiumInstalled()
val isAppManagerAvailable = installer.isAppManagerInstalled()

Expand Down Expand Up @@ -513,10 +547,9 @@ class DetailsViewModel(
assetName = assetName,
size = sizeBytes,
tag = releaseTag,
result = if (isUpdate) {
LogResult.UpdateStarted
} else LogResult.DownloadStarted
result = if (isUpdate) LogResult.UpdateStarted else LogResult.DownloadStarted
)

_state.value = _state.value.copy(
downloadError = null,
installError = null,
Expand All @@ -527,23 +560,61 @@ class DetailsViewModel(
extOrMime = assetName.substringAfterLast('.', "").lowercase()
)

_state.value = _state.value.copy(downloadStage = DownloadStage.DOWNLOADING)
downloader.download(downloadUrl, assetName).collect { p ->
_state.value = _state.value.copy(downloadProgressPercent = p.percent)
if (p.percent == 100) {
_state.value = _state.value.copy(downloadStage = DownloadStage.VERIFYING)
// Check if file already exists and validate
val existingFilePath = downloader.getDownloadedFilePath(assetName)
val validatedFilePath = if (existingFilePath != null) {
// Verify file size matches expected
val fileSize = downloader.getFileSize(existingFilePath)
if (fileSize == sizeBytes) {
Logger.d { "File already exists with correct size ($fileSize bytes), skipping download: $existingFilePath" }
appendLog(
assetName = assetName,
size = sizeBytes,
tag = releaseTag,
result = LogResult.Downloaded
)
existingFilePath
} else {
Logger.w { "Existing file size mismatch (expected: $sizeBytes, found: $fileSize), re-downloading" }
downloader.cancelDownload(assetName)
null
}
} else {
null
}

val filePath = downloader.getDownloadedFilePath(assetName)
?: throw IllegalStateException("Downloaded file not found")
// Download if no valid file exists
val filePath = validatedFilePath ?: run {
_state.value = _state.value.copy(downloadStage = DownloadStage.DOWNLOADING)
downloader.download(downloadUrl, assetName).collect { p ->
_state.value = _state.value.copy(downloadProgressPercent = p.percent)
if (p.percent == 100) {
_state.value = _state.value.copy(downloadStage = DownloadStage.VERIFYING)
}
}

appendLog(
assetName = assetName,
size = sizeBytes,
tag = releaseTag,
result = LogResult.Downloaded
)
val downloadedPath = downloader.getDownloadedFilePath(assetName)
?: throw IllegalStateException("Downloaded file not found")

// Verify downloaded file size
val downloadedSize = downloader.getFileSize(downloadedPath)
if (downloadedSize != sizeBytes) {
Logger.e { "Downloaded file size mismatch (expected: $sizeBytes, got: $downloadedSize)" }
downloader.cancelDownload(assetName)
throw IllegalStateException("Downloaded file size mismatch - expected $sizeBytes bytes, got $downloadedSize bytes")
}

Logger.d { "Download verified - file size matches: $downloadedSize bytes" }

appendLog(
assetName = assetName,
size = sizeBytes,
tag = releaseTag,
result = LogResult.Downloaded
)

downloadedPath
}

_state.value = _state.value.copy(downloadStage = DownloadStage.INSTALLING)
val ext = assetName.substringAfterLast('.', "").lowercase()
Expand Down Expand Up @@ -575,9 +646,7 @@ class DetailsViewModel(
assetName = assetName,
size = sizeBytes,
tag = releaseTag,
result = if (isUpdate) {
LogResult.Updated
} else LogResult.Installed
result = if (isUpdate) LogResult.Updated else LogResult.Installed
)

} catch (t: Throwable) {
Expand Down Expand Up @@ -785,13 +854,9 @@ class DetailsViewModel(

override fun onCleared() {
super.onCleared()
currentDownloadJob?.cancel()

currentAssetName?.let { assetName ->
viewModelScope.launch {
downloader.cancelDownload(assetName)
}
}
currentDownloadJob?.cancel()
currentDownloadJob = null
}

private companion object {
Expand Down
57 changes: 52 additions & 5 deletions ...oseApp/src/jvmMain/kotlin/zed/rainxch/githubstore/core/data/services/DesktopDownloader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.channelFlow
import kotlinx.coroutines.isActive
import kotlinx.coroutines.withContext
import zed.rainxch.githubstore.core.domain.model.DownloadedFile
import zed.rainxch.githubstore.feature.details.domain.model.DownloadProgress
import java.io.File
import java.io.FileOutputStream
Expand All @@ -23,7 +24,7 @@ class DesktopDownloader(

override fun download(url: String, suggestedFileName: String?): Flow<DownloadProgress> = channelFlow {
withContext(Dispatchers.IO) {
val dir = File(files.userDownloadsDir())
val dir = File(files.appDownloadsDir())
if (!dir.exists()) dir.mkdirs()

val safeName = (suggestedFileName?.takeIf { it.isNotBlank() }
Expand Down Expand Up @@ -82,7 +83,7 @@ class DesktopDownloader(
}

override suspend fun saveToFile(url: String, suggestedFileName: String?): String = withContext(Dispatchers.IO) {
val dir = File(files.userDownloadsDir())
val dir = File(files.appDownloadsDir()) // Changed from userDownloadsDir()
val safeName = (suggestedFileName?.takeIf { it.isNotBlank() }
?: url.substringAfterLast('/')
.ifBlank { "asset-${UUID.randomUUID()}" })
Expand All @@ -101,7 +102,7 @@ class DesktopDownloader(
}

override suspend fun getDownloadedFilePath(fileName: String): String? = withContext(Dispatchers.IO) {
val dir = File(files.userDownloadsDir())
val dir = File(files.appDownloadsDir()) // Changed from userDownloadsDir()
val file = File(dir, fileName)

if (file.exists() && file.length() > 0) {
Expand All @@ -112,13 +113,13 @@ class DesktopDownloader(
}

override suspend fun cancelDownload(fileName: String): Boolean = withContext(Dispatchers.IO) {
val dir = File(files.userDownloadsDir())
val dir = File(files.appDownloadsDir()) // Changed from userDownloadsDir()
val file = File(dir, fileName)

if (file.exists()) {
val deleted = file.delete()
if (deleted) {
Logger.d { "Deleted file from Downloads: ${file.absolutePath}" }
Logger.d { "Deleted file from app Downloads: ${file.absolutePath}" }
} else {
Logger.w { "Failed to delete file: ${file.absolutePath}" }
}
Expand All @@ -128,6 +129,52 @@ class DesktopDownloader(
}
}

override suspend fun listDownloadedFiles(): List<DownloadedFile> = withContext(Dispatchers.IO) {
val dir = File(files.appDownloadsDir()) // Already correct
if (!dir.exists()) return@withContext emptyList()

dir.listFiles()
?.filter { it.isFile && it.length() > 0 }
?.map { file ->
DownloadedFile(
fileName = file.name,
filePath = file.absolutePath,
fileSizeBytes = file.length(),
downloadedAt = file.lastModified()
)
}
?.sortedByDescending { it.downloadedAt }
?: emptyList()
}

override suspend fun getLatestDownload(): DownloadedFile? = withContext(Dispatchers.IO) {
listDownloadedFiles().firstOrNull()
}

override suspend fun getFileSize(filePath: String): Long? = withContext(Dispatchers.IO) {
try {
val file = File(filePath)
if (file.exists() && file.isFile) {
file.length()
} else {
null
}
} catch (e: Exception) {
Logger.e { "Failed to get file size for $filePath: ${e.message}" }
null
}
}

override suspend fun getLatestDownloadForAssets(assetNames: List<String>): DownloadedFile? =
withContext(Dispatchers.IO) {
listDownloadedFiles()
.firstOrNull { downloadedFile ->
assetNames.any { assetName ->
downloadedFile.fileName == assetName
}
}
}

companion object {
private const val DEFAULT_BUFFER_SIZE = 8 * 1024
}
Expand Down