Code-review follow-up: release hardening, correctness and cleanup#21
Merged
Conversation
The core:service module doesn't depend on AppCompat/Material, so ?attr/colorControlNormal failed aapt verification in release builds. Notifications re-tint the small icon with the system accent anyway — the attribute was a no-op at runtime. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
staticCompositionLocalOf invalidates every reader on any change. rememberAnimatedAppTheme produces a new AppTheme instance every frame during the 350ms theme transition (24 animateColorAsState values), so with the static local the whole subtree recomposed ~21 times per theme swap. compositionLocalOf limits invalidation to readers of the changed fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
StarField was assigning dpToPx from inside a Canvas draw lambda while a LaunchedEffect read the same value — the "state-write-from-draw" anti-pattern that can cause invalidation loops. Read LocalDensity once in the composable body and capture the pixel value as a plain val. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
IRemoteProcess.waitFor() is a blocking binder call — if the Shizuku server wedges, the coroutine would block indefinitely. Wrap in withTimeout(5_000) and treat TimeoutCancellationException as a silent false so timer cancel can't hang on a stuck binder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Device admin can be revoked from system Settings without any broadcast the app is subscribed to — the "Screen" row stayed at the old state until another unrelated emission happened. Add an adminRefreshTicker StateFlow to the VM's combine source and bump it from a LifecycleEventEffect(ON_RESUME) in SettingsScreen so returning from Settings → Security → Device admin reflects reality. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Defaults were duplicated between the UserSettings data class and the per-key ?: fallbacks in the repository. Single source of truth: instantiate UserSettings() once and read its fields as the ?: side. Adding a new preference now only requires updating the data class. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Notification strings lived in both app/res and core/service/res (with the service copy as <plurals>, app as <string>). The service reads from its own R class, so the app copies were just dead weight. ServiceModule had no @Provides/@BINDS methods — all dependencies are @Inject constructable @singletons. Dropping the empty module. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- <queries> entry for moe.shizuku.privileged.api so PackageManager.getPackageInfo() can see Shizuku under targetSdk 30+ package visibility rules. - dataExtractionRules excluding the settings DataStore from cloud-backup only. Device-transfer is still allowed so prefs follow the user on local migration, but a cold cloud restore onto a different account/device won't carry over toggles whose backing grants (device admin, Shizuku) are device-local. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
updateState was exposed on the TimerRepository interface, so any ViewModel could overwrite service-owned timer state. Move mutation to TimerRepositoryImpl as a non-interface public method and inject the concrete impl only where mutation is legitimate (currently SleepTimerService). ViewModels keep their read-only interface binding via Hilt and can no longer desync the foreground service. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
If the OS recreates the service from a stale notification action PendingIntent after process death (ACTION_ADD_MINUTES / SUBTRACT / CANCEL), the old code never called startForeground and crashed with ForegroundServiceDidNotStartInTimeException inside the 5s foreground-start window. Bail out early with stopSelf() when a non-START action arrives without an active countdown. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two problems under minified release builds: 1. Navigation route objects (@serializable data object) rely on generated $serializer companions that R8 stripped without explicit keep rules. Added targeted rules for dev.xitee.sleeptimer.navigation.** plus a keep for IShizukuService AIDL stub used by ShizukuShell. 2. TimerViewModel and TimerNotificationManager built Intents for SleepTimerService from hardcoded string literals. Services declared in the manifest are kept by default Android rules, so this didn't actually break — but it's fragile and a future class rename would silently fail. Replaced with SleepTimerService::class.java.name, consolidated Intent construction into a small serviceIntent() helper, and dropped the duplicated action constants in TimerViewModel's companion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The dial called onMinutesChanged on every drag tick, and the VM persisted to DataStore on every call — 30+ writes per drag. Split setMinutes (UI-only, for drag ticks) from commitMinutes (UI + DataStore write, for +/- step buttons and dial onDragEnd). One persist per gesture instead of tens. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to a code review of the vibe-coded initial pass. Addresses correctness, release-build robustness, and minor cleanup. No user-visible behavior changes except the device-admin row now updates on resume.
Split into 12 focused commits. Each was individually verified to compile; final commit verified with `./gradlew assembleRelease` (R8 + resource shrinking).
Notable fixes
Release blockers:
Service robustness:
Compose correctness/perf:
UX:
Manifest:
Cleanup:
Test plan
🤖 Generated with Claude Code