Skip to content

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

Merged
rainxchzed merged 2 commits into
mainfrom
fix/orchestrator-delegated-outcome-v2
May 3, 2026
Merged

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

Conversation

@rainxchzed
Copy link
Copy Markdown
Member

@rainxchzed rainxchzed commented May 3, 2026

Replaces #488 (closed due to merge conflicts after #489 landed). Same root cause, fresh branch off latest main.

Bug

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

Fix

  • `OrchestratedDownload` carries the actual `installOutcome` produced by the orchestrator's own install attempt. `null` for entries whose install never went through `runInstall` (foreground-driven paths capture the outcome themselves).
  • `DefaultDownloadOrchestrator.runInstall` branches on the outcome: `COMPLETED` clears the parked file path / notifier and transitions to `Completed` (current behavior); `DELEGATED_TO_SYSTEM` keeps the file parked, propagates the outcome, and transitions to `Completed` so consumers know it's not a confirmed install.
  • `DetailsViewModel.OrchestratorStage.Completed` reads `entry.installOutcome ?: InstallOutcome.COMPLETED` and forwards it to `saveInstalledAppToDatabase` (which already maps `outcome != COMPLETED` → `isPendingInstall = true`). Skips the success-telemetry record and the orchestrator dismiss for delegated outcomes.

After this lands, 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.
  • Same flow but accept the system install prompt → after `PACKAGE_ADDED` fires, the row flips to installed.
  • Genuine Shizuku silent install (binder ready) → row appears as installed immediately, telemetry fires, orchestrator dismisses (unchanged).
  • Standard installer (no Shizuku selected) → unchanged: foreground VM still owns validation + DB save via the AwaitingInstall path.

Summary by CodeRabbit

Release Notes

  • New Features

    • Improved app installation handling with enhanced support for different installation methods.
  • Bug Fixes

    • Fixed download state management to prevent premature dismissal of completed operations.
    • Enhanced error handling and validation during app installation.
  • Documentation

    • Updated authentication and rate-limiting behavior documentation.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 3, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 991b982e-011c-457c-9612-16839d489857

📥 Commits

Reviewing files that changed from the base of the PR and between 71b618a and f5531d2.

📒 Files selected for processing (4)
  • CLAUDE.md
  • 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 pull request refines installation outcome handling by introducing an InstallOutcome property to the download orchestrator domain model, updating the default orchestrator to capture and branch on installation results (completed vs. delegated-to-system), and modifying the details view model to conditionally persist apps and manage UI state based on the outcome. It also updates backend token documentation for rate-limit keying and error semantics.

Changes

Installation Outcome Tracking & Persistence

Layer / File(s) Summary
Domain Model
core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/DownloadOrchestrator.kt
OrchestratedDownload adds nullable installOutcome: InstallOutcome? property to expose install result state.
Core Installation Logic
core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.kt
runInstall captures InstallOutcome from installer and branches: COMPLETED clears pending metadata and notifications; DELEGATED_TO_SYSTEM skips cleanup and logs delegation. Both set DownloadStage.Completed with the captured outcome.
Presentation & Persistence
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
OrchestratorStage.Completed handler derives isCompleted from outcome, conditionally writes install logs based on completion status and operation type, persists apps to database only when finalized, and gates orchestrator dismissal on isCompleted.

Backend Token & Request Semantics Documentation

Layer / File(s) Summary
API Documentation
CLAUDE.md
Updated X-GitHub-Token backend passthrough behavior: token-hash vs. IP bucketing for rate limits, cache-keying as `

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • GitHub-Store#245: Adds the initial CLAUDE.md documentation file that this PR refines and extends with additional backend semantics.
  • GitHub-Store#490: Also modifies CLAUDE.md (X-GitHub-Token passthrough docs) and DetailsViewModel, indicating overlapping concerns around token forwarding and completion handling.

Poem

🐰 Outcomes whisper through the download streams,
Delegated or done—the orchestrator dreams,
No early dismissals for half-finished tries,
Just conditional persists and truthful replies.

✨ 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-v2

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.

@rainxchzed rainxchzed merged commit 4b596cf into main May 3, 2026
1 check was pending
@rainxchzed rainxchzed deleted the fix/orchestrator-delegated-outcome-v2 branch May 3, 2026 06:50
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