From 0eb58ac290b98881828280b509910f276f72434b Mon Sep 17 00:00:00 2001 From: Matt <57228426+xtantaudio@users.noreply.github.com> Date: Sun, 30 Aug 2026 22:36:57 -0400 Subject: [PATCH] fix: config commit blanket-clears dirty flags for unsent sections When a config edit transaction committed, ConfigEditor marked every pending section as clean and overwrote the whole local baseline with current working state - regardless of which sections were actually included in that specific commit's outgoing payload. A field that was never transmitted to the device could get silently laundered into looking 'saved' in the UI simply because a different, unrelated commit succeeded around the same time. Added configEquality/configMerge helpers so commit only clears dirty state and updates baseline for the sections that genuinely went out on the wire, leaving any other still-pending edit correctly marked dirty for the next commit. Verified live against real hardware: an edit that previously appeared saved but was absent from the device's own persisted config (confirmed via raw protobuf decode) now stays correctly marked dirty until an actual transaction sends it. --- .../features/config/domain/ConfigEditor.ts | 140 ++--- .../features/config/domain/configEquality.ts | 140 +++++ .../features/config/domain/configMerge.ts | 78 +++ .../config/ConfigEditor.commit.test.ts | 323 ++++++++++++ .../config/ConfigEditor.sections.test.ts | 479 ++++++++++++++++++ .../features/config/domain/ConfigEditor.ts | 240 ++++++--- .../config/domain/configEquality.test.ts | 82 +++ .../features/config/domain/configEquality.ts | 140 +++++ .../config/domain/configMerge.test.ts | 77 +++ .../src/features/config/domain/configMerge.ts | 78 +++ 10 files changed, 1657 insertions(+), 120 deletions(-) create mode 100644 apps/web/src/sdk-preview/features/config/domain/configEquality.ts create mode 100644 apps/web/src/sdk-preview/features/config/domain/configMerge.ts create mode 100644 packages/sdk/src/features/config/ConfigEditor.commit.test.ts create mode 100644 packages/sdk/src/features/config/ConfigEditor.sections.test.ts create mode 100644 packages/sdk/src/features/config/domain/configEquality.test.ts create mode 100644 packages/sdk/src/features/config/domain/configEquality.ts create mode 100644 packages/sdk/src/features/config/domain/configMerge.test.ts create mode 100644 packages/sdk/src/features/config/domain/configMerge.ts diff --git a/apps/web/src/sdk-preview/features/config/domain/ConfigEditor.ts b/apps/web/src/sdk-preview/features/config/domain/ConfigEditor.ts index 3c8a9f94a..64e24a68b 100644 --- a/apps/web/src/sdk-preview/features/config/domain/ConfigEditor.ts +++ b/apps/web/src/sdk-preview/features/config/domain/ConfigEditor.ts @@ -18,6 +18,8 @@ import { buildModuleConfig, buildRadioConfig, } from "../infrastructure/configBuilders.ts"; +import { configValuesEqual } from "./configEquality.ts"; +import { mergeStagedValue } from "./configMerge.ts"; import { ConfigMapper } from "../infrastructure/ConfigMapper.ts"; import type { ModuleConfig, ModuleConfigSection } from "./ModuleConfig.ts"; import type { RadioConfig, RadioConfigSection } from "./RadioConfig.ts"; @@ -119,7 +121,11 @@ export class ConfigEditor { key: K, value: RadioConfig[K], ): void { - this.workingRadio.value = { ...this.workingRadio.peek(), [key]: value }; + // Forms stage the output of their Zod resolver, which drops every field + // the form does not declare. Merge over the device's value so those + // fields are not silently reset to their protobuf defaults on commit. + const merged = mergeStagedValue(this.baselineRadio.peek()[key], value); + this.workingRadio.value = { ...this.workingRadio.peek(), [key]: merged }; this.recomputeDirty(); } @@ -127,7 +133,11 @@ export class ConfigEditor { key: K, value: ModuleConfig[K], ): void { - this.workingModules.value = { ...this.workingModules.peek(), [key]: value }; + const merged = mergeStagedValue(this.baselineModules.peek()[key], value); + this.workingModules.value = { + ...this.workingModules.peek(), + [key]: merged, + }; this.recomputeDirty(); } @@ -140,26 +150,54 @@ export class ConfigEditor { /** * Send every dirty section to the device inside a beginEdit/commitEdit pair. - * On success the baseline is replaced with the working copy (optimistic); - * inbound config packets after commit reconcile. Any failure aborts and - * returns the error — the baseline is left untouched. + * + * The payload is frozen synchronously before the first `await`, and on + * success only the sections that were actually transmitted are promoted into + * the baseline. Edits staged while the transaction was in flight stay dirty + * and go out on the next commit instead of being silently marked as saved. + * Any failure aborts and returns the error — the baseline is left untouched. */ public async commit(): Promise> { if (!this._isDirty.peek()) { return Result.ok(undefined); } - const begin = await beginEditSettings(this.client); - if (Result.isError(begin)) { - return Result.err(begin.error); - } - const radio = this.workingRadio.peek(); + const radioPayload: Array< + [RadioConfigSection, NonNullable] + > = []; for (const section of this._dirtyRadioSections.peek()) { const value = radio[section]; if (value === undefined) { continue; } + radioPayload.push([section, value]); + } + + const modules = this.workingModules.peek(); + const modulePayload: Array< + [ModuleConfigSection, NonNullable] + > = []; + for (const section of this._dirtyModuleSections.peek()) { + const value = modules[section]; + if (value === undefined) { + continue; + } + modulePayload.push([section, value]); + } + + if (radioPayload.length === 0 && modulePayload.length === 0) { + // An empty begin/commit pair makes the device rewrite its config + // unchanged and report success — a "save" that saved nothing. + return Result.ok(undefined); + } + + const begin = await beginEditSettings(this.client); + if (Result.isError(begin)) { + return Result.err(begin.error); + } + + for (const [section, value] of radioPayload) { const result = await setConfig( this.client, buildRadioConfig(section, value), @@ -169,12 +207,7 @@ export class ConfigEditor { } } - const modules = this.workingModules.peek(); - for (const section of this._dirtyModuleSections.peek()) { - const value = modules[section]; - if (value === undefined) { - continue; - } + for (const [section, value] of modulePayload) { const result = await setModuleConfig( this.client, buildModuleConfig(section, value), @@ -189,11 +222,37 @@ export class ConfigEditor { return Result.err(commit.error); } - this.baselineRadio.value = this.workingRadio.peek(); - this.baselineModules.value = this.workingModules.peek(); - this._dirtyRadioSections.value = []; - this._dirtyModuleSections.value = []; - this._isDirty.value = false; + // Promote only what went on the wire, and only where the working copy is + // still the exact object we transmitted (setters always replace the whole + // section, so reference identity is an exact "untouched since" test). + if (radioPayload.length > 0) { + const current = this.workingRadio.peek(); + const baseline: Record = { + ...this.baselineRadio.peek(), + }; + for (const [section, value] of radioPayload) { + if (current[section] === value) { + baseline[section] = value; + } + } + this.baselineRadio.value = baseline as RadioConfig; + } + if (modulePayload.length > 0) { + const current = this.workingModules.peek(); + const baseline: Record = { + ...this.baselineModules.peek(), + }; + for (const [section, value] of modulePayload) { + if (current[section] === value) { + baseline[section] = value; + } + } + this.baselineModules.value = baseline as ModuleConfig; + } + + // Recompute instead of blanket-clearing, so anything staged mid-flight + // stays flagged as pending. + this.recomputeDirty(); return Result.ok(undefined); } @@ -206,7 +265,7 @@ export class ConfigEditor { ...Object.keys(radioWorking), ])) { const section = key as RadioConfigSection; - if (!shallowEqual(radioBase[section], radioWorking[section])) { + if (!configValuesEqual(radioBase[section], radioWorking[section])) { radioDirty.push(section); } } @@ -219,7 +278,7 @@ export class ConfigEditor { ...Object.keys(moduleWorking), ])) { const section = key as ModuleConfigSection; - if (!shallowEqual(moduleBase[section], moduleWorking[section])) { + if (!configValuesEqual(moduleBase[section], moduleWorking[section])) { moduleDirty.push(section); } } @@ -229,38 +288,3 @@ export class ConfigEditor { this._isDirty.value = radioDirty.length > 0 || moduleDirty.length > 0; } } - -/** Recursive value-equality used for dirty detection (matches the SDK helper). */ -function shallowEqual(a: unknown, b: unknown): boolean { - if (a === b) { - return true; - } - if (a === undefined || b === undefined || a === null || b === null) { - return false; - } - if (typeof a !== "object" || typeof b !== "object") { - return false; - } - const ao = a as Record; - const bo = b as Record; - const aKeys = Object.keys(ao); - const bKeys = Object.keys(bo); - if (aKeys.length !== bKeys.length) { - return false; - } - for (const k of aKeys) { - const av = ao[k]; - const bv = bo[k]; - if (av === bv) { - continue; - } - if (typeof av === "object" && typeof bv === "object") { - if (!shallowEqual(av, bv)) { - return false; - } - } else { - return false; - } - } - return true; -} diff --git a/apps/web/src/sdk-preview/features/config/domain/configEquality.ts b/apps/web/src/sdk-preview/features/config/domain/configEquality.ts new file mode 100644 index 000000000..2cf565016 --- /dev/null +++ b/apps/web/src/sdk-preview/features/config/domain/configEquality.ts @@ -0,0 +1,140 @@ +/** + * Value-equality used by {@link ConfigEditor} (mirror of the `@meshtastic/sdk` helper) to decide whether a config + * section still matches the device's baseline. + * + * This has to compare two things that are *never* structurally identical even + * when they mean exactly the same thing: + * + * - the **baseline**, which is a `@bufbuild/protobuf` message: it carries a + * `$typeName` marker and every singular scalar field is materialised as an + * own property holding its zero value (`false` / `0` / `""`), and + * - the **working copy**, which is whatever the UI staged. Web forms hand over + * plain objects produced by a Zod resolver, so they have no `$typeName`, and + * fields the form does not render are simply absent. + * + * A naive key-count/`Object.keys(a)` comparison therefore reports "changed" + * forever, which pins the working copy (the editor refuses to let inbound + * device config overwrite a section it believes is dirty) and makes the UI + * report stale values as if they were saved. It also gets `undefined` vs + * `false` wrong in both directions. + * + * The rules implemented here mirror protobuf's own semantics: + * + * - `$typeName` is metadata, not data. + * - Keys are compared over the *union* of both sides, so a field that only one + * side carries is never skipped. + * - An absent field is equal to that field's protobuf default + * (`undefined` == `false` / `0` / `0n` / `""` / empty bytes / empty list), + * and *only* to its default — an absent field is never equal to `true`. + * - An absent sub-message is equal to a sub-message whose fields are all + * defaults. + * - `Uint8Array` is compared byte-wise, repeated fields element-wise. + */ + +function isAbsent(value: unknown): boolean { + return value === undefined || value === null; +} + +/** True when `value` is the protobuf zero value for its type (or absent). */ +function isProtobufDefault(value: unknown): boolean { + if (isAbsent(value)) { + return true; + } + if (value instanceof Uint8Array) { + return value.byteLength === 0; + } + if (Array.isArray(value)) { + return value.length === 0; + } + if (typeof value === "object") { + return Object.entries(value as Record).every( + ([key, entry]) => key === "$typeName" || isProtobufDefault(entry), + ); + } + return value === false || value === 0 || value === 0n || value === ""; +} + +function bytesEqual(a: Uint8Array, b: Uint8Array): boolean { + if (a.byteLength !== b.byteLength) { + return false; + } + for (let i = 0; i < a.byteLength; i++) { + if (a[i] !== b[i]) { + return false; + } + } + return true; +} + +function isRecord(value: unknown): value is Record { + return ( + typeof value === "object" && + value !== null && + !Array.isArray(value) && + !(value instanceof Uint8Array) + ); +} + +const EMPTY_RECORD: Record = {}; + +export function configValuesEqual(a: unknown, b: unknown): boolean { + if (a === b) { + return true; + } + + if (a instanceof Uint8Array || b instanceof Uint8Array) { + if (a instanceof Uint8Array && b instanceof Uint8Array) { + return bytesEqual(a, b); + } + // One side is absent: equal only if the present side is empty bytes. + const present = a instanceof Uint8Array ? a : (b as Uint8Array); + const other = a instanceof Uint8Array ? b : a; + return present.byteLength === 0 && isAbsent(other); + } + + if (Array.isArray(a) || Array.isArray(b)) { + if (!Array.isArray(a) && !isAbsent(a)) { + return false; + } + if (!Array.isArray(b) && !isAbsent(b)) { + return false; + } + const left = Array.isArray(a) ? a : []; + const right = Array.isArray(b) ? b : []; + if (left.length !== right.length) { + return false; + } + return left.every((value, index) => configValuesEqual(value, right[index])); + } + + if (isRecord(a) || isRecord(b)) { + if (!isRecord(a) && !isAbsent(a)) { + return false; + } + if (!isRecord(b) && !isAbsent(b)) { + return false; + } + const left = isRecord(a) ? a : EMPTY_RECORD; + const right = isRecord(b) ? b : EMPTY_RECORD; + const keys = new Set([...Object.keys(left), ...Object.keys(right)]); + for (const key of keys) { + if (key === "$typeName") { + continue; + } + if (!configValuesEqual(left[key], right[key])) { + return false; + } + } + return true; + } + + // Scalars. An absent field carries its protobuf default, so `undefined` + // equals `false`/`0`/`""` — but never `true`, `1`, or a non-empty string. + if (isAbsent(a)) { + return isProtobufDefault(b); + } + if (isAbsent(b)) { + return isProtobufDefault(a); + } + return false; +} diff --git a/apps/web/src/sdk-preview/features/config/domain/configMerge.ts b/apps/web/src/sdk-preview/features/config/domain/configMerge.ts new file mode 100644 index 000000000..36622bc6d --- /dev/null +++ b/apps/web/src/sdk-preview/features/config/domain/configMerge.ts @@ -0,0 +1,78 @@ +/** + * Merge a staged (UI-produced) config value over the value the device last + * reported, so that a partial form submission can never silently reset fields + * the form does not manage. + * + * Why this is needed + * ------------------ + * Every settings form in the web app hands {@link ConfigEditor} the *output of + * its Zod resolver*, and `commit()` feeds that object straight into + * `create(SomeSchema, value)`. Zod object schemas strip keys they do not + * declare, so any protobuf field the form does not list simply disappears from + * the staged object — and `create()` then materialises it at its protobuf + * default. Saving one toggle therefore silently rewrites every unlisted field + * on the device to `false` / `0` / `""`. + * + * The same gap also produced phantom dirty state: a section whose baseline + * carries a non-default value that the form omits never compares equal to the + * staged object, so it stayed flagged as "unsaved" forever. + * + * The rule implemented here is deliberately narrow: + * + * - Only keys that are **absent** (`undefined`) in the staged value and + * **present** in the baseline are filled in. A key the form did send always + * wins, including `false`, `0`, `""` and `[]` — clearing a list or turning a + * toggle off must still reach the device. + * - Recursion happens only where both sides are plain records, so protobuf + * sub-messages (`mapReportSettings`, `moduleSettings`, `ipv4Config`, …) are + * merged field-by-field instead of being replaced wholesale. + * - `Uint8Array` and arrays are values, never merged element-wise. + * - When the staged value is already a full protobuf message every singular + * field is materialised, so this is a no-op — staging a `create()`d message + * (channels, the LoRa import path, …) behaves exactly as before. + */ + +function isMergeableRecord(value: unknown): value is Record { + return ( + typeof value === "object" && + value !== null && + !Array.isArray(value) && + !(value instanceof Uint8Array) + ); +} + +export function mergeStagedValue(baseline: unknown, staged: T): T { + if (!isMergeableRecord(staged) || !isMergeableRecord(baseline)) { + return staged; + } + + let merged: Record | undefined; + const write = (key: string, value: unknown): void => { + merged ??= { ...(staged as Record) }; + merged[key] = value; + }; + + for (const [key, baseValue] of Object.entries(baseline)) { + if (key === "$typeName") { + continue; + } + const stagedValue = (staged as Record)[key]; + + if (stagedValue === undefined) { + // The form never declared this field — keep what the device reported. + if (baseValue !== undefined) { + write(key, baseValue); + } + continue; + } + + if (isMergeableRecord(stagedValue) && isMergeableRecord(baseValue)) { + const mergedChild = mergeStagedValue(baseValue, stagedValue); + if (mergedChild !== stagedValue) { + write(key, mergedChild); + } + } + } + + return (merged ?? staged) as T; +} diff --git a/packages/sdk/src/features/config/ConfigEditor.commit.test.ts b/packages/sdk/src/features/config/ConfigEditor.commit.test.ts new file mode 100644 index 000000000..b379a3457 --- /dev/null +++ b/packages/sdk/src/features/config/ConfigEditor.commit.test.ts @@ -0,0 +1,323 @@ +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"; +import { createFakeTransport } from "../../core/testing/createFakeTransport.ts"; + +/** + * Regression coverage for the "the device committed, but the value never went + * out" class of bug. + * + * `ConfigEditor.commit()` used to read the dirty lists lazily (interleaved + * with network round-trips) and then blanket-overwrite the baseline with the + * whole working copy and clear *every* dirty flag. Anything staged while the + * transaction was in flight was therefore folded into the baseline and marked + * synced without ever having been transmitted: the daemon logged a genuine + * beginEditSettings/commitEditSettings pair (and rewrote its config file), the + * UI showed no pending changes, and the field silently never reached the + * radio. + */ + +interface Harness { + client: MeshClient; + editor: MeshClient["config"]["editor"]; + /** Decoded admin messages, in transmission order. */ + sent: Protobuf.Admin.AdminMessage[]; + /** Resolves the in-flight admin send whose case matches `gate`. */ + release(): void; +} + +function createHarness( + gate?: Protobuf.Admin.AdminMessage["payloadVariant"]["case"], +): Harness { + const { transport } = createFakeTransport(); + const client = new MeshClient({ transport }); + const sent: Protobuf.Admin.AdminMessage[] = []; + let release: (() => void) | undefined; + // The gate only holds the first matching message, so a follow-up commit in + // the same test runs to completion. + let gated = gate !== undefined; + + client.sendPacket = (async (payload: Uint8Array) => { + // Decode from the wire bytes, so the assertions see exactly what the + // device would see (protobuf omits default booleans, so a missing + // `enabled` here is a missing `enabled` on the radio). + const admin = fromBinary(Protobuf.Admin.AdminMessageSchema, payload); + sent.push(admin); + if (gated && admin.payloadVariant.case === gate) { + gated = false; + await new Promise((resolve) => { + release = resolve; + }); + } + return 1; + }) as never; + + return { + client, + editor: client.config.editor, + sent, + release: () => release?.(), + }; +} + +function summarise(sent: Protobuf.Admin.AdminMessage[]): string[] { + return sent.map((admin) => { + const variant = admin.payloadVariant; + switch (variant.case) { + case "setConfig": + return `setConfig:${variant.value.payloadVariant.case}`; + case "setModuleConfig": + return `setModuleConfig:${variant.value.payloadVariant.case}`; + default: + return String(variant.case); + } + }); +} + +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; +} + +function mqttPacket( + init: Partial<{ enabled: boolean; address: string; username: string }>, +): Protobuf.ModuleConfig.ModuleConfig { + return create(Protobuf.ModuleConfig.ModuleConfigSchema, { + payloadVariant: { + case: "mqtt", + value: create(Protobuf.ModuleConfig.ModuleConfig_MQTTConfigSchema, init), + }, + }); +} + +function loraPacket(region: number): Protobuf.Config.Config { + return create(Protobuf.Config.ConfigSchema, { + payloadVariant: { + case: "lora", + value: create(Protobuf.Config.Config_LoRaConfigSchema, { region }), + }, + }); +} + +/** + * What the web MQTT form hands to `setModuleSection`: a plain object produced + * by the Zod resolver, so no `$typeName` and no protobuf prototype. + */ +function stagedMqttForm(enabled: boolean) { + return { + enabled, + address: "mqtt.example.org", + username: "meshdev", + password: "large4cats", + encryptionEnabled: false, + jsonEnabled: false, + tlsEnabled: false, + root: "msh", + proxyToClientEnabled: false, + mapReportingEnabled: false, + }; +} + +describe("ConfigEditor.commit() payload", () => { + it("transmits the MQTT `enabled` boolean the form staged", async () => { + const { client, editor, sent } = createHarness(); + client.events.onModuleConfigPacket.dispatch( + mqttPacket({ enabled: false, address: "mqtt.example.org" }), + ); + + editor.setModuleSection("mqtt", stagedMqttForm(true) as never); + expect(editor.dirtyModuleSections.value).toEqual(["mqtt"]); + + const result = await editor.commit(); + expect(result.status).toBe("ok"); + + // `enabled` must survive protobuf encoding — protobuf drops default + // booleans, so decoding it back off the wire proves it was really set. + expect(mqttFromWire(sent)?.enabled).toBe(true); + expect(summarise(sent)).toEqual([ + "beginEditSettings", + "setModuleConfig:mqtt", + "commitEditSettings", + ]); + }); + + it("keeps an edit staged during an in-flight commit pending instead of marking it synced", async () => { + const harness = createHarness("commitEditSettings"); + const { client, editor, sent } = harness; + + client.events.onConfigPacket.dispatch(loraPacket(1)); + client.events.onModuleConfigPacket.dispatch(mqttPacket({ enabled: false })); + + // The user saves an unrelated LoRa change... + editor.setRadioSection( + "lora", + create(Protobuf.Config.Config_LoRaConfigSchema, { region: 4 }), + ); + const pending = editor.commit(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + // ...and toggles "MQTT enabled" on while that save is still round-tripping. + editor.setModuleSection("mqtt", stagedMqttForm(true) as never); + expect(editor.dirtyModuleSections.value).toEqual(["mqtt"]); + + harness.release(); + expect((await pending).status).toBe("ok"); + + // The MQTT edit was never part of this transaction... + expect(summarise(sent)).toEqual([ + "beginEditSettings", + "setConfig:lora", + "commitEditSettings", + ]); + expect(mqttFromWire(sent)).toBeUndefined(); + // ...so it must still be pending, not silently laundered as "saved". + expect(editor.dirtyModuleSections.value).toEqual(["mqtt"]); + expect(editor.isDirty.value).toBe(true); + + // And it goes out on the next commit, with `enabled` intact. + expect((await editor.commit()).status).toBe("ok"); + expect(mqttFromWire(sent)?.enabled).toBe(true); + expect(editor.dirtyModuleSections.value).toEqual([]); + expect(editor.isDirty.value).toBe(false); + }); + + it("committing one section leaves an unrelated section's edit dirty", async () => { + const harness = createHarness("beginEditSettings"); + const { client, editor, sent } = harness; + + client.events.onConfigPacket.dispatch(loraPacket(1)); + client.events.onModuleConfigPacket.dispatch(mqttPacket({ enabled: false })); + + // Only LoRa is staged when the commit starts. + editor.setRadioSection( + "lora", + create(Protobuf.Config.Config_LoRaConfigSchema, { region: 4 }), + ); + const pending = editor.commit(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + // MQTT is staged after the payload was frozen, so it is not in this commit. + editor.setModuleSection("mqtt", stagedMqttForm(true) as never); + + harness.release(); + expect((await pending).status).toBe("ok"); + + expect(summarise(sent)).not.toContain("setModuleConfig:mqtt"); + expect(editor.dirtyRadioSections.value).toEqual([]); + expect(editor.dirtyModuleSections.value).toEqual(["mqtt"]); + expect(editor.modules.value.mqtt?.enabled).toBe(true); + }); + + it("re-staging a section mid-flight keeps it dirty so the newer value is sent", async () => { + const harness = createHarness("commitEditSettings"); + const { client, editor, sent } = harness; + + client.events.onModuleConfigPacket.dispatch(mqttPacket({ enabled: false })); + + editor.setModuleSection("mqtt", stagedMqttForm(true) as never); + const pending = editor.commit(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + // The user turns it back off before the transaction completes. + editor.setModuleSection("mqtt", stagedMqttForm(false) as never); + + harness.release(); + expect((await pending).status).toBe("ok"); + + expect(mqttFromWire(sent)?.enabled).toBe(true); + // The newer (off) value was never transmitted, so it is still pending. + expect(editor.dirtyModuleSections.value).toEqual(["mqtt"]); + }); + + it("does not open an empty begin/commit transaction", async () => { + const { client, editor, sent } = createHarness(); + client.events.onModuleConfigPacket.dispatch( + mqttPacket({ enabled: true, address: "mqtt.example.org" }), + ); + + // A section that is dirty only because the baseline has a value the + // working copy lost cannot be transmitted; opening a transaction for it + // would make the device rewrite its config unchanged and report success. + editor.setModuleSection("mqtt", undefined as never); + expect(editor.isDirty.value).toBe(true); + + const result = await editor.commit(); + expect(result.status).toBe("ok"); + expect(sent).toEqual([]); + // Still pending — nothing was sent, so nothing may be marked synced. + expect(editor.dirtyModuleSections.value).toEqual(["mqtt"]); + }); + + it("does not mark a skipped section synced when another section is sent", async () => { + const { client, editor, sent } = createHarness(); + client.events.onConfigPacket.dispatch(loraPacket(1)); + client.events.onModuleConfigPacket.dispatch( + mqttPacket({ enabled: true, address: "mqtt.example.org" }), + ); + + editor.setRadioSection( + "lora", + create(Protobuf.Config.Config_LoRaConfigSchema, { region: 4 }), + ); + editor.setModuleSection("mqtt", undefined as never); + + expect((await editor.commit()).status).toBe("ok"); + + expect(summarise(sent)).toEqual([ + "beginEditSettings", + "setConfig:lora", + "commitEditSettings", + ]); + expect(editor.dirtyRadioSections.value).toEqual([]); + expect(editor.dirtyModuleSections.value).toEqual(["mqtt"]); + }); +}); + +describe("ConfigEditor dirty tracking", () => { + it("does not flag a staged form object that matches the device", () => { + const { client, editor } = createHarness(); + client.events.onModuleConfigPacket.dispatch( + mqttPacket({ enabled: false, address: "mqtt.example.org" }), + ); + + // Re-saving the form without touching anything must not look like a change + // just because the form stages a plain object rather than a protobuf + // message. + editor.setModuleSection("mqtt", { + enabled: false, + address: "mqtt.example.org", + } as never); + + expect(editor.dirtyModuleSections.value).toEqual([]); + expect(editor.isDirty.value).toBe(false); + }); + + it("lets inbound device config refresh a section that is no longer really dirty", () => { + const { client, editor } = createHarness(); + client.events.onModuleConfigPacket.dispatch( + mqttPacket({ enabled: false, address: "mqtt.example.org" }), + ); + editor.setModuleSection("mqtt", { + enabled: false, + address: "mqtt.example.org", + } as never); + + // Previously this section stayed dirty forever (plain object vs protobuf + // message never compared equal), so the editor refused to apply device + // updates and the UI kept showing the stale staged value. + client.events.onModuleConfigPacket.dispatch( + mqttPacket({ enabled: true, address: "mqtt.example.org" }), + ); + + expect(editor.dirtyModuleSections.value).toEqual([]); + expect(editor.modules.value.mqtt?.enabled).toBe(true); + }); +}); diff --git a/packages/sdk/src/features/config/ConfigEditor.sections.test.ts b/packages/sdk/src/features/config/ConfigEditor.sections.test.ts new file mode 100644 index 000000000..0fb4f1a5a --- /dev/null +++ b/packages/sdk/src/features/config/ConfigEditor.sections.test.ts @@ -0,0 +1,479 @@ +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"; +import { createFakeTransport } from "../../core/testing/createFakeTransport.ts"; + +/** + * Per-section regression coverage for the four config categories the editor + * tracks independently — radio config (LoRa), device config (`device`, + * `bluetooth`, … — the same radio-section mechanism), module config and + * channels. + * + * Every assertion decodes the constructed AdminMessage from its wire bytes, + * because protobuf omits fields at their default: a boolean that was never + * really set is simply absent from the encoding, which is exactly how a + * "successful" save can leave the radio unchanged. + */ + +interface Harness { + client: MeshClient; + editor: MeshClient["config"]["editor"]; + sent: Protobuf.Admin.AdminMessage[]; + gateOn(value: Protobuf.Admin.AdminMessage["payloadVariant"]["case"]): void; + release(): void; +} + +function createHarness(): Harness { + const { transport } = createFakeTransport(); + const client = new MeshClient({ transport }); + 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; + + return { + client, + editor: client.config.editor, + sent, + gateOn: (value) => { + gate = value; + }, + release: () => release?.(), + }; +} + +function summarise(sent: Protobuf.Admin.AdminMessage[]): string[] { + return sent.map((admin) => { + const variant = admin.payloadVariant; + switch (variant.case) { + case "setConfig": + return `setConfig:${variant.value.payloadVariant.case}`; + case "setModuleConfig": + return `setModuleConfig:${variant.value.payloadVariant.case}`; + default: + return String(variant.case); + } + }); +} + +function channelFromWire( + sent: Protobuf.Admin.AdminMessage[], +): Protobuf.Channel.Channel | undefined { + for (const admin of sent) { + if (admin.payloadVariant.case === "setChannel") { + return admin.payloadVariant.value; + } + } + return undefined; +} + +/** + * 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 radioFromWire( + 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; +} + +function primaryChannel( + init: Partial<{ + uplinkEnabled: boolean; + downlinkEnabled: boolean; + isMuted: boolean; + channelNum: number; + }> = {}, +): Protobuf.Channel.Channel { + return create(Protobuf.Channel.ChannelSchema, { + index: 0, + role: Protobuf.Channel.Channel_Role.PRIMARY, + settings: create(Protobuf.Channel.ChannelSettingsSchema, { + channelNum: init.channelNum ?? 0, + psk: new Uint8Array([1]), + name: "", + id: 4242, + uplinkEnabled: init.uplinkEnabled ?? false, + downlinkEnabled: init.downlinkEnabled ?? false, + moduleSettings: create(Protobuf.Channel.ModuleSettingsSchema, { + positionPrecision: 13, + isMuted: init.isMuted ?? false, + }), + }), + }); +} + +/** What a channel form stages: a freshly built message, uplink flipped on. */ +function stagedChannel(uplinkEnabled: boolean): Protobuf.Channel.Channel { + const next = primaryChannel(); + return create(Protobuf.Channel.ChannelSchema, { + ...next, + settings: { ...next.settings, uplinkEnabled }, + }); +} + +function devicePacket( + init: Partial<{ + role: number; + buttonGpio: number; + doubleTapAsButtonPress: boolean; + }>, +): Protobuf.Config.Config { + return create(Protobuf.Config.ConfigSchema, { + payloadVariant: { + case: "device", + value: create(Protobuf.Config.Config_DeviceConfigSchema, init as never), + }, + }); +} + +function mqttPacket( + init: Partial<{ enabled: boolean; address: string }>, +): Protobuf.ModuleConfig.ModuleConfig { + return create(Protobuf.ModuleConfig.ModuleConfigSchema, { + payloadVariant: { + case: "mqtt", + value: create(Protobuf.ModuleConfig.ModuleConfig_MQTTConfigSchema, init), + }, + }); +} + +describe("ConfigEditor — channels", () => { + it("transmits the uplinkEnabled flag the channel form staged", async () => { + const { client, editor, sent } = createHarness(); + client.events.onChannelPacket.dispatch(primaryChannel()); + + editor.setChannel(stagedChannel(true)); + expect(editor.dirtyChannels.value).toEqual([0]); + + expect((await editor.commit()).status).toBe("ok"); + + // `uplink_enabled` is field 5; `false` is not encoded at all, so reading + // it back off the wire is the only proof it was genuinely set. + expect(channelFromWire(sent)?.settings?.uplinkEnabled).toBe(true); + expect(summarise(sent)).toEqual([ + "beginEditSettings", + "setChannel", + "commitEditSettings", + ]); + expect(editor.dirtyChannels.value).toEqual([]); + expect(editor.isDirty.value).toBe(false); + }); + + it("keeps a channel edit staged during an in-flight commit pending", async () => { + const harness = createHarness(); + const { client, editor, sent } = harness; + + client.events.onChannelPacket.dispatch(primaryChannel()); + client.events.onModuleConfigPacket.dispatch(mqttPacket({ enabled: false })); + + // An unrelated module save is in flight... + editor.setModuleSection("mqtt", { + enabled: true, + address: "mqtt.example.org", + } as never); + harness.gateOn("commitEditSettings"); + const pending = editor.commit(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + // ...and the user flips the channel's uplink flag while it round-trips. + editor.setChannel(stagedChannel(true)); + expect(editor.dirtyChannels.value).toEqual([0]); + + harness.release(); + expect((await pending).status).toBe("ok"); + + // The channel was never part of that transaction, so it must stay dirty + // instead of being laundered as "saved". + expect(summarise(sent)).toEqual([ + "beginEditSettings", + "setModuleConfig:mqtt", + "commitEditSettings", + ]); + expect(channelFromWire(sent)).toBeUndefined(); + expect(editor.dirtyChannels.value).toEqual([0]); + expect(editor.isDirty.value).toBe(true); + + // And it goes out on the next commit with the flag intact. + expect((await editor.commit()).status).toBe("ok"); + expect(channelFromWire(sent)?.settings?.uplinkEnabled).toBe(true); + expect(editor.dirtyChannels.value).toEqual([]); + }); + + it("committing a channel leaves an unrelated staged section dirty", async () => { + const harness = createHarness(); + const { client, editor, sent } = harness; + + client.events.onChannelPacket.dispatch(primaryChannel()); + client.events.onConfigPacket.dispatch(devicePacket({ buttonGpio: 4 })); + + editor.setChannel(stagedChannel(true)); + harness.gateOn("beginEditSettings"); + const pending = editor.commit(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + // Staged after the payload was frozen — not part of this transaction. + editor.setRadioSection("device", { buttonGpio: 7 } as never); + + harness.release(); + expect((await pending).status).toBe("ok"); + + expect(summarise(sent)).toEqual([ + "beginEditSettings", + "setChannel", + "commitEditSettings", + ]); + expect(editor.dirtyChannels.value).toEqual([]); + expect(editor.dirtyRadioSections.value).toEqual(["device"]); + }); + + it("does not clear a staged channel when another section commits", async () => { + const { client, editor, sent } = createHarness(); + client.events.onChannelPacket.dispatch(primaryChannel()); + client.events.onModuleConfigPacket.dispatch(mqttPacket({ enabled: true })); + + editor.setChannel(stagedChannel(true)); + // A channel that cannot be transmitted (no working value) must not be + // marked synced by an unrelated successful commit either. + editor.setModuleSection("mqtt", undefined as never); + + expect((await editor.commit()).status).toBe("ok"); + + expect(summarise(sent)).toEqual([ + "beginEditSettings", + "setChannel", + "commitEditSettings", + ]); + expect(editor.dirtyChannels.value).toEqual([]); + expect(editor.dirtyModuleSections.value).toEqual(["mqtt"]); + }); + + it("preserves the channel sub-message the staged value omits", async () => { + const { client, editor, sent } = createHarness(); + client.events.onChannelPacket.dispatch( + primaryChannel({ isMuted: true, channelNum: 20 }), + ); + + // A `create()`d message materialises every singular scalar, so only the + // absent sub-message can be recovered — and it must be, or saving the + // uplink toggle would silently drop the channel's mute flag and location + // precision. + editor.setChannel( + create(Protobuf.Channel.ChannelSchema, { + index: 0, + role: Protobuf.Channel.Channel_Role.PRIMARY, + settings: { + channelNum: 20, + psk: new Uint8Array([1]), + id: 4242, + uplinkEnabled: true, + }, + }), + ); + + expect((await editor.commit()).status).toBe("ok"); + + const wire = channelFromWire(sent); + expect(wire?.settings?.uplinkEnabled).toBe(true); + expect(wire?.settings?.channelNum).toBe(20); + expect(wire?.settings?.id).toBe(4242); + expect(wire?.settings?.moduleSettings?.positionPrecision).toBe(13); + expect(wire?.settings?.moduleSettings?.isMuted).toBe(true); + }); + + it("preserves channel fields a partial staged object omits", async () => { + const { client, editor, sent } = createHarness(); + client.events.onChannelPacket.dispatch( + primaryChannel({ isMuted: true, channelNum: 20 }), + ); + + // A resolver output that only carries the fields its form declares. + editor.setChannel({ + index: 0, + role: Protobuf.Channel.Channel_Role.PRIMARY, + settings: { uplinkEnabled: true }, + } as never); + + expect((await editor.commit()).status).toBe("ok"); + + const wire = channelFromWire(sent); + expect(wire?.settings?.uplinkEnabled).toBe(true); + expect(wire?.settings?.channelNum).toBe(20); + expect(wire?.settings?.id).toBe(4242); + expect(wire?.settings?.psk).toEqual(new Uint8Array([1])); + expect(wire?.settings?.moduleSettings?.isMuted).toBe(true); + }); +}); + +describe("ConfigEditor — device config", () => { + it("transmits the device-config value the form staged", async () => { + const { client, editor, sent } = createHarness(); + client.events.onConfigPacket.dispatch( + devicePacket({ buttonGpio: 4, doubleTapAsButtonPress: false }), + ); + + editor.setRadioSection("device", { + buttonGpio: 4, + doubleTapAsButtonPress: true, + } as never); + expect(editor.dirtyRadioSections.value).toEqual(["device"]); + + expect((await editor.commit()).status).toBe("ok"); + + const wire = radioFromWire( + sent, + "device", + ); + expect(wire?.doubleTapAsButtonPress).toBe(true); + expect(wire?.buttonGpio).toBe(4); + expect(summarise(sent)).toEqual([ + "beginEditSettings", + "setConfig:device", + "commitEditSettings", + ]); + expect(editor.dirtyRadioSections.value).toEqual([]); + }); + + it("keeps a device edit staged during an in-flight commit pending", async () => { + const harness = createHarness(); + const { client, editor, sent } = harness; + + client.events.onConfigPacket.dispatch(devicePacket({ buttonGpio: 4 })); + client.events.onChannelPacket.dispatch(primaryChannel()); + + editor.setChannel(stagedChannel(true)); + harness.gateOn("commitEditSettings"); + const pending = editor.commit(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + editor.setRadioSection("device", { + buttonGpio: 4, + doubleTapAsButtonPress: true, + } as never); + + harness.release(); + expect((await pending).status).toBe("ok"); + + expect( + radioFromWire(sent, "device"), + ).toBeUndefined(); + expect(editor.dirtyRadioSections.value).toEqual(["device"]); + expect(editor.isDirty.value).toBe(true); + + expect((await editor.commit()).status).toBe("ok"); + expect( + radioFromWire(sent, "device") + ?.doubleTapAsButtonPress, + ).toBe(true); + expect(editor.dirtyRadioSections.value).toEqual([]); + }); + + it("committing device config leaves a staged channel dirty", async () => { + const harness = createHarness(); + const { client, editor, sent } = harness; + + client.events.onConfigPacket.dispatch(devicePacket({ buttonGpio: 4 })); + client.events.onChannelPacket.dispatch(primaryChannel()); + + editor.setRadioSection("device", { + buttonGpio: 4, + doubleTapAsButtonPress: true, + } as never); + harness.gateOn("beginEditSettings"); + const pending = editor.commit(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + editor.setChannel(stagedChannel(true)); + + harness.release(); + expect((await pending).status).toBe("ok"); + + expect(summarise(sent)).toEqual([ + "beginEditSettings", + "setConfig:device", + "commitEditSettings", + ]); + expect(editor.dirtyRadioSections.value).toEqual([]); + expect(editor.dirtyChannels.value).toEqual([0]); + }); + + it("preserves device-config fields the form does not declare", async () => { + const { client, editor, sent } = createHarness(); + client.events.onConfigPacket.dispatch( + devicePacket({ role: 2, buttonGpio: 4 }), + ); + + // Zod strips undeclared keys, so a form that never rendered `buttonGpio` + // used to reset it to 0 on the device on every save. + editor.setRadioSection("device", { + role: 2, + doubleTapAsButtonPress: true, + } as never); + + expect((await editor.commit()).status).toBe("ok"); + + const wire = radioFromWire( + sent, + "device", + ); + expect(wire?.doubleTapAsButtonPress).toBe(true); + expect(wire?.buttonGpio).toBe(4); + expect(wire?.role).toBe(2); + }); + + it("does not flag a section dirty over a field the form omits", () => { + const { client, editor } = createHarness(); + client.events.onConfigPacket.dispatch( + devicePacket({ role: 2, buttonGpio: 4 }), + ); + + // Re-saving without changing anything: the omitted `buttonGpio` must not + // read as a change, or the section stays "unsaved" forever. + editor.setRadioSection("device", { role: 2 } as never); + + expect(editor.dirtyRadioSections.value).toEqual([]); + expect(editor.isDirty.value).toBe(false); + }); +}); + +describe("ConfigEditor — module config", () => { + it("preserves module fields the form does not declare", async () => { + const { client, editor, sent } = createHarness(); + client.events.onModuleConfigPacket.dispatch( + mqttPacket({ enabled: false, address: "mqtt.example.org" }), + ); + + editor.setModuleSection("mqtt", { enabled: true } as never); + + expect((await editor.commit()).status).toBe("ok"); + + for (const admin of sent) { + if (admin.payloadVariant.case !== "setModuleConfig") continue; + const variant = admin.payloadVariant.value.payloadVariant; + if (variant.case !== "mqtt") continue; + expect(variant.value.enabled).toBe(true); + expect(variant.value.address).toBe("mqtt.example.org"); + } + }); +}); diff --git a/packages/sdk/src/features/config/domain/ConfigEditor.ts b/packages/sdk/src/features/config/domain/ConfigEditor.ts index e41f7bea1..bd43e3491 100644 --- a/packages/sdk/src/features/config/domain/ConfigEditor.ts +++ b/packages/sdk/src/features/config/domain/ConfigEditor.ts @@ -19,9 +19,38 @@ import { setModuleConfig, } from "../application/ConfigUseCases.ts"; import { ConfigMapper } from "../infrastructure/ConfigMapper.ts"; +import { configValuesEqual } from "./configEquality.ts"; +import { mergeStagedValue } from "./configMerge.ts"; import type { ModuleConfig, ModuleConfigSection } from "./ModuleConfig.ts"; import type { RadioConfig, RadioConfigSection } from "./RadioConfig.ts"; +/** + * Everything a single `commit()` will actually put on the wire, captured + * before the first `await` so later staging cannot silently join (or be + * mistaken for part of) the transaction. + */ +interface CommitPayload { + radio: Array< + [RadioConfigSection, NonNullable] + >; + modules: Array< + [ModuleConfigSection, NonNullable] + >; + channels: Array<[number, Protobuf.Channel.Channel]>; + owner: Protobuf.Mesh.User | undefined; + adminMessages: readonly Protobuf.Admin.AdminMessage[]; +} + +function isEmptyPayload(payload: CommitPayload): boolean { + return ( + payload.radio.length === 0 && + payload.modules.length === 0 && + payload.channels.length === 0 && + payload.owner === undefined && + payload.adminMessages.length === 0 + ); +} + /** * Per-section editor for radio config, module config, and channels. * @@ -182,7 +211,10 @@ export class ConfigEditor { } public setOwner(owner: Protobuf.Mesh.User): void { - this.workingOwner.value = owner; + this.workingOwner.value = mergeStagedValue( + this.baselineOwner.peek(), + owner, + ); this.recomputeDirty(); } @@ -204,7 +236,11 @@ export class ConfigEditor { key: K, value: RadioConfig[K], ): void { - this.workingRadio.value = { ...this.workingRadio.peek(), [key]: value }; + // Forms stage the output of their Zod resolver, which drops every field + // the form does not declare. Merge over the device's value so those + // fields are not silently reset to their protobuf defaults on commit. + const merged = mergeStagedValue(this.baselineRadio.peek()[key], value); + this.workingRadio.value = { ...this.workingRadio.peek(), [key]: merged }; this.recomputeDirty(); } @@ -212,13 +248,21 @@ export class ConfigEditor { key: K, value: ModuleConfig[K], ): void { - this.workingModules.value = { ...this.workingModules.peek(), [key]: value }; + const merged = mergeStagedValue(this.baselineModules.peek()[key], value); + this.workingModules.value = { + ...this.workingModules.peek(), + [key]: merged, + }; this.recomputeDirty(); } public setChannel(channel: Protobuf.Channel.Channel): void { + const merged = mergeStagedValue( + this.baselineChannels.peek().get(channel.index), + channel, + ); const next = new Map(this.workingChannels.peek()); - next.set(channel.index, channel); + next.set(merged.index, merged); this.workingChannels.value = next; this.recomputeDirty(); } @@ -231,52 +275,156 @@ export class ConfigEditor { this.recomputeDirty(); } + /** + * Snapshot of everything the *next* commit would transmit, taken + * synchronously so it cannot drift while the transaction is in flight. + * + * A dirty section with no working value is deliberately left out: it cannot + * be transmitted, so it must also not be treated as synced afterwards. + */ + 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; + radioPayload.push([variant, value]); + } + + const modules = this.workingModules.peek(); + const modulePayload: CommitPayload["modules"] = []; + for (const variant of this._dirtyModuleSections.peek()) { + const value = modules[variant]; + if (value === undefined) continue; + modulePayload.push([variant, value]); + } + + const channels = this.workingChannels.peek(); + const channelPayload: CommitPayload["channels"] = []; + for (const index of this._dirtyChannels.peek()) { + const channel = channels.get(index); + if (!channel) continue; + channelPayload.push([index, channel]); + } + + const owner = this._isOwnerDirty.peek() + ? this.workingOwner.peek() + : undefined; + + return { + radio: radioPayload, + modules: modulePayload, + channels: channelPayload, + owner, + adminMessages: this.queuedAdminMessages.peek().slice(), + }; + } + + /** + * Promote exactly the values that were transmitted into the baseline. + * + * Every setter replaces the section object wholesale, so reference identity + * is an exact test for "the user has not touched this since we sent it". A + * section that was re-staged mid-flight (or was never part of the payload at + * all) keeps its old baseline and therefore stays dirty, so the edit goes + * out on the next commit instead of being silently discarded. + */ + private reconcileAfterCommit(payload: CommitPayload): void { + if (payload.radio.length > 0) { + const current = this.workingRadio.peek(); + const baseline: Record = { + ...this.baselineRadio.peek(), + }; + for (const [variant, value] of payload.radio) { + if (current[variant] === value) baseline[variant] = value; + } + this.baselineRadio.value = baseline as RadioConfig; + } + + if (payload.modules.length > 0) { + const current = this.workingModules.peek(); + const baseline: Record = { + ...this.baselineModules.peek(), + }; + for (const [variant, value] of payload.modules) { + if (current[variant] === value) baseline[variant] = value; + } + this.baselineModules.value = baseline as ModuleConfig; + } + + if (payload.channels.length > 0) { + const current = this.workingChannels.peek(); + const baseline = new Map(this.baselineChannels.peek()); + for (const [index, channel] of payload.channels) { + if (current.get(index) === channel) baseline.set(index, channel); + } + this.baselineChannels.value = baseline; + } + + if ( + payload.owner !== undefined && + this.workingOwner.peek() === payload.owner + ) { + this.baselineOwner.value = payload.owner; + } + + if (payload.adminMessages.length > 0) { + const sent = new Set(payload.adminMessages); + this.queuedAdminMessages.value = this.queuedAdminMessages + .peek() + .filter((message) => !sent.has(message)); + } + + // Recompute rather than blanket-clearing: anything staged while the + // transaction was in flight still differs from the baseline and stays + // flagged as pending. + this.recomputeDirty(); + } + /** * Send all dirty sections to the device wrapped in a beginEdit/commitEdit - * pair. On success the baseline is replaced with the working copy - * (optimistic update); inbound config packets after commit will reconcile. + * pair. On success only the sections that were actually transmitted are + * promoted into the baseline; edits staged while the commit was in flight + * remain dirty and go out on the next commit. */ public async commit(): Promise> { if (!this._isDirty.peek()) return Result.ok(undefined); + // Freeze what this transaction carries before the first await. + const payload = this.collectPayload(); + if (isEmptyPayload(payload)) { + // Opening a beginEdit/commitEdit pair with nothing inside makes the + // device rewrite its config file unchanged and report success — the + // firmware-side signature of a "saved" change that was never sent. + return Result.ok(undefined); + } + 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()) { - const value = radio[variant]; - if (value === undefined) continue; + for (const [variant, value] of payload.radio) { const config = buildRadioConfig(variant, value); const result = await setConfig(this.client, config); if (Result.isError(result)) return Result.err(result.error); } - const modules = this.workingModules.peek(); - for (const variant of this._dirtyModuleSections.peek()) { - const value = modules[variant]; - if (value === undefined) continue; + for (const [variant, value] of payload.modules) { const moduleConfig = buildModuleConfig(variant, value); const result = await setModuleConfig(this.client, moduleConfig); if (Result.isError(result)) return Result.err(result.error); } - const channels = this.workingChannels.peek(); - for (const index of this._dirtyChannels.peek()) { - const channel = channels.get(index); - if (!channel) continue; + for (const [, channel] of payload.channels) { 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 (payload.owner) { + const result = await setOwner(this.client, payload.owner); + if (Result.isError(result)) return Result.err(result.error); } - for (const message of this.queuedAdminMessages.peek()) { + for (const message of payload.adminMessages) { const variant = message.payloadVariant; if (!variant.case) continue; try { @@ -289,16 +437,7 @@ 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._dirtyRadioSections.value = []; - this._dirtyModuleSections.value = []; - this._dirtyChannels.value = []; - this._isOwnerDirty.value = false; - this._isDirty.value = false; + this.reconcileAfterCommit(payload); return Result.ok(undefined); } @@ -313,7 +452,7 @@ export class ConfigEditor { ]); for (const key of radioKeys) { if ( - !shallowEqual( + !configValuesEqual( radioBase[key as keyof RadioConfig], radioWorking[key as keyof RadioConfig], ) @@ -331,7 +470,7 @@ export class ConfigEditor { ]); for (const key of moduleKeys) { if ( - !shallowEqual( + !configValuesEqual( moduleBase[key as keyof ModuleConfig], moduleWorking[key as keyof ModuleConfig], ) @@ -348,12 +487,12 @@ export class ConfigEditor { ...channelWorking.keys(), ]); for (const idx of channelKeys) { - if (!shallowEqual(channelBase.get(idx), channelWorking.get(idx))) { + if (!configValuesEqual(channelBase.get(idx), channelWorking.get(idx))) { channelDirty.push(idx); } } - const ownerDirty = !shallowEqual( + const ownerDirty = !configValuesEqual( this.baselineOwner.peek(), this.workingOwner.peek(), ); @@ -395,26 +534,3 @@ function buildModuleConfig( } as Protobuf.ModuleConfig.ModuleConfig["payloadVariant"], }); } - -function shallowEqual(a: unknown, b: unknown): boolean { - if (a === b) return true; - if (a === undefined || b === undefined) return false; - if (a === null || b === null) return false; - if (typeof a !== "object" || typeof b !== "object") return false; - const ao = a as Record; - const bo = b as Record; - const aKeys = Object.keys(ao); - const bKeys = Object.keys(bo); - if (aKeys.length !== bKeys.length) return false; - for (const k of aKeys) { - const av = ao[k]; - const bv = bo[k]; - if (av === bv) continue; - if (typeof av === "object" && typeof bv === "object") { - if (!shallowEqual(av, bv)) return false; - } else { - return false; - } - } - return true; -} diff --git a/packages/sdk/src/features/config/domain/configEquality.test.ts b/packages/sdk/src/features/config/domain/configEquality.test.ts new file mode 100644 index 000000000..e657ec801 --- /dev/null +++ b/packages/sdk/src/features/config/domain/configEquality.test.ts @@ -0,0 +1,82 @@ +import { create } from "@bufbuild/protobuf"; +import * as Protobuf from "@meshtastic/protobufs"; +import { describe, expect, it } from "vitest"; +import { configValuesEqual } from "./configEquality.ts"; + +describe("configValuesEqual", () => { + it("treats an absent field as its protobuf default", () => { + expect(configValuesEqual({ enabled: false }, {})).toBe(true); + expect(configValuesEqual({ root: "" }, {})).toBe(true); + expect(configValuesEqual({ publishIntervalSecs: 0 }, {})).toBe(true); + expect(configValuesEqual({ psk: new Uint8Array() }, {})).toBe(true); + }); + + it("never treats an absent field as a set boolean", () => { + expect(configValuesEqual({ enabled: true }, {})).toBe(false); + expect(configValuesEqual({}, { enabled: true })).toBe(false); + }); + + it("detects a boolean flip regardless of which side carries the key", () => { + expect(configValuesEqual({ enabled: false }, { enabled: true })).toBe( + false, + ); + // Key sets differ but have the same size — the old Object.keys(a)-only + // walk could skip the key that is missing from `a`. + expect( + configValuesEqual( + { address: "m", root: "msh", username: "u" }, + { address: "m", root: "msh", enabled: true }, + ), + ).toBe(false); + }); + + it("ignores the protobuf $typeName marker", () => { + const message = create( + Protobuf.ModuleConfig.ModuleConfig_MQTTConfigSchema, + { enabled: true, address: "mqtt.example.org" }, + ); + // What a Zod-parsed web form stages: same values, no $typeName, and the + // untouched zero-valued fields are simply absent. + expect( + configValuesEqual(message, { + enabled: true, + address: "mqtt.example.org", + }), + ).toBe(true); + expect( + configValuesEqual(message, { + enabled: false, + address: "mqtt.example.org", + }), + ).toBe(false); + }); + + it("treats an absent sub-message as an all-defaults sub-message", () => { + expect( + configValuesEqual( + { mapReportSettings: { publishIntervalSecs: 0, positionPrecision: 0 } }, + {}, + ), + ).toBe(true); + expect( + configValuesEqual({ mapReportSettings: { positionPrecision: 13 } }, {}), + ).toBe(false); + }); + + it("compares bytes and repeated fields by value", () => { + expect( + configValuesEqual( + { psk: new Uint8Array([1, 2, 3]) }, + { psk: new Uint8Array([1, 2, 3]) }, + ), + ).toBe(true); + expect( + configValuesEqual( + { psk: new Uint8Array([1, 2, 3]) }, + { psk: new Uint8Array([1, 2, 4]) }, + ), + ).toBe(false); + expect(configValuesEqual({ list: [1, 2] }, { list: [1, 2] })).toBe(true); + expect(configValuesEqual({ list: [1, 2] }, { list: [2, 1] })).toBe(false); + }); +}); diff --git a/packages/sdk/src/features/config/domain/configEquality.ts b/packages/sdk/src/features/config/domain/configEquality.ts new file mode 100644 index 000000000..4edb01771 --- /dev/null +++ b/packages/sdk/src/features/config/domain/configEquality.ts @@ -0,0 +1,140 @@ +/** + * Value-equality used by {@link ConfigEditor} to decide whether a config + * section still matches the device's baseline. + * + * This has to compare two things that are *never* structurally identical even + * when they mean exactly the same thing: + * + * - the **baseline**, which is a `@bufbuild/protobuf` message: it carries a + * `$typeName` marker and every singular scalar field is materialised as an + * own property holding its zero value (`false` / `0` / `""`), and + * - the **working copy**, which is whatever the UI staged. Web forms hand over + * plain objects produced by a Zod resolver, so they have no `$typeName`, and + * fields the form does not render are simply absent. + * + * A naive key-count/`Object.keys(a)` comparison therefore reports "changed" + * forever, which pins the working copy (the editor refuses to let inbound + * device config overwrite a section it believes is dirty) and makes the UI + * report stale values as if they were saved. It also gets `undefined` vs + * `false` wrong in both directions. + * + * The rules implemented here mirror protobuf's own semantics: + * + * - `$typeName` is metadata, not data. + * - Keys are compared over the *union* of both sides, so a field that only one + * side carries is never skipped. + * - An absent field is equal to that field's protobuf default + * (`undefined` == `false` / `0` / `0n` / `""` / empty bytes / empty list), + * and *only* to its default — an absent field is never equal to `true`. + * - An absent sub-message is equal to a sub-message whose fields are all + * defaults. + * - `Uint8Array` is compared byte-wise, repeated fields element-wise. + */ + +function isAbsent(value: unknown): boolean { + return value === undefined || value === null; +} + +/** True when `value` is the protobuf zero value for its type (or absent). */ +function isProtobufDefault(value: unknown): boolean { + if (isAbsent(value)) { + return true; + } + if (value instanceof Uint8Array) { + return value.byteLength === 0; + } + if (Array.isArray(value)) { + return value.length === 0; + } + if (typeof value === "object") { + return Object.entries(value as Record).every( + ([key, entry]) => key === "$typeName" || isProtobufDefault(entry), + ); + } + return value === false || value === 0 || value === 0n || value === ""; +} + +function bytesEqual(a: Uint8Array, b: Uint8Array): boolean { + if (a.byteLength !== b.byteLength) { + return false; + } + for (let i = 0; i < a.byteLength; i++) { + if (a[i] !== b[i]) { + return false; + } + } + return true; +} + +function isRecord(value: unknown): value is Record { + return ( + typeof value === "object" && + value !== null && + !Array.isArray(value) && + !(value instanceof Uint8Array) + ); +} + +const EMPTY_RECORD: Record = {}; + +export function configValuesEqual(a: unknown, b: unknown): boolean { + if (a === b) { + return true; + } + + if (a instanceof Uint8Array || b instanceof Uint8Array) { + if (a instanceof Uint8Array && b instanceof Uint8Array) { + return bytesEqual(a, b); + } + // One side is absent: equal only if the present side is empty bytes. + const present = a instanceof Uint8Array ? a : (b as Uint8Array); + const other = a instanceof Uint8Array ? b : a; + return present.byteLength === 0 && isAbsent(other); + } + + if (Array.isArray(a) || Array.isArray(b)) { + if (!Array.isArray(a) && !isAbsent(a)) { + return false; + } + if (!Array.isArray(b) && !isAbsent(b)) { + return false; + } + const left = Array.isArray(a) ? a : []; + const right = Array.isArray(b) ? b : []; + if (left.length !== right.length) { + return false; + } + return left.every((value, index) => configValuesEqual(value, right[index])); + } + + if (isRecord(a) || isRecord(b)) { + if (!isRecord(a) && !isAbsent(a)) { + return false; + } + if (!isRecord(b) && !isAbsent(b)) { + return false; + } + const left = isRecord(a) ? a : EMPTY_RECORD; + const right = isRecord(b) ? b : EMPTY_RECORD; + const keys = new Set([...Object.keys(left), ...Object.keys(right)]); + for (const key of keys) { + if (key === "$typeName") { + continue; + } + if (!configValuesEqual(left[key], right[key])) { + return false; + } + } + return true; + } + + // Scalars. An absent field carries its protobuf default, so `undefined` + // equals `false`/`0`/`""` — but never `true`, `1`, or a non-empty string. + if (isAbsent(a)) { + return isProtobufDefault(b); + } + if (isAbsent(b)) { + return isProtobufDefault(a); + } + return false; +} diff --git a/packages/sdk/src/features/config/domain/configMerge.test.ts b/packages/sdk/src/features/config/domain/configMerge.test.ts new file mode 100644 index 000000000..a1798970e --- /dev/null +++ b/packages/sdk/src/features/config/domain/configMerge.test.ts @@ -0,0 +1,77 @@ +import { describe, expect, it } from "vitest"; +import { mergeStagedValue } from "./configMerge.ts"; + +describe("mergeStagedValue", () => { + it("fills in fields the staged value never declared", () => { + const merged = mergeStagedValue( + { $typeName: "meshtastic.Config.DeviceConfig", role: 2, buttonGpio: 4 }, + { role: 2 }, + ); + expect(merged).toEqual({ role: 2, buttonGpio: 4 }); + }); + + it("never overrides a field the form did send", () => { + const merged = mergeStagedValue( + { enabled: true, address: "old", root: "msh", tlsEnabled: true }, + { enabled: false, address: "", tlsEnabled: false }, + ); + // `false` and `""` are real edits, not absences. + expect(merged).toEqual({ + enabled: false, + address: "", + tlsEnabled: false, + root: "msh", + }); + }); + + it("merges sub-messages field by field", () => { + const merged = mergeStagedValue( + { + moduleSettings: { positionPrecision: 13, isMuted: true }, + psk: new Uint8Array([1]), + }, + { moduleSettings: { positionPrecision: 32 } }, + ); + expect(merged).toEqual({ + moduleSettings: { positionPrecision: 32, isMuted: true }, + psk: new Uint8Array([1]), + }); + }); + + it("treats arrays and byte fields as values, not as things to merge", () => { + const merged = mergeStagedValue( + { ignoreIncoming: [1, 2, 3], psk: new Uint8Array([9, 9]) }, + { ignoreIncoming: [], psk: new Uint8Array([]) }, + ); + // Clearing a list or a key must reach the device. + expect(merged).toEqual({ + ignoreIncoming: [], + psk: new Uint8Array([]), + }); + }); + + it("is a no-op when there is no baseline", () => { + const staged = { enabled: true }; + expect(mergeStagedValue(undefined, staged)).toBe(staged); + expect(mergeStagedValue({}, staged)).toBe(staged); + }); + + it("is a no-op for a fully materialised protobuf message", () => { + const staged = { + $typeName: "meshtastic.Config.DeviceConfig", + role: 0, + buttonGpio: 0, + }; + expect( + mergeStagedValue( + { $typeName: "meshtastic.Config.DeviceConfig", role: 2, buttonGpio: 4 }, + staged, + ), + ).toBe(staged); + }); + + it("leaves non-object values alone", () => { + expect(mergeStagedValue({ a: 1 }, undefined)).toBeUndefined(); + expect(mergeStagedValue(5, 7)).toBe(7); + }); +}); diff --git a/packages/sdk/src/features/config/domain/configMerge.ts b/packages/sdk/src/features/config/domain/configMerge.ts new file mode 100644 index 000000000..36622bc6d --- /dev/null +++ b/packages/sdk/src/features/config/domain/configMerge.ts @@ -0,0 +1,78 @@ +/** + * Merge a staged (UI-produced) config value over the value the device last + * reported, so that a partial form submission can never silently reset fields + * the form does not manage. + * + * Why this is needed + * ------------------ + * Every settings form in the web app hands {@link ConfigEditor} the *output of + * its Zod resolver*, and `commit()` feeds that object straight into + * `create(SomeSchema, value)`. Zod object schemas strip keys they do not + * declare, so any protobuf field the form does not list simply disappears from + * the staged object — and `create()` then materialises it at its protobuf + * default. Saving one toggle therefore silently rewrites every unlisted field + * on the device to `false` / `0` / `""`. + * + * The same gap also produced phantom dirty state: a section whose baseline + * carries a non-default value that the form omits never compares equal to the + * staged object, so it stayed flagged as "unsaved" forever. + * + * The rule implemented here is deliberately narrow: + * + * - Only keys that are **absent** (`undefined`) in the staged value and + * **present** in the baseline are filled in. A key the form did send always + * wins, including `false`, `0`, `""` and `[]` — clearing a list or turning a + * toggle off must still reach the device. + * - Recursion happens only where both sides are plain records, so protobuf + * sub-messages (`mapReportSettings`, `moduleSettings`, `ipv4Config`, …) are + * merged field-by-field instead of being replaced wholesale. + * - `Uint8Array` and arrays are values, never merged element-wise. + * - When the staged value is already a full protobuf message every singular + * field is materialised, so this is a no-op — staging a `create()`d message + * (channels, the LoRa import path, …) behaves exactly as before. + */ + +function isMergeableRecord(value: unknown): value is Record { + return ( + typeof value === "object" && + value !== null && + !Array.isArray(value) && + !(value instanceof Uint8Array) + ); +} + +export function mergeStagedValue(baseline: unknown, staged: T): T { + if (!isMergeableRecord(staged) || !isMergeableRecord(baseline)) { + return staged; + } + + let merged: Record | undefined; + const write = (key: string, value: unknown): void => { + merged ??= { ...(staged as Record) }; + merged[key] = value; + }; + + for (const [key, baseValue] of Object.entries(baseline)) { + if (key === "$typeName") { + continue; + } + const stagedValue = (staged as Record)[key]; + + if (stagedValue === undefined) { + // The form never declared this field — keep what the device reported. + if (baseValue !== undefined) { + write(key, baseValue); + } + continue; + } + + if (isMergeableRecord(stagedValue) && isMergeableRecord(baseValue)) { + const mergedChild = mergeStagedValue(baseValue, stagedValue); + if (mergedChild !== stagedValue) { + write(key, mergedChild); + } + } + } + + return (merged ?? staged) as T; +}