fix(settings): stop dropping admin config responses (0% stall, missing remote channels) - #6391
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe changes add request-ID callbacks before radio sends, track expected local node restarts, update connection and settings UI states, synchronize channel loading state, and treat queue status 35 as successful. Tests cover request ordering, restart lifecycle, queue completion, and updated view-model behavior. ChangesQueue and request lifecycle
Restart-aware connection and settings UI
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RadioConfigViewModel
participant NodeRestartTracker
participant MeshConnectionManagerImpl
participant ConnectionsViewModel
participant SettingsDialog
RadioConfigViewModel->>NodeRestartTracker: expectRestart()
NodeRestartTracker-->>ConnectionsViewModel: restartExpected = true
MeshConnectionManagerImpl->>NodeRestartTracker: onConnected()
NodeRestartTracker-->>ConnectionsViewModel: restartExpected = false
SettingsDialog-->>RadioConfigViewModel: display reboot-aware completion
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt (1)
390-413: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnregister failed radio sends instead of waiting 30 seconds.
These methods call
onRequestId(::registerRequestId)before theradioController.*/adminActionsUseCase.*send.safeLaunchcatches throwables, logsunknown_error, and returns normally, so a send failure after ID registration leaves the request inLoadingwith a pending 30s timeout. Wrap the send area with cleanup for the just-registered request IDs/request timeouts, or move registration/timeout cancellation out of the send path unless registration is guaranteed to complete successfully.🤖 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 `@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt` around lines 390 - 413, Update setHamMode and setOwner so a failed send cannot leave the newly registered request in Loading until the 30-second timeout. Track the request ID registered by onRequestId and clean up its request state/timeout when radioConfigUseCase.setHamMode or setOwner throws within safeLaunch, or move registration until the send is guaranteed to succeed while preserving successful request tracking.
🤖 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
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.kt`:
- Around line 175-177: Update the success/full early-return condition in the
queue status handling of PacketHandlerImpl so it returns only when res == 0 and
free == 0; ERRNO_SHOULD_RELEASE must continue to queueResponse even when the
queue is full. Add a regression test covering res == ERRNO_SHOULD_RELEASE with
free == 0 and verifying queueResponse completes.
---
Outside diff comments:
In
`@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt`:
- Around line 390-413: Update setHamMode and setOwner so a failed send cannot
leave the newly registered request in Loading until the 30-second timeout. Track
the request ID registered by onRequestId and clean up its request state/timeout
when radioConfigUseCase.setHamMode or setOwner throws within safeLaunch, or move
registration until the send is guaranteed to succeed while preserving successful
request tracking.
🪄 Autofix (Beta)
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: 0d5d0d20-ad28-4adb-8609-68853bc0b2ec
📒 Files selected for processing (8)
core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.ktcore/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/settings/AdminActionsUseCase.ktcore/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/settings/RadioConfigUseCase.ktcore/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/settings/RadioConfigUseCaseTest.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/channel/ChannelConfigScreen.ktfeature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt
Comment on lines
175
to
177
| val (success, isFull, requestId) = | ||
| with(queueStatus) { Triple(res == 0 || res == ERRNO_SHOULD_RELEASE, free == 0, mesh_packet_id) } | ||
| if (success && isFull) return |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not suppress ERRNO_SHOULD_RELEASE when the queue is full.
Because status 35 now makes success true, res == 35 && free == 0 returns before completing queueResponse. A locally delivered packet can therefore time out whenever the TX queue has no free slots. Restrict the early return to res == 0 && free == 0, and add a regression test for this combination.
🤖 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
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.kt`
around lines 175 - 177, Update the success/full early-return condition in the
queue status handling of PacketHandlerImpl so it returns only when res == 0 and
free == 0; ERRNO_SHOULD_RELEASE must continue to queueResponse even when the
queue is full. Add a regression test covering res == ERRNO_SHOULD_RELEASE with
free == 0 and verifying queueResponse completes.
Collaborator
Author
|
Remote admin verification (the #6317 scenario, on-hardware): Setup: Pixel 6a ↔ M5Stack Cardputer (local, BLE, fw 2.8.0.d9150e8) remote-administering a Heltec V3 ( Result with this branch: opening the remote Channels screen runs the full sequential fetch — 🤖 Generated with Claude Code |
Radio-config sub-screens correlate admin responses to requests via a request-id set, but the id was registered only after the suspending send returned — and the send suspends until the radio acks the packet via QueueStatus. Firmware 2.8 (meshtastic/firmware#10967) routes self-addressed packets through a synchronous local loopback, so the admin response now reaches the phone BEFORE that ack. The response flowed through meshPacketFlow while the request-id set was still empty, was silently dropped by the correlation guard, and every local config screen sat at a 0% loading overlay until the 30s timeout. Every request method on RadioConfigUseCase / AdminActionsUseCase now takes an onRequestId callback invoked with the packet id before the send is issued, and RadioConfigViewModel registers there instead of after the call. The manual-channel batch helper threads the same callback through writeChannel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ccess Firmware 2.8 returns its internal ErrorCode ERRNO_SHOULD_RELEASE (35, "no error, but the packet should still be released", MeshTypes.h) in QueueStatus.res for self-addressed packets delivered through the synchronous local loopback instead of the TX queue. The handler treated any non-zero res as a failed send, marking every local admin/telemetry request "success false". Note 35 numerically collides with Routing.Error.PKI_UNKNOWN_PUBKEY — QueueStatus.res carries ErrorCode semantics, not Routing.Error, so this is a benign local delivery, not a PKI failure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…equest-chain gap Fixes #6317. The channel editor copied the authoritative channel list into its editable snapshot exactly once, at first composition — which happens the moment channel 0 arrives. A remote channel fetch streams the remaining channels in one response at a time after that seed, so they never rendered; the footer Cancel button's replaceWith() was the accidental community workaround. The editor now adopts the authoritative list while the fetch is in flight (the loading overlay blocks user edits during that window, so no edits can be clobbered). Also defer the request-id removal in processPacketResponse behind any chain continuation launched by the same response (launches run FIFO on the main dispatcher, and registration is the continuation's first act). The inline removal could observe a momentarily-empty request set in the gap between chained getChannel requests, tear down the whole flow via clearPacketResponse, and strand the rest of the chain — leaving a partial channel list and a stranded loading overlay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Firmware applies most config sections with a node reboot a few seconds after
the save is acked (module saves even disable Bluetooth immediately). Today
the app cries wolf — every save shows the same "may disconnect and reboot"
line, including saves that never reboot — and the resulting BLE drop renders
as an alarming red "disconnected" that looks identical to a real failure.
Worse, the save's own response waits for a routing ACK the reboot eats, so
it sits at 0% for 30s then throws a spurious "Timeout" error.
Three changes:
**Attribute the disconnect.** A NodeRestartTracker opens a ~90s "expected
restart" window at send time whenever a LOCAL save/action reboots the
connected node (reboot-applying config/module sections, explicit reboot,
factory reset, ham-mode). Remote destinations don't open it — a remote reboot
doesn't drop our transport. While open, the transport drop presents as
ConnectionStatus.RESTARTING ("Restarting…" on the Connection card, orange
connecting-treatment nav icon instead of red) and the foreground-service
notification keeps the connecting presentation. The window closes when the
post-reboot handshake completes (ConnectionState.Connected) or expires.
**Complete the save on reboot.** A reboot-applying save can't survive the
reboot it triggers. The transport-drop during the restart window IS the
confirmation the save was persisted (firmware only reboots after saveChanges
writes to disk), so the pending save resolves to a "node is restarting"
success the moment the node drops, rather than hanging to a 30s timeout error.
The request timeout carries a matching backstop.
**Honest consent.** A coarse RebootBehavior map (ALWAYS / MAY_RESTART / NEVER)
mirrors the firmware's per-section reboot decision. Always-reboot sections
(Position, Network, Bluetooth, Security + every module except status message)
show a "Save & restart" footer and a "the node is restarting" success notice;
field-dependent sections keep the softer "may reboot" copy; sections that
never reboot (Channels, Status Message) drop the warning entirely. The map is
deliberately coarse — firmware's decision is field-level and drifts between
releases, so the app never claims precision it can't keep.
Verified on hardware (Pixel 6a ↔ Heltec DUT, fw 2.8.0): saving Position shows
"Save & restart", resolves to the restarting-success (no more 0%/timeout),
presents "Restarting…" with an orange (not red) nav icon throughout the
disconnect, and clears to Connected on reconnect.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jamesarich
force-pushed
the
claude/packet-authenticity-2-8-0-5b07c8
branch
from
July 23, 2026 18:48
4b29b97 to
33b961c
Compare
… full Widening handleQueueStatus success to include ERRNO_SHOULD_RELEASE (35) also widened the `success && free == 0` early return, so a self-addressed local-loopback delivery (res=35) that coincided with a full TX queue (free=0) returned before completing queueResponse — hanging until the 5s TIMEOUT, the exact stall this branch set out to fix. Scope the early return to the plain res=0 "accepted, now full" echo. res=35 now always completes its response. Adds a regression test for res=35 + free=0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/viewmodel/ConnectionsViewModelTest.kt (2)
66-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame real-dispatcher
NodeRestartTrackerconstruction as the other ViewModel test files.See consolidated comment (anchored on
RadioConfigViewModelTest.kt) for the shared fix.🤖 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 `@core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/viewmodel/ConnectionsViewModelTest.kt` at line 66, Update the nodeRestartTracker initialization in ConnectionsViewModelTest to use the same test dispatcher and lifecycle setup as the consolidated fix in the other ViewModel tests, rather than constructing NodeRestartTracker with a standalone CoroutineScope(SupervisorJob()).
156-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test covers the new
ConnectionStatus.RESTARTINGmapping.
connectionStatusnow mapsConnecting/DisconnectedtoRESTARTINGwhennodeRestartTracker.restartExpectedis true, but no test in this file drivesnodeRestartTracker.expectRestart()to exercise that branch — existing tests only cover the pre-existing CONNECTING/RECONNECTING/NOT_CONNECTED paths. Worth adding a test analogous toConnecting state maps to CONNECTING regardless of progress textthat callsnodeRestartTracker.expectRestart()first and assertsRESTARTINGfor bothConnectingandDisconnected.🤖 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 `@core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/viewmodel/ConnectionsViewModelTest.kt` around lines 156 - 182, The connection status tests do not cover the RESTARTING mapping when a restart is expected. Add a test in ConnectionsViewModelTest analogous to the existing Connecting-state test, call nodeRestartTracker.expectRestart() before changing states, and assert RESTARTING for both Connecting and Disconnected states.feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt (1)
371-382: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM overall on the restart-window plumbing; minor style inconsistency.
FACTORY_RESETcallsnodeRestartTracker.expectRestart()directly instead of going through theexpectRestartIfLocalhelper used everywhere else (setHamMode,setConfig,setModuleConfig,REBOOT). Functionally equivalent since it's already gated byisLocal, but routing through the shared helper would keep the "who opens the window" logic in one place.Also applies to: 601-605, 618-623
🤖 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 `@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt` around lines 371 - 382, Route the FACTORY_RESET handling, including the related paths around setHamMode/setConfig/setModuleConfig and REBOOT, through expectRestartIfLocal instead of calling nodeRestartTracker.expectRestart() directly. Preserve the existing local-node and reboot-behavior gating while centralizing restart-window triggering in the shared helper.feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/ProfileRoundTripTest.kt (1)
92-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame real-dispatcher
NodeRestartTrackerconstruction as the other ViewModel test files.See consolidated comment (anchored on
RadioConfigViewModelTest.kt) for the shared fix.🤖 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 `@feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/ProfileRoundTripTest.kt` at line 92, Update ProfileRoundTripTest’s nodeRestartTracker initialization to use the same test-safe dispatcher or coroutine-scope construction established by the consolidated fix in RadioConfigViewModelTest.kt, rather than creating NodeRestartTracker with a real-dispatcher CoroutineScope. Keep the tracker behavior and test lifecycle consistent with the other ViewModel tests.feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt (1)
126-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
NodeRestartTrackerbacked by a real dispatcher instead of the test's virtual-time scheduler.Unlike
NodeRestartTrackerTest.ktandMeshConnectionManagerImplTest.kt(which usebackgroundScope), this constructsNodeRestartTracker(CoroutineScope(SupervisorJob()))— a scope with noTestDispatcher, so its internaldelay(window)expiry job runs onDispatchers.Defaultin real time, unaffected byadvanceTimeBy/runCurrent, and is never cancelled at teardown. See consolidated comment for the shared fix across this file,ProfileRoundTripTest.kt, andConnectionsViewModelTest.kt.🤖 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 `@feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt` at line 126, Update the NodeRestartTracker test fixture to construct it with the test-managed backgroundScope instead of CoroutineScope(SupervisorJob()). Ensure the tracker uses the test scheduler for virtual-time control and is cancelled automatically during teardown, matching the setup in NodeRestartTrackerTest and MeshConnectionManagerImplTest.
🤖 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
`@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt`:
- Around line 865-879: Prevent stale restart expectations from completing manual
channel batches as successful saves. Update completeRestartingSaveIfPending and
the registerRequestId timeout backstop to require the pending loading state is
not a beginManualChannelBatch operation, while preserving the existing
restartExpected and empty-route checks.
- Around line 625-629: Update the NODEDB_RESET branch in the safeLaunch block to
call nodeRestartTracker.expectRestart() when isLocal is true, immediately before
adminActionsUseCase.nodedbReset(...). Keep remote resets unchanged and match the
existing FACTORY_RESET behavior.
---
Nitpick comments:
In
`@core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/viewmodel/ConnectionsViewModelTest.kt`:
- Line 66: Update the nodeRestartTracker initialization in
ConnectionsViewModelTest to use the same test dispatcher and lifecycle setup as
the consolidated fix in the other ViewModel tests, rather than constructing
NodeRestartTracker with a standalone CoroutineScope(SupervisorJob()).
- Around line 156-182: The connection status tests do not cover the RESTARTING
mapping when a restart is expected. Add a test in ConnectionsViewModelTest
analogous to the existing Connecting-state test, call
nodeRestartTracker.expectRestart() before changing states, and assert RESTARTING
for both Connecting and Disconnected states.
In
`@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt`:
- Around line 371-382: Route the FACTORY_RESET handling, including the related
paths around setHamMode/setConfig/setModuleConfig and REBOOT, through
expectRestartIfLocal instead of calling nodeRestartTracker.expectRestart()
directly. Preserve the existing local-node and reboot-behavior gating while
centralizing restart-window triggering in the shared helper.
In
`@feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/ProfileRoundTripTest.kt`:
- Line 92: Update ProfileRoundTripTest’s nodeRestartTracker initialization to
use the same test-safe dispatcher or coroutine-scope construction established by
the consolidated fix in RadioConfigViewModelTest.kt, rather than creating
NodeRestartTracker with a real-dispatcher CoroutineScope. Keep the tracker
behavior and test lifecycle consistent with the other ViewModel tests.
In
`@feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt`:
- Line 126: Update the NodeRestartTracker test fixture to construct it with the
test-managed backgroundScope instead of CoroutineScope(SupervisorJob()). Ensure
the tracker uses the test scheduler for virtual-time control and is cancelled
automatically during teardown, matching the setup in NodeRestartTrackerTest and
MeshConnectionManagerImplTest.
🪄 Autofix (Beta)
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: fcb7e481-dd45-4eb6-9f11-75ad31578a8b
📒 Files selected for processing (43)
.skills/compose-ui/strings-index.txtcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConnectionManagerImpl.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshConnectionManagerImplTest.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.ktcore/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/settings/AdminActionsUseCase.ktcore/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/settings/RadioConfigUseCase.ktcore/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/settings/RadioConfigUseCaseTest.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/NodeRestartTracker.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/di/CoreRepositoryModule.ktcore/repository/src/commonTest/kotlin/org/meshtastic/core/repository/NodeRestartTrackerTest.ktcore/resources/src/commonMain/composeResources/values/strings.xmlcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/MeshtasticNavigationSuite.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/ConnectionsViewModel.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/viewmodel/ConnectionsViewModelTest.ktfeature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/ConnectingDeviceInfo.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RebootBehavior.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/channel/ChannelConfigScreen.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/channel/ChannelScreen.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/AmbientLightingConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/AudioConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/BluetoothConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/CannedMessageConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/DetectionSensorConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/MQTTConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/NeighborInfoConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/NetworkConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/PacketResponseStateDialog.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/PaxcounterConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/PositionConfigScreen.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/RadioConfigScreenList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/RangeTestConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/RemoteHardwareConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/SecurityConfigScreen.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/SerialConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/StatusMessageConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/StoreForwardConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/TAKConfigItemList.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/TelemetryConfigItemList.ktfeature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/ProfileRoundTripTest.ktfeature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.kt
- feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/channel/ChannelConfigScreen.kt
- core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.kt
- core/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/settings/RadioConfigUseCaseTest.kt
- core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/settings/RadioConfigUseCase.kt
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.kt (1)
137-151: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the preserved
res == 0 && free == 0branch.These tests prove that
res = 35completes successfully when the queue is full, but do not protect the intentional early return for ordinary successful responses withfree = 0. Add a regression test that confirms such a response does not complete the awaited result.🤖 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 `@core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.kt` around lines 137 - 151, Add a test alongside handleQueueStatus completes ERRNO_SHOULD_RELEASE even when queue is full that starts sendToRadioAndAwait, processes QueueStatus with the matching mesh_packet_id, res = 0, and free = 0, then verifies the awaited result remains incomplete without hanging the test. Preserve the existing res = 35 coverage and assert the plain successful full-queue response follows the intentional early-return path.
🤖 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.
Nitpick comments:
In
`@core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.kt`:
- Around line 137-151: Add a test alongside handleQueueStatus completes
ERRNO_SHOULD_RELEASE even when queue is full that starts sendToRadioAndAwait,
processes QueueStatus with the matching mesh_packet_id, res = 0, and free = 0,
then verifies the awaited result remains incomplete without hanging the test.
Preserve the existing res = 35 coverage and assert the plain successful
full-queue response follows the intentional early-return path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a564cbc6-ed5c-41ea-a7ea-d9bccd552f1c
📒 Files selected for processing (2)
core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.kt
…anual channel batches Two CodeRabbit findings on the reboot-visibility change: - NODEDB_RESET now opens the expected-restart window for a local reset, mirroring FACTORY_RESET. Firmware reboots after a nodedb reset, so without this the ensuing transport drop surfaced as a surprise disconnect instead of "restarting". - A manual channel batch shares the save shape (empty route + Loading) that the restart-success paths key on, so a stale restart window (a local MAY_RESTART save that never actually rebooted) could flip an incomplete batch to Success on a transient disconnect or request timeout — silently misreporting a partial channel write. Gate both completeRestartingSaveIfPending and the request-timeout backstop on manualChannelBatchInFlight(), which tracks the batch from enqueue through its ack-wait (past finishManualChannelBatch) via the pending batch request ids — the enqueue flag alone leaves the ack-wait window exposed. Adds regression tests for both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8 tasks
This was referenced Aug 11, 2026
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.
Firmware 2.8 (meshtastic/firmware#10967) delivers a locally connected node's admin responses through a synchronous loopback, so the response now reaches the phone before the
QueueStatusack for the request that produced it. The app registered each request's id for response correlation only after the ack-awaiting send returned — the response sailed throughmeshPacketFlowwhile the id set was still empty, was silently dropped by the correlation guard, and every radio-config sub-screen stalled at a 0% loading overlay for 30s before a timeout dialog against 2.8 firmware. Root-caused with a dual-ended capture (app logcat + firmware serial): the device handledGet config: LoRaand enqueued its reply to the phone in milliseconds; the app dropped it.The same audit closed two adjacent holes behind #6317 (remote channel list shows only the first channel; the community workaround was "press Cancel"):
Likely also resolves #5592 (settings screens hang at 0% indefinitely) — same symptom, though that report is on 2.7.x firmware while the root cause fixed here is the 2.8 loopback-ordering race; worth confirming the reporter's stall clears before closing.
🐛 Fixes
RadioConfigUseCase/AdminActionsUseCaserequest methods take anonRequestIdcallback invoked with the packet id ahead of the send;RadioConfigViewModelregisters there instead of after the suspending call returns. Response ordering can no longer race registration, local or remote.QueueStatus.res = 35is not a failure. Firmware 2.8 returns its internalERRNO_SHOULD_RELEASE("delivered locally, packet consumed") for self-addressed packets; the handler treated any non-zeroresas a failed send. Note: 35 numerically collides withRouting.Error.PKI_UNKNOWN_PUBKEY—QueueStatus.rescarriesErrorCodesemantics, notRouting.Error(diagnosis trap). ThehandleQueueStatusearly return is scoped to the plainres = 0"accepted, now full" echo, so ares = 35delivery still completes its response even when the TX queue is full (free = 0) — otherwise it would re-introduce the same 5s-timeout stall under queue pressure.replaceWith()was the accidental workaround. The editor adopts the authoritative list while the fetch is in flight (the loading overlay blocks edits during that window, so nothing can be clobbered).processPacketResponseremoved the completed request id inline, and could observe a momentarily-empty id set in the gap between chainedgetChannelrequests — tearing the flow down (clearPacketResponse) and stranding the rest of the chain with a partial list. The removal now runs behind any chain continuation launched by the same response (FIFO on the main dispatcher; registration is the continuation's first act).NODEDB_RESETmarks an expected local restart. Firmware reboots after a nodedb reset, but onlyFACTORY_RESETopened the restart window — a local nodedb reset surfaced the ensuing transport drop as a surprise disconnect instead of "restarting".NODEDB_RESETnow callsexpectRestart()for local resets, mirroringFACTORY_RESET.Loading) that the restart-success paths key on. A localMAY_RESTARTsave that opened the 90s window but never actually rebooted left that window open; a transient disconnect or request timeout during a subsequent batch would then flip the incomplete batch toSuccess, silently misreporting a partial channel write. BothcompleteRestartingSaveIfPendingand the request-timeout backstop now gate onmanualChannelBatchInFlight(), which tracks the batch from enqueue through its ack-wait (pastfinishManualChannelBatch) via the pending batch request ids — the enqueue flag alone left the ack-wait window exposed.🧹 Follow-up (firmware, not this PR)
QueueStatus.resleaking internalERRNO_*values to clients is worth an upstream firmware issue — mappingERRNO_SHOULD_RELEASE→ 0 would spare every client the same trap.Testing Performed
getConfig invokes onRequestId with the packet id before issuing the send(ordering regression guard);handleQueueStatus treats ERRNO_SHOULD_RELEASE as success/other nonzero res as failure;handleQueueStatus completes ERRNO_SHOULD_RELEASE even when queue is full(theres = 35+free = 0queue-full path — covered by test, since on-device runs didn't exercise a full TX queue);NODEDB_RESET marks an expected local restart;manual channel batch is not completed as success by a stale restart window(this one caught an insufficient first fix that guarded only the enqueue phase, not the ack-wait — both restart-window edge cases are test-covered rather than device-exercised).spotlessApply spotlessCheck detekt assembleDebug test allTestsgreen.🤖 Generated with Claude Code
Summary by CodeRabbit