Skip to content

fix: don't mark app installed when Shizuku falls back to system installer#488

Closed
rainxchzed wants to merge 2 commits into
mainfrom
fix/orchestrator-delegated-outcome
Closed

fix: don't mark app installed when Shizuku falls back to system installer#488
rainxchzed wants to merge 2 commits into
mainfrom
fix/orchestrator-delegated-outcome

Conversation

@rainxchzed
Copy link
Copy Markdown
Member

@rainxchzed rainxchzed commented May 2, 2026

Summary

When the user has Shizuku/Sui selected as their installer but the binder isn't actually ready (binder died, Sui not running, permission revoked, etc.), ShizukuInstallerWrapper silently falls back to the standard Android installer and reports InstallOutcome.DELEGATED_TO_SYSTEM. The orchestrator's runInstall was discarding that return value and unconditionally moving the entry to DownloadStage.Completed, and DetailsViewModel's Completed handler hard-coded installOutcome = COMPLETED when persisting the DB row. Result: the app would be marked as installed the moment the system install prompt appeared — even if the user immediately tapped Cancel.

Fix

  • OrchestratedDownload now carries the actual installOutcome produced by the orchestrator's own install attempt. null for entries that never went through runInstall (foreground-driven install paths capture the outcome themselves).
  • DefaultDownloadOrchestrator.runInstall now branches on the outcome:
    • COMPLETED → clear pending file path, clear notifier, transition to Completed (current behavior).
    • DELEGATED_TO_SYSTEM → keep the parked file path and notifier intact, transition to Completed carrying the delegated outcome so consumers know it's not a confirmed install.
  • DetailsViewModel.OrchestratorStage.Completed reads the entry's outcome and forwards it to saveInstalledAppToDatabase (which already maps outcome != COMPLETEDisPendingInstall = true). It also avoids firing recordInstallSucceeded telemetry, the "Installed" log line, and the orchestrator dismiss for delegated-only outcomes — those are reserved for confirmed installs.

After the fix, a Shizuku-fallback install that the user cancels at the system prompt leaves the row with isPendingInstall = true and a parked file. PackageEventReceiver flips it to false only when PACKAGE_ADDED actually fires; if the user never accepts, the 24h stale-pending sweep in SyncInstalledAppsUseCase cleans it up.

Test plan

  • Set installer type to Shizuku, then disable/kill the Shizuku/Sui service so the wrapper falls back to the standard installer. Tap Install on a repo, then tap Cancel on the system install prompt → row must NOT appear as installed; the apps list shows it as pending / ready-to-install (not as a successfully installed app).
  • Same flow but accept the system install prompt → after PACKAGE_ADDED fires, the row flips to installed (verified by Apps section showing it under "Up to date").
  • Genuine Shizuku silent install (binder ready) → row appears as installed immediately, telemetry fires, orchestrator dismisses (unchanged behavior).
  • Standard installer (no Shizuku selected) → unchanged: foreground VM still owns validation + DB save via the AwaitingInstall path.

Summary by CodeRabbit

  • New Features

    • Installation outcomes are now tracked and reported with greater accuracy.
    • The app distinguishes between direct installations and system-delegated installations, improving transparency.
  • Bug Fixes

    • Download entries now persist correctly when installations are delegated to the system.
    • Installation validation and status displays now reflect actual installation completion.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 2, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 75e410e7-d736-4ca5-b9c2-4a4bc5e1a87d

📥 Commits

Reviewing files that changed from the base of the PR and between 25a848f and 7ac1988.

📒 Files selected for processing (3)
  • core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.kt
  • core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/DownloadOrchestrator.kt
  • feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt

Walkthrough

The PR refactors install outcome handling across three layers. The orchestrator service now branches on the installer's returned InstallOutcome, capturing COMPLETED versus DELEGATED_TO_SYSTEM paths. The domain model adds an installOutcome field to expose this result. The presentation layer consumes it to conditionally persist data and defer dismissal for delegated installs.

Changes

Install Outcome Propagation & Conditional Handling

Layer / File(s) Summary
Domain Contract
core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/DownloadOrchestrator.kt
OrchestratedDownload gains nullable installOutcome: InstallOutcome? field with KDoc clarifying when it is populated (orchestrator-initiated install only) and how consumers should interpret DownloadStage.Completed based on this value.
Orchestrator Service Logic
core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.kt
runInstall branches on installer.install() result: InstallOutcome.COMPLETED clears pending state and notifications, while InstallOutcome.DELEGATED_TO_SYSTEM skips that cleanup to preserve pending metadata for downstream handling. Both paths record the outcome in the orchestrator entry.
Presentation & Persistence
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
DetailsViewModel reads installOutcome from completed orchestrator entries, computes whether install was confirmed, logs conditionally, persists to Android DB with explicit validation and error handling, and defers orchestrator dismissal only when the install was confirmed (delegated outcomes remain visible).

Sequence Diagram

sequenceDiagram
    actor User
    participant DetailsVM as DetailsViewModel
    participant Orchestrator as DefaultDownloadOrchestrator
    participant Installer as Installer
    participant AppDB as InstalledAppsRepository
    
    User->>DetailsVM: Observes completion
    DetailsVM->>Orchestrator: Polls orchestrator entry
    activate Orchestrator
    Orchestrator->>Installer: installer.install(filePath, ext)
    Installer-->>Orchestrator: InstallOutcome.COMPLETED
    Orchestrator->>AppDB: setPendingInstallFilePath(null)
    AppDB-->>Orchestrator: ✓
    Orchestrator-->>DetailsVM: OrchestratedDownload{installOutcome=COMPLETED}
    deactivate Orchestrator
    
    DetailsVM->>DetailsVM: resolvedOutcome=COMPLETED<br/>isCompleted=true
    DetailsVM->>AppDB: Persist installed app
    AppDB-->>DetailsVM: ✓
    DetailsVM->>Orchestrator: dismiss(packageKey)
    
    par Alternative Path: Delegated Install
        Orchestrator->>Installer: installer.install(...)
        Installer-->>Orchestrator: InstallOutcome.DELEGATED_TO_SYSTEM
        Orchestrator-->>DetailsVM: OrchestratedDownload{installOutcome=DELEGATED_TO_SYSTEM}
        DetailsVM->>DetailsVM: resolvedOutcome=DELEGATED<br/>isCompleted=false
        DetailsVM->>AppDB: Persist with delegated outcome
        Note over DetailsVM: Skip dismiss() — keep entry visible
    end
Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A rabbit hops through install paths twain—
One swift, one deferred by system's reign.
The outcome blooms in orchestral hand,
Now ViewModel knows where each app will land! 🌱✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main problem being fixed: preventing apps from being marked as installed when the system installer is used as a fallback.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/orchestrator-delegated-outcome

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.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant