E1: external-import domain interfaces and models#461
Merged
rainxchzed merged 47 commits intomainfrom Apr 28, 2026
Merged
Conversation
- Introduce `DesktopAppDataPaths` to manage OS-specific application data directories (macOS, Windows, Linux). - Migrate database and DataStore files from the system temporary directory to the new persistent app data location. - Ensure all SQLite sidecar files (`.db-shm`, `.db-wal`) are included in the migration to preserve WAL transactions. - Update `initDatabase` and `createDataStore` to utilize the new persistent storage paths. - Add `roadmap/` to `.gitignore`.
… github url to link
…, manifest hint, or fingerprint hit)
Contributor
|
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:
WalkthroughImplements a complete external-app import feature: platform scanners (Android/JVM), DB schema v15 and migration, new DAOs/entities, backend match APIs/DTOs, repository logic and telemetry, tweaks, UI flow/components (Compose), DI/viewmodel wiring, startup/worker changes, and supporting docs/config updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as UI/User
participant VM as ExternalImportViewModel
participant Repo as ExternalImportRepository
participant Scanner as ExternalAppScanner
participant PM as PackageManager
participant DB as AppDatabase
participant MatchAPI as ExternalMatchApi
User->>VM: Start import flow / user actions
VM->>Repo: scheduleInitialScanIfNeeded / runFullScan
Repo->>Scanner: snapshot()
Scanner->>PM: query installed packages & package info
PM-->>Scanner: package list & metadata
Scanner-->>Repo: ExternalAppCandidate list
Repo->>DB: read existing external_links & signing_fingerprints
DB-->>Repo: existing rows
Repo->>MatchAPI: postExternalMatch(batch)
MatchAPI-->>Repo: ExternalMatchResponse
Repo->>DB: upsert merged external_links / signing_fingerprints
DB-->>Repo: ack
Repo-->>VM: emit ScanResult + pendingCandidatesFlow updates
User->>VM: pick/link/skip/unlink
VM->>Repo: linkManually / skipPackage / unlink / rescanSinglePackage
Repo->>DB: persist changes
DB-->>Repo: ack
Repo-->>VM: updated flows/results
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Member
Author
CodeRabbit review: 7 applied, 4 pushed backCommit Applied (7)
Pushed back (4)
Build verification
Commit
|
![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: 3
♻️ Duplicate comments (1)
feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt (1)
106-121:⚠️ Potential issue | 🟠 MajorBanner dismissal can still race and re-show itself.
Both handlers hide the banner locally, but
observePendingExternalImports()can still recomputeshowImportProposalBanner = truebeforesetExternalImportBannerDismissedAtCount(current)finishes. TheisExternalImportInFlightguard does not help here because this file never flips it totrue, so the earlier race is still present and now affects both Review and Dismiss paths.You need a synchronous guard/local watermark that is applied before launching the async write, then cleared from a completion path after the flow is done.
Also applies to: 471-491
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt` around lines 106 - 121, observePendingExternalImports() can recompute showImportProposalBanner true before the async write in setExternalImportBannerDismissedAtCount(...) completes; add a synchronous local watermark flag on the ViewModel (e.g., private var localBannerDismissalWatermark: Long? or private var isLocalBannerDismissed: Boolean) that the Review/Dismiss handlers set immediately before launching the suspend write, use that flag in observePendingExternalImports()'s shouldShow computation to suppress the banner (in addition to isExternalImportInFlight), and clear/reset the watermark in the write completion/failure callbacks (and/or when the repository flow emits an updated dismissedAt) so the flow can return to normal; apply the same pattern for the duplicate handler region around the code at 471-491.
🤖 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/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.kt`:
- Around line 118-121: runDeltaScan currently deletes external-link rows
whenever a package has no current evidence, which can remove preserved decisions
(MATCHED/NEVER_ASK/SKIPPED); change the logic in runDeltaScan to first determine
whether the package truly no longer exists vs exists-but-has-no-evidence and
only hard-delete when the package is absent or the existing external-link state
is PENDING_REVIEW. Concretely, update the code around runDeltaScan and the calls
to externalLinkDao.prunePendingReviewNotIn / any delete calls so you: (1)
compute currentPackages (like livePackages in runFullScan), (2) for rows whose
package is absent perform hard delete, (3) for rows whose package exists but has
no evidence keep the row unless its state is PENDING_REVIEW (in which case you
may delete or move to appropriate state), and (4) mirror this same fix for the
similar block at the other occurrence (lines ~150-155). Ensure you reference and
adjust methods handling deletes (e.g., prunePendingReviewNotIn and whatever
delete/update methods used in runDeltaScan) so preserved decisions are not
removed on transient evidence misses.
In
`@feature/apps/presentation/src/androidMain/kotlin/zed/rainxch/apps/presentation/import/util/PackageVisibilityRequester.android.kt`:
- Around line 36-50: The requestOrOpenSettings() implementation is misleading
because QUERY_ALL_PACKAGES is not user-grantable and opening
Settings.ACTION_APPLICATION_DETAILS_SETTINGS cannot change package visibility;
remove or refactor requestOrOpenSettings() in
PackageVisibilityRequester.android.kt: either delete the method entirely and
rely on manifest-declared QUERY_ALL_PACKAGES with graceful degradation in
callers, or turn it into a clear no-op that immediately returns false and
documents that the user cannot change package visibility at runtime (update
callers of isGranted() accordingly to avoid expecting a runtime remediation
path).
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/components/PermissionRationaleScreen.kt`:
- Around line 80-90: The code currently calls
ExternalImportAction.OnPermissionGranted(sdkInt) immediately after
requester.requestOrOpenSettings(), which can mark the permission granted before
the user actually grants it; change the Button onClick handler (the scope.launch
block that calls onAction and uses requester.isGranted/requestOrOpenSettings) so
it only dispatches OnPermissionGranted(sdkInt) when requester.isGranted() truly
returns true (e.g., re-check on resume or after request completes), and instead
dispatch a new explicit action such as OnPermissionSettingsOpened or
OnPermissionDenied when the settings deep-link is launched or permission remains
false; remove the optimistic immediate OnPermissionGranted dispatch to ensure
the requester contract’s re-check is respected.
---
Duplicate comments:
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt`:
- Around line 106-121: observePendingExternalImports() can recompute
showImportProposalBanner true before the async write in
setExternalImportBannerDismissedAtCount(...) completes; add a synchronous local
watermark flag on the ViewModel (e.g., private var
localBannerDismissalWatermark: Long? or private var isLocalBannerDismissed:
Boolean) that the Review/Dismiss handlers set immediately before launching the
suspend write, use that flag in observePendingExternalImports()'s shouldShow
computation to suppress the banner (in addition to isExternalImportInFlight),
and clear/reset the watermark in the write completion/failure callbacks (and/or
when the repository flow emits an updated dismissedAt) so the flow can return to
normal; apply the same pattern for the duplicate handler region around the code
at 471-491.
🪄 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: 77a10f29-7016-4c16-a757-465cc4f170bc
📒 Files selected for processing (14)
composeApp/src/androidMain/AndroidManifest.xmlcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/external/InstallerSourceClassifier.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/external/SigningFingerprintComputer.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/dto/SigningFingerprintSeedResponse.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/local/db/initDatabase.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/ExternalImportRepository.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/ExternalDecisionSnapshot.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/ScanResult.ktfeature/apps/presentation/src/androidMain/kotlin/zed/rainxch/apps/presentation/import/util/PackageVisibilityRequester.android.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/components/PermissionRationaleScreen.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/util/PackageVisibilityRequester.ktfeature/apps/presentation/src/jvmMain/kotlin/zed/rainxch/apps/presentation/import/util/PackageVisibilityRequester.jvm.kt
✅ Files skipped from review due to trivial changes (3)
- feature/apps/presentation/src/jvmMain/kotlin/zed/rainxch/apps/presentation/import/util/PackageVisibilityRequester.jvm.kt
- composeApp/src/androidMain/AndroidManifest.xml
- core/data/src/commonMain/kotlin/zed/rainxch/core/data/dto/SigningFingerprintSeedResponse.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- core/data/src/jvmMain/kotlin/zed/rainxch/core/data/local/db/initDatabase.kt
- feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/util/PackageVisibilityRequester.kt
- core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/external/InstallerSourceClassifier.kt
- core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/ExternalImportRepository.kt
Member
Author
Round 2 — CodeRabbit review fixes + E1 closureCodeRabbit fixes (3)
E1 closure (4)
Build statusBoth What's left for E1 GAEngineering: none from the closure list. Tests (recommended, not blocking):
Backend (out of our scope): deploy i18n backfill: the 12 non-English locales pick up the 54 new keys from the existing translation pipeline. Manual TalkBack pass: flagged by the Accessibility Auditor as a pre-ship verification step. E1 is functionally feature-complete and merge-ready. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.kt (1)
109-113:⚠️ Potential issue | 🔴 CriticalThe new delta scan is unreachable for first-time installs.
getAppByPackage(packageName) ?: returnexitsonPackageInstalled()before thegetBackstopScope().launch { shouldRescan(...) }block runs. For previously-untracked installs, the import flow never sees the package, so the banner/wizard won't pick up new sideloaded apps at all.Possible fix
- val app = repo.getAppByPackage(packageName) ?: return - - if (app.isPendingInstall) { + val app = repo.getAppByPackage(packageName) + + if (app?.isPendingInstall == true) { val systemInfo = monitor.getInstalledPackageInfo(packageName) if (systemInfo != null) { val expectedVersionCode = app.latestVersionCode ?: 0L val wasActuallyUpdated = expectedVersionCode > 0L && systemInfo.versionCode >= expectedVersionCode @@ - } else { + } else if (app != null) { handleExternalInstall(packageName, app, repo, monitor) }Also applies to: 165-179
🤖 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 109 - 113, The onPackageInstalled handler exits early due to getAppByPackage(packageName) ?: return, which prevents the backstop rescan launched via getBackstopScope().launch { shouldRescan(...) } from running for first-time installs; change the control flow in onPackageInstalled (and the similar block around lines 165-179) so that the backstop rescan is invoked regardless of whether repo.getAppByPackage(...) returned an app — i.e., call getBackstopScope().launch { shouldRescan(packageName, monitor, repo, /*...*/) } before returning or in the null branch (use the packageName and monitor/getRepository() as needed), while keeping the early return only for any operations that truly require an existing App object.
🤖 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/presentation/src/commonMain/composeResources/values/strings.xml`:
- Around line 806-809: The privacy text in the string resource
external_import_permission_body understates the backend payload; update that
string so it accurately lists the additional optional fields sent to
/v1/external-match (signingFingerprint, installerKind, manifestHint) along with
package name and app label, and rephrase the sentence about what is sent (e.g.,
"The optional backend lookup sends only the package name and app label, and when
available the signingFingerprint, installerKind, and manifestHint — never a full
list of what's installed.") to ensure the permission rationale matches the
actual payload.
In
`@feature/apps/presentation/src/androidMain/kotlin/zed/rainxch/apps/presentation/import/util/PackageVisibilityRequester.android.kt`:
- Around line 25-30: The current isGranted() in PackageVisibilityRequester uses
a fragile count heuristic (visible.size >= GRANT_THRESHOLD); replace this with
the scanner's sentinel-based check or rename the API to reflect it's only a
heuristic. Concretely, modify PackageVisibilityRequester.isGranted() to attempt
to resolve or getPackageInfo for the same sentinel package name used by the
scanner (e.g., call packageManager.getPackageInfo(sentinelPackage, 0) or
queryIntentActivities for a sentinel Intent wrapped in runCatching and return
true if the sentinel is visible) so the result matches the scanner logic, or
alternatively rename the method (and its occurrences) to
isLikelyGranted/isHeuristicGranted and keep the existing implementation if you
prefer the heuristic approach.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.kt`:
- Around line 568-622: autoSummaryUndoAll currently deletes installed app rows
unconditionally; mirror the per-row undo logic by recording whether an
installed_apps row existed before auto-linking in autoMaterialize and consulting
that when undoing. In autoMaterialize (function autoMaterialize) capture
hadInstalledAppRowBefore per package (e.g., alongside
autoLinkedPackages/autoLinkedLabels or in a new autoLinkedHadInstalledRow map)
and persist it into state/pendingUndo; then in autoSummaryUndoAll only call
installedAppsRepository.deleteInstalledApp(pkg) when that recorded flag for pkg
is false (the same check used in undoLast: PendingUndo.Kind.Link &&
!hadInstalledAppRowBefore). Ensure the new field is cleared when you reset
autoLinkedPackages so the undo-all logic uses the recorded pre-link state.
- Around line 398-423: The skip flow currently logs failures from
externalImportRepository.skipPackage(...) but continues to
removeCardFromState(...), set pendingUndo and emit
ExternalImportEvent.ShowUndoSnackbar; change this so non-CancellationException
failures short-circuit: after catching Exception (non-Cancellation) send an
error event (e.g., ExternalImportEvent.ShowError with a clear message), do not
call removeCardFromState, do not set pendingUndo, do not send ShowUndoSnackbar,
and do not record telemetry.importSkipped; only proceed with
removeCardFromState, pendingUndo assignment, _events.send(ShowUndoSnackbar...)
and telemetry.importSkipped when skipPackage completes successfully.
- Around line 830-839: The when in InstallerKind.toUiLabel currently handles
only six enum values and uses an else branch that accidentally swallows
STORE_PLAY, STORE_AURORA, STORE_GALAXY, STORE_OEM_OTHER and SYSTEM; update the
function to remove the else branch and add explicit branches for STORE_PLAY,
STORE_AURORA, STORE_GALAXY, STORE_OEM_OTHER and SYSTEM (mapping each to the
appropriate string resource, e.g. Res.string.external_import_installer_play /
aurora / galaxy / oem_other / system or, if the intended behavior is to treat
them as unknown, explicitly map each of those five values to
getString(Res.string.external_import_installer_unknown)) so every InstallerKind
enum value is handled explicitly in toUiLabel.
In `@roadmap/E1_BACKEND_HANDOFF.md`:
- Line 79: The cache-key description is incorrect: currently stating that
excluding signingFingerprint forces fresh lookups is inverted; update the text
or the key so behavior matches intent — either change the cache key tuple to
include signingFingerprint (i.e., use (packageName, appLabel,
signingFingerprint) instead of (packageName, appLabel)) or rewrite the sentence
to say that excluding signingFingerprint will cause users with a different
fingerprint to reuse the same cached entry, not get a fresh lookup; reference
the cache key tuple `(packageName, appLabel)` and the `signingFingerprint`
symbol in your edit.
- Around line 68-71: The roadmap currently promises a 429 retry path but the
client does not implement WorkManager retries; update the text so it does not
instruct backends to rely on client retry behavior. Specifically, remove or
reword the `429 — rate limited; include Retry-After (client schedules
WorkManager retry)` line and instead state that 429 may be returned and that
clients should include Retry-After, but note that
ExternalImportRepositoryImpl.resolveMatches() only logs `external-match`
failures and does not schedule retries (or add instructions to implement retry
behavior in ExternalImportRepositoryImpl.resolveMatches() if backend must depend
on it). Ensure you reference ExternalImportRepositoryImpl.resolveMatches() in
the note so readers know the current client behavior.
---
Outside diff comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.kt`:
- Around line 109-113: The onPackageInstalled handler exits early due to
getAppByPackage(packageName) ?: return, which prevents the backstop rescan
launched via getBackstopScope().launch { shouldRescan(...) } from running for
first-time installs; change the control flow in onPackageInstalled (and the
similar block around lines 165-179) so that the backstop rescan is invoked
regardless of whether repo.getAppByPackage(...) returned an app — i.e., call
getBackstopScope().launch { shouldRescan(packageName, monitor, repo, /*...*/) }
before returning or in the null branch (use the packageName and
monitor/getRepository() as needed), while keeping the early return only for any
operations that truly require an existing App object.
🪄 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: 0303e299-a2d5-4b16-90be-4d4693b8cdc2
📒 Files selected for processing (13)
composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/app/GithubStoreApp.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateCheckWorker.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/apps/presentation/src/androidMain/kotlin/zed/rainxch/apps/presentation/import/util/PackageVisibilityRequester.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/import/ExternalImportViewModel.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/components/PermissionRationaleScreen.ktroadmap/E1_BACKEND_HANDOFF.md
✅ Files skipped from review due to trivial changes (1)
- feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsAction.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateCheckWorker.kt
- composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/app/GithubStoreApp.kt
- feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsRoot.kt
Comment on lines
+25
to
+30
| override suspend fun isGranted(): Boolean = | ||
| withContext(Dispatchers.IO) { | ||
| if (Build.VERSION.SDK_INT < Build.VERSION_CODES.R) return@withContext true | ||
| val pm = context.packageManager | ||
| val visible = runCatching { pm.getInstalledPackages(0) }.getOrElse { emptyList() } | ||
| visible.size >= GRANT_THRESHOLD |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On Android 11+, is using PackageManager.getInstalledPackages(0).size >= N a reliable way to determine whether an app has broad package visibility / QUERY_ALL_PACKAGES access, especially on devices with few installed apps?
💡 Result:
No, using PackageManager.getInstalledPackages(0).size >= N is not a reliable way to determine whether an app has broad package visibility or QUERY_ALL_PACKAGES access, especially on devices with few installed apps.
Citations:
- 1: https://developer.android.com/training/package-visibility/testing
- 2: https://developer.android.com/training/package-visibility
- 3: https://developer.android.com/reference/android/content/pm/PackageManager
- 4: https://commonsware.com/R/pages/chap-package-005
Replace package-count heuristic with actual permission check or rename the API.
Using PackageManager.getInstalledPackages(0).size >= GRANT_THRESHOLD to determine broad package visibility is unreliable. A device with QUERY_ALL_PACKAGES access but fewer than 30 installed apps will be incorrectly classified as denied. Either switch to a proper sentinel-based check (matching the scanner logic) or rename isGranted() to reflect that it's a heuristic, not a real permission check.
🤖 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/import/util/PackageVisibilityRequester.android.kt`
around lines 25 - 30, The current isGranted() in PackageVisibilityRequester uses
a fragile count heuristic (visible.size >= GRANT_THRESHOLD); replace this with
the scanner's sentinel-based check or rename the API to reflect it's only a
heuristic. Concretely, modify PackageVisibilityRequester.isGranted() to attempt
to resolve or getPackageInfo for the same sentinel package name used by the
scanner (e.g., call packageManager.getPackageInfo(sentinelPackage, 0) or
queryIntentActivities for a sentinel Intent wrapped in runCatching and return
true if the sentinel is visible) so the result matches the scanner logic, or
alternatively rename the method (and its occurrences) to
isLikelyGranted/isHeuristicGranted and keep the existing implementation if you
prefer the heuristic approach.
… skip, exhaustive installer label
…29 client behavior
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.kt (1)
690-727:⚠️ Potential issue | 🟠 MajorDon't clear the whole wizard when bulk skip partially fails.
This still has the old per-card skip failure mode in bulk form: each
skipPackage()error is only logged, but the flow clears all cards, emits confetti, and records telemetry forremaining.sizeanyway. Any package whose skip did not persist will silently come back on the next scan.Suggested direction
viewModelScope.launch { + val skippedPackages = mutableListOf<String>() remaining.forEach { card -> try { externalImportRepository.skipPackage(card.packageName, neverAsk = false) + skippedPackages += card.packageName } catch (e: CancellationException) { throw e } catch (e: Exception) { logger.error("Skip-remaining failed for ${card.packageName}: ${e.message}") } } + if (skippedPackages.isEmpty()) { + _events.send( + ExternalImportEvent.ShowError( + getString(Res.string.external_import_error_link_failed), + ), + ) + return@launch + } + runCatching { telemetry.importSkipped( - countBucket = bucketCount(remaining.size), + countBucket = bucketCount(skippedPackages.size), persisted = "7day", ) } pendingUndo = null _state.update { + val failed = it.cards.filter { card -> card.packageName !in skippedPackages } it.copy( - cards = persistentListOf(), - expandedPackages = persistentSetOf(), - activeSearchPackage = null, - searchQuery = "", - searchResults = persistentListOf(), - isSearching = false, - searchError = null, - phase = ImportPhase.Done, - skipped = it.skipped + remaining.size, - showCompletionToast = true, + cards = failed.toPersistentList(), + phase = if (failed.isEmpty()) ImportPhase.Done else ImportPhase.AwaitingReview, + skipped = it.skipped + skippedPackages.size, + showCompletionToast = failed.isEmpty(), ) } - _events.send(ExternalImportEvent.PlayConfetti) + if (_state.value.cards.isEmpty()) { + _events.send(ExternalImportEvent.PlayConfetti) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.kt` around lines 690 - 727, The bulk-skip currently logs per-package failures but proceeds to clear the entire wizard, emit confetti, and record telemetry using remaining.size even if some skipPackage calls failed; change the flow to collect which packages actually succeeded and which failed: inside viewModelScope.launch iterate remaining calling externalImportRepository.skipPackage, rethrow CancellationException, and for each success add to a successes list and for each failure add to a failures list (and log); after the loop, call telemetry.importSkipped with the count of successes (not remaining.size), update _state to remove only the successfully skipped cards and increment skipped by successes.size, keep pendingUndo unchanged (or set appropriately), and only send ExternalImportEvent.PlayConfetti and set phase=Done/showCompletionToast when failures.isEmpty(); if there are failures, preserve the failed cards in the wizard and surface an error/snackbar instead of clearing everything.
🧹 Nitpick comments (1)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.kt (1)
195-203: Replace raw link-state strings with typed constants/enums.Using
"MATCHED"/"NEVER_ASK"as literals makes scan gating brittle to typos or future renames. Prefer a shared state enum/constant source from the external-link model/DAO.🤖 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 195 - 203, The shouldRescan function currently compares link?.state to raw strings "MATCHED" and "NEVER_ASK"; replace those literals with the shared typed constants or enum values from the external-link model/DAO (e.g., ExternalLink.State.MATCHED / ExternalLinkState.NEVER_ASK or ExternalLink.STATE_MATCHED) by importing the model/enum used by getExternalLinkDao() and comparing link.state to those typed values; if such an enum/constant does not yet exist, add a single source-of-truth enum/companion object in the external link model and use it here (update shouldRescan and any other callers to use the enum/constants instead of raw strings).
🤖 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 310-311: The unlink call for external imports (runCatching {
getExternalImport().unlink(packageName) }) can fail and leave stale
external_links (MATCHED/NEVER_ASK) preventing future rescans; modify the failure
handling to retry the unlink once on the application coroutine scope: onFailure
log the error via Logger.w and then launch a single retry task (with a short
delay/backoff) that calls getExternalImport().unlink(packageName) again and logs
success or final failure, so shouldRescan() won’t be permanently suppressed by a
transient unlink error.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.kt`:
- Around line 590-621: autoSummaryUndoAll currently calls
externalImportRepository.snapshotDecision after the auto-link exists, so
restoreDecision just re-applies the linked state; change autoSummaryUndoAll to
capture and store each package's pre-link snapshot via
externalImportRepository.snapshotDecision before any auto-linking occurs (so
restoreDecision will revert to the true pre-link row), and then continue the
existing delete/restore/unlink rollback logic using those pre-link snapshots;
apply the same change to the analogous undo path referenced around the other
block (the similar logic at the 731-759 region), and ensure that when
autoLinkedHadInstalledRow is reset you also clear autoLinkedSnapshots so no
stale snapshots remain.
In `@roadmap/E1_BACKEND_HANDOFF.md`:
- Around line 68-71: Update the 503 fallback wording to say the client falls
back to local matching (e.g., signing-cert seed matching) rather than
"manifest-only"; edit the text around the bullet for `503 — partial outage` and
any duplicate mention (also at the later occurrence) so it references local
matching capability and how it interacts with
`BackendApiClient.postExternalMatch` and
`ExternalImportRepositoryImpl.resolveMatches` behavior, and ensure the "Cannot
defer" section is consistent with this change.
---
Duplicate comments:
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.kt`:
- Around line 690-727: The bulk-skip currently logs per-package failures but
proceeds to clear the entire wizard, emit confetti, and record telemetry using
remaining.size even if some skipPackage calls failed; change the flow to collect
which packages actually succeeded and which failed: inside viewModelScope.launch
iterate remaining calling externalImportRepository.skipPackage, rethrow
CancellationException, and for each success add to a successes list and for each
failure add to a failures list (and log); after the loop, call
telemetry.importSkipped with the count of successes (not remaining.size), update
_state to remove only the successfully skipped cards and increment skipped by
successes.size, keep pendingUndo unchanged (or set appropriately), and only send
ExternalImportEvent.PlayConfetti and set phase=Done/showCompletionToast when
failures.isEmpty(); if there are failures, preserve the failed cards in the
wizard and surface an error/snackbar instead of clearing everything.
---
Nitpick comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.kt`:
- Around line 195-203: The shouldRescan function currently compares link?.state
to raw strings "MATCHED" and "NEVER_ASK"; replace those literals with the shared
typed constants or enum values from the external-link model/DAO (e.g.,
ExternalLink.State.MATCHED / ExternalLinkState.NEVER_ASK or
ExternalLink.STATE_MATCHED) by importing the model/enum used by
getExternalLinkDao() and comparing link.state to those typed values; if such an
enum/constant does not yet exist, add a single source-of-truth enum/companion
object in the external link model and use it here (update shouldRescan and any
other callers to use the enum/constants instead of raw strings).
🪄 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: 60ee65d5-9b6b-4894-839c-1fa18037d974
📒 Files selected for processing (4)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.ktroadmap/E1_BACKEND_HANDOFF.md
✅ Files skipped from review due to trivial changes (1)
- core/presentation/src/commonMain/composeResources/values/strings.xml
Member
Author
Round 3 — CodeRabbit review fixes7 commits added on top of round 2. Both Android APK and Desktop JAR build clean. Applied (6)
Pushed back (1)
Build
Commit log (round 3) |
Member
Author
Round 4 — CodeRabbit review fixes3 commits added on top of round 3. Both targets build clean. Applied (5)
Build
Behavior changes worth knowing
Commits (round 4) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.kt (1)
403-405:⚠️ Potential issue | 🟠 MajorDon’t collapse snapshot-read failures into “no previous decision.”
getOrNull()makessnapshot == nullmean both “there was no prior row” and “snapshotDecision(...)threw”. In the failure case,undoLast()/autoSummaryUndoAll()will fall back tounlink(...)and can delete a pre-existingexternal_links` row instead of restoring it. Please fail the action/auto-link when the pre-link snapshot cannot be read, or carry an explicit capture-status flag so rollback never treats I/O failure as “first-ever decision.”Also applies to: 468-470, 772-774
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.kt` around lines 403 - 405, The code currently collapses snapshot read failures into a null "no previous decision" by using runCatching { externalImportRepository.snapshotDecision(packageName) }.getOrNull(); change this to capture the runCatching result explicitly (e.g., val snapshotResult = runCatching { externalImportRepository.snapshotDecision(packageName) }) and then branch: if snapshotResult.isFailure then fail/abort the action (or set an explicit error flag) so undoLast/autoSummaryUndoAll do not treat I/O errors as "no prior row", and only treat snapshotResult.getOrNull() as meaning "no row" when snapshotResult.isSuccess; apply the same fix to the other snapshotDecision usages in the file (the other runCatching(...).getOrNull() instances) and ensure undoLast, autoSummaryUndoAll and unlink logic check the failure path and do not perform destructive fallback on I/O failures.
🧹 Nitpick comments (1)
roadmap/E1_BACKEND_HANDOFF.md (1)
44-67: 🟡 Minor: Response example doesn’t include DTOcoverage(but DTO context indicates it exists).The response example (Lines 46-63) and surrounding text describe
matches[].candidates[]fields, but don’t mention thecoveragefield present inExternalMatchResponse.MatchEntry(from the provided DTO context snippet).If
coverageis actually emitted by the backend, the roadmap should document it (optional/nullable + semantics). If it’s not emitted, the DTO might be stale or should be clarified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roadmap/E1_BACKEND_HANDOFF.md` around lines 44 - 67, The example response and docs are missing the matches[].candidates[].coverage field that exists on the DTO ExternalMatchResponse.MatchEntry; either update the example JSON and field list to include "coverage" (document it as nullable/optional and describe its semantics and units/range) or remove/clarify the stale coverage property from the DTO; specifically edit the roadmap example (the Response (200) JSON and the bullets describing candidates fields) to add "coverage": null | number (and a short sentence about its meaning and whether it can be omitted), or mark ExternalMatchResponse.MatchEntry.coverage as deprecated/removed if backend does not emit it.
🤖 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 319-331: The retry-success path that only deletes the
external-import row can miss fast reinstall races; update the onSuccess handler
inside the getBackstopScope().launch retry so that after successful
getExternalImport().unlink(packageName) it also triggers an immediate recovery
delta scan (call the same runDeltaScan() logic or a package-scoped equivalent)
for packageName on the appropriate coroutine scope; this ensures a reinstall
that happened during the 1s backoff is detected rather than waiting for the
periodic sweep.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.kt`:
- Around line 520-535: The undo rollback currently swallows failures (in the
runCatching blocks around installedAppsRepository.deleteInstalledApp,
externalImportRepository.restoreDecision, and externalImportRepository.unlink),
allowing UI state and undo metadata to be cleared even when persistence fails;
change these to fail-fast: remove silent runCatching usage or check its result
and if an exception occurred, throw or return early to abort the undo flow so
autoSummaryUndoAll() and subsequent UI updates do not run and the existing undo
metadata is preserved, ensuring callers of the block see the error and can
retry; apply the same change to the analogous code paths (including the block
referenced at lines 616-635) so all repository rollback failures prevent local
state changes.
In `@roadmap/E1_BACKEND_HANDOFF.md`:
- Around line 26-43: Add a one-line clarification to the request contract
explaining the semantics of candidates[].manifestHint and its owner/repo fields:
state whether the manifestHint object may be omitted entirely (versus present
with nulls), confirm whether omitted and present-but-null are treated
identically by the backend, and specify that validation is applied only to
non-null fields (e.g., if owner is null and repo is non-null, only repo is
validated against `^[\w.-]{1,100}$`). Reference the symbols
`candidates[].manifestHint`, `candidates[].manifestHint.owner`, and
`candidates[].manifestHint.repo` when adding this sentence.
- Around line 73-78: The roadmap omits whether server-side scoring allows
negative confidence after applying the −0.20 no‑APK penalty; update the document
to explicitly state the backend's behavior for final confidence values (either
allow negatives as a valid low score or clamp post-calculation), reference the
scoring rules involving manifestHint.owner/repo HEAD validation,
signingFingerprint → signing_fingerprint lookup, and the search-only cap at
0.85, and specify the numeric clamp range the backend will enforce (e.g., clamp
to [0,1] or [0,∞) with explicit justification) so it aligns with client-side
display clamping (.coerceIn(0,100)) and tier logic (>=0.85/>=0.5).
---
Duplicate comments:
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.kt`:
- Around line 403-405: The code currently collapses snapshot read failures into
a null "no previous decision" by using runCatching {
externalImportRepository.snapshotDecision(packageName) }.getOrNull(); change
this to capture the runCatching result explicitly (e.g., val snapshotResult =
runCatching { externalImportRepository.snapshotDecision(packageName) }) and then
branch: if snapshotResult.isFailure then fail/abort the action (or set an
explicit error flag) so undoLast/autoSummaryUndoAll do not treat I/O errors as
"no prior row", and only treat snapshotResult.getOrNull() as meaning "no row"
when snapshotResult.isSuccess; apply the same fix to the other snapshotDecision
usages in the file (the other runCatching(...).getOrNull() instances) and ensure
undoLast, autoSummaryUndoAll and unlink logic check the failure path and do not
perform destructive fallback on I/O failures.
---
Nitpick comments:
In `@roadmap/E1_BACKEND_HANDOFF.md`:
- Around line 44-67: The example response and docs are missing the
matches[].candidates[].coverage field that exists on the DTO
ExternalMatchResponse.MatchEntry; either update the example JSON and field list
to include "coverage" (document it as nullable/optional and describe its
semantics and units/range) or remove/clarify the stale coverage property from
the DTO; specifically edit the roadmap example (the Response (200) JSON and the
bullets describing candidates fields) to add "coverage": null | number (and a
short sentence about its meaning and whether it can be omitted), or mark
ExternalMatchResponse.MatchEntry.coverage as deprecated/removed if backend does
not emit it.
🪄 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: 0ea53297-0800-4f2d-9f6b-c5da2828c369
📒 Files selected for processing (3)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.ktroadmap/E1_BACKEND_HANDOFF.md
…etadata, surfaces error)
Member
Author
Round 5 — CodeRabbit review fixes3 commits added on top of round 4. Both targets build clean. Applied (5)
Pushed back (1)
Behavior changes worth knowing
Build
Commits (round 5) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit