feat(sync): surface per-peer hostnames in sync report UI - #303
TimeToBuildBob wants to merge 2 commits into
Conversation
The Rust JNI `to_jni_json()` already emits a `peers` array with per-device `hostname` and `outcome.kind` fields; only aggregate counts were parsed on the Kotlin side. - Add `SyncPeer(hostname, outcome)` data class - Extend `SyncStatus` with `peers: List<SyncPeer>` (empty-default for backward compatibility with older native libs) - Parse the `peers` JSON array in `fromJniResponse()` - Update `formatSyncDetail()` to append `(desktop, laptop, !server)` after the aggregate peer line when per-peer data is available; `!` prefix marks failed peers for instant visual distinction without extra prose - 4 new unit tests covering parsing, rendering, and the no-peers fallback Closes ActivityWatch#285 Git-Session-Id: e1f4
|
| // Per-peer breakdown from the "peers" array in the JNI response. Empty for | ||
| // older native libs that pre-date the SyncReport JNI output. | ||
| val peers: List<SyncPeer> = emptyList(), |
There was a problem hiding this comment.
peers is added to SyncStatus, but the SharedPreferences adapter neither writes nor restores it. The settings UI always reloads the status through prefs.getLastSyncStatus(), so this field defaults to an empty list even after the completion broadcast. As a result, the new hostname rendering never appears in the actual UI; persist and restore the peer summaries alongside the aggregate fields.
There was a problem hiding this comment.
Fixed in 8752040. AWPreferences now persists peers as JSON (lastSyncPeers) next to the aggregate counts, and getLastSyncStatus() restores them. Decode never throws. Added encode/decode round-trip tests so formatSyncDetail still shows hostnames after the prefs reload the UI actually uses.
The settings UI reloads via getLastSyncStatus(), which dropped the new peers list so hostname rendering never appeared. Store peers as JSON next to the aggregate counts; decode never throws. Git-Session-Id: 7229f9f5-7b91-55aa-b26d-81c6a42708c7
|
@greptileai review |
|
CI-green and mergeable (Greptile 5/5) — waiting only on a maintainer click. This PR is ready to merge, but the bot has pull-only access to this repo and can't self-merge — surfacing it here so it isn't lost. The monitoring loop will stop re-flagging it now that this note is posted. |
🤖 AI code reviewAdds a SyncPeer data class and a peers list to SyncStatus, parses the peers JSON array from the JNI response, persists the peers list through SharedPreferences via new encodePeers/decodePeers helpers, and extends formatSyncDetail to append imported/failed hostnames in parentheses. Adds unit tests for parsing, rendering, and prefs round-tripping. Safe to merge — no P0/P1 findingsConfidence 5/5 ✅ No thread-worthy findings. Advisory notes follow; they are retained without opening review threads. 2 advisory findings (summary-only, not scored)These P2 guard, heuristic, trade-off, or documentation claims are retained for judgment without opening review threads.
The new peers list is parsed from the JNI response but is not bounded or capped, unlike warnings which are capped at MAX_WARNINGS. A malicious or buggy native lib (or a compromised sync server that controls the JNI response) could return an arbitrarily large peers array, causing unbounded memory use in fromJniResponse and in the SharedPreferences string written by encodePeers. The UI also joins all hostnames into a single line, so a large peer count produces an extremely long settings line. The existing code caps warnings to 5 and errors to 500 chars; peers have no equivalent cap. This is a robustness/security hardening gap rather than a demonstrated current-input failure, so it is a guard finding. How this was verified: Checked fromJniResponse in SyncInterface.kt lines 105-112; no length cap on arr. Compared to warnings cap at lines 97-103. Checked encodePeers lines 141-152 which serializes the full list.
The formatSyncDetail function appends imported and failed hostnames but does not include skipped peers. The comment says skipped peers are intentionally omitted, but the aggregate line still shows the skipped count. If a user has a peer that is skipped (e.g., 'up to date'), they see '1 skipped' but no name, which may be confusing. More importantly, the order of hostnames in the parentheses is not guaranteed to match the order of the aggregate counts: the peers array order is whatever the JNI returns, while the counts are separate. If the JNI returns peers in a different order than the counts imply, the display could show 'peers 2/4 imported' but the named list might include a different set. However, the JNI is expected to be consistent, so this is speculative. The real issue is that the code filters by outcome string, and if the JNI uses a different casing or value (e.g., 'Imported'), the names would be omitted. The PR description says the kind field is 'imported', 'skipped', or 'failed', so this is likely fine. How this was verified: Checked formatSyncDetail lines 99-104. The filter is case-sensitive and exact-match. Files changed (4) — the diff as I read it
Reviewed Maintainer commands
|
Summary
The Rust JNI
to_jni_json()already emits a fullpeersarray with per-devicehostnameandoutcome.kind; only the aggregate counts (peers_imported,peers_skipped,peers_failed) were parsed on the Kotlin side.This PR wires the per-peer data through:
SyncPeer(hostname, outcome)data class toSyncInterface.ktSyncStatuswithpeers: List<SyncPeer>(empty-default — fully backward-compatible with older native libs that don't emit thepeerskey)peersJSON array infromJniResponse()formatSyncDetail()to append hostnames in parens after the aggregate peer line, e.g.:!prefix marks failed peers for quick visual distinction without extra proseCloses #285
Test plan
./gradlew :mobile:testStandardDebugUnitTest --tests "net.activitywatch.android.SyncSettingsActivityTest"Co-Authored-By: Bob timetobuildbob@gmail.com