From 94fa7b1d282f4cff07d623dc55d7565733e2288a Mon Sep 17 00:00:00 2001 From: Dan Ditomaso Date: Sun, 18 Oct 2026 15:58:15 -0400 Subject: [PATCH 1/6] fix(sdk): keep edits made during a commit pending ConfigEditor.commit() read the working copy again after its async sends and then cleared every dirty flag, so a section edited while a save was in flight was marked saved without ever being sent. The edit was dropped silently: the form showed it, the device never got it. Snapshot the working copy, dirty sections and admin messages when the commit starts, send from that snapshot, make it the new baseline, and recompute dirtiness. Anything changed during the commit stays dirty for the next one. The regression was found by the save-flow tests proposed in #1422. Co-authored-by: Matt <57228426+xtantaudio@users.noreply.github.com> --- .../src/features/config/ConfigEditor.test.ts | 56 +++++++++++++++- .../features/config/domain/ConfigEditor.ts | 65 +++++++++++-------- 2 files changed, 94 insertions(+), 27 deletions(-) diff --git a/packages/sdk/src/features/config/ConfigEditor.test.ts b/packages/sdk/src/features/config/ConfigEditor.test.ts index 09d5a4901..a21a6b763 100644 --- a/packages/sdk/src/features/config/ConfigEditor.test.ts +++ b/packages/sdk/src/features/config/ConfigEditor.test.ts @@ -1,4 +1,4 @@ -import { create } from "@bufbuild/protobuf"; +import { create, fromBinary } from "@bufbuild/protobuf"; import * as Protobuf from "@meshtastic/protobufs"; import { describe, expect, it } from "vitest"; import { MeshClient } from "../../core/client/MeshClient.ts"; @@ -215,4 +215,58 @@ describe("ConfigEditor", () => { expect(editor.radio.value).toEqual({}); expect(editor.isDirty.value).toBe(false); }); + + it("keeps an edit made while a commit is in flight dirty", async () => { + const { transport } = createFakeTransport(); + const client = new MeshClient({ transport }); + const editor = client.config.editor; + + client.events.onConfigPacket.dispatch(loraPacket(1)); + client.events.onModuleConfigPacket.dispatch(mqttPacket(false)); + editor.setRadioSection( + "lora", + create(Protobuf.Config.Config_LoRaConfigSchema, { region: 4 }), + ); + + // Hold the first commitEditSettings open so the commit is in flight. + const sent: Protobuf.Admin.AdminMessage[] = []; + let release: (() => void) | undefined; + let held = false; + client.sendPacket = (async (payload: Uint8Array) => { + const admin = fromBinary(Protobuf.Admin.AdminMessageSchema, payload); + sent.push(admin); + if (!held && admin.payloadVariant.case === "commitEditSettings") { + held = true; + await new Promise((resolve) => { + release = resolve; + }); + } + return 1; + }) as never; + + const pending = editor.commit(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + editor.setModuleSection( + "mqtt", + create(Protobuf.ModuleConfig.ModuleConfig_MQTTConfigSchema, { + enabled: true, + }), + ); + release?.(); + expect((await pending).status).toBe("ok"); + + // The lora edit was sent and is clean; the mqtt edit was not and is not. + expect(editor.dirtyRadioSections.value).toEqual([]); + expect(editor.dirtyModuleSections.value).toEqual(["mqtt"]); + expect(editor.isDirty.value).toBe(true); + expect(sent.map((a) => a.payloadVariant.case)).not.toContain( + "setModuleConfig", + ); + + // The next commit carries it. + expect((await editor.commit()).status).toBe("ok"); + expect(sent.map((a) => a.payloadVariant.case)).toContain("setModuleConfig"); + expect(editor.isDirty.value).toBe(false); + }); }); diff --git a/packages/sdk/src/features/config/domain/ConfigEditor.ts b/packages/sdk/src/features/config/domain/ConfigEditor.ts index 38a0872b5..a675778a0 100644 --- a/packages/sdk/src/features/config/domain/ConfigEditor.ts +++ b/packages/sdk/src/features/config/domain/ConfigEditor.ts @@ -297,17 +297,31 @@ export class ConfigEditor { /** * Send all dirty sections to the device wrapped in a beginEdit/commitEdit - * pair. On success the baseline is replaced with the working copy + * pair. On success the baseline is replaced with what was sent * (optimistic update); inbound config packets after commit will reconcile. + * + * The user can keep editing while a commit is in flight. Those edits replace + * the working signals (they are never mutated), so they are not part of the + * snapshot taken here and stay dirty for the next commit. */ public async commit(): Promise> { if (!this._isDirty.peek()) return Result.ok(undefined); + const radio = this.workingRadio.peek(); + const dirtyRadio = this._dirtyRadioSections.peek(); + const modules = this.workingModules.peek(); + const dirtyModules = this._dirtyModuleSections.peek(); + const channels = this.workingChannels.peek(); + const dirtyChannels = this._dirtyChannels.peek(); + const owner = this.workingOwner.peek(); + const ownerDirty = this._isOwnerDirty.peek(); + const queued = this.queuedAdminMessages.peek(); + const stagedByKey = this._stagedAdminMessages.peek(); + const beginResult = await beginEditSettings(this.client); if (Result.isError(beginResult)) return Result.err(beginResult.error); - const radio = this.workingRadio.peek(); - for (const variant of this._dirtyRadioSections.peek()) { + for (const variant of dirtyRadio) { const value = radio[variant]; if (value === undefined) continue; const config = buildRadioConfig(variant, value); @@ -315,8 +329,7 @@ export class ConfigEditor { if (Result.isError(result)) return Result.err(result.error); } - const modules = this.workingModules.peek(); - for (const variant of this._dirtyModuleSections.peek()) { + for (const variant of dirtyModules) { const value = modules[variant]; if (value === undefined) continue; const moduleConfig = buildModuleConfig(variant, value); @@ -324,23 +337,19 @@ export class ConfigEditor { if (Result.isError(result)) return Result.err(result.error); } - const channels = this.workingChannels.peek(); - for (const index of this._dirtyChannels.peek()) { + for (const index of dirtyChannels) { const channel = channels.get(index); if (!channel) continue; const result = await setChannel(this.client, channel); if (Result.isError(result)) return Result.err(result.error); } - if (this._isOwnerDirty.peek()) { - const owner = this.workingOwner.peek(); - if (owner) { - const result = await setOwner(this.client, owner); - if (Result.isError(result)) return Result.err(result.error); - } + if (ownerDirty && owner) { + const result = await setOwner(this.client, owner); + if (Result.isError(result)) return Result.err(result.error); } - for (const message of this.queuedAdminMessages.peek()) { + for (const message of queued) { const variant = message.payloadVariant; if (!variant.case) continue; try { @@ -350,7 +359,7 @@ export class ConfigEditor { } } - const staged = [...this._stagedAdminMessages.peek().values()]; + const staged = [...stagedByKey.values()]; for (const message of staged) { const variant = message.payloadVariant; if (!variant.case) continue; @@ -364,17 +373,21 @@ export class ConfigEditor { const commitResult = await commitEditSettings(this.client); if (Result.isError(commitResult)) return Result.err(commitResult.error); - this.baselineRadio.value = this.workingRadio.peek(); - this.baselineModules.value = this.workingModules.peek(); - this.baselineChannels.value = new Map(this.workingChannels.peek()); - this.baselineOwner.value = this.workingOwner.peek(); - this.queuedAdminMessages.value = []; - this._stagedAdminMessages.value = new Map(); - this._dirtyRadioSections.value = []; - this._dirtyModuleSections.value = []; - this._dirtyChannels.value = []; - this._isOwnerDirty.value = false; - this._isDirty.value = false; + this.baselineRadio.value = radio; + this.baselineModules.value = modules; + this.baselineChannels.value = new Map(channels); + this.baselineOwner.value = owner; + // Drop only the admin messages that were sent; one queued or staged while + // the commit was in flight has not been sent yet. + this.queuedAdminMessages.value = this.queuedAdminMessages + .peek() + .filter((message) => !queued.includes(message)); + const unsent = new Map(this._stagedAdminMessages.peek()); + for (const [key, message] of stagedByKey) { + if (unsent.get(key) === message) unsent.delete(key); + } + this._stagedAdminMessages.value = unsent; + this.recomputeDirty(); for (const message of staged) { this.onStagedAdminMessageCommitted?.(message); From da49c8ffa3c8c9024a688a4be42fa0ead748d513 Mon Sep 17 00:00:00 2001 From: Dan Ditomaso Date: Sun, 18 Oct 2026 15:58:19 -0400 Subject: [PATCH 2/6] test(web): cover the MQTT and device config save flows Adds end-to-end tests that drive the real forms through ConfigEditor and decode what goes out on the wire: - MQTT `enabled` and a device-config flag reach the radio as `true` - a toggle flipped during an unrelated in-flight save stays pending - Bluetooth saves on a radio with no fixed PIN, but still requires six digits for FIXED_PIN pairing Adapted from #1422 to the Settings layout introduced in #1463 (labels "MQTT Enabled" and "Double Tap as Button"). Co-authored-by: Matt <57228426+xtantaudio@users.noreply.github.com> --- .../PageComponents/ModuleConfig/MQTT.test.tsx | 168 ++++++++++++ .../Settings/DeviceConfig.test.tsx | 253 ++++++++++++++++++ 2 files changed, 421 insertions(+) create mode 100644 apps/web/src/components/PageComponents/ModuleConfig/MQTT.test.tsx create mode 100644 apps/web/src/components/PageComponents/Settings/DeviceConfig.test.tsx diff --git a/apps/web/src/components/PageComponents/ModuleConfig/MQTT.test.tsx b/apps/web/src/components/PageComponents/ModuleConfig/MQTT.test.tsx new file mode 100644 index 000000000..b6986375c --- /dev/null +++ b/apps/web/src/components/PageComponents/ModuleConfig/MQTT.test.tsx @@ -0,0 +1,168 @@ +import { create, fromBinary } from "@bufbuild/protobuf"; +import { CurrentDeviceContext, useDeviceStore } from "@core/stores"; +import { MeshClient, MeshRegistry, Protobuf } from "@meshtastic/sdk"; +import { createFakeTransport } from "@meshtastic/sdk/testing"; +import { MeshRegistryProvider } from "@meshtastic/sdk-react"; +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { Suspense } from "react"; +import { describe, expect, it } from "vitest"; +import { MQTT } from "./MQTT.tsx"; + +/** + * End-to-end cover for the MQTT module-config save path: the real settings + * form -> `ConfigEditor` staging -> the AdminMessage bytes that go to the + * radio. + * + * The assertions decode the outgoing packet from its wire encoding, because + * protobuf omits default booleans: if `enabled` is not genuinely `true` when + * the message is serialised, field 1 is simply absent from the bytes and the + * device silently keeps MQTT switched off — which is exactly the bug this + * covers. + */ + +let deviceIdSeq = 100; + +function setup(mqtt: Protobuf.ModuleConfig.ModuleConfig_MQTTConfig) { + const { transport } = createFakeTransport(); + const registry = new MeshRegistry(); + const client = new MeshClient({ transport }); + const connectionId = deviceIdSeq++; + registry.register(connectionId, client); + registry.setActive(connectionId); + + const sent: Protobuf.Admin.AdminMessage[] = []; + let release: (() => void) | undefined; + let gate: Protobuf.Admin.AdminMessage["payloadVariant"]["case"] | undefined; + + client.sendPacket = (async (payload: Uint8Array) => { + const admin = fromBinary(Protobuf.Admin.AdminMessageSchema, payload); + sent.push(admin); + if (gate && admin.payloadVariant.case === gate) { + gate = undefined; + await new Promise((resolve) => { + release = resolve; + }); + } + return 1; + }) as never; + + const packet = create(Protobuf.ModuleConfig.ModuleConfigSchema, { + payloadVariant: { case: "mqtt", value: mqtt }, + }); + const device = useDeviceStore.getState().addDevice(connectionId); + device.setModuleConfig(packet); + client.events.onModuleConfigPacket.dispatch(packet); + + render( + + + loading}> + {}} /> + + + , + ); + + return { + client, + editor: client.config.editor, + sent, + gateOn: ( + value: Protobuf.Admin.AdminMessage["payloadVariant"]["case"], + ): void => { + gate = value; + }, + release: () => release?.(), + }; +} + +function mqttFromWire( + sent: Protobuf.Admin.AdminMessage[], +): Protobuf.ModuleConfig.ModuleConfig_MQTTConfig | undefined { + for (const admin of sent) { + const variant = admin.payloadVariant; + if (variant.case !== "setModuleConfig") continue; + const module = variant.value.payloadVariant; + if (module.case === "mqtt") return module.value; + } + return undefined; +} + +const deviceMqtt = () => + create(Protobuf.ModuleConfig.ModuleConfig_MQTTConfigSchema, { + enabled: false, + address: "mqtt.example.org", + username: "meshdev", + password: "large4cats", + root: "msh", + }); + +describe("MQTT module config", () => { + it("puts `enabled: true` on the wire when the toggle is switched on", async () => { + const { editor, sent } = setup(deviceMqtt()); + + await userEvent.click(await screen.findByLabelText("MQTT Enabled")); + + await waitFor(() => + expect(editor.dirtyModuleSections.value).toContain("mqtt"), + ); + expect((await editor.commit()).status).toBe("ok"); + + const wire = mqttFromWire(sent); + expect(wire).toBeDefined(); + expect(wire?.enabled).toBe(true); + // The text fields the user did not touch must still round-trip. + expect(wire?.address).toBe("mqtt.example.org"); + expect(wire?.username).toBe("meshdev"); + expect(wire?.password).toBe("large4cats"); + expect(wire?.root).toBe("msh"); + }); + + it("does not report the toggle as saved when it was not part of the commit", async () => { + const { client, editor, sent, gateOn, release } = setup(deviceMqtt()); + + client.events.onConfigPacket.dispatch( + create(Protobuf.Config.ConfigSchema, { + payloadVariant: { + case: "lora", + value: create(Protobuf.Config.Config_LoRaConfigSchema, { region: 1 }), + }, + }), + ); + editor.setRadioSection( + "lora", + create(Protobuf.Config.Config_LoRaConfigSchema, { region: 4 }), + ); + + // Start an unrelated save and hold it open on the closing commitEditSettings. + gateOn("commitEditSettings"); + const pending = editor.commit(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + // The user flips "MQTT enabled" while that save is still in flight. + await userEvent.click(await screen.findByLabelText("MQTT Enabled")); + await waitFor(() => + expect(editor.dirtyModuleSections.value).toContain("mqtt"), + ); + + release(); + expect((await pending).status).toBe("ok"); + + // The transaction really did open and commit on the device, but it never + // carried the MQTT payload — so the edit has to stay pending. + expect(sent.map((a) => a.payloadVariant.case)).toEqual([ + "beginEditSettings", + "setConfig", + "commitEditSettings", + ]); + expect(mqttFromWire(sent)).toBeUndefined(); + expect(editor.dirtyModuleSections.value).toContain("mqtt"); + expect(editor.isDirty.value).toBe(true); + + // The next save carries it, with `enabled` intact. + expect((await editor.commit()).status).toBe("ok"); + expect(mqttFromWire(sent)?.enabled).toBe(true); + expect(editor.isDirty.value).toBe(false); + }); +}); diff --git a/apps/web/src/components/PageComponents/Settings/DeviceConfig.test.tsx b/apps/web/src/components/PageComponents/Settings/DeviceConfig.test.tsx new file mode 100644 index 000000000..e0851950d --- /dev/null +++ b/apps/web/src/components/PageComponents/Settings/DeviceConfig.test.tsx @@ -0,0 +1,253 @@ +import { create, fromBinary } from "@bufbuild/protobuf"; +import { BluetoothValidationSchema } from "@app/validation/config/bluetooth.ts"; +import { Bluetooth } from "@components/PageComponents/Settings/Bluetooth.tsx"; +import { Device } from "@components/PageComponents/Settings/Device/index.tsx"; +import { CurrentDeviceContext, useDeviceStore } from "@core/stores"; +import { MeshClient, MeshRegistry, Protobuf } from "@meshtastic/sdk"; +import { createFakeTransport } from "@meshtastic/sdk/testing"; +import { MeshRegistryProvider } from "@meshtastic/sdk-react"; +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { type ComponentType, Suspense } from "react"; +import { describe, expect, it } from "vitest"; + +/** + * End-to-end cover for the Device settings page. Its tabs (Device, Bluetooth, + * Display, Power, Network, Position) all stage through the *same* radio-config + * mechanism as LoRa — `ConfigEditor.setRadioSection` and `_dirtyRadioSections` + * — so they share both the commit bookkeeping and the "the form silently + * refuses to save" failure mode. + * + * Assertions decode the outgoing AdminMessage from its wire bytes: protobuf + * omits fields at their default, so a boolean that was never really set is + * simply missing on the device. + */ + +let deviceIdSeq = 800; + +function setup( + Component: ComponentType<{ onFormInit: () => void }>, + config: Protobuf.Config.Config, +) { + const { transport } = createFakeTransport(); + const registry = new MeshRegistry(); + const client = new MeshClient({ transport }); + const connectionId = deviceIdSeq++; + registry.register(connectionId, client); + registry.setActive(connectionId); + + const sent: Protobuf.Admin.AdminMessage[] = []; + let release: (() => void) | undefined; + let gate: Protobuf.Admin.AdminMessage["payloadVariant"]["case"] | undefined; + + client.sendPacket = (async (payload: Uint8Array) => { + const admin = fromBinary(Protobuf.Admin.AdminMessageSchema, payload); + sent.push(admin); + if (gate && admin.payloadVariant.case === gate) { + gate = undefined; + await new Promise((resolve) => { + release = resolve; + }); + } + return 1; + }) as never; + + const device = useDeviceStore.getState().addDevice(connectionId); + device.setConfig(config); + client.events.onConfigPacket.dispatch(config); + + render( + + + loading}> + {}} /> + + + , + ); + + return { + client, + editor: client.config.editor, + sent, + gateOn: ( + value: Protobuf.Admin.AdminMessage["payloadVariant"]["case"], + ): void => { + gate = value; + }, + release: () => release?.(), + }; +} + +/** + * Pull a config section back out of the transmitted AdminMessages. The value + * is cast to the caller's section type: the wire union cannot be narrowed + * generically, and the `case` check above already guarantees it. + */ +function fromWire( + sent: Protobuf.Admin.AdminMessage[], + section: Protobuf.Config.Config["payloadVariant"]["case"], +): T | undefined { + for (const admin of sent) { + if (admin.payloadVariant.case !== "setConfig") continue; + const variant = admin.payloadVariant.value.payloadVariant; + if (variant.case === section) { + return variant.value as T; + } + } + return undefined; +} + +const deviceConfig = () => + create(Protobuf.Config.ConfigSchema, { + payloadVariant: { + case: "device", + value: create(Protobuf.Config.Config_DeviceConfigSchema, { + role: Protobuf.Config.Config_DeviceConfig_Role.CLIENT, + buttonGpio: 4, + buzzerGpio: 2, + nodeInfoBroadcastSecs: 10800, + doubleTapAsButtonPress: false, + }), + }, + }); + +const bluetoothConfig = (fixedPin: number) => + create(Protobuf.Config.ConfigSchema, { + payloadVariant: { + case: "bluetooth", + value: create(Protobuf.Config.Config_BluetoothConfigSchema, { + enabled: false, + mode: Protobuf.Config.Config_BluetoothConfig_PairingMode.RANDOM_PIN, + fixedPin, + }), + }, + }); + +describe("Device settings", () => { + it("puts a toggled device-config flag on the wire", async () => { + const { editor, sent } = setup(Device as never, deviceConfig()); + + await userEvent.click(await screen.findByLabelText("Double Tap as Button")); + + await waitFor(() => + expect(editor.dirtyRadioSections.value).toContain("device"), + ); + expect((await editor.commit()).status).toBe("ok"); + + const wire = fromWire(sent, "device"); + expect(wire?.doubleTapAsButtonPress).toBe(true); + // Untouched fields must survive. + expect(wire?.buttonGpio).toBe(4); + expect(wire?.buzzerGpio).toBe(2); + expect(wire?.nodeInfoBroadcastSecs).toBe(10800); + expect(wire?.role).toBe(Protobuf.Config.Config_DeviceConfig_Role.CLIENT); + }); + + it("does not report the toggle as saved when it was not part of the commit", async () => { + const { client, editor, sent, gateOn, release } = setup( + Device as never, + deviceConfig(), + ); + + client.events.onChannelPacket.dispatch( + create(Protobuf.Channel.ChannelSchema, { + index: 0, + role: Protobuf.Channel.Channel_Role.PRIMARY, + settings: create(Protobuf.Channel.ChannelSettingsSchema, { + psk: new Uint8Array([1]), + }), + }), + ); + editor.setChannel( + create(Protobuf.Channel.ChannelSchema, { + index: 0, + role: Protobuf.Channel.Channel_Role.PRIMARY, + settings: create(Protobuf.Channel.ChannelSettingsSchema, { + psk: new Uint8Array([1]), + uplinkEnabled: true, + }), + }), + ); + + // An unrelated channel save is in flight, held open on its closing commit. + gateOn("commitEditSettings"); + const pending = editor.commit(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + await userEvent.click(await screen.findByLabelText("Double Tap as Button")); + await waitFor(() => + expect(editor.dirtyRadioSections.value).toContain("device"), + ); + + release(); + expect((await pending).status).toBe("ok"); + + // A genuine begin/commit pair happened on the device, but it never carried + // the device config — so that edit has to stay pending. + expect(sent.map((a) => a.payloadVariant.case)).toEqual([ + "beginEditSettings", + "setChannel", + "commitEditSettings", + ]); + expect( + fromWire(sent, "device"), + ).toBeUndefined(); + expect(editor.dirtyRadioSections.value).toContain("device"); + expect(editor.isDirty.value).toBe(true); + + expect((await editor.commit()).status).toBe("ok"); + expect( + fromWire(sent, "device") + ?.doubleTapAsButtonPress, + ).toBe(true); + expect(editor.isDirty.value).toBe(false); + }); + + it("saves Bluetooth on a device that has no fixed PIN", async () => { + // `fixed_pin` is 0 until FIXED_PIN pairing is used. The schema used to + // demand six digits unconditionally, which made this device's Bluetooth + // form permanently invalid and silently unsavable. + const { editor, sent } = setup(Bluetooth as never, bluetoothConfig(0)); + + await userEvent.click(await screen.findByLabelText("Bluetooth enabled")); + + await waitFor(() => + expect(editor.dirtyRadioSections.value).toContain("bluetooth"), + ); + expect((await editor.commit()).status).toBe("ok"); + + expect( + fromWire(sent, "bluetooth") + ?.enabled, + ).toBe(true); + }); + + it("still requires a six-digit PIN for FIXED_PIN pairing", () => { + // The relaxation is conditional, not a removal: PIN validation still + // applies where the PIN is actually used. + const base = { + enabled: true, + fixedPin: 0, + }; + expect( + BluetoothValidationSchema.safeParse({ + ...base, + mode: Protobuf.Config.Config_BluetoothConfig_PairingMode.FIXED_PIN, + }).success, + ).toBe(false); + expect( + BluetoothValidationSchema.safeParse({ + ...base, + fixedPin: 123456, + mode: Protobuf.Config.Config_BluetoothConfig_PairingMode.FIXED_PIN, + }).success, + ).toBe(true); + expect( + BluetoothValidationSchema.safeParse({ + ...base, + mode: Protobuf.Config.Config_BluetoothConfig_PairingMode.RANDOM_PIN, + }).success, + ).toBe(true); + }); +}); From 5f256857c0e39714f30fa5827ff700526217dc90 Mon Sep 17 00:00:00 2001 From: Dan Ditomaso Date: Sun, 18 Oct 2026 16:12:44 -0400 Subject: [PATCH 3/6] fix(sdk): only promote what a commit sent into the baseline Review follow-ups on the commit snapshot: - Update the baseline per section that was sent, so a device packet for an unsent section that arrives mid-commit isn't rolled back and shown as dirty. The owner baseline only changes when the owner was sent. - Track a connection generation. A commit that finishes after a disconnect no longer writes its payload back into the cleared editor. - Remove one queued admin message per message sent, so the same object queued again mid-commit is still sent. - Run commits one at a time. A commit requested during another waits for it, then sends what is still dirty. - When a send fails after beginEditSettings, close the transaction (there is no abort message) and leave everything dirty for a retry. - Skip empty transactions. The per-section reconcile and empty-transaction check follow #1421. Adds interceptAdminMessages to @meshtastic/sdk/testing for holding and failing individual sends, with a test for each case above. Co-authored-by: Matt <57228426+xtantaudio@users.noreply.github.com> --- packages/sdk/src/core/testing/index.ts | 5 + .../core/testing/interceptAdminMessages.ts | 115 +++++++++ .../src/features/config/ConfigEditor.test.ts | 187 +++++++++++---- .../features/config/domain/ConfigEditor.ts | 226 +++++++++++++----- 4 files changed, 433 insertions(+), 100 deletions(-) create mode 100644 packages/sdk/src/core/testing/interceptAdminMessages.ts diff --git a/packages/sdk/src/core/testing/index.ts b/packages/sdk/src/core/testing/index.ts index 67af02a73..e6c482a73 100644 --- a/packages/sdk/src/core/testing/index.ts +++ b/packages/sdk/src/core/testing/index.ts @@ -1,4 +1,9 @@ export { createFakeTransport } from "./createFakeTransport.ts"; +export { + type AdminMessageInterceptor, + type HeldSend, + interceptAdminMessages, +} from "./interceptAdminMessages.ts"; export type { FakeResponder, FakeTransportHandle, diff --git a/packages/sdk/src/core/testing/interceptAdminMessages.ts b/packages/sdk/src/core/testing/interceptAdminMessages.ts new file mode 100644 index 000000000..81cea27f9 --- /dev/null +++ b/packages/sdk/src/core/testing/interceptAdminMessages.ts @@ -0,0 +1,115 @@ +import { fromBinary } from "@bufbuild/protobuf"; +import * as Protobuf from "@meshtastic/protobufs"; +import type { MeshClient } from "../client/MeshClient.ts"; + +type AdminCase = NonNullable< + Protobuf.Admin.AdminMessage["payloadVariant"]["case"] +>; +type ConfigVariant = Protobuf.Config.Config["payloadVariant"]; +type ModuleConfigVariant = Protobuf.ModuleConfig.ModuleConfig["payloadVariant"]; +type ConfigCase = NonNullable; +type ModuleConfigCase = NonNullable; +type ConfigValue = Extract< + ConfigVariant, + { case: K } +>["value"]; +type ModuleConfigValue = Extract< + ModuleConfigVariant, + { case: K } +>["value"]; + +export interface HeldSend { + /** Resolves once the send is being held. */ + reached: Promise; + release(): void; +} + +export interface AdminMessageInterceptor { + /** Every admin message sent, decoded, in order. */ + sent: Protobuf.Admin.AdminMessage[]; + cases(): (AdminCase | undefined)[]; + /** Hold the next send of `adminCase` open until released. */ + hold(adminCase: AdminCase): HeldSend; + /** Make the next send of `adminCase` fail. */ + failNext(adminCase: AdminCase): void; + sentConfig(section: K): ConfigValue | undefined; + sentModuleConfig( + section: K, + ): ModuleConfigValue | undefined; +} + +/** + * Replace `client.sendPacket` so a test can see the admin messages a commit + * sends, and pause or fail individual sends. + */ +export function interceptAdminMessages( + client: MeshClient, +): AdminMessageInterceptor { + const sent: Protobuf.Admin.AdminMessage[] = []; + const holds: { + adminCase: AdminCase; + onReached: () => void; + released: Promise; + }[] = []; + const failures: AdminCase[] = []; + + client.sendPacket = (async (payload: Uint8Array) => { + const admin = fromBinary(Protobuf.Admin.AdminMessageSchema, payload); + sent.push(admin); + const adminCase = admin.payloadVariant.case; + + const failure = failures.indexOf(adminCase as AdminCase); + if (failure !== -1) { + failures.splice(failure, 1); + throw new Error(`send failed: ${adminCase}`); + } + + const index = holds.findIndex((h) => h.adminCase === adminCase); + const hold = index === -1 ? undefined : holds.splice(index, 1)[0]; + if (hold) { + hold.onReached(); + await hold.released; + } + return 1; + }) as MeshClient["sendPacket"]; + + return { + sent, + cases: () => sent.map((admin) => admin.payloadVariant.case), + hold(adminCase) { + let onReached!: () => void; + let release!: () => void; + const reached = new Promise((resolve) => { + onReached = resolve; + }); + const released = new Promise((resolve) => { + release = resolve; + }); + holds.push({ adminCase, onReached, released }); + return { reached, release }; + }, + failNext(adminCase) { + failures.push(adminCase); + }, + sentConfig(section: K) { + for (const admin of sent) { + if (admin.payloadVariant.case !== "setConfig") continue; + const variant = admin.payloadVariant.value.payloadVariant; + if (variant.case === section) { + return variant.value as ConfigValue; + } + } + return undefined; + }, + sentModuleConfig(section: K) { + for (const admin of sent) { + if (admin.payloadVariant.case !== "setModuleConfig") continue; + const variant = admin.payloadVariant.value.payloadVariant; + if (variant.case === section) { + return variant.value as ModuleConfigValue; + } + } + return undefined; + }, + }; +} diff --git a/packages/sdk/src/features/config/ConfigEditor.test.ts b/packages/sdk/src/features/config/ConfigEditor.test.ts index a21a6b763..6649693f4 100644 --- a/packages/sdk/src/features/config/ConfigEditor.test.ts +++ b/packages/sdk/src/features/config/ConfigEditor.test.ts @@ -1,8 +1,9 @@ -import { create, fromBinary } from "@bufbuild/protobuf"; +import { create } from "@bufbuild/protobuf"; import * as Protobuf from "@meshtastic/protobufs"; import { describe, expect, it } from "vitest"; import { MeshClient } from "../../core/client/MeshClient.ts"; import { createFakeTransport } from "../../core/testing/createFakeTransport.ts"; +import { interceptAdminMessages } from "../../core/testing/interceptAdminMessages.ts"; import { DeviceStatusEnum } from "../../core/transport/Transport.ts"; function loraPacket(region: number): Protobuf.Config.Config { @@ -215,58 +216,162 @@ describe("ConfigEditor", () => { expect(editor.radio.value).toEqual({}); expect(editor.isDirty.value).toBe(false); }); +}); - it("keeps an edit made while a commit is in flight dirty", async () => { - const { transport } = createFakeTransport(); - const client = new MeshClient({ transport }); - const editor = client.config.editor; +const LORA = "lora" as const; + +/** A connected editor with lora edited from region 1 to 4. */ +function setupCommit() { + const { transport } = createFakeTransport(); + const client = new MeshClient({ transport }); + const editor = client.config.editor; + const radio = interceptAdminMessages(client); + + client.events.onConfigPacket.dispatch(loraPacket(1)); + client.events.onModuleConfigPacket.dispatch(mqttPacket(false)); + client.events.onChannelPacket.dispatch(channelPacket(1, "primary")); + editor.setRadioSection( + LORA, + create(Protobuf.Config.Config_LoRaConfigSchema, { region: 4 }), + ); + return { client, editor, radio }; +} - client.events.onConfigPacket.dispatch(loraPacket(1)); - client.events.onModuleConfigPacket.dispatch(mqttPacket(false)); - editor.setRadioSection( - "lora", - create(Protobuf.Config.Config_LoRaConfigSchema, { region: 4 }), - ); +const enableMqtt = () => + create(Protobuf.ModuleConfig.ModuleConfig_MQTTConfigSchema, { + enabled: true, + }); - // Hold the first commitEditSettings open so the commit is in flight. - const sent: Protobuf.Admin.AdminMessage[] = []; - let release: (() => void) | undefined; - let held = false; - client.sendPacket = (async (payload: Uint8Array) => { - const admin = fromBinary(Protobuf.Admin.AdminMessageSchema, payload); - sent.push(admin); - if (!held && admin.payloadVariant.case === "commitEditSettings") { - held = true; - await new Promise((resolve) => { - release = resolve; - }); - } - return 1; - }) as never; +const user = (longName: string) => + create(Protobuf.Mesh.UserSchema, { id: "!00000001", longName }); - const pending = editor.commit(); - await new Promise((resolve) => setTimeout(resolve, 0)); +describe("ConfigEditor.commit", () => { + it("keeps an edit made while a commit is in flight dirty", async () => { + const { editor, radio } = setupCommit(); + const held = radio.hold("commitEditSettings"); - editor.setModuleSection( - "mqtt", - create(Protobuf.ModuleConfig.ModuleConfig_MQTTConfigSchema, { - enabled: true, - }), - ); - release?.(); + const pending = editor.commit(); + await held.reached; + editor.setModuleSection("mqtt", enableMqtt()); + held.release(); expect((await pending).status).toBe("ok"); - // The lora edit was sent and is clean; the mqtt edit was not and is not. expect(editor.dirtyRadioSections.value).toEqual([]); expect(editor.dirtyModuleSections.value).toEqual(["mqtt"]); + expect(radio.sentModuleConfig("mqtt")).toBeUndefined(); + + expect((await editor.commit()).status).toBe("ok"); + expect(radio.sentModuleConfig("mqtt")?.enabled).toBe(true); + expect(editor.isDirty.value).toBe(false); + }); + + it("keeps device packets for unsent sections that arrive during a commit", async () => { + const { client, editor, radio } = setupCommit(); + const held = radio.hold("commitEditSettings"); + + const pending = editor.commit(); + await held.reached; + client.events.onModuleConfigPacket.dispatch(mqttPacket(true)); + client.events.onChannelPacket.dispatch(channelPacket(1, "renamed")); + held.release(); + expect((await pending).status).toBe("ok"); + + expect(editor.isDirty.value).toBe(false); + expect(editor.modules.value.mqtt?.enabled).toBe(true); + expect(editor.channels.value.get(1)?.settings?.name).toBe("renamed"); + }); + + it("keeps an owner update that arrives during a commit", async () => { + const { editor, radio } = setupCommit(); + editor.setBaselineOwner(user("Before")); + const held = radio.hold("commitEditSettings"); + + const pending = editor.commit(); + await held.reached; + editor.setBaselineOwner(user("After")); + held.release(); + expect((await pending).status).toBe("ok"); + + expect(editor.isOwnerDirty.value).toBe(false); + expect(editor.owner.value?.longName).toBe("After"); + expect(radio.cases()).not.toContain("setOwner"); + }); + + it("leaves the editor empty when the device disconnects during a commit", async () => { + const { client, editor, radio } = setupCommit(); + const held = radio.hold("commitEditSettings"); + + const pending = editor.commit(); + await held.reached; + client.events.onDeviceStatus.dispatch(DeviceStatusEnum.DeviceDisconnected); + held.release(); + expect((await pending).status).toBe("error"); + + expect(editor.radio.value).toEqual({}); + expect(editor.isDirty.value).toBe(false); + client.events.onConfigPacket.dispatch(loraPacket(1)); + expect(editor.isDirty.value).toBe(false); + }); + + it("sends a message queued again during a commit", async () => { + const { editor, radio } = setupCommit(); + const message = create(Protobuf.Admin.AdminMessageSchema, { + payloadVariant: { case: "removeFixedPosition", value: true }, + }); + editor.queueAdminMessage(message); + const held = radio.hold("commitEditSettings"); + + const pending = editor.commit(); + await held.reached; + editor.queueAdminMessage(message); + held.release(); + expect((await pending).status).toBe("ok"); expect(editor.isDirty.value).toBe(true); - expect(sent.map((a) => a.payloadVariant.case)).not.toContain( - "setModuleConfig", - ); - // The next commit carries it. expect((await editor.commit()).status).toBe("ok"); - expect(sent.map((a) => a.payloadVariant.case)).toContain("setModuleConfig"); + expect( + radio.cases().filter((c) => c === "removeFixedPosition"), + ).toHaveLength(2); + expect(editor.isDirty.value).toBe(false); + }); + + it("runs a commit requested during another after it finishes", async () => { + const { editor, radio } = setupCommit(); + const held = radio.hold("commitEditSettings"); + + const first = editor.commit(); + await held.reached; + editor.setModuleSection("mqtt", enableMqtt()); + const second = editor.commit(); + held.release(); + + expect((await first).status).toBe("ok"); + expect((await second).status).toBe("ok"); + expect(radio.cases()).toEqual([ + "beginEditSettings", + "setConfig", + "commitEditSettings", + "beginEditSettings", + "setModuleConfig", + "commitEditSettings", + ]); expect(editor.isDirty.value).toBe(false); }); + + it("closes the transaction and stays dirty when a send fails", async () => { + const { editor, radio } = setupCommit(); + editor.setModuleSection("mqtt", enableMqtt()); + radio.failNext("setModuleConfig"); + + expect((await editor.commit()).status).toBe("error"); + + expect(radio.cases()).toEqual([ + "beginEditSettings", + "setConfig", + "setModuleConfig", + "commitEditSettings", + ]); + expect(editor.dirtyRadioSections.value).toEqual([LORA]); + expect(editor.dirtyModuleSections.value).toEqual(["mqtt"]); + }); }); diff --git a/packages/sdk/src/features/config/domain/ConfigEditor.ts b/packages/sdk/src/features/config/domain/ConfigEditor.ts index a675778a0..f9e9b6a66 100644 --- a/packages/sdk/src/features/config/domain/ConfigEditor.ts +++ b/packages/sdk/src/features/config/domain/ConfigEditor.ts @@ -107,6 +107,10 @@ export class ConfigEditor { message: Protobuf.Admin.AdminMessage, ) => void; + private commitQueue: Promise = Promise.resolve(); + /** Bumped on disconnect so an in-flight commit can tell. */ + private connectionGeneration = 0; + constructor( client: MeshClient, options: { @@ -172,6 +176,7 @@ export class ConfigEditor { client.events.onDeviceStatus.subscribe((status) => { if (status === DeviceStatusEnum.DeviceDisconnected) { + this.connectionGeneration++; this.baselineRadio.value = {}; this.baselineModules.value = {}; this.baselineChannels.value = new Map(); @@ -297,59 +302,119 @@ export class ConfigEditor { /** * Send all dirty sections to the device wrapped in a beginEdit/commitEdit - * pair. On success the baseline is replaced with what was sent - * (optimistic update); inbound config packets after commit will reconcile. - * - * The user can keep editing while a commit is in flight. Those edits replace - * the working signals (they are never mutated), so they are not part of the - * snapshot taken here and stay dirty for the next commit. + * pair. Commits run one at a time; edits made during a commit stay dirty + * for the next one. */ - public async commit(): Promise> { + public commit(): Promise> { + const run = this.commitQueue.then(() => this.runCommit()); + this.commitQueue = run.catch(() => undefined); + return run; + } + + private async runCommit(): Promise> { if (!this._isDirty.peek()) return Result.ok(undefined); - const radio = this.workingRadio.peek(); - const dirtyRadio = this._dirtyRadioSections.peek(); - const modules = this.workingModules.peek(); - const dirtyModules = this._dirtyModuleSections.peek(); - const channels = this.workingChannels.peek(); - const dirtyChannels = this._dirtyChannels.peek(); - const owner = this.workingOwner.peek(); - const ownerDirty = this._isOwnerDirty.peek(); - const queued = this.queuedAdminMessages.peek(); - const stagedByKey = this._stagedAdminMessages.peek(); + // Taken before the first await. Setters replace the working signals, so + // later edits never reach it. + const payload = this.collectPayload(); + // An empty transaction makes the radio rewrite its config and report + // success for nothing. + if (isEmptyPayload(payload)) return Result.ok(undefined); + const generation = this.connectionGeneration; const beginResult = await beginEditSettings(this.client); if (Result.isError(beginResult)) return Result.err(beginResult.error); - for (const variant of dirtyRadio) { + const sendResult = await this.sendPayload(payload); + if (Result.isError(sendResult)) { + // There is no abort message. Close the transaction rather than leave it + // open; everything stays dirty, so the next commit resends it. + if (generation === this.connectionGeneration) { + await commitEditSettings(this.client); + } + return Result.err(sendResult.error); + } + + const commitResult = await commitEditSettings(this.client); + if (Result.isError(commitResult)) return Result.err(commitResult.error); + + // Disconnect already cleared the editor; don't carry this payload into + // the next connection. + if (generation !== this.connectionGeneration) { + return Result.err( + new Error("The device disconnected before the commit finished"), + ); + } + + this.reconcileAfterCommit(payload); + for (const message of payload.staged.values()) { + this.onStagedAdminMessageCommitted?.(message); + } + return Result.ok(undefined); + } + + private collectPayload(): CommitPayload { + const radio = this.workingRadio.peek(); + const radioPayload: CommitPayload["radio"] = []; + for (const variant of this._dirtyRadioSections.peek()) { const value = radio[variant]; - if (value === undefined) continue; - const config = buildRadioConfig(variant, value); - const result = await setConfig(this.client, config); - if (Result.isError(result)) return Result.err(result.error); + if (value !== undefined) radioPayload.push([variant, value]); } - for (const variant of dirtyModules) { + const modules = this.workingModules.peek(); + const modulePayload: CommitPayload["modules"] = []; + for (const variant of this._dirtyModuleSections.peek()) { const value = modules[variant]; - if (value === undefined) continue; - const moduleConfig = buildModuleConfig(variant, value); - const result = await setModuleConfig(this.client, moduleConfig); - if (Result.isError(result)) return Result.err(result.error); + if (value !== undefined) modulePayload.push([variant, value]); } - for (const index of dirtyChannels) { + const channels = this.workingChannels.peek(); + const channelPayload: CommitPayload["channels"] = []; + for (const index of this._dirtyChannels.peek()) { const channel = channels.get(index); - if (!channel) continue; + if (channel) channelPayload.push([index, channel]); + } + + return { + radio: radioPayload, + modules: modulePayload, + channels: channelPayload, + owner: this._isOwnerDirty.peek() ? this.workingOwner.peek() : undefined, + queued: this.queuedAdminMessages.peek(), + staged: this._stagedAdminMessages.peek(), + }; + } + + private async sendPayload( + payload: CommitPayload, + ): Promise> { + for (const [variant, value] of payload.radio) { + const result = await setConfig( + this.client, + buildRadioConfig(variant, value), + ); + if (Result.isError(result)) return Result.err(result.error); + } + + for (const [variant, value] of payload.modules) { + const result = await setModuleConfig( + this.client, + buildModuleConfig(variant, value), + ); + if (Result.isError(result)) return Result.err(result.error); + } + + for (const [, channel] of payload.channels) { const result = await setChannel(this.client, channel); if (Result.isError(result)) return Result.err(result.error); } - if (ownerDirty && owner) { - const result = await setOwner(this.client, owner); + if (payload.owner) { + const result = await setOwner(this.client, payload.owner); if (Result.isError(result)) return Result.err(result.error); } - for (const message of queued) { + for (const message of [...payload.queued, ...payload.staged.values()]) { const variant = message.payloadVariant; if (!variant.case) continue; try { @@ -359,41 +424,60 @@ export class ConfigEditor { } } - const staged = [...stagedByKey.values()]; - for (const message of staged) { - const variant = message.payloadVariant; - if (!variant.case) continue; - try { - await sendAdminMessage(this.client, variant); - } catch (e) { - return Result.err(e instanceof Error ? e : new Error(String(e))); + return Result.ok(undefined); + } + + /** + * Make only what was sent the new baseline. Other sections keep theirs, + * including device packets that arrived during the commit. + */ + private reconcileAfterCommit(payload: CommitPayload): void { + if (payload.radio.length > 0) { + const baseline: Record = { + ...this.baselineRadio.peek(), + }; + for (const [variant, value] of payload.radio) baseline[variant] = value; + this.baselineRadio.value = baseline as RadioConfig; + } + + if (payload.modules.length > 0) { + const baseline: Record = { + ...this.baselineModules.peek(), + }; + for (const [variant, value] of payload.modules) baseline[variant] = value; + this.baselineModules.value = baseline as ModuleConfig; + } + + if (payload.channels.length > 0) { + const baseline = new Map(this.baselineChannels.peek()); + for (const [index, channel] of payload.channels) { + baseline.set(index, channel); } + this.baselineChannels.value = baseline; } - const commitResult = await commitEditSettings(this.client); - if (Result.isError(commitResult)) return Result.err(commitResult.error); + if (payload.owner) this.baselineOwner.value = payload.owner; - this.baselineRadio.value = radio; - this.baselineModules.value = modules; - this.baselineChannels.value = new Map(channels); - this.baselineOwner.value = owner; - // Drop only the admin messages that were sent; one queued or staged while - // the commit was in flight has not been sent yet. - this.queuedAdminMessages.value = this.queuedAdminMessages - .peek() - .filter((message) => !queued.includes(message)); - const unsent = new Map(this._stagedAdminMessages.peek()); - for (const [key, message] of stagedByKey) { - if (unsent.get(key) === message) unsent.delete(key); + // One entry per message sent: the same object may have been queued again + // during the commit, and that copy hasn't been sent. + if (payload.queued.length > 0) { + const remaining = [...this.queuedAdminMessages.peek()]; + for (const message of payload.queued) { + const index = remaining.indexOf(message); + if (index !== -1) remaining.splice(index, 1); + } + this.queuedAdminMessages.value = remaining; } - this._stagedAdminMessages.value = unsent; - this.recomputeDirty(); - for (const message of staged) { - this.onStagedAdminMessageCommitted?.(message); + if (payload.staged.size > 0) { + const unsent = new Map(this._stagedAdminMessages.peek()); + for (const [key, message] of payload.staged) { + if (unsent.get(key) === message) unsent.delete(key); + } + this._stagedAdminMessages.value = unsent; } - return Result.ok(undefined); + this.recomputeDirty(); } private recomputeDirty(): void { @@ -467,6 +551,30 @@ export class ConfigEditor { } } +interface CommitPayload { + radio: [RadioConfigSection, NonNullable][]; + modules: [ + ModuleConfigSection, + NonNullable, + ][]; + channels: [number, Protobuf.Channel.Channel][]; + /** Set only when the owner was dirty. */ + owner: Protobuf.Mesh.User | undefined; + queued: readonly Protobuf.Admin.AdminMessage[]; + staged: ReadonlyMap; +} + +function isEmptyPayload(payload: CommitPayload): boolean { + return ( + payload.radio.length === 0 && + payload.modules.length === 0 && + payload.channels.length === 0 && + payload.owner === undefined && + payload.queued.length === 0 && + payload.staged.size === 0 + ); +} + function buildRadioConfig( variant: RadioConfigSection, value: unknown, From fc10def78bb4506e763e9ce9e6eebcca693a28c3 Mon Sep 17 00:00:00 2001 From: Dan Ditomaso Date: Sun, 18 Oct 2026 16:12:45 -0400 Subject: [PATCH 4/6] fix(web): don't report edits made during a save as saved If the editor still has pending changes after a commit, show a "save again" toast and keep the form dirty instead of "All configuration changes have been saved". --- apps/web/public/i18n/locales/en/ui.json | 4 ++++ apps/web/src/pages/Settings/index.tsx | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/apps/web/public/i18n/locales/en/ui.json b/apps/web/public/i18n/locales/en/ui.json index 80d0f2edd..802a113e8 100644 --- a/apps/web/public/i18n/locales/en/ui.json +++ b/apps/web/public/i18n/locales/en/ui.json @@ -67,6 +67,10 @@ "title": "Saved", "description": "All configuration changes have been saved." }, + "savePending": { + "title": "Saved", + "description": "Changes made while saving haven't been sent yet. Save again to send them." + }, "favoriteNode": { "title": "{{action}} {{nodeName}} {{direction}} favorites.", "action": { diff --git a/apps/web/src/pages/Settings/index.tsx b/apps/web/src/pages/Settings/index.tsx index a66be6552..2ad84c168 100644 --- a/apps/web/src/pages/Settings/index.tsx +++ b/apps/web/src/pages/Settings/index.tsx @@ -313,6 +313,16 @@ const ConfigPage = () => { if (result.status === "error") { throw result.error; } + + // Edits made while the save was in flight are still pending. + if (editor.isDirty.value) { + toast({ + title: t("toast.savePending.title"), + description: t("toast.savePending.description"), + }); + return; + } + toast({ title: t("toast.saveAllSuccess.title"), description: t("toast.saveAllSuccess.description"), From b2fe1f8740bada571623560c9518ea5962954f49 Mon Sep 17 00:00:00 2001 From: Dan Ditomaso Date: Sun, 18 Oct 2026 16:12:46 -0400 Subject: [PATCH 5/6] test(web): share the settings form harness and clean up after tests - renderSettingsForm builds the fake radio, registry and providers and removes the device and client after each test. - In-flight tests wait for the held send to be reached instead of a fixed tick, so a slower send path can't make them hang. - Use the SDK's typed sent-config decoders instead of local copies. Co-authored-by: Matt <57228426+xtantaudio@users.noreply.github.com> --- .../PageComponents/ModuleConfig/MQTT.test.tsx | 143 +++--------- .../Settings/DeviceConfig.test.tsx | 219 ++++-------------- apps/web/src/tests/renderSettingsForm.tsx | 64 +++++ 3 files changed, 142 insertions(+), 284 deletions(-) create mode 100644 apps/web/src/tests/renderSettingsForm.tsx diff --git a/apps/web/src/components/PageComponents/ModuleConfig/MQTT.test.tsx b/apps/web/src/components/PageComponents/ModuleConfig/MQTT.test.tsx index b6986375c..a51aacd1b 100644 --- a/apps/web/src/components/PageComponents/ModuleConfig/MQTT.test.tsx +++ b/apps/web/src/components/PageComponents/ModuleConfig/MQTT.test.tsx @@ -1,127 +1,54 @@ -import { create, fromBinary } from "@bufbuild/protobuf"; -import { CurrentDeviceContext, useDeviceStore } from "@core/stores"; -import { MeshClient, MeshRegistry, Protobuf } from "@meshtastic/sdk"; -import { createFakeTransport } from "@meshtastic/sdk/testing"; -import { MeshRegistryProvider } from "@meshtastic/sdk-react"; -import { render, screen, waitFor } from "@testing-library/react"; +import { create } from "@bufbuild/protobuf"; +import { renderSettingsForm } from "@app/tests/renderSettingsForm.tsx"; +import { Protobuf } from "@meshtastic/sdk"; +import { screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; -import { Suspense } from "react"; import { describe, expect, it } from "vitest"; import { MQTT } from "./MQTT.tsx"; -/** - * End-to-end cover for the MQTT module-config save path: the real settings - * form -> `ConfigEditor` staging -> the AdminMessage bytes that go to the - * radio. - * - * The assertions decode the outgoing packet from its wire encoding, because - * protobuf omits default booleans: if `enabled` is not genuinely `true` when - * the message is serialised, field 1 is simply absent from the bytes and the - * device silently keeps MQTT switched off — which is exactly the bug this - * covers. - */ +// Protobuf omits default booleans on the wire, so these tests decode what was +// actually sent rather than trusting the editor's state. -let deviceIdSeq = 100; - -function setup(mqtt: Protobuf.ModuleConfig.ModuleConfig_MQTTConfig) { - const { transport } = createFakeTransport(); - const registry = new MeshRegistry(); - const client = new MeshClient({ transport }); - const connectionId = deviceIdSeq++; - registry.register(connectionId, client); - registry.setActive(connectionId); - - const sent: Protobuf.Admin.AdminMessage[] = []; - let release: (() => void) | undefined; - let gate: Protobuf.Admin.AdminMessage["payloadVariant"]["case"] | undefined; - - client.sendPacket = (async (payload: Uint8Array) => { - const admin = fromBinary(Protobuf.Admin.AdminMessageSchema, payload); - sent.push(admin); - if (gate && admin.payloadVariant.case === gate) { - gate = undefined; - await new Promise((resolve) => { - release = resolve; - }); - } - return 1; - }) as never; - - const packet = create(Protobuf.ModuleConfig.ModuleConfigSchema, { - payloadVariant: { case: "mqtt", value: mqtt }, - }); - const device = useDeviceStore.getState().addDevice(connectionId); - device.setModuleConfig(packet); - client.events.onModuleConfigPacket.dispatch(packet); - - render( - - - loading}> - {}} /> - - - , - ); - - return { - client, - editor: client.config.editor, - sent, - gateOn: ( - value: Protobuf.Admin.AdminMessage["payloadVariant"]["case"], - ): void => { - gate = value; +const deviceMqtt = () => + create(Protobuf.ModuleConfig.ModuleConfigSchema, { + payloadVariant: { + case: "mqtt", + value: create(Protobuf.ModuleConfig.ModuleConfig_MQTTConfigSchema, { + enabled: false, + address: "mqtt.example.org", + username: "meshdev", + password: "large4cats", + root: "msh", + }), }, - release: () => release?.(), - }; -} - -function mqttFromWire( - sent: Protobuf.Admin.AdminMessage[], -): Protobuf.ModuleConfig.ModuleConfig_MQTTConfig | undefined { - for (const admin of sent) { - const variant = admin.payloadVariant; - if (variant.case !== "setModuleConfig") continue; - const module = variant.value.payloadVariant; - if (module.case === "mqtt") return module.value; - } - return undefined; -} + }); -const deviceMqtt = () => - create(Protobuf.ModuleConfig.ModuleConfig_MQTTConfigSchema, { - enabled: false, - address: "mqtt.example.org", - username: "meshdev", - password: "large4cats", - root: "msh", +const renderMqtt = () => + renderSettingsForm( {}} />, { + moduleConfig: [deviceMqtt()], }); describe("MQTT module config", () => { it("puts `enabled: true` on the wire when the toggle is switched on", async () => { - const { editor, sent } = setup(deviceMqtt()); + const { editor, radio } = renderMqtt(); await userEvent.click(await screen.findByLabelText("MQTT Enabled")); - await waitFor(() => expect(editor.dirtyModuleSections.value).toContain("mqtt"), ); expect((await editor.commit()).status).toBe("ok"); - const wire = mqttFromWire(sent); - expect(wire).toBeDefined(); + const wire = radio.sentModuleConfig("mqtt"); expect(wire?.enabled).toBe(true); - // The text fields the user did not touch must still round-trip. + // Fields the user didn't touch still round-trip. expect(wire?.address).toBe("mqtt.example.org"); expect(wire?.username).toBe("meshdev"); expect(wire?.password).toBe("large4cats"); expect(wire?.root).toBe("msh"); }); - it("does not report the toggle as saved when it was not part of the commit", async () => { - const { client, editor, sent, gateOn, release } = setup(deviceMqtt()); - + it("keeps the toggle pending when it was flipped during another save", async () => { + const { client, editor, radio } = renderMqtt(); client.events.onConfigPacket.dispatch( create(Protobuf.Config.ConfigSchema, { payloadVariant: { @@ -135,34 +62,26 @@ describe("MQTT module config", () => { create(Protobuf.Config.Config_LoRaConfigSchema, { region: 4 }), ); - // Start an unrelated save and hold it open on the closing commitEditSettings. - gateOn("commitEditSettings"); + const held = radio.hold("commitEditSettings"); const pending = editor.commit(); - await new Promise((resolve) => setTimeout(resolve, 0)); + await held.reached; - // The user flips "MQTT enabled" while that save is still in flight. await userEvent.click(await screen.findByLabelText("MQTT Enabled")); await waitFor(() => expect(editor.dirtyModuleSections.value).toContain("mqtt"), ); - - release(); + held.release(); expect((await pending).status).toBe("ok"); - // The transaction really did open and commit on the device, but it never - // carried the MQTT payload — so the edit has to stay pending. - expect(sent.map((a) => a.payloadVariant.case)).toEqual([ + expect(radio.cases()).toEqual([ "beginEditSettings", "setConfig", "commitEditSettings", ]); - expect(mqttFromWire(sent)).toBeUndefined(); expect(editor.dirtyModuleSections.value).toContain("mqtt"); - expect(editor.isDirty.value).toBe(true); - // The next save carries it, with `enabled` intact. expect((await editor.commit()).status).toBe("ok"); - expect(mqttFromWire(sent)?.enabled).toBe(true); + expect(radio.sentModuleConfig("mqtt")?.enabled).toBe(true); expect(editor.isDirty.value).toBe(false); }); }); diff --git a/apps/web/src/components/PageComponents/Settings/DeviceConfig.test.tsx b/apps/web/src/components/PageComponents/Settings/DeviceConfig.test.tsx index e0851950d..c0c5b2f74 100644 --- a/apps/web/src/components/PageComponents/Settings/DeviceConfig.test.tsx +++ b/apps/web/src/components/PageComponents/Settings/DeviceConfig.test.tsx @@ -1,102 +1,15 @@ -import { create, fromBinary } from "@bufbuild/protobuf"; +import { create } from "@bufbuild/protobuf"; +import { renderSettingsForm } from "@app/tests/renderSettingsForm.tsx"; import { BluetoothValidationSchema } from "@app/validation/config/bluetooth.ts"; import { Bluetooth } from "@components/PageComponents/Settings/Bluetooth.tsx"; import { Device } from "@components/PageComponents/Settings/Device/index.tsx"; -import { CurrentDeviceContext, useDeviceStore } from "@core/stores"; -import { MeshClient, MeshRegistry, Protobuf } from "@meshtastic/sdk"; -import { createFakeTransport } from "@meshtastic/sdk/testing"; -import { MeshRegistryProvider } from "@meshtastic/sdk-react"; -import { render, screen, waitFor } from "@testing-library/react"; +import { Protobuf } from "@meshtastic/sdk"; +import { screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; -import { type ComponentType, Suspense } from "react"; import { describe, expect, it } from "vitest"; -/** - * End-to-end cover for the Device settings page. Its tabs (Device, Bluetooth, - * Display, Power, Network, Position) all stage through the *same* radio-config - * mechanism as LoRa — `ConfigEditor.setRadioSection` and `_dirtyRadioSections` - * — so they share both the commit bookkeeping and the "the form silently - * refuses to save" failure mode. - * - * Assertions decode the outgoing AdminMessage from its wire bytes: protobuf - * omits fields at their default, so a boolean that was never really set is - * simply missing on the device. - */ - -let deviceIdSeq = 800; - -function setup( - Component: ComponentType<{ onFormInit: () => void }>, - config: Protobuf.Config.Config, -) { - const { transport } = createFakeTransport(); - const registry = new MeshRegistry(); - const client = new MeshClient({ transport }); - const connectionId = deviceIdSeq++; - registry.register(connectionId, client); - registry.setActive(connectionId); - - const sent: Protobuf.Admin.AdminMessage[] = []; - let release: (() => void) | undefined; - let gate: Protobuf.Admin.AdminMessage["payloadVariant"]["case"] | undefined; - - client.sendPacket = (async (payload: Uint8Array) => { - const admin = fromBinary(Protobuf.Admin.AdminMessageSchema, payload); - sent.push(admin); - if (gate && admin.payloadVariant.case === gate) { - gate = undefined; - await new Promise((resolve) => { - release = resolve; - }); - } - return 1; - }) as never; - - const device = useDeviceStore.getState().addDevice(connectionId); - device.setConfig(config); - client.events.onConfigPacket.dispatch(config); - - render( - - - loading}> - {}} /> - - - , - ); - - return { - client, - editor: client.config.editor, - sent, - gateOn: ( - value: Protobuf.Admin.AdminMessage["payloadVariant"]["case"], - ): void => { - gate = value; - }, - release: () => release?.(), - }; -} - -/** - * Pull a config section back out of the transmitted AdminMessages. The value - * is cast to the caller's section type: the wire union cannot be narrowed - * generically, and the `case` check above already guarantees it. - */ -function fromWire( - sent: Protobuf.Admin.AdminMessage[], - section: Protobuf.Config.Config["payloadVariant"]["case"], -): T | undefined { - for (const admin of sent) { - if (admin.payloadVariant.case !== "setConfig") continue; - const variant = admin.payloadVariant.value.payloadVariant; - if (variant.case === section) { - return variant.value as T; - } - } - return undefined; -} +// Protobuf omits default booleans on the wire, so these tests decode what was +// actually sent rather than trusting the editor's state. const deviceConfig = () => create(Protobuf.Config.ConfigSchema, { @@ -124,130 +37,92 @@ const bluetoothConfig = (fixedPin: number) => }, }); +const primaryChannel = (uplinkEnabled: boolean) => + create(Protobuf.Channel.ChannelSchema, { + index: 0, + role: Protobuf.Channel.Channel_Role.PRIMARY, + settings: create(Protobuf.Channel.ChannelSettingsSchema, { + psk: new Uint8Array([1]), + uplinkEnabled, + }), + }); + +const renderDevice = () => + renderSettingsForm( {}} />, { + config: [deviceConfig()], + }); + describe("Device settings", () => { it("puts a toggled device-config flag on the wire", async () => { - const { editor, sent } = setup(Device as never, deviceConfig()); + const { editor, radio } = renderDevice(); await userEvent.click(await screen.findByLabelText("Double Tap as Button")); - await waitFor(() => expect(editor.dirtyRadioSections.value).toContain("device"), ); expect((await editor.commit()).status).toBe("ok"); - const wire = fromWire(sent, "device"); + const wire = radio.sentConfig("device"); expect(wire?.doubleTapAsButtonPress).toBe(true); - // Untouched fields must survive. expect(wire?.buttonGpio).toBe(4); expect(wire?.buzzerGpio).toBe(2); expect(wire?.nodeInfoBroadcastSecs).toBe(10800); expect(wire?.role).toBe(Protobuf.Config.Config_DeviceConfig_Role.CLIENT); }); - it("does not report the toggle as saved when it was not part of the commit", async () => { - const { client, editor, sent, gateOn, release } = setup( - Device as never, - deviceConfig(), - ); - - client.events.onChannelPacket.dispatch( - create(Protobuf.Channel.ChannelSchema, { - index: 0, - role: Protobuf.Channel.Channel_Role.PRIMARY, - settings: create(Protobuf.Channel.ChannelSettingsSchema, { - psk: new Uint8Array([1]), - }), - }), - ); - editor.setChannel( - create(Protobuf.Channel.ChannelSchema, { - index: 0, - role: Protobuf.Channel.Channel_Role.PRIMARY, - settings: create(Protobuf.Channel.ChannelSettingsSchema, { - psk: new Uint8Array([1]), - uplinkEnabled: true, - }), - }), - ); + it("keeps the toggle pending when it was flipped during another save", async () => { + const { client, editor, radio } = renderDevice(); + client.events.onChannelPacket.dispatch(primaryChannel(false)); + editor.setChannel(primaryChannel(true)); - // An unrelated channel save is in flight, held open on its closing commit. - gateOn("commitEditSettings"); + const held = radio.hold("commitEditSettings"); const pending = editor.commit(); - await new Promise((resolve) => setTimeout(resolve, 0)); + await held.reached; await userEvent.click(await screen.findByLabelText("Double Tap as Button")); await waitFor(() => expect(editor.dirtyRadioSections.value).toContain("device"), ); - - release(); + held.release(); expect((await pending).status).toBe("ok"); - // A genuine begin/commit pair happened on the device, but it never carried - // the device config — so that edit has to stay pending. - expect(sent.map((a) => a.payloadVariant.case)).toEqual([ + expect(radio.cases()).toEqual([ "beginEditSettings", "setChannel", "commitEditSettings", ]); - expect( - fromWire(sent, "device"), - ).toBeUndefined(); expect(editor.dirtyRadioSections.value).toContain("device"); - expect(editor.isDirty.value).toBe(true); expect((await editor.commit()).status).toBe("ok"); - expect( - fromWire(sent, "device") - ?.doubleTapAsButtonPress, - ).toBe(true); + expect(radio.sentConfig("device")?.doubleTapAsButtonPress).toBe(true); expect(editor.isDirty.value).toBe(false); }); it("saves Bluetooth on a device that has no fixed PIN", async () => { - // `fixed_pin` is 0 until FIXED_PIN pairing is used. The schema used to - // demand six digits unconditionally, which made this device's Bluetooth - // form permanently invalid and silently unsavable. - const { editor, sent } = setup(Bluetooth as never, bluetoothConfig(0)); + // Radios that never used FIXED_PIN pairing report a PIN of 0. + const { editor, radio } = renderSettingsForm( + {}} />, + { config: [bluetoothConfig(0)] }, + ); await userEvent.click(await screen.findByLabelText("Bluetooth enabled")); - await waitFor(() => expect(editor.dirtyRadioSections.value).toContain("bluetooth"), ); expect((await editor.commit()).status).toBe("ok"); - expect( - fromWire(sent, "bluetooth") - ?.enabled, - ).toBe(true); + expect(radio.sentConfig("bluetooth")?.enabled).toBe(true); }); it("still requires a six-digit PIN for FIXED_PIN pairing", () => { - // The relaxation is conditional, not a removal: PIN validation still - // applies where the PIN is actually used. - const base = { - enabled: true, - fixedPin: 0, - }; - expect( - BluetoothValidationSchema.safeParse({ - ...base, - mode: Protobuf.Config.Config_BluetoothConfig_PairingMode.FIXED_PIN, - }).success, - ).toBe(false); - expect( - BluetoothValidationSchema.safeParse({ - ...base, - fixedPin: 123456, - mode: Protobuf.Config.Config_BluetoothConfig_PairingMode.FIXED_PIN, - }).success, - ).toBe(true); - expect( - BluetoothValidationSchema.safeParse({ - ...base, - mode: Protobuf.Config.Config_BluetoothConfig_PairingMode.RANDOM_PIN, - }).success, - ).toBe(true); + const { FIXED_PIN, RANDOM_PIN } = + Protobuf.Config.Config_BluetoothConfig_PairingMode; + const parse = (mode: number, fixedPin: number) => + BluetoothValidationSchema.safeParse({ enabled: true, mode, fixedPin }) + .success; + + expect(parse(FIXED_PIN, 0)).toBe(false); + expect(parse(FIXED_PIN, 123456)).toBe(true); + expect(parse(RANDOM_PIN, 0)).toBe(true); }); }); diff --git a/apps/web/src/tests/renderSettingsForm.tsx b/apps/web/src/tests/renderSettingsForm.tsx new file mode 100644 index 000000000..6efc689fd --- /dev/null +++ b/apps/web/src/tests/renderSettingsForm.tsx @@ -0,0 +1,64 @@ +import { CurrentDeviceContext, useDeviceStore } from "@core/stores"; +import { MeshClient, MeshRegistry, type Protobuf } from "@meshtastic/sdk"; +import { + createFakeTransport, + interceptAdminMessages, +} from "@meshtastic/sdk/testing"; +import { MeshRegistryProvider } from "@meshtastic/sdk-react"; +import { cleanup, render } from "@testing-library/react"; +import { type ReactElement, Suspense } from "react"; +import { afterEach } from "vitest"; + +let nextDeviceId = 1000; +const teardowns: (() => void)[] = []; + +afterEach(() => { + // Unmount before removing the device the form reads from. + cleanup(); + for (const teardown of teardowns.splice(0)) teardown(); +}); + +/** + * Render a settings form against a connected fake radio, with `device` as the + * config it has reported. Torn down after each test. + */ +export function renderSettingsForm( + ui: ReactElement, + device: { + config?: Protobuf.Config.Config[]; + moduleConfig?: Protobuf.ModuleConfig.ModuleConfig[]; + }, +) { + const { transport } = createFakeTransport(); + const registry = new MeshRegistry(); + const client = new MeshClient({ transport }); + const id = nextDeviceId++; + registry.register(id, client); + registry.setActive(id); + const radio = interceptAdminMessages(client); + + const stored = useDeviceStore.getState().addDevice(id); + for (const config of device.config ?? []) { + stored.setConfig(config); + client.events.onConfigPacket.dispatch(config); + } + for (const moduleConfig of device.moduleConfig ?? []) { + stored.setModuleConfig(moduleConfig); + client.events.onModuleConfigPacket.dispatch(moduleConfig); + } + + teardowns.push(() => { + registry.unregister(id); + useDeviceStore.getState().removeDevice(id); + }); + + render( + + + loading}>{ui} + + , + ); + + return { client, editor: client.config.editor, radio }; +} From 17a2e7afd36e29a69cb51b24af9ba9b0ebf3e451 Mon Sep 17 00:00:00 2001 From: Dan Ditomaso Date: Sun, 18 Oct 2026 16:31:34 -0400 Subject: [PATCH 6/6] fix(sdk): treat an undefined config section as reverting it setRadioSection/setModuleSection stored undefined and marked the section dirty, but collectPayload skips undefined sections, so the commit sent nothing and the editor stayed dirty. Restore the baseline section instead. Raised by CodeRabbit on #1476. --- .../src/features/config/ConfigEditor.test.ts | 22 ++++++++++++++++++ .../features/config/domain/ConfigEditor.ts | 23 +++++++++++++++---- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/packages/sdk/src/features/config/ConfigEditor.test.ts b/packages/sdk/src/features/config/ConfigEditor.test.ts index 6649693f4..b8ecbf0c1 100644 --- a/packages/sdk/src/features/config/ConfigEditor.test.ts +++ b/packages/sdk/src/features/config/ConfigEditor.test.ts @@ -199,6 +199,28 @@ describe("ConfigEditor", () => { expect(editor.isDirty.value).toBe(false); }); + it("an undefined section reverts to the baseline", async () => { + const { transport } = createFakeTransport(); + const client = new MeshClient({ transport }); + const editor = client.config.editor; + const radio = interceptAdminMessages(client); + + client.events.onConfigPacket.dispatch(loraPacket(4)); + client.events.onModuleConfigPacket.dispatch(mqttPacket(false)); + editor.setRadioSection( + "lora", + create(Protobuf.Config.Config_LoRaConfigSchema, { region: 7 }), + ); + editor.setRadioSection("lora", undefined); + editor.setModuleSection("mqtt", undefined); + editor.setRadioSection("position", undefined); + + expect(editor.radio.value.lora?.region).toBe(4); + expect(editor.isDirty.value).toBe(false); + expect((await editor.commit()).status).toBe("ok"); + expect(radio.sent).toEqual([]); + }); + it("disconnect drops baseline and working", () => { const { transport } = createFakeTransport(); const client = new MeshClient({ transport }); diff --git a/packages/sdk/src/features/config/domain/ConfigEditor.ts b/packages/sdk/src/features/config/domain/ConfigEditor.ts index f9e9b6a66..b0f092892 100644 --- a/packages/sdk/src/features/config/domain/ConfigEditor.ts +++ b/packages/sdk/src/features/config/domain/ConfigEditor.ts @@ -254,17 +254,24 @@ export class ConfigEditor { /** * Stage a radio config section. `value` is merged over the current working * (or baseline) section, so fields a form doesn't render keep the device's - * value instead of being sent as protobuf defaults. + * value instead of being sent as protobuf defaults. `undefined` reverts the + * section to the baseline. */ public setRadioSection( key: K, value: RadioConfig[K], ): void { const working = this.workingRadio.peek(); - const current = working[key] ?? this.baselineRadio.peek()[key]; + const baseline = this.baselineRadio.peek()[key]; + const current = working[key] ?? baseline; this.workingRadio.value = { ...working, - [key]: current && value ? { ...current, ...value } : value, + [key]: + value === undefined + ? baseline + : current + ? { ...current, ...value } + : value, }; this.recomputeDirty(); } @@ -275,10 +282,16 @@ export class ConfigEditor { value: ModuleConfig[K], ): void { const working = this.workingModules.peek(); - const current = working[key] ?? this.baselineModules.peek()[key]; + const baseline = this.baselineModules.peek()[key]; + const current = working[key] ?? baseline; this.workingModules.value = { ...working, - [key]: current && value ? { ...current, ...value } : value, + [key]: + value === undefined + ? baseline + : current + ? { ...current, ...value } + : value, }; this.recomputeDirty(); }