Merge pull request #253 from rainxchzed/feat-shizuku-installer#317
Open
rainxchzed wants to merge 7 commits intomainfrom
Open
Merge pull request #253 from rainxchzed/feat-shizuku-installer#317rainxchzed wants to merge 7 commits intomainfrom
rainxchzed wants to merge 7 commits intomainfrom
Conversation
feat(android): Add Shizuku-based silent installation support - Implement `ShizukuInstallerServiceImpl` using Shizuku UserService for privileged package operations (install/uninstall). - Create `ShizukuServiceManager` to manage Shizuku lifecycle, status detection, and permission requests. - Add `ShizukuInstallerWrapper` to delegate installation tasks to Shizuku when enabled and available, falling back to the standard installer on failure. - Update `Profile` screen with a new "Installation" section to allow users to toggle between Default and Shizuku installer types. - Add `InstallerStatusProvider` and `InstallerType` to core domain to handle platform-agnostic installer state. - Update Android manifest and Gradle dependencies to include Shizuku API and HIDL stubs. - Persist installer type preference in `ThemesRepository`.
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:
WalkthroughAdds optional Shizuku-based silent install/uninstall: AIDL interface, privileged Shizuku user service, ShizukuServiceManager and wrapper that fall back to AndroidInstaller, installer-type persistence + UI, DI/platform bindings, Android provider entry, and new library deps. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Profile UI
participant VM as ProfileViewModel
participant Themes as ThemesRepository
participant StatusProv as InstallerStatusProvider
participant Manager as ShizukuServiceManager
participant Wrapper as ShizukuInstallerWrapper
participant AndroidInst as AndroidInstaller
rect rgba(200,150,255,0.5)
User->>UI: open profile
UI->>VM: load state
VM->>Themes: getInstallerType()
VM->>StatusProv: subscribe shizukuAvailability
StatusProv->>Manager: observe/refresh
Manager->>StatusProv: emit availability
VM->>UI: render
end
rect rgba(100,200,100,0.5)
User->>UI: start install
UI->>Wrapper: install(file)
Wrapper->>Manager: getService()
alt Shizuku READY
Manager->>Wrapper: IShizukuInstallerService
Wrapper->>Manager: installPackage(pfd, size)
Manager->>Wrapper: install result
Wrapper->>UI: return result
else fallback
Wrapper->>AndroidInst: install(file)
AndroidInst->>Wrapper: result
Wrapper->>UI: return result
end
end
rect rgba(255,150,100,0.5)
User->>UI: request Shizuku permission
UI->>VM: OnRequestShizukuPermission
VM->>StatusProv: requestShizukuPermission()
StatusProv->>Manager: requestPermission()
Manager->>User: prompt grant
Manager->>StatusProv: update status
StatusProv->>VM: emit updated availability
VM->>UI: update badge
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 9
🧹 Nitpick comments (2)
core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/InstallerStatusProvider.kt (1)
6-8: Keep the common contract installer-centric, not Shizuku-centric.This lives in
commonMain, butshizukuAvailabilityandrequestShizukuPermission()hard-code an Android-only integration into the shared domain API. That is why the JVM implementation has to expose a permanentUNAVAILABLEstate and a no-op permission call. If the goal is a platform-agnostic installer status contract, model generic installer capability/status here and keep the Shizuku-specific mapping inandroidMain.🤖 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/system/InstallerStatusProvider.kt` around lines 6 - 8, The interface InstallerStatusProvider currently exposes Android-only members shizukuAvailability and requestShizukuPermission; change the common contract to be generic (e.g., replace shizukuAvailability: StateFlow<ShizukuAvailability> with a platform-agnostic installerAvailability: StateFlow<InstallerAvailability> and rename requestShizukuPermission() to requestInstallerPermission() or similar) and keep the Android-specific Shizuku types and mapping inside the androidMain implementation where you translate ShizukuAvailability -> InstallerAvailability and implement requestInstallerPermission() via Shizuku; update JVM/no-op implementations to return a stable InstallerAvailability.UNAVAILABLE and a no-op requestInstallerPermission().core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ThemesRepositoryImpl.kt (1)
92-102: Avoid persisting raw enum names for a long-lived preference.
type.nameplusfromName(...)ties the stored value to the enum constant names. A later rename ofSHIZUKUwill silently reset existing users toDEFAULT. Prefer an explicit persisted value onInstallerTypeand read/write that instead.Possible refactor
override fun getInstallerType(): Flow<InstallerType> { return preferences.data.map { prefs -> - val name = prefs[INSTALLER_TYPE_KEY] - InstallerType.fromName(name) + val value = prefs[INSTALLER_TYPE_KEY] + InstallerType.fromPersistedValue(value) } } override suspend fun setInstallerType(type: InstallerType) { preferences.edit { prefs -> - prefs[INSTALLER_TYPE_KEY] = type.name + prefs[INSTALLER_TYPE_KEY] = type.persistedValue } }enum class InstallerType(val persistedValue: String) { DEFAULT("default"), SHIZUKU("shizuku"); companion object { fun fromPersistedValue(value: String?): InstallerType = entries.firstOrNull { it.persistedValue == value } ?: DEFAULT } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ThemesRepositoryImpl.kt` around lines 92 - 102, Currently getInstallerType() and setInstallerType(...) persist the enum's name (type.name) and reconstruct via InstallerType.fromName(...), which is fragile; change InstallerType to expose an explicit persistedValue (e.g., val persistedValue: String) and a companion lookup like fromPersistedValue(value: String?): InstallerType, then update setInstallerType to store type.persistedValue into INSTALLER_TYPE_KEY and update getInstallerType to call InstallerType.fromPersistedValue(name) instead of InstallerType.fromName(...), keeping references to preferences, INSTALLER_TYPE_KEY, getInstallerType, setInstallerType, and InstallerType to locate the changes.
🤖 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/aidl/zed/rainxch/core/data/services/shizuku/IShizukuInstallerService.aidl`:
- Line 4: The AIDL contract currently takes a filesystem path
(IShizukuInstallerService.installPackage(String apkPath)) which forces the
Shizuku service to open the app-private APK and fails due to permission; update
the AIDL to accept a ParcelFileDescriptor instead and change the caller to open
the APK with ParcelFileDescriptor.open(...) and pass that descriptor; then
modify ShizukuInstallerServiceImpl.installPackage(...) to read from the incoming
ParcelFileDescriptor (rather than calling FileInputStream(file) at the failing
spot) and close the descriptor when done; ensure method signatures and any
bindings/stubs are updated to use ParcelFileDescriptor so the caller-side opens
the file descriptor in the app context and the service reads from it.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt`:
- Around line 67-68: The code passes a Linux UID/hard-coded 0 where the Android
framework expects a user/profile id; update the calls that use Process.myUid()
and the hard-coded 0 so they pass the framework user id instead. Locate
installer.createSession (where sessionId is set) and the uninstall(...) calls
and replace Process.myUid()/0 with a derived user id obtained via
UserHandle.getUserId(Process.myUid()) (or equivalent method that returns the
Android user/profile id for the current process) so both createSession and
uninstall target the correct Android user/profile.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.kt`:
- Around line 131-149: The uninstall method currently reads
shizukuServiceManager.installerService directly (in uninstall(packageName))
which can be null in a fresh process; instead call
shizukuServiceManager.getService() (or the method that establishes/binds the
user service) and await/verify it before using installerService so the Shizuku
service is actually bound when isShizukuEnabled() &&
shizukuServiceManager.status.value == ShizukuStatus.READY; update the logic in
uninstall(packageName) to invoke getService(), check the returned/non-null
service (or installerService after binding), then proceed with
service.uninstallPackage(...) and only fall back to
androidInstaller.uninstall(packageName) if binding or uninstall fails.
- Around line 95-128: The install logic in ShizukuInstallerWrapper currently
rethrows on non-zero Shizuku result and may skip calling
androidInstaller.ensurePermissionsOrThrow when falling back; change
install(filePath: String, extOrMime: String) so any Shizuku failure (service
null, non-zero result, or exception) simply logs and falls through to the
standard installer instead of throwing, and before calling
androidInstaller.install ensure you call
androidInstaller.ensurePermissionsOrThrow(extOrMime) so the fallback has the
required permission check; adjust the try/catch to not rethrow
IllegalStateException for install failures (you can still rethrow only true
fatal errors if needed) and remove the throw IllegalStateException("Shizuku
install failed (code: $result)") so fallback always runs.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.kt`:
- Around line 145-153: The cached-installer check in ShizukuServiceManager uses
installerService?.asBinder().pingBinder() but ignores the boolean result; update
the logic in the getter/initializer where installerService is checked so you
call service.asBinder().pingBinder(), and only return installerService when that
call returns true; if it returns false or throws, clear installerService
(installerService = null) and continue with rebind logic so callers don't
receive a dead binder from installerService.
- Around line 83-90: The destroy/unbind path currently only clears local
references and never calls Shizuku.unbindUserService, leaving the privileged
process bound; fix this by storing the UserServiceArgs returned/used when
binding as a member property (e.g. a nullable var userServiceArgs:
UserServiceArgs?) when the bind succeeds, and update unbindService() to call
Shizuku.unbindUserService(userServiceArgs, connection) if userServiceArgs is
non-null (then clear it and the connection), and ensure destroy() still removes
listeners (binderReceivedListener, binderDeadListener, permissionResultListener)
and then calls unbindService() so the actual Shizuku.unbindUserService call runs
during app teardown.
In
`@feature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/components/sections/Installation.kt`:
- Around line 85-138: The InstallerOption radio-group pattern currently uses
clickable semantics which hides selection from screen readers; wrap the parent
Column with Modifier.selectableGroup() and replace the per-option
Modifier.clickable usage inside InstallerOption invocations with
Modifier.selectable(selected = selectedType == InstallerType.DEFAULT, onClick =
{ onTypeSelected(InstallerType.DEFAULT) }, role = Role.RadioButton) (and
similarly for InstallerType.SHIZUKU) so the selection state is exposed; update
both occurrences where InstallerOption is used (the block with selectedType and
onTypeSelected and the other occurrence around the lines referenced in the
review) and ensure imports for androidx.compose.ui.semantics.Role and
Modifier.selectable/selectableGroup are added.
In
`@feature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/ProfileRoot.kt`:
- Around line 203-210: The Spacer after installationSection is rendered
unconditionally causing a 32.dp gap on non-Android builds; modify the call site
so the spacer is only emitted when the installation section is actually shown by
either moving the Spacer(Modifier.height(32.dp)) into the installationSection
implementation or wrapping the Spacer with the same platform check used by
installationSection (i.e., the same early-return/Android guard) so that Spacer
is skipped on non-Android targets.
---
Nitpick comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ThemesRepositoryImpl.kt`:
- Around line 92-102: Currently getInstallerType() and setInstallerType(...)
persist the enum's name (type.name) and reconstruct via
InstallerType.fromName(...), which is fragile; change InstallerType to expose an
explicit persistedValue (e.g., val persistedValue: String) and a companion
lookup like fromPersistedValue(value: String?): InstallerType, then update
setInstallerType to store type.persistedValue into INSTALLER_TYPE_KEY and update
getInstallerType to call InstallerType.fromPersistedValue(name) instead of
InstallerType.fromName(...), keeping references to preferences,
INSTALLER_TYPE_KEY, getInstallerType, setInstallerType, and InstallerType to
locate the changes.
In
`@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/InstallerStatusProvider.kt`:
- Around line 6-8: The interface InstallerStatusProvider currently exposes
Android-only members shizukuAvailability and requestShizukuPermission; change
the common contract to be generic (e.g., replace shizukuAvailability:
StateFlow<ShizukuAvailability> with a platform-agnostic installerAvailability:
StateFlow<InstallerAvailability> and rename requestShizukuPermission() to
requestInstallerPermission() or similar) and keep the Android-specific Shizuku
types and mapping inside the androidMain implementation where you translate
ShizukuAvailability -> InstallerAvailability and implement
requestInstallerPermission() via Shizuku; update JVM/no-op implementations to
return a stable InstallerAvailability.UNAVAILABLE and a no-op
requestInstallerPermission().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fdaf632-58c2-45b5-b3fe-d4f8120627ec
📒 Files selected for processing (22)
composeApp/src/androidMain/AndroidManifest.xmlcore/data/build.gradle.ktscore/data/src/androidMain/aidl/zed/rainxch/core/data/services/shizuku/IShizukuInstallerService.aidlcore/data/src/androidMain/kotlin/zed/rainxch/core/data/di/PlatformModule.android.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/AndroidInstallerStatusProvider.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ThemesRepositoryImpl.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/di/PlatformModule.jvm.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopInstallerStatusProvider.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/InstallerType.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/ShizukuAvailability.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/ThemesRepository.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/InstallerStatusProvider.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/ProfileAction.ktfeature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/ProfileRoot.ktfeature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/ProfileState.ktfeature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/ProfileViewModel.ktfeature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/components/sections/Installation.ktgradle/libs.versions.toml
...ta/src/androidMain/aidl/zed/rainxch/core/data/services/shizuku/IShizukuInstallerService.aidl
Outdated
Show resolved
Hide resolved
Comment on lines
+20
to
+31
| override val shizukuAvailability: StateFlow<ShizukuAvailability> = | ||
| shizukuServiceManager.status.map { status -> | ||
| when (status) { | ||
| ShizukuStatus.NOT_INSTALLED -> ShizukuAvailability.UNAVAILABLE | ||
| ShizukuStatus.NOT_RUNNING -> ShizukuAvailability.NOT_RUNNING | ||
| ShizukuStatus.PERMISSION_NEEDED -> ShizukuAvailability.PERMISSION_NEEDED | ||
| ShizukuStatus.READY -> ShizukuAvailability.READY | ||
| } | ||
| }.stateIn( | ||
| scope = scope, | ||
| started = SharingStarted.Eagerly, | ||
| initialValue = ShizukuAvailability.UNAVAILABLE |
Contributor
There was a problem hiding this comment.
Preserve the NOT_INSTALLED state instead of collapsing it to UNAVAILABLE.
Line 23 drops a user-actionable distinction the new UI needs. With this mapping, the profile flow cannot tell “Shizuku is not installed” apart from generic unavailability, so the install-specific status/hint strings added in this PR become unreachable. Please expose a dedicated availability for that case, or pass the raw ShizukuStatus through to the UI layer.
Comment on lines
+67
to
+68
| val installerPackageName = "com.android.shell" | ||
| val sessionId = installer.createSession(params, installerPackageName, null, android.os.Process.myUid()) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What does the userId parameter of Android IPackageInstaller.createSession and uninstall represent, and should it use a UserHandle-derived userId instead of Process.myUid() or a hard-coded 0?
💡 Result:
userId in the hidden Binder APIs IPackageInstaller.createSession(..., int userId) and IPackageInstaller.uninstall(..., int userId) is the Android framework “user/profile id” (e.g., owner user 0, secondary users, work profile), i.e. the target user whose package state the operation applies to, not a Linux/Android app UID. This is evident from the AIDL signatures and from PackageInstallerService, which stores session.userId and passes userId through to PackageManagerService.deletePackage(..., userId, ...) for uninstall. [1][2][3]
Because the service enforces cross-user access (enforceCrossUserPermission(callingUid, userId, ...)), you generally must pass the correct target UserHandle-derived userId (usually “current process user”, unless you explicitly intend another profile/user and hold cross-user privileges). [2][3]
So:
- Do not pass
Process.myUid()directly (that’s a UID, not a userId). - Do not hard-code
0unless you specifically mean the owner/system user; it will be wrong on secondary users/work profiles and may fail permission checks. [2][4] - If all you have is a UID, convert it to a userId via
UserHandle.getUserId(uid); if you have aUserHandle, useuserHandle.getIdentifier()(or equivalent). [4]
Sources:
[1] IPackageInstaller.aidl signatures (createSession/uninstall include int userId). (android.googlesource.com)
[2] PackageInstallerService#createSessionInternal(...) enforces cross-user permission for userId. (android.googlesource.com)
[3] PackageInstallerService#uninstall(...) enforces cross-user permission and calls deletePackage(..., userId, ...). (android.googlesource.com)
[4] Shell command handling shows --user is a userId and is translated/validated as an incoming user. (android.googlesource.com)
Citations:
- 1: https://android.googlesource.com/platform/frameworks/base/%2B/d6ee0ba/core/java/android/content/pm/IPackageInstaller.aidl?utm_source=openai
- 2: https://android.googlesource.com/platform/frameworks/base/%2B/5064fddf81e477a90c81d72f7328aa5887c67511/services/core/java/com/android/server/pm/PackageInstallerService.java
- 3: https://android.googlesource.com/platform/frameworks/base/%2B/5064fddf81e477a90c81d72f7328aa5887c67511/services/core/java/com/android/server/pm/PackageInstallerService.java
- 4: https://android.googlesource.com/platform/frameworks/base/%2B/990fb6b/services/core/java/com/android/server/pm/PackageManagerShellCommand.java
🏁 Script executed:
cat -n core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt | sed -n '60,75p'Repository: OpenHub-Store/GitHub-Store
Length of output: 818
🏁 Script executed:
cat -n core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt | sed -n '150,170p'Repository: OpenHub-Store/GitHub-Store
Length of output: 937
Pass the correct Android user/profile ID to both createSession and uninstall calls instead of Process.myUid() or hard-coded 0.
The userId parameter in IPackageInstaller.createSession(...) and uninstall(...) is the Android framework user/profile ID (owner user, secondary user, work profile, etc.), not a Linux UID or a fixed value. Currently, line 68 passes Process.myUid() (a UID) and lines 157–164 hard-code 0, both of which are incorrect. This will target the wrong profile or fail permission checks on multi-user devices. Use UserHandle.getUserId(uid) or equivalent to derive the correct user ID.
🤖 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/shizuku/ShizukuInstallerServiceImpl.kt`
around lines 67 - 68, The code passes a Linux UID/hard-coded 0 where the Android
framework expects a user/profile id; update the calls that use Process.myUid()
and the hard-coded 0 so they pass the framework user id instead. Locate
installer.createSession (where sessionId is set) and the uninstall(...) calls
and replace Process.myUid()/0 with a derived user id obtained via
UserHandle.getUserId(Process.myUid()) (or equivalent method that returns the
Android user/profile id for the current process) so both createSession and
uninstall target the correct Android user/profile.
...ata/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.kt
Show resolved
Hide resolved
...ata/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.kt
Show resolved
Hide resolved
.../data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.kt
Show resolved
Hide resolved
.../data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.kt
Show resolved
Hide resolved
...n/src/commonMain/kotlin/zed/rainxch/profile/presentation/components/sections/Installation.kt
Show resolved
Hide resolved
Comment on lines
+203
to
+210
| installationSection( | ||
| state = state, | ||
| onAction = onAction | ||
| ) | ||
|
|
||
| item { | ||
| Spacer(Modifier.height(32.dp)) | ||
| } |
Contributor
There was a problem hiding this comment.
Gate the spacer with the installation section.
installationSection() already returns early off Android, but the spacer at Lines 208-210 is unconditional. That leaves an extra 32.dp gap on non-Android builds. Please move the spacer into the section itself or guard it with the same platform check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/ProfileRoot.kt`
around lines 203 - 210, The Spacer after installationSection is rendered
unconditionally causing a 32.dp gap on non-Android builds; modify the call site
so the spacer is only emitted when the installation section is actually shown by
either moving the Spacer(Modifier.height(32.dp)) into the installationSection
implementation or wrapping the Spacer with the same platform check used by
installationSection (i.e., the same early-return/Android guard) so that Spacer
is skipped on non-Android targets.
…ndroid framework APIs. - Replace direct `IPackageInstaller` and `IPackageInstallerSession` references with reflection to improve compatibility across Android versions. - Implement dynamic method lookups for `createSession`, `openSession`, `openWrite`, `commit`, and `uninstall` to handle varying method signatures. - Update `createStatusReceiver` to use a dynamic proxy for `IIntentSender` and reflectively instantiate `IntentSender`. - Update `Shizuku.unbindUserService` call in `ShizukuServiceManager` to include the `remove` flag.
# Conflicts: # core/data/src/androidMain/kotlin/zed/rainxch/core/data/di/PlatformModule.android.kt # core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ThemesRepositoryImpl.kt # core/data/src/jvmMain/kotlin/zed/rainxch/core/data/di/PlatformModule.jvm.kt # core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/ThemesRepository.kt # core/presentation/src/commonMain/composeResources/values/strings.xml # feature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/ProfileAction.kt # feature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/ProfileState.kt # feature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/ProfileViewModel.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.kt (2)
145-153:⚠️ Potential issue | 🟠 MajorUse
pingBinder()result before returning the cached service.Line 148 invokes
pingBinder()but Line 149 returns the cached service unconditionally, so a dead binder can still be reused.Suggested fix
installerService?.let { service -> try { - service.asBinder().pingBinder() - return service + if (service.asBinder().pingBinder()) { + return service + } + installerService = null } catch (_: Exception) { installerService = null } }#!/bin/bash # Verify pingBinder() return value is used before returning cached service sed -n '142,156p' core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.kt🤖 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/shizuku/ShizukuServiceManager.kt` around lines 145 - 153, The cached installerService is returned without checking pingBinder()'s boolean result; update the check in the method that returns installerService (the block using installerService?.let { service -> service.asBinder().pingBinder() ... }) to only return service when pingBinder() returns true, and set installerService = null when pingBinder() returns false or throws; ensure you reference installerService and the service.asBinder().pingBinder() call so the cached dead binder is not reused.
158-195:⚠️ Potential issue | 🟠 MajorPersist
UserServiceArgsand unbind the Shizuku user service during teardown.
unbindService()(Lines 202-205) only clears local refs and never callsShizuku.unbindUserService(...), sodestroy()does not actually unbind the privileged service.Suggested fix
class ShizukuServiceManager( private val context: Context ) { @@ private var serviceConnection: ServiceConnection? = null + private var userServiceArgs: Shizuku.UserServiceArgs? = null @@ val args = Shizuku.UserServiceArgs( ComponentName( context.packageName, ShizukuInstallerServiceImpl::class.java.name ) ) .daemon(false) .processNameSuffix("installer") .version(1) + userServiceArgs = args @@ private fun unbindService() { + try { + val args = userServiceArgs + val connection = serviceConnection + if (args != null) { + Shizuku.unbindUserService(args, connection, true) + } + } catch (_: Exception) {} installerService = null serviceConnection = null + userServiceArgs = null } }#!/bin/bash # Verify unbind path really calls Shizuku.unbindUserService(...) rg -n -C3 'unbindService\(|unbindUserService\(' core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.ktAlso applies to: 202-205
🤖 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/shizuku/ShizukuServiceManager.kt` around lines 158 - 195, bindService() currently creates a local UserServiceArgs ("args") and never persists it, and unbindService()/destroy() only clears refs without calling Shizuku.unbindUserService(...); persist the args by storing the created Shizuku.UserServiceArgs into a field (e.g., userServiceArgs) when calling Shizuku.bindUserService(args, connection), and update unbindService() / destroy() to call Shizuku.unbindUserService(userServiceArgs, serviceConnection, true) (guarded by null checks) and then clear userServiceArgs, serviceConnection, and installerService; also ensure the continuation.invokeOnCancellation block uses the persisted userServiceArgs/serviceConnection or performs the same unbind call consistently.core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt (1)
210-223:⚠️ Potential issue | 🔴 CriticalPass framework user/profile ID, not UID/hard-coded
0, to installer APIs.Line 222 passes
Process.myUid()and Lines 266/279 pass0; these calls should use Android user/profile ID consistently to target the correct user context.Suggested fix
+import android.os.Process +import android.os.UserHandle @@ - val uid = android.os.Process.myUid() + val userId = UserHandle.getUserId(Process.myUid()) @@ - return method.invoke(installer, params, callerPackage, null, uid) as Int + return method.invoke(installer, params, callerPackage, null, userId) as Int @@ - method.invoke(installer, versionedPackage, callerPackage, 0, statusReceiver, 0) + val userId = UserHandle.getUserId(Process.myUid()) + method.invoke(installer, versionedPackage, callerPackage, userId, statusReceiver, 0) return @@ - method.invoke(installer, versionedPackage, callerPackage, 0, statusReceiver) + val userId = UserHandle.getUserId(Process.myUid()) + method.invoke(installer, versionedPackage, callerPackage, userId, statusReceiver) return#!/bin/bash # Verify current arguments passed to createSession/uninstall reflection calls sed -n '205,235p' core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt sed -n '254,282p' core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.ktAlso applies to: 256-267, 270-280
🤖 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/shizuku/ShizukuInstallerServiceImpl.kt` around lines 210 - 223, The code is passing a UID or hard-coded 0 to installer APIs but must pass the Android user/profile ID; replace usages of android.os.Process.myUid() and any hard-coded 0 passed into reflected installer calls with the user id computed via android.os.UserHandle.getUserId(android.os.Process.myUid()) and pass that Int into the reflected invokes (e.g. the reflected createSession invocation obtained via installerClass.getMethod("createSession", ...) and the reflected uninstall/uninstallExistingPackage invokes elsewhere); update all places in ShizukuInstallerServiceImpl where a UID or 0 is passed to installer APIs to use this computed userId instead.
🧹 Nitpick comments (2)
core/presentation/src/commonMain/composeResources/values/strings.xml (1)
512-512: Keep section label casing consistent with other profile sections.
section_installationuses title case while other section labels in this file are uppercase; consider matching style for visual consistency.Suggested tweak
- <string name="section_installation">Installation</string> + <string name="section_installation">INSTALLATION</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/presentation/src/commonMain/composeResources/values/strings.xml` at line 512, section_installation currently uses title case ("Installation") while other profile section labels are uppercase; update the string resource with name "section_installation" to use uppercase ("INSTALLATION") to match the existing style for consistency across the file.core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt (1)
152-154: Log swallowed exceptions in install/uninstall failure paths.Lines 152-154 and 196-198 discard exception context; this makes production failures hard to diagnose.
Suggested fix
+import co.touchlab.kermit.Logger @@ class ShizukuInstallerServiceImpl() : IShizukuInstallerService.Stub() { + private val logger = Logger.withTag("ShizukuInstallerServiceImpl") @@ - } catch (e: Exception) { + } catch (e: Exception) { + logger.w(e) { "installPackage failed for path=$apkPath" } STATUS_FAILURE } @@ - } catch (e: Exception) { + } catch (e: Exception) { + logger.w(e) { "uninstallPackage failed for package=$packageName" } STATUS_FAILURE }Also applies to: 196-198
🤖 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/shizuku/ShizukuInstallerServiceImpl.kt` around lines 152 - 154, The catch blocks in ShizukuInstallerServiceImpl that return STATUS_FAILURE currently swallow exceptions; update the catch blocks in the install and uninstall paths to log the caught Exception (include the exception message and stacktrace) before returning STATUS_FAILURE — e.g., in the catch (e: Exception) {...} inside the install and uninstall methods, call your logger (or processLogger) with a descriptive message and pass e (logger.error("Failed to install/uninstall via Shizuku", e)), then return STATUS_FAILURE.
🤖 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/shizuku/ShizukuInstallerServiceImpl.kt`:
- Around line 323-326: The "asBinder" branch currently returns null which causes
NPEs; change it to return a real Binder instance instead: create a Binder
subclass (or an instance of android.os.Binder) tied to the dynamic proxy in
ShizukuInstallerServiceImpl so IIntentSender.asBinder() returns that binder;
implement the Binder to forward/mediate any required transact/parcel behavior
back to the proxy or keep a no-op binder if only identity is needed, and update
the "asBinder" case to return that Binder instance rather than null so
IntentSender serialization/parcelling succeeds.
---
Duplicate comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt`:
- Around line 210-223: The code is passing a UID or hard-coded 0 to installer
APIs but must pass the Android user/profile ID; replace usages of
android.os.Process.myUid() and any hard-coded 0 passed into reflected installer
calls with the user id computed via
android.os.UserHandle.getUserId(android.os.Process.myUid()) and pass that Int
into the reflected invokes (e.g. the reflected createSession invocation obtained
via installerClass.getMethod("createSession", ...) and the reflected
uninstall/uninstallExistingPackage invokes elsewhere); update all places in
ShizukuInstallerServiceImpl where a UID or 0 is passed to installer APIs to use
this computed userId instead.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.kt`:
- Around line 145-153: The cached installerService is returned without checking
pingBinder()'s boolean result; update the check in the method that returns
installerService (the block using installerService?.let { service ->
service.asBinder().pingBinder() ... }) to only return service when pingBinder()
returns true, and set installerService = null when pingBinder() returns false or
throws; ensure you reference installerService and the
service.asBinder().pingBinder() call so the cached dead binder is not reused.
- Around line 158-195: bindService() currently creates a local UserServiceArgs
("args") and never persists it, and unbindService()/destroy() only clears refs
without calling Shizuku.unbindUserService(...); persist the args by storing the
created Shizuku.UserServiceArgs into a field (e.g., userServiceArgs) when
calling Shizuku.bindUserService(args, connection), and update unbindService() /
destroy() to call Shizuku.unbindUserService(userServiceArgs, serviceConnection,
true) (guarded by null checks) and then clear userServiceArgs,
serviceConnection, and installerService; also ensure the
continuation.invokeOnCancellation block uses the persisted
userServiceArgs/serviceConnection or performs the same unbind call consistently.
---
Nitpick comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt`:
- Around line 152-154: The catch blocks in ShizukuInstallerServiceImpl that
return STATUS_FAILURE currently swallow exceptions; update the catch blocks in
the install and uninstall paths to log the caught Exception (include the
exception message and stacktrace) before returning STATUS_FAILURE — e.g., in the
catch (e: Exception) {...} inside the install and uninstall methods, call your
logger (or processLogger) with a descriptive message and pass e
(logger.error("Failed to install/uninstall via Shizuku", e)), then return
STATUS_FAILURE.
In `@core/presentation/src/commonMain/composeResources/values/strings.xml`:
- Line 512: section_installation currently uses title case ("Installation")
while other profile section labels are uppercase; update the string resource
with name "section_installation" to use uppercase ("INSTALLATION") to match the
existing style for consistency across the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3172006e-fd43-4b4d-898f-f4208528a04d
📒 Files selected for processing (5)
core/data/build.gradle.ktscore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ThemesRepositoryImpl.ktcore/presentation/src/commonMain/composeResources/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ThemesRepositoryImpl.kt
Comment on lines
+323
to
+326
| "asBinder" -> { | ||
| // Return the proxy itself as a binder stand-in | ||
| null | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify asBinder branch in dynamic proxy for status receiver
sed -n '291,335p' core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 2073
🌐 Web query:
For Android hidden API usage, is an IIntentSender implementation valid if asBinder() returns null when constructing IntentSender(IIntentSender)?
💡 Result:
No. While new IntentSender(IIntentSender target) doesn’t itself validate target or target.asBinder(), the class assumes mTarget.asBinder() is non-null and calls it without null checks (e.g., writeToParcel() and writeIntentSenderOrNullToParcel()), so an IIntentSender whose asBinder() returns null will crash (NPE) or be treated as “no sender” when written as a binder. Use a real Binder-backed IIntentSender (e.g., an AIDL Stub/Proxy), not a null-asBinder() implementation. [1]
Sources:
[1] AOSP IntentSender.java (constructors + writeToParcel / writeIntentSenderOrNullToParcel) https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/content/IntentSender.java
Return a Binder instance (not null) from IIntentSender.asBinder().
Returning null from asBinder() on line 325 will cause NullPointerException when IntentSender attempts to serialize or parcel itself (via writeToParcel() or writeIntentSenderOrNullToParcel()), leading to install/uninstall failures. The IntentSender class assumes asBinder() returns a non-null Binder instance and does not perform null checks before invoking it. Use a real Binder-backed implementation instead of a dynamic proxy returning null.
🤖 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/shizuku/ShizukuInstallerServiceImpl.kt`
around lines 323 - 326, The "asBinder" branch currently returns null which
causes NPEs; change it to return a real Binder instance instead: create a Binder
subclass (or an instance of android.os.Binder) tied to the dynamic proxy in
ShizukuInstallerServiceImpl so IIntentSender.asBinder() returns that binder;
implement the Binder to forward/mediate any required transact/parcel behavior
back to the proxy or keep a no-op binder if only identity is needed, and update
the "asBinder" case to return that Binder instance rather than null so
IntentSender serialization/parcelling succeeds.
…allation and improve service logging. - Update `IShizukuInstallerService.aidl` to accept `ParcelFileDescriptor` and file size instead of a file path. - Modify `ShizukuInstallerServiceImpl` to read APK data from the provided file descriptor and add comprehensive logging for session creation and installation status. - Improve `ShizukuServiceManager` status checking and service binding with additional diagnostic logs. - Update `ShizukuInstallerWrapper` to open and pass a `ParcelFileDescriptor` during installation and add detailed tracing for fallback logic.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt (2)
247-266:⚠️ Potential issue | 🔴 CriticalUse a framework user/profile ID here, not a UID or owner-user literal.
The reflected
createSession(...)anduninstall(...)overloads expect the target Android userId, not a Linux UID or a hard-coded owner user.Process.myUid()and0will target the wrong profile or fail on work-profile / multi-user devices; derive the userId once withUserHandle.getUserId(Process.myUid())and pass that to the userId parameter in both paths.Also applies to: 306-334
🤖 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/shizuku/ShizukuInstallerServiceImpl.kt` around lines 247 - 266, The createSession implementation (and the corresponding uninstall overloads) uses a Linux UID/hard-coded 0 for the reflected userId parameter; instead derive the Android userId once via UserHandle.getUserId(Process.myUid()) and pass that userId to the reflected createSession method call (the Int parameter in the API 33+ signature invoked on installerClass.createSession) and to the older createSession/uninstall reflection paths referenced later (the same int userId parameter), replacing Process.myUid() and literal 0 so the correct profile/work-user is targeted.
397-400:⚠️ Potential issue | 🔴 Critical
asBinder()cannot returnnullon this callback object.Once
IntentSendercrosses the binder boundary, only theIIntentSenderbinder is transported. Returningnullhere breaks that callback channel, so install/uninstall status delivery can crash or disappear before the latch is released. Use a Binder-backedIIntentSenderimplementation and return that binder fromasBinder().🤖 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/shizuku/ShizukuInstallerServiceImpl.kt` around lines 397 - 400, The asBinder() implementation in the anonymous IIntentSender callback must not return null; replace the current null return with a Binder-backed IIntentSender implementation (e.g., create an object extending IIntentSender.Stub or a Binder wrapper inside ShizukuInstallerServiceImpl) and return that object's asBinder(); ensure the callback logic (the send/handle method used for install/uninstall status delivery) is executed through that Binder-backed instance so the IntentSender can cross the binder boundary and reliably deliver results and release the latch.core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.kt (1)
83-90:⚠️ Potential issue | 🟠 MajorActually unbind the Shizuku UserService on teardown.
bindService()createsUserServiceArgslocally, butdestroy()/unbindService()only clear local references. The privileged process stays bound because the normal cleanup path never callsShizuku.unbindUserService(...).🧩 Store the bind args and use them during cleanup
private var serviceConnection: ServiceConnection? = null + private var userServiceArgs: Shizuku.UserServiceArgs? = null @@ val args = Shizuku.UserServiceArgs(componentName) .daemon(false) .processNameSuffix("installer") .version(1) + userServiceArgs = args @@ private fun unbindService() { - installerService = null - serviceConnection = null + try { + userServiceArgs?.let { args -> + Shizuku.unbindUserService(args, serviceConnection, true) + } + } catch (_: Exception) { + } finally { + userServiceArgs = null + installerService = null + serviceConnection = null + } }Also applies to: 176-230
🤖 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/shizuku/ShizukuServiceManager.kt` around lines 83 - 90, bindService() currently creates UserServiceArgs locally and destroy()/unbindService() only clear local refs, leaving the privileged process bound; update ShizukuServiceManager to persist the UserServiceArgs created in bindService() (e.g., a private var storedUserServiceArgs) and in destroy() / unbindService() call Shizuku.unbindUserService(storedUserServiceArgs) (null-check first), then remove listeners and clear storedUserServiceArgs; apply the same fix to the other bind/unbind pair referenced around the 176-230 region so both binding paths unbind using the saved UserServiceArgs.core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.kt (2)
152-166:⚠️ Potential issue | 🟠 MajorSilent uninstall never binds the Shizuku service in a fresh process.
This path reads
shizukuServiceManager.installerServicedirectly, but onlygetService()establishes the UserService connection. Withstatus == READYand no earlier bind, the first uninstall after launch always falls back to the standard flow.Because
uninstall()is synchronous today, the least invasive fix is to bind eagerly when Shizuku becomes READY, or move this path to an async/suspend flow that can awaitgetService()before deciding to fall back.🤖 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/shizuku/ShizukuInstallerWrapper.kt` around lines 152 - 166, The uninstall path reads shizukuServiceManager.installerService directly and can miss binding in a fresh process; change the flow in the uninstall method inside ShizukuInstallerWrapper so that when shizukuServiceManager.status == ShizukuStatus.READY you call shizukuServiceManager.getService() (which establishes the UserService connection) and await its result before accessing installerService, or alternatively convert this branch to an async/suspend variant that awaits getService() and only falls back after getService() fails; ensure you reference shizukuServiceManager.getService(), shizukuServiceManager.installerService, and the uninstall function when making the change.
95-145:⚠️ Potential issue | 🟠 MajorKeep the fallback path permission-safe.
ensurePermissionsOrThrow()returns early whenever Shizuku is READY, butinstall()still falls back on null service / exceptions, and a non-zero Shizuku result is rethrown instead of falling through. That leaves the standard installer either unreachable or running withoutandroidInstaller.ensurePermissionsOrThrow(extOrMime).🧩 One way to preserve the fallback end-to-end
override suspend fun ensurePermissionsOrThrow(extOrMime: String) { Logger.d(TAG) { "ensurePermissionsOrThrow() — extOrMime=$extOrMime, cachedType=$cachedInstallerType, status=${shizukuServiceManager.status.value}" } - if (shouldUseShizuku()) { - Logger.d(TAG) { "Shizuku active — skipping unknown sources permission check" } - return + if (!shouldUseShizuku()) { + Logger.d(TAG) { "Delegating ensurePermissionsOrThrow to AndroidInstaller" } + androidInstaller.ensurePermissionsOrThrow(extOrMime) } - Logger.d(TAG) { "Delegating ensurePermissionsOrThrow to AndroidInstaller" } - androidInstaller.ensurePermissionsOrThrow(extOrMime) } @@ if (result == 0) { Logger.d(TAG) { "Shizuku install SUCCEEDED for: $filePath" } return } - Logger.w(TAG) { "Shizuku install FAILED with code: $result" } - throw IllegalStateException("Shizuku install failed (code: $result)") + Logger.w(TAG) { "Shizuku install FAILED with code: $result, falling back" } } else { Logger.w(TAG) { "Shizuku service is NULL, falling back to standard installer" } } - } catch (e: IllegalStateException) { - Logger.e(TAG) { "Shizuku install error (re-throwing): ${e.message}" } - throw e } catch (e: Exception) { Logger.e(TAG) { "Shizuku install exception, falling back: ${e.javaClass.simpleName}: ${e.message}" } Logger.e(TAG) { e.stackTraceToString() } } + androidInstaller.ensurePermissionsOrThrow(extOrMime) } else { Logger.d(TAG) { "Not using Shizuku (enabled=${isShizukuEnabled()}, status=${shizukuServiceManager.status.value})" } } Logger.d(TAG) { "Using standard AndroidInstaller for: $filePath" } androidInstaller.install(filePath, extOrMime)🤖 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/shizuku/ShizukuInstallerWrapper.kt` around lines 95 - 145, The Shizuku install path currently may short-circuit or throw and never run androidInstaller.ensurePermissionsOrThrow(extOrMime) before falling back; update install(filePath, extOrMime) so that any fallback path (service == null, non-zero result, or caught Exception) does not throw away the normal permission check: 1) when shouldUseShizuku() is true, attempt Shizuku via shizukuServiceManager.getService(); if service is null or installPackage returns non-zero, do NOT throw IllegalStateException—log and proceed to fallback; 2) on any caught Exception during Shizuku attempt, log and proceed to fallback without rethrowing; 3) before calling androidInstaller.install(filePath, extOrMime) (the fallback), always call androidInstaller.ensurePermissionsOrThrow(extOrMime); keep existing early-return when Shizuku is active and intended to be used successfully, but ensure all fallback routes call ensurePermissionsOrThrow on androidInstaller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt`:
- Around line 247-266: The createSession implementation (and the corresponding
uninstall overloads) uses a Linux UID/hard-coded 0 for the reflected userId
parameter; instead derive the Android userId once via
UserHandle.getUserId(Process.myUid()) and pass that userId to the reflected
createSession method call (the Int parameter in the API 33+ signature invoked on
installerClass.createSession) and to the older createSession/uninstall
reflection paths referenced later (the same int userId parameter), replacing
Process.myUid() and literal 0 so the correct profile/work-user is targeted.
- Around line 397-400: The asBinder() implementation in the anonymous
IIntentSender callback must not return null; replace the current null return
with a Binder-backed IIntentSender implementation (e.g., create an object
extending IIntentSender.Stub or a Binder wrapper inside
ShizukuInstallerServiceImpl) and return that object's asBinder(); ensure the
callback logic (the send/handle method used for install/uninstall status
delivery) is executed through that Binder-backed instance so the IntentSender
can cross the binder boundary and reliably deliver results and release the
latch.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.kt`:
- Around line 152-166: The uninstall path reads
shizukuServiceManager.installerService directly and can miss binding in a fresh
process; change the flow in the uninstall method inside ShizukuInstallerWrapper
so that when shizukuServiceManager.status == ShizukuStatus.READY you call
shizukuServiceManager.getService() (which establishes the UserService
connection) and await its result before accessing installerService, or
alternatively convert this branch to an async/suspend variant that awaits
getService() and only falls back after getService() fails; ensure you reference
shizukuServiceManager.getService(), shizukuServiceManager.installerService, and
the uninstall function when making the change.
- Around line 95-145: The Shizuku install path currently may short-circuit or
throw and never run androidInstaller.ensurePermissionsOrThrow(extOrMime) before
falling back; update install(filePath, extOrMime) so that any fallback path
(service == null, non-zero result, or caught Exception) does not throw away the
normal permission check: 1) when shouldUseShizuku() is true, attempt Shizuku via
shizukuServiceManager.getService(); if service is null or installPackage returns
non-zero, do NOT throw IllegalStateException—log and proceed to fallback; 2) on
any caught Exception during Shizuku attempt, log and proceed to fallback without
rethrowing; 3) before calling androidInstaller.install(filePath, extOrMime) (the
fallback), always call androidInstaller.ensurePermissionsOrThrow(extOrMime);
keep existing early-return when Shizuku is active and intended to be used
successfully, but ensure all fallback routes call ensurePermissionsOrThrow on
androidInstaller.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.kt`:
- Around line 83-90: bindService() currently creates UserServiceArgs locally and
destroy()/unbindService() only clear local refs, leaving the privileged process
bound; update ShizukuServiceManager to persist the UserServiceArgs created in
bindService() (e.g., a private var storedUserServiceArgs) and in destroy() /
unbindService() call Shizuku.unbindUserService(storedUserServiceArgs)
(null-check first), then remove listeners and clear storedUserServiceArgs; apply
the same fix to the other bind/unbind pair referenced around the 176-230 region
so both binding paths unbind using the saved UserServiceArgs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 437c6449-ef82-4488-99c2-0ba6b4a0a99e
📒 Files selected for processing (4)
core/data/src/androidMain/aidl/zed/rainxch/core/data/services/shizuku/IShizukuInstallerService.aidlcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.kt
- Replace fragile reflection-based `PackageInstaller` session logic with `pm install` and `pm uninstall` shell commands - Implement APK streaming to `pm install` via stdin pipe using `ParcelFileDescriptor` - Offload blocking AIDL installation and uninstallation calls to IO dispatchers/background threads to prevent ANRs - Simplify error handling and status mapping by relying on process exit codes and output strings - Update file descriptor mode from `READ_WRITE` to `READ_ONLY` for installation source files
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.kt (2)
129-141:⚠️ Potential issue | 🟠 MajorDo not rethrow Shizuku install failures; always fall through to standard installer with permission check.
At Lines 133-140, non-zero Shizuku result is converted to
IllegalStateExceptionand rethrown, which breaks the documented fallback behavior. Also, before Line 150 fallback install, ensure standard-installer permissions are validated for this path.Suggested fix
- Logger.w(TAG) { "Shizuku install FAILED with code: $result" } - throw IllegalStateException("Shizuku install failed (code: $result)") + Logger.w(TAG) { "Shizuku install FAILED with code: $result, falling back" } } else { Logger.w(TAG) { "Shizuku service is NULL, falling back to standard installer" } } - } catch (e: IllegalStateException) { - Logger.e(TAG) { "Shizuku install error (re-throwing): ${e.message}" } - throw e } catch (e: Exception) { Logger.e(TAG) { "Shizuku install exception, falling back: ${e.javaClass.simpleName}: ${e.message}" } Logger.e(TAG) { e.stackTraceToString() } } } else { @@ - Logger.d(TAG) { "Using standard AndroidInstaller for: $filePath" } + androidInstaller.ensurePermissionsOrThrow(extOrMime) + Logger.d(TAG) { "Using standard AndroidInstaller for: $filePath" } androidInstaller.install(filePath, extOrMime)Also applies to: 149-150
🤖 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/shizuku/ShizukuInstallerWrapper.kt` around lines 129 - 141, The ShizukuInstallerWrapper install flow currently throws IllegalStateException on non-zero Shizuku results (in the try block around the Shizuku install path) which prevents falling back to the standard installer; change the logic in the ShizukuInstallerWrapper method handling the Shizuku install result so that a non-zero result is logged (use Logger.w/e) but not rethrown, allow execution to continue to the standard installer path, and before invoking the standard installer call the same permission validation used elsewhere (reuse the permission-check helper or the validate-permissions logic present around the fallback path) to ensure standard-installer permissions are verified when Shizuku fails. Ensure you remove or stop rethrowing IllegalStateException in both the Shizuku result branch and the catch (e: IllegalStateException) block so fallback is always attempted.
157-177:⚠️ Potential issue | 🟠 MajorUninstall path should bind/resolve service before use and fallback on Shizuku failure codes.
At Line 160, using cached
installerServicedirectly can miss first-call binding; and after Line 171 non-zero Shizuku result only logs while Line 177 already returned, so standard fallback is skipped. Resolve/bind service before calling uninstall and invokeandroidInstaller.uninstall(...)on null service, non-zero result, or exception.#!/bin/bash # Verify service resolution path and uninstall fallback behavior sed -n '1,260p' core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.kt sed -n '145,210p' core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.kt rg -nP 'getService\s*\(|installerService|uninstallPackage\(|androidInstaller\.uninstall\(' \ core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.kt \ core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.ktAlso applies to: 186-187
🤖 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/shizuku/ShizukuInstallerWrapper.kt` around lines 157 - 177, The cached installerService is used directly in ShizukuInstallerWrapper (inside the Thread block calling installerService.uninstallPackage), which can skip binding on first use and prematurely return on non-zero result or exception so the androidInstaller fallback never runs; modify the uninstall flow to first attempt to resolve/bind the Shizuku service via ShizukuServiceManager (use its getService/getInstallerService resolver method) before calling uninstallPackage, and if the resolved service is null, if uninstallPackage returns a non-zero result, or if an exception occurs, call androidInstaller.uninstall(...) as the fallback (apply the same change to the second uninstall site around lines 186-187), and only return early when the Shizuku uninstall definitively succeeded (result == 0).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt`:
- Around line 70-73: The writer thread join result is ignored and
process.waitFor() calls are unbounded; in the method in
ShizukuInstallerServiceImpl that performs the pm install (references:
writeThread and process.waitFor()), change writeThread.join(120_000) to check
its boolean return and, if it times out, call process.destroyForcibly() and
return failure; replace unbounded process.waitFor() calls with timed waits
(e.g., process.waitFor(timeout, TimeUnit.MILLISECONDS)) and if those return
false (timeout), destroy the process and return failure; apply the same change
to the second waitFor() occurrence so both joins and process waits enforce
timeouts and abort cleanly.
---
Duplicate comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.kt`:
- Around line 129-141: The ShizukuInstallerWrapper install flow currently throws
IllegalStateException on non-zero Shizuku results (in the try block around the
Shizuku install path) which prevents falling back to the standard installer;
change the logic in the ShizukuInstallerWrapper method handling the Shizuku
install result so that a non-zero result is logged (use Logger.w/e) but not
rethrown, allow execution to continue to the standard installer path, and before
invoking the standard installer call the same permission validation used
elsewhere (reuse the permission-check helper or the validate-permissions logic
present around the fallback path) to ensure standard-installer permissions are
verified when Shizuku fails. Ensure you remove or stop rethrowing
IllegalStateException in both the Shizuku result branch and the catch (e:
IllegalStateException) block so fallback is always attempted.
- Around line 157-177: The cached installerService is used directly in
ShizukuInstallerWrapper (inside the Thread block calling
installerService.uninstallPackage), which can skip binding on first use and
prematurely return on non-zero result or exception so the androidInstaller
fallback never runs; modify the uninstall flow to first attempt to resolve/bind
the Shizuku service via ShizukuServiceManager (use its
getService/getInstallerService resolver method) before calling uninstallPackage,
and if the resolved service is null, if uninstallPackage returns a non-zero
result, or if an exception occurs, call androidInstaller.uninstall(...) as the
fallback (apply the same change to the second uninstall site around lines
186-187), and only return early when the Shizuku uninstall definitively
succeeded (result == 0).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f5dba75-d43c-4a79-83fd-61a02aa41cd9
📒 Files selected for processing (2)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.kt
...src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt
Outdated
Show resolved
Hide resolved
- Add 120s install and 30s uninstall timeouts to `ShizukuInstallerServiceImpl`. - Fix Shizuku service unbinding by tracking `UserServiceArgs` in `ShizukuServiceManager`. - Improve error handling in `ShizukuInstallerWrapper` to fallback to standard installer on failure. - Update profile UI with adjusted spacing and improved accessibility for installer selection. - Add `cache_cleared` string resource.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
feature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/components/sections/Installation.kt (1)
116-137: Show Shizuku permission/hint UI only when Shizuku is selected.Right now the Shizuku CTA/hints render even when Default installer is selected, which adds unrelated noise. Consider gating this block with
selectedType == InstallerType.SHIZUKU.Suggested change
- when (shizukuAvailability) { - ShizukuAvailability.PERMISSION_NEEDED -> { - FilledTonalButton( - onClick = onRequestPermission, - modifier = Modifier.fillMaxWidth(), - shape = RoundedCornerShape(12.dp) - ) { - Text( - text = stringResource(Res.string.shizuku_grant_permission), - style = MaterialTheme.typography.titleSmall, - fontWeight = FontWeight.Bold - ) - } - } - - ShizukuAvailability.UNAVAILABLE -> { - HintText(text = stringResource(Res.string.shizuku_install_hint)) - } - - ShizukuAvailability.NOT_RUNNING -> { - HintText(text = stringResource(Res.string.shizuku_start_hint)) - } - - ShizukuAvailability.READY -> { - // No hint needed - } - } + if (selectedType == InstallerType.SHIZUKU) { + when (shizukuAvailability) { + ShizukuAvailability.PERMISSION_NEEDED -> { + FilledTonalButton( + onClick = onRequestPermission, + modifier = Modifier.fillMaxWidth(), + shape = RoundedCornerShape(12.dp) + ) { + Text( + text = stringResource(Res.string.shizuku_grant_permission), + style = MaterialTheme.typography.titleSmall, + fontWeight = FontWeight.Bold + ) + } + } + + ShizukuAvailability.UNAVAILABLE -> { + HintText(text = stringResource(Res.string.shizuku_install_hint)) + } + + ShizukuAvailability.NOT_RUNNING -> { + HintText(text = stringResource(Res.string.shizuku_start_hint)) + } + + ShizukuAvailability.READY -> { + // No hint needed + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/components/sections/Installation.kt` around lines 116 - 137, The Shizuku permission/hint UI block (using shizukuAvailability, FilledTonalButton, HintText and onRequestPermission) is currently always rendered; wrap this when-block so it only runs when the selected installer is Shizuku by checking selectedType == InstallerType.SHIZUKU before evaluating shizukuAvailability, i.e. gate the entire permission/hint branch with that condition to prevent showing Shizuku CTAs/hints for other installers.
🤖 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/shizuku/ShizukuInstallerWrapper.kt`:
- Around line 166-170: The Shizuku uninstall branch only logs non-zero results
but doesn't perform the documented fallback; update the else path (where
`result` is non-zero) in ShizukuInstallerWrapper's `uninstallPackage` handling
to call `androidInstaller.uninstall(...)` with the same packageName and listener
so the uninstall is delegated on failure (and likewise ensure any exception path
also delegates to `androidInstaller.uninstall`). Make sure to preserve existing
logging (Logger.w(TAG) {...}) and to return/propagate the listener callback
behavior consistently after delegating to `androidInstaller.uninstall`.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.kt`:
- Around line 192-213: The ServiceConnection instance (local variable
connection) must be stored to serviceConnection before calling
Shizuku.bindUserService to ensure unbindService() won’t miss a pending
connection; assign serviceConnection = connection (and set boundUserServiceArgs
= args) prior to Shizuku.bindUserService(args, connection), leaving
onServiceConnected to overwrite installerService and resume the continuation as
before, and ensure any teardown uses serviceConnection to unbind even if
onServiceConnected never fired; apply the same change at the second occurrence
around lines 229-233.
- Around line 151-175: Concurrent callers of getService() may simultaneously
call bindService() and race on installerService, serviceConnection, and
boundUserServiceArgs; serialize bind/unbind operations by introducing a single
coroutine Mutex (e.g., private val bindMutex = Mutex()) and wrap the entire
binding-critical section in getService() and bindService() with
bindMutex.withLock { ... }, re-checking installerService after entering the lock
to return a newly-set cached service if another coroutine bound it, and likewise
protect any unbind logic that touches
serviceConnection/installerService/boundUserServiceArgs with the same mutex to
ensure correct bookkeeping.
In `@core/presentation/src/commonMain/composeResources/values/strings.xml`:
- Line 473: The string resource named "cache_cleared" has awkward English ("The
cache is gradually cleared."); update its value to a natural status phrase such
as "Cache cleared" for a completed state or "Clearing cache…" for an in-progress
state (choose the one that matches the UI context), replacing the current text
in the <string name="cache_cleared"> entry.
In
`@feature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/components/sections/Installation.kt`:
- Around line 61-63: The code is uppercasing a localized string at runtime which
is locale-sensitive; remove the .uppercase() call on
stringResource(Res.string.section_installation) in the SectionHeader invocation
and instead provide the correctly-cased version in your localized resources (or
keep the original casing in resources), i.e., pass
stringResource(Res.string.section_installation) directly to SectionHeader and
update the Res.string.section_installation translations per locale to include
any desired uppercase styling.
---
Nitpick comments:
In
`@feature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/components/sections/Installation.kt`:
- Around line 116-137: The Shizuku permission/hint UI block (using
shizukuAvailability, FilledTonalButton, HintText and onRequestPermission) is
currently always rendered; wrap this when-block so it only runs when the
selected installer is Shizuku by checking selectedType == InstallerType.SHIZUKU
before evaluating shizukuAvailability, i.e. gate the entire permission/hint
branch with that condition to prevent showing Shizuku CTAs/hints for other
installers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77fa7e89-7c9d-4a00-b56f-3cc21dbc7990
📒 Files selected for processing (6)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/ProfileRoot.ktfeature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/components/sections/Installation.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- feature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/ProfileRoot.kt
- core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerServiceImpl.kt
Comment on lines
+166
to
+170
| if (result == 0) { | ||
| Logger.d(TAG) { "Shizuku uninstall SUCCEEDED for: $packageName" } | ||
| } else { | ||
| Logger.w(TAG) { "Shizuku uninstall FAILED with code: $result" } | ||
| } |
Contributor
There was a problem hiding this comment.
Uninstall should also fallback on non-zero Shizuku result.
A non-zero uninstallPackage result is currently logged but not delegated to androidInstaller.uninstall(...), so uninstall can silently fail despite documented “fallback on any failure” behavior.
Suggested fix
if (result == 0) {
Logger.d(TAG) { "Shizuku uninstall SUCCEEDED for: $packageName" }
} else {
Logger.w(TAG) { "Shizuku uninstall FAILED with code: $result" }
+ androidInstaller.uninstall(packageName)
}🤖 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/shizuku/ShizukuInstallerWrapper.kt`
around lines 166 - 170, The Shizuku uninstall branch only logs non-zero results
but doesn't perform the documented fallback; update the else path (where
`result` is non-zero) in ShizukuInstallerWrapper's `uninstallPackage` handling
to call `androidInstaller.uninstall(...)` with the same packageName and listener
so the uninstall is delegated on failure (and likewise ensure any exception path
also delegates to `androidInstaller.uninstall`). Make sure to preserve existing
logging (Logger.w(TAG) {...}) and to return/propagate the listener callback
behavior consistently after delegating to `androidInstaller.uninstall`.
Comment on lines
+151
to
+175
| suspend fun getService(): IShizukuInstallerService? { | ||
| Logger.d(TAG) { "getService() — current status=${_status.value}" } | ||
| if (_status.value != ShizukuStatus.READY) { | ||
| Logger.w(TAG) { "getService() — Shizuku not READY (status=${_status.value}), returning null" } | ||
| return null | ||
| } | ||
|
|
||
| // Return cached service if still alive | ||
| installerService?.let { service -> | ||
| try { | ||
| val alive = service.asBinder().pingBinder() | ||
| Logger.d(TAG) { "getService() — cached service ping=$alive" } | ||
| if (alive) return service | ||
| Logger.w(TAG) { "getService() — cached service binder dead, rebinding..." } | ||
| installerService = null | ||
| } catch (e: Exception) { | ||
| Logger.w(TAG) { "getService() — cached service error: ${e.message}, rebinding..." } | ||
| installerService = null | ||
| } | ||
| } ?: run { | ||
| Logger.d(TAG) { "getService() — no cached service, binding..." } | ||
| } | ||
|
|
||
| return bindService() | ||
| } |
Contributor
There was a problem hiding this comment.
Serialize bind attempts to prevent connection/args races.
Parallel getService() callers can enter bindService() simultaneously and race on installerService, serviceConnection, and boundUserServiceArgs, which can break unbind bookkeeping.
Suggested fix
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
@@
class ShizukuServiceManager(
private val context: Context
) {
+ private val bindMutex = Mutex()
@@
suspend fun getService(): IShizukuInstallerService? {
- Logger.d(TAG) { "getService() — current status=${_status.value}" }
- if (_status.value != ShizukuStatus.READY) {
- Logger.w(TAG) { "getService() — Shizuku not READY (status=${_status.value}), returning null" }
- return null
- }
+ return bindMutex.withLock {
+ Logger.d(TAG) { "getService() — current status=${_status.value}" }
+ if (_status.value != ShizukuStatus.READY) {
+ Logger.w(TAG) { "getService() — Shizuku not READY (status=${_status.value}), returning null" }
+ return@withLock null
+ }
@@
- return bindService()
+ bindService()
+ }
}Also applies to: 177-221
🤖 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/shizuku/ShizukuServiceManager.kt`
around lines 151 - 175, Concurrent callers of getService() may simultaneously
call bindService() and race on installerService, serviceConnection, and
boundUserServiceArgs; serialize bind/unbind operations by introducing a single
coroutine Mutex (e.g., private val bindMutex = Mutex()) and wrap the entire
binding-critical section in getService() and bindService() with
bindMutex.withLock { ... }, re-checking installerService after entering the lock
to return a newly-set cached service if another coroutine bound it, and likewise
protect any unbind logic that touches
serviceConnection/installerService/boundUserServiceArgs with the same mutex to
ensure correct bookkeeping.
Comment on lines
+192
to
+213
| val connection = object : ServiceConnection { | ||
| override fun onServiceConnected(name: ComponentName?, binder: IBinder?) { | ||
| Logger.d(TAG) { "onServiceConnected() — name=$name, binder=${binder?.javaClass?.name}, binderAlive=${binder?.pingBinder()}" } | ||
| val service = IShizukuInstallerService.Stub.asInterface(binder) | ||
| installerService = service | ||
| serviceConnection = this | ||
| Logger.d(TAG) { "Shizuku installer service connected and cached" } | ||
| if (continuation.isActive) { | ||
| continuation.resume(service) | ||
| } | ||
| } | ||
|
|
||
| override fun onServiceDisconnected(name: ComponentName?) { | ||
| installerService = null | ||
| Logger.d(TAG) { "Shizuku installer service disconnected: $name" } | ||
| } | ||
| } | ||
|
|
||
| Logger.d(TAG) { "Calling Shizuku.bindUserService()..." } | ||
| boundUserServiceArgs = args | ||
| Shizuku.bindUserService(args, connection) | ||
| Logger.d(TAG) { "Shizuku.bindUserService() called, waiting for callback..." } |
Contributor
There was a problem hiding this comment.
Track ServiceConnection before binding to avoid missed unbinds.
unbindService() depends on serviceConnection, but it’s only set in onServiceConnected. If teardown happens before callback, unbind can be skipped.
Suggested fix
val connection = object : ServiceConnection {
override fun onServiceConnected(name: ComponentName?, binder: IBinder?) {
Logger.d(TAG) { "onServiceConnected() — name=$name, binder=${binder?.javaClass?.name}, binderAlive=${binder?.pingBinder()}" }
val service = IShizukuInstallerService.Stub.asInterface(binder)
installerService = service
- serviceConnection = this
Logger.d(TAG) { "Shizuku installer service connected and cached" }
if (continuation.isActive) {
continuation.resume(service)
}
}
@@
Logger.d(TAG) { "Calling Shizuku.bindUserService()..." }
+ serviceConnection = connection
boundUserServiceArgs = args
Shizuku.bindUserService(args, connection)Also applies to: 229-233
🤖 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/shizuku/ShizukuServiceManager.kt`
around lines 192 - 213, The ServiceConnection instance (local variable
connection) must be stored to serviceConnection before calling
Shizuku.bindUserService to ensure unbindService() won’t miss a pending
connection; assign serviceConnection = connection (and set boundUserServiceArgs
= args) prior to Shizuku.bindUserService(args, connection), leaving
onServiceConnected to overwrite installerService and resume the continuation as
before, and ensure any teardown uses serviceConnection to unbind even if
onServiceConnected never fired; apply the same change at the second occurrence
around lines 229-233.
| <string name="clear_cache">Clear cache</string> | ||
| <string name="current_size">Current size:</string> | ||
| <string name="clear">Clear</string> | ||
| <string name="cache_cleared">The cache is gradually cleared.</string> |
Contributor
There was a problem hiding this comment.
Polish the cache-cleared message wording.
Line 473 reads unnatural in English (“The cache is gradually cleared.”). Consider a clearer status phrase.
Suggested change
- <string name="cache_cleared">The cache is gradually cleared.</string>
+ <string name="cache_cleared">Cache is being cleared in the background.</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <string name="cache_cleared">The cache is gradually cleared.</string> | |
| <string name="cache_cleared">Cache is being cleared in the background.</string> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/presentation/src/commonMain/composeResources/values/strings.xml` at line
473, The string resource named "cache_cleared" has awkward English ("The cache
is gradually cleared."); update its value to a natural status phrase such as
"Cache cleared" for a completed state or "Clearing cache…" for an in-progress
state (choose the one that matches the UI context), replacing the current text
in the <string name="cache_cleared"> entry.
Comment on lines
+61
to
+63
| SectionHeader( | ||
| text = stringResource(Res.string.section_installation).uppercase() | ||
| ) |
Contributor
There was a problem hiding this comment.
Avoid locale-sensitive uppercasing on localized text.
Line 62 uppercases a translated string at runtime, which can produce incorrect casing in some locales. Prefer casing directly in resources per locale (or keep original casing).
Suggested change
- SectionHeader(
- text = stringResource(Res.string.section_installation).uppercase()
- )
+ SectionHeader(
+ text = stringResource(Res.string.section_installation)
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| SectionHeader( | |
| text = stringResource(Res.string.section_installation).uppercase() | |
| ) | |
| SectionHeader( | |
| text = stringResource(Res.string.section_installation) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/components/sections/Installation.kt`
around lines 61 - 63, The code is uppercasing a localized string at runtime
which is locale-sensitive; remove the .uppercase() call on
stringResource(Res.string.section_installation) in the SectionHeader invocation
and instead provide the correctly-cased version in your localized resources (or
keep the original casing in resources), i.e., pass
stringResource(Res.string.section_installation) directly to SectionHeader and
update the Res.string.section_installation translations per locale to include
any desired uppercase styling.
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.
feat(android): Add Shizuku-based silent installation support
ShizukuInstallerServiceImplusing Shizuku UserService for privileged package operations (install/uninstall).ShizukuServiceManagerto manage Shizuku lifecycle, status detection, and permission requests.ShizukuInstallerWrapperto delegate installation tasks to Shizuku when enabled and available, falling back to the standard installer on failure.Profilescreen with a new "Installation" section to allow users to toggle between Default and Shizuku installer types.InstallerStatusProviderandInstallerTypeto core domain to handle platform-agnostic installer state.ThemesRepository.Summary by CodeRabbit