fix: without permissions the app claimed every earning app had stopped - #49
Merged
Conversation
Started on the permissions half of kv8, which described a UI nit: a dismissible
banner that ought to block. The defect underneath it is far worse.
AppDetector.kt:64 reduced detection to one line:
running = notificationActive || recentlyActive || hasRecentNetworkActivity
ALL THREE INPUTS DEGRADE TO FALSE WHEN THE PERMISSIONS ARE DENIED. Without
notification-listener access the service gets no callbacks; without usage access
getLastActiveTime returns null (call site `?: false`) and getNetworkStats
returns `0L to 0L` -- NetworkStatsManager needs PACKAGE_USAGE_STATS too. And
AppStatus.running was a NON-NULLABLE Boolean, so the type could not even express
"could not determine".
So a device with access revoked reported EVERY app as STOPPED. The screen stated
as fact that the user's earning apps had died when it simply could not see them,
and a user acting on that would go restart apps that were running perfectly. The
same claim went upstream in the heartbeat, so a paired server inherited it.
It also got LOUDER with the attention-first sort shipped earlier: eleven false
STOPPED cards, now sorted to the top of the screen.
THE RULE, in service/Detection.kt: a NEGATIVE is only trustworthy when every
signal source was available; a POSITIVE needs just one, because each is proof of
life. Most bandwidth apps run with no visible notification and are caught solely
by network activity, so "nothing seen" with usage access denied is precisely the
app we failed to see.
any positive, whatever is missing -> true
nothing, with FULL access -> false (act on the app)
nothing, with partial or no access -> null (act on the permission)
AppState gains UNKNOWN, deliberately not a flavour of STOPPED: different cause,
different fix. It ranks second -- an unknown app MIGHT be fine, a stopped one is
definitely not -- and renders amber rather than the stopped red.
The wire spelling is "unknown", not "stopped". The server maps a falsy running
to "stopped" (main.py:241), so collapsing them here would hand the fleet page
the same false claim. A server-side fix is filed separately.
Permissions.kt extracts the two access checks that previously lived only in
MainViewModel -- AppDetector had no way to ask whether it could see anything,
and did not: getSystemService hands out a manager even when the permission is
denied (enforcement is at QUERY time), so the empty results looked like
"stopped".
SIX TESTS WERE ASSERTING THE OLD BEHAVIOUR and are updated, including two of my
own from the ordering change. Three of them had COPIED the production expression
`if (app.running) "running" else "stopped"` into themselves; that mapping now
lives in Detection.wireStatus and they call it.
Four negative controls fire: restoring the one-liner, collapsing unknown to
stopped on the wire, ignoring a positive signal when a permission is missing,
and mapping UNKNOWN back to STOPPED in the UI.
📝 WalkthroughWalkthroughThe PR adds permission-aware tri-state app detection. It preserves unknown status in heartbeat payloads, adds ChangesTri-state detection flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/cashpilot/android/service/HeartbeatService.kt`:
- Around line 106-110: Update the SystemInfo construction in HeartbeatService to
pass the populated apps map via apps = apps, while retaining the existing
root-level apps field for backward compatibility and preserving the detected
status values.
In `@app/src/test/java/com/cashpilot/android/SerializationTest.kt`:
- Line 270: Update the status mapping in the serialization test to use
Detection.wireStatus(it.running), preserving the "unknown" result when running
is null while retaining the existing running and stopped mappings. Add a fixture
assertion covering a null running value and its expected "unknown" wire status.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 21fac023-ce3f-4473-9b34-96f7428d7d87
📒 Files selected for processing (20)
app/src/main/java/com/cashpilot/android/model/Heartbeat.ktapp/src/main/java/com/cashpilot/android/service/AppDetector.ktapp/src/main/java/com/cashpilot/android/service/Detection.ktapp/src/main/java/com/cashpilot/android/service/HeartbeatService.ktapp/src/main/java/com/cashpilot/android/ui/AppPresentation.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/util/Permissions.ktapp/src/main/res/values/strings.xmlapp/src/test/java/com/cashpilot/android/AppPresentationTest.ktapp/src/test/java/com/cashpilot/android/AppStateResolutionTest.ktapp/src/test/java/com/cashpilot/android/AppStateTest.ktapp/src/test/java/com/cashpilot/android/DataClassContractTest.ktapp/src/test/java/com/cashpilot/android/DetectionTest.ktapp/src/test/java/com/cashpilot/android/HeartbeatLogicTest.ktapp/src/test/java/com/cashpilot/android/HeartbeatModelTest.ktapp/src/test/java/com/cashpilot/android/HeartbeatPayloadBuildTest.ktapp/src/test/java/com/cashpilot/android/ModelEdgeCaseTest.ktapp/src/test/java/com/cashpilot/android/SerializationEdgeCaseTest.ktapp/src/test/java/com/cashpilot/android/SerializationTest.kt
Comment on lines
+106
to
+110
| // Three-valued on the wire too. `unknown` is NOT "stopped": | ||
| // the server maps a falsy running to "stopped", so sending | ||
| // false while blind would hand the fleet page the same false | ||
| // claim this fix removes from the phone. | ||
| status = Detection.wireStatus(app.running), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Populate system_info.apps with detected app status.
This map updates legacy containers only. Line 121 sets the root apps field, but Lines 122-128 construct SystemInfo without apps = apps. The heartbeat therefore sends an empty system_info.apps list.
Pass apps = apps to SystemInfo, while retaining the root field for backward compatibility.
As per coding guidelines: “Heartbeat POST payload must match server's WorkerHeartbeat schema with Android-specific app status in system_info.apps.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/cashpilot/android/service/HeartbeatService.kt` around
lines 106 - 110, Update the SystemInfo construction in HeartbeatService to pass
the populated apps map via apps = apps, while retaining the existing root-level
apps field for backward compatibility and preserving the detected status values.
Source: Coding guidelines
| slug = it.slug, | ||
| name = "cashpilot-${it.slug}", | ||
| status = if (it.running) "running" else "stopped", | ||
| status = if (it.running == true) "running" else "stopped", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the unknown wire status.
Line 270 maps running == null to "stopped". The tri-state contract requires "unknown". Use Detection.wireStatus(it.running) and add a null-running fixture assertion.
Proposed fix
- status = if (it.running == true) "running" else "stopped",
+ status = Detection.wireStatus(it.running),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/test/java/com/cashpilot/android/SerializationTest.kt` at line 270,
Update the status mapping in the serialization test to use
Detection.wireStatus(it.running), preserving the "unknown" result when running
is null while retaining the existing running and stopped mappings. Add a fixture
assertion covering a null running value and its expected "unknown" wire status.
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.
Closes
CashPilot-android-1oo, which I filed on starting the permissions half ofkv8.The bead described a UI nit. The defect underneath is much worse.
AppDetector.kt:64reduced detection to one line:All three inputs degrade to
falsewhen the permissions are denied:notificationActivefalse— the listener gets no callbacks at allrecentlyActivegetLastActiveTimereturns null, call site is?: falsehasRecentNetworkActivitygetNetworkStatsreturns0L to 0L— NetworkStatsManager needsPACKAGE_USAGE_STATStooAnd
AppStatus.runningwas a non-nullableBoolean, so the type could not even express "could not determine".So a device with access revoked reported every app as STOPPED — stating as fact that your earning apps had died when it simply could not see them. Anyone acting on that would go restart apps that were running perfectly. The same claim went upstream in the heartbeat, so a paired server inherited it.
It also got louder with the attention-first sort I shipped earlier: eleven false STOPPED cards, now sorted to the top of the screen.
The rule
A negative is only trustworthy when every signal source was available. A positive needs just one, because each is proof of life.
That middle row is what STOPPED should mean. Most bandwidth apps run with no visible notification and are caught solely by network activity — so "nothing seen" with usage access denied is precisely the app we failed to see.
AppStategainsUNKNOWN, deliberately not a flavour of STOPPED: different cause, different fix. It ranks second — an unknown app might be fine, a stopped one definitely is not — and renders amber rather than the stopped red.Why
AppDetectornever noticedgetSystemServicehands out a manager even when the permission is denied — enforcement happens at query time. So a non-null manager proved nothing, and the empty results it returned looked exactly like "stopped". The two access checks lived only inMainViewModel; they are now inutil/Permissions.ktand both call the same one.The wire format matters too
Detection.wireStatusspells it"unknown", never"stopped". The server maps a falsyrunningto"stopped"(main.py:241), so collapsing them here would hand the fleet page the same false claim. A server-side fix is filed separately — this PR stops the phone lying; the server still needs to stop translating null into "stopped".Six tests were asserting the old behaviour
Including two of my own from the ordering change — I had written "an undetectable app counts as stopped, not as fine" and argued for it. That reasoning was wrong once the cause was traced.
Three others had copied the production expression
if (app.running) "running" else "stopped"into themselves — the same copy-not-call trap this repo keeps producing. That mapping now lives inDetection.wireStatusand they call it.Evidence
367 tests, 0 failures,
lintDebugclean, viascripts/remote-gradle.sh(a pre-check — CI still builds the signed release variant and lints against the baseline).Four negative controls fire: restoring the one-liner, collapsing unknown to stopped on the wire, ignoring a positive signal when a permission is missing, and mapping
UNKNOWNback toSTOPPEDin the UI.Still to come in
kv8The blocking permission screen itself. Strings are in place, and it is now honest to show it — because with no access every app genuinely reads UNKNOWN rather than being mislabelled as dead.
Summary by CodeRabbit
New Features
Bug Fixes
Tests