Fix device configuration streaming and Settings lifecycle#1325
Open
RCGV1 wants to merge 2 commits into
Open
Conversation
|
@RCGV1 is attempting to deploy a commit to the Meshtastic Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe changes add LoRa configuration defaults, make configuration waiting resolve from device-store updates, remount settings forms after resets, improve editor dirty-state handling, and replace the SDK’s configuration handshake with a nonce-validated two-stage flow using unacknowledged packet sends. ChangesWeb configuration behavior
SDK configuration handshake
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MeshClient
participant Queue
participant Firmware
MeshClient->>Queue: Send heartbeat and config-stage request
Queue->>Firmware: Transmit without routing acknowledgement
Firmware-->>MeshClient: Return config completion nonce
MeshClient->>Queue: Send nodes-stage request
Queue->>Firmware: Transmit without routing acknowledgement
Firmware-->>MeshClient: Return nodes completion nonce
MeshClient->>MeshClient: Dispatch completion and mark configured
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
RCGV1
marked this pull request as ready for review
July 22, 2026 16:50
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@apps/web/src/core/hooks/useWaitForConfig.ts`:
- Around line 24-42: Update the suspended waiter in useWaitForConfig so each
store subscription is disposed when the waiting component unmounts or the
suspended render is abandoned, while still resolving and unsubscribing when
hasRequestedConfig returns true. Ensure repeated pending renders do not leave
accumulated listeners on useDeviceStore.
In `@packages/sdk/src/core/client/MeshClient.progress.test.ts`:
- Around line 113-120: Update the test around client.configure and the
config-complete responses to await completion through handleConfigComplete or
the completed collection instead of using the fixed 20 ms timeout. Ensure both
config-complete packets are processed before asserting sent,
progress.value.phase, and completed, while preserving the expected ignored-nonce
results.
In `@packages/sdk/src/core/client/MeshClient.ts`:
- Around line 158-164: Update the connect() documentation and completion
behavior to reflect that it resolves after configure() writes only the config
request; the nodes-only request is sent later by handleConfigComplete(). Remove
the inaccurate “both requests” contract while preserving the existing
DeviceConnecting status update and configure() flow.
In `@packages/sdk/src/core/queue/Queue.ts`:
- Around line 113-120: Update the wait loop in the queue send flow around
queuedItem and processQueue so it also detects when complete() has cleared the
queued control packet. Reject the pending configure() or heartbeat() operation
when the detached item is removed, instead of continuing to poll queuedItem.sent
forever; preserve the existing success path and cleanup for packets that are
sent normally.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 540b024a-e082-4e7e-8717-aac204e75997
📒 Files selected for processing (12)
apps/web/src/components/PageComponents/Settings/LoRa.tsxapps/web/src/core/hooks/useWaitForConfig.test.tsxapps/web/src/core/hooks/useWaitForConfig.tsapps/web/src/pages/Settings/index.tsxapps/web/src/validation/config/lora.test.tsapps/web/src/validation/config/lora.tspackages/sdk/src/core/client/MeshClient.progress.test.tspackages/sdk/src/core/client/MeshClient.tspackages/sdk/src/core/packet-codec/decodePacket.tspackages/sdk/src/core/queue/Queue.tspackages/sdk/src/features/config/ConfigEditor.test.tspackages/sdk/src/features/config/domain/ConfigEditor.ts
Comment on lines
+24
to
+42
| throw new Promise<void>((resolve) => { | ||
| const hasRequestedConfig = (): boolean => { | ||
| const current = useDeviceStore.getState().getDevice(device.id); | ||
| if (!current) return false; | ||
| return configCase | ||
| ? current.config[configCase] !== undefined | ||
| : current.moduleConfig[moduleConfigCase as ValidModuleConfigType] !== | ||
| undefined; | ||
| }; | ||
|
|
||
| let unsubscribe = (): void => {}; | ||
| const check = (): void => { | ||
| if (hasRequestedConfig()) { | ||
| unsubscribe(); | ||
| resolve(); | ||
| } | ||
| }; | ||
| unsubscribe = useDeviceStore.subscribe(check); | ||
| check(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map and inspect the target file and related store APIs.
printf '\n== useWaitForConfig outline ==\n'
ast-grep outline apps/web/src/core/hooks/useWaitForConfig.ts --view expanded || true
printf '\n== useWaitForConfig full file (numbered) ==\n'
cat -n apps/web/src/core/hooks/useWaitForConfig.ts
printf '\n== Search for subscribe/getState usages around device store ==\n'
rg -n "useDeviceStore\.(subscribe|getState)|create\(.*useDeviceStore|subscribe\(" apps/web/src -g '!**/node_modules/**' || true
printf '\n== Locate device store definition ==\n'
fd -a "useDeviceStore.*" apps/web/src || true
fd -a "*device*store*" apps/web/src || trueRepository: meshtastic/web
Length of output: 9848
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== deviceStore subscribe implementation outline ==\n'
ast-grep outline apps/web/src/core/stores/deviceStore/index.ts --view expanded || true
printf '\n== relevant subscribe/getState sections in deviceStore ==\n'
rg -n "subscribe\\(|getState\\(|create\\(" apps/web/src/core/stores/deviceStore/index.ts || true
printf '\n== deviceStore slices around subscribe implementation ==\n'
# File seems large; inspect only relevant ranges found by rg if possible.
sed -n '760,860p' apps/web/src/core/stores/deviceStore/index.ts
printf '\n== useWaitForConfig test file ==\n'
cat -n apps/web/src/core/hooks/useWaitForConfig.test.tsx
printf '\n== useFeatureFlags hook for comparison ==\n'
cat -n apps/web/src/core/hooks/useFeatureFlags.tsRepository: meshtastic/web
Length of output: 9684
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== useWaitForConfig usages ==\n'
rg -n "useWaitForConfig\\(" apps/web/src -g '!**/node_modules/**' || true
printf '\n== nearby contexts for those usages ==\n'
for f in $(rg -l "useWaitForConfig\\(" apps/web/src -g '!**/node_modules/**' || true); do
printf '\n--- %s ---\n' "$f"
rg -n -A6 -B6 "useWaitForConfig\\(" "$f"
done
printf '\n== search for Suspense wrappers in settings/UI flows ==\n'
rg -n "<Suspense|Suspense\\s*from" apps/web/src/pages apps/web/src/components apps/web/src/core -g '!**/node_modules/**' || trueRepository: meshtastic/web
Length of output: 18581
Dispose suspended waiters. Each suspended render creates a new global store subscription, and there’s no cleanup if the component unmounts before config arrives. Leaving Settings pending—or re-rendering while still waiting—can accumulate listeners on the device store.
🤖 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 `@apps/web/src/core/hooks/useWaitForConfig.ts` around lines 24 - 42, Update the
suspended waiter in useWaitForConfig so each store subscription is disposed when
the waiting component unmounts or the suspended render is abandoned, while still
resolving and unsubscribing when hasRequestedConfig returns true. Ensure
repeated pending renders do not leave accumulated listeners on useDeviceStore.
Comment on lines
+113
to
+120
| await client.configure(); | ||
| respond.withConfigCompleteId(NODES_ONLY_NONCE); | ||
| respond.withConfigCompleteId(12345); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 20)); | ||
| expect(sent).toHaveLength(2); | ||
| expect(client.progress.value.phase).toBe("configuring"); | ||
| expect(completed).toEqual([]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline packages/sdk/src/core/client/MeshClient.progress.test.ts --view expanded || true
printf '\n== Relevant test section ==\n'
sed -n '1,220p' packages/sdk/src/core/client/MeshClient.progress.test.ts
printf '\n== Search for handleConfigComplete and related logic ==\n'
rg -n "handleConfigComplete|configComplete|progress.value.phase|configure\(" packages/sdk/src/core/client -SRepository: meshtastic/web
Length of output: 7973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/sdk/src/core/client/MeshClient.progress.test.ts
printf '\n---\n'
rg -n "handleConfigComplete|configComplete|progress\\.value\\.phase|configure\\(" packages/sdk/src/core/client -SRepository: meshtastic/web
Length of output: 7801
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== MeshClient.ts outline ==\n'
ast-grep outline packages/sdk/src/core/client/MeshClient.ts --view expanded || true
printf '\n== Relevant MeshClient.ts section ==\n'
sed -n '150,260p' packages/sdk/src/core/client/MeshClient.ts
printf '\n== Fake transport ==\n'
sed -n '1,260p' packages/sdk/src/core/testing/createFakeTransport.tsRepository: meshtastic/web
Length of output: 10526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Remaining MeshClient.ts section ==\n'
sed -n '236,340p' packages/sdk/src/core/client/MeshClient.ts
printf '\n== Decode packet path ==\n'
rg -n "function decodePacket|const decodePacket|decodePacket\\(" packages/sdk/src/core/client packages/sdk/src/core -S
printf '\n== fromDeviceStream / toDeviceStream ==\n'
sed -n '1,220p' packages/sdk/src/core/packet-codec/fromDevice.ts
printf '\n---\n'
sed -n '1,220p' packages/sdk/src/core/packet-codec/toDevice.tsRepository: meshtastic/web
Length of output: 6821
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== decodePacket.ts ==\n'
sed -n '1,220p' packages/sdk/src/core/packet-codec/decodePacket.ts
printf '\n== transport types ==\n'
sed -n '1,220p' packages/sdk/src/core/transport/Transport.ts
printf '\n== queue / event bus hooks used by config complete ==\n'
rg -n "onConfigComplete|dispatch\\(|handleConfigComplete\\(" packages/sdk/src/core -SRepository: meshtastic/web
Length of output: 16612
Wait for config-complete delivery before asserting ignored nonces. The 20 ms sleep is brittle here; wait on handleConfigComplete/completed instead so both packets are processed before checking sent and completed.
🤖 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 `@packages/sdk/src/core/client/MeshClient.progress.test.ts` around lines 113 -
120, Update the test around client.configure and the config-complete responses
to await completion through handleConfigComplete or the completed collection
instead of using the fixed 20 ms timeout. Ensure both config-complete packets
are processed before asserting sent, progress.value.phase, and completed, while
preserving the expected ignored-nonce results.
Comment on lines
+158
to
164
| * Begin the two-stage config-only → nodes-only handshake. Resolves once | ||
| * both requests have been written; streamed completion is asynchronous. | ||
| */ | ||
| public async connect(): Promise<void> { | ||
| this.updateDeviceStatus(DeviceStatusEnum.DeviceConnecting); | ||
| await this.configure(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the connect() completion contract.
Line 163 resolves after the config-only request is written. The nodes-only request is not sent until a later handleConfigComplete() call, so “both requests have been written” is inaccurate.
🤖 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 `@packages/sdk/src/core/client/MeshClient.ts` around lines 158 - 164, Update
the connect() documentation and completion behavior to reflect that it resolves
after configure() writes only the config request; the nodes-only request is sent
later by handleConfigComplete(). Remove the inaccurate “both requests” contract
while preserving the existing DeviceConnecting status update and configure()
flow.
Comment on lines
+113
to
+120
| while (!queuedItem.sent) { | ||
| await this.processQueue(outputStream); | ||
| if (!queuedItem.sent) { | ||
| await new Promise((resolve) => setTimeout(resolve, 10)); | ||
| } | ||
| } | ||
| this.remove(item.id); | ||
| return item.id; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject when a queued control packet is cleared.
Line 113 polls queuedItem.sent even if complete() clears the queue during a concurrent send. The detached item can never become sent, so configure() or heartbeat() remains pending forever after disconnect.
Proposed fix
while (!queuedItem.sent) {
await this.processQueue(outputStream);
+ if (!queuedItem.sent && !this.queue.includes(queuedItem)) {
+ throw new Error(`Packet ${item.id} was cancelled before being sent`);
+ }
if (!queuedItem.sent) {
await new Promise((resolve) => setTimeout(resolve, 10));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while (!queuedItem.sent) { | |
| await this.processQueue(outputStream); | |
| if (!queuedItem.sent) { | |
| await new Promise((resolve) => setTimeout(resolve, 10)); | |
| } | |
| } | |
| this.remove(item.id); | |
| return item.id; | |
| while (!queuedItem.sent) { | |
| await this.processQueue(outputStream); | |
| if (!queuedItem.sent && !this.queue.includes(queuedItem)) { | |
| throw new Error(`Packet ${item.id} was cancelled before being sent`); | |
| } | |
| if (!queuedItem.sent) { | |
| await new Promise((resolve) => setTimeout(resolve, 10)); | |
| } | |
| } | |
| this.remove(item.id); | |
| return item.id; |
🤖 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 `@packages/sdk/src/core/queue/Queue.ts` around lines 113 - 120, Update the wait
loop in the queue send flow around queuedItem and processQueue so it also
detects when complete() has cleared the queued control packet. Reject the
pending configure() or heartbeat() operation when the detached item is removed,
instead of continuing to poll queuedItem.sent forever; preserve the existing
success path and cleanup for packets that are sent normally.
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.
Summary
serialHalOnlyform value, correctly reset controlled forms, and ignore protobuf metadata when calculating dirty state.Root cause
The web client treated local configuration requests like acknowledged mesh packets and used one completion path for multiple streaming modes. Settings also suspended on a promise that could never resolve. Separately, form values omitted by older firmware and protobuf metadata caused invalid boolean and false-dirty states.
Impact
HTTP and serial clients can complete configuration streaming consistently, Settings loads after the requested section arrives, and
Serial HAL Onlybehaves as a boolean with a clean edit/reset lifecycle.Validation
NODE_OPTIONS=--no-experimental-webstorage pnpm vitest run: 70 files, 393 tests passed.pnpm --filter meshtastic-web build: passed.pnpm lint: passed with existing warnings.git diff --check: passed.E2E (real device): passed in 2m8s.Serial HAL Onlychangedfalse → true → Reset → false; Save returned disabled, dirty state cleared, and no boolean validation error appeared. No configuration was written to the device.Notes
Summary by CodeRabbit
Bug Fixes
serialHalOnlysetting.Tests