Fix toggle race, IP retry, permission banner, heartbeat schema#6
Merged
Fix toggle race, IP retry, permission banner, heartbeat schema#6
Conversation
…de 24 - toggleApp: pass new slugs directly to doRefresh to avoid StateFlow lag - Public IP: don't retry on failure, reset flag when config cleared - Permission banner: reset dismissal when permissions revoked - Heartbeat: send apps as top-level field instead of faking containers - CI: opt into Node 24 for GitHub Actions
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds CI env and adjusts signing step gating; extends heartbeat model and HeartbeatService with timeouts, consecutive-failure tracking, and exponential backoff; updates ViewModel/UI (toggle logic, public-IP failure tracking, back handler, permission banner, cleartext warnings, referral links); adds privacy docs and strings. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (HeartbeatService)
participant Settings as SettingsStore
participant Http as Ktor HttpClient
participant Server as CashPilot Server
App->>Settings: read heartbeatInterval, server config
Note right of App: build WorkerHeartbeat (containers with slug + apps)
App->>Http: POST /heartbeat (with HttpTimeout)
Http-->>Server: network request
Server-->>Http: HTTP response (2xx / non-2xx)
Http-->>App: result or exception
alt success
App->>App: consecutiveFailures = 0
App->>Settings: schedule next send after baseInterval
else failure
App->>App: consecutiveFailures += 1
App->>App: compute exponential backoff delay (capped)
App->>Settings: schedule send after backoff delay
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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
🧹 Nitpick comments (2)
app/src/main/java/com/cashpilot/android/ui/MainViewModel.kt (2)
176-191: Consider extracting the hardcoded ipify URL to a constant or configuration.The URL
https://api.ipify.orgis hardcoded. While this is a stable third-party service (not a CashPilot server), extracting it to a companion object constant would improve maintainability and make it easier to swap services if needed.companion object { private const val PUBLIC_IP_API_URL = "https://api.ipify.org" }As per coding guidelines: "Never hardcode server URLs or tokens in CashPilot-android code." While this refers primarily to CashPilot servers, centralizing external URLs is good practice.
🤖 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 176 - 191, Extract the hardcoded ipify URL in fetchPublicIp() into a constant (e.g., PUBLIC_IP_API_URL) on the class companion object and use that constant inside URL(...) so the external service endpoint is centralized and easy to change; update fetchPublicIp() to reference PUBLIC_IP_API_URL and ensure the companion object defines it as a private const val.
140-147: Public IP failure flag only resets when server config becomes incomplete.The
publicIpFailedflag resets when!serverReady(lines 144-146), but if a user changes from one valid server configuration to another valid configuration, the failure state persists. This means if the IP fetch failed with the old config, users must briefly clear the server URL/API key to trigger a retry with the new server.This is a minor edge case and acceptable if intentional—just noting for awareness.
🤖 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 140 - 147, The publicIpFailed flag is only reset when serverReady becomes false, so it persists across valid-to-valid config changes; update the logic in MainViewModel (around serverReady/_publicIp/publicIpFailed/fetchPublicIp) to also clear publicIpFailed and attempt fetch when the server configuration actually changes while remaining valid—e.g., detect a change in settings.value.serverUrl or settings.value.apiKey (or keep a previousConfig snapshot) and if serverReady is true and the config differs, set publicIpFailed = false and call fetchPublicIp() so the new valid config retriggers the IP fetch.
🤖 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/model/Heartbeat.kt`:
- Around line 14-18: The Heartbeat data class introduced a top-level val apps:
List<AppStatus> which violates the WorkerHeartbeat schema; remove the top-level
apps field and move Android app status into SystemInfo (use systemInfo.apps) so
the serialized payload contains name, url, containers, and system_info only; if
you need a migration window, optionally dual-write by populating systemInfo.apps
and, for a short time only, mirror it into the removed top-level field via a
transient/ignored property or explicit serializer, but ensure production
serialization matches WorkerHeartbeat (check the Heartbeat class, the apps
property, and the SystemInfo class).
In `@app/src/main/java/com/cashpilot/android/service/HeartbeatService.kt`:
- Around line 79-87: The Heartbeat payload created in HeartbeatService
(WorkerHeartbeat instantiation) currently sets apps at top-level instead of
placing Android app statuses into SystemInfo, violating the WorkerHeartbeat
contract; update the WorkerHeartbeat construction in HeartbeatService (and
related Heartbeat/HeartbeatService functions) so that the apps list is assigned
to SystemInfo.apps (or duplicated there) — i.e., add an apps field inside the
SystemInfo instance (populated with the existing apps variable) and remove or
keep the top-level apps only if duplication is desired, ensuring the emitted
payload includes system_info.apps as per the server schema.
---
Nitpick comments:
In `@app/src/main/java/com/cashpilot/android/ui/MainViewModel.kt`:
- Around line 176-191: Extract the hardcoded ipify URL in fetchPublicIp() into a
constant (e.g., PUBLIC_IP_API_URL) on the class companion object and use that
constant inside URL(...) so the external service endpoint is centralized and
easy to change; update fetchPublicIp() to reference PUBLIC_IP_API_URL and ensure
the companion object defines it as a private const val.
- Around line 140-147: The publicIpFailed flag is only reset when serverReady
becomes false, so it persists across valid-to-valid config changes; update the
logic in MainViewModel (around
serverReady/_publicIp/publicIpFailed/fetchPublicIp) to also clear publicIpFailed
and attempt fetch when the server configuration actually changes while remaining
valid—e.g., detect a change in settings.value.serverUrl or settings.value.apiKey
(or keep a previousConfig snapshot) and if serverReady is true and the config
differs, set publicIpFailed = false and call fetchPublicIp() so the new valid
config retriggers the IP fetch.
🪄 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: fc28797c-2959-492f-9069-84acc03ef14d
📒 Files selected for processing (5)
.github/workflows/ci.ymlapp/src/main/java/com/cashpilot/android/model/Heartbeat.ktapp/src/main/java/com/cashpilot/android/service/HeartbeatService.ktapp/src/main/java/com/cashpilot/android/ui/MainViewModel.ktapp/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt
Comment on lines
+14
to
18
| /** Docker containers (empty for Android workers). */ | ||
| val containers: List<AppContainer> = emptyList(), | ||
| /** Android app statuses (empty for Docker workers). */ | ||
| val apps: List<AppStatus> = emptyList(), | ||
| @SerialName("system_info") val systemInfo: SystemInfo = SystemInfo(), |
There was a problem hiding this comment.
Schema contract drift: apps should not be top-level in WorkerHeartbeat.
This change introduces a payload shape that conflicts with the documented WorkerHeartbeat contract used by this repo. Keep Android app status under system_info.apps (optionally dual-write only during migration windows).
Suggested fix
data class WorkerHeartbeat(
val name: String,
val url: String = "",
/** Docker containers (empty for Android workers). */
val containers: List<AppContainer> = emptyList(),
- /** Android app statuses (empty for Docker workers). */
- val apps: List<AppStatus> = emptyList(),
`@SerialName`("system_info") val systemInfo: SystemInfo = SystemInfo(),
)As per coding guidelines: "Heartbeat payload in CashPilot-android must match the server's WorkerHeartbeat schema with name, url, containers, and system_info fields, with Android-specific app status packed into system_info.apps".
🤖 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/model/Heartbeat.kt` around lines 14 -
18, The Heartbeat data class introduced a top-level val apps: List<AppStatus>
which violates the WorkerHeartbeat schema; remove the top-level apps field and
move Android app status into SystemInfo (use systemInfo.apps) so the serialized
payload contains name, url, containers, and system_info only; if you need a
migration window, optionally dual-write by populating systemInfo.apps and, for a
short time only, mirror it into the removed top-level field via a
transient/ignored property or explicit serializer, but ensure production
serialization matches WorkerHeartbeat (check the Heartbeat class, the apps
property, and the SystemInfo class).
Comment on lines
79
to
87
| val heartbeat = WorkerHeartbeat( | ||
| name = "${Build.MANUFACTURER} ${Build.MODEL} (${deviceId()})", | ||
| containers = containers, | ||
| apps = apps, | ||
| systemInfo = SystemInfo( | ||
| os = "Android", | ||
| arch = Build.SUPPORTED_ABIS.firstOrNull() ?: "unknown", | ||
| osVersion = "Android ${Build.VERSION.RELEASE} (API ${Build.VERSION.SDK_INT})", | ||
| deviceType = "android", | ||
| apps = apps, | ||
| ), |
There was a problem hiding this comment.
Heartbeat payload is emitted with empty system_info.apps.
At send time, app statuses are placed at top-level and not in system_info.apps, which breaks the repo’s declared WorkerHeartbeat contract. Move (or at least also copy) apps into SystemInfo.
Suggested fix
val heartbeat = WorkerHeartbeat(
name = "${Build.MANUFACTURER} ${Build.MODEL} (${deviceId()})",
- apps = apps,
systemInfo = SystemInfo(
os = "Android",
arch = Build.SUPPORTED_ABIS.firstOrNull() ?: "unknown",
osVersion = "Android ${Build.VERSION.RELEASE} (API ${Build.VERSION.SDK_INT})",
deviceType = "android",
+ apps = apps,
),
)Based on learnings: "Applies to **/{Heartbeat,HeartbeatService}.kt : Heartbeat payload in CashPilot-android must match the server's WorkerHeartbeat schema with name, url, containers, and system_info fields, with Android-specific app status packed into system_info.apps".
🤖 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 79 - 87, The Heartbeat payload created in HeartbeatService
(WorkerHeartbeat instantiation) currently sets apps at top-level instead of
placing Android app statuses into SystemInfo, violating the WorkerHeartbeat
contract; update the WorkerHeartbeat construction in HeartbeatService (and
related Heartbeat/HeartbeatService functions) so that the apps list is assigned
to SystemInfo.apps (or duplicated there) — i.e., add an apps field inside the
SystemInfo instance (populated with the existing apps variable) and remove or
keep the top-level apps only if duplication is desired, ensuring the emitted
payload includes system_info.apps as per the server schema.
7 tasks
- CI: build release APK on PRs too (validates minify/signing) - HeartbeatService: add 15s HTTP timeout and exponential backoff (30s → 60s → 120s → 5min max) on consecutive failures - HeartbeatService: send both containers (legacy) and apps (new) for backward-compatible server transition - MainActivity: add BackHandler so system Back returns from Settings to Dashboard instead of finishing the activity - Setup/Settings: show cleartext warning when server URL is http:// - README + fastlane: disclose api.ipify.org lookup in privacy section
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/com/cashpilot/android/ui/screen/SettingsScreen.kt (1)
104-107: Extract URL cleartext check into a shared helper used by both screens.The same inline predicate exists in
SetupScreenandSettingsScreen; centralizing it avoids behavioral drift.Refactor sketch
- isError = localUrl.startsWith("http://"), - supportingText = if (localUrl.startsWith("http://")) { + val isCleartextUrl = isCleartextHttpUrl(localUrl) + isError = isCleartextUrl, + supportingText = if (isCleartextUrl) { { Text(stringResource(R.string.cleartext_warning)) } } else null,// e.g., shared util fun isCleartextHttpUrl(url: String): Boolean = url.trimStart().startsWith("http://", ignoreCase = true)🤖 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/SettingsScreen.kt` around lines 104 - 107, Create a shared helper function isCleartextHttpUrl(url: String): Boolean (e.g., in a common util object) that returns url.trimStart().startsWith("http://", ignoreCase = true), then replace the inline predicates in SettingsScreen and SetupScreen (the instances currently using localUrl.startsWith("http://")) to call isCleartextHttpUrl(localUrl) for both the isError and supportingText checks so both screens share the same logic.
🤖 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/MainActivity.kt`:
- Around line 78-81: The BackHandler is currently gated only by showSettings so
it can incorrectly consume Back when needsSetup becomes true; update the handler
to gate on the actual Settings-visible state (e.g., compute an isSettingsVisible
boolean from showSettings and needsSetup or the actual rendered screen state)
and use that for BackHandler.enabled, and keep the existing lambda that sets
showSettings = false; refer to BackHandler, showSettings and needsSetup (or your
rendered-screen variable) to locate and replace the enabled expression.
In `@app/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.kt`:
- Around line 117-120: The cleartext URL check in SetupScreen.kt is brittle
because it uses localUrl.startsWith("http://") directly; update both the isError
and supportingText checks to operate on a normalized string (e.g., val
normalized = localUrl.trim().lowercase()) and then use
normalized.startsWith("http://") so values like " HTTP://host" or mixed-case
schemes trigger the warning; modify the references in the SetupScreen composable
(where localUrl is used for isError and supportingText) to use this normalized
variable.
---
Nitpick comments:
In `@app/src/main/java/com/cashpilot/android/ui/screen/SettingsScreen.kt`:
- Around line 104-107: Create a shared helper function isCleartextHttpUrl(url:
String): Boolean (e.g., in a common util object) that returns
url.trimStart().startsWith("http://", ignoreCase = true), then replace the
inline predicates in SettingsScreen and SetupScreen (the instances currently
using localUrl.startsWith("http://")) to call isCleartextHttpUrl(localUrl) for
both the isError and supportingText checks so both screens share the same logic.
🪄 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: 2c903dd7-e005-4b55-a187-21058eaf937d
📒 Files selected for processing (8)
.github/workflows/ci.ymlREADME.mdapp/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/screen/SettingsScreen.ktapp/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.ktapp/src/main/res/values/strings.xmlfastlane/metadata/android/en-US/full_description.txt
✅ Files skipped from review due to trivial changes (3)
- app/src/main/res/values/strings.xml
- fastlane/metadata/android/en-US/full_description.txt
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- app/src/main/java/com/cashpilot/android/service/HeartbeatService.kt
app/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.kt
Outdated
Show resolved
Hide resolved
- Settings: add About section with GitHub repo links (Android + Server) and GitHub Sponsors donate button - MonitoredApp: add optional referralUrl field; populated for EarnApp, IPRoyal Pawns, Traffmonetizer, and Grass - DashboardScreen: use referral URL when tapping not-installed apps (opens browser with referral tracking → Play Store redirect), falls back to direct Play Store link for apps without referrals
- AppContainer: add slug field so server discovers worker items correctly via containers[*].slug - CI: guard release build on KEYSTORE_BASE64 secret presence so forked/external PRs don't fail on missing secrets - Referral codes: use actual codes from CashPilot AGENTS.md for EarnApp, IPRoyal, Traffmonetizer, Bytelixir, Grass, Titan, Uprock (7 of 11 apps now have referral links)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 430-451: The referral branch currently opens info.app.referralUrl
without any fallback, so if that launch fails users lose the install route;
update the code around info.app.referralUrl to wrap
context.startActivity(Intent(..., Uri.parse(info.app.referralUrl))) in a
try/catch and on failure attempt the same
market://details?id=${info.app.packageName} Intent used in the non-referral
branch, and if that also fails fall back to the
https://play.google.com/store/apps/details?id=${info.app.packageName} web
Intent; reference info.app.referralUrl, info.app.packageName,
context.startActivity, and the market:// and play.google.com URIs when making
the changes.
In `@app/src/main/java/com/cashpilot/android/ui/screen/SettingsScreen.kt`:
- Around line 189-213: The About section in SettingsScreen.kt uses hard-coded
user-facing strings (Text("About"), Text("GitHub — CashPilot Android"),
Text("GitHub — CashPilot Server"), Text("Sponsor / Donate")); add these four
keys to strings.xml (e.g., about, about_github_android, about_github_server,
about_sponsor_donate) and replace the string literals in SettingsScreen.kt with
stringResource(R.string.<key>) calls (keeping the existing calls to openUrl and
the Button/OutlinedButton components intact) so all UI text is sourced from
resources for i18n.
🪄 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: f4504e59-d8d9-49bc-b1a5-602a2b73ee91
📒 Files selected for processing (3)
app/src/main/java/com/cashpilot/android/model/MonitoredApp.ktapp/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.ktapp/src/main/java/com/cashpilot/android/ui/screen/SettingsScreen.kt
app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt
Outdated
Show resolved
Hide resolved
- Referral install: try/catch with fallback to Play Store on failure - BackHandler: disable when setup screen is active - Cleartext warning: trim + case-insensitive check - About section: move hardcoded strings to strings.xml - Extract PUBLIC_IP_URL constant in MainViewModel
Secrets can't be referenced in if: expressions directly. Use an intermediate step that sets an output when the secret env var exists.
Adds android.content.Context import needed by the openAppInstall function.
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/screen/DashboardScreen.kt (1)
543-551:⚠️ Potential issue | 🟡 MinorHardcoded string "never" should be moved to
strings.xml.The string
"never"at line 544 is user-facing but not defined instrings.xml, which breaks i18n support.Proposed fix
In
res/values/strings.xml:<string name="time_never">never</string>Then update the function:
-private fun relativeTime(millis: Long): String { - if (millis == 0L) return "never" +@Composable +private fun relativeTime(millis: Long): String { + if (millis == 0L) return stringResource(R.string.time_never)Note: Making this
@Composablechanges call sites. Alternatively, pass the string as a parameter from the calling Composable.As per coding guidelines: "All user-facing strings in CashPilot-android must be defined in
strings.xmlfor future internationalization support."🤖 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 543 - 551, The hardcoded user-facing string "never" in the function relativeTime should be moved to resources: add an entry (e.g., name="time_never") in res/values/strings.xml and replace the literal in relativeTime with a reference to that string; because relativeTime is a plain function (not `@Composable`) either make it accept the localized string as a parameter from calling Composables or convert relativeTime to `@Composable` so you can call stringResource("time_never") inside it—update all call sites accordingly to pass the string or to call the new `@Composable` version.
♻️ Duplicate comments (1)
app/src/main/java/com/cashpilot/android/service/HeartbeatService.kt (1)
110-120:⚠️ Potential issue | 🟠 MajorMove app statuses from top-level
appsto nestedsystem_info.apps.The code populates
WorkerHeartbeat.apps(top-level) instead ofSystemInfo.apps(nested), violating the documented contract. AGENTS.md and the coding guidelines require Android app status to be packed intosystem_info.apps. The top-levelappsfield is for Docker containers ("empty for Docker workers" per the schema comment). Update the heartbeat construction to populatesystemInfo.appsinstead of the top-levelappsfield.Current code
val heartbeat = WorkerHeartbeat( name = "${Build.MANUFACTURER} ${Build.MODEL} (${deviceId()})", containers = containers, apps = apps, systemInfo = SystemInfo( os = "Android", arch = Build.SUPPORTED_ABIS.firstOrNull() ?: "unknown", osVersion = "Android ${Build.VERSION.RELEASE} (API ${Build.VERSION.SDK_INT})", deviceType = "android", ), )🤖 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 110 - 120, The heartbeat currently sets the Android app list into the top-level WorkerHeartbeat.apps field; instead populate SystemInfo.apps with that data. In the WorkerHeartbeat construction (where WorkerHeartbeat(...) is created), remove or leave WorkerHeartbeat.apps empty and pass the apps variable into the nested SystemInfo by setting its apps property (SystemInfo(..., apps = apps, ...)); ensure containers and other fields (name, systemInfo) remain unchanged and keep using deviceId(), Build.MANUFACTURER/BUILD.MODEL, and Build.VERSION values as before.
🧹 Nitpick comments (1)
app/src/main/java/com/cashpilot/android/service/HeartbeatService.kt (1)
150-156: Hardcoded user-facing strings should move tostrings.xml.Strings like
"CashPilot Agent","Shows monitoring status", and"CashPilot"are hardcoded. Per coding guidelines, all user-facing strings should be defined instrings.xmlfor i18n support.♻️ Example refactor
In
strings.xml:<string name="notification_channel_name">CashPilot Agent</string> <string name="notification_channel_description">Shows monitoring status</string> <string name="notification_title">CashPilot</string>Then in
HeartbeatService.kt:private fun createNotificationChannel() { val channel = NotificationChannel( CHANNEL_ID, - "CashPilot Agent", + getString(R.string.notification_channel_name), NotificationManager.IMPORTANCE_LOW, ).apply { - description = "Shows monitoring status" + description = getString(R.string.notification_channel_description) setShowBadge(false) }As per coding guidelines: "All user-facing strings in CashPilot-android must be defined in
strings.xmlfor future internationalization support".Also applies to: 162-169
🤖 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 150 - 156, Move all hardcoded user-facing strings in HeartbeatService (e.g., the NotificationChannel label "CashPilot Agent", its description "Shows monitoring status", and any notification title like "CashPilot") into strings.xml and replace the literals with context.getString(R.string.<name>) references; update the NotificationChannel creation (NotificationChannel(...)) and any notification builder code that sets the title or content to use the new R.string keys (for example notification_channel_name, notification_channel_description, notification_title) so the NotificationChannel constructor and setDescription call consume getString(...) instead of hardcoded text.
🤖 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/service/HeartbeatService.kt`:
- Around line 77-85: The backoff calculation in HeartbeatService (variables
settings.heartbeatIntervalSeconds, baseDelay and backoff inside the retry logic)
can produce a zero or negative delay if heartbeatIntervalSeconds is 0 or
negative; enforce a minimum positive floor (e.g., MIN_HEARTBEAT_DELAY_MS) by
normalizing baseDelay = max(settings.heartbeatIntervalSeconds * 1000L,
MIN_HEARTBEAT_DELAY_MS) (or treat negative as the default interval) before
applying the exponential backoff and coerceAtMost cap so delay(backoff) never
receives 0 or a negative value.
In `@app/src/main/java/com/cashpilot/android/ui/MainViewModel.kt`:
- Around line 95-107: The toggleApp function currently reads
settings.value.enabledSlugs outside the SettingsStore.update transaction,
causing a TOCTOU race; move the computation of the newSlugs inside the
SettingsStore.update lambda so you use the authoritative s.enabledSlugs (e.g.,
compute newSet from s.enabledSlugs and return s.copy(enabledSlugs = newSet)
inside update), then after the update completes call doRefresh(enabledOverride =
newSet); keep the refreshJob?.cancel() and the viewModelScope.launch wrapper but
do not rely on settings.value for the toggle computation.
---
Outside diff comments:
In `@app/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.kt`:
- Around line 543-551: The hardcoded user-facing string "never" in the function
relativeTime should be moved to resources: add an entry (e.g.,
name="time_never") in res/values/strings.xml and replace the literal in
relativeTime with a reference to that string; because relativeTime is a plain
function (not `@Composable`) either make it accept the localized string as a
parameter from calling Composables or convert relativeTime to `@Composable` so you
can call stringResource("time_never") inside it—update all call sites
accordingly to pass the string or to call the new `@Composable` version.
---
Duplicate comments:
In `@app/src/main/java/com/cashpilot/android/service/HeartbeatService.kt`:
- Around line 110-120: The heartbeat currently sets the Android app list into
the top-level WorkerHeartbeat.apps field; instead populate SystemInfo.apps with
that data. In the WorkerHeartbeat construction (where WorkerHeartbeat(...) is
created), remove or leave WorkerHeartbeat.apps empty and pass the apps variable
into the nested SystemInfo by setting its apps property (SystemInfo(..., apps =
apps, ...)); ensure containers and other fields (name, systemInfo) remain
unchanged and keep using deviceId(), Build.MANUFACTURER/BUILD.MODEL, and
Build.VERSION values as before.
---
Nitpick comments:
In `@app/src/main/java/com/cashpilot/android/service/HeartbeatService.kt`:
- Around line 150-156: Move all hardcoded user-facing strings in
HeartbeatService (e.g., the NotificationChannel label "CashPilot Agent", its
description "Shows monitoring status", and any notification title like
"CashPilot") into strings.xml and replace the literals with
context.getString(R.string.<name>) references; update the NotificationChannel
creation (NotificationChannel(...)) and any notification builder code that sets
the title or content to use the new R.string keys (for example
notification_channel_name, notification_channel_description, notification_title)
so the NotificationChannel constructor and setDescription call consume
getString(...) instead of hardcoded text.
🪄 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: afecf30d-5b5f-44ea-aa39-0ba979104ba9
📒 Files selected for processing (10)
.github/workflows/ci.ymlapp/src/main/java/com/cashpilot/android/model/Heartbeat.ktapp/src/main/java/com/cashpilot/android/model/MonitoredApp.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.ktapp/src/main/java/com/cashpilot/android/ui/screen/SettingsScreen.ktapp/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.ktapp/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (2)
- app/src/main/java/com/cashpilot/android/ui/MainActivity.kt
- app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.kt
- app/src/main/java/com/cashpilot/android/ui/screen/SettingsScreen.kt
- .github/workflows/ci.yml
- app/src/main/java/com/cashpilot/android/model/MonitoredApp.kt
- app/src/main/java/com/cashpilot/android/model/Heartbeat.kt
- Move slug toggle logic inside DataStore transaction to prevent concurrent toggle races - Add 5s minimum floor to heartbeat delay to prevent tight loops if heartbeatIntervalSeconds is zero
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
doRefresh()instead of reading from StateFlow, which can lag the DataStore write by one emissionpublicIpFailedflag so a blocked/unreachableapi.ipify.orgdoesn't retry on every 30s refresh. Flag resets when server config changes.LaunchedEffectresets thedismissedflag when notification or usage access is revoked after dismissalappsfield instead of being faked ascontainers. Server needs corresponding update to readappsfordeviceType=androidworkers.FORCE_JAVASCRIPT_ACTIONS_TO_NODE24to suppress GitHub Actions deprecation warningsNote
The heartbeat schema change means the CashPilot server will see
containers: []andapps: [...]for Android workers. The server-side needs a matching update to display apps distinctly from Docker containers.Test plan
api.ipify.orgvia airplane mode → verify no repeated requests in logcatappsarray (notcontainers) via server logsSummary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation