-
Notifications
You must be signed in to change notification settings - Fork 299
Fix device configuration streaming and Settings lifecycle #1325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import { create } from "@bufbuild/protobuf"; | ||
| import { CurrentDeviceContext } from "@core/hooks/useDeviceContext.ts"; | ||
| import { useDeviceStore } from "@core/stores/deviceStore/index.ts"; | ||
| import { Protobuf } from "@meshtastic/sdk"; | ||
| import { act, renderHook } from "@testing-library/react"; | ||
| import type { PropsWithChildren } from "react"; | ||
| import { beforeEach, describe, expect, it } from "vitest"; | ||
| import { useWaitForConfig } from "./useWaitForConfig.ts"; | ||
|
|
||
| const DEVICE_ID = 4242; | ||
|
|
||
| const wrapper = ({ children }: PropsWithChildren) => ( | ||
| <CurrentDeviceContext.Provider value={{ deviceId: DEVICE_ID }}> | ||
| {children} | ||
| </CurrentDeviceContext.Provider> | ||
| ); | ||
|
|
||
| describe("useWaitForConfig", () => { | ||
| beforeEach(() => { | ||
| useDeviceStore.getState().removeDevice(DEVICE_ID); | ||
| }); | ||
|
|
||
| it("resolves its suspense promise when the requested config arrives", async () => { | ||
| const device = useDeviceStore.getState().addDevice(DEVICE_ID); | ||
| const { result } = renderHook( | ||
| () => { | ||
| try { | ||
| useWaitForConfig({ configCase: "lora" }); | ||
| return "ready" as const; | ||
| } catch (pending) { | ||
| return pending as Promise<void>; | ||
| } | ||
| }, | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| expect(result.current).toBeInstanceOf(Promise); | ||
| const pending = result.current as Promise<void>; | ||
|
|
||
| act(() => { | ||
| device.setConfig( | ||
| create(Protobuf.Config.ConfigSchema, { | ||
| payloadVariant: { | ||
| case: "lora", | ||
| value: create(Protobuf.Config.Config_LoRaConfigSchema, {}), | ||
| }, | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| const outcome = await Promise.race([ | ||
| pending.then(() => "resolved"), | ||
| new Promise<string>((resolve) => | ||
| setTimeout(() => resolve("timed-out"), 50), | ||
| ), | ||
| ]); | ||
| expect(outcome).toBe("resolved"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,13 @@ | ||
| import { create } from "@bufbuild/protobuf"; | ||
| import { create, fromBinary } from "@bufbuild/protobuf"; | ||
| import * as Protobuf from "@meshtastic/protobufs"; | ||
| import { describe, expect, it } from "vitest"; | ||
| import { describe, expect, it, vi } from "vitest"; | ||
| import { createFakeTransport } from "../testing/createFakeTransport.ts"; | ||
| import { MeshClient } from "./MeshClient.ts"; | ||
|
|
||
| describe("MeshClient.progress", () => { | ||
| const CONFIG_ONLY_NONCE = 69420; | ||
| const NODES_ONLY_NONCE = 69421; | ||
|
|
||
| it("starts in the idle phase before configure() is called", () => { | ||
| const { transport } = createFakeTransport(); | ||
| const client = new MeshClient({ transport }); | ||
|
|
@@ -60,21 +63,63 @@ describe("MeshClient.progress", () => { | |
| expect(cur.received.modules).toBe(0); | ||
| }); | ||
|
|
||
| it("flips to configured when onConfigComplete fires", () => { | ||
| const { transport } = createFakeTransport(); | ||
| it("requests config and nodes in separate firmware-compatible stages", async () => { | ||
| const { transport, respond, sent } = createFakeTransport(); | ||
| const client = new MeshClient({ transport }); | ||
| void client.configure(); | ||
|
|
||
| await client.configure(); | ||
|
|
||
| const initialPackets = sent.map( | ||
| (packet) => | ||
| fromBinary(Protobuf.Mesh.ToRadioSchema, packet).payloadVariant, | ||
| ); | ||
| expect(initialPackets[0]?.case).toBe("heartbeat"); | ||
| expect(initialPackets[1]).toEqual({ | ||
| case: "wantConfigId", | ||
| value: CONFIG_ONLY_NONCE, | ||
| }); | ||
|
|
||
| client.events.onConfigPacket.dispatch( | ||
| create(Protobuf.Config.ConfigSchema, {}), | ||
| ); | ||
| client.events.onConfigComplete.dispatch(0); | ||
| respond.withConfigCompleteId(CONFIG_ONLY_NONCE); | ||
|
|
||
| await vi.waitFor(() => { | ||
| expect(sent).toHaveLength(3); | ||
| }); | ||
| expect( | ||
| fromBinary(Protobuf.Mesh.ToRadioSchema, sent[2]!).payloadVariant, | ||
| ).toEqual({ case: "wantConfigId", value: NODES_ONLY_NONCE }); | ||
| expect(client.progress.value.phase).toBe("configuring"); | ||
|
|
||
| respond.withConfigCompleteId(NODES_ONLY_NONCE); | ||
|
|
||
| await vi.waitFor(() => { | ||
| expect(client.progress.value.phase).toBe("configured"); | ||
| }); | ||
|
|
||
| const cur = client.progress.value; | ||
| expect(cur.phase).toBe("configured"); | ||
| if (cur.phase !== "configured") throw new Error("unreachable"); | ||
| expect(cur.received.config).toBe(1); | ||
| }); | ||
|
|
||
| it("ignores stale and out-of-order config completion nonces", async () => { | ||
| const { transport, respond, sent } = createFakeTransport(); | ||
| const client = new MeshClient({ transport }); | ||
| const completed: number[] = []; | ||
| client.events.onConfigComplete.subscribe((id) => completed.push(id)); | ||
|
|
||
| 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([]); | ||
|
Comment on lines
+113
to
+120
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 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 🤖 Prompt for AI Agents |
||
| }); | ||
|
|
||
| it("ignores packets that arrive while idle (post-completion)", () => { | ||
| const { transport } = createFakeTransport(); | ||
| const client = new MeshClient({ transport }); | ||
|
|
@@ -85,14 +130,13 @@ describe("MeshClient.progress", () => { | |
| expect(client.progress.value.phase).toBe("idle"); | ||
| }); | ||
|
|
||
| it("resets counters when configure() runs again", () => { | ||
| it("resets counters when configure() runs again", async () => { | ||
| const { transport } = createFakeTransport(); | ||
| const client = new MeshClient({ transport }); | ||
| void client.configure(); | ||
| await client.configure(); | ||
| client.events.onConfigPacket.dispatch( | ||
| create(Protobuf.Config.ConfigSchema, {}), | ||
| ); | ||
| client.events.onConfigComplete.dispatch(0); | ||
| void client.configure(); | ||
| expect(client.progress.value).toEqual({ | ||
| phase: "configuring", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: meshtastic/web
Length of output: 9848
🏁 Script executed:
Repository: meshtastic/web
Length of output: 9684
🏁 Script executed:
Repository: 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