fix: Defer setting connected state until after nodeDB load#4505
Conversation
This commit moves the setting of the `Connected` state and the connection broadcast to after the node database has finished loading. Previously, the connected state was set when the radio configuration was committed. This could lead to a race condition where the UI might try to access node information before the node database was fully loaded. By deferring the `Connected` state update, we ensure that the application is fully ready and the node database is populated before other components are notified of the connection. Additionally, a check in `onConnectionChanged` is updated to allow re-broadcasting the `Connected` state. Signed-off-by: James Rich <2199651+jamesarich@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adjusts the mesh service connection lifecycle so the app only transitions to (and broadcasts) Connected after the node database has finished loading, preventing UI/components from observing a “connected” state before node data is available.
Changes:
- Move
ConnectionState.Connected+ connection broadcast from “radio config committed” to “node DB ready”. - Loosen the
onConnectionChangedearly-return logic to permit re-processingConnectedevents.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/src/main/java/com/geeksville/mesh/service/MeshConnectionManager.kt | Updates connection-state change gating to allow Connected to pass through even when unchanged. |
| app/src/main/java/com/geeksville/mesh/service/MeshConfigFlowManager.kt | Defers setting/broadcasting Connected until after node DB load completion. |
| myNodeInfo?.let { | ||
| nodeRepository.installConfig(it, entities) | ||
| sendAnalytics(it) | ||
| } | ||
| nodeManager.isNodeDbReady.value = true | ||
| nodeManager.allowNodeDbWrites.value = true | ||
| connectionStateHolder.setState(ConnectionState.Connected) | ||
| serviceBroadcasts.broadcastConnection() |
There was a problem hiding this comment.
handleNodeInfoComplete() now sets the service to Connected and broadcasts connection even if myNodeInfo is null (i.e., the config-only stage logged an invalid config and never committed myNodeInfo). That can mark the app as connected with an incomplete/invalid local config. Consider gating setState(Connected)/broadcastConnection() (and possibly isNodeDbReady) on myNodeInfo != null (or otherwise handling the error path explicitly).
| if (connectionStateHolder.connectionState.value == c && c !is ConnectionState.Connected) return | ||
| Logger.d { "onConnectionChanged: ${connectionStateHolder.connectionState.value} -> $c" } |
There was a problem hiding this comment.
The new equality check means that if onConnectionChanged(ConnectionState.Connected) is invoked while the service state is already Connected (e.g., MeshService is recreated while ConnectionStateHandler is still Connected in the same process), this will fall through and call handleConnected(), which clears node info and restarts the config-only flow. If the intent is only to re-broadcast, add a special-case branch that only re-broadcasts/updates notifications when already Connected, and returns without calling handleConnected() (or revert to the previous early-return behavior and trigger re-broadcast elsewhere).
| if (connectionStateHolder.connectionState.value == c && c !is ConnectionState.Connected) return | |
| Logger.d { "onConnectionChanged: ${connectionStateHolder.connectionState.value} -> $c" } | |
| val previousState = connectionStateHolder.connectionState.value | |
| if (previousState == c) { | |
| if (c is ConnectionState.Connected) { | |
| Logger.d { "onConnectionChanged: already Connected, re-broadcasting connection" } | |
| // Service recreated while we still think we're connected: just re-broadcast/update UI. | |
| serviceBroadcasts.broadcastConnection() | |
| updateStatusNotification() | |
| } | |
| return | |
| } | |
| Logger.d { "onConnectionChanged: $previousState -> $c" } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4505 +/- ##
==========================================
+ Coverage 11.03% 11.05% +0.02%
==========================================
Files 429 429
Lines 14461 14461
Branches 2394 2394
==========================================
+ Hits 1596 1599 +3
+ Misses 12561 12555 -6
- Partials 304 307 +3 ☔ View full report in Codecov by Sentry. |
This commit moves the setting of the
Connectedstate and the connection broadcast to after the node database has finished loading.Previously, the connected state was set when the radio configuration was committed. This could lead to a race condition where the UI might try to access node information before the node database was fully loaded.
By deferring the
Connectedstate update, we ensure that the application is fully ready and the node database is populated before other components are notified of the connection. Additionally, a check inonConnectionChangedis updated to allow re-broadcasting theConnectedstate.