feat: RNode disconnect notification by torlando-tech · Pull Request #628 · torlando-tech/columba

torlando-tech · 2026-03-08T07:19:59Z

Summary

Adds a heads-up notification (IMPORTANCE_HIGH) when an RNode radio disconnects, with auto-cancel on tap
Appends "(RNode disconnected)" to the foreground service notification detail text while network is READY
Dismisses alert automatically when RNode reconnects
Threads interfaceName through the full Python → KotlinRNodeBridge → ReticulumServiceBinder → ServiceNotificationManager chain

Closes #625

Test plan

Connect RNode via Bluetooth, verify normal operation
Power off RNode → heads-up "RNode Disconnected" notification appears within seconds
Foreground notification detail text shows "(RNode disconnected)"
Power RNode back on → alert dismissed, foreground notification returns to normal
Verify existing unit tests pass (KotlinRNodeBridgeOnlineStatusTest, test_wrapper_ble.py)
Verify no notification spam on rapid connect/disconnect cycles

🤖 Generated with Claude Code

sentry · 2026-03-08T07:22:51Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

torlando-tech · 2026-03-11T04:26:24Z

@greptileai

greptile-apps · 2026-03-11T04:29:06Z

Greptile Summary

This PR adds an RNode disconnect notification feature: a heads-up alert (IMPORTANCE_HIGH) fires when any RNode radio loses connection, the foreground service notification appends "(RNode disconnected)" while the network is otherwise READY, and both are dismissed automatically on reconnect. The interfaceName parameter is threaded through the full stack from Python → KotlinRNodeBridge → ReticulumServiceBinder → ServiceNotificationManager.

Key implementation highlights:

Per-interface tracking uses ConcurrentHashMap.newKeySet() so multiple simultaneous RNode disconnects are handled correctly and individually cancelable on reconnect
Debounce gates fresh heads-up postings using activeNotifications as ground truth (covering autoCancel taps and user swipes), with lastDisconnectNotifyMs reset to 0 on full reconnect so a disconnect immediately following a reconnect always re-fires
Sync guard prevents the foreground notification from being refreshed mid-sync
A new ServiceNotificationManagerRNodeTest covers disconnect/reconnect, multi-interface scenarios, foreground text content, debounce behaviour, and the sync guard — however, the sync guard test assertion is always true regardless of whether the guard is active (see inline comment)

Confidence Score: 4/5

Safe to merge — core notification logic is correct; one test has a false-positive assertion that should be fixed before merging if test coverage quality is important
The implementation correctly addresses all issues raised in previous review rounds (per-interface tracking, debounce with activeNotifications ground truth, debounce reset on reconnect, sync guard). The only remaining issue is that the sync guard unit test always passes regardless of whether the guard works, so a future regression could go undetected.
app/src/test/java/com/lxmf/messenger/service/manager/ServiceNotificationManagerRNodeTest.kt — the foreground notification is not refreshed during active sync test needs a stronger assertion

Important Files Changed

Filename	Overview
app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt	Adds per-interface RNode disconnect tracking via ConcurrentHashMap, a heads-up notification channel, debounce logic using activeNotifications for ground-truth visibility, and appends "(RNode disconnected)" to the foreground notification. Implementation is solid overall.
app/src/test/java/com/lxmf/messenger/service/manager/ServiceNotificationManagerRNodeTest.kt	New Robolectric test class covering disconnect/reconnect, multi-interface tracking, foreground text, debounce, and the sync guard. The sync guard test assertion is always true regardless of whether the guard is active, leaving a regression path uncovered.
app/src/main/java/com/lxmf/messenger/service/binder/ReticulumServiceBinder.kt	Minimal, correct change: threads interfaceName through onRNodeOnlineStatusChanged and wires the call to notificationManager.updateRNodeStatus.
reticulum/src/main/java/com/lxmf/messenger/reticulum/rnode/KotlinRNodeBridge.kt	Adds interfaceName parameter to the RNodeOnlineStatusListener interface and notifyOnlineStatusChanged, propagates it through the listener dispatch loop. Also includes several cosmetic expression-body refactors that are correct.
reticulum/src/test/java/com/lxmf/messenger/reticulum/rnode/KotlinRNodeBridgeOnlineStatusTest.kt	All existing tests updated to pass testInterfaceName; a new test verifies the interface name is correctly forwarded to listeners. No issues found.
python/reticulum_wrapper.py	Single-line change: passes interface_name as second argument to notifyOnlineStatusChanged, consistent with the updated Kotlin bridge signature.
python/test_wrapper_ble.py	Test assertion updated to verify interface_name ('rnode0') is now passed through to notifyOnlineStatusChanged alongside the online status boolean.

Sequence Diagram

sequenceDiagram
    participant PY as reticulum_wrapper.py
    participant KRB as KotlinRNodeBridge
    participant RSB as ReticulumServiceBinder
    participant SNM as ServiceNotificationManager
    participant NM as NotificationManager (Android)

    Note over PY: RNode hardware disconnects
    PY->>KRB: notifyOnlineStatusChanged(false, interfaceName)
    KRB->>RSB: onRNodeOnlineStatusChanged(false, interfaceName)
    RSB->>SNM: updateRNodeStatus(false, interfaceName)
    Note over SNM: mainHandler.post {}
    SNM->>SNM: disconnectedRNodeInterfaces.add(interfaceName)
    SNM->>NM: activeNotifications.any { id == NOTIFICATION_ID_RNODE }
    alt Notification already visible
        SNM->>NM: notify(NOTIFICATION_ID_RNODE, alert) [silent content update]
    else Not visible AND outside debounce window
        SNM->>NM: notify(NOTIFICATION_ID_RNODE, alert) [heads-up]
    end
    SNM->>NM: notify(NOTIFICATION_ID, foreground) [appends "(RNode disconnected)"]

    Note over PY: RNode hardware reconnects
    PY->>KRB: notifyOnlineStatusChanged(true, interfaceName)
    KRB->>RSB: onRNodeOnlineStatusChanged(true, interfaceName)
    RSB->>SNM: updateRNodeStatus(true, interfaceName)
    Note over SNM: mainHandler.post {}
    SNM->>SNM: disconnectedRNodeInterfaces.remove(interfaceName)
    alt All interfaces back online
        SNM->>NM: cancel(NOTIFICATION_ID_RNODE)
        SNM->>SNM: lastDisconnectNotifyMs = 0L
        SNM->>NM: notify(NOTIFICATION_ID, foreground) [removes "(RNode disconnected)"]
    else Some interfaces still disconnected
        SNM->>NM: notify(NOTIFICATION_ID_RNODE, alert) [update names]
    end

Comments Outside Diff (1)

app/src/test/java/com/lxmf/messenger/service/manager/ServiceNotificationManagerRNodeTest.kt, line 408-427 (link)

Sync guard test assertion is always true — guard is never actually verified

The assertion assertEquals(beforeCount + 1, allNotifications.size) passes regardless of whether the sync guard is functioning, making this test a false positive.

Here's why: repostNotification falls back to notificationManager.notify(NOTIFICATION_ID, ...) when service is null (which it is in the test, since no Service is attached). After updateSyncProgress + drainMainLooper(), notification 1001 already exists in allNotifications. If updateRNodeStatus were to call repostNotification despite the sync guard (i.e., the guard was removed), it would call notificationManager.notify(1001, ...) again — which replaces the existing notification in-place. Since replacing does not change allNotifications.size, the count is still beforeCount + 1 either way.

To actually verify the sync guard, the test needs to check the content of the foreground notification, not just the count. For example, verify that notification 1001 still contains sync text (not RNode-disconnected text) after updateRNodeStatus runs:
```
// After draining, the foreground notification should still show sync content, not RNode state
val foregroundNotif = shadowNotificationManager.getNotification(ServiceNotificationManager.NOTIFICATION_ID)
assertNotNull("Foreground notification should exist", foregroundNotif)
val bigText = foregroundNotif.extras.getString("android.bigText") ?: ""
assertFalse(
    "Foreground notification should NOT include '(RNode disconnected)' during active sync",
    bigText.contains("(RNode disconnected)"),
)
```

_{Last reviewed commit: 9814138}

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt

Thread interface name through Python→KotlinRNodeBridge→ServiceNotificationManager so users get a visible heads-up alert when their RNode radio loses connection, plus a "(RNode disconnected)" suffix on the foreground service notification. Closes #625 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

torlando-tech · 2026-03-11T17:37:07Z

@greptileai

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt

Address Greptile review feedback: - Replace single rnodeOnline boolean with per-interface MutableSet (ConcurrentHashMap.newKeySet) so one interface reconnecting doesn't dismiss the alert for another still-disconnected interface - Add 10s debounce on heads-up disconnect notifications to prevent spam on flaky BLE connections - Add ServiceNotificationManagerRNodeTest (7 tests) covering disconnect/reconnect, per-interface tracking, foreground text, and debounce behaviour Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

torlando-tech · 2026-03-11T17:48:29Z

@greptileai

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt

Reset lastDisconnectNotifyMs to 0 when all interfaces reconnect and the alert is cancelled, so a fresh disconnect always fires a heads-up notification even if it occurs within 10s of a previous one. Update tests to verify reconnect-disconnect cycles re-post the alert, and add test for debounce suppression when a second interface disconnects while the alert is already visible. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt

torlando-tech · 2026-03-11T19:04:12Z

@greptileai

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt

Separate content updates from heads-up gating: notify() is always called to keep the notification body accurate (showing only currently disconnected interfaces), while the debounce timer only controls whether the heads-up overlay (sound + vibration) re-triggers. Track rnodeAlertVisible so a fresh disconnect always fires an alert after the user taps to dismiss (autoCancel) or after a reconnect. Add tests for partial-reconnect content updates and multi-interface notification body accuracy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

torlando-tech · 2026-03-11T19:27:47Z

@greptileai

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt

The debounce condition was updating lastDisconnectNotifyMs but notify() was called unconditionally — making the debounce dead code. Now notify() is only called inside the debounce branches: - Fresh heads-up: when alert is not visible or cooldown expired - Silent content update: when alert is already in the shade Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

sentry · 2026-03-11T19:38:02Z

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt

+                    rnodeAlertVisible = true
+                } else if (rnodeAlertVisible) {
+                    // Silent content-only update while alert is still in the shade
+                    notificationManager.notify(NOTIFICATION_ID_RNODE, alert)
+                }
+            } else {
+                // All RNode interfaces are online — dismiss the alert
+                notificationManager.cancel(NOTIFICATION_ID_RNODE)
+                rnodeAlertVisible = false
+                lastDisconnectNotifyMs = 0L
+            }


Bug: If a user dismisses the disconnect notification, rnodeAlertVisible is not reset. A subsequent disconnect within the cooldown period incorrectly re-posts a new heads-up notification, bypassing the debounce logic.
_{Severity: MEDIUM}

Suggested Fix

Use setDeleteIntent on the notification. This allows you to provide a PendingIntent that will be sent when the user dismisses the notification, which can then be used to reset the rnodeAlertVisible flag to false and keep the state synchronized.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt#L249-L266 Potential issue: The `rnodeAlertVisible` flag is not reset to false when a user dismisses the RNode disconnect notification. If another disconnect event occurs within the 10-second cooldown period, the code enters a branch intended for silent updates. However, since the notification was already dismissed, calling `notificationManager.notify()` creates a new notification. This new notification, being on a high-importance channel, incorrectly triggers a new heads-up alert with sound and vibration, defeating the purpose of the debounce logic which is to prevent notification spam.

torlando-tech · 2026-03-11T19:39:03Z

@greptileai

app/src/test/java/com/lxmf/messenger/service/manager/ServiceNotificationManagerRNodeTest.kt

Verify that updateRNodeStatus does not refresh the foreground notification while a propagation sync is in progress, covering the documented but previously untested behaviour. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

rnodeAlertVisible was never cleared when the user tapped the notification (autoCancel), leaving the debounce in a stale state. Replace the manual flag with notificationManager.activeNotifications query — this gives ground truth about whether the notification is currently in the shade, regardless of how it was dismissed (user tap, swipe, or programmatic cancel). The debounce now has two clear branches: - Visible: silent content-only update (always allowed) - Not visible: fresh heads-up gated by 10s cooldown Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

torlando-tech · 2026-03-11T19:57:19Z

@greptileai

sentry · 2026-03-11T19:58:24Z

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt

+                } else {
+                    // Notification not visible — debounce fresh heads-up postings
+                    val now = System.currentTimeMillis()
+                    if (now - lastDisconnectNotifyMs >= disconnectNotifyCooldownMs) {
+                        lastDisconnectNotifyMs = now
+                        notificationManager.notify(NOTIFICATION_ID_RNODE, alert)
+                    }
+                }


Bug: If a user dismisses a disconnect notification, a subsequent disconnect within the 10-second debounce window will not trigger a new notification, leaving the user uninformed.
_{Severity: MEDIUM}

Suggested Fix

Modify the logic to ensure notify() is always called when the notification is not visible (isCurrentlyVisible is false), regardless of the debounce timer. The debounce logic should only control whether the notification is posted with high priority (triggering a heads-up display and sound) versus standard priority (a silent update to the notification shade).

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt#L256-L263 Potential issue: The logic to show RNode disconnect notifications fails to re-post a notification under specific conditions. If a user dismisses the initial disconnect notification, and a second interface disconnects within the 10-second debounce window, the `notify()` function is not called. This happens because the code checks if the notification `isCurrentlyVisible` (which is false after dismissal) and then checks if the debounce period has expired. When the debounce is still active, neither condition is met, and the notification is not re-displayed, leaving the user unaware of the second disconnection.

torlando-tech marked this pull request as ready for review March 11, 2026 04:26

greptile-apps bot reviewed Mar 11, 2026

View reviewed changes

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt Outdated Show resolved Hide resolved

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt Outdated Show resolved Hide resolved

torlando-tech force-pushed the feat/rnode-disconnect-notification branch from 5db2fd5 to 51f523d Compare March 11, 2026 17:30

greptile-apps bot reviewed Mar 11, 2026

View reviewed changes

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt Outdated Show resolved Hide resolved

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt Outdated Show resolved Hide resolved

greptile-apps bot reviewed Mar 11, 2026

View reviewed changes

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt Show resolved Hide resolved

torlando-tech force-pushed the feat/rnode-disconnect-notification branch from 0f81b4f to 3b8875e Compare March 11, 2026 18:44

sentry bot reviewed Mar 11, 2026

View reviewed changes

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt Outdated Show resolved Hide resolved

greptile-apps bot reviewed Mar 11, 2026

View reviewed changes

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt Outdated Show resolved Hide resolved

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt Outdated Show resolved Hide resolved

greptile-apps bot reviewed Mar 11, 2026

View reviewed changes

app/src/main/java/com/lxmf/messenger/service/manager/ServiceNotificationManager.kt Outdated Show resolved Hide resolved

sentry bot reviewed Mar 11, 2026

View reviewed changes

greptile-apps bot reviewed Mar 11, 2026

View reviewed changes

app/src/test/java/com/lxmf/messenger/service/manager/ServiceNotificationManagerRNodeTest.kt Show resolved Hide resolved

torlando-tech and others added 2 commits March 11, 2026 15:53

sentry bot reviewed Mar 11, 2026

View reviewed changes

torlando-tech merged commit 46504f7 into main Mar 11, 2026
25 of 27 checks passed

torlando-tech deleted the feat/rnode-disconnect-notification branch March 11, 2026 20:21

Uh oh!

Conversation

torlando-tech commented Mar 8, 2026

Summary

Test plan

Uh oh!

sentry bot commented Mar 8, 2026

Codecov Report

Uh oh!

torlando-tech commented Mar 11, 2026

Uh oh!

greptile-apps bot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Comments Outside Diff (1)

Uh oh!

Uh oh!

Uh oh!

torlando-tech commented Mar 11, 2026

Uh oh!

Uh oh!

Uh oh!

torlando-tech commented Mar 11, 2026

Uh oh!

Uh oh!

Uh oh!

torlando-tech commented Mar 11, 2026

Uh oh!

Uh oh!

Uh oh!

torlando-tech commented Mar 11, 2026

Uh oh!

Uh oh!

sentry bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

torlando-tech commented Mar 11, 2026

Uh oh!

Uh oh!

torlando-tech commented Mar 11, 2026

Uh oh!

sentry bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

greptile-apps bot commented Mar 11, 2026 •

edited

Loading