feat: track installation outcome to improve pending install state#348
Merged
rainxchzed merged 4 commits intomainfrom Mar 21, 2026
Merged
feat: track installation outcome to improve pending install state#348rainxchzed merged 4 commits intomainfrom
rainxchzed merged 4 commits intomainfrom
Conversation
- Introduce `InstallOutcome` enum to distinguish between synchronous (`COMPLETED`) and asynchronous (`DELEGATED_TO_SYSTEM`) installation processes. - Update `Installer.install` signature to return `InstallOutcome`. - Implement `InstallOutcome.COMPLETED` in `ShizukuInstallerWrapper` for successful silent installs. - Implement `InstallOutcome.DELEGATED_TO_SYSTEM` for standard Android and Desktop installers where the OS handles the UI. - Update `DetailsViewModel` to use the installation outcome to more accurately set the `isPendingInstall` flag. - Refactor internal install logic in `DetailsViewModel` to propagate the outcome to repository sync operations.
Contributor
WalkthroughInstaller API now returns an InstallOutcome; platform installers (Android, Shizuku, Desktop) and callers were updated to return and propagate that outcome. New InstallationManager and AttestationVerifier types centralize APK validation, fingerprint checks, persistence, and attestation verification; DetailsViewModel was refactored to use them. Changes
sequenceDiagram
participant VM as DetailsViewModel
participant IM as InstallationManager
participant INS as Installer (Android/Shizuku/Desktop)
participant Repo as InstalledAppsRepository
participant AV as AttestationVerifier
VM->>IM: validateApk(filePath,isUpdate,trackedPackage)
IM-->>VM: ApkValidationResult (Valid / ExtractionFailed / PackageMismatch)
alt Valid
VM->>IM: checkSigningFingerprint(apkInfo)
IM-->>VM: FingerprintCheckResult (Ok / Mismatch)
VM->>AV: verify(owner, repoName, filePath)
AV-->>VM: Boolean (attestation ok?)
VM->>IM: saveNewInstalledApp(params...)
IM->>INS: installer.install(filePath, extOrMime)
INS-->>IM: InstallOutcome (COMPLETED / DELEGATED_TO_SYSTEM)
IM->>Repo: save/update installed app (isPending = outcome != COMPLETED)
IM-->>VM: InstalledApp? / completion
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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📝 Generate docstrings
🧪 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: 2
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)
1392-1400:⚠️ Potential issue | 🟠 MajorMake
installOutcomemandatory in this helper.The default value is already masking a missed call-site update in the signing-key-override flow above: it still calls
saveInstalledAppToDatabase(...)without the real outcome, so a successful Shizuku install is persisted as pending. Requiring the argument here lets the compiler catch every path that forgets to thread the enum through.Suggested change
private suspend fun saveInstalledAppToDatabase( assetName: String, assetUrl: String, assetSize: Long, releaseTag: String, isUpdate: Boolean, filePath: String, - installOutcome: InstallOutcome = InstallOutcome.DELEGATED_TO_SYSTEM, + installOutcome: InstallOutcome, )val installOutcome = installer.install(warning.pendingFilePath, ext) saveInstalledAppToDatabase( assetName = warning.pendingAssetName, assetUrl = warning.pendingDownloadUrl, assetSize = warning.pendingSizeBytes, releaseTag = warning.pendingReleaseTag, isUpdate = warning.pendingIsUpdate, filePath = warning.pendingFilePath, installOutcome = installOutcome, )🤖 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 1392 - 1400, The helper saveInstalledAppToDatabase currently has a defaulted parameter installOutcome which masks missing call-site updates; remove the default value from saveInstalledAppToDatabase's signature so installOutcome is mandatory, then update every call-site (notably the signing-key-override flow that calls saveInstalledAppToDatabase after installer.install) to pass the actual enum returned by installer.install (e.g., val installOutcome = installer.install(...) then pass installOutcome = installOutcome). Ensure all references to saveInstalledAppToDatabase are updated so the compiler enforces threading the InstallOutcome enum.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopInstaller.kt`:
- Around line 354-374: The install(...) function currently ignores the outcome
from platform-specific installers and always returns
InstallOutcome.DELEGATED_TO_SYSTEM; change it to capture and return the
InstallOutcome returned by installWindows(file, ext), installMacOS(file, ext),
or installLinux(file, ext) so synchronous flows (e.g., installAppImage,
successful installDebPackage/installRpmPackage, and the .pkg installer flow) can
return InstallOutcome.COMPLETED when appropriate; update the when(...)
expression to assign its result to a variable (or directly return the when
result) and ensure the method returns that InstallOutcome instead of the
hard-coded value.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 1459-1462: The update path currently calls updateAppVersion(...)
but never sets isPendingInstall, so delegated Android updates are not marked
pending and PackageEventReceiver.onPackageInstalled() skips pending-install
logic; fix by either extending updateAppVersion(...) to accept and persist an
isPendingInstall parameter (compute isPendingInstall = installOutcome !=
InstallOutcome.COMPLETED and update the DB there) or, if you prefer minimal
change, call a new or existing updatePendingStatus(appId, isPendingInstall)
immediately after updateAppVersion(...) in the update branch; ensure you
reference InstallOutcome, isPendingInstall, updateAppVersion, and
PackageEventReceiver.onPackageInstalled when making the change so the pending
flag is stored consistently with the insert branch.
---
Outside diff comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 1392-1400: The helper saveInstalledAppToDatabase currently has a
defaulted parameter installOutcome which masks missing call-site updates; remove
the default value from saveInstalledAppToDatabase's signature so installOutcome
is mandatory, then update every call-site (notably the signing-key-override flow
that calls saveInstalledAppToDatabase after installer.install) to pass the
actual enum returned by installer.install (e.g., val installOutcome =
installer.install(...) then pass installOutcome = installOutcome). Ensure all
references to saveInstalledAppToDatabase are updated so the compiler enforces
threading the InstallOutcome enum.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46800d1c-339c-44cd-9586-4a9f1718aa12
📒 Files selected for processing (5)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidInstaller.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopInstaller.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/Installer.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
Comment on lines
354
to
374
| override suspend fun install( | ||
| filePath: String, | ||
| extOrMime: String, | ||
| ) = withContext(Dispatchers.IO) { | ||
| val file = File(filePath) | ||
| if (!file.exists()) { | ||
| throw IllegalStateException("File not found: $filePath") | ||
| } | ||
| ): InstallOutcome = | ||
| withContext(Dispatchers.IO) { | ||
| val file = File(filePath) | ||
| if (!file.exists()) { | ||
| throw IllegalStateException("File not found: $filePath") | ||
| } | ||
|
|
||
| val ext = extOrMime.lowercase().removePrefix(".") | ||
| val ext = extOrMime.lowercase().removePrefix(".") | ||
|
|
||
| when (platform) { | ||
| Platform.WINDOWS -> installWindows(file, ext) | ||
| Platform.MACOS -> installMacOS(file, ext) | ||
| Platform.LINUX -> installLinux(file, ext) | ||
| else -> throw UnsupportedOperationException("Installation not supported on $platform") | ||
| when (platform) { | ||
| Platform.WINDOWS -> installWindows(file, ext) | ||
| Platform.MACOS -> installMacOS(file, ext) | ||
| Platform.LINUX -> installLinux(file, ext) | ||
| else -> throw UnsupportedOperationException("Installation not supported on $platform") | ||
| } | ||
|
|
||
| InstallOutcome.DELEGATED_TO_SYSTEM | ||
| } |
Contributor
There was a problem hiding this comment.
Return COMPLETED for the synchronous desktop paths.
This now always reports DELEGATED_TO_SYSTEM, but several branches finish before install(...) returns: installAppImage(), successful installDebPackage() / installRpmPackage() runs, and the successful .pkg flow that waits for installer. Any caller that relies on InstallOutcome will misclassify those installs as pending. Thread the outcome back from the platform helpers instead of hard-coding it here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopInstaller.kt`
around lines 354 - 374, The install(...) function currently ignores the outcome
from platform-specific installers and always returns
InstallOutcome.DELEGATED_TO_SYSTEM; change it to capture and return the
InstallOutcome returned by installWindows(file, ext), installMacOS(file, ext),
or installLinux(file, ext) so synchronous flows (e.g., installAppImage,
successful installDebPackage/installRpmPackage, and the .pkg installer flow) can
return InstallOutcome.COMPLETED when appropriate; update the when(...)
expression to assign its result to a variable (or directly return the when
result) and ensure the method returns that InstallOutcome instead of the
hard-coded value.
Comment on lines
1459
to
1462
| systemArchitecture = installer.detectSystemArchitecture().name, | ||
| fileExtension = assetName.substringAfterLast('.', ""), | ||
| isPendingInstall = true, | ||
| isPendingInstall = installOutcome != InstallOutcome.COMPLETED, | ||
| installedVersionName = apkInfo.versionName, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== updateAppVersion declarations / implementations ==="
fd 'InstalledAppsRepository.*\.kt$' | while read -r file; do
echo "--- $file ---"
rg -n -C4 '\bupdateAppVersion\s*\(' "$file"
done
echo
echo "=== pending-flag writes near repository methods ==="
fd 'InstalledAppsRepository.*\.kt$' | while read -r file; do
echo "--- $file ---"
rg -n -C4 'isPendingInstall|updatePendingStatus' "$file"
done
echo
echo "=== package receiver handling ==="
fd 'PackageEventReceiver\.kt$' | while read -r file; do
echo "--- $file ---"
rg -n -C4 'updateAppVersion|updatePendingStatus|isPendingInstall' "$file"
doneRepository: OpenHub-Store/GitHub-Store
Length of output: 4300
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full updateAppVersion implementation ==="
rg -n -A 20 'override suspend fun updateAppVersion' core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.kt
echo
echo "=== DetailsViewModel update branch around insertOrUpdateApp ==="
rg -n -B 5 -A 15 'updateAppVersion' feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt | head -80
echo
echo "=== DetailsViewModel insert branch for comparison ==="
rg -n -B 5 -A 15 'insertApp\|isPendingInstall.*=' feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt | head -100Repository: OpenHub-Store/GitHub-Store
Length of output: 1949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full updateAppVersion implementation (lines 181-240) ==="
sed -n '181,240p' core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.kt
echo
echo "=== DetailsViewModel isPendingInstall assignments ==="
rg -n 'isPendingInstall\s*=' feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
echo
echo "=== DetailsViewModel updatePendingStatus calls ==="
rg -n 'updatePendingStatus' feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
echo
echo "=== Complete insert branch in DetailsViewModel (lines 1433-1465) ==="
sed -n '1433,1465p' feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 4514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Context around updatePendingStatus call at line 272 ==="
sed -n '260,285p' feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
echo
echo "=== Check if there's a pending status write between updateAppVersion call and end of update branch ==="
sed -n '1423,1480p' feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 4248
Update path does not set pending status for delegated installs.
The insert branch correctly sets isPendingInstall = installOutcome != InstallOutcome.COMPLETED, but the update branch at line 1424 calls updateAppVersion(...) without any subsequent pending-status handling. The updateAppVersion() method does not include isPendingInstall in its update and has no parameter for it. This means delegated Android updates are never marked pending, causing PackageEventReceiver.onPackageInstalled() to skip the pending-install logic since the flag remains unset.
Either add a updatePendingStatus() call after updateAppVersion() in the update path, or add isPendingInstall handling to updateAppVersion() itself to match the insert path behavior.
🤖 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 1459 - 1462, The update path currently calls updateAppVersion(...)
but never sets isPendingInstall, so delegated Android updates are not marked
pending and PackageEventReceiver.onPackageInstalled() skips pending-install
logic; fix by either extending updateAppVersion(...) to accept and persist an
isPendingInstall parameter (compute isPendingInstall = installOutcome !=
InstallOutcome.COMPLETED and update the DB there) or, if you prefer minimal
change, call a new or existing updatePendingStatus(appId, isPendingInstall)
immediately after updateAppVersion(...) in the update branch; ensure you
reference InstallOutcome, isPendingInstall, updateAppVersion, and
PackageEventReceiver.onPackageInstalled when making the change so the pending
flag is stored consistently with the insert branch.
…wModel` - Extract APK validation, fingerprint checking, and database persistence into a new `InstallationManager` interface and implementation. - Introduce `AttestationVerifier` to handle GitHub supply-chain security verification independently. - Move version normalization and semantic comparison logic to a pure `VersionHelper` utility. - Refactor `DetailsViewModel` to utilize the new managers, significantly reducing its complexity and size. - Define sealed interfaces for `ApkValidationResult` and `FingerprintCheckResult` to handle installation edge cases more robustly. - Update Koin dependency injection modules to include the new domain and data layer components. - Standardize parameters for saving and updating installed apps using new data models (`SaveInstalledAppParams`, `UpdateInstalledAppParams`).
- Move domain models `ApkValidationResult`, `FingerprintCheckResult`, `SaveInstalledAppParams`, and `UpdateInstalledAppParams` to the `domain.model` package. - Update `UpdateInstalledAppParams` and the database save/update logic to include an `isPendingInstall` flag based on the `InstallOutcome`. - Update `InstallationManagerImpl` to persist the pending status in the repository when updating an app version. - Modify `DetailsViewModel` to pass the installation outcome when saving or updating app records after an install attempt.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/system/InstallationManager.kt (1)
40-43:⚠️ Potential issue | 🟠 Major
updateInstalledAppVersionstill can't persist pending installs.
InstallOutcomeonly helps if the update path can writeisPendingInstall.feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/model/UpdateInstalledAppParams.kt:1-10still carries only version metadata, so delegated Android updates have no way to be marked pending through this contract. Please extend this API, or add a dedicated pending-status update, so update flows can persistinstallOutcome != COMPLETED.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/system/InstallationManager.kt` around lines 40 - 43, The current updateInstalledAppVersion(params: UpdateInstalledAppParams) cannot persist pending installs because UpdateInstalledAppParams lacks install outcome info; modify the contract so UpdateInstalledAppParams includes an InstallOutcome (or a nullable isPendingInstall/installOutcome field) OR add a new suspend method (e.g. updateInstalledAppPendingStatus(appId: String, installOutcome: InstallOutcome)) on InstallationManager; update callers of updateInstalledAppVersion and any delegation paths that need to mark installs pending to use the new field/method so installOutcome != COMPLETED can be persisted.
🧹 Nitpick comments (1)
feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/util/VersionHelper.kt (1)
67-71: Pre-release identifiers are not compared when both versions have them.Currently,
"1.0.0-alpha"vs"1.0.0-beta"returns0(equal). Per SemVer spec, pre-release identifiers should be compared lexicographically/numerically. For downgrade detection this may be acceptable since both represent pre-release states of the same core version, but consider adding identifier comparison if finer granularity is needed later.♻️ Optional: Compare pre-release identifiers
val aHasPre = aCore.size > 1 val bHasPre = bCore.size > 1 if (aHasPre != bHasPre) return if (aHasPre) -1 else 1 + if (aHasPre && bHasPre) { + return aCore[1].compareTo(bCore[1]) + } return 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/util/VersionHelper.kt` around lines 67 - 71, The current logic in VersionHelper.kt returns 0 when both aHasPre and bHasPre are true, ignoring pre-release identifiers; update the comparison to iterate over the pre-release identifier arrays (the parts after core parsed into identifiers used to determine aHasPre/bHasPre), comparing each identifier lexicographically if non-numeric or numerically if both numeric, returning -1/1 on first difference, and if all equal return shorter identifier list as lower precedence; modify the block around aHasPre/bHasPre (using the same parsed pre-release arrays and existing functions in VersionHelper) to perform this identifier-by-identifier SemVer comparison instead of returning 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/system/AttestationVerifierImpl.kt`:
- Around line 6-8: The imports and JVM-specific implementation (File,
FileInputStream, MessageDigest) and use of String.format in
AttestationVerifierImpl.kt are platform-specific and must be moved out of
commonMain; refactor by removing computeSha256() from commonMain and either (A)
declare an expect fun computeSha256(path: String): String in commonMain and
implement actual fun computeSha256 in jvmMain/androidMain using
File/FileInputStream/MessageDigest and platform-safe string formatting, or (B)
move the entire computeSha256 implementation into jvmMain/androidMain and call
it from AttestationVerifierImpl; update references to use the platform
implementation and replace String.format with platform-appropriate formatting
(e.g., Kotlin string templates or platform-specific formatter) in the platform
files.
In
`@feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/system/InstallationManagerImpl.kt`:
- Around line 11-15: Update the imports in InstallationManagerImpl so the types
ApkValidationResult, FingerprintCheckResult, SaveInstalledAppParams, and
UpdateInstalledAppParams are imported from zed.rainxch.details.domain.model (not
zed.rainxch.details.domain.system); locate the import block that currently
brings in those four symbols and change their package to domain.model so the
class compiles.
In
`@feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/util/VersionHelper.kt`:
- Around line 9-14: In normalizeVersion, trim the input first then strip known
prefixes so leading whitespace doesn't prevent removal; specifically, call
trim()/orEmpty() before applying removePrefix("refs/tags/") and then
removePrefix("v")/removePrefix("V") (or normalize case) to handle tags like "
v1.2.3" and "refs/tags/v1.2.3"; update the logic in normalizeVersion to perform
trimming first and add stripping for the "refs/tags/" prefix while preserving
the existing orEmpty() fallback.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 45-50: The ViewModel imports the wrong package for several domain
types, causing compilation errors; change the imports for ApkValidationResult,
FingerprintCheckResult, SaveInstalledAppParams, and UpdateInstalledAppParams
from zed.rainxch.details.domain.system to zed.rainxch.details.domain.model so
DetailsViewModel can reference the correct types (leave AttestationVerifier and
InstallationManager under the system package). Locate the import block in
DetailsViewModel.kt and replace the four incorrect imports with the model
package equivalents for those specific symbols.
- Around line 555-644: The openAppManager branch launches a download coroutine
but never assigns its Job to currentDownloadJob, so CancelCurrentDownload cannot
cancel it; modify openAppManager to save the Job returned by
viewModelScope.launch into currentDownloadJob (same pattern used in
installViaExternalApp), and ensure you clear currentDownloadJob when the
download completes, when switching downloadStage to IDLE, and in the catch
block; reference the existing symbols openAppManager, currentDownloadJob,
viewModelScope.launch, downloader.download(...) and the catch/cleanup paths to
mirror the job bookkeeping already implemented for installViaExternalApp.
- Around line 860-862: The overrideSigningKeyWarning handler currently reads val
warning = _state.value.signingKeyWarning ?: return and calls
dismissDowngradeWarning() but does not clear the state-driven dialog flag;
update the state inside overrideSigningKeyWarning (invoked from
OnOverrideSigningKeyWarning) to null out signingKeyWarning (e.g., set
_state.value = _state.value.copy(signingKeyWarning = null) or call the existing
state-update helper) so the Compose screen stops showing the dialog after the
user accepts the override.
- Around line 1210-1217: The saveInstalledAppToDatabase function currently
defaults installOutcome to InstallOutcome.DELEGATED_TO_SYSTEM which hides real
results; remove the default so installOutcome is a required parameter on
saveInstalledAppToDatabase and update every caller to explicitly pass the actual
InstallOutcome returned by installers (e.g., ensure ShizukuInstallerWrapper
functions that can return InstallOutcome.COMPLETED forward that value into
saveInstalledAppToDatabase rather than relying on a default). Also run a
project-wide search for saveInstalledAppToDatabase usages and update each call
site to supply the correct InstallOutcome value.
---
Duplicate comments:
In
`@feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/system/InstallationManager.kt`:
- Around line 40-43: The current updateInstalledAppVersion(params:
UpdateInstalledAppParams) cannot persist pending installs because
UpdateInstalledAppParams lacks install outcome info; modify the contract so
UpdateInstalledAppParams includes an InstallOutcome (or a nullable
isPendingInstall/installOutcome field) OR add a new suspend method (e.g.
updateInstalledAppPendingStatus(appId: String, installOutcome: InstallOutcome))
on InstallationManager; update callers of updateInstalledAppVersion and any
delegation paths that need to mark installs pending to use the new field/method
so installOutcome != COMPLETED can be persisted.
---
Nitpick comments:
In
`@feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/util/VersionHelper.kt`:
- Around line 67-71: The current logic in VersionHelper.kt returns 0 when both
aHasPre and bHasPre are true, ignoring pre-release identifiers; update the
comparison to iterate over the pre-release identifier arrays (the parts after
core parsed into identifiers used to determine aHasPre/bHasPre), comparing each
identifier lexicographically if non-numeric or numerically if both numeric,
returning -1/1 on first difference, and if all equal return shorter identifier
list as lower precedence; modify the block around aHasPre/bHasPre (using the
same parsed pre-release arrays and existing functions in VersionHelper) to
perform this identifier-by-identifier SemVer comparison instead of returning 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 501ec347-7b44-4299-abcb-2630fc03d0ff
📒 Files selected for processing (11)
feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/di/SharedModule.ktfeature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/system/AttestationVerifierImpl.ktfeature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/system/InstallationManagerImpl.ktfeature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/model/ApkValidationResult.ktfeature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/model/FingerprintCheckResult.ktfeature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/model/SaveInstalledAppParams.ktfeature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/model/UpdateInstalledAppParams.ktfeature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/system/AttestationVerifier.ktfeature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/system/InstallationManager.ktfeature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/util/VersionHelper.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
✅ Files skipped from review due to trivial changes (4)
- feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/model/UpdateInstalledAppParams.kt
- feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/system/AttestationVerifier.kt
- feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/model/FingerprintCheckResult.kt
- feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/model/SaveInstalledAppParams.kt
Comment on lines
+6
to
+8
| import java.io.File | ||
| import java.io.FileInputStream | ||
| import java.security.MessageDigest |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== JVM-specific imports under commonMain ==="
rg -n -g '**/commonMain/**/*.kt' '^import java\.'
echo
echo "=== JVM-style format calls under commonMain ==="
rg -n -g '**/commonMain/**/*.kt' '\.format\s*\('
echo
echo "=== AttestationVerifierImpl.kt ==="
fd 'AttestationVerifierImpl\.kt$' --exec sed -n '1,80p' {}Repository: OpenHub-Store/GitHub-Store
Length of output: 4909
Move JVM-only digest implementation out of commonMain.
Lines 6-8 import JVM-specific APIs (java.io.File, java.io.FileInputStream, java.security.MessageDigest), and line 36 uses String.format() which is not available across all Kotlin Multiplatform targets. This breaks the shared code requirement. Move computeSha256() to a platform-specific source set (jvmMain for Desktop, androidMain for Android), or define the method as expect in commonMain and provide actual implementations per platform.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/system/AttestationVerifierImpl.kt`
around lines 6 - 8, The imports and JVM-specific implementation (File,
FileInputStream, MessageDigest) and use of String.format in
AttestationVerifierImpl.kt are platform-specific and must be moved out of
commonMain; refactor by removing computeSha256() from commonMain and either (A)
declare an expect fun computeSha256(path: String): String in commonMain and
implement actual fun computeSha256 in jvmMain/androidMain using
File/FileInputStream/MessageDigest and platform-safe string formatting, or (B)
move the entire computeSha256 implementation into jvmMain/androidMain and call
it from AttestationVerifierImpl; update references to use the platform
implementation and replace String.format with platform-appropriate formatting
(e.g., Kotlin string templates or platform-specific formatter) in the platform
files.
...etails/data/src/commonMain/kotlin/zed/rainxch/details/data/system/InstallationManagerImpl.kt
Outdated
Show resolved
Hide resolved
Comment on lines
+9
to
+14
| fun normalizeVersion(version: String?): String = | ||
| version | ||
| ?.removePrefix("v") | ||
| ?.removePrefix("V") | ||
| ?.trim() | ||
| .orEmpty() |
Contributor
There was a problem hiding this comment.
trim() should be called before removePrefix() to handle leading whitespace.
If the input contains leading whitespace (e.g., " v1.2.3"), removePrefix("v") won't match because the string doesn't start with "v". Per the context, tagName from the GitHub API may contain whitespace.
Additionally, GitHub tag names can sometimes include "refs/tags/" prefix—consider stripping that as well for robustness.
🐛 Proposed fix
fun normalizeVersion(version: String?): String =
version
+ ?.trim()
+ ?.removePrefix("refs/tags/")
?.removePrefix("v")
?.removePrefix("V")
- ?.trim()
.orEmpty()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/util/VersionHelper.kt`
around lines 9 - 14, In normalizeVersion, trim the input first then strip known
prefixes so leading whitespace doesn't prevent removal; specifically, call
trim()/orEmpty() before applying removePrefix("refs/tags/") and then
removePrefix("v")/removePrefix("V") (or normalize case) to handle tags like "
v1.2.3" and "refs/tags/v1.2.3"; update the logic in normalizeVersion to perform
trimming first and add stripping for the "refs/tags/" prefix while preserving
the existing orEmpty() fallback.
...ails/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
Outdated
Show resolved
Hide resolved
Comment on lines
+555
to
+644
| private fun openAppManager() { | ||
| viewModelScope.launch { | ||
| try { | ||
| val primary = _state.value.primaryAsset | ||
| val release = _state.value.selectedRelease | ||
|
|
||
| if (primary != null && release != null) { | ||
| currentAssetName = primary.name | ||
|
|
||
| appendLog( | ||
| assetName = primary.name, | ||
| size = primary.size, | ||
| tag = release.tagName, | ||
| result = LogResult.PreparingForAppManager, | ||
| ) | ||
|
|
||
| _state.value = | ||
| _state.value.copy( | ||
| downloadError = null, | ||
| installError = null, | ||
| downloadProgressPercent = null, | ||
| downloadStage = DownloadStage.DOWNLOADING, | ||
| ) | ||
|
|
||
| downloader.download(primary.downloadUrl, primary.name).collect { p -> | ||
| _state.value = | ||
| _state.value.copy(downloadProgressPercent = p.percent) | ||
| if (p.percent == 100) { | ||
| _state.value = | ||
| _state.value.copy(downloadStage = DownloadStage.VERIFYING) | ||
| } | ||
| } | ||
|
|
||
| val filePath = | ||
| downloader.getDownloadedFilePath(primary.name) | ||
| ?: throw IllegalStateException("Downloaded file not found") | ||
|
|
||
| appendLog( | ||
| assetName = primary.name, | ||
| size = primary.size, | ||
| tag = release.tagName, | ||
| result = LogResult.Downloaded, | ||
| ) | ||
|
|
||
| _state.value = _state.value.copy(downloadStage = DownloadStage.IDLE) | ||
| currentAssetName = null | ||
|
|
||
| installer.openInAppManager( | ||
| filePath = filePath, | ||
| onOpenInstaller = { | ||
| viewModelScope.launch { | ||
| _events.send( | ||
| DetailsEvent.OnOpenRepositoryInApp(APP_MANAGER_REPO_ID), | ||
| ) | ||
| } | ||
| }, | ||
| ) | ||
|
|
||
| appendLog( | ||
| assetName = primary.name, | ||
| size = primary.size, | ||
| tag = release.tagName, | ||
| result = LogResult.OpenedInAppManager, | ||
| ) | ||
| } | ||
| } catch (t: Throwable) { | ||
| logger.error("Failed to open in AppManager: ${t.message}") | ||
| _state.value = | ||
| _state.value.copy( | ||
| downloadStage = DownloadStage.IDLE, | ||
| installError = t.message, | ||
| ) | ||
| currentAssetName = null | ||
|
|
||
| _state.value.primaryAsset?.let { asset -> | ||
| _state.value.selectedRelease?.let { release -> | ||
| appendLog( | ||
| assetName = asset.name, | ||
| size = asset.size, | ||
| tag = release.tagName, | ||
| result = Error(t.message), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| _state.update { | ||
| it.copy(isInstallDropdownExpanded = false) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Track the App Manager download in currentDownloadJob.
Unlike installViaExternalApp(), this branch never stores its Job, so DetailsAction.CancelCurrentDownload cannot cancel the ongoing download once downloadStage switches to DOWNLOADING. Mirror the same job bookkeeping here.
🤖 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 555 - 644, The openAppManager branch launches a download coroutine
but never assigns its Job to currentDownloadJob, so CancelCurrentDownload cannot
cancel it; modify openAppManager to save the Job returned by
viewModelScope.launch into currentDownloadJob (same pattern used in
installViaExternalApp), and ensure you clear currentDownloadJob when the
download completes, when switching downloadStage to IDLE, and in the catch
block; reference the existing symbols openAppManager, currentDownloadJob,
viewModelScope.launch, downloader.download(...) and the catch/cleanup paths to
mirror the job bookkeeping already implemented for installViaExternalApp.
Comment on lines
+860
to
+862
| private fun overrideSigningKeyWarning() { | ||
| val warning = _state.value.signingKeyWarning ?: return | ||
| dismissDowngradeWarning() |
Contributor
There was a problem hiding this comment.
Clear signingKeyWarning when the user accepts the override.
This handler is entered from OnOverrideSigningKeyWarning, but it never nulls the state that drives that dialog. In a state-driven Compose screen the warning can stay visible even after the install continues.
💡 Proposed fix
private fun overrideSigningKeyWarning() {
val warning = _state.value.signingKeyWarning ?: return
+ _state.update { it.copy(signingKeyWarning = null) }
dismissDowngradeWarning()
viewModelScope.launch {🤖 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 860 - 862, The overrideSigningKeyWarning handler currently reads
val warning = _state.value.signingKeyWarning ?: return and calls
dismissDowngradeWarning() but does not clear the state-driven dialog flag;
update the state inside overrideSigningKeyWarning (invoked from
OnOverrideSigningKeyWarning) to null out signingKeyWarning (e.g., set
_state.value = _state.value.copy(signingKeyWarning = null) or call the existing
state-update helper) so the Compose screen stops showing the dialog after the
user accepts the override.
Comment on lines
+1210
to
+1217
| private suspend fun saveInstalledAppToDatabase( | ||
| assetName: String, | ||
| assetUrl: String, | ||
| assetSize: Long, | ||
| releaseTag: String, | ||
| isUpdate: Boolean, | ||
| filePath: String, | ||
| installOutcome: InstallOutcome = InstallOutcome.DELEGATED_TO_SYSTEM, |
Contributor
There was a problem hiding this comment.
Don't default installOutcome to DELEGATED_TO_SYSTEM.
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.kt:98-142 can return InstallOutcome.COMPLETED, but this helper silently falls back to DELEGATED_TO_SYSTEM. That already lets overrideSigningKeyWarning() persist a synchronous Shizuku install as pending simply because it forgot to forward the real result. Make the parameter mandatory so every install path passes the actual outcome.
💡 Proposed fix
private suspend fun saveInstalledAppToDatabase(
assetName: String,
assetUrl: String,
assetSize: Long,
releaseTag: String,
isUpdate: Boolean,
filePath: String,
- installOutcome: InstallOutcome = InstallOutcome.DELEGATED_TO_SYSTEM,
+ installOutcome: InstallOutcome,
) {🤖 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 1210 - 1217, The saveInstalledAppToDatabase function currently
defaults installOutcome to InstallOutcome.DELEGATED_TO_SYSTEM which hides real
results; remove the default so installOutcome is a required parameter on
saveInstalledAppToDatabase and update every caller to explicitly pass the actual
InstallOutcome returned by installers (e.g., ensure ShizukuInstallerWrapper
functions that can return InstallOutcome.COMPLETED forward that value into
saveInstalledAppToDatabase rather than relying on a default). Also run a
project-wide search for saveInstalledAppToDatabase usages and update each call
site to supply the correct InstallOutcome value.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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)
1295-1307:⚠️ Potential issue | 🟡 MinorDon't turn a user cancel into a download error.
When
cancelCurrentDownload()cancels the job, it raisesCancellationExceptionin the coroutine. The currentcatch (Throwable)handler at lines 1295–1307 catches this exception, settingdownloadErrorand loggingLogResult.Error, resulting in both aCancelledlog (from the explicit cancellation) and an error log for the same operation. This masks the user's intentional cancellation.The codebase already uses this pattern elsewhere (lines 466, 936, 1625): catch
CancellationExceptionseparately and rethrow it to let the coroutine framework handle cancellation properly.Proposed fix
- } catch (t: Throwable) { + } catch (e: CancellationException) { + throw e + } catch (t: Throwable) { _state.value = _state.value.copy( isDownloading = false, downloadError = t.message, )🤖 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 1295 - 1307, The catch-all in DetailsViewModel's download coroutine (catch (t: Throwable) around appendLog and _state updates) is treating CancellationException as an error; change the handler to rethrow CancellationException instead of handling it (i.e., catch CancellationException separately and throw, or check and rethrow inside the catch), and only set _state.downloadError and call appendLog(LogResult.Error(...)) for non-cancellation failures; reference the cancelCurrentDownload logic, the catch (t: Throwable) block, appendLog(...) and currentAssetName handling to locate where to apply this change.
♻️ Duplicate comments (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt (1)
860-862:⚠️ Potential issue | 🟠 MajorClear
signingKeyWarningbefore continuing the override flow.This handler is entered from the warning dialog, but it never nulls the state that drives that dialog. In a state-driven Compose screen the warning can stay visible while the install proceeds.
💡 Proposed fix
private fun overrideSigningKeyWarning() { val warning = _state.value.signingKeyWarning ?: return + _state.update { it.copy(signingKeyWarning = null) } dismissDowngradeWarning() viewModelScope.launch {🤖 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 860 - 862, The overrideSigningKeyWarning handler never clears the signingKeyWarning state so the warning UI may remain visible; update the function overrideSigningKeyWarning to set _state.value.signingKeyWarning to null (or otherwise clear that field on the state object) before calling dismissDowngradeWarning() so the Compose-driven dialog is dismissed; reference the state container _state and the signingKeyWarning property when applying the clear so the UI no longer shows the warning while the install proceeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/system/InstallationManagerImpl.kt`:
- Around line 122-133: The current updateInstalledAppVersion suspends and calls
installedAppsRepository.updateAppVersion(...) and then
installedAppsRepository.updatePendingStatus(...), risking partial writes; change
this to perform a single repository update that includes the pending flag in the
same transaction: either add a new parameter (e.g., isPendingInstall) to
installedAppsRepository.updateAppVersion(...) and persist it there, or create a
new repository method (e.g., updateAppVersionWithPending(...) or
updateInstalledAppTransaction(...)) that atomically updates newTag,
newAssetName, newAssetUrl, newVersionName, newVersionCode, signingFingerprint
and isPendingInstall in one DB operation/transaction, then call that single
repository method from updateInstalledAppVersion.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 1097-1104: Attestation failures are being conflated with verifier
errors because AttestationVerifier.verify() returns false on exceptions; change
the contract so errors are distinguishable (e.g., make verify return a
Result<Boolean> or a sealed VerificationResult { Verified, Unverified,
Error(reason)}) and update launchAttestationCheck() to map the new error case to
a new AttestationStatus (add e.g. ATTEMPT_FAILED or UNABLE_TO_VERIFY to the
AttestationStatus enum) instead of UNVERIFIED; also update
AttestationVerifierImpl.verify() to stop swallowing exceptions and return the
error variant so the UI can show “could not verify” separately from “missing
attestation.”
- Around line 1211-1227: The helper saveInstalledAppToDatabase currently
re-extracts APK metadata via
installer.getApkInfoExtractor().extractPackageInfo(filePath) and skips
persistence if that fails; instead change saveInstalledAppToDatabase signature
to accept the already-validated ApkPackageInfo (e.g., parameter name apkInfo),
remove the second extraction and null-check there, and use the passed apkInfo
fields for persistence; then update the caller installAsset to pass its
validated apkInfo into saveInstalledAppToDatabase so persistence always uses the
validated metadata.
---
Outside diff comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 1295-1307: The catch-all in DetailsViewModel's download coroutine
(catch (t: Throwable) around appendLog and _state updates) is treating
CancellationException as an error; change the handler to rethrow
CancellationException instead of handling it (i.e., catch CancellationException
separately and throw, or check and rethrow inside the catch), and only set
_state.downloadError and call appendLog(LogResult.Error(...)) for
non-cancellation failures; reference the cancelCurrentDownload logic, the catch
(t: Throwable) block, appendLog(...) and currentAssetName handling to locate
where to apply this change.
---
Duplicate comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 860-862: The overrideSigningKeyWarning handler never clears the
signingKeyWarning state so the warning UI may remain visible; update the
function overrideSigningKeyWarning to set _state.value.signingKeyWarning to null
(or otherwise clear that field on the state object) before calling
dismissDowngradeWarning() so the Compose-driven dialog is dismissed; reference
the state container _state and the signingKeyWarning property when applying the
clear so the UI no longer shows the warning while the install proceeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ceee88b-8f59-41ae-baa0-7040c5d296c9
📒 Files selected for processing (3)
feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/system/InstallationManagerImpl.ktfeature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/model/UpdateInstalledAppParams.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/model/UpdateInstalledAppParams.kt
Comment on lines
+122
to
+133
| override suspend fun updateInstalledAppVersion(params: UpdateInstalledAppParams) { | ||
| val packageName = params.apkInfo.packageName | ||
| installedAppsRepository.updateAppVersion( | ||
| packageName = packageName, | ||
| newTag = params.releaseTag, | ||
| newAssetName = params.assetName, | ||
| newAssetUrl = params.assetUrl, | ||
| newVersionName = params.apkInfo.versionName, | ||
| newVersionCode = params.apkInfo.versionCode, | ||
| signingFingerprint = params.apkInfo.signingFingerprint, | ||
| ) | ||
| installedAppsRepository.updatePendingStatus(packageName, params.isPendingInstall) |
Contributor
There was a problem hiding this comment.
Update the pending flag in the same persistence operation.
updateAppVersion(...) and updatePendingStatus(...) are separate writes here. If the second call fails, the record keeps the new version fields with the old pending flag, which directly breaks the install-outcome tracking this PR is adding. Please fold isPendingInstall into the same repository update/transaction as the version write.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/system/InstallationManagerImpl.kt`
around lines 122 - 133, The current updateInstalledAppVersion suspends and calls
installedAppsRepository.updateAppVersion(...) and then
installedAppsRepository.updatePendingStatus(...), risking partial writes; change
this to perform a single repository update that includes the pending flag in the
same transaction: either add a new parameter (e.g., isPendingInstall) to
installedAppsRepository.updateAppVersion(...) and persist it there, or create a
new repository method (e.g., updateAppVersionWithPending(...) or
updateInstalledAppTransaction(...)) that atomically updates newTag,
newAssetName, newAssetUrl, newVersionName, newVersionCode, signingFingerprint
and isPendingInstall in one DB operation/transaction, then call that single
repository method from updateInstalledAppVersion.
Comment on lines
1097
to
1104
| viewModelScope.launch { | ||
| try { | ||
| val digest = computeSha256(filePath) | ||
| val verified = detailsRepository.checkAttestations(owner, repoName, digest) | ||
| _state.update { | ||
| it.copy( | ||
| attestationStatus = | ||
| if (verified) AttestationStatus.VERIFIED else AttestationStatus.UNVERIFIED, | ||
| ) | ||
| } | ||
| } catch (e: Exception) { | ||
| logger.debug("Attestation check error: ${e.message}") | ||
| _state.update { it.copy(attestationStatus = AttestationStatus.UNVERIFIED) } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun computeSha256(filePath: String): String { | ||
| val digest = MessageDigest.getInstance("SHA-256") | ||
| val buffer = ByteArray(8192) | ||
| FileInputStream(File(filePath)).use { fis -> | ||
| var bytesRead: Int | ||
| while (fis.read(buffer).also { bytesRead = it } != -1) { | ||
| digest.update(buffer, 0, bytesRead) | ||
| val verified = attestationVerifier.verify(owner, repoName, filePath) | ||
| _state.update { | ||
| it.copy( | ||
| attestationStatus = | ||
| if (verified) AttestationStatus.VERIFIED else AttestationStatus.UNVERIFIED, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Don't report verifier failures as UNVERIFIED.
launchAttestationCheck() maps every false from AttestationVerifier.verify() to AttestationStatus.UNVERIFIED, but feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/system/AttestationVerifierImpl.kt also returns false on exceptions. A timeout or file-read failure now looks identical to a genuine attestation miss. This should return a richer result so the UI can distinguish “could not verify” from “verified and missing”.
🤖 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 1097 - 1104, Attestation failures are being conflated with verifier
errors because AttestationVerifier.verify() returns false on exceptions; change
the contract so errors are distinguishable (e.g., make verify return a
Result<Boolean> or a sealed VerificationResult { Verified, Unverified,
Error(reason)}) and update launchAttestationCheck() to map the new error case to
a new AttestationStatus (add e.g. ATTEMPT_FAILED or UNABLE_TO_VERIFY to the
AttestationStatus enum) instead of UNVERIFIED; also update
AttestationVerifierImpl.verify() to stop swallowing exceptions and return the
error variant so the UI can show “could not verify” separately from “missing
attestation.”
Comment on lines
+1211
to
+1227
| private suspend fun saveInstalledAppToDatabase( | ||
| assetName: String, | ||
| assetUrl: String, | ||
| assetSize: Long, | ||
| releaseTag: String, | ||
| isUpdate: Boolean, | ||
| filePath: String, | ||
| installOutcome: InstallOutcome = InstallOutcome.DELEGATED_TO_SYSTEM, | ||
| ) { | ||
| val repo = _state.value.repository ?: return | ||
| if (platform != Platform.ANDROID || !assetName.lowercase().endsWith(".apk")) return | ||
|
|
||
| delay(1000) | ||
| val updatedApp = installedAppsRepository.getAppByPackage(apkInfo.packageName) | ||
| _state.value = _state.value.copy(installedApp = updatedApp) | ||
| val apkInfo = installer.getApkInfoExtractor().extractPackageInfo(filePath) | ||
| if (apkInfo == null) { | ||
| logger.error("Failed to extract APK info for $assetName") | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Reuse the validated APK metadata instead of extracting it again.
installAsset() already validated this APK before installer.install(...), but saveInstalledAppToDatabase() parses the file again afterwards. If that second extraction fails, persistence is skipped while the caller still clears the stage and appends an Installed/Updated log. Please thread the validated ApkPackageInfo through this helper instead of re-reading the file here.
🤖 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 1211 - 1227, The helper saveInstalledAppToDatabase currently
re-extracts APK metadata via
installer.getApkInfoExtractor().extractPackageInfo(filePath) and skips
persistence if that fails; instead change saveInstalledAppToDatabase signature
to accept the already-validated ApkPackageInfo (e.g., parameter name apkInfo),
remove the second extraction and null-check there, and use the passed apkInfo
fields for persistence; then update the caller installAsset to pass its
validated apkInfo into saveInstalledAppToDatabase so persistence always uses the
validated metadata.
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.
InstallOutcomeenum to distinguish between synchronous (COMPLETED) and asynchronous (DELEGATED_TO_SYSTEM) installation processes.Installer.installsignature to returnInstallOutcome.InstallOutcome.COMPLETEDinShizukuInstallerWrapperfor successful silent installs.InstallOutcome.DELEGATED_TO_SYSTEMfor standard Android and Desktop installers where the OS handles the UI.DetailsViewModelto use the installation outcome to more accurately set theisPendingInstallflag.DetailsViewModelto propagate the outcome to repository sync operations.Summary by CodeRabbit
New Features
Refactor