feat: fleet-at-a-glance dashboard redesign#4
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughMade AppDetector's package check public; added heartbeat outcome StateFlows; MainViewModel now exposes apps/summary, permission and heartbeat state, and uses cancellable async refresh; MainActivity refreshes on each resume; Dashboard rebuilt as a 2-column grid with pull/periodic refresh, permission banner and new per-app UI. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MainActivity
participant Lifecycle
participant MainViewModel
participant AppDetector
participant HeartbeatService
participant Dashboard
User->>MainActivity: open / return to app
MainActivity->>Lifecycle: enter RESUMED
Lifecycle->>MainViewModel: trigger refreshStatuses()
MainViewModel->>AppDetector: isInstalled(slug)
MainViewModel->>HeartbeatService: read lastHeartbeat / lastHeartbeatFailed
MainViewModel->>MainViewModel: compute apps list & summary
MainViewModel->>Dashboard: emit apps, summary, isRefreshing, permission states
Dashboard->>User: recompose UI (grid, summary, permission banner)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/cashpilot/android/ui/MainViewModel.kt (1)
97-104:⚠️ Potential issue | 🟠 MajorRace condition:
refreshStatuses()runs before settings are persisted.
updateSettingslaunches a coroutine to persist changes asynchronously, butrefreshStatuses()is called immediately after and readssettings.value. The new enabled/disabled state won't be reflected yet, causing the UI to show stale data until the next refresh.🐛 Proposed fix: Refresh after settings update completes
fun toggleApp(slug: String) { - updateSettings { s -> + viewModelScope.launch { + SettingsStore.update(getApplication()) { s -> val new = s.enabledSlugs.toMutableSet() if (slug in new) new.remove(slug) else new.add(slug) s.copy(enabledSlugs = new) + } + refreshStatuses() } - refreshStatuses() }🤖 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/MainViewModel.kt` around lines 97 - 104, toggleApp currently calls refreshStatuses immediately after calling updateSettings, causing a race because updateSettings persists asynchronously; change the flow so refreshStatuses runs only after the settings persistence completes. Update the updateSettings API (or add a callback/return a Deferred/mark it suspend) so callers can await completion, then in toggleApp await updateSettings(...) (or pass a completion callback) and call refreshStatuses after that await so refreshStatuses reads the persisted settings.value; reference the existing functions toggleApp, updateSettings and refreshStatuses when making the change.
🧹 Nitpick comments (4)
app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt (1)
306-312: Permission checks during composition may cause recomposition issues.
hasNotificationAccess()andhasUsageAccess()are called directly during composition without being remembered. If these methods have side effects or are slow, they'll run on every recomposition. Consider remembering the results or moving them to ViewModel state.♻️ Proposed refactor
`@Composable` private fun PermissionBanner(viewModel: MainViewModel) { - val hasNotif = viewModel.hasNotificationAccess() - val hasUsage = viewModel.hasUsageAccess() + // Remember permission state and update on resume via key + val lifecycleOwner = LocalLifecycleOwner.current + var hasNotif by remember { mutableStateOf(viewModel.hasNotificationAccess()) } + var hasUsage by remember { mutableStateOf(viewModel.hasUsageAccess()) } + + LaunchedEffect(lifecycleOwner) { + lifecycleOwner.lifecycle.repeatOnLifecycle(Lifecycle.State.RESUMED) { + hasNotif = viewModel.hasNotificationAccess() + hasUsage = viewModel.hasUsageAccess() + } + }🤖 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/DashboardScreen.kt` around lines 306 - 312, PermissionBanner currently calls viewModel.hasNotificationAccess() and viewModel.hasUsageAccess() directly during composition which can run on every recomposition; change this so these checks are read from stable Compose state (either expose them as State/LiveData/Flow from MainViewModel and collectAsState in PermissionBanner, or remember the results once using remember/rememberSaveable and update from ViewModel side). Specifically, modify PermissionBanner to stop invoking hasNotificationAccess() and hasUsageAccess() inline and instead read remembered/observed state provided by MainViewModel (refer to PermissionBanner and the methods hasNotificationAccess / hasUsageAccess) so the permission checks don’t execute on every recomposition.app/src/main/java/com/cashpilot/android/service/HeartbeatService.kt (1)
172-179: Companion object StateFlows persist across service lifecycle.Using companion object for
lastHeartbeatandlastHeartbeatFailedmeans these values survive service restarts and aren't reset whenHeartbeatServiceis destroyed/recreated. While this is generally acceptable for a monitoring dashboard (stale heartbeat times will naturally age out in the UI), be aware thatlastHeartbeatFailedcould show a stale failure state if the service was killed before a successful retry.Consider resetting
_lastHeartbeatFailed.value = falseinonCreate()if you want fresh failure state on service restart.🤖 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/service/HeartbeatService.kt` around lines 172 - 179, The companion-held StateFlow _lastHeartbeatFailed retains its value across service restarts causing stale failure state; in HeartbeatService.onCreate() reset the failure state by setting _lastHeartbeatFailed.value = false (and optionally reset _lastHeartbeat.value if you want a fresh timestamp) so the service starts with a clean state; locate the companion/object where _lastHeartbeatFailed and _lastHeartbeat are declared and add the reset call(s) in the HeartbeatService.onCreate() method.app/src/main/java/com/cashpilot/android/ui/MainViewModel.kt (2)
58-89: Consider movingrefreshStatuses()off the main thread.
detector.detectAll()anddetector.isInstalled()perform system queries (UsageStatsManager,NetworkStatsManager,PackageManager) that may block the main thread. SincerefreshStatuses()is called from UI events (pull-to-refresh, lifecycle resume), this could cause UI jank.♻️ Proposed refactor: Run detection on IO dispatcher
fun refreshStatuses() { + viewModelScope.launch(Dispatchers.IO) { val enabled = settings.value.enabledSlugs val detected = detector.detectAll(enabled).associateBy { it.slug } val displayList = KnownApps.all.map { app -> // ... existing logic } val sorted = displayList.sortedBy { it.state.ordinal } - _apps.value = sorted - _summary.value = FleetSummary(...) + _apps.value = sorted + _summary.value = FleetSummary( + running = sorted.count { it.state == AppState.RUNNING }, + stopped = sorted.count { it.state == AppState.STOPPED }, + notInstalled = sorted.count { it.state == AppState.NOT_INSTALLED }, + disabled = sorted.count { it.state == AppState.DISABLED }, + totalTx = sorted.mapNotNull { it.status?.netTx24h }.sum(), + totalRx = sorted.mapNotNull { it.status?.netRx24h }.sum(), + ) + } }Note: If you do this, ensure concurrent calls are handled (e.g., use a
Mutexor cancel previous job).🤖 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/MainViewModel.kt` around lines 58 - 89, refreshStatuses() performs blocking system queries (detector.detectAll and detector.isInstalled) on the main thread; move the heavy work into a background coroutine (e.g., use viewModelScope.launch(Dispatchers.IO)) to run detection and list building off the UI thread, then switch back to the main thread to update _apps and _summary (or use postValue if LiveData). Ensure concurrent invocations are handled by cancelling any previous refresh job or protecting the work with a Mutex/atomic Job field so multiple calls (from pull-to-refresh or onResume) don’t overlap; update references to refreshStatuses(), detector.detectAll, detector.isInstalled, _apps, _summary, and FleetSummary accordingly.
106-127: Permission check methods query system APIs synchronously.
hasNotificationAccess()andhasUsageAccess()are called directly from composables (PermissionBanner). WhilehasNotificationAccess()just reads a system setting string,hasUsageAccess()queriesUsageStatsManagerwhich could be slow. Consider caching these results or making them reactive StateFlows that update on lifecycle resume.🤖 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/MainViewModel.kt` around lines 106 - 127, The permission checks hasNotificationAccess() and hasUsageAccess() should not be called synchronously from composables; convert them into cached reactive properties (e.g., private MutableStateFlow<Boolean> and public StateFlow<Boolean>) on MainViewModel, populate/update their values from viewModelScope using a background dispatcher (Dispatchers.IO) and refresh them on lifecycle resume (via a LifecycleObserver or a resume callback) so composables observe the StateFlows instead of calling hasNotificationAccess()/hasUsageAccess() directly; keep the original methods for one-time checks if needed but ensure PermissionBanner reads the exposed StateFlows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt`:
- Around line 268-271: Several user-facing hardcoded strings in
DashboardScreen.kt (the Text composable showing "Not connected — configure
server" and the labels "Permissions needed", "Grant Notification Access", "Grant
Usage Access", and status labels "Running"/"Stopped"/"Disabled"/"Not installed")
must be moved to strings.xml for i18n; create appropriate string resource
entries (e.g., dashboard_not_connected, permissions_needed,
grant_notification_access, grant_usage_access, status_running, status_stopped,
status_disabled, status_not_installed) and replace the literal usages in the
DashboardScreen composable with stringResource(R.string.<name>); also add the
import for androidx.compose.ui.res.stringResource if missing.
- Around line 99-105: The refresh indicator is flipped true then false
immediately so the animation won't show; change the onRefresh handler for
PullToRefreshBox to await actual work instead of setting isRefreshing=false
synchronously: either make MainViewModel.refreshStatuses a suspend function and
call it from a coroutine (launch/rememberCoroutineScope) and only set
isRefreshing=false after await, or better expose a isLoading: StateFlow<Boolean>
in MainViewModel and bind PullToRefreshBox's isRefreshing to that StateFlow
(observe it in DashboardScreen) so the UI reflects real async work from
viewModel.refreshStatuses.
---
Outside diff comments:
In `@app/src/main/java/com/cashpilot/android/ui/MainViewModel.kt`:
- Around line 97-104: toggleApp currently calls refreshStatuses immediately
after calling updateSettings, causing a race because updateSettings persists
asynchronously; change the flow so refreshStatuses runs only after the settings
persistence completes. Update the updateSettings API (or add a callback/return a
Deferred/mark it suspend) so callers can await completion, then in toggleApp
await updateSettings(...) (or pass a completion callback) and call
refreshStatuses after that await so refreshStatuses reads the persisted
settings.value; reference the existing functions toggleApp, updateSettings and
refreshStatuses when making the change.
---
Nitpick comments:
In `@app/src/main/java/com/cashpilot/android/service/HeartbeatService.kt`:
- Around line 172-179: The companion-held StateFlow _lastHeartbeatFailed retains
its value across service restarts causing stale failure state; in
HeartbeatService.onCreate() reset the failure state by setting
_lastHeartbeatFailed.value = false (and optionally reset _lastHeartbeat.value if
you want a fresh timestamp) so the service starts with a clean state; locate the
companion/object where _lastHeartbeatFailed and _lastHeartbeat are declared and
add the reset call(s) in the HeartbeatService.onCreate() method.
In `@app/src/main/java/com/cashpilot/android/ui/MainViewModel.kt`:
- Around line 58-89: refreshStatuses() performs blocking system queries
(detector.detectAll and detector.isInstalled) on the main thread; move the heavy
work into a background coroutine (e.g., use
viewModelScope.launch(Dispatchers.IO)) to run detection and list building off
the UI thread, then switch back to the main thread to update _apps and _summary
(or use postValue if LiveData). Ensure concurrent invocations are handled by
cancelling any previous refresh job or protecting the work with a Mutex/atomic
Job field so multiple calls (from pull-to-refresh or onResume) don’t overlap;
update references to refreshStatuses(), detector.detectAll,
detector.isInstalled, _apps, _summary, and FleetSummary accordingly.
- Around line 106-127: The permission checks hasNotificationAccess() and
hasUsageAccess() should not be called synchronously from composables; convert
them into cached reactive properties (e.g., private MutableStateFlow<Boolean>
and public StateFlow<Boolean>) on MainViewModel, populate/update their values
from viewModelScope using a background dispatcher (Dispatchers.IO) and refresh
them on lifecycle resume (via a LifecycleObserver or a resume callback) so
composables observe the StateFlows instead of calling
hasNotificationAccess()/hasUsageAccess() directly; keep the original methods for
one-time checks if needed but ensure PermissionBanner reads the exposed
StateFlows.
In `@app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt`:
- Around line 306-312: PermissionBanner currently calls
viewModel.hasNotificationAccess() and viewModel.hasUsageAccess() directly during
composition which can run on every recomposition; change this so these checks
are read from stable Compose state (either expose them as State/LiveData/Flow
from MainViewModel and collectAsState in PermissionBanner, or remember the
results once using remember/rememberSaveable and update from ViewModel side).
Specifically, modify PermissionBanner to stop invoking hasNotificationAccess()
and hasUsageAccess() inline and instead read remembered/observed state provided
by MainViewModel (refer to PermissionBanner and the methods
hasNotificationAccess / hasUsageAccess) so the permission checks don’t execute
on every recomposition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e75564e-da34-4da7-ae66-bb302a6cf156
📒 Files selected for processing (5)
app/src/main/java/com/cashpilot/android/service/AppDetector.ktapp/src/main/java/com/cashpilot/android/service/HeartbeatService.ktapp/src/main/java/com/cashpilot/android/ui/MainActivity.ktapp/src/main/java/com/cashpilot/android/ui/MainViewModel.ktapp/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/cashpilot/android/ui/MainViewModel.kt`:
- Around line 69-70: Multiple concurrent triggers call refreshStatuses() which
launches independent coroutines that race in doRefresh(), causing conflicts on
shared state (_apps, _summary) and incorrect _isRefreshing clearing; change the
implementation to serialize/coalesce refreshes by creating a single-flight
mechanism (e.g., a shared nullable Job or a Mutex/Mutex.withLock and a single
coroutine Deferred) for doRefresh() so that
refreshStatuses()/toggleApp()/timer/resume either return early or await the
existing refresh job instead of starting a new one, and ensure the shared
loading flag (_isRefreshing) is set before the single-flight run and only
cleared after that single job completes; update references in refreshStatuses(),
doRefresh(), toggleApp(), and the block that currently sets/clears _isRefreshing
to use this single-flight job/mutex.
- Around line 73-82: In toggleApp, avoid reading the possibly-stale settings
StateFlow after SettingsStore.update by computing the new enabled set locally
and passing it into doRefresh; specifically, inside toggleApp compute val newSet
= s.enabledSlugs.toMutableSet(); apply the add/remove logic on newSet, call
SettingsStore.update(...) to persist the change using s.copy(enabledSlugs =
newSet), and then call doRefresh(newSet) (or add an overload/optional parameter
to doRefresh) so doRefresh uses the freshly computed set instead of
settings.value.enabledSlugs.
In `@app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt`:
- Around line 123-126: The PermissionBanner dismissed state should be lifted
into the ViewModel and persisted instead of using rememberSaveable in the
PermissionBanner composable: add a bannerDismissed Boolean to the ViewModel UI
state (like hasNotificationAccess/hasUsageAccess), wire it to SettingsStore so
it survives cold starts, remove the rememberSaveable dismissed from
PermissionBanner, and update the dismiss button handler (currently updating
local state around line ~365) to call a ViewModel function (e.g.,
setBannerDismissed(true) or dismissBanner()) that updates and persists
bannerDismissed; finally, in DashboardScreen where the grid item is emitted,
wrap the item(span = { GridItemSpan(2) }) { PermissionBanner(viewModel) } in a
conditional check (if !bannerDismissed && !(hasNotificationAccess &&
hasUsageAccess)) so the grid cell is only created when the banner should be
shown.
- Around line 79-85: The 30-second refresh loop in DashboardScreen is
composition-scoped via LaunchedEffect(Unit) and should be lifecycle-scoped;
replace the infinite LaunchedEffect loop that calls viewModel.refreshStatuses()
with a lifecycle-aware coroutine using
lifecycle.repeatOnLifecycle(Lifecycle.State.RESUMED) (or the existing
Activity/Owner lifecycleScope pattern used in MainActivity.kt) so
refreshStatuses() runs only while the UI is visible/resumed; locate the
LaunchedEffect block in the DashboardScreen composable and move its logic into a
lifecycle.repeatOnLifecycle coroutine that delays 30_000 and calls
viewModel.refreshStatuses() inside the RESUMED block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d498852-5000-4d79-89b1-bbe616be31b2
📒 Files selected for processing (2)
app/src/main/java/com/cashpilot/android/ui/MainViewModel.ktapp/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt
Comment on lines
+73
to
+82
| fun toggleApp(slug: String) { | ||
| viewModelScope.launch { | ||
| SettingsStore.update(getApplication()) { s -> | ||
| val new = s.enabledSlugs.toMutableSet() | ||
| if (slug in new) new.remove(slug) else new.add(slug) | ||
| s.copy(enabledSlugs = new) | ||
| } | ||
| // withContext(IO) yields to main, letting the settings StateFlow update | ||
| doRefresh() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine SettingsStore.kt
fd -t f 'SettingsStore.kt' -x cat -n {}Repository: GeiserX/CashPilot-android
Length of output: 2580
🏁 Script executed:
# Find and examine MainViewModel.kt to verify the full context
fd -t f 'MainViewModel.kt' -x cat -n {}Repository: GeiserX/CashPilot-android
Length of output: 6888
🏁 Script executed:
# Check DataStore implementation details and behavior
rg -t kotlin 'DataStore|StateFlow' -A 3 -B 1 | head -100Repository: GeiserX/CashPilot-android
Length of output: 7209
🏁 Script executed:
# Check for other similar patterns where doRefresh or similar operations read settings.value after updates
rg -t kotlin 'SettingsStore\.update|settings\.value' -B 3 -A 3Repository: GeiserX/CashPilot-android
Length of output: 2142
Pass the freshly computed enabled set into doRefresh() to avoid stale reads.
Line 89 calls doRefresh() immediately after SettingsStore.update(...) completes, but line 96 re-reads settings.value.enabledSlugs from a StateFlow backed by DataStore. Although the write is persisted when update() returns, the StateFlow may not have emitted the updated value yet. The toggled app can remain in its previous enabled/disabled state until the StateFlow catches up. Capture the computed slug set before storing and pass it to doRefresh(...) to ensure immediate consistency.
💡 Example fix
fun toggleApp(slug: String) {
viewModelScope.launch {
+ var enabledAfterToggle: Set<String> = emptySet()
SettingsStore.update(getApplication()) { s ->
val new = s.enabledSlugs.toMutableSet()
if (slug in new) new.remove(slug) else new.add(slug)
- s.copy(enabledSlugs = new)
+ enabledAfterToggle = new.toSet()
+ s.copy(enabledSlugs = enabledAfterToggle)
}
- // withContext(IO) yields to main, letting the settings StateFlow update
- doRefresh()
+ doRefresh(enabledAfterToggle)
}
}
-private suspend fun doRefresh() {
+private suspend fun doRefresh(enabled: Set<String> = settings.value.enabledSlugs) {
_isRefreshing.value = true
withContext(Dispatchers.IO) {
- val enabled = settings.value.enabledSlugs
val detected = detector.detectAll(enabled).associateBy { it.slug }🤖 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/MainViewModel.kt` around lines 73
- 82, In toggleApp, avoid reading the possibly-stale settings StateFlow after
SettingsStore.update by computing the new enabled set locally and passing it
into doRefresh; specifically, inside toggleApp compute val newSet =
s.enabledSlugs.toMutableSet(); apply the add/remove logic on newSet, call
SettingsStore.update(...) to persist the change using s.copy(enabledSlugs =
newSet), and then call doRefresh(newSet) (or add an overload/optional parameter
to doRefresh) so doRefresh uses the freshly computed set instead of
settings.value.enabledSlugs.
Comment on lines
+79
to
+85
| // Auto-refresh every 30s while visible | ||
| LaunchedEffect(Unit) { | ||
| while (true) { | ||
| delay(30_000) | ||
| viewModel.refreshStatuses() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the DashboardScreen.kt file
find . -name "DashboardScreen.kt" -type fRepository: GeiserX/CashPilot-android
Length of output: 139
🏁 Script executed:
# Read the DashboardScreen.kt file to see full context
cat -n ./app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt | head -120Repository: GeiserX/CashPilot-android
Length of output: 6089
🏁 Script executed:
# Continue reading the file to see the complete composition and understand the full context
cat -n ./app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt | tail -n +120 | head -50Repository: GeiserX/CashPilot-android
Length of output: 2062
🏁 Script executed:
# Search for other lifecycle handling, DisposableEffect, or similar patterns in the file
rg -n "DisposableEffect|rememberCoroutineScope|LaunchedEffect|lifecycle|Activity" ./app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.ktRepository: GeiserX/CashPilot-android
Length of output: 258
🏁 Script executed:
# Check for any lifecycle-aware implementations in the codebase
rg -n "Lifecycle\.|LocalLifecycleOwner\|LifecycleEventObserver" app/src/main/java/com/cashpilot/android/ -t kt | head -20Repository: GeiserX/CashPilot-android
Length of output: 95
🏁 Script executed:
# Check for any lifecycle-aware implementations in the codebase
rg -n "Lifecycle\.|LocalLifecycleOwner|LifecycleEventObserver" app/src/main/java/com/cashpilot/android/Repository: GeiserX/CashPilot-android
Length of output: 197
🏁 Script executed:
# Let me check MainActivity.kt to see the lifecycle pattern being used there
cat -n app/src/main/java/com/cashpilot/android/ui/MainActivity.kt | head -60Repository: GeiserX/CashPilot-android
Length of output: 2726
🏁 Script executed:
# Check if there are any other references to refreshStatuses to understand the full refresh pattern
rg -n "refreshStatuses" app/src/main/java/com/cashpilot/android/Repository: GeiserX/CashPilot-android
Length of output: 507
🏁 Script executed:
# Let me check if there are any DisposableEffect, snapshotFlow, or other lifecycle patterns in other screens
find app/src/main/java/com/cashpilot/android/ui/screen -name "*.kt" -type f | xargs rg -l "DisposableEffect|collectAsStateWithLifecycle|LaunchedEffect"Repository: GeiserX/CashPilot-android
Length of output: 137
🏁 Script executed:
# Check the Jetpack Compose documentation pattern - let me search for any comments or patterns that suggest how periodic tasks should be handled
rg -n "while.*true\|infinite.*loop\|periodic" app/src/main/java/com/cashpilot/android/Repository: GeiserX/CashPilot-android
Length of output: 51
🏁 Script executed:
# Let me understand the context better - check if DashboardScreen is conditionally shown
cat -n app/src/main/java/com/cashpilot/android/ui/MainActivity.kt | sed -n '49,70p'Repository: GeiserX/CashPilot-android
Length of output: 925
Scope the 30-second refresh loop to the lifecycle state.
The LaunchedEffect(Unit) on line 80 launches a coroutine that is composition-scoped, not lifecycle-scoped. While the DashboardScreen composable remains in composition (e.g., when the activity is paused but the UI is not torn down), the infinite loop continues calling refreshStatuses() every 30 seconds in the background, contradicting the "while visible" intent. Use lifecycle-aware handling (e.g., lifecycle.repeatOnLifecycle(Lifecycle.State.RESUMED)) instead, following the pattern already established in MainActivity.kt.
🤖 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/DashboardScreen.kt` around
lines 79 - 85, The 30-second refresh loop in DashboardScreen is
composition-scoped via LaunchedEffect(Unit) and should be lifecycle-scoped;
replace the infinite LaunchedEffect loop that calls viewModel.refreshStatuses()
with a lifecycle-aware coroutine using
lifecycle.repeatOnLifecycle(Lifecycle.State.RESUMED) (or the existing
Activity/Owner lifecycleScope pattern used in MainActivity.kt) so
refreshStatuses() runs only while the UI is visible/resumed; locate the
LaunchedEffect block in the DashboardScreen composable and move its logic into a
lifecycle.repeatOnLifecycle coroutine that delays 30_000 and calls
viewModel.refreshStatuses() inside the RESUMED block.
Comment on lines
+123
to
+126
| // -- Permission banner (full width) -- | ||
| item(span = { GridItemSpan(2) }) { | ||
| PermissionBanner(viewModel) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and check the file
find . -path "**/DashboardScreen.kt" -type fRepository: GeiserX/CashPilot-android
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Check file size
wc -l ./app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.ktRepository: GeiserX/CashPilot-android
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Read the critical sections with context
echo "=== Lines 110-135 (context around 123-126) ==="
sed -n '110,135p' ./app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt
echo -e "\n=== Lines 290-320 (context around 302-309) ==="
sed -n '290,320p' ./app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt
echo -e "\n=== Lines 360-375 (context around 365-366) ==="
sed -n '360,375p' ./app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.ktRepository: GeiserX/CashPilot-android
Length of output: 2669
🏁 Script executed:
#!/bin/bash
# Check for SettingsStore in the codebase
echo "=== Searching for SettingsStore ==="
find . -name "*SettingsStore*" -o -name "*Settings*" | grep -E "\.kt$" | head -20
echo -e "\n=== Searching for state persistence patterns ==="
rg "rememberSaveable|dataStore|SharedPreferences" --type kt -l | head -10Repository: GeiserX/CashPilot-android
Length of output: 392
🏁 Script executed:
#!/bin/bash
# Read SettingsStore implementation
echo "=== SettingsStore.kt ==="
cat -n ./app/src/main/java/com/cashpilot/android/util/SettingsStore.kt
echo -e "\n=== Settings.kt ==="
cat -n ./app/src/main/java/com/cashpilot/android/model/Settings.ktRepository: GeiserX/CashPilot-android
Length of output: 3005
🏁 Script executed:
#!/bin/bash
# Find and read MainViewModel
find . -name "MainViewModel*" -type f | head -5Repository: GeiserX/CashPilot-android
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Check size of MainViewModel
wc -l ./app/src/main/java/com/cashpilot/android/ui/MainViewModel.kt
# Read the full file
cat -n ./app/src/main/java/com/cashpilot/android/ui/MainViewModel.ktRepository: GeiserX/CashPilot-android
Length of output: 6954
Move banner visibility state to ViewModel and only render grid item when banner should be shown.
The dismissed state in PermissionBanner uses rememberSaveable, which is lost on cold start when the app process is killed. Additionally, the item(span = { GridItemSpan(2) }) always reserves grid space even when the banner's early return leaves it empty, creating a blank row.
Add a bannerDismissed field to the ViewModel's state (similar to hasNotificationAccess/hasUsageAccess), persist it via SettingsStore, and conditionally emit the grid item only when the banner should be shown:
if (!dismissedBanner && !(hasNotif && hasUsage)) {
item(span = { GridItemSpan(2) }) {
PermissionBanner(viewModel)
}
}Related code locations
- 302-309: PermissionBanner composable with rememberSaveable
- 365-366: Dismiss button that updates local state
🤖 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/DashboardScreen.kt` around
lines 123 - 126, The PermissionBanner dismissed state should be lifted into the
ViewModel and persisted instead of using rememberSaveable in the
PermissionBanner composable: add a bannerDismissed Boolean to the ViewModel UI
state (like hasNotificationAccess/hasUsageAccess), wire it to SettingsStore so
it survives cold starts, remove the rememberSaveable dismissed from
PermissionBanner, and update the dismiss button handler (currently updating
local state around line ~365) to call a ViewModel function (e.g.,
setBannerDismissed(true) or dismissBanner()) that updates and persists
bannerDismissed; finally, in DashboardScreen where the grid item is emitted,
wrap the item(span = { GridItemSpan(2) }) { PermissionBanner(viewModel) } in a
conditional check (if !bannerDismissed && !(hasNotificationAccess &&
hasUsageAccess)) so the grid cell is only created when the banner should be
shown.
- Show all 17 known apps in a 2-column grid (not just enabled+installed) - Four distinct visual states: Running (green), Stopped (red), Disabled (gray), Not Installed (dimmed) - Summary header with running/stopped/N/A counts, total 24h bandwidth, and live heartbeat status with animated indicator dot - Permission banner auto-detects missing notification/usage access and links directly to system settings; dismissible - Pull-to-refresh replaces FAB for natural refresh gesture - Auto-refresh every 30s while screen is visible (LaunchedEffect) - Refresh on every activity resume (repeatOnLifecycle RESUMED) so returning from permission screens updates immediately - AppDetector.isInstalled() exposed publicly for composite state - HeartbeatService tracks last heartbeat timestamp and failure state via companion StateFlows for UI consumption - Apps sorted by state: running first, then stopped, disabled, N/A
- Move app detection to Dispatchers.IO via private suspend doRefresh() - Fix toggleApp() race: await DataStore write before refreshing - Convert permission checks to reactive StateFlows updated on refresh - Expose isRefreshing StateFlow for proper pull-to-refresh state
- Track refreshJob and cancel previous before launching new one - Prevents stale results from overwriting newer state - Run checkPermissions() in init to avoid false-negative flash on cold start
GeiserX
force-pushed
the
feat/dashboard-redesign
branch
from
April 4, 2026 12:31
5321d23 to
d044d6b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt (1)
90-90: Additional hardcoded strings beyond those previously flagged.Beyond the strings noted in the prior review, several more user-facing strings require extraction to
strings.xml:
- Line 90:
"CashPilot"(app title)- Line 240:
"24h"(time period label)- Lines 288-289:
"No heartbeat yet","Last heartbeat"(heartbeat status messages)- Line 450:
"Notification active"(indicator label)- Line 500:
"never"(relative time fallback)- Content descriptions:
"Settings"(93),"Upload"(214),"Download"(228),"Dismiss"(368),"Active notification"(444)As per coding guidelines, all user-facing strings in CashPilot-android must be defined in
strings.xmlfor future internationalization support.Also applies to: 288-289, 450-450, 500-500
🤖 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/DashboardScreen.kt` at line 90, Multiple hardcoded user-facing strings in DashboardScreen.kt (e.g., the app title Text("CashPilot"), time label "24h", heartbeat messages "No heartbeat yet"/"Last heartbeat", "Notification active", "never", and contentDescriptions "Settings", "Upload", "Download", "Dismiss", "Active notification") must be moved to strings.xml; update the UI to reference them via Android string resources (use stringResource(R.string.xxx) or context.getString where appropriate) and create descriptive keys in strings.xml (e.g., app_title, time_24h_label, heartbeat_none, heartbeat_last, notification_active_label, never_label, cd_settings, cd_upload, cd_download, cd_dismiss, cd_active_notification) so the composables (DashboardScreen and any named Text/Modifier.contentDescription usages) use resource lookups instead of hardcoded literals.
🤖 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/DashboardScreen.kt`:
- Line 90: Multiple hardcoded user-facing strings in DashboardScreen.kt (e.g.,
the app title Text("CashPilot"), time label "24h", heartbeat messages "No
heartbeat yet"/"Last heartbeat", "Notification active", "never", and
contentDescriptions "Settings", "Upload", "Download", "Dismiss", "Active
notification") must be moved to strings.xml; update the UI to reference them via
Android string resources (use stringResource(R.string.xxx) or context.getString
where appropriate) and create descriptive keys in strings.xml (e.g., app_title,
time_24h_label, heartbeat_none, heartbeat_last, notification_active_label,
never_label, cd_settings, cd_upload, cd_download, cd_dismiss,
cd_active_notification) so the composables (DashboardScreen and any named
Text/Modifier.contentDescription usages) use resource lookups instead of
hardcoded literals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64a50ae9-7649-45c3-8fda-b6d5ea286269
📒 Files selected for processing (5)
app/src/main/java/com/cashpilot/android/service/AppDetector.ktapp/src/main/java/com/cashpilot/android/service/HeartbeatService.ktapp/src/main/java/com/cashpilot/android/ui/MainActivity.ktapp/src/main/java/com/cashpilot/android/ui/MainViewModel.ktapp/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/com/cashpilot/android/service/AppDetector.kt
- app/src/main/java/com/cashpilot/android/service/HeartbeatService.kt
- app/src/main/java/com/cashpilot/android/ui/MainViewModel.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
Complete redesign of the dashboard from a flat list of detected apps to a monitoring-style fleet view (think Uptime Kuma / Portainer container list).
Key changes:
Files changed:
DashboardScreen.ktMainViewModel.ktAppDisplayInfo/FleetSummarymodels, permission checks, all-apps exposureAppDetector.ktisInstalled()made publicHeartbeatService.ktlastHeartbeat/lastHeartbeatFailedcompanion StateFlowsMainActivity.ktrepeatOnLifecycle(RESUMED)for auto-refresh on resumeTest plan
Summary by CodeRabbit