feat: APK Inspect feature + faithful pending-row UX#489
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an APK inspection feature and UI (button, coachmark, bottom sheet), domain model and ApkInspector interface, Android/JVM inspector implementations, DI wiring, parked-APK tracking and discard flows, icon fallback from parked APKs, preferences/repo changes, and sync/cleanup logic for stale parked files. ChangesAPK Inspection + Pending-Install UX
Sequence Diagram(s)sequenceDiagram
participant UI as Compose UI
participant VM as DetailsViewModel
participant Inspector as ApkInspector
participant PM as PackageManager/FS/Repo
UI->>VM: OnInspectApk
VM->>VM: set isApkInspectLoading/isApkInspectSheetVisible
VM->>Inspector: inspectInstalled(package) or inspectFile(path)
Inspector->>PM: read PackageInfo / APK file
PM-->>Inspector: PackageInfo / file metadata
Inspector-->>VM: ApkInspection? (or null)
VM->>TweaksRepo: set coachmark shown (if needed)
VM-->>UI: state update -> show sheet / content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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)
2101-2125:⚠️ Potential issue | 🔴 CriticalClear pending-install metadata when update completes successfully with
isPendingInstall = false.The
updateAppVersionimplementation inInstalledAppsRepositoryImpl.ktdoes not clearpendingInstallFilePath,pendingInstallVersion, andpendingInstallAssetNamewhen the update completes. Theapp.copy(...)call only explicitly setsisPendingInstallwithout nulling the parked metadata fields, so they retain their previous values. When combined with the DetailsViewModel code here that only writes pending paths on pending installs, a row that previously had parked metadata from a cancelled install will keep those fields after a successful update, causing the UI to incorrectly display the app as "ready to install" even though it has already been updated.Code reference
if (isUpdate) { installationManager.updateInstalledAppVersion( UpdateInstalledAppParams( apkInfo = apkInfo, assetName = assetName, assetUrl = assetUrl, releaseTag = releaseTag, isPendingInstall = isPending, ), ) // For pending updates, also park the file path on the row // so the apps list can resume the install in one tap if // the user dismissed the system prompt. if (pendingPath != null) { runCatching { installedAppsRepository.setPendingInstallFilePath( packageName = apkInfo.packageName, path = pendingPath, version = releaseTag, assetName = assetName, ) }.onFailure { t -> logger.warn("Failed to park pending install path on update: ${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 2101 - 2125, The updateAppVersion flow in InstalledAppsRepositoryImpl (the method that builds app.copy(...)) must null out pendingInstallFilePath, pendingInstallVersion, and pendingInstallAssetName when isPendingInstall is false; modify the updateAppVersion implementation so that when isPendingInstall == false you set those three fields to null in the app.copy(...) result (in addition to setting isPendingInstall), ensuring any previously parked metadata is cleared after a successful update and matches the DetailsViewModel calls to setPendingInstallFilePath.
🧹 Nitpick comments (3)
core/data/src/jvmMain/kotlin/zed/rainxch/core/data/di/PlatformModule.jvm.kt (1)
66-68: 💤 Low valueLGTM — binding is correct; consider adding imports for consistency.
Minor nit: Lines 66–68 use fully-qualified type names while every other binding in the file uses imported short names. Adding
import zed.rainxch.core.domain.system.ApkInspectorandimport zed.rainxch.core.data.services.DesktopApkInspectorwould keep the module consistent.🤖 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/di/PlatformModule.jvm.kt` around lines 66 - 68, The binding uses fully-qualified names instead of imports; add imports for ApkInspector and DesktopApkInspector and replace the fully-qualified references in the single<...> declaration so the module matches the project's import style (reference symbols: ApkInspector and DesktopApkInspector, and the single binding that currently instantiates DesktopApkInspector).feature/apps/presentation/src/androidMain/kotlin/zed/rainxch/apps/presentation/components/InstalledAppIcon.android.kt (1)
69-69: ⚡ Quick win
getPackageArchiveInfo(String, Int)is deprecated since API 33
#getPackageArchiveInfo(String, PackageInfoFlags)should be used when long flags are needed. Theintflags parameter was replaced withPackageInfoFlagsin API 33. The current call will produce a lint/compiler deprecation warning on targets API 33+. The fix requires a version branch:♻️ Proposed fix — branch on API level
+import android.os.Build +import android.content.pm.PackageManager.PackageInfoFlags val info = packageManager.getPackageArchiveInfo(apkFilePath, 0)- val info = packageManager.getPackageArchiveInfo(apkFilePath, 0) + val info = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + packageManager.getPackageArchiveInfo(apkFilePath, PackageInfoFlags.of(0L)) + } else { + `@Suppress`("DEPRECATION") + packageManager.getPackageArchiveInfo(apkFilePath, 0) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/apps/presentation/src/androidMain/kotlin/zed/rainxch/apps/presentation/components/InstalledAppIcon.android.kt` at line 69, The call to packageManager.getPackageArchiveInfo(apkFilePath, 0) in InstalledAppIcon.android.kt is using the deprecated int-flag overload; update it to branch on API level: for SDK_INT >= 33 call packageManager.getPackageArchiveInfo(apkFilePath, PackageInfoFlags.of(0)) and for older SDKs keep packageManager.getPackageArchiveInfo(apkFilePath, 0); locate the usage of apkFilePath and the getPackageArchiveInfo call inside the InstalledAppIcon component and replace with a version-guarded call (import PackageInfoFlags) so the code compiles without deprecation warnings on API 33+.feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsRoot.kt (1)
373-379: 💤 Low valuePrefer an import over the fully-qualified class name for
ApkInspectSheet.
ApkInspectSheetis already imported unqualified inHeader.kt. Using the fully-qualified name here is inconsistent with every other component reference in this file.♻️ Proposed fix
Add to the import block (alongside the existing component imports):
+import zed.rainxch.details.presentation.components.ApkInspectSheetThen simplify the call site:
- zed.rainxch.details.presentation.components.ApkInspectSheet( + ApkInspectSheet( inspection = state.apkInspection, isLoading = state.isApkInspectLoading, onDismiss = { viewModel.onAction(DetailsAction.OnDismissApkInspect) }, )🤖 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/DetailsRoot.kt` around lines 373 - 379, The call site uses a fully-qualified name for ApkInspectSheet; add an import for zed.rainxch.details.presentation.components.ApkInspectSheet to the import block and replace the fully-qualified reference in DetailsRoot (the conditional guarded by state.isApkInspectSheetVisible) with the simple ApkInspectSheet(...) call, keeping the same parameters (inspection = state.apkInspection, isLoading = state.isApkInspectLoading, onDismiss = { viewModel.onAction(DetailsAction.OnDismissApkInspect) }).
🤖 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/androidMain/kotlin/zed/rainxch/core/data/services/AndroidApkInspector.kt`:
- Around line 22-71: Both inspectFile and inspectInstalled can let
PackageManager/resource lookup exceptions escape from buildInspection(),
violating the ApkInspector contract; wrap the buildInspection(...) calls in a
try/catch that catches Throwable (or at minimum RuntimeException/Exception) and
on any failure log the error (using Logger.w or Logger.e with TAG) and return
null from the withContext block so callers like
DetailsViewModel.openApkInspectSheet() receive null instead of staying stuck;
specifically modify the calls to buildInspection inside inspectFile and
inspectInstalled to be inside a try { val result = buildInspection(...) ; result
} catch (t: Throwable) { Logger.w(TAG) { "inspect...: failed to extract APK
metadata: $t" } ; null }.
In
`@feature/apps/presentation/src/androidMain/kotlin/zed/rainxch/apps/presentation/components/InstalledAppIcon.android.kt`:
- Around line 47-58: The function resolveInstalledIcon currently only catches
NameNotFoundException which lets other runtime errors (e.g., SecurityException
from packageManager.getApplicationIcon or exceptions from Drawable.toBitmap)
bubble up into the remember lambda; change its error handling to match
resolveApkIcon by broadening the catch to catch Throwable (or Exception) around
the whole try block so any unexpected exceptions are swallowed and the function
returns null instead of crashing the Composable; update the catch site in
resolveInstalledIcon and ensure you still return null on failure.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsRoot.kt`:
- Around line 1163-1172: The discard IconButton currently calls
onDiscardPendingClick immediately; add a confirmation guard like the existing
appPendingUninstall flow so users must confirm before the parked APK/DB row is
removed: introduce a local boolean state (e.g., showConfirmDiscardPending), show
the same confirmation dialog UI used by appPendingUninstall when the IconButton
is clicked (set showConfirmDiscardPending = true), and only call
onDiscardPendingClick when the user confirms; apply the same pattern to the
legacy full-width "Discard" Button handling the pending rows so both triggers
use the confirmation dialog.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/Header.kt`:
- Around line 141-143: The computed flag canInspectApk is wrong: replace the
current expression with a check that installedApp exists and either is not a
pending install or has a pendingInstallFilePath; e.g. grab val app =
state.installedApp and set val canInspectApk = app != null &&
(!app.isPendingInstall || app.pendingInstallFilePath != null) so
InspectApkButton is only shown when there is something concrete to inspect.
---
Outside diff comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 2101-2125: The updateAppVersion flow in
InstalledAppsRepositoryImpl (the method that builds app.copy(...)) must null out
pendingInstallFilePath, pendingInstallVersion, and pendingInstallAssetName when
isPendingInstall is false; modify the updateAppVersion implementation so that
when isPendingInstall == false you set those three fields to null in the
app.copy(...) result (in addition to setting isPendingInstall), ensuring any
previously parked metadata is cleared after a successful update and matches the
DetailsViewModel calls to setPendingInstallFilePath.
---
Nitpick comments:
In `@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/di/PlatformModule.jvm.kt`:
- Around line 66-68: The binding uses fully-qualified names instead of imports;
add imports for ApkInspector and DesktopApkInspector and replace the
fully-qualified references in the single<...> declaration so the module matches
the project's import style (reference symbols: ApkInspector and
DesktopApkInspector, and the single binding that currently instantiates
DesktopApkInspector).
In
`@feature/apps/presentation/src/androidMain/kotlin/zed/rainxch/apps/presentation/components/InstalledAppIcon.android.kt`:
- Line 69: The call to packageManager.getPackageArchiveInfo(apkFilePath, 0) in
InstalledAppIcon.android.kt is using the deprecated int-flag overload; update it
to branch on API level: for SDK_INT >= 33 call
packageManager.getPackageArchiveInfo(apkFilePath, PackageInfoFlags.of(0)) and
for older SDKs keep packageManager.getPackageArchiveInfo(apkFilePath, 0); locate
the usage of apkFilePath and the getPackageArchiveInfo call inside the
InstalledAppIcon component and replace with a version-guarded call (import
PackageInfoFlags) so the code compiles without deprecation warnings on API 33+.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsRoot.kt`:
- Around line 373-379: The call site uses a fully-qualified name for
ApkInspectSheet; add an import for
zed.rainxch.details.presentation.components.ApkInspectSheet to the import block
and replace the fully-qualified reference in DetailsRoot (the conditional
guarded by state.isApkInspectSheetVisible) with the simple ApkInspectSheet(...)
call, keeping the same parameters (inspection = state.apkInspection, isLoading =
state.isApkInspectLoading, onDismiss = {
viewModel.onAction(DetailsAction.OnDismissApkInspect) }).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff95af83-f787-40ed-848f-e0aa5e6d0df9
📒 Files selected for processing (27)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/di/ViewModelsModule.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/di/PlatformModule.android.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidApkInspector.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/SigningFingerprint.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/di/PlatformModule.jvm.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopApkInspector.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/ApkInspection.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/TweaksRepository.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/ApkInspector.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/apps/presentation/src/androidMain/kotlin/zed/rainxch/apps/presentation/components/InstalledAppIcon.android.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsAction.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsRoot.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/CompactAppRow.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/InstalledAppIcon.ktfeature/apps/presentation/src/jvmMain/kotlin/zed/rainxch/apps/presentation/components/InstalledAppIcon.jvm.ktfeature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/system/InstallationManagerImpl.ktfeature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/model/SaveInstalledAppParams.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsAction.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsRoot.ktfeature/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/ApkInspectSheet.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/InspectApkButton.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/Header.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ApkInspectSheet.kt`:
- Around line 206-210: The InspectRow currently combines inspection.versionName
and inspection.versionCode under the label used in ApkInspectSheet, making the
code value ambiguous; change this to render two separate rows: one InspectRow
for the version name (use stringResource for a version name label, e.g.,
Res.string.apk_inspect_version_name) that displays inspection.versionName or "—"
when null, and a second InspectRow for the version code (keep the existing
Res.string.apk_inspect_version_code) that displays
inspection.versionCode?.toString() or "—" when null; update any nearby layout
spacing/ordering so the two rows appear where the combined row was.
- Around line 300-305: The expand/collapse map stored in expanded is created
with remember { mutableStateMapOf(...) } so it survives across inspections;
change the remember key to include the current inspection identity (e.g.,
permissions or an inspectionId) so a new APK re-initializes the map. Concretely,
update the remember call that builds expanded (the mutableStateMapOf populated
via orderedLevels and defaultExpanded) to use remember(permissions) or
remember(inspectionId, permissions) so the map is recreated per inspection and
each APK uses its own defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6aae0b9-61f1-4bfe-affc-49c59e152445
📒 Files selected for processing (4)
core/presentation/src/commonMain/composeResources/values/strings.xmlfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ApkInspectSheet.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/Header.kt
✅ Files skipped from review due to trivial changes (2)
- core/presentation/src/commonMain/composeResources/values/strings.xml
- feature/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/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/Header.kt
Comment on lines
+206
to
+210
| InspectRow( | ||
| label = stringResource(Res.string.apk_inspect_version_code), | ||
| value = inspection.versionName?.let { name -> | ||
| inspection.versionCode?.let { code -> "$name ($code)" } ?: name | ||
| } ?: inspection.versionCode?.toString() ?: "—", |
Contributor
There was a problem hiding this comment.
Split the version name and version code display.
versionName and versionCode are separate fields, but this row merges them under one label. When both are present, the sheet no longer shows a clear version code value on its own, which makes build comparisons ambiguous.
🤖 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/ApkInspectSheet.kt`
around lines 206 - 210, The InspectRow currently combines inspection.versionName
and inspection.versionCode under the label used in ApkInspectSheet, making the
code value ambiguous; change this to render two separate rows: one InspectRow
for the version name (use stringResource for a version name label, e.g.,
Res.string.apk_inspect_version_name) that displays inspection.versionName or "—"
when null, and a second InspectRow for the version code (keep the existing
Res.string.apk_inspect_version_code) that displays
inspection.versionCode?.toString() or "—" when null; update any nearby layout
spacing/ordering so the two rows appear where the combined row was.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.kt`:
- Around line 131-140: In clearParkedInstall (PackageEventReceiver), don't treat
File.delete() as infallible and avoid clearing DB metadata before confirming
file removal: if parkedFilePath is non-null, first check
java.io.File(parkedFilePath).exists() and call delete(), inspect the boolean
result, log a warning if it returns false and skip/abort calling
repo.setPendingInstallFilePath(packageName = packageName, path = null); only
when delete() returns true (or file didn't exist) call
repo.setPendingInstallFilePath to clear the metadata, and preserve the existing
runCatching/onFailure logging around repo.setPendingInstallFilePath (or include
the caught exception) so partial cleanup outcomes are explicit.
In
`@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/use_cases/SyncInstalledAppsUseCase.kt`:
- Around line 124-133: The clears of parked-file metadata
(installedAppsRepository.setPendingInstallFilePath) are racy because they use a
local executeInTransaction wrapper and unconditionally set path=null; change
this to use the repository-backed transactional API (use the repository's real
transaction method instead of local executeInTransaction) and perform a
conditional/CAS-style update: read the current pending path inside that
repository transaction and only setPendingInstallFilePath(packageName, null) if
the current value still equals the expected stale value (the value you observed
earlier); apply the same pattern to the other clear block referenced (lines
around 138-152) so concurrent writes cannot be clobbered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 711ff01c-7a20-4ca0-b578-93ddffede015
📒 Files selected for processing (2)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/use_cases/SyncInstalledAppsUseCase.kt
Comment on lines
+131
to
+140
| runCatching { | ||
| repo.setPendingInstallFilePath(packageName = packageName, path = null) | ||
| }.onFailure { | ||
| Logger.w(it) { "Failed to clear parked install metadata for $packageName" } | ||
| } | ||
| if (parkedFilePath != null) { | ||
| runCatching { java.io.File(parkedFilePath).takeIf { it.exists() }?.delete() } | ||
| .onFailure { | ||
| Logger.w(it) { "Failed to delete parked APK at $parkedFilePath" } | ||
| } |
Contributor
There was a problem hiding this comment.
Handle partial cleanup outcomes explicitly in clearParkedInstall.
delete() can fail by returning false (no exception), so this path can silently skip cleanup. Also, deleting the file after a failed metadata clear can leave DB state pointing to a non-existent file.
Suggested patch
- runCatching {
- repo.setPendingInstallFilePath(packageName = packageName, path = null)
- }.onFailure {
+ val metadataCleared = runCatching {
+ repo.setPendingInstallFilePath(packageName = packageName, path = null)
+ }.onFailure {
Logger.w(it) { "Failed to clear parked install metadata for $packageName" }
- }
- if (parkedFilePath != null) {
- runCatching { java.io.File(parkedFilePath).takeIf { it.exists() }?.delete() }
+ }.isSuccess
+ if (metadataCleared && parkedFilePath != null) {
+ runCatching {
+ val file = java.io.File(parkedFilePath)
+ if (file.exists() && !file.delete()) {
+ Logger.w { "Failed to delete parked APK at $parkedFilePath" }
+ }
+ }
.onFailure {
Logger.w(it) { "Failed to delete parked APK at $parkedFilePath" }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.kt`
around lines 131 - 140, In clearParkedInstall (PackageEventReceiver), don't
treat File.delete() as infallible and avoid clearing DB metadata before
confirming file removal: if parkedFilePath is non-null, first check
java.io.File(parkedFilePath).exists() and call delete(), inspect the boolean
result, log a warning if it returns false and skip/abort calling
repo.setPendingInstallFilePath(packageName = packageName, path = null); only
when delete() returns true (or file didn't exist) call
repo.setPendingInstallFilePath to clear the metadata, and preserve the existing
runCatching/onFailure logging around repo.setPendingInstallFilePath (or include
the caught exception) so partial cleanup outcomes are explicit.
Comment on lines
+124
to
133
| // Resolution implies the system holds the | ||
| // package — drop the parked-file metadata | ||
| // so the apps row stops advertising an | ||
| // Install CTA on a file the user already | ||
| // installed. | ||
| installedAppsRepository.setPendingInstallFilePath( | ||
| packageName = app.packageName, | ||
| path = null, | ||
| ) | ||
| } catch (e: Exception) { |
Contributor
There was a problem hiding this comment.
These parked-metadata clears are vulnerable to lost updates under concurrency.
Both clears use stale snapshot data and run under a local executeInTransaction wrapper that does not provide real DB isolation. A concurrent writer can set a fresh parked path while this sweep clears it back to null, causing state regression.
Use a true repository-backed transaction and add a conditional/CAS-style clear (e.g., clear only if current row still matches expected pending state/path) to avoid clobbering newer writes.
Also applies to: 138-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/use_cases/SyncInstalledAppsUseCase.kt`
around lines 124 - 133, The clears of parked-file metadata
(installedAppsRepository.setPendingInstallFilePath) are racy because they use a
local executeInTransaction wrapper and unconditionally set path=null; change
this to use the repository-backed transactional API (use the repository's real
transaction method instead of local executeInTransaction) and perform a
conditional/CAS-style update: read the current pending path inside that
repository transaction and only setPendingInstallFilePath(packageName, null) if
the current value still equals the expected stale value (the value you observed
earlier); apply the same pattern to the other clear block referenced (lines
around 138-152) so concurrent writes cannot be clobbered.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/androidMain/kotlin/zed/rainxch/core/data/services/AndroidApkInspector.kt`:
- Around line 159-163: The current granted expression is a no-op; update
resolvePermission to accept an installed-package flag (e.g., add a parameter
like isInstalledPackage) and implement the intended logic there: if protection
== ProtectionLevel.NORMAL then return true when isInstalledPackage is true and
return null for file-only; otherwise preserve existing granted handling. Then
update the call site in buildInspection to pass the installation context
(isInstalledPackage) into resolvePermission so NORMAL permissions are
auto-granted for installed packages and remain null for file-only artifacts.
- Around line 62-88: The try/catch around pm.getPackageInfoCompat in
inspectInstalled is currently only catching PackageManager.NameNotFoundException
which lets other runtime/binder/security exceptions escape; change that call to
catch Throwable (or at least Exception) so any unexpected errors from
getPackageInfoCompat (e.g., SecurityException, DeadObjectException) are caught,
log the throwable with Logger.w(TAG) including the packageName and exception
details, and return null (same behavior as for NameNotFoundException) so the
coroutine doesn’t propagate unexpected PM exceptions; keep the rest of
inspectInstalled (sourceDir, sizeBytes, buildInspection) unchanged.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt`:
- Around line 1471-1479: The catch block in discardPendingInstall swallows
failures from installedAppsRepository.deleteInstalledApp and leaves the DB row
pointing to a deleted file; update the catch to mirror uninstallApp's behavior:
rethrow CancellationException, and for other Throwables log the error and
dispatch an AppsEvent.ShowError (including app.packageName and t.message) so the
UI shows feedback; locate the code in AppsViewModel around
discardPendingInstall/deleteInstalledApp and replace the current logger-only
catch with a call to the same event/emission mechanism used by uninstallApp
while preserving the existing log entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4352bdf1-53a6-4fd3-a9f5-dbb4098f9254
📒 Files selected for processing (12)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/di/PlatformModule.android.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidApkInspector.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/di/PlatformModule.jvm.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/apps/presentation/src/androidMain/kotlin/zed/rainxch/apps/presentation/components/InstalledAppIcon.android.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsAction.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsRoot.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsState.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsRoot.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ApkInspectSheet.kt
✅ Files skipped from review due to trivial changes (1)
- core/presentation/src/commonMain/composeResources/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsRoot.kt
- feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/ApkInspectSheet.kt
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.
Combines two related changes that share the apps-list / pending-install surface:
A — Faithful pending row (commit 1)
When the user taps Install but cancels the system installer prompt, the row used to land in apps with a GitHub Store logo and a broken "Open" button (snackbar of failure). Now:
SaveInstalledAppParamscarriespendingInstallFilePath.DetailsViewModel.saveInstalledAppToDatabaseplumbs the parked file path through whenever the install came back asDELEGATED_TO_SYSTEM, so the freshly-created DB row actually points at its parked APK.InstalledAppIcon(Android) gains anapkFilePathfallback. When the package isn't on the system yet it loads the icon out of the parked APK viaPackageManager.getPackageArchiveInfo(sourceDirpatched soloadIcon()returns the real icon, not the platform default).AppItemCardandCompactAppRowpass the parked path.pendingInstallFilePath != null→ primary "Install" button + a small Discard escape hatch.isPendingInstall && pendingInstallFilePath == null(legacy/edge) → "Discard" replaces the broken Open.OnDiscardPendingInstallaction: cancels orchestrator entry, deletes parked APK file, removes DB row.B — APK Inspect feature (commit 2)
A "peek inside before installing" bottom sheet that works for both parked APKs and installed packages.
core/domainaddsApkInspector+ApkInspection+ApkPermission+ProtectionLevel(NORMAL / DANGEROUS / SIGNATURE / PRIVILEGED / UNKNOWN).core/data/androidMainshipsAndroidApkInspectorreading fromPackageManagerwithGET_PERMISSIONS | GET_ACTIVITIES | GET_SERVICES | GET_RECEIVERS | GET_SIGNING_CERTIFICATES. Signing fingerprint extraction lifted into a sharedSigningFingerprinthelper. JVM ships a stub that returnsnull.ApkInspectSheet(Material 3ModalBottomSheet) renders identity, compatibility, permissions (color-coded by danger), components and file info. Dangerous permissions float to the top of the list.DetailsViewModelresolves the source automatically — installed manifest wins over a parked file when the row isn't pending; falls back the other way for pre-install inspections.InspectApkButtonnext toSmartInstallButtonon the details screen. The first time it appears for a user, it does a slow scale-pulse + tilt and pops a tooltip-style coachmark with arrow ("Peek inside before installing — permissions, signing, what apps can do"). Acknowledged on tap or when the inspect sheet opens, persisted viaTweaksRepository.apkInspectCoachmarkShown.Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Settings