fix: prevent setup auto-dismiss, improve settings UX#9
Merged
Conversation
- Add setupCompleted flag to DataStore so setup screen stays until user explicitly clicks Continue/Skip (no more auto-redirect after granting the last permission) - Migration: existing installs with server URL set are auto-marked as setup completed - Settings: remove notification/usage access buttons (handled in setup), keep only battery optimization with state-aware text - Settings: fix battery intent to use direct per-app prompt - Settings: add GitHub icon to About section buttons - Update string resources for battery state and about labels
📝 WalkthroughWalkthroughAdded a persisted Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Setup as SetupScreen
participant VM as SettingsViewModel
participant Store as SettingsStore
participant Main as MainActivity
User->>Setup: tap "Continue"/"Skip"
Setup->>VM: updateSettings(serverUrl, apiKey, setupCompleted=true)
VM->>Store: write preferences (server_url, api_key, setup_completed)
Store-->>VM: confirm write
Store-->>Main: emit updated Settings (setupCompleted = true)
Main->>Main: hide SetupScreen, show main UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.kt (1)
190-201: Consider extracting shared completion action to avoid drift.Lines 190 and 201 duplicate the same
updateSettingsblock. A small local lambda would keep Continue/Skip behavior synchronized.Refactor sketch
+ val completeSetup = { + viewModel.updateSettings { + it.copy(serverUrl = localUrl, apiKey = localKey, setupCompleted = true) + } + onComplete() + } Button( onClick = { - viewModel.updateSettings { it.copy(serverUrl = localUrl, apiKey = localKey, setupCompleted = true) } - onComplete() + completeSetup() }, ) { ... } TextButton( onClick = { - viewModel.updateSettings { it.copy(serverUrl = localUrl, apiKey = localKey, setupCompleted = true) } - onComplete() + completeSetup() }, ) { ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.kt` around lines 190 - 201, Extract the duplicated completion logic into a single local lambda and call it from both buttons so their behavior stays synchronized: create a val like completeAction = { viewModel.updateSettings { it.copy(serverUrl = localUrl, apiKey = localKey, setupCompleted = true) }; onComplete() } and replace the two inline updateSettings/onComplete blocks in the Continue and Skip TextButton onClick handlers with a call to completeAction; reference viewModel.updateSettings, localUrl, localKey, setupCompleted, and onComplete when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.kt`:
- Around line 190-201: Extract the duplicated completion logic into a single
local lambda and call it from both buttons so their behavior stays synchronized:
create a val like completeAction = { viewModel.updateSettings {
it.copy(serverUrl = localUrl, apiKey = localKey, setupCompleted = true) };
onComplete() } and replace the two inline updateSettings/onComplete blocks in
the Continue and Skip TextButton onClick handlers with a call to completeAction;
reference viewModel.updateSettings, localUrl, localKey, setupCompleted, and
onComplete when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35f28dee-435b-4225-b985-f84baa3c4d88
📒 Files selected for processing (7)
app/src/main/java/com/cashpilot/android/model/Settings.ktapp/src/main/java/com/cashpilot/android/ui/MainActivity.ktapp/src/main/java/com/cashpilot/android/ui/screen/SettingsScreen.ktapp/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.ktapp/src/main/java/com/cashpilot/android/util/SettingsStore.ktapp/src/main/res/drawable/ic_github.xmlapp/src/main/res/values/strings.xml
- Migration now requires both server URL and API key to be non-empty before marking setupCompleted (prevents skipping wizard for partial configs) - Extract finishSetup lambda in SetupScreen so Continue and Skip stay in sync
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.kt (1)
80-83: Consider refactoringfinishSetupto await DataStore persistence before callingonComplete.
updateSettings()launches a coroutine and returns immediately (fire-and-forget pattern), meaningonComplete()executes before the DataStore write completes. While the current code is safe—onCompleteis an empty lambda and navigation is driven by thesettings.setupCompletedFlow state—this pattern is fragile ifonCompletegains meaningful logic in the future.For robustness, expose a suspend variant in
MainViewModel:suspend fun updateSettingsAwait(transform: (Settings) -> Settings) { SettingsStore.update(getApplication(), transform) }Then await it in the composable:
val scope = rememberCoroutineScope() val finishSetup: () -> Unit = { scope.launch { viewModel.updateSettingsAwait { it.copy(serverUrl = localUrl, apiKey = localKey, setupCompleted = true) } onComplete() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.kt` around lines 80 - 83, The finishSetup lambda currently calls viewModel.updateSettings (which returns immediately) and then onComplete, risking onComplete firing before DataStore write finishes; add a suspend API on MainViewModel (e.g., suspend fun updateSettingsAwait(transform: (Settings) -> Settings) that internally calls SettingsStore.update(getApplication(), transform)), then in SetupScreen use rememberCoroutineScope and replace the current finishSetup with a coroutine: scope.launch { viewModel.updateSettingsAwait { it.copy(serverUrl = localUrl, apiKey = localKey, setupCompleted = true) }; onComplete() } so onComplete runs only after persistence completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.kt`:
- Around line 80-83: The finishSetup lambda currently calls
viewModel.updateSettings (which returns immediately) and then onComplete,
risking onComplete firing before DataStore write finishes; add a suspend API on
MainViewModel (e.g., suspend fun updateSettingsAwait(transform: (Settings) ->
Settings) that internally calls SettingsStore.update(getApplication(),
transform)), then in SetupScreen use rememberCoroutineScope and replace the
current finishSetup with a coroutine: scope.launch {
viewModel.updateSettingsAwait { it.copy(serverUrl = localUrl, apiKey = localKey,
setupCompleted = true) }; onComplete() } so onComplete runs only after
persistence completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b236fa0-a480-46ef-9999-0376b40af6e7
📒 Files selected for processing (2)
app/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.ktapp/src/main/java/com/cashpilot/android/util/SettingsStore.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/cashpilot/android/util/SettingsStore.kt
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
setupCompletedflag in DataStore (with migration for existing installs).ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONSprompt instead of global settings list.Test plan
Summary by CodeRabbit
New Features
Improvements