Skip to content

fix(node): stop rendering saved-on-phone nodes as freshly heard - #6747

Draft
jamesarich wants to merge 2 commits into
mainfrom
feat/6263-node-presence-display
Draft

fix(node): stop rendering saved-on-phone nodes as freshly heard#6747
jamesarich wants to merge 2 commits into
mainfrom
feat/6263-node-presence-display

Conversation

@jamesarich

Copy link
Copy Markdown
Collaborator

Summary

Root-caused via #6682 / #6693 (see the diagnosis comment on #6263): Node.hopsAway defaults to the sentinel -1 instead of null, and nothing distinguished a locally-retained node from one the connected radio actually reported this session. Together, a legitimately cached row (multi-radio use, or a radio whose own NodeDB is smaller/has been wiped relative to the phone's) rendered as if it were freshly heard, with 0 hops, 0 SNR, 0 RSSI.

  • Add Node.hopsAwayOrNull, extending the existing snrOrNull/rssiOrNull null-safety pattern already used in NodeItem.kt.
  • Fix both copies of getRelayNode — the one actually wired into production (Packet.Companion.getRelayNode, used by DebugViewModel) and the unused duplicate in Node.Companion — whose plain minByOrNull { it.hopsAway } let the unresolved -1 sentinel look "closer" than a real, known hop count.
  • Track the current connection session's exact NodeDB membership via NodeManager.currentSessionNodeNums: published right after each Stage 2 handshake completes, and reconciled against the same generation-race window already used for connectionIdentity (a delayed clearStaleConnectionIdentity collector must not blank a snapshot already published for the active generation).
  • Add a "Saved on phone" badge (node list rows, both densities, and the node detail screen) for any node retained locally but absent from that session snapshot — written fresh, not copied from the closed fix(nodes): mark nodes absent from connected radio #6693 (which was closed as low-effort, not as the wrong direction).
  • Stop those "saved on phone" rows from claiming online from a cached, no-longer-current lastHeard — in the visible list rows and in the TalkBack accessibility description — since that's the same false-freshness claim the badge exists to correct.

Scope notes / deliberate non-changes

  • CommonGetNodeDetailsUseCase's isSavedOnPhone derivation has no direct unit test. That class has 8 constructor dependencies and no existing test fixture at all (pre-existing gap, not introduced here); the logic itself is a one-line derivation (sessionNodeNums != null && node.num !in sessionNodeNums), and the session-membership flow it derives from is exhaustively covered in NodeManagerImplTest and NodeListViewModelTest.
  • Online counts, the online/direct node-list filters, and the hop histogram still use the cumulative phone DB, not session membership. That broader surface is what made the closed fix(nodes): mark nodes absent from connected radio #6693 read as overreaching; deliberately left out of this PR as follow-up work rather than folded in here.

Testing

  • direnv exec . ./gradlew spotlessApply spotlessCheck detekt assembleDebug test allTestsBUILD SUCCESSFUL, all tests (including every new/updated one) passed.
  • ./gradlew :screenshot-tests:updateDebugScreenshotTest then :screenshot-tests:validateDebugScreenshotTestBUILD SUCCESSFUL. Only 6 new golden PNGs were produced (the new "Saved on phone" previews); no existing golden changed, confirming the default (isSavedOnPhone = false) rendering is pixel-identical to before.
  • New tests: Node.hopsAwayOrNull (null + zero-value pair), Packet.getRelayNode / Node.getRelayNode sentinel-ordering regression, NodeManager.currentSessionNodeNums generation-reconciliation (including the delayed-collector race), MeshConfigFlowManagerImpl publish-before-ready ordering, NodeListViewModel pass-through, and Compose-level badge/online-suppression coverage in NodeItemZeroMetricsTest + BuildNodeDescriptionTest.

Fixes #6263

Root-caused via #6682/#6693: Node.hopsAway defaults to the sentinel -1
instead of null, and nothing distinguished a locally-retained node from
one the connected radio just reported this session. Together that made
a legitimately cached row (multi-radio use, or a radio with a smaller
NodeDB than the phone) render as if it were freshly heard with 0 hops,
0 SNR, 0 RSSI.

- Add Node.hopsAwayOrNull, extending the existing snrOrNull/rssiOrNull
  pattern, and fix both copies of getRelayNode (the live one in
  Packet.kt and the unused one in Node.kt) whose plain
  minByOrNull { it.hopsAway } let the unresolved -1 sentinel look
  closer than a real hop count.
- Track the current connection session's exact NodeDB membership via
  NodeManager.currentSessionNodeNums, published right after each Stage
  2 handshake and reconciled against the same generation-race window
  already used for connectionIdentity.
- Add a "Saved on phone" badge (node list rows + detail screen) for any
  node retained locally but absent from that snapshot, and stop those
  rows from claiming "online" from a cached, no-longer-current
  lastHeard (list rows, a11y description).

New/updated tests across core:model, core:data, core:ui, core:database
and feature:node; screenshot goldens regenerated for the new "Saved on
phone" previews. Full baseline
(spotlessApply/spotlessCheck/detekt/assembleDebug/test/allTests) and
:screenshot-tests:validateDebugScreenshotTest are green.

Fixes #6263

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added bugfix PR tag enhancement New feature or request labels Aug 16, 2026
@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8a22aac6-d00b-4ada-b3a8-13cdba17b5e9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

currentSessionNodeNums was captured once, from the Stage 2 NodeDB
download, and never updated again for the life of the connection. Any
node that announced itself afterwards — extremely common on a long-lived
BLE connection — was therefore absent from the set and got badged "Saved
on phone" with its online/fresh status suppressed, reintroducing the very
"presence looks wrong" defect #6263 exists to fix, only inverted:
genuinely fresh nodes now looked falsely stale.

Every mid-session path that learns about a node through the radio carries
a RadioSessionContext, and every local-only write (optimistic admin
projections, shared-contact imports, fixed-position edits) carries none.
That distinction is exactly the one the badge needs, so it drives the new
private noteHeardInSession(): NodeInfo via handleReceivedUser, position,
node status, PaxCounter, and the updateNodeForSession path that mesh
telemetry and admin replies arrive on all extend the set; sessionless
writes deliberately do not, since those nodes really are phone-only.

Membership is claimed only where the mutation actually committed. For
handleReceivedUser that means the winning CAS branch, using a new
ReceivedUserTransition.sessionMemberNodeNum so a suppressed retired-number
replay claims nothing and a stale noncanonical presentation credits the
canonical row it yields to rather than the slot it vacates. The generation
is re-read inside the StateFlow update so a concurrent session boundary
makes it a no-op instead of resurrecting a number into the new session's
set, and an already-present member returns the same instance rather than
copying a set of up to MAX_IN_MEMORY_NODES on every inbound packet.

The converse gap is documented rather than fixed: a node evicted from the
radio's own bounded NodeDB mid-session keeps its membership, because
firmware sends no eviction notification and the phone has no signal short
of re-downloading the NodeDB. Withholding a badge is the safe direction.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@jamesarich

Copy link
Copy Markdown
Collaborator Author

Follow-up: session membership now grows mid-session

Adversarial review of this branch caught a high-severity bug that undermined the whole point of the PR. Good that it did not ship.

The bug. currentSessionNodeNums was published exactly once, from the Stage 2 NodeDB download in installAndPublishNodeDatabase, and nothing updated it for the rest of the connection. Live mesh traffic arriving after the handshake — handleReceivedUser and friends — legitimately added nodes to the phone's node DB but never touched the session set. So on any long-lived BLE connection, a node that announced itself mid-session was absent from the set and got badged "Saved on phone" with its online/fresh status suppressed. That is the same "presence looks wrong" defect #6263 set out to fix, just inverted: instead of stale nodes looking falsely fresh, genuinely fresh nodes looked falsely stale.

The fix. Every mid-session path that learns about a node through the radio carries a RadioSessionContext; every local-only write (optimistic admin projections, shared-contact imports, fixed-position edits) carries none. That distinction is exactly the one the badge needs, so it drives a new private noteHeardInSession() in NodeManagerImpl — no new public API, and publishCurrentSessionNodeNums is untouched. NodeInfo, position, node status, PaxCounter, and the updateNodeForSession path that mesh telemetry and admin replies arrive on all extend the set now; sessionless writes deliberately do not, since those nodes genuinely are phone-only.

Three details worth flagging for re-review:

  • Only committed mutations claim membership. For handleReceivedUser that is the winning-CAS branch, via a new ReceivedUserTransition.sessionMemberNodeNum. A packet that loses the CAS is logged as discarded and claims nothing; a suppressed retired-number replay claims nothing; and a stale noncanonical presentation credits the canonical row it yields to rather than the slot it vacates, so no badge lands on a row held by a different identity.
  • Generation is re-read inside the StateFlow update, so a concurrent session boundary makes the add a no-op instead of resurrecting a number into the new session's set — the same race clearStaleConnectionIdentity already guards.
  • Already-a-member returns the same instance. This is the hot inbound path; a blind set + num would copy up to MAX_IN_MEMORY_NODES entries per packet for a change StateFlow conflates away anyway.

On the converse gap the reviewer raised (should a node evicted from the radio's own bounded NodeDB mid-session lose membership): documented in the KDoc rather than deferred, because it is not implementable as stated — firmware sends no eviction notification, so the phone has no signal short of re-downloading the whole NodeDB. Withholding a badge is the safe direction: it declines to claim locally-retained history rather than falsely claiming it.

Tests. Nine new cases in NodeManagerImplTest cover the gap the existing tests missed (they only exercised the one-time initial publish): a node absent from the Stage 2 snapshot then heard mid-session is not flagged saved-on-phone; the same for position, node status, and updateNodeForSession; plus the negatives that lock the discriminator in place — a sessionless update cannot claim membership, traffic from a superseded generation cannot contaminate the active set, traffic before any snapshot leaves it null rather than fabricating a partial one, a suppressed retired-number replay claims nothing, and repeat traffic allocates no new set.

Validation: spotlessApply spotlessCheck detekt assembleDebug test allTests all green (NodeManagerImplTest 108/108). Also ran :screenshot-tests:validateDebugScreenshotTest — passes with zero golden churn, as expected since this commit touches no Composable, layout, or string.

@jamesarich

Copy link
Copy Markdown
Collaborator Author

Parking this one in draft. The fix itself (session-membership tracking, sentinel-zero cleanup) is solid and verified, but the "Saved on phone" badge as a user-facing UX concept needs more thought before shipping — a persistent badge on node rows risks reading as noise/confusion rather than useful signal, especially for the common case of "just hasn't been heard from since reconnect yet." Not rejecting the approach, just not confident enough in the UX to merge as-is. Will revisit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backlog bugfix PR tag enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: App-provided nodes appear in list with spurious 0SNR 0RSSI 0HOPS

1 participant