Merged
Conversation
This commit adds `android:hasFragileUserData="true"` to the `AndroidManifest.xml`. This ensures that when a user uninstalls the app, the system prompts them to confirm if they want to also remove the app's data. The change also includes `tools:targetApi="29"` to support this attribute.
This commit enables the application to receive and process shared URLs (for `github.com` and `github-store.org`) from other apps through the Android Share Sheet. When a user shares text containing a supported URL with the app, the app will now extract the URL and navigate to the corresponding repository details screen. - **feat(android)**: Added an `intent-filter` for `ACTION_SEND` in `AndroidManifest.xml` to accept `text/plain` and `text/html` content. - **feat(deeplink)**: Implemented `DeepLinkParser.extractSupportedUrl()` to find the first `github.com` or `github-store.org` URL within a given text string. - **refactor(android)**: Updated `MainActivity` to handle `ACTION_SEND` intents, extract the URL from the shared text, and trigger the deep link flow. - **fix(android)**: Corrected the `pathPattern` in the `AndroidManifest.xml` for `github.com` deep links to correctly match repository paths.
This commit enhances the user experience on the details screen by improving the download and version management logic. Downloads are now cached within a screen session. If a user dismisses an install prompt and retries the installation, the app will reuse the already-downloaded file instead of re-downloading it. The download progress indicator now displays the downloaded size (e.g., "14.2 MB / 25.0 MB") for better feedback. Temporary files from canceled or completed downloads are now reliably cleaned up when the user navigates away from the screen. The downgrade detection logic has also been made more robust. It now uses the release list order as the primary method to determine if a selected version is older, falling back to semantic version comparison only when necessary. This provides more accurate downgrade warnings. Finally, ProGuard/R8 is now enabled for release builds to optimize and shrink the application size. - **feat(download)**: Reuse an already-downloaded file if the user cancels the install prompt and then re-initiates it in the same session. - **feat(download)**: Display download progress with file sizes (e.g., "X MB / Y MB") instead of just a percentage. - **fix(download)**: Ensure partially downloaded and cached files are properly cleaned up when the user leaves the details screen.
…ing the release list order, with semantic version comparison as a fallback. - **chore(build)**: Enabled ProGuard (`isMinifyEnabled`) and resource shrinking (`isShrinkResources`) for release builds.
… es, fr, hi, it, ja, kr, pl, ru, tr, zh-rCN) related to installation, versioning, and app status.
Contributor
WalkthroughRelease build now enables code and resource shrinking; Android deep-link handling centralized and expanded (VIEW + SEND); download flow adds byte-level tracking, file reuse, and downgrade logic; many new localization strings added across multiple locales. Changes
Sequence DiagramssequenceDiagram
participant App as MainActivity
participant Handler as handleIncomingIntent()
participant Parser as DeepLinkParser
participant UI as Compose UI
App->>Handler: onCreate / onNewIntent(intent)
Handler->>Handler: detect ACTION_VIEW or ACTION_SEND
alt ACTION_VIEW
Handler->>Handler: uri = intent.data
else ACTION_SEND
Handler->>Parser: extractSupportedUrl(intent.getStringExtra(EXTRA_TEXT))
Parser-->>Handler: supported URL or null
end
Handler-->>UI: set deepLinkUri / navigate
sequenceDiagram
participant User as User
participant UI as SmartInstallButton
participant VM as DetailsViewModel
participant Storage as File storage / Downloader
User->>UI: Tap install
UI->>VM: startDownload(assetName)
VM->>Storage: checkExistingFile(assetName)
alt file exists
Storage-->>VM: file path
VM->>VM: set progress=100%, state=VERIFYING, cache assetName
VM-->>UI: show verifying / ready
else file missing
Storage-->>VM: none
VM->>Storage: start streaming download
Storage-->>VM: emit downloadedBytes, totalBytes events
VM->>VM: update downloadedBytes/totalBytes, cache assetName
VM-->>UI: show byte-progress (formatFileSize)
end
User->>UI: cancel or finish
VM->>Storage: onCancel or onCleared -> clean up cached/current assets as configured
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt (1)
1004-1023:⚠️ Potential issue | 🟠 Major
viewModelScope.launchinonCleared()may not complete — cleanup can silently fail.When
onCleared()returns,viewModelScopeis cancelled, which will cancel any coroutines launched within it. Thedownloader.cancelDownload(asset)suspend call may never execute, leaving stale download files on disk.Consider using a non-cancellable context or a scope that outlives the ViewModel for cleanup:
Proposed fix
override fun onCleared() { super.onCleared() currentDownloadJob?.cancel() val assetsToClean = listOfNotNull(currentAssetName, cachedDownloadAssetName).distinct() if (assetsToClean.isNotEmpty()) { - viewModelScope.launch { + kotlinx.coroutines.MainScope().launch { for (asset in assetsToClean) { try { downloader.cancelDownload(asset) logger.debug("Cleaned up download on screen leave: $asset") } catch (t: Throwable) { logger.error("Failed to clean download on leave: ${t.message}") } } } } }Alternatively, use
kotlinx.coroutines.withContext(NonCancellable)inside the existing scope, or inject an application-scopedCoroutineScopefor cleanup tasks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt` around lines 1004 - 1023, onCleared currently launches cleanup in viewModelScope which may be cancelled before downloader.cancelDownload runs; wrap the cleanup loop in a non-cancellable context so it completes reliably. Replace the viewModelScope.launch { for (asset in assetsToClean) { ... } } block with viewModelScope.launch { withContext(kotlinx.coroutines.NonCancellable) { for (asset in assetsToClean) { try { downloader.cancelDownload(asset); logger.debug("Cleaned up download on screen leave: $asset") } catch (t: Throwable) { logger.error("Failed to clean download on leave: ${t.message}") } } } } (or inject and use an application-scoped CoroutineScope instead of viewModelScope) so onCleared -> downloader.cancelDownload is executed in a NonCancellable context; keep references to currentAssetName, cachedDownloadAssetName and currentDownloadJob as before.
🧹 Nitpick comments (4)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/SmartInstallButton.kt (1)
460-467:String.formatis locale-sensitive — decimal separator may vary.
"%.1f MB".format(...)uses the default locale on Android/JVM, so users in locales with comma decimal separators will see e.g., "1,5 MB". If that's intentional for a localization-focused PR, this is fine. If you want a consistent dot separator regardless of locale, useString.format(java.util.Locale.US, "%.1f MB", ...).Given the PR's localization focus, this may well be the desired behavior — just flagging for awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/SmartInstallButton.kt` around lines 460 - 467, The formatFileSize function uses locale-sensitive String.format via "%.1f ..." which can produce comma decimal separators in some locales; update the three floating-point formats in formatFileSize to use an explicit Locale (e.g., java.util.Locale.US) when formatting so the decimal separator is consistent across locales (use String.format(Locale.US, "...", ... ) or the Kotlin equivalent) while leaving the integer branch unchanged.composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/MainActivity.kt (1)
45-60: Clean centralized intent handling — minor note on staledeepLinkUri.Good refactoring that consolidates intent processing. One subtle edge case:
deepLinkUriis only set, never cleared. If two consecutive intents resolve to the same URI, theLaunchedEffect(deepLinkUri)inMain.ktwon't re-trigger because the key hasn't changed. This means a repeated share of the same repo link won't re-navigate.If this matters, consider appending a unique token or resetting
deepLinkUritonullafter consumption:Optional approach — reset after consumption
In
Main.kt, after navigation:LaunchedEffect(deepLinkUri) { deepLinkUri?.let { uri -> when (val destination = DeepLinkParser.parse(uri)) { is DeepLinkDestination.Repository -> { navBackStack.navigate(...) } DeepLinkDestination.None -> { /* ignore */ } } // Signal consumption back to MainActivity if needed } }Or in
handleIncomingIntent, reset before re-setting to force theLaunchedEffectto fire:uriString?.let { deepLinkUri = null // force state change deepLinkUri = it }Note: This quick-reset approach depends on Compose state coalescing behavior and may not always work. A more robust solution would use a unique event wrapper (e.g., with a UUID) or a Channel-based one-shot event.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/MainActivity.kt` around lines 45 - 60, handleIncomingIntent currently only sets deepLinkUri and never clears it, so Compose's LaunchedEffect(deepLinkUri) in Main.kt won't re-run for repeated identical URIs; fix by turning the deep link into a one-shot event: update handleIncomingIntent to produce a unique event value (e.g., attach a short UUID/token or wrapper) or explicitly reset deepLinkUri to null before setting it, or better, replace deepLinkUri with a one-time event mechanism (Channel/StateFlow/Event wrapper) consumed by LaunchedEffect; touch functions/variables: handleIncomingIntent, deepLinkUri, Main.kt LaunchedEffect(deepLinkUri), DeepLinkParser.parse/DeepLinkDestination.Repository to ensure the event is consumed and navigation triggers every time.feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt (1)
725-725:java.io.Fileandjava.util.concurrent.atomic.AtomicBooleanincommonMainlimits KMP portability.Line 725 uses
java.io.File(existingPath).exists(), and line 45 importsjava.util.concurrent.atomic.AtomicBoolean(used on line 90) — both are JVM-only APIs. While this works today since the project targets only Android/JVM platforms, these JVM-specific APIs incommonMainprevent future expansion to non-JVM targets (iOS, JavaScript, Wasm).For better KMP structure, move this logic to
androidMainorjvmMainsource sets, or replace with KMP-compatible alternatives. Additional JVM-specific java.io usage exists inAppsViewModel.ktandHttpClientFactory.ktin their respectivecommonMaindirectories.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt` at line 725, The commonMain code uses JVM-only APIs: the java.io.File(existingPath).exists() call in DetailsViewModel and the java.util.concurrent.atomic.AtomicBoolean import/usage; move these JVM-specific checks into platform source sets (androidMain/jvmMain) or replace them with platform-agnostic abstractions (expect/actual or a multiplatform FileExists/fileExists(path): Boolean and a multiplatform atomic flag abstraction) so commonMain contains no direct java.io or java.util.concurrent.atomic usage; update DetailsViewModel to call the new common API (e.g., fileExists(existingPath) and a multiplatform atomic wrapper) and similarly refactor the JVM-only usages found in AppsViewModel.kt and HttpClientFactory.kt.core/presentation/src/commonMain/composeResources/values-ja/strings-ja.xml (1)
296-337: LGTM — New Japanese locale strings are correct and well-formed.Format specifiers and XML syntax are all valid. Translations are natural.
One very minor terminology nit: Line 336 uses "アップデート" (
checking_for_updates) while the existingcheck_for_updateson line 6 uses "更新". Both are perfectly understood, but if you prefer consistency within the file, consider aligning on one term.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/presentation/src/commonMain/composeResources/values-ja/strings-ja.xml` around lines 296 - 337, The translations use two different terms for "update" — string name="check_for_updates" uses "更新" while string name="checking_for_updates" uses "アップデート"; make them consistent by changing the value of checking_for_updates to use "更新" (or alternately change check_for_updates to "アップデート") so both string entries use the same term, preserving existing format specifiers and XML syntax.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@build-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.kt`:
- Around line 31-36: The release buildType enables minification but lacks
ProGuard/R8 rule files; update AndroidApplicationConventionPlugin.kt (inside the
buildTypes { getByName("release") { ... } } block) to call
proguardFiles(getDefaultProguardFile("proguard-android-optimize.txt"),
"proguard-rules.pro") so the module's proguard-rules.pro is applied during R8
runs; then verify proguard-rules.pro contains keep rules for Koin DI,
kotlinx.serialization `@Serializable` classes, Compose, and any
reflection-dependent code.
In
`@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/deeplink/DeepLinkParser.kt`:
- Around line 178-181: The regex in DeepLinkParser.extractSupportedUrl is too
permissive and can match spoofed domains and trailing punctuation; update the
regex used in extractSupportedUrl so the domain match requires a trailing slash
(e.g., require "/") after github\.com or github-store\.org and tighten the path
character class to stop at common trailing punctuation/closing characters (e.g.,
exclude whitespace, < > " ' ) ] and final punctuation like period/comma).
Replace the current regex assignment in extractSupportedUrl with the stricter
pattern so it returns only true repo/store URLs and avoids capturing sentence
punctuation.
In `@core/presentation/src/commonMain/composeResources/values-bn/strings-bn.xml`:
- Line 370: The string resource last_checked_hours_ago uses inconsistent Bengali
spelling "ঘন্টা"; update its value to use the standard/consistent spelling
"ঘণ্টা" (i.e., change "%1$d ঘন্টা আগে" to "%1$d ঘণ্টা আগে") so it matches other
keys like updated_hours_ago, added_hours_ago, hours_ago, time_hours_ago, and
released_hours_ago.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 721-757: Existing-file branch reuses
downloader.getDownloadedFilePath(assetName) and sets _state to
DownloadStage.VERIFYING without any integrity check; add a validation step
(e.g., compare java.io.File(existingPath).length() to sizeBytes or compute the
expected hash) before treating the file as valid, and if validation fails fall
back to the download path (update _state to DOWNLOADING and call
downloader.download(...).collect as in the else branch); ensure
cachedDownloadAssetName is only set after successful validation and that _state
transitions reflect actual verification results instead of being purely
cosmetic.
---
Outside diff comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 1004-1023: onCleared currently launches cleanup in viewModelScope
which may be cancelled before downloader.cancelDownload runs; wrap the cleanup
loop in a non-cancellable context so it completes reliably. Replace the
viewModelScope.launch { for (asset in assetsToClean) { ... } } block with
viewModelScope.launch { withContext(kotlinx.coroutines.NonCancellable) { for
(asset in assetsToClean) { try { downloader.cancelDownload(asset);
logger.debug("Cleaned up download on screen leave: $asset") } catch (t:
Throwable) { logger.error("Failed to clean download on leave: ${t.message}") } }
} } (or inject and use an application-scoped CoroutineScope instead of
viewModelScope) so onCleared -> downloader.cancelDownload is executed in a
NonCancellable context; keep references to currentAssetName,
cachedDownloadAssetName and currentDownloadJob as before.
---
Nitpick comments:
In `@composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/MainActivity.kt`:
- Around line 45-60: handleIncomingIntent currently only sets deepLinkUri and
never clears it, so Compose's LaunchedEffect(deepLinkUri) in Main.kt won't
re-run for repeated identical URIs; fix by turning the deep link into a one-shot
event: update handleIncomingIntent to produce a unique event value (e.g., attach
a short UUID/token or wrapper) or explicitly reset deepLinkUri to null before
setting it, or better, replace deepLinkUri with a one-time event mechanism
(Channel/StateFlow/Event wrapper) consumed by LaunchedEffect; touch
functions/variables: handleIncomingIntent, deepLinkUri, Main.kt
LaunchedEffect(deepLinkUri), DeepLinkParser.parse/DeepLinkDestination.Repository
to ensure the event is consumed and navigation triggers every time.
In `@core/presentation/src/commonMain/composeResources/values-ja/strings-ja.xml`:
- Around line 296-337: The translations use two different terms for "update" —
string name="check_for_updates" uses "更新" while string
name="checking_for_updates" uses "アップデート"; make them consistent by changing the
value of checking_for_updates to use "更新" (or alternately change
check_for_updates to "アップデート") so both string entries use the same term,
preserving existing format specifiers and XML syntax.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/SmartInstallButton.kt`:
- Around line 460-467: The formatFileSize function uses locale-sensitive
String.format via "%.1f ..." which can produce comma decimal separators in some
locales; update the three floating-point formats in formatFileSize to use an
explicit Locale (e.g., java.util.Locale.US) when formatting so the decimal
separator is consistent across locales (use String.format(Locale.US, "...", ...
) or the Kotlin equivalent) while leaving the integer branch unchanged.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Line 725: The commonMain code uses JVM-only APIs: the
java.io.File(existingPath).exists() call in DetailsViewModel and the
java.util.concurrent.atomic.AtomicBoolean import/usage; move these JVM-specific
checks into platform source sets (androidMain/jvmMain) or replace them with
platform-agnostic abstractions (expect/actual or a multiplatform
FileExists/fileExists(path): Boolean and a multiplatform atomic flag
abstraction) so commonMain contains no direct java.io or
java.util.concurrent.atomic usage; update DetailsViewModel to call the new
common API (e.g., fileExists(existingPath) and a multiplatform atomic wrapper)
and similarly refactor the JVM-only usages found in AppsViewModel.kt and
HttpClientFactory.kt.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
build-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.ktcomposeApp/release/baselineProfiles/0/composeApp-release.dmcomposeApp/release/baselineProfiles/1/composeApp-release.dmcomposeApp/src/androidMain/AndroidManifest.xmlcomposeApp/src/androidMain/kotlin/zed/rainxch/githubstore/MainActivity.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/Main.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/deeplink/DeepLinkParser.ktcore/presentation/src/commonMain/composeResources/values-bn/strings-bn.xmlcore/presentation/src/commonMain/composeResources/values-es/strings-es.xmlcore/presentation/src/commonMain/composeResources/values-fr/strings-fr.xmlcore/presentation/src/commonMain/composeResources/values-hi/strings-hi.xmlcore/presentation/src/commonMain/composeResources/values-it/strings-it.xmlcore/presentation/src/commonMain/composeResources/values-ja/strings-ja.xmlcore/presentation/src/commonMain/composeResources/values-kr/strings-kr.xmlcore/presentation/src/commonMain/composeResources/values-pl/strings-pl.xmlcore/presentation/src/commonMain/composeResources/values-ru/strings-ru.xmlcore/presentation/src/commonMain/composeResources/values-tr/strings-tr.xmlcore/presentation/src/commonMain/composeResources/values-zh-rCN/strings-zh-rCN.xmlfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsState.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/SmartInstallButton.kt
build-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.kt
Show resolved
Hide resolved
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/deeplink/DeepLinkParser.kt
Show resolved
Hide resolved
core/presentation/src/commonMain/composeResources/values-bn/strings-bn.xml
Outdated
Show resolved
Hide resolved
Comment on lines
721
to
757
| // Check if file was already downloaded (e.g. user dismissed install dialog) | ||
| val existingPath = downloader.getDownloadedFilePath(assetName) | ||
| val filePath: String | ||
|
|
||
| if (existingPath != null && java.io.File(existingPath).exists()) { | ||
| logger.debug("Reusing already downloaded file: $assetName") | ||
| filePath = existingPath | ||
| _state.value = _state.value.copy( | ||
| downloadProgressPercent = 100, | ||
| downloadedBytes = sizeBytes, | ||
| totalBytes = sizeBytes, | ||
| downloadStage = DownloadStage.VERIFYING | ||
| ) | ||
| } else { | ||
| _state.value = _state.value.copy( | ||
| downloadStage = DownloadStage.DOWNLOADING, | ||
| downloadedBytes = 0L, | ||
| totalBytes = sizeBytes | ||
| ) | ||
| downloader.download(downloadUrl, assetName).collect { p -> | ||
| _state.value = _state.value.copy( | ||
| downloadProgressPercent = p.percent, | ||
| downloadedBytes = p.bytesDownloaded, | ||
| totalBytes = p.totalBytes ?: sizeBytes | ||
| ) | ||
| if (p.percent == 100) { | ||
| _state.value = | ||
| _state.value.copy(downloadStage = DownloadStage.VERIFYING) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| val filePath = downloader.getDownloadedFilePath(assetName) | ||
| ?: throw IllegalStateException("Downloaded file not found") | ||
| filePath = downloader.getDownloadedFilePath(assetName) | ||
| ?: throw IllegalStateException("Downloaded file not found") | ||
|
|
||
| // Cache asset name so it persists for reuse until screen is left | ||
| cachedDownloadAssetName = assetName | ||
| } |
Contributor
There was a problem hiding this comment.
File reuse skips actual verification — VERIFYING stage is cosmetic.
When an existing file is found (line 725), the state is set to DownloadStage.VERIFYING but no integrity check (e.g., hash or size comparison) is performed before proceeding to install. If the file was corrupted (e.g., due to an earlier crash, disk issue, or partial write), the user would get a confusing install failure.
Consider at minimum validating the file size matches sizeBytes before reusing:
Proposed fix
-if (existingPath != null && java.io.File(existingPath).exists()) {
+val existingFile = existingPath?.let { java.io.File(it) }
+if (existingFile != null && existingFile.exists() && existingFile.length() == sizeBytes) {
logger.debug("Reusing already downloaded file: $assetName")
filePath = existingPathThis way, a size mismatch triggers a fresh download, avoiding corrupt installs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`
around lines 721 - 757, Existing-file branch reuses
downloader.getDownloadedFilePath(assetName) and sets _state to
DownloadStage.VERIFYING without any integrity check; add a validation step
(e.g., compare java.io.File(existingPath).length() to sizeBytes or compute the
expected hash) before treating the file as valid, and if validation fails fall
back to the download path (update _state to DOWNLOADING and call
downloader.download(...).collect as in the else branch); ensure
cachedDownloadAssetName is only set after successful validation and that _state
transitions reflect actual verification results instead of being purely
cosmetic.
This commit refactors the `DetailsViewModel` to enhance the download caching mechanism and remove obsolete code. The asset download logic is now more robust. It verifies not only the existence of a cached file but also confirms that its size matches the expected size from the asset metadata, preventing the use of incomplete or stale downloads. The ViewModel's code has also been streamlined by removing redundant comments and unused logic, particularly around download caching and downgrade warnings, improving overall readability and maintenance. - **feat(details)**: Added a file size check to the download cache logic, ensuring that a cached asset is only reused if its size matches the expected size. - **refactor(details)**: Removed obsolete comments and simplified code within the `DetailsViewModel`, including cleanup of the `onCleared` method. - **fix(deeplink)**: Improved the regex in `DeepLinkParser` to more accurately extract GitHub URLs from surrounding text by better handling trailing characters. - **chore(build)**: Overhauled and organized the ProGuard rules for release builds, ensuring proper shrinking and optimization for libraries like Ktor, Koin, Room, and Compose. - **fix(i18n)**: Corrected a minor typo in the Bengali (`bn`) translation for "hours ago".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
core/presentation/src/commonMain/composeResources/values-bn/strings-bn.xml (1)
349-371: Remaining new strings LGTM.All other additions are linguistically correct and follow the established conventions in this file:
- The previously flagged
last_checked_hours_agospelling is now%1$d ঘণ্টা আগে(retroflex ণ) — consistent with all other hour strings.- Format specifiers in
downgrade_warning_message(%1$s/%2$s) match the expected argument order.checking_for_updatesis correctly distinct from the action stringcheck_for_updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/presentation/src/commonMain/composeResources/values-bn/strings-bn.xml` around lines 349 - 371, No changes needed—all new Bengali strings are correct; approve and merge as-is. Confirm the entries pending_install, uninstall, open_app, downgrade_requires_uninstall, downgrade_warning_message, uninstall_first, install_version, failed_to_open_app, failed_to_uninstall, latest_badge, last_checked, last_checked_never, last_checked_just_now, last_checked_minutes_ago, last_checked_hours_ago, and checking_for_updates can be committed without edits.
🧹 Nitpick comments (7)
core/presentation/src/commonMain/composeResources/values-bn/strings-bn.xml (1)
354-354:open_appandfailed_to_open_appduplicate existing string values.
open_app(line 354) →"খুলুন"is identical toopen(line 26).failed_to_open_app(line 359) →"%1$s খুলতে ব্যর্থ"is identical tofailed_to_open(line 13).Having separate keys is fine for semantic clarity and future divergence, but Android Lint will warn about duplicate string values. If the values are intentionally the same and the keys will never diverge, consider aliasing via
tools:ignore="DuplicateStrings", or simply note this as expected duplication.Also applies to: 359-359
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/presentation/src/commonMain/composeResources/values-bn/strings-bn.xml` at line 354, The bn strings open_app and failed_to_open_app duplicate existing values (open and failed_to_open); either remove the duplicates and reference the original keys where used, or mark them as intentional duplicates to silence Lint by adding tools:ignore="DuplicateStrings" on the <string> elements for open_app and failed_to_open_app (and ensure the resources root has the xmlns:tools namespace). Update the <string name="open_app"> and <string name="failed_to_open_app"> entries accordingly.composeApp/proguard-rules.pro (3)
158-161:**.BuildKonfigon Line 161 is a superset of Line 160 — likely unnecessary.Line 160 already covers all
BuildKonfigclasses underzed.rainxch.**. Line 161 broadens to any package, which would also prevent R8 from strippingBuildKonfigclasses from third-party libraries (if any exist). If only the app's own config is needed, Line 160 is sufficient.♻️ Suggested cleanup
-keep class zed.rainxch.githubstore.BuildConfig { *; } -keep class zed.rainxch.**.BuildKonfig { *; } --keep class **.BuildKonfig { *; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/proguard-rules.pro` around lines 158 - 161, The proguard rule "keep class **.BuildKonfig { *; }" is overly broad and redundant given the existing "keep class zed.rainxch.**.BuildKonfig { *; }"; remove the global rule (or replace it with a more specific package pattern) so only your app's BuildKonfig (zed.rainxch.**.BuildKonfig) is preserved and third‑party BuildKonfig classes can still be optimized/obfuscated.
27-46: Consider scoping downkotlinx.serialization.**keep rule.Line 29 keeps the entire
kotlinx.serializationlibrary (all formats, all internals). If the app only uses JSON, R8 could otherwise strip unused formats (CBOR, Protobuf, Properties, etc.). The annotation-based rules on Lines 33-46 already cover the serializer mechanics well.A tighter alternative:
♻️ Suggested refinement
-# Serialization engine internals --keep class kotlinx.serialization.** { *; } +-keep class kotlinx.serialization.internal.** { *; } +-keep class kotlinx.serialization.json.** { *; }This preserves the core internals and JSON module while letting R8 strip unused serialization formats. Only relevant if the app doesn't use other formats.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/proguard-rules.pro` around lines 27 - 46, Replace the overly-broad "-keep class kotlinx.serialization.** { *; }" rule with narrower keeps that preserve the core runtime and the JSON module only (e.g., keep class kotlinx.serialization.json.** { *; } and keep class kotlinx.serialization.encoding.** { *; } / kotlinx.serialization.descriptors.** { *; } or other core packages your app uses) and remove the blanket kotlinx.serialization.** entry so R8 can strip unused formats (CBOR/Protobuf/etc.); keep the existing annotation-based rules for generated serializers (the rules referencing **$$serializer and `@kotlinx.serialization.Serializable`) intact.
85-87: Same pattern:androidx.room.**blanket keep is likely unnecessary.Room ships consumer ProGuard rules. The app-specific keeps on Lines 75-83 (database, entities, DAOs) should suffice alongside Room's bundled rules.
♻️ Suggested refinement
# Room runtime --keep class androidx.room.** { *; } -dontwarn androidx.room.**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/proguard-rules.pro` around lines 85 - 87, The proguard rules currently include a blanket keep and dontwarn for androidx.room via the lines '-keep class androidx.room.** { *; }' and 'dontwarn androidx.room.**' which is unnecessary because Room ships its own consumer ProGuard rules and you already have app-specific keeps for your database, entities and DAOs; remove those two broad rules from proguard-rules.pro so only the targeted keeps (your database/entities/DAOs) remain and rely on Room's bundled rules to avoid over-keeping.feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt (3)
1018-1020:normalizeVersionis duplicated in three places.Identical implementations exist here (line 1018), in
SmartInstallButton.kt:455, andInstalledAppsRepositoryImpl.kt:227. Consider extracting it to a shared utility in the core module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt` around lines 1018 - 1020, The normalizeVersion function is duplicated across DetailsViewModel, SmartInstallButton, and InstalledAppsRepositoryImpl; extract it into a single shared utility (e.g., a new function normalizeVersion in a core/common utility object or file) and replace the three local implementations with calls to that new utility; update references in DetailsViewModel.normalizeVersion, SmartInstallButton, and InstalledAppsRepositoryImpl to use the shared function and remove the duplicated definitions to avoid redundancy.
369-369: Only the last cancelled download's file is tracked for cleanup.
cachedDownloadAssetNameis a singleString?, so if the user cancels download A, then cancels download B, the reference to A's partial file is lost when B overwrites it. Similarly,installAsset(line 694) cancels the previous job without caching its asset name first.If temp file cleanup matters, consider using a
MutableSet<String>instead of a single nullable string.Sketch
- private var cachedDownloadAssetName: String? = null + private val cachedDownloadAssetNames = mutableSetOf<String>()Then add to the set on each cancel, and iterate in
onCleared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt` at line 369, The code tracks only one pending cancelled download via cachedDownloadAssetName, so cancelled downloads overwrite each other and partial temp files are lost; change cachedDownloadAssetName from a single String? to a MutableSet<String> (e.g., cachedCancelledDownloadAssetNames), add the asset name to that set whenever a download is cancelled (including before installAsset cancels a previous job), and update onCleared to iterate the set and delete each corresponding temp/partial file and then clear the set; update all references to cachedDownloadAssetName accordingly (including where installAsset cancels previous jobs and where cleanup is performed) so multiple cancelled assets are tracked and cleaned up.
1003-1014: Cleanup pattern is safe with Lifecycle 2.9.1, but considerNonCancellablefor defensive robustness.The project uses Lifecycle 2.9.1 (≥ 2.8+), where
onCleared()is called before the viewModelScope is cancelled, makingviewModelScope.launchinsideonCleared()safe. However, for defensive cleanup code that must complete regardless of scope state, consider wrapping inNonCancellablecontext:viewModelScope.launch(NonCancellable) { for (asset in assetsToClean) { try { downloader.cancelDownload(asset) logger.debug("Cleaned up download on screen leave: $asset") } catch (t: Throwable) { logger.error("Failed to clean download on leave: ${t.message}") } } }This ensures cleanup completes even if the scope transitions to cancellation state during execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt` around lines 1003 - 1014, The cleanup coroutine launched in onCleared uses viewModelScope.launch which can be cancelled; wrap the cleanup in a NonCancellable context to ensure it runs to completion: change the launch to viewModelScope.launch(NonCancellable) and keep the existing loop over assetsToClean calling downloader.cancelDownload(asset) with the same try/catch and logger.debug/logger.error so cleanup completes even if the scope is cancelled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composeApp/proguard-rules.pro`:
- Around line 124-130: The proguard rules currently include overly broad keeps
(-keep class androidx.compose.** { *; }, -keep class androidx.navigation.** { *;
}, -keep class androidx.lifecycle.** { *; }) which prevent R8 from shrinking
those libraries and will bloat the APK; remove these blanket -keep entries and
rely on the libraries' consumer ProGuard/R8 rules instead, and if you encounter
a runtime crash after minification, add a targeted keep for the specific class
or symbol (e.g., keep only androidx.lifecycle.ViewModelProvider$* or the exact
class reported) rather than restoring the broad package-wide keeps.
- Around line 185-195: The comment above the ProGuard rules incorrectly
references ServiceLoader; update it to say these rules are for
java.io.Serializable compatibility (preserving serialVersionUID,
serialPersistentFields, non-static/non-transient fields, and serialization hooks
like writeObject/readObject/writeReplace/readResolve) so future maintainers
understand the rules apply to Serializable, not ServiceLoader; locate the
comment that precedes the rules referencing java.io.Serializable and replace the
ServiceLoader wording with a concise note that these rules preserve Serializable
compatibility and related members (mentioning serialVersionUID,
serialPersistentFields, writeObject, readObject, writeReplace, readResolve).
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 1054-1064: compareSemanticVersions currently strips alphabetic
pre-release identifiers (using filter { it.isDigit() }) so "1.0.0-alpha"
compares equal to "1.0.0"; update compareSemanticVersions to first compare
numeric components without removing non-digit suffixes (parse leading numeric
portion of each part into Long), and after numeric parts are equal check for
pre-release presence by inspecting the original part strings (if one part
contains a non-empty non-numeric suffix or the version has extra parts with
non-numeric chars treat it as pre-release) and return that a version with a
pre-release suffix is less than the same numeric release; keep existing behavior
of comparing numeric segments first and only apply the pre-release rule when
numerics match.
- Around line 371-376: The appendLog call in DetailsViewModel (the cancellation
path) is passing _state.value.downloadedBytes (partial bytes) into
InstallLogItem.assetSizeBytes, which records a misleading size; change that
argument to the total asset size (use the existing total size variable used
elsewhere such as sizeBytes or primary.size) so appendLog(assetName, size =
<totalAssetSize>, tag = releaseTag, result = LogResult.Cancelled) records the
full asset size; locate the appendLog invocation in DetailsViewModel.kt and
replace downloadedBytes with the appropriate total size variable to ensure
InstallLogItem.assetSizeBytes is correct.
---
Duplicate comments:
In `@core/presentation/src/commonMain/composeResources/values-bn/strings-bn.xml`:
- Around line 349-371: No changes needed—all new Bengali strings are correct;
approve and merge as-is. Confirm the entries pending_install, uninstall,
open_app, downgrade_requires_uninstall, downgrade_warning_message,
uninstall_first, install_version, failed_to_open_app, failed_to_uninstall,
latest_badge, last_checked, last_checked_never, last_checked_just_now,
last_checked_minutes_ago, last_checked_hours_ago, and checking_for_updates can
be committed without edits.
---
Nitpick comments:
In `@composeApp/proguard-rules.pro`:
- Around line 158-161: The proguard rule "keep class **.BuildKonfig { *; }" is
overly broad and redundant given the existing "keep class
zed.rainxch.**.BuildKonfig { *; }"; remove the global rule (or replace it with a
more specific package pattern) so only your app's BuildKonfig
(zed.rainxch.**.BuildKonfig) is preserved and third‑party BuildKonfig classes
can still be optimized/obfuscated.
- Around line 27-46: Replace the overly-broad "-keep class
kotlinx.serialization.** { *; }" rule with narrower keeps that preserve the core
runtime and the JSON module only (e.g., keep class kotlinx.serialization.json.**
{ *; } and keep class kotlinx.serialization.encoding.** { *; } /
kotlinx.serialization.descriptors.** { *; } or other core packages your app
uses) and remove the blanket kotlinx.serialization.** entry so R8 can strip
unused formats (CBOR/Protobuf/etc.); keep the existing annotation-based rules
for generated serializers (the rules referencing **$$serializer and
`@kotlinx.serialization.Serializable`) intact.
- Around line 85-87: The proguard rules currently include a blanket keep and
dontwarn for androidx.room via the lines '-keep class androidx.room.** { *; }'
and 'dontwarn androidx.room.**' which is unnecessary because Room ships its own
consumer ProGuard rules and you already have app-specific keeps for your
database, entities and DAOs; remove those two broad rules from
proguard-rules.pro so only the targeted keeps (your database/entities/DAOs)
remain and rely on Room's bundled rules to avoid over-keeping.
In `@core/presentation/src/commonMain/composeResources/values-bn/strings-bn.xml`:
- Line 354: The bn strings open_app and failed_to_open_app duplicate existing
values (open and failed_to_open); either remove the duplicates and reference the
original keys where used, or mark them as intentional duplicates to silence Lint
by adding tools:ignore="DuplicateStrings" on the <string> elements for open_app
and failed_to_open_app (and ensure the resources root has the xmlns:tools
namespace). Update the <string name="open_app"> and <string
name="failed_to_open_app"> entries accordingly.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 1018-1020: The normalizeVersion function is duplicated across
DetailsViewModel, SmartInstallButton, and InstalledAppsRepositoryImpl; extract
it into a single shared utility (e.g., a new function normalizeVersion in a
core/common utility object or file) and replace the three local implementations
with calls to that new utility; update references in
DetailsViewModel.normalizeVersion, SmartInstallButton, and
InstalledAppsRepositoryImpl to use the shared function and remove the duplicated
definitions to avoid redundancy.
- Line 369: The code tracks only one pending cancelled download via
cachedDownloadAssetName, so cancelled downloads overwrite each other and partial
temp files are lost; change cachedDownloadAssetName from a single String? to a
MutableSet<String> (e.g., cachedCancelledDownloadAssetNames), add the asset name
to that set whenever a download is cancelled (including before installAsset
cancels a previous job), and update onCleared to iterate the set and delete each
corresponding temp/partial file and then clear the set; update all references to
cachedDownloadAssetName accordingly (including where installAsset cancels
previous jobs and where cleanup is performed) so multiple cancelled assets are
tracked and cleaned up.
- Around line 1003-1014: The cleanup coroutine launched in onCleared uses
viewModelScope.launch which can be cancelled; wrap the cleanup in a
NonCancellable context to ensure it runs to completion: change the launch to
viewModelScope.launch(NonCancellable) and keep the existing loop over
assetsToClean calling downloader.cancelDownload(asset) with the same try/catch
and logger.debug/logger.error so cleanup completes even if the scope is
cancelled.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
build-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.ktcomposeApp/proguard-rules.procomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/deeplink/DeepLinkParser.ktcore/presentation/src/commonMain/composeResources/values-bn/strings-bn.xmlfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- build-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.kt
- composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/deeplink/DeepLinkParser.kt
Comment on lines
124
to
130
| # ── Compose / AndroidX ──────────────────────────────────────────────────── | ||
| # Compose runtime and navigation (most rules come bundled with the library) | ||
| -keep class androidx.compose.** { *; } | ||
| -dontwarn androidx.compose.** | ||
| -keep class androidx.navigation.** { *; } | ||
| -keep class androidx.lifecycle.** { *; } | ||
| -dontwarn androidx.lifecycle.** |
Contributor
There was a problem hiding this comment.
Over-broad keeps for Compose/AndroidX will significantly bloat the APK.
-keep class androidx.compose.** { *; } prevents R8 from shrinking any Compose class — runtime, foundation, material, UI, animation, etc. The same applies to androidx.navigation.** and androidx.lifecycle.**. These are some of the largest dependencies in a Compose app, and they already ship consumer ProGuard rules in their AARs that handle the necessary reflection/keep cases.
Since the PR is enabling R8 minification, these blanket keeps directly undermine the size savings. In projects I've seen, removing the Compose blanket keep alone can save 2–5 MB from the release APK.
Recommendation: Remove these three blanket keeps and rely on the libraries' bundled consumer rules. If you hit a specific runtime issue (e.g., a crash from a stripped class), add a targeted keep for just that class.
♻️ Proposed change
# ── Compose / AndroidX ────────────────────────────────────────────────────
# Compose runtime and navigation (most rules come bundled with the library)
--keep class androidx.compose.** { *; }
-dontwarn androidx.compose.**
--keep class androidx.navigation.** { *; }
--keep class androidx.lifecycle.** { *; }
-dontwarn androidx.lifecycle.**If specific classes cause runtime issues after removal, add targeted rules like:
# Example: keep only if a specific reflection-based crash occurs
-keep class androidx.lifecycle.ViewModelProvider$* { *; }
📝 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
| # ── Compose / AndroidX ──────────────────────────────────────────────────── | |
| # Compose runtime and navigation (most rules come bundled with the library) | |
| -keep class androidx.compose.** { *; } | |
| -dontwarn androidx.compose.** | |
| -keep class androidx.navigation.** { *; } | |
| -keep class androidx.lifecycle.** { *; } | |
| -dontwarn androidx.lifecycle.** | |
| # ── Compose / AndroidX ──────────────────────────────────────────────────── | |
| # Compose runtime and navigation (most rules come bundled with the library) | |
| -dontwarn androidx.compose.** | |
| -dontwarn androidx.lifecycle.** |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composeApp/proguard-rules.pro` around lines 124 - 130, The proguard rules
currently include overly broad keeps (-keep class androidx.compose.** { *; },
-keep class androidx.navigation.** { *; }, -keep class androidx.lifecycle.** {
*; }) which prevent R8 from shrinking those libraries and will bloat the APK;
remove these blanket -keep entries and rely on the libraries' consumer
ProGuard/R8 rules instead, and if you encounter a runtime crash after
minification, add a targeted keep for the specific class or symbol (e.g., keep
only androidx.lifecycle.ViewModelProvider$* or the exact class reported) rather
than restoring the broad package-wide keeps.
composeApp/proguard-rules.pro
Outdated
Comment on lines
185
to
195
| # ── ServiceLoader (used by Ktor, Koin, etc.) ────────────────────────────── | ||
| -keepnames class * implements java.io.Serializable | ||
| -keepclassmembers class * implements java.io.Serializable { | ||
| static final long serialVersionUID; | ||
| private static final java.io.ObjectStreamField[] serialPersistentFields; | ||
| !static !transient <fields>; | ||
| private void writeObject(java.io.ObjectOutputStream); | ||
| private void readObject(java.io.ObjectInputStream); | ||
| java.lang.Object writeReplace(); | ||
| java.lang.Object readResolve(); | ||
| } |
Contributor
There was a problem hiding this comment.
Misleading section comment: these are java.io.Serializable rules, not ServiceLoader rules.
ServiceLoader is a separate mechanism (META-INF/services + java.util.ServiceLoader). The rules here preserve serialVersionUID, writeObject/readObject, and related hooks — all specific to java.io.Serializable compatibility. Consider updating the comment to avoid confusion during future maintenance.
✏️ Suggested comment fix
-# ── ServiceLoader (used by Ktor, Koin, etc.) ──────────────────────────────
+# ── Java Serializable Compatibility ───────────────────────────────────────🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composeApp/proguard-rules.pro` around lines 185 - 195, The comment above the
ProGuard rules incorrectly references ServiceLoader; update it to say these
rules are for java.io.Serializable compatibility (preserving serialVersionUID,
serialPersistentFields, non-static/non-transient fields, and serialization hooks
like writeObject/readObject/writeReplace/readResolve) so future maintainers
understand the rules apply to Serializable, not ServiceLoader; locate the
comment that precedes the rules referencing java.io.Serializable and replace the
ServiceLoader wording with a concise note that these rules preserve Serializable
compatibility and related members (mentioning serialVersionUID,
serialPersistentFields, writeObject, readObject, writeReplace, readResolve).
Comment on lines
371
to
376
| appendLog( | ||
| assetName = assetName, | ||
| size = _state.value.downloadedBytes, | ||
| tag = releaseTag, | ||
| result = LogResult.Cancelled | ||
| ) |
Contributor
There was a problem hiding this comment.
appendLog logs partial downloadedBytes as assetSizeBytes.
All other appendLog call-sites pass the total asset size (e.g., sizeBytes, primary.size), but here _state.value.downloadedBytes is passed — which is only the bytes received so far. This will record a misleading value in InstallLogItem.assetSizeBytes.
Proposed fix
+ val releaseTag = _state.value.selectedRelease?.tagName ?: ""
+ val totalSize = _state.value.totalBytes ?: _state.value.downloadedBytes
appendLog(
assetName = assetName,
- size = _state.value.downloadedBytes,
+ size = totalSize,
tag = releaseTag,
result = LogResult.Cancelled
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`
around lines 371 - 376, The appendLog call in DetailsViewModel (the cancellation
path) is passing _state.value.downloadedBytes (partial bytes) into
InstallLogItem.assetSizeBytes, which records a misleading size; change that
argument to the total asset size (use the existing total size variable used
elsewhere such as sizeBytes or primary.size) so appendLog(assetName, size =
<totalAssetSize>, tag = releaseTag, result = LogResult.Cancelled) records the
full asset size; locate the appendLog invocation in DetailsViewModel.kt and
replace downloadedBytes with the appropriate total size variable to ensure
InstallLogItem.assetSizeBytes is correct.
Comment on lines
1054
to
1064
| private fun compareSemanticVersions(a: String, b: String): Int { | ||
| val aParts = a.split("[.\\-]".toRegex()) | ||
| val bParts = b.split("[.\\-]".toRegex()) | ||
| val maxLen = maxOf(aParts.size, bParts.size) | ||
| for (i in 0 until maxLen) { | ||
| val aPart = aParts.getOrNull(i)?.filter { it.isDigit() }?.toLongOrNull() ?: 0L | ||
| val bPart = bParts.getOrNull(i)?.filter { it.isDigit() }?.toLongOrNull() ?: 0L | ||
| if (aPart != bPart) return aPart.compareTo(bPart) | ||
| } | ||
| return 0 | ||
| } |
Contributor
There was a problem hiding this comment.
compareSemanticVersions cannot distinguish pre-release from release.
filter { it.isDigit() } strips alphabetic pre-release identifiers entirely, so 1.0.0-alpha and 1.0.0 are considered equal. If both versions happen to be missing from allReleases (the fallback path), a pre-release → release install won't be flagged as a downgrade and vice-versa. This is mitigated by the list-index primary check, but worth noting for correctness.
A minimal improvement: after the numeric loop, if all numeric parts are equal, treat the version with a pre-release suffix as less than the one without.
Sketch
private fun compareSemanticVersions(a: String, b: String): Int {
- val aParts = a.split("[.\\-]".toRegex())
- val bParts = b.split("[.\\-]".toRegex())
+ val aCore = a.split("-", limit = 2)
+ val bCore = b.split("-", limit = 2)
+ val aParts = aCore[0].split(".")
+ val bParts = bCore[0].split(".")
val maxLen = maxOf(aParts.size, bParts.size)
for (i in 0 until maxLen) {
val aPart = aParts.getOrNull(i)?.toLongOrNull() ?: 0L
val bPart = bParts.getOrNull(i)?.toLongOrNull() ?: 0L
if (aPart != bPart) return aPart.compareTo(bPart)
}
+ // Equal numeric parts: a pre-release tag is "less than" no tag
+ val aHasPre = aCore.size > 1
+ val bHasPre = bCore.size > 1
+ if (aHasPre != bHasPre) return if (aHasPre) -1 else 1
return 0
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`
around lines 1054 - 1064, compareSemanticVersions currently strips alphabetic
pre-release identifiers (using filter { it.isDigit() }) so "1.0.0-alpha"
compares equal to "1.0.0"; update compareSemanticVersions to first compare
numeric components without removing non-digit suffixes (parse leading numeric
portion of each part into Long), and after numeric parts are equal check for
pre-release presence by inspecting the original part strings (if one part
contains a non-empty non-numeric suffix or the version has extra parts with
non-numeric chars treat it as pre-release) and return that a version with a
pre-release suffix is less than the same numeric release; keep existing behavior
of comparing numeric segments first and only apply the pre-release rule when
numerics match.
This commit introduces a more robust semantic version comparison algorithm and corrects the download size logging for cancelled downloads. The version comparison logic now properly handles pre-release identifiers (e.g., `-alpha`, `-beta`), ensuring that stable releases are correctly prioritized over pre-releases. Additionally, when a download is cancelled, the log now correctly records the total expected size of the asset rather than the partially downloaded bytes. - **refactor(versioning)**: Updated `compareSemanticVersions` to correctly handle pre-release suffixes. - **fix(logging)**: Corrected `appendLog` to use the asset's total size instead of downloaded bytes when a download is cancelled. - **chore(proguard)**: Removed redundant `-keep` rules for AndroidX Compose and Navigation, as these are now bundled with the respective libraries.
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
Improvements