From d4ef873a8219e02bc07591a2c728e217208f279c Mon Sep 17 00:00:00 2001 From: Harted Date: Wed, 16 Sep 2026 15:01:39 +0200 Subject: [PATCH 01/19] chore: the audit skill names twelve conformance rules, not nine grep -c '^describe(' src/__tests__/conformance.test.ts returns 12. At nine, an auditor reports three rules the suite already asserts. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- .claude/skills/audit/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/skills/audit/SKILL.md b/.claude/skills/audit/SKILL.md index 47877ed4..51aee187 100644 --- a/.claude/skills/audit/SKILL.md +++ b/.claude/skills/audit/SKILL.md @@ -26,7 +26,7 @@ Split by what the code shares, not by directory size. One area per run. Read `CLAUDE.md`, `CONTRIBUTING.md` and `src/__tests__/conformance.test.ts`. -The suite already asserts nine conventions. **Do not report what it asserts** — +The suite already asserts twelve conventions. **Do not report what it asserts** — it is green, so those are closed. Audit what a test cannot see. ## The eight criteria From c35d807860f72b8c142e1a891c6f1bb52cfaa495 Mon Sep 17 00:00:00 2001 From: Harted Date: Wed, 16 Sep 2026 15:48:11 +0200 Subject: [PATCH 02/19] fix: the split out server window no longer drives main's client `App.tsx` imports `containers/Client` statically and `Client.tsx` imports `client.zustand`, so `out/renderer/assets/` holds one js file and both windows evaluate that module scope. It called main five times from whichever window ran it, and `set_read_configuration(false)` is the one that costs: `modbusClient.ts` reads that flag, and off it polls one flat `[address, length]` block instead of the mapping's groups, while the main window's toggle still reads on. `init`'s own `set` also wrote `CLIENT_ZUSTAND_STORAGE_KEY` from the split window, because persist wraps `setState`. Closed from both sides. `client.zustand.ts` gates its module tail on `isServerWindow`, the question `layout.zustand.ts` and `server.zustand.ts` already ask. And `createIpcHandle` refuses the fourteen channels that drive main's one Modbus client when they come from a window that is not `windows.main`, so a caller added to that module later, or a client component mounted in the server window, is refused rather than served. `list_serial_ports` and `validate_serial_port` stay open, because they enumerate hardware and the RTU server's COM field reads the first of the two from the server window. The version fetch stays where it is. `get_app_version` asks `app` rather than the client, and `containers/Home.tsx` is the only reader of what it answers, which `App.tsx` does not draw in that window. So S-16 was not the precondition the cluster document called it. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- CHANGELOG.md | 5 ++ .../31-split-view-client-state.spec.ts | 78 +++++++++++++++++++ src/main/__tests__/ipc.test.ts | 76 +++++++++++++++++- src/main/ipc.ts | 65 +++++++++++++++- src/main/windows.ts | 9 +++ .../clientStoreInServerWindow.test.ts | 62 +++++++++++++++ .../src/context/__tests__/stubRenderer.ts | 14 +++- src/renderer/src/context/client.zustand.ts | 23 +++++- 8 files changed, 320 insertions(+), 12 deletions(-) create mode 100644 e2e/specs/01-main/31-split-view-client-state.spec.ts create mode 100644 src/renderer/src/context/__tests__/clientStoreInServerWindow.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index fe749986..7ed0c79f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Splitting the server into its own window no longer turns Read + Configuration off.** The toggle stayed on while the reading went back to the + address and length in the toolbar, so every configured register above the + tenth stopped being read and its value left the grid. The split out window now + leaves the client alone. - **A DATETIME write now carries the date you typed.** The write dialog took a value field whose range was stated in seconds while the register is encoded from milliseconds, so every value it accepted fell before the year 2000 and diff --git a/e2e/specs/01-main/31-split-view-client-state.spec.ts b/e2e/specs/01-main/31-split-view-client-state.spec.ts new file mode 100644 index 00000000..13b70a34 --- /dev/null +++ b/e2e/specs/01-main/31-split-view-client-state.spec.ts @@ -0,0 +1,78 @@ +import { test, expect } from '../../fixtures/electron-app' +import { + navigateToClient, + navigateToHome, + connectClient, + disconnectClient, + enableAdvancedMode, + enableReadConfiguration, + disableReadConfiguration, + cleanServerState, + loadServerConfig, + loadClientConfig, + selectRegisterType, + splitOutServerWindow, + expectCell +} from '../../fixtures/helpers' +import { resolve } from 'path' +import { type Page } from '@playwright/test' + +const CONFIG_DIR = resolve(__dirname, '../../fixtures/config-files') +const SERVER_CONFIG = resolve(CONFIG_DIR, 'server-large-config.json') +const CLIENT_CONFIG = resolve(CONFIG_DIR, 'client-large-config.json') + +let serverPage: Page + +// Both windows load the one renderer bundle, so both evaluate +// `client.zustand`'s module scope, and `init` there called +// `setReadConfiguration(false)` with no addressee. The main window's toggle +// still read on, and the next read went out as one flat `[address, length]` +// block instead of the mapping's groups: the length field holds 10, so every +// configured register above address 9 stopped being read. Only two windows can +// see it. +test.describe.serial('Read configuration survives the split out server window', () => { + test.afterAll(async ({ electronApp }) => { + await electronApp.evaluate(({ BrowserWindow }) => { + BrowserWindow.getAllWindows() + .filter((w) => w.getTitle() === 'Server') + .forEach((w) => w.close()) + }) + }) + + test('clean server state', async ({ mainPage }) => { + await cleanServerState(mainPage) + }) + + test('load 120 holding registers into the server and the matching mapping', async ({ + mainPage + }) => { + await loadServerConfig(mainPage, SERVER_CONFIG) + await navigateToClient(mainPage) + await enableAdvancedMode(mainPage) + await selectRegisterType(mainPage, 'Holding Registers') + await loadClientConfig(mainPage, CLIENT_CONFIG) + }) + + test('connected, read configuration fills every configured register', async ({ mainPage }) => { + await connectClient(mainPage, '127.0.0.1', '502', '0') + await enableReadConfiguration(mainPage) + + await expectCell(mainPage, 0, 'word_uint16', '1') + await expectCell(mainPage, 50, 'word_uint16', '51') + }) + + test('the split out window leaves the mapping reading', async ({ electronApp, mainPage }) => { + await navigateToHome(mainPage) + serverPage = await splitOutServerWindow(electronApp, mainPage) + await expect(serverPage.getByTestId('section-holding_registers')).toBeVisible() + + await mainPage.getByTestId('read-btn').click() + + await expectCell(mainPage, 50, 'word_uint16', '51') + }) + + test('cleanup', async ({ mainPage }) => { + await disableReadConfiguration(mainPage) + await disconnectClient(mainPage) + }) +}) diff --git a/src/main/__tests__/ipc.test.ts b/src/main/__tests__/ipc.test.ts index 812bdb6c..965209bc 100644 --- a/src/main/__tests__/ipc.test.ts +++ b/src/main/__tests__/ipc.test.ts @@ -36,18 +36,25 @@ interface SentMessage { message: BackendMessage } +/** Stands in for the `WebContents` that `ipcMain.handle` hands every listener. */ +const SENDER = { id: 'the window that asked' } + +/** + * `windows`, with `SENDER` standing in for the main window's contents. + * + * `isMain` is what the client channels are refused on, so a stub answering + * false for everything would refuse the fourteen of them in every test here. + */ const createWindows = (): { windows: Windows; sent: SentMessage[] } => { const sent: SentMessage[] = [] const windows = { send: (_event: string, message: BackendMessage, to: unknown = 'all') => - sent.push({ to, message }) + sent.push({ to, message }), + isMain: (contents: unknown) => contents === SENDER } as unknown as Windows return { windows, sent } } -/** Stands in for the `WebContents` that `ipcMain.handle` hands every listener. */ -const SENDER = { id: 'the window that asked' } - /** Invokes the listener that was registered for `channel`. */ const invoke = async ( channel: string, @@ -172,6 +179,67 @@ describe('createIpcHandle', () => { ipcHandle('get_privileged_port_status', vi.fn(), PortSchema) }) + // `main/index.ts:23` constructs one `ModbusClient` and none of these channels + // carries an addressee, so before this any window could aim them at it. The + // split out server window did, from `client.zustand`'s module scope. + it('refuses a client channel from a window that is not the main one', async () => { + const { windows, sent } = createWindows() + const ipcHandle = createIpcHandle(windows) + const listener = vi.fn() + const serverWindow = { id: 'the split out server window' } + + ipcHandle('set_read_configuration', listener) + const returned = await invoke('set_read_configuration', false, serverWindow) + + expect(listener).not.toHaveBeenCalled() + expect(returned).toBeUndefined() + expect(sent).toHaveLength(1) + expect(sent[0]?.to).toBe(serverWindow) + expect(String(sent[0]?.message.error)).toContain('set_read_configuration') + }) + + it('refuses a guarded client channel before it reads the payload', async () => { + const { windows, sent } = createWindows() + const ipcHandle = createIpcHandle(windows) + const listener = vi.fn() + + ipcHandle('scan_unit_ids', listener, ScanUnitIDParametersSchema) + await invoke( + 'scan_unit_ids', + { range: [1, 5], address: 0, length: 2, registerTypes: ['holding_registers'], timeout: 500 }, + { id: 'the split out server window' } + ) + + expect(listener).not.toHaveBeenCalled() + expect(String(sent[0]?.message.error)).toContain('not the main one') + }) + + it('takes the same channel from the main window', async () => { + const { windows, sent } = createWindows() + const ipcHandle = createIpcHandle(windows) + const listener = vi.fn() + + ipcHandle('set_read_configuration', listener) + await invoke('set_read_configuration', true) + + expect(listener).toHaveBeenCalledWith({ sender: SENDER }, true) + expect(sent).toEqual([]) + }) + + // The RTU server's COM field reads the port list from the server window, so a + // rule over every channel main's client happens to own would break it. + it('takes serial discovery from either window', async () => { + const { windows, sent } = createWindows() + const ipcHandle = createIpcHandle(windows) + const listener = vi.fn() + + ipcHandle('list_serial_ports', listener) + await invoke('list_serial_ports', undefined, { id: 'the split out server window' }) + + expect(listener).toHaveBeenCalledOnce() + expect(sent).toEqual([]) + }) + it('accepts one where the answer admits undefined', () => { const { windows } = createWindows() const ipcHandle = createIpcHandle(windows) diff --git a/src/main/ipc.ts b/src/main/ipc.ts index d0f51d0b..e3888c49 100644 --- a/src/main/ipc.ts +++ b/src/main/ipc.ts @@ -51,6 +51,47 @@ type PayloadSchema = ? ZodType : never +/** A channel a refusal has an answer for, which is the same rule as above. */ +type RefusableChannel = { + [C in keyof IpcHandlerMap]: undefined extends Awaited ? C : never +}[keyof IpcHandlerMap] + +/** + * The channels that drive main's one Modbus client, answered for `windows.main` + * alone. + * + * `main/index.ts` constructs one `ModbusClient`, and not one of these channels + * carries an addressee, so any window could aim them at it. Both windows load + * the same renderer bundle, so both hold the client store; the near-term cost + * was its module scope calling `set_read_configuration` and + * `stop_scanning_unit_ids` from the split out server window, which + * `client.zustand.ts` now gates on `isServerWindow`. This side is the rule + * rather than the instance: a caller added to that module later, or a client + * component mounted in the server window, reaches the same single client. + * + * `list_serial_ports` and `validate_serial_port` are not here. They enumerate + * hardware rather than touch the client, and `server.zustand.ts:467` calls the + * first of the two for the RTU server's COM field. `get_client_state` is not + * here either: it answers a `ClientState` rather than `undefined`, and reading + * what main is doing changes nothing about it. + */ +const CLIENT_CHANNELS: readonly RefusableChannel[] = [ + 'connect', + 'disconnect', + 'read', + 'start_polling', + 'stop_polling', + 'write', + 'scan_unit_ids', + 'stop_scanning_unit_ids', + 'scan_registers', + 'stop_scanning_registers', + 'update_connection_config', + 'update_register_config', + 'set_register_mapping', + 'set_read_configuration' +] + /** * Builds the `ipcHandle` used below, bound to the windows it reports through. * @@ -67,6 +108,10 @@ type PayloadSchema = * windows were listening, which in split view was the main one, so a payload * refused on a channel the server window owns reported into a window the user * was not looking at. + * + * A channel in `CLIENT_CHANNELS` is refused the same way when it comes from a + * window that is not `windows.main`, and that question is asked before the + * schema, because the payload is beside the point once the asker is wrong. */ export const createIpcHandle = (windows: Windows) => @@ -75,12 +120,24 @@ export const createIpcHandle = listener: IpcListener, schema?: PayloadSchema ): void => { - if (!schema) { - ipcMain.handle(channel, listener) - return - } + const drivesTheClient = (CLIENT_CHANNELS as readonly string[]).includes(channel) ipcMain.handle(channel, (event: IpcMainInvokeEvent, ...args: unknown[]) => { + if (drivesTheClient && !windows.isMain(event.sender)) { + windows.send( + 'backend_message', + { + message: 'The Modbus client is driven from the main window', + variant: 'error', + error: `${channel}: refused, the asking window is not the main one` + }, + event.sender + ) + return undefined + } + + if (!schema) return listener(event, ...(args as IpcHandlerMap[C]['args'])) + const result = schema.safeParse(args[0]) if (!result.success) { diff --git a/src/main/windows.ts b/src/main/windows.ts index 51051e56..06dc31e2 100644 --- a/src/main/windows.ts +++ b/src/main/windows.ts @@ -72,6 +72,15 @@ export class Windows { return [to] } + /** + * Whether `contents` is the main window's, which is where client work happens. + * + * Reuses the same liveness question `send` asks, so a main window that is + * gone or destroyed answers false rather than throwing on `webContents`. + */ + public isMain = (contents: WebContents): boolean => + this._liveContents(this._windows.main).includes(contents) + private _liveContents(window: BrowserWindow | null): WebContents[] { if (!window || window.isDestroyed() || !window.webContents) return [] return [window.webContents] diff --git a/src/renderer/src/context/__tests__/clientStoreInServerWindow.test.ts b/src/renderer/src/context/__tests__/clientStoreInServerWindow.test.ts new file mode 100644 index 00000000..f8af07ad --- /dev/null +++ b/src/renderer/src/context/__tests__/clientStoreInServerWindow.test.ts @@ -0,0 +1,62 @@ +// @vitest-environment happy-dom +// +// `App.tsx` imports `containers/Client` statically and `Client.tsx:11` imports +// `client.zustand`, so `out/renderer/assets/` holds one js file and both windows +// evaluate that module scope. It called main five times from whichever window +// ran it, and `set_read_configuration(false)` is the one that costs: with the +// flag off, `modbusClient.ts:586` polls one flat `[address, length]` block +// instead of the configured groups, while the main window's toggle reads on. +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { CLIENT_ZUSTAND_STORAGE_KEY } from '@shared' +import { ApiCall, recordApiCalls, stubRenderer } from './stubRenderer' + +const calls: ApiCall[] = [] + +/** Waits out `init`'s microtasks, so a call it makes is recorded before the read. */ +const settle = async (): Promise => { + await Promise.resolve() + await Promise.resolve() +} + +beforeEach(() => { + vi.resetModules() + localStorage.clear() + calls.length = 0 +}) + +describe('client.zustand in the split out server window', () => { + it('tells main nothing at module scope', async () => { + stubRenderer({ isServerWindow: true }) + recordApiCalls(calls) + + await import('../client.zustand') + await settle() + + expect(calls.map(({ method }) => method)).toEqual(['getAppVersion']) + }) + + // persist wraps `setState`, so `init`'s own `set` wrote the whole partialized + // state under the shared key on every open of the split out window. + it('does not write the shared storage key', async () => { + stubRenderer({ isServerWindow: true }) + + await import('../client.zustand') + await settle() + + expect(localStorage.getItem(CLIENT_ZUSTAND_STORAGE_KEY)).toBeNull() + }) + + it('still calls main from the main window', async () => { + stubRenderer() + recordApiCalls(calls) + + await import('../client.zustand') + await settle() + + const methods = calls.map(({ method }) => method) + expect(methods).toContain('updateConnectionConfig') + expect(methods).toContain('updateRegisterConfig') + expect(methods).toContain('stopScanningUnitIds') + expect(calls.find(({ method }) => method === 'setReadConfiguration')?.payload).toBe(false) + }) +}) diff --git a/src/renderer/src/context/__tests__/stubRenderer.ts b/src/renderer/src/context/__tests__/stubRenderer.ts index 5a1ca32a..617b413b 100644 --- a/src/renderer/src/context/__tests__/stubRenderer.ts +++ b/src/renderer/src/context/__tests__/stubRenderer.ts @@ -62,7 +62,17 @@ export const fireEvent = (event: string, ...args: unknown[]): void => { for (const listener of listeners.get(event) ?? []) listener(...args) } -export const stubRenderer = (): void => { +/** + * Which window the stub answers as. + * + * `isServerWindow` is the flag three stores read at module scope to decide what + * they may do, so a test about the split out window has to be able to say so. + */ +export interface StubOptions { + isServerWindow?: boolean +} + +export const stubRenderer = ({ isServerWindow = false }: StubOptions = {}): void => { listeners.clear() const w = window as unknown as { electron: unknown; api: unknown } w.electron = { @@ -86,7 +96,7 @@ export const stubRenderer = (): void => { // and would make every window look like the server window. get: (_target, method: string): unknown => method === 'isServerWindow' - ? false + ? isServerWindow : (answers[method] ?? ((): Promise => Promise.resolve(undefined))) } ) diff --git a/src/renderer/src/context/client.zustand.ts b/src/renderer/src/context/client.zustand.ts index d2e88404..050c164f 100644 --- a/src/renderer/src/context/client.zustand.ts +++ b/src/renderer/src/context/client.zustand.ts @@ -528,6 +528,25 @@ export const useClientZustand = create< const clientZustand = useClientZustand.getState() +/** + * The window this module may call main from, the question its two siblings ask. + * + * `App.tsx` imports `containers/Client` statically and `Client.tsx:11` imports + * this file, so `out/renderer/assets/` holds one js file and both windows + * evaluate this module scope. Without the guard, opening the split out server + * window ran `init`, which hands main the config this window loaded and calls + * `setReadConfiguration(false)`. `modbusClient.ts:586` reads that flag, and off + * it polls one flat `[address, length]` block instead of the configured groups, + * while the main window's toggle still reads on. `init`'s own `set` writes + * `CLIENT_ZUSTAND_STORAGE_KEY` as well, because persist wraps `setState`, so + * the split window overwrote the shared key on every open. + * + * The version fetch below stays outside it. `get_app_version` asks `app` rather + * than the client, and `containers/Home.tsx` is the only reader of what it + * answers, which `App.tsx` does not draw in this window. + */ +const isServerWindow = window.api.isServerWindow + // Keep the fields that parsed and default the rest, then say which went. const repair = repairPersistedStore(useClientZustand, PersistedClientZustandSchema, { storageKey: CLIENT_ZUSTAND_STORAGE_KEY, @@ -538,7 +557,7 @@ const repair = repairPersistedStore(useClientZustand, PersistedClientZustandSche if (repair) useClientZustand.setState({ ...repair.state, configReset: repair.reset }) // Sync the main process state with the front end -clientZustand.init() +if (!isServerWindow) clientZustand.init() // // @@ -575,7 +594,7 @@ onEvent('scan_progress', (scanProgress) => { // // Stop scanning when reloaded, shouldn't be a problem with the build app, // but just in case and for development, stop scanning when the frontend is reloaded -window.api.stopScanningUnitIds() +if (!isServerWindow) window.api.stopScanningUnitIds() window.api.getAppVersion().then((version) => { useLayoutZustand.getState().setVersion(version) From c01310475d9c96ef58128784cd2e8062f4332d58 Mon Sep 17 00:00:00 2001 From: Harted Date: Wed, 16 Sep 2026 15:58:04 +0200 Subject: [PATCH 03/19] fix: a server register the encoder cannot serve is refused by name `RegisterParamsSchema` bounded `address` through `RegisterAddressSchema` and left three fields bare, and a config file and a persisted blob both arrive on it and nothing else asks. `length` reached `createStringRegisters`, which is `Buffer.alloc(length * 2)`: at 1e12 Electron 43 answers ERR_OUT_OF_RANGE, at 1e9 it allocates and then builds a billion words, and `getUsedAddresses` loops the same number. `value` reached `createRegisters`, where `writeUInt32BE(-1)` and `writeBigUInt64BE(2n ** 64n)` throw and `BigInt(1.5)` throws. `interval` had a floor and no ceiling, and above 2147483647 `setInterval` warns `TimeoutOverflowWarning` and sets the duration to 1. Each of the three rejected the invoke `init` awaits, which leaves every `ready` false and the server view faded out on that launch and every one after it. The bounds were already stated elsewhere and are applied here now. `getValueRangeError` sits beside `createRegisters` and reads `getMinMaxValues`, the table the add dialog's mask reads, over the seven types whose writer throws: `float` and `double` take anything, `unix` goes through `value >>> 0`, `datetime` through the clamp in `encodeIEC870DateTime`, and `none` and `utf8` never reach the encoder, so a rule over those would refuse a config that works. `getAddressFitError` is the pair of `address` and `length` and is the add dialog's own rule. A stored register outside the bounds is dropped rather than refused with the blob around it, which is what store version 5 and 6 already do for an address and an interval. Version 7 runs the same step, and `dropUnservableRegisters` reads `RegisterParamsSchema`, so tightening the schema is what tightens the drop. A config file is still refused whole, with the register and the field named, because the user picked the file. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- CHANGELOG.md | 8 ++ .../AddRegister/AddRegister.tsx | 7 +- .../__tests__/AddRegister.test.tsx | 4 +- src/shared/__tests__/schemas.test.ts | 85 ++++++++++++++++++- .../__tests__/registerAddress.test.ts | 80 +++++++++++++++++ src/shared/migrations/server/zustand.ts | 6 +- src/shared/migrations/shared.ts | 17 ++-- src/shared/types/server.ts | 52 ++++++++++-- src/shared/utils.ts | 48 +++++++++++ 9 files changed, 288 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ed0c79f..4c89b8ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A server register the encoder cannot serve is now refused by name.** A + config file could put a fixed value outside the range of its own data type, a + string wider than the register map, a register running past the last address, + or a generator whose interval was so long it fired every millisecond instead. + Opening it left the server view blank for that launch and every launch after + it, with no message and nothing on screen to clear it with. The file is now + refused with the register and the field named, and one already saved is + dropped on the next launch with the rest of the setup kept. - **Splitting the server into its own window no longer turns Read Configuration off.** The toggle stayed on while the reading went back to the address and length in the toolbar, so every configured register above the diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/AddRegister.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/AddRegister.tsx index 4da52abe..8aac1d04 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/AddRegister.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/AddRegister.tsx @@ -78,9 +78,10 @@ const AddRegister = meme(() => { addRegisterZustand.setValue(FIELD_DEFAULTS.value, true) } else if (value !== undefined && isTimestampType(dataType)) { // The picker works in milliseconds and a unix register stores seconds. - // `RegisterParamsSchema` bounds no value, so a config file can hold one - // outside the window, and the field says so rather than the picker having - // to be touched first. + // `getValueRangeError` leaves `unix` and `datetime` alone, because one + // goes through `value >>> 0` and the other through a clamp rather than + // throwing, so a config file can hold one outside the window and the + // field says so rather than the picker having to be touched first. const milliseconds = dataType === 'unix' ? Number(value) * 1000 : Number(value) addRegisterZustand.setValue(String(milliseconds), inTimestampWindow(dataType, milliseconds)) } else if (value !== undefined) { diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/__tests__/AddRegister.test.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/__tests__/AddRegister.test.tsx index 1a019f1f..299f7080 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/__tests__/AddRegister.test.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/__tests__/AddRegister.test.tsx @@ -316,7 +316,9 @@ describe('the edit dialog buttons', () => { expect(Number(useAddRegisterZustand.getState().value)).toBeGreaterThanOrEqual(openedAt) }) - // `RegisterParamsSchema` bounds no value, so this arrives from a config file. + // `getValueRangeError` leaves `datetime` alone, because + // `encodeIEC870DateTime` clamps rather than throws, so this arrives from a + // config file and the field is where it is said. it('marks a stored datetime outside the window wrong before anything is typed', () => { renderEditing(timestampAt100('datetime', Date.UTC(2200, 0, 1))) diff --git a/src/shared/__tests__/schemas.test.ts b/src/shared/__tests__/schemas.test.ts index 9c26639a..1415b3df 100644 --- a/src/shared/__tests__/schemas.test.ts +++ b/src/shared/__tests__/schemas.test.ts @@ -309,10 +309,18 @@ describe('Server RegisterParamsSchema — generator', () => { expect(RegisterParamsSchema.safeParse({ ...generator, min: 0, max: 0 }).success).toBe(true) }) - it.each([0, -5, 500, 1000.5])('refuses interval %s', (interval) => { + // Above 2147483647 `setInterval` warns `TimeoutOverflowWarning` and sets the + // duration to 1, so 1e12 fires every millisecond: the ceiling is Node's. + it.each([0, -5, 500, 1000.5, 2147483648, 1e12])('refuses interval %s', (interval) => { expect(RegisterParamsSchema.safeParse({ ...generator, interval }).success).toBe(false) }) + it('accepts the highest interval setInterval holds', () => { + expect(RegisterParamsSchema.safeParse({ ...generator, interval: 2147483647 }).success).toBe( + true + ) + }) + // The dialog lets one through and the generator covers the same range either // way, so a rule here would refuse what the Add button sends. it('accepts a min above its max', () => { @@ -333,6 +341,81 @@ describe('Server RegisterParamsSchema — generator', () => { }) }) +// `RegisterParamsSchema` bounded the address and left `length` and `value` +// bare, so a config file reached the code that cannot take them: +// `createStringRegisters` is `Buffer.alloc(length * 2)`, and `addRegister`'s +// fixed branch hands the value straight to `createRegisters`. +describe('Server RegisterParamsSchema — the width and the value', () => { + const fixed = (params: Record): unknown => ({ + address: 0, + registerType: 'holding_registers', + comment: '', + ...params + }) + + it.each([0, -1, 10.5, 1e9, 1e12])('refuses a string of length %s', (length) => { + expect( + RegisterParamsSchema.safeParse( + fixed({ dataType: 'utf8', length, stringValue: 'x', value: 0 }) + ).success + ).toBe(false) + }) + + it('accepts a string that fills the map from address 0', () => { + expect( + RegisterParamsSchema.safeParse( + fixed({ dataType: 'utf8', length: 65536, stringValue: 'x', value: 0 }) + ).success + ).toBe(true) + }) + + it('refuses a register that runs past address 65535', () => { + expect( + RegisterParamsSchema.safeParse(fixed({ address: 65534, dataType: 'uint64', value: 1 })) + .success + ).toBe(false) + expect( + RegisterParamsSchema.safeParse(fixed({ address: 65532, dataType: 'uint64', value: 1 })) + .success + ).toBe(true) + }) + + // `writeUInt16BE(70000)`, `writeInt16BE(40000)` and `BigInt(1.5)` all throw, + // and each of the three passed this schema. + it.each([ + ['uint16', 70000], + ['uint16', -1], + ['int16', 40000], + ['int32', 2147483648], + ['uint32', -1], + ['int64', 1.5], + ['uint64', -1], + ['bitmap', 65536] + ])('refuses %s value %s', (dataType, value) => { + expect(RegisterParamsSchema.safeParse(fixed({ dataType, value })).success).toBe(false) + }) + + // The encoder takes these, so a rule refusing them would cost a working + // config. `float` and `double` write anything, `unix` goes through + // `value >>> 0` and `datetime` through the clamp in `encodeIEC870DateTime`. + it.each([ + ['float', 1e300], + ['double', -1e300], + ['unix', -1], + ['datetime', 0], + ['uint16', 65535], + ['int16', -32768] + ])('accepts %s value %s', (dataType, value) => { + expect(RegisterParamsSchema.safeParse(fixed({ dataType, value })).success).toBe(true) + }) + + it('names the field it refused', () => { + const result = RegisterParamsSchema.safeParse(fixed({ dataType: 'uint16', value: 70000 })) + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.path).toEqual(['value']) + }) +}) + describe('SerialPortOptionsSchema', () => { const withOptions = (options: Record): unknown => ({ baudRate: '9600', diff --git a/src/shared/migrations/__tests__/registerAddress.test.ts b/src/shared/migrations/__tests__/registerAddress.test.ts index bc68ccda..c890a015 100644 --- a/src/shared/migrations/__tests__/registerAddress.test.ts +++ b/src/shared/migrations/__tests__/registerAddress.test.ts @@ -390,6 +390,86 @@ describe('the used addresses the drop writes back', () => { }) }) +describe('a persisted register the encoder cannot serve', () => { + /** A fixed register at `address`, with `params` over the defaults. */ + const register = ( + address: number, + overrides: Record + ): Record => ({ + value: 1, + params: { ...params(address), ...overrides } + }) + + const persisted = ( + registers: Record> + ): Record => ({ + serverRegisters: { + u: { + '1': { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: registers + } + } + } + }) + + const holding = (state: Record): Record => { + const perUuid = state.serverRegisters as Record> + const unit = perUuid.u?.['1'] as Record + return unit.holding_registers as Record + } + + // `Buffer.alloc(2e12)` answers ERR_OUT_OF_RANGE and `getUsedAddresses` loops + // the same number, so the register that went is the one that hung the launch. + it('drops a string wider than the map and keeps its neighbours', () => { + const state = persisted({ + '0': register(0, { dataType: 'utf8', length: 10, stringValue: 'x', value: 0 }), + '20': register(20, { dataType: 'utf8', length: 1e12, stringValue: 'x', value: 0 }), + '40': register(40, {}) + }) + dropUnservableRegisters(state) + + expect(Object.keys(holding(state))).toEqual(['0', '40']) + }) + + it('drops a fixed value its data type cannot encode', () => { + const state = persisted({ + '0': register(0, { value: 70000 }), + '10': register(10, { dataType: 'int64', value: 1.5 }), + '20': register(20, { value: 65535 }) + }) + dropUnservableRegisters(state) + + expect(Object.keys(holding(state))).toEqual(['20']) + }) + + it('drops a register that runs past address 65535', () => { + const state = persisted({ + '65534': register(65534, { dataType: 'uint64', value: 1 }), + '65532': register(65532, { dataType: 'uint64', value: 1 }) + }) + dropUnservableRegisters(state) + + expect(Object.keys(holding(state))).toEqual(['65532']) + }) + + // The drop on its own is not the store's behaviour; the step in `migrate` is. + it('is dropped by the migration a v6 blob runs', () => { + const state = migrateServerState( + persisted({ '0': register(0, { value: 70000 }), '10': register(10, {}) }), + 6 + ) + + expect(Object.keys(holding(state))).toEqual(['10']) + }) + + it('is behind a version the store has moved past', () => { + expect(CURRENT_SERVER_ZUSTAND_VERSION).toBeGreaterThan(6) + }) +}) + describe('a persisted mapping entry outside the map', () => { /** The last client store version whose blobs can carry any numeric key. */ const LAST_VERSION_ACCEPTING_ANY_KEY = 3 diff --git a/src/shared/migrations/server/zustand.ts b/src/shared/migrations/server/zustand.ts index f544f36c..7e2b5965 100644 --- a/src/shared/migrations/server/zustand.ts +++ b/src/shared/migrations/server/zustand.ts @@ -1,7 +1,7 @@ import { V1RegisterParams, V1ServerRegistersPerUnit, extractGlobalEndianness } from './shared' import { dropUnservableRegisters, migrateBoolShapeForUnit, repairPersistedParity } from '../shared' -export const CURRENT_SERVER_ZUSTAND_VERSION = 6 +export const CURRENT_SERVER_ZUSTAND_VERSION = 7 /** Where the server store keeps its state. */ export const SERVER_ZUSTAND_STORAGE_KEY = 'server.zustand' @@ -145,7 +145,9 @@ export function migrateServerState( // v4→v5: registers at an address outside the 16 bit map // v5→v6: and generators the interval floor now refuses - if (version < 6) { + // v6→v7: and a string width, a fixed value or an interval outside what the + // encoder, the map or `setInterval` can take + if (version < 7) { dropUnservableRegisters(state) } diff --git a/src/shared/migrations/shared.ts b/src/shared/migrations/shared.ts index 62a03f34..64983805 100644 --- a/src/shared/migrations/shared.ts +++ b/src/shared/migrations/shared.ts @@ -93,12 +93,17 @@ const usedAddressesOfUnit = ( * Drop persisted registers the current `RegisterParamsSchema` no longer names, * and rewrite the used addresses of every unit walked. * - * Two rules arrived after registers had already been persisted against looser - * ones. `RegisterParamsBasePartSchema.address` was a bare number, so a config - * file could put a register at 70000. `interval` was a bare number, so one - * could carry a generator that fires every millisecond. `repairPersisted` - * works a top level field at a time, and without this one such register costs - * every register on every server and every unit. + * Every rule here arrived after registers had already been persisted against a + * looser one. `address` was a bare number, so a config file could put a + * register at 70000. `interval` had no floor, so one could carry a generator + * that fires every millisecond, and then no ceiling, where `setInterval` sets + * the duration to 1 and does the same thing. `length` was bare, so a string + * could ask for `Buffer.alloc(2e12)`, and a fixed `value` was bare, so a + * `uint16` could carry 70000 and throw out of `createRegisters`. A register + * running past address 65535 is the pair of `address` and `length` and was + * refused by neither. `repairPersisted` works a top level field at a time, and + * without this one such register costs every register on every server and every + * unit. * * `usedAddresses` is persisted beside the registers, `isAddressInUse` refuses an * address against it, and the only thing recomputing it on launch is diff --git a/src/shared/types/server.ts b/src/shared/types/server.ts index 395a01db..8243f842 100644 --- a/src/shared/types/server.ts +++ b/src/shared/types/server.ts @@ -4,6 +4,7 @@ import { BitMapConfigSchema } from './bitmap' import { PortSchema, RegisterAddressKeySchema, RegisterAddressSchema } from './ranges' import { SerialPortOptionsSchema } from './serial' import { UnitIdString, UnitIdStringSchema } from './unitid' +import { getAddressFitError, getValueRangeError } from '../utils' import { BooleanRegisters, BooleanRegistersSchema, @@ -45,8 +46,10 @@ export type StartRtuServerParams = z.infer * stored in milliseconds, so anything below a second reaches this only from a * config file. An interval of 0 is the one that costs: `valueGenerator` hands * it to `setInterval`, and an interval of 0 fired 78 times in 100 ms when - * measured. The ceiling is the mask's rather than the generator's, so it is - * not stated here. + * measured. The ceiling is Node's rather than the mask's: above 2147483647 it + * warns `TimeoutOverflowWarning` and sets the duration to 1, so an interval of + * 1e12 fires every millisecond, which is the flood the floor was written to + * prevent. * * `min` and `max` are each a bare number and stay one. The dialog lets a min * above a max through, and `Math.random() * (max - min) + min` covers the same @@ -57,7 +60,7 @@ export type StartRtuServerParams = z.infer const RegisterParamsGeneratorPartSchema = z.object({ min: z.number(), max: z.number(), - interval: z.number().int().min(1000), + interval: z.number().int().min(1000).max(2147483647), value: z.undefined() // Explicitly forbid 'value' }) export type RegisterParamsGeneratorPart = z.infer @@ -88,22 +91,59 @@ export type RegisterValue = { * `sync_server_register` take. `remove_server_register` takes * `RegisterAddressSchema`, so while this field was a bare number an address * outside the map went in and could not come back out. + * + * `length` is the width of a string, and it is the one field the width of the + * register is read off. Left bare, `length: 1e12` passed here and reached + * `createStringRegisters`, which is `Buffer.alloc(2e12)`: inside Electron 43 + * that answers ERR_OUT_OF_RANGE, and `length: 1e9` allocates and then builds a + * billion words instead. `getUsedAddresses` loops the same number. */ export const RegisterParamsBasePartSchema = z.object({ address: RegisterAddressSchema, registerType: NumberRegistersSchema, dataType: BaseDataTypeSchema, comment: z.string(), - length: z.number().optional(), + length: z.number().int().min(1).optional(), stringValue: z.string().optional(), bitMap: BitMapConfigSchema.optional() }) export type RegisterParamsBasePart = z.infer -// Final RegisterValueParameters schema with conditional fields +/** + * Two rules over fields that bound each other, which is why they sit here + * rather than beside the fields. + * + * The register has to fit the map. `address` is 16 bit and `length` is bounded + * above, and a register can still run off the end: the add dialog refuses that + * with `getAddressFitError`, and a config file did not ask. + * + * A fixed value has to be one its own `dataType` can encode, which is + * `getValueRangeError`. The pair is the rule, not the field: 70000 is a + * `uint32` and not a `uint16`, and `addRegister`'s fixed branch hands whatever + * arrives straight to `createRegisters`. + * + * Both are stated where the add dialog states them, so the file and the field + * answer the same question. `interval`, `min` and `max` are the generator's and + * are bounded above. + */ export const RegisterParamsSchema = RegisterParamsBasePartSchema.and( z.union([RegisterParamsGeneratorPartSchema, RegisterParamsStaticPartSchema]) -) +).superRefine((params, ctx) => { + if (getAddressFitError(params.dataType, params.address, params.length)) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: [params.length === undefined ? 'address' : 'length'], + message: `A ${params.dataType} register at ${params.address} runs past address 65535` + }) + } + + if (params.value === undefined) return + + const rangeError = getValueRangeError(params.dataType, params.value) + if (rangeError) { + ctx.addIssue({ code: z.ZodIssueCode.custom, path: ['value'], message: rangeError }) + } +}) export type RegisterParams = z.infer // Schema for a single boolean entry with optional comment diff --git a/src/shared/utils.ts b/src/shared/utils.ts index e8fce3ce..8cb40ff4 100644 --- a/src/shared/utils.ts +++ b/src/shared/utils.ts @@ -209,6 +209,54 @@ export const getMinMaxValues = (dataType: DataType): { min: number; max: number } } +/** + * The types `createRegisters` throws on for a value outside their range. + * + * Measured against the writers it calls. `writeUInt16BE(1.5)` and + * `writeInt32BE(1.5)` truncate, `writeFloatBE(1e300)` and `writeDoubleBE` take + * anything, `unix` is written through `value >>> 0` and `datetime` through + * `encodeIEC870DateTime`, which clamps to the format's own window. What is left + * is the seven below, where `writeUInt32BE(-1)` and + * `writeBigUInt64BE(2n ** 64n)` both answer ERR_OUT_OF_RANGE. `none` and `utf8` + * are not `EncodableDataType` and never reach the encoder at all. + */ +const RANGE_CHECKED_TYPES: readonly DataType[] = [ + 'int16', + 'uint16', + 'bitmap', + 'int32', + 'uint32', + 'int64', + 'uint64' +] + +/** + * Why `createRegisters` cannot encode `value` as `dataType`, or nothing. + * + * `RegisterParamsSchema` bounded the address and left the value bare, so a + * config file carrying `{ dataType: 'uint16', value: 70000 }` reached + * `writeUInt16BE` and threw out of `addRegister`. The ranges are + * `getMinMaxValues`, which the add dialog's mask already reads, so the file and + * the field now answer the same question. + * + * A 64 bit value also has to be a whole number, because `BigInt(1.5)` throws + * where `writeUInt16BE(1.5)` truncates. + */ +export const getValueRangeError = (dataType: DataType, value: number): string | undefined => { + if (!RANGE_CHECKED_TYPES.includes(dataType)) return undefined + + if ((dataType === 'int64' || dataType === 'uint64') && !Number.isInteger(value)) { + return `A ${dataType} value has to be a whole number` + } + + const { min, max } = getMinMaxValues(dataType) + if (value < min || value > max) { + return `A ${dataType} value has to be between ${min} and ${max}` + } + + return undefined +} + /** * Whether the mask has taken anything a number could be made of. * From 2c83adb9d1fed1163b9021514724d768ebd20bf8 Mon Sep 17 00:00:00 2001 From: Harted Date: Wed, 16 Sep 2026 17:35:42 +0200 Subject: [PATCH 04/19] test: the client channel list gets a meter and three tests get their claim back `/code-review` on `218a60f` found four things. `CLIENT_CHANNELS` was a hand written list with nothing asserting it stays complete, which is the defect class the commit closes. The thirteenth conformance rule reads the handler's own body: a listener naming `client` or `state` reaches what one window owns, and it has to be in the list or in the test's own three named exceptions. Proved by adding a `get_app_version` handler that reads `client.state`, which the rule printed. The test named `refuses a guarded client channel before it reads the payload` did not measure that ordering: its payload was valid, so both orders answer the same. It now sends a payload the schema also refuses, and asserts the message names the window and not Zod, with the main window's half beside it. Spec 31's assertion was already satisfied by the rows the test before it left, so detection rested on a scroll being slower than a localhost read. It clears the grid first, and the row is asserted absent before the Read. Measured red again with the guard out both sides. `Windows.isMain` had no test of the real implementation, because `ipc.test.ts` replaces it. `windows.test.ts` covers the main window, the other window, no main window and a destroyed one. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- .claude/skills/audit/SKILL.md | 2 +- CONTRIBUTING.md | 15 ++++ .../31-split-view-client-state.spec.ts | 7 ++ src/__tests__/conformance.test.ts | 77 +++++++++++++++++++ src/main/__tests__/ipc.test.ts | 24 ++++-- src/main/__tests__/windows.test.ts | 31 ++++++++ 6 files changed, 150 insertions(+), 6 deletions(-) diff --git a/.claude/skills/audit/SKILL.md b/.claude/skills/audit/SKILL.md index 51aee187..0cfaafdb 100644 --- a/.claude/skills/audit/SKILL.md +++ b/.claude/skills/audit/SKILL.md @@ -26,7 +26,7 @@ Split by what the code shares, not by directory size. One area per run. Read `CLAUDE.md`, `CONTRIBUTING.md` and `src/__tests__/conformance.test.ts`. -The suite already asserts twelve conventions. **Do not report what it asserts** — +The suite already asserts thirteen conventions. **Do not report what it asserts** — it is green, so those are closed. Audit what a test cannot see. ## The eight criteria diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2a1fcfc7..0f2117ca 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -145,6 +145,21 @@ like a guard. The caller has to be in `src/renderer`, because a channel only the e2e suite drives is one the app does not use, and that is a decision to take rather than to let happen. +**Every channel that reaches main's client is placed.** `main/index.ts` +constructs one `ModbusClient` and no client channel carries an addressee, so any +window can aim one at it, and both windows load the same renderer bundle. +`CLIENT_CHANNELS` in `main/ipc.ts` is the list `createIpcHandle` refuses from a +window that is not `windows.main`, and it is written by hand. A channel added +later is a name, a spec entry and a handler, and every other rule here stays +green while it escapes that list. The signal the test reads is the handler's own +body: a listener naming `client` or `state` reaches what one window owns. +Everything else drives a server, which is addressed by uuid, or asks `app` or a +Linux helper. Three reach the client and are deliberately not refused, and the +test names them with the reason: `list_serial_ports` and `validate_serial_port` +enumerate hardware and the RTU server's COM field asks for the first of the two +from the server window, and `get_client_state` answers a value rather than +`undefined`, so a refusal would have nothing to hand back. + **Every configured path alias is imported through.** `@renderer/*` and `@shared` are the two, in the tsconfigs and in `electron.vite.config.ts` alike. An alias nobody imports through resolves whatever it points at, including a directory diff --git a/e2e/specs/01-main/31-split-view-client-state.spec.ts b/e2e/specs/01-main/31-split-view-client-state.spec.ts index 13b70a34..60f9026e 100644 --- a/e2e/specs/01-main/31-split-view-client-state.spec.ts +++ b/e2e/specs/01-main/31-split-view-client-state.spec.ts @@ -8,6 +8,7 @@ import { enableReadConfiguration, disableReadConfiguration, cleanServerState, + clearData, loadServerConfig, loadClientConfig, selectRegisterType, @@ -66,6 +67,12 @@ test.describe.serial('Read configuration survives the split out server window', serverPage = await splitOutServerWindow(electronApp, mainPage) await expect(serverPage.getByTestId('section-holding_registers')).toBeVisible() + // The rows the previous test read already satisfy the assertion below, and + // the read that would empty them lands after it. Clear them first, so the + // grid has to be filled again rather than left alone. + await clearData(mainPage) + await expect(mainPage.locator('.MuiDataGrid-row[data-id="50"]')).toHaveCount(0) + await mainPage.getByTestId('read-btn').click() await expectCell(mainPage, 50, 'word_uint16', '51') diff --git a/src/__tests__/conformance.test.ts b/src/__tests__/conformance.test.ts index 120894b0..0667faf8 100644 --- a/src/__tests__/conformance.test.ts +++ b/src/__tests__/conformance.test.ts @@ -698,6 +698,83 @@ describe('the lists that name a channel agree', () => { }) }) +// +// ─── Every channel that reaches main's client is placed ────────────────────── +// +// `main/index.ts` constructs one `ModbusClient`, and no client channel carries +// an addressee, so any window can aim one at it. `CLIENT_CHANNELS` in +// `main/ipc.ts` is the list `createIpcHandle` refuses from a window that is not +// `windows.main`, and it is hand written. A channel added later is three lines, +// a name, a spec entry and a handler, and every one of the rules above stays +// green while it escapes the guard, which is the defect this list was added to +// close. +// +// The signal is the handler's own body: a listener naming `client` or `state` +// reaches what one window owns. Everything else drives a server, which is +// addressed by uuid, or asks `app` or a Linux helper. + +describe('every channel that reaches main’s client is placed', () => { + /** + * The channels that reach the client and are deliberately not refused. + * + * The two serial ones enumerate hardware rather than touch the client, and + * `server.zustand.ts refreshSerialPorts` calls the first of the two for the + * RTU server's COM field. `get_client_state` answers a `ClientState` rather + * than `undefined`, so a refusal has nothing to hand back, and reading what + * main is doing changes nothing about it. + */ + const ALLOWED_FROM_EITHER_WINDOW = [ + 'list_serial_ports', + 'validate_serial_port', + 'get_client_state' + ] + + const ipc = parse(join(repoRoot, 'src/main/ipc.ts')) + + /** Every channel whose listener body names `client` or `state`. */ + const reachesTheClient = new Set() + eachNode(ipc, (node) => { + if (!ts.isCallExpression(node) || !ts.isIdentifier(node.expression)) return + if (node.expression.text !== 'ipcHandle') return + const [channel, listener] = node.arguments + if (!channel || !ts.isStringLiteral(channel) || !listener) return + if (/\b(client|state)\./.test(listener.getText(ipc))) reachesTheClient.add(channel.text) + }) + + /** The names `CLIENT_CHANNELS` holds. */ + const refused = new Set() + eachNode(ipc, (node) => { + if (!ts.isVariableDeclaration(node)) return + if (node.name.getText(ipc) !== 'CLIENT_CHANNELS') return + eachNode(node as unknown as ts.SourceFile, (child) => { + if (ts.isStringLiteral(child)) refused.add(child.text) + }) + }) + + it('finds channels to check', () => { + expect(reachesTheClient.size).toBeGreaterThan(10) + expect(refused.size).toBeGreaterThan(10) + }) + + it('refuses every one of them from the server window, or says why not', () => { + const unplaced = [...reachesTheClient] + .filter((channel) => !refused.has(channel)) + .filter((channel) => !ALLOWED_FROM_EITHER_WINDOW.includes(channel)) + .sort() + expect(unplaced).toEqual([]) + }) + + it('names no channel the handlers do not reach the client through', () => { + const stale = [...refused].filter((channel) => !reachesTheClient.has(channel)).sort() + expect(stale).toEqual([]) + }) + + it('allows only channels that are there', () => { + const gone = ALLOWED_FROM_EITHER_WINDOW.filter((channel) => !reachesTheClient.has(channel)) + expect(gone).toEqual([]) + }) +}) + // // ─── Every channel carrying an object declares a schema ────────────────────── // diff --git a/src/main/__tests__/ipc.test.ts b/src/main/__tests__/ipc.test.ts index 965209bc..c6beb6f2 100644 --- a/src/main/__tests__/ipc.test.ts +++ b/src/main/__tests__/ipc.test.ts @@ -198,20 +198,34 @@ describe('createIpcHandle', () => { expect(String(sent[0]?.message.error)).toContain('set_read_configuration') }) + // A payload the schema also refuses is the input the two orders answer + // differently on: schema first reports the Zod failure and says nothing about + // the window. The asker is the question worth answering, so it goes first. it('refuses a guarded client channel before it reads the payload', async () => { const { windows, sent } = createWindows() const ipcHandle = createIpcHandle(windows) const listener = vi.fn() ipcHandle('scan_unit_ids', listener, ScanUnitIDParametersSchema) - await invoke( - 'scan_unit_ids', - { range: [1, 5], address: 0, length: 2, registerTypes: ['holding_registers'], timeout: 500 }, - { id: 'the split out server window' } - ) + await invoke('scan_unit_ids', 'not a payload', { id: 'the split out server window' }) expect(listener).not.toHaveBeenCalled() + expect(sent).toHaveLength(1) expect(String(sent[0]?.message.error)).toContain('not the main one') + expect(String(sent[0]?.message.error)).not.toContain('Expected') + }) + + it('reads the payload of a guarded client channel from the main window', async () => { + const { windows, sent } = createWindows() + const ipcHandle = createIpcHandle(windows) + const listener = vi.fn() + + ipcHandle('scan_unit_ids', listener, ScanUnitIDParametersSchema) + await invoke('scan_unit_ids', 'not a payload') + + expect(listener).not.toHaveBeenCalled() + expect(String(sent[0]?.message.error)).toContain('scan_unit_ids') + expect(String(sent[0]?.message.error)).not.toContain('not the main one') }) it('takes the same channel from the main window', async () => { diff --git a/src/main/__tests__/windows.test.ts b/src/main/__tests__/windows.test.ts index d7b13bf7..5fc04bb7 100644 --- a/src/main/__tests__/windows.test.ts +++ b/src/main/__tests__/windows.test.ts @@ -242,6 +242,37 @@ describe('Windows', () => { }) }) + // `createIpcHandle` refuses the fourteen channels that drive main's one + // Modbus client on this answer, so a window it cannot place has to answer + // false rather than throw. + describe('isMain', () => { + it('answers for the main window and for no other', () => { + const mainWindow = createMockWindow() + const serverWindow = createMockWindow() + windows.main = mainWindow as never + windows.server = serverWindow as never + + expect(windows.isMain(mainWindow.webContents as never)).toBe(true) + expect(windows.isMain(serverWindow.webContents as never)).toBe(false) + }) + + it('answers false with no main window', () => { + const orphan = createMockWindow() + + expect(windows.isMain(orphan.webContents as never)).toBe(false) + }) + + // On macos the app outlives its windows, so a destroyed main window is a + // state the handle reaches rather than a hypothetical. + it('answers false for a destroyed main window', () => { + const mainWindow = createMockWindow() + windows.main = mainWindow as never + mainWindow.isDestroyed.mockReturnValue(true) + + expect(windows.isMain(mainWindow.webContents as never)).toBe(false) + }) + }) + describe('main getter/setter', () => { // ! Coverage-only: trivial initial state it('returns null initially', () => { From 0765ca4d9aa97091e3f70a92ddda5095b46a0464 Mon Sep 17 00:00:00 2001 From: Harted Date: Wed, 16 Sep 2026 17:44:00 +0200 Subject: [PATCH 05/19] fix: main refuses one register it cannot encode and keeps the unit `addRegister` called `createRegisters` unguarded, so a value the data type cannot take threw straight out of it. `createIpcHandle` puts no try around a listener, so the invoke rejected rather than answering, and `syncServerRegisters` adds in a bare `for`, so the throw took every register after it in that unit with it. `server.zustand.helpers.ts` awaits that invoke before it writes `ready`, and `containers/Server.tsx` wraps the whole server view in ``, so one such register left the view blank on that launch and every one after it. One register is refused now. `addRegister` answers `undefined`, which the store already reads as nothing changed, and the window showing the server is told which register and why. Nothing is written for it, and the rest of the unit stands. This is the class answering for its own input rather than a reachable defect. `src/main/modules/__tests__/unencodableRegister.test.ts` measures the three doors `AUDIT-clusters.md` names as still open behind it, the add dialog, a repaired persisted store and the width overflow in `M-05`, and `RegisterParamsSchema` refuses all seven inputs on both guarded channels since `a301600`. `addRegister` is public and its only two callers cross that boundary, so no shipped path reaches the refusal, and it gets no changelog line. The generator branch is not answered for here. `ValueGenerator` encodes inside an unawaited async tick, so a `min` or `max` its data type cannot take writes nothing and warns once per interval instead of throwing. `RegisterParamsGeneratorPartSchema` leaves both bare on purpose, so that door is open, and closing it is `M-02` on the annoying branch. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- .../modules/__tests__/modbusServer.test.ts | 161 ++++++++++++++---- .../__tests__/unencodableRegister.test.ts | 55 ++++++ src/main/modules/modbusServer.ts | 71 ++++++-- 3 files changed, 245 insertions(+), 42 deletions(-) create mode 100644 src/main/modules/__tests__/unencodableRegister.test.ts diff --git a/src/main/modules/__tests__/modbusServer.test.ts b/src/main/modules/__tests__/modbusServer.test.ts index fa3470be..aaa59c1f 100644 --- a/src/main/modules/__tests__/modbusServer.test.ts +++ b/src/main/modules/__tests__/modbusServer.test.ts @@ -183,6 +183,17 @@ const lastSerialPortOptions = (): unknown => { const createMockWindows = (): Windows => ({ send: vi.fn() }) as unknown as Windows +/** + * The words `addRegister` answered, or a failure naming the refusal. + * + * `addRegister` answers `undefined` for a register it cannot encode, and a + * helper passing that on fails at whatever reads it next with nothing to say. + */ +const addedOrThrow = (words: number[] | undefined): number[] => { + if (!words) throw new Error('addRegister refused the register') + return words +} + describe('ModbusServer', () => { let server: ModbusServer let windows: Windows @@ -190,22 +201,28 @@ describe('ModbusServer', () => { const unitId: UnitIdString = '1' const unitIdNumber = Number(unitId) - /** A register the unit hosts, which is what makes the server answer for it at all. */ + /** + * A register the unit hosts, which is what makes the server answer for it at + * all. Throws by name on a refusal, because a helper answering `undefined` + * here fails somewhere else with nothing to say. + */ const hostUnit = (): number[] => - server.addRegister({ - uuid, - unitId, - params: { - address: 100, - registerType: 'holding_registers', - dataType: 'uint16', - comment: '', - value: 1, - min: undefined, - max: undefined, - interval: undefined - } - }) + addedOrThrow( + server.addRegister({ + uuid, + unitId, + params: { + address: 100, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: 1, + min: undefined, + max: undefined, + interval: undefined + } + }) + ) beforeEach(() => { vi.useFakeTimers() @@ -263,6 +280,41 @@ describe('ModbusServer', () => { ) }) + // `undefined` is what the store reads as nothing changed, so the refusal + // reuses the answer a refused payload already has. It used to throw, and + // `createIpcHandle` puts no try around a listener. + it('refuses a value its data type cannot encode and writes nothing', () => { + const answer = server.addRegister({ + uuid, + unitId, + params: { + address: 0, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: 70000, + min: undefined, + max: undefined, + interval: undefined + } + }) + + expect(answer).toBeUndefined() + expect(windows.send).not.toHaveBeenCalledWith( + 'register_value', + expect.anything(), + expect.anything() + ) + expect(windows.send).toHaveBeenCalledWith( + 'backend_message', + expect.objectContaining({ + variant: 'error', + message: 'The uint16 register at 0 was not added: main cannot encode that value' + }), + 'serverView' + ) + }) + // The `register_value` events above go out before this answer does, so the // renderer's store has no entry to put them in and drops them. The answer // is what that store folds instead. @@ -305,8 +357,8 @@ describe('ModbusServer', () => { }) expect(answer).toHaveLength(1) - expect(answer[0]).toBeGreaterThanOrEqual(100) - expect(answer[0]).toBeLessThanOrEqual(200) + expect(answer?.[0]).toBeGreaterThanOrEqual(100) + expect(answer?.[0]).toBeLessThanOrEqual(200) }) // `none` holds its address open and writes nothing, so there is no word to @@ -1003,6 +1055,51 @@ describe('ModbusServer', () => { ) }) + // `createRegisters` threw straight out of `addRegister`, and this loop is a + // bare `for`, so one register the encoder refused took every register after + // it in the unit with it and rejected the invoke the renderer's `init` + // awaits. `RegisterParamsSchema` refuses the payload before it gets here + // now, so this is the class answering for its own input. + it('keeps the registers after one the encoder refuses', () => { + server.syncServerRegisters({ + uuid, + unitId, + registerValues: [ + { + address: 0, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: -1, + min: undefined, + max: undefined, + interval: undefined + }, + { + address: 5, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: 7, + min: undefined, + max: undefined, + interval: undefined + } + ] + }) + + expect(windows.send).toHaveBeenCalledWith( + 'register_value', + expect.objectContaining({ address: 5, value: 7 }), + 'serverView' + ) + expect(windows.send).toHaveBeenCalledWith( + 'backend_message', + expect.objectContaining({ variant: 'error' }), + 'serverView' + ) + }) + it('disposes existing generators before syncing', () => { // Add generators for both register types server.addRegister({ @@ -2185,20 +2282,22 @@ describe('ModbusServer', () => { } const hostUnit = (id: UnitIdString, address: number, value: number): number[] => - server.addRegister({ - uuid, - unitId: id, - params: { - address, - registerType: 'holding_registers', - dataType: 'uint16', - comment: '', - value, - min: undefined, - max: undefined, - interval: undefined - } - }) + addedOrThrow( + server.addRegister({ + uuid, + unitId: id, + params: { + address, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value, + min: undefined, + max: undefined, + interval: undefined + } + }) + ) const tcpVector = async (): Promise => { await server.createServer({ uuid, port: 5020 }) diff --git a/src/main/modules/__tests__/unencodableRegister.test.ts b/src/main/modules/__tests__/unencodableRegister.test.ts new file mode 100644 index 00000000..be89f4b6 --- /dev/null +++ b/src/main/modules/__tests__/unencodableRegister.test.ts @@ -0,0 +1,55 @@ +// The three doors `AUDIT-clusters.md` names as still reaching `addRegister` +// after the schema was tightened: the add dialog, a repaired persisted store, +// and the width overflow. All three arrive on a guarded channel, so this asks +// the two schemas `main/ipc.ts` guards with whether they still let the input +// through that `createRegisters` and `createStringRegisters` throw on. +import { describe, expect, it } from 'vitest' +import { AddRegisterParamsSchema, SyncRegisterValueParamsSchema } from '@shared' + +const params = (overrides: Record): Record => ({ + address: 0, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: 1, + ...overrides +}) + +const addTakes = (overrides: Record): boolean => + AddRegisterParamsSchema.safeParse({ uuid: 'u', unitId: '1', params: params(overrides) }).success + +const syncTakes = (overrides: Record): boolean => + SyncRegisterValueParamsSchema.safeParse({ + uuid: 'u', + unitId: '1', + registerValues: [params(overrides)] + }).success + +describe('the register main cannot encode', () => { + const unencodable: [string, Record][] = [ + ['a uint16 of 70000', { value: 70000 }], + ['an int16 of 40000', { dataType: 'int16', value: 40000 }], + ['an int64 of 1.5', { dataType: 'int64', value: 1.5 }], + ['a string of 1e12 words', { dataType: 'utf8', length: 1e12, stringValue: 'x', value: 0 }], + ['a string of 1e9 words', { dataType: 'utf8', length: 1e9, stringValue: 'x', value: 0 }], + [ + 'a string of ten at 65530', + { address: 65530, dataType: 'utf8', length: 10, stringValue: 'x', value: 0 } + ], + ['an interval of 1e12', { interval: 1e12, min: 0, max: 10, value: undefined }] + ] + + it.each(unencodable)('is refused on add before main sees it: %s', (_name, overrides) => { + expect(addTakes(overrides)).toBe(false) + }) + + it.each(unencodable)('is refused on sync before main sees it: %s', (_name, overrides) => { + expect(syncTakes(overrides)).toBe(false) + }) + + it('still takes the registers the app sends', () => { + expect(addTakes({})).toBe(true) + expect(addTakes({ dataType: 'utf8', length: 10, stringValue: 'hello', value: 0 })).toBe(true) + expect(addTakes({ interval: 1000, min: 0, max: 10, value: undefined })).toBe(true) + }) +}) diff --git a/src/main/modules/modbusServer.ts b/src/main/modules/modbusServer.ts index 3de84d75..4a76046a 100644 --- a/src/main/modules/modbusServer.ts +++ b/src/main/modules/modbusServer.ts @@ -17,7 +17,8 @@ import { RegisterType, RegisterValue, DataBits, - StopBits + StopBits, + ValuedDataType } from '@shared' import { ServerTCP, ServerSerial } from 'modbus-serial' import { ServerEndianness } from '@shared' @@ -455,6 +456,37 @@ export class ModbusServer { this._littleEndian.set(uuid, littleEndian) } + /** + * The words a fixed register holds, or nothing at all when it cannot be + * encoded. + * + * `RegisterParamsSchema` bounds the pair of `dataType` and `value` at the IPC + * boundary, and `getValueRangeError` there reads the same table this asks + * about. This is the class answering for its own input: `addRegister` is + * public, and a caller inside main reaches it without crossing that boundary. + */ + private _encode = ({ + dataType, + value, + littleEndian, + stringValue, + length + }: { + dataType: ValuedDataType + value: number + littleEndian: boolean + stringValue?: string + length?: number + }): number[] | undefined => { + try { + return dataType === 'utf8' + ? createStringRegisters(stringValue ?? '', length ?? DEFAULT_UTF8_LENGTH) + : createRegisters(dataType, value, littleEndian) + } catch { + return undefined + } + } + private _unitData = (uuid: string, unitId: UnitIdString): ServerData => { const perUnitMap = this._ensureInnerMap(this._serverData, uuid) const serverData = perUnitMap.get(unitId) ?? getDefaultServerData() @@ -467,14 +499,26 @@ export class ModbusServer { * If a generator already exists at the address, it is disposed and replaced. * If a fixed value is provided, sets the register directly. * - * Answers the words now held from `address` on, which is nothing for `none` - * because that writes none. The renderer's store waits for this answer before - * it writes, and every `register_value` below goes out before the answer - * does, so those words would otherwise reach a store with no entry to put - * them in and be dropped. The store folds what comes back through the same + * Answers the words now held from `address` on, which is an empty list for + * `none` because that writes none. The renderer's store waits for this answer + * before it writes, and every `register_value` below goes out before the + * answer does, so those words would otherwise reach a store with no entry to + * put them in and be dropped. The store folds what comes back through the same * merge the event feeds, which is where a word becomes a value. + * + * `undefined` is the refusal, which is what the store already reads as + * nothing changed. An encoder that cannot take the register used to throw + * straight out of here: `createIpcHandle` puts no try around a listener, so + * the invoke rejected rather than answering, and `syncServerRegisters` adds + * in a bare loop, so the throw took every register after it in that unit + * with it. One register is refused now and the rest of the unit stands. + * + * The generator branch cannot be answered for here. `ValueGenerator` encodes + * inside an unawaited async tick, so a range its data type cannot take writes + * nothing and warns once per interval instead of throwing, which is `M-02` + * and is a change to that class. */ - public addRegister = ({ uuid, unitId, params }: AddRegisterParams): number[] => { + public addRegister = ({ uuid, unitId, params }: AddRegisterParams): number[] | undefined => { const littleEndian = this._littleEndian.get(uuid) ?? false const { address, @@ -517,10 +561,15 @@ export class ModbusServer { // If a fixed value is provided, set the register directly const fixedValue = !interval && value !== undefined if (fixedValue) { - const registers = - dataType === 'utf8' - ? createStringRegisters(stringValue ?? '', length ?? DEFAULT_UTF8_LENGTH) - : createRegisters(dataType, value, littleEndian) + const registers = this._encode({ dataType, value, littleEndian, stringValue, length }) + if (!registers) { + this._emitMessage({ + message: `The ${dataType} register at ${address} was not added: main cannot encode that value`, + variant: 'error' + }) + return undefined + } + registers.forEach((register, index) => { const registerAddress = address + index serverData[registerType][registerAddress] = register From b8c338cbc05e6db71bde1931aa4d79795d7d5a09 Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 07:27:52 +0200 Subject: [PATCH 06/19] fix: the server view draws on whether init ran, not on whether it worked `containers/Server.tsx` faded the whole server view in on `Object.values(z.ready).some(...)`. `ready` is per uuid and written only by `syncUuidToBackend`, after every invoke in it got through, and `init` sets every uuid false before its loop. So one rejected invoke anywhere in that loop left the view blank on that launch and on every launch after it, with no message and nothing on screen to clear the configuration with. The verdicts of `M-03`, `SH-01`, `SH-02` and `M-16` all end at that state, and fixing any one of them removed a door and left the latch. Two questions, two flags. `initialized` says `init` has run, whatever came of it, and the view draws on that. `ready` keeps its meaning, which is whether main knows this uuid, and keeps gating `setPort`, `setUnitId` and `setLittleEndian`. A failed sync now costs the one server it belonged to. `init` catches as well as sets the flag, because it runs from module scope with nothing awaiting it, so a rejection there was an unhandled one. Main reports its own failures through `backend_message`, and a rejected invoke carries the channel name and nothing the user can act on. The other half of the same flag: `ready` is not in `partialize`, so the `window_update` rehydrate brought back `uuids`, `port` and `selectedUuid` from the split out window and left `ready` where this window's `init` had put it. A server made in that window came back with no entry at all and refused its three setters with no message until a restart. The window that made it ran `createServer` for it, so main does know it, and the rehydrate says so. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- CHANGELOG.md | 8 + src/renderer/src/containers/Server.tsx | 7 +- .../__tests__/serverStoreRehydrate.test.ts | 45 ++++++ .../serverViewDrawsAfterInit.test.ts | 120 ++++++++++++++ src/renderer/src/context/server.zustand.ts | 146 +++++++++++------- .../src/context/server.zustand.types.ts | 21 +++ 6 files changed, 293 insertions(+), 54 deletions(-) create mode 100644 src/renderer/src/context/__tests__/serverViewDrawsAfterInit.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c89b8ec..f4fa9d4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **The server view no longer comes up blank and stays that way.** One thing main + refused while the setup was being handed over left the whole view faded out, + on that launch and on every launch after it, with no message and nothing on + screen to clear the configuration with. A refusal now costs the one server it + belonged to. +- **A server added in the split out window can be changed when that window + closes.** Its port, unit id and byte order each refused every change with no + message until Modbux was restarted. - **A server register the encoder cannot serve is now refused by name.** A config file could put a fixed value outside the range of its own data type, a string wider than the register map, a register running past the last address, diff --git a/src/renderer/src/containers/Server.tsx b/src/renderer/src/containers/Server.tsx index 0a7d5978..d2fde6cb 100644 --- a/src/renderer/src/containers/Server.tsx +++ b/src/renderer/src/containers/Server.tsx @@ -35,9 +35,12 @@ const ServerName = meme(() => { }) const Server = meme(() => { - const ready = useServerZustand((z) => Object.values(z.ready).some((ready) => ready)) + // `initialized` rather than `ready`, which is per uuid and written only on a + // sync that got through: one rejected invoke in `init`'s loop left this view + // blank on that launch and every one after it. + const initialized = useServerZustand((z) => z.initialized) return ( - + version: CURRENT_SERVER_ZUSTAND_VERSION }) +/** The same blob with a second server in it, as the split out window writes it. */ +const withSecondServer = (raw: string, uuid: string): string => { + const blob = JSON.parse(raw) + const { state } = blob + state.uuids.push(uuid) + state.selectedUuid = uuid + state.port[uuid] = '503' + state.unitId[uuid] = '0' + state.littleEndian[uuid] = false + state.serverRegisters[uuid] = {} + return JSON.stringify(blob) +} + +/** Waits out the rehydrate, which persist answers with a promise. */ +const settle = async (): Promise => { + await Promise.resolve() + await Promise.resolve() + await Promise.resolve() +} + const held = async (): Promise => { const { useServerZustand } = await import('../server.zustand') return Object.keys(useServerZustand.getState().serverRegisters.u?.['0']?.holding_registers ?? {}) @@ -84,6 +104,31 @@ describe('the server window closing', () => { expect(await held()).toEqual(['0']) }) + // `ready` is not persisted either, so the rehydrate brought a uuid back with + // no entry, and `setPort`, `setUnitId` and `setLittleEndian` each refuse on + // that with no message. The window that made the server ran `createServer` + // for it, so main does know it. + it('marks a server the split out window made as one main knows', async () => { + localStorage.setItem(SERVER_ZUSTAND_STORAGE_KEY, store([0])) + const { useServerZustand } = await import('../server.zustand') + await settle() + + localStorage.setItem( + SERVER_ZUSTAND_STORAGE_KEY, + withSecondServer(store([0]), 'made-in-the-split-window') + ) + + fireEvent('window_update', { main: true, server: true }) + fireEvent('window_update', { main: true, server: false }) + await settle() + + expect(useServerZustand.getState().uuids).toContain('made-in-the-split-window') + expect(useServerZustand.getState().ready['made-in-the-split-window']).toBe(true) + + useServerZustand.getState().setUnitId('7') + expect(useServerZustand.getState().unitId['made-in-the-split-window']).toBe('7') + }) + // `rtuServerActive` is not persisted, so re-reading the key leaves it where // it was, and the split out window is the one that heard the last change. it('asks main for the RTU status, at load and at the close', async () => { diff --git a/src/renderer/src/context/__tests__/serverViewDrawsAfterInit.test.ts b/src/renderer/src/context/__tests__/serverViewDrawsAfterInit.test.ts new file mode 100644 index 00000000..cb28b833 --- /dev/null +++ b/src/renderer/src/context/__tests__/serverViewDrawsAfterInit.test.ts @@ -0,0 +1,120 @@ +// @vitest-environment happy-dom +// +// `containers/Server.tsx` fades the whole server view in on one flag. It read +// `ready`, which `syncUuidToBackend` writes only for a uuid whose sync got +// through, and `init` sets every uuid false before its loop. So a rejected +// invoke anywhere in that loop left the view blank on that launch and on every +// one after it, with nothing on screen to clear the config with. The four +// verdicts in `tmp/audit-2` that end in a blank server view all end here. +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + CURRENT_SERVER_ZUSTAND_VERSION, + MAIN_SERVER_UUID, + SERVER_ZUSTAND_STORAGE_KEY +} from '@shared' +import { stubRenderer } from './stubRenderer' + +/** A blob with one server holding one register, so `init` has a sync to make. */ +const persisted = (): string => + JSON.stringify({ + state: { + selectedUuid: MAIN_SERVER_UUID, + uuids: [MAIN_SERVER_UUID], + port: { [MAIN_SERVER_UUID]: '502' }, + name: { [MAIN_SERVER_UUID]: 'bench' }, + unitId: { [MAIN_SERVER_UUID]: '0' }, + littleEndian: { [MAIN_SERVER_UUID]: false }, + usedAddresses: {}, + serverRegisters: { + [MAIN_SERVER_UUID]: { + '0': { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: { + '0': { + value: 1, + params: { + address: 0, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: 1 + } + } + } + } + } + } + }, + version: CURRENT_SERVER_ZUSTAND_VERSION + }) + +/** Main answering the port it bound, which is what `init` continues past. */ +const stubCreateServer = (port: number): void => { + const boundary = window.api as unknown as Record + window.api = new Proxy(boundary, { + get: (target, method: string): unknown => + method === 'createServer' + ? (): Promise => Promise.resolve(port) + : Reflect.get(target, method) + }) as never +} + +/** Makes one channel reject, the way a handler that throws does. */ +const rejectChannel = (method: string): void => { + const boundary = window.api as unknown as Record + window.api = new Proxy( + {}, + { + get: (_target, asked: string): unknown => { + if (asked === 'isServerWindow') return boundary[asked] + if (asked !== method) return boundary[asked] + return (): Promise => Promise.reject(new Error(`${method} threw`)) + } + } + ) as never +} + +/** Waits out `init`, which runs from module scope with nothing awaiting it. */ +const settle = async (): Promise => { + for (let i = 0; i < 20; i++) await Promise.resolve() +} + +beforeEach(() => { + vi.resetModules() + localStorage.clear() + stubRenderer() + localStorage.setItem(SERVER_ZUSTAND_STORAGE_KEY, persisted()) +}) + +describe('the flag the server view draws on', () => { + it('is set once init has run', async () => { + stubCreateServer(502) + const { useServerZustand } = await import('../server.zustand') + await settle() + + expect(useServerZustand.getState().initialized).toBe(true) + expect(useServerZustand.getState().ready[MAIN_SERVER_UUID]).toBe(true) + }) + + it('is set even when a register main refuses rejects the sync', async () => { + stubCreateServer(502) + rejectChannel('syncServerRegister') + const { useServerZustand } = await import('../server.zustand') + await settle() + + expect(useServerZustand.getState().initialized).toBe(true) + // The uuid is the one that is not ready, which is what the three setters + // refuse on, and it is the only thing the failure costs. + expect(useServerZustand.getState().ready[MAIN_SERVER_UUID]).toBe(false) + }) + + it('is set when creating the server fails outright', async () => { + rejectChannel('createServer') + const { useServerZustand } = await import('../server.zustand') + await settle() + + expect(useServerZustand.getState().initialized).toBe(true) + }) +}) diff --git a/src/renderer/src/context/server.zustand.ts b/src/renderer/src/context/server.zustand.ts index 3580373f..1fe8b120 100644 --- a/src/renderer/src/context/server.zustand.ts +++ b/src/renderer/src/context/server.zustand.ts @@ -61,6 +61,7 @@ export const useServerZustand = create< state.configReset = undefined }), ready: { [MAIN_SERVER_UUID]: false }, + initialized: false, selectedUuid: MAIN_SERVER_UUID, uuids: [MAIN_SERVER_UUID], port: { [MAIN_SERVER_UUID]: '502' }, @@ -139,70 +140,93 @@ export const useServerZustand = create< await window.api.resetServer(uuid) get().clean(uuid) }, + /** + * Hands main what the store holds, and marks the store initialized + * whatever came of that. + * + * `containers/Server.tsx` draws nothing until the flag is set, so a + * rejected invoke below used to cost the whole server view rather than + * the one uuid it belonged to, on that launch and on every one after it. + * The uuid keeps `ready` false, which is what its three setters refuse + * on, and that is the whole cost now. + * + * The `catch` is empty because this runs from module scope with nothing + * awaiting it, and a rejection there is an unhandled one. Main reports + * its own failures through `backend_message`; a rejected invoke carries + * the channel name and nothing the user can act on. + */ init: async (uuid) => { - set((state) => { - if (uuid) state.ready[uuid] = false - else for (const u of state.uuids) state.ready[u] = false - }) - const state = get() - const mode = state.serverMode ?? 'tcp' + try { + set((state) => { + if (uuid) state.ready[uuid] = false + else for (const u of state.uuids) state.ready[u] = false + }) + const state = get() + const mode = state.serverMode ?? 'tcp' - // Ensure every uuid has a unitId and littleEndian entry (for backward compatibility) - set((state) => { - for (const uuid of state.uuids) { - if (state.unitId[uuid] === undefined) { - state.unitId[uuid] = '0' - } - if (state.littleEndian[uuid] === undefined) { - state.littleEndian[uuid] = false // Default to Big-Endian + // Ensure every uuid has a unitId and littleEndian entry (for backward compatibility) + set((state) => { + for (const uuid of state.uuids) { + if (state.unitId[uuid] === undefined) { + state.unitId[uuid] = '0' + } + if (state.littleEndian[uuid] === undefined) { + state.littleEndian[uuid] = false // Default to Big-Endian + } } - } - }) + }) - if (mode === 'rtu') { - // RTU serves the main server's registers, so that is the only uuid - // there is anything to open for. - const serialConfig = state.serialConfig ?? getDefaultSerialConfig() - - // Only start if COM port is configured - if (serialConfig.com.trim()) { - try { - await window.api.startRtuServer({ uuid: MAIN_SERVER_UUID, serialConfig }) - } catch { - // Error is reported via backend_message event + if (mode === 'rtu') { + // RTU serves the main server's registers, so that is the only uuid + // there is anything to open for. + const serialConfig = state.serialConfig ?? getDefaultSerialConfig() + + // Only start if COM port is configured + if (serialConfig.com.trim()) { + try { + await window.api.startRtuServer({ uuid: MAIN_SERVER_UUID, serialConfig }) + } catch { + // Error is reported via backend_message event + } } - } - - await syncUuidToBackend(set, get, MAIN_SERVER_UUID) - - set((state) => { - state.selectedUuid = MAIN_SERVER_UUID - }) - } else { - // TCP mode: existing flow - const uuidsToSync = uuid ? [uuid] : state.uuids - for (const syncUuid of uuidsToSync) { - const port = Number(state.port[syncUuid]) - const actualPort = await window.api.createServer({ uuid: syncUuid, port }) - if (actualPort === undefined) continue + await syncUuidToBackend(set, get, MAIN_SERVER_UUID) set((state) => { - state.port[syncUuid] = String(actualPort) + state.selectedUuid = MAIN_SERVER_UUID }) + } else { + // TCP mode: existing flow + const uuidsToSync = uuid ? [uuid] : state.uuids - await syncUuidToBackend(set, get, syncUuid) - } + for (const syncUuid of uuidsToSync) { + const port = Number(state.port[syncUuid]) + const actualPort = await window.api.createServer({ uuid: syncUuid, port }) + if (actualPort === undefined) continue - if (state.uuids.length === 0) { - state.createServer({ port: 502, uuid: MAIN_SERVER_UUID }) - set((state) => { - state.ready[MAIN_SERVER_UUID] = true - }) + set((state) => { + state.port[syncUuid] = String(actualPort) + }) + + await syncUuidToBackend(set, get, syncUuid) + } + + if (state.uuids.length === 0) { + state.createServer({ port: 502, uuid: MAIN_SERVER_UUID }) + set((state) => { + state.ready[MAIN_SERVER_UUID] = true + }) + } } - } - get().cleanOrphanedServerState() + get().cleanOrphanedServerState() + } catch { + // Reported by main, and unawaited here. See the note above. + } finally { + set((state) => { + state.initialized = true + }) + } }, setSelectedUuid: (uuid) => set((state) => { @@ -763,6 +787,24 @@ readRtuServerStatus() /** Whether a split out server window has been the one writing the key. */ let serverWindowOwnsTheKey = false +/** + * Mark every uuid the rehydrate brought back as one main knows. + * + * `ready` is not in `partialize`, so a rehydrate brings back `uuids`, `port` + * and `selectedUuid` and leaves this window's own `ready` where `init` left it. + * A server made in the split out window therefore came back with no entry, and + * `setPort`, `setUnitId` and `setLittleEndian` each refuse on that with no + * message, until a restart. The window that made it ran `createServer` for it, + * so main does know it. + */ +const markRehydratedUuidsReady = (): void => { + const { uuids, ready } = useServerZustand.getState() + if (uuids.every((uuid) => ready[uuid])) return + useServerZustand.setState({ + ready: Object.fromEntries(uuids.map((uuid) => [uuid, true])) + }) +} + /** * Re-read the key the split out window has been writing. * @@ -781,6 +823,6 @@ onEvent('window_update', ({ server }) => { } if (!serverWindowOwnsTheKey) return serverWindowOwnsTheKey = false - useServerZustand.persist.rehydrate() + void Promise.resolve(useServerZustand.persist.rehydrate()).then(markRehydratedUuidsReady) readRtuServerStatus() }) diff --git a/src/renderer/src/context/server.zustand.types.ts b/src/renderer/src/context/server.zustand.types.ts index 5acd5170..db2daba2 100644 --- a/src/renderer/src/context/server.zustand.types.ts +++ b/src/renderer/src/context/server.zustand.types.ts @@ -63,7 +63,28 @@ export type ServerZustand = { configReset: ConfigReset | undefined /** Called once the reset has been reported, so it is reported once. */ acknowledgeConfigReset: () => void + /** + * Whether main knows this uuid, which is what `setPort`, `setUnitId` and + * `setLittleEndian` refuse on: a port main has not bound is a port nothing + * can be changed about. + * + * Written by `syncUuidToBackend`, by `createServer`, and by the + * `window_update` rehydrate for a uuid the split out window made and synced + * itself. `init` sets every uuid false before its loop, so a sync that fails + * leaves that one uuid refusing its three setters and no other. + */ ready: { [uuid: string]: boolean } + /** + * Whether `init` has run to its end, whatever it managed. + * + * `containers/Server.tsx` fades the whole server view in on this. It used to + * read `ready`, which is per uuid and written only on a sync that got + * through, so a rejected invoke anywhere in `init`'s loop left the view blank + * on that launch and on every one after it, with nothing on screen to clear + * the config with. The two questions are separate: this one is about the + * store, `ready` is about a server. + */ + initialized: boolean clean: (uuid: string) => void /** * Remove all state entries for uuids that are not present in the uuids array. From 9d8ca6eb261520c29639ba6973378f168b5a6c6c Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 07:34:06 +0200 Subject: [PATCH 07/19] fix: the generator's range, the string width and the register being replaced `/code-review` on `a301600` found six things. `min` and `max` stayed bare on the reasoning that a rule there would refuse what the Add button sends and cost the whole persisted register map, and both halves are measured false. `useMinMaxInteger` already masks both fields to `getMinMaxValues(dataType)` over the same seven types that `getValueRangeError` reads, so switching a generator from `uint32` to `uint16` rewrites a max of 100000 to 65535 and reports it valid. And store version 7 drops the one register rather than the map. The door was live: `{ dataType: 'uint16', max: 1e9 }` from a config file hands a draw of about 5e8 to `writeUInt16BE` inside an unawaited async tick, so the register answers 0 for as long as the generator runs and main takes an unhandled rejection every interval. `addRegister` disposed the generator it was replacing and zeroed its words before it tried the encode, so a refusal answered `undefined`, the store read that as nothing changed, and the grid went on drawing a generator that no longer ran. It encodes first. `length` was bounded by the map and not by the bound already stated for it. `MAX_UTF8_LENGTH` is that bound, 124, which `RegisterLengthInput` has always masked to and which the protocol puts a ceiling under either way, because FC3 and FC4 answer at most 125 registers. At 65536 one string made `addRegister` send 65536 `register_value` messages in one synchronous loop. `RemoveRegisterParamsSchema.length` takes the same bound, because `removeRegister` loops `registerWidth` times writing into the register array. The fit issue named `length` whenever a register carried one, and `registerWidth` reads `length` for `utf8` alone, so a `uint64` still carrying one from an earlier edit was refused at a field with no bearing on its width. `utils.ts` reads the three type modules rather than the barrel. The import `a301600` added made a cycle through `types/index.ts` that costs nothing only while every name in it is a type the compiler erases. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- CHANGELOG.md | 3 +- .../modules/__tests__/modbusServer.test.ts | 44 ++++++++++ src/main/modules/modbusServer.ts | 27 +++++-- .../AddRegister/maskedInputs.tsx | 4 +- src/shared/__tests__/schemas.test.ts | 81 ++++++++++++++++++- src/shared/types/server.ts | 61 +++++++++----- src/shared/utils.ts | 21 ++++- 7 files changed, 208 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4fa9d4c..38fc8ecb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **A server register the encoder cannot serve is now refused by name.** A config file could put a fixed value outside the range of its own data type, a string wider than the register map, a register running past the last address, - or a generator whose interval was so long it fired every millisecond instead. + a generator drawing between two numbers its data type cannot hold, or a + generator whose interval was so long it fired every millisecond instead. Opening it left the server view blank for that launch and every launch after it, with no message and nothing on screen to clear it with. The file is now refused with the register and the field named, and one already saved is diff --git a/src/main/modules/__tests__/modbusServer.test.ts b/src/main/modules/__tests__/modbusServer.test.ts index aaa59c1f..5621d936 100644 --- a/src/main/modules/__tests__/modbusServer.test.ts +++ b/src/main/modules/__tests__/modbusServer.test.ts @@ -315,6 +315,50 @@ describe('ModbusServer', () => { ) }) + // The replacement was taken apart before the encode was tried, so a + // refusal left the address zeroed and its generator gone while the store, + // reading `undefined` as nothing changed, went on drawing it. + it('leaves the register it was replacing running when it refuses', () => { + server.addRegister({ + uuid, + unitId, + params: { + address: 0, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: undefined, + min: 300, + max: 300, + interval: 5000 + } + }) + + const answer = server.addRegister({ + uuid, + unitId, + params: { + address: 0, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: 70000, + min: undefined, + max: undefined, + interval: undefined + } + }) + expect(answer).toBeUndefined() + + const sent = (windows.send as ReturnType).mock.calls.length + vi.advanceTimersByTime(6000) + const stillGenerating = (windows.send as ReturnType).mock.calls + .slice(sent) + .filter((call) => call[0] === 'register_value') + expect(stillGenerating.length).toBeGreaterThan(0) + expect(stillGenerating[0]?.[1]).toMatchObject({ address: 0, value: 300 }) + }) + // The `register_value` events above go out before this answer does, so the // renderer's store has no entry to put them in and drops them. The answer // is what that store folds instead. diff --git a/src/main/modules/modbusServer.ts b/src/main/modules/modbusServer.ts index 4a76046a..247ff822 100644 --- a/src/main/modules/modbusServer.ts +++ b/src/main/modules/modbusServer.ts @@ -545,22 +545,31 @@ export class ModbusServer { } const generators = serverGenerators[registerType] - const generator = generators.get(address) - generator?.dispose() - generators?.delete(address) - // Ensure server data map for this server and unitId - const serverData = this._unitData(uuid, unitId) - this._setServerData(uuid, unitId, serverData) + /** Frees the register this call is replacing, and answers its data map. */ + const takeTheAddress = (): ServerData => { + generators.get(address)?.dispose() + generators.delete(address) + const serverData = this._unitData(uuid, unitId) + this._setServerData(uuid, unitId, serverData) + return serverData + } // `none` is an address held open with nothing in it, so there is nothing to - // write and nothing to generate. The generator above is disposed either way, + // write and nothing to generate. The generator is disposed either way, // which is what editing a register to `none` has to do. - if (dataType === 'none') return [] + if (dataType === 'none') { + takeTheAddress() + return [] + } // If a fixed value is provided, set the register directly const fixedValue = !interval && value !== undefined if (fixedValue) { + // Encoded before the address is taken, because a refusal answers + // `undefined` and the store reads that as nothing changed. Disposing + // first zeroed the words of the generator being replaced and dropped it, + // so the grid went on drawing a generator that no longer ran. const registers = this._encode({ dataType, value, littleEndian, stringValue, length }) if (!registers) { this._emitMessage({ @@ -570,6 +579,7 @@ export class ModbusServer { return undefined } + const serverData = takeTheAddress() registers.forEach((register, index) => { const registerAddress = address + index serverData[registerType][registerAddress] = register @@ -590,6 +600,7 @@ export class ModbusServer { } // Otherwise, add a value generator for this register + const serverData = takeTheAddress() generators.set( address, new ValueGenerator({ diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/maskedInputs.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/maskedInputs.tsx index 306eac5c..77aaec7c 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/maskedInputs.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/maskedInputs.tsx @@ -11,7 +11,7 @@ import { meme } from '@renderer/components/shared/inputs/meme' import { MaskInputProps } from '@renderer/components/shared/inputs/types' import { forwardRef } from 'react' import { IMask, IMaskInput } from 'react-imask' -import { notEmpty, registerWidth } from '@shared' +import { MAX_UTF8_LENGTH, notEmpty, registerWidth } from '@shared' import { useMinMaxInteger } from '@renderer/hooks' const AddressInputForward = forwardRef((props, ref) => { @@ -159,7 +159,7 @@ const RegisterLengthForward = forwardRef((prop {...other} {...integerMask} min={1} - max={124} + max={MAX_UTF8_LENGTH} autofix inputRef={ref} onAccept={(value) => set(value, notEmpty(value))} diff --git a/src/shared/__tests__/schemas.test.ts b/src/shared/__tests__/schemas.test.ts index 1415b3df..a22c00e9 100644 --- a/src/shared/__tests__/schemas.test.ts +++ b/src/shared/__tests__/schemas.test.ts @@ -6,8 +6,9 @@ import { } from '../types/client' import { defaultConnectionConfig, defaultRegisterConfig } from '../default' import { BitColorSchema, BitMapEntrySchema, BitMapConfigSchema } from '../types/bitmap' -import { RegisterParamsSchema } from '../types/server' +import { RegisterParamsSchema, RemoveRegisterParamsSchema } from '../types/server' import { DataBitsSchema, SerialPortOptionsSchema, StopBitsSchema } from '../types/serial' +import { MAX_UTF8_LENGTH } from '../utils' describe('RegisterMapObjectSchema', () => { it('accepts numeric string keys', () => { @@ -361,10 +362,10 @@ describe('Server RegisterParamsSchema — the width and the value', () => { ).toBe(false) }) - it('accepts a string that fills the map from address 0', () => { + it('accepts the widest string the dialog offers', () => { expect( RegisterParamsSchema.safeParse( - fixed({ dataType: 'utf8', length: 65536, stringValue: 'x', value: 0 }) + fixed({ dataType: 'utf8', length: MAX_UTF8_LENGTH, stringValue: 'x', value: 0 }) ).success ).toBe(true) }) @@ -414,6 +415,80 @@ describe('Server RegisterParamsSchema — the width and the value', () => { expect(result.success).toBe(false) expect(result.error?.issues[0]?.path).toEqual(['value']) }) + + // `registerWidth` reads `length` for `utf8` alone, so naming it for a + // `uint64` that still carries one from an earlier edit points at a field with + // no bearing on the width. + it('names the address when the width is not the length', () => { + const result = RegisterParamsSchema.safeParse( + fixed({ address: 65534, dataType: 'uint64', value: 1, length: 4 }) + ) + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.path).toEqual(['address']) + }) + + // `removeRegister` loops `registerWidth(dataType, length)` times writing into + // the register array, so the remove channel needs the bound add and sync have. + it('holds a string to the same width on add, sync and remove', () => { + const tooWide = { dataType: 'utf8', length: 125, stringValue: 'x', value: 0 } + expect(RegisterParamsSchema.safeParse(fixed(tooWide)).success).toBe(false) + expect( + RemoveRegisterParamsSchema.safeParse({ + uuid: 'u', + unitId: '1', + registerType: 'holding_registers', + address: 0, + dataType: 'utf8', + length: 125 + }).success + ).toBe(false) + expect( + RegisterParamsSchema.safeParse(fixed({ ...tooWide, length: MAX_UTF8_LENGTH })).success + ).toBe(true) + }) +}) + +// The generator draws between `min` and `max` and hands the draw to +// `createRegisters`, inside an `async` tick nobody awaits, so a range the data +// type cannot take answers 0 for as long as the generator runs and warns once +// per interval. The dialog masks both fields to `getMinMaxValues(dataType)` +// already, so the rule refuses nothing the Add button sends. +describe('Server RegisterParamsSchema — what a generator draws between', () => { + const generator = (overrides: Record): unknown => ({ + address: 10, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + min: 0, + max: 100, + interval: 1000, + ...overrides + }) + + it.each([ + ['max', 1e9], + ['max', 65536], + ['min', -1] + ])('refuses %s of %s on a uint16', (field, bound) => { + const result = RegisterParamsSchema.safeParse(generator({ [field]: bound })) + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.path).toEqual([field]) + }) + + it('takes a range the data type holds', () => { + expect(RegisterParamsSchema.safeParse(generator({ min: 0, max: 65535 })).success).toBe(true) + expect( + RegisterParamsSchema.safeParse(generator({ dataType: 'uint32', max: 1e9 })).success + ).toBe(true) + }) + + // `float` and `double` write anything, so the pair stays open there. + it('leaves a float generator alone', () => { + expect( + RegisterParamsSchema.safeParse(generator({ dataType: 'float', min: -1e300, max: 1e300 })) + .success + ).toBe(true) + }) }) describe('SerialPortOptionsSchema', () => { diff --git a/src/shared/types/server.ts b/src/shared/types/server.ts index 8243f842..4b282628 100644 --- a/src/shared/types/server.ts +++ b/src/shared/types/server.ts @@ -4,7 +4,7 @@ import { BitMapConfigSchema } from './bitmap' import { PortSchema, RegisterAddressKeySchema, RegisterAddressSchema } from './ranges' import { SerialPortOptionsSchema } from './serial' import { UnitIdString, UnitIdStringSchema } from './unitid' -import { getAddressFitError, getValueRangeError } from '../utils' +import { getAddressFitError, getValueRangeError, MAX_UTF8_LENGTH } from '../utils' import { BooleanRegisters, BooleanRegistersSchema, @@ -51,11 +51,25 @@ export type StartRtuServerParams = z.infer * 1e12 fires every millisecond, which is the flood the floor was written to * prevent. * - * `min` and `max` are each a bare number and stay one. The dialog lets a min - * above a max through, and `Math.random() * (max - min) + min` covers the same - * range either way: ten thousand draws of min 100 max 10 ran 10 to 100, the - * same as min 10 max 100. A rule here would refuse a payload the Add button - * sends and cost the whole persisted register map on the next launch. + * `min` and `max` are each held to their own data type, in the refine below, + * because the generator draws between them and hands the draw to + * `createRegisters`. `ValueGenerator._updateValue` is `async` and nobody awaits + * it, so a draw the type cannot encode is not a throw the caller sees: the + * register answers 0 for as long as the generator runs and main takes an + * unhandled rejection every interval. Electron 43 stays alive through that, + * which is why it is quiet rather than loud. + * + * The reason they were bare was that a rule here would refuse what the Add + * button sends and cost the whole persisted register map, and both halves were + * measured false. `useMinMaxInteger` already masks both fields to + * `getMinMaxValues(dataType)` over the same seven types, so switching a + * generator from `uint32` to `uint16` rewrites a max of 100000 to 65535 and + * reports it valid. And one register the schema refuses costs that register + * now, because store version 7 drops it and keeps the rest. + * + * A min above a max is not a rule here. The dialog lets one through, and + * `Math.random() * (max - min) + min` covers the same range either way: ten + * thousand draws of min 100 max 10 ran 10 to 100, the same as min 10 max 100. */ const RegisterParamsGeneratorPartSchema = z.object({ min: z.number(), @@ -96,14 +110,15 @@ export type RegisterValue = { * register is read off. Left bare, `length: 1e12` passed here and reached * `createStringRegisters`, which is `Buffer.alloc(2e12)`: inside Electron 43 * that answers ERR_OUT_OF_RANGE, and `length: 1e9` allocates and then builds a - * billion words instead. `getUsedAddresses` loops the same number. + * billion words instead. `getUsedAddresses` loops the same number. The ceiling + * is `MAX_UTF8_LENGTH`, which `RegisterLengthInput` masks to. */ export const RegisterParamsBasePartSchema = z.object({ address: RegisterAddressSchema, registerType: NumberRegistersSchema, dataType: BaseDataTypeSchema, comment: z.string(), - length: z.number().int().min(1).optional(), + length: z.number().int().min(1).max(MAX_UTF8_LENGTH).optional(), stringValue: z.string().optional(), bitMap: BitMapConfigSchema.optional() }) @@ -122,26 +137,34 @@ export type RegisterParamsBasePart = z.infer { if (getAddressFitError(params.dataType, params.address, params.length)) { + // `registerWidth` reads `length` for `utf8` alone, so naming it for any + // other type points at a field with no bearing on the width. A `uint64` + // that still carries a `length` from an earlier edit is one of those. ctx.addIssue({ code: z.ZodIssueCode.custom, - path: [params.length === undefined ? 'address' : 'length'], + path: [params.dataType === 'utf8' ? 'length' : 'address'], message: `A ${params.dataType} register at ${params.address} runs past address 65535` }) } - if (params.value === undefined) return - - const rangeError = getValueRangeError(params.dataType, params.value) - if (rangeError) { - ctx.addIssue({ code: z.ZodIssueCode.custom, path: ['value'], message: rangeError }) + for (const field of ['value', 'min', 'max'] as const) { + const bound = params[field] + if (bound === undefined) continue + const rangeError = getValueRangeError(params.dataType, bound) + if (rangeError) { + ctx.addIssue({ code: z.ZodIssueCode.custom, path: [field], message: rangeError }) + } } }) export type RegisterParams = z.infer @@ -227,7 +250,9 @@ export const RemoveRegisterParamsSchema = z.object({ dataType: BaseDataTypeSchema, // Only a string has a width the user chose, and without it the server has to // guess how much of the map the register occupied and erases the guess. - length: z.number().optional() + // Bounded the way add and sync bound it: `removeRegister` loops + // `registerWidth(dataType, length)` times writing into the register array. + length: z.number().int().min(1).max(MAX_UTF8_LENGTH).optional() }) export type RemoveRegisterParams = z.infer diff --git a/src/shared/utils.ts b/src/shared/utils.ts index 8cb40ff4..b68a43b1 100644 --- a/src/shared/utils.ts +++ b/src/shared/utils.ts @@ -1,4 +1,10 @@ -import { CamelCase, DataType, EncodableDataType, RegisterParams, ServerRegisters } from './types' +// `types/server.ts` imports `getAddressFitError` and `getValueRangeError` from +// here, so this reads the barrel no longer: it would be a cycle through +// `types/index.ts`, and one that costs nothing only while every name below is a +// type the compiler erases. `import type` says which of the two these are. +import type { CamelCase } from './types/utils' +import type { DataType, EncodableDataType } from './types/datatype' +import type { RegisterParams, ServerRegisters } from './types/server' /** * Whether bit `bit` of `word` is set. @@ -13,6 +19,19 @@ export const getBit = (word: number, bit: number): boolean => Math.floor(word / /** The width the add dialog offers for a string when the field is left alone. */ export const DEFAULT_UTF8_LENGTH = 10 +/** + * The widest string a register map holds, which is the add dialog's own mask. + * + * The protocol puts a ceiling under it either way: FC3 and FC4 answer at most + * 125 registers, so a string wider than that cannot be read in one request. + * The number here is the one `RegisterLengthInput` has always masked to, and + * `RegisterParamsBasePartSchema` states it too, because a config file is the + * other door and the map is the only thing that bounded it there: one string of + * 65536 made `addRegister` send 65536 `register_value` messages in one + * synchronous loop. + */ +export const MAX_UTF8_LENGTH = 124 + /** * How many registers a value of this type occupies. * From 946830ef46dc2984d096b2af506e6eae2ca9ebfa Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 07:37:55 +0200 Subject: [PATCH 08/19] fix: a server config file is read before the server is emptied `useOpen` awaited `resetServer(selectedUuid)` above its `try`, and `resetServer` reaches main and then clears the store. The parse came after. `migrateServerConfig` throws on both shapes a user picks by accident, a client config file and malformed JSON, and the `catch` only enqueues a snackbar, so the configuration that was on screen was gone in main and in the store before anything had read the file. The reset moves inside the `try`, after the migration returns. `migrateServerConfig(content)` needs nothing from the store, and the reason the reset exists, that a unit the file does not name must not keep answering a master, is kept by doing it later rather than first. `file.text()` moves in with it, and the flag it sets comes off in a `finally`, because a file that is gone by the time it is read left `opening` true and every server button disabled until the app was restarted. `05-file-io` opens `client-basic.json` into the server view. Without the change it reads `Holding Registers (0)` where the config it just loaded had two. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- CHANGELOG.md | 4 ++ e2e/specs/01-main/05-file-io.spec.ts | 30 +++++++++++++++ .../src/components/server/OpenSaveClear.tsx | 37 ++++++++++++------- 3 files changed, 58 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38fc8ecb..cbfe6897 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Opening the wrong file no longer empties the server.** Picking a client + config, or any file Modbux cannot read as a server config, cleared the setup + that was on screen before anything had read the file, and then reported the + failure. The file is read first now, and a refused one costs nothing. - **The server view no longer comes up blank and stays that way.** One thing main refused while the setup was being handed over left the whole view faded out, on that launch and on every launch after it, with no message and nothing on diff --git a/e2e/specs/01-main/05-file-io.spec.ts b/e2e/specs/01-main/05-file-io.spec.ts index 5a525fcc..5e867207 100644 --- a/e2e/specs/01-main/05-file-io.spec.ts +++ b/e2e/specs/01-main/05-file-io.spec.ts @@ -48,6 +48,36 @@ test.describe.serial('File I/O — open, save, clear server and client configs', await mainPage.waitForTimeout(300) }) + // ─── A file the server cannot read ──────────────────────────────────── + + // `useOpen` awaited `resetServer` above its `try`, and `resetServer` reaches + // main and then clears the store. `migrateServerConfig` throws on both shapes + // a user picks by accident, a client config file and malformed JSON, and the + // `catch` only enqueues a snackbar, so the configuration on screen was gone + // before anything had read the file. + test('opening a client config into the server says so and keeps the config', async ({ + mainPage + }) => { + const fileInput = mainPage.getByTestId('server-open-file-input') + await fileInput.setInputFiles(CONFIG_FILES.clientBasic) + + await expect(mainPage.locator('.notistack-SnackbarContainer')).toContainText( + 'Failed to load config', + { timeout: 5000 } + ) + + await expect(mainPage.getByTestId('server-name-input').locator('input')).toHaveValue( + 'Basic Server' + ) + await expect(mainPage.getByTestId('section-holding_registers')).toContainText('(2)') + await expect(mainPage.getByTestId('section-input_registers')).toContainText('(1)') + }) + + test('the buttons come back after a file it could not read', async ({ mainPage }) => { + await expect(mainPage.getByTestId('server-clear-btn')).toBeEnabled() + await expect(mainPage.getByTestId('server-save-btn')).toBeEnabled() + }) + // ─── Server config clear ────────────────────────────────────────────── test('clear server config — verify all sections show (0)', async ({ mainPage }) => { diff --git a/src/renderer/src/components/server/OpenSaveClear.tsx b/src/renderer/src/components/server/OpenSaveClear.tsx index 26f7817d..9dba1650 100644 --- a/src/renderer/src/components/server/OpenSaveClear.tsx +++ b/src/renderer/src/components/server/OpenSaveClear.tsx @@ -42,18 +42,24 @@ const useOpen: UseOpenHook = () => { const serverZustand = useServerZustand.getState() - // A unit the file does not name is not written over on the way in, so - // whatever the previous config left on it would answer a master after - // this one is loaded. Reset takes both sides down to nothing first. - await serverZustand.resetServer(serverZustand.selectedUuid) - - const content = await file.text() - try { + const content = await file.text() + // Use migration framework to handle all config versions const migrationResult = migrateServerConfig(content) const { config, migrated, warning, wasMixedEndianness } = migrationResult + // A unit the file does not name is not written over on the way in, so + // whatever the previous config left on it would answer a master after + // this one is loaded. Reset takes both sides down to nothing first. + // + // After the migration rather than before it, because `migrateServerConfig` + // needs nothing from the store and throws on both shapes a user picks by + // accident, a client config file and malformed JSON. Reset first, and the + // configuration on screen was gone in main and in the store before + // anything had read the file. + await serverZustand.resetServer(serverZustand.selectedUuid) + // Set name and littleEndian serverZustand.setName(config.name) serverZustand.setLittleEndian(config.littleEndian) @@ -103,13 +109,18 @@ const useOpen: UseOpenHook = () => { const tError = error as Error enqueueSnackbar({ variant: 'error', message: `Failed to load config: ${tError.message}` }) console.error('Config load error:', error) + } finally { + // In the `finally` because `file.text()` is inside the `try` now, and a + // file that is gone by the time it is read left `opening` true and + // every server button disabled until the app was restarted. + // + // `init` runs either way: on a refused file it hands main the + // configuration that is still there, which is the one on screen. + await serverZustand.init(serverZustand.selectedUuid) + + openingRef.current = false + setOpening(false) } - - // Synchronize only the selected server after opening the configuration - await serverZustand.init(serverZustand.selectedUuid) - - openingRef.current = false - setOpening(false) }, [enqueueSnackbar] ) From 80455526f9fde963e30fa2166e4c07701e2faf99 Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 07:41:47 +0200 Subject: [PATCH 09/19] fix: a file that is not a configuration is refused rather than migrated Both migrations opened with `JSON.parse(raw)` and then `parsed.version ?? 1`. On a number, a string, a boolean or an array that is `undefined`, so 1, so the v1 to v2 step ran, and every field the server migration needs sits behind a `??`. So `5`, `[]`, `"hello"` and `true` each came out a valid empty v2 config, and the user was told the configuration had been updated from an older format. `null` reached them as `Cannot read properties of null (reading 'version')`. The client migration refused the same input, but only because `migrateClientV1toV2` feeds the whole blob into `registerMapping` and the final `safeParse` catches it, which is an accident of that one field. `parseConfigFile` is the guard, before `.version` is read, in both. It answers the version as well, and that half was not in the finding. `JSON.parse` answers `any`, so nothing had asked what was in that field, and a `version` of `"3"` or `true` took the v1 path with everything else. Typing the blob as a record of `unknown` is what surfaced it: six lines of `migrateServerConfig` and `migrateClientConfig` stopped compiling. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- CHANGELOG.md | 5 ++++ src/shared/__tests__/migrations.test.ts | 28 ++++++++++++++++++ src/shared/migrations/client/config.ts | 5 ++-- src/shared/migrations/server/config.ts | 13 ++++++--- src/shared/migrations/shared.ts | 38 +++++++++++++++++++++++++ 5 files changed, 82 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cbfe6897..3b7f80d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A file that is not a configuration is now refused instead of opened.** + Picking a JSON file holding a number, a string, a list or nothing at all + emptied the server and reported that the configuration had been updated from + an older format. A file claiming a version that is not a number took the same + path. - **Opening the wrong file no longer empties the server.** Picking a client config, or any file Modbux cannot read as a server config, cleared the setup that was on screen before anything had read the file, and then reported the diff --git a/src/shared/__tests__/migrations.test.ts b/src/shared/__tests__/migrations.test.ts index cc14ca49..34753862 100644 --- a/src/shared/__tests__/migrations.test.ts +++ b/src/shared/__tests__/migrations.test.ts @@ -197,6 +197,34 @@ describe('configMigration', () => { }) }) + // `parsed.version ?? 1` on any of these is `undefined`, so 1, so the v1 to + // v2 step ran, and every field the migration needs sits behind a `??`. Each + // came out a valid empty v2 config, reported as updated from an older + // format, and `useOpen` had already emptied the server by then. + describe('A file that is not a configuration', () => { + it.each(['5', '[]', '"hello"', 'true', 'null'])('refuses %s', (raw) => { + expect(() => migrateServerConfig(raw)).toThrow( + 'This file does not hold a Modbux configuration' + ) + expect(() => migrateClientConfig(raw)).toThrow( + 'This file does not hold a Modbux configuration' + ) + }) + + // `JSON.parse` answers `any`, so nothing asked what was in this field and + // a version that is not one took the v1 path with the rest. + it.each(['"3"', 'true', '2.5', 'null'])('refuses a version of %s', (version) => { + const raw = `{"version": ${version}, "serverRegistersPerUnit": {}}` + expect(() => migrateServerConfig(raw)).toThrow('which is not a version') + }) + + it('still takes a file with no version at all', () => { + const result = migrateServerConfig(JSON.stringify({ name: 'v1', coils: {} })) + expect(result.fromVersion).toBe(1) + expect(result.migrated).toBe(true) + }) + }) + // A refused file is refused whole, so the message is the only thing that // says which register to go and fix. describe('What a refused config names', () => { diff --git a/src/shared/migrations/client/config.ts b/src/shared/migrations/client/config.ts index ac41eb35..36377a4f 100644 --- a/src/shared/migrations/client/config.ts +++ b/src/shared/migrations/client/config.ts @@ -1,6 +1,6 @@ import { RegisterMapConfigSchema, RegisterMapConfig, RegisterMapping } from '../../types/client' import { MigrationResult, Migration } from '../types' -import { formatZodError, renameLegacyRegisterTypeKeys } from '../shared' +import { formatZodError, parseConfigFile, renameLegacyRegisterTypeKeys } from '../shared' /** * The version the Save button writes into a client config file. @@ -42,8 +42,7 @@ function migrateClientV1toV2(v1Config: unknown): RegisterMapConfig { * Migrate client config to current version */ export function migrateClientConfig(raw: string): MigrationResult { - const parsed = JSON.parse(raw) - const detectedVersion = parsed.version ?? 1 + const { parsed, detectedVersion } = parseConfigFile(raw) // Current version - no migration needed if (detectedVersion === CURRENT_CLIENT_CONFIG_VERSION) { diff --git a/src/shared/migrations/server/config.ts b/src/shared/migrations/server/config.ts index 070b2504..dd035216 100644 --- a/src/shared/migrations/server/config.ts +++ b/src/shared/migrations/server/config.ts @@ -11,6 +11,7 @@ import { formatZodError, migrateBoolShapeForUnit, objectValues, + parseConfigFile, renameLegacyRegisterTypeKeys } from '../shared' import { V1ServerConfig, extractGlobalEndianness } from './shared' @@ -118,8 +119,7 @@ function migrateBoolShapeInConfig(config: Record): void { * Migrate server config to current version */ export function migrateServerConfig(raw: string): MigrationResult { - const parsed = JSON.parse(raw) - const detectedVersion = parsed.version ?? 1 + const { parsed, detectedVersion } = parseConfigFile(raw) // Current version - migrate bool shape if needed, then validate if (detectedVersion === CURRENT_SERVER_CONFIG_VERSION) { @@ -166,11 +166,16 @@ export function migrateServerConfig(raw: string): MigrationResult throw new Error(`Migration produced invalid config: ${formatZodError(result.error)}`) } + // `migrateServerV1toV2` sets this flag, and `parseConfigFile` types the blob + // it reads as a record of `unknown` rather than the `any` `JSON.parse` gave, + // so the flag has to be read as the boolean it is. + const wasMixedEndianness = config.wasMixedEndianness === true + return { config: result.data, migrated: true, fromVersion: detectedVersion, - warning: config.wasMixedEndianness ? 'MIXED_ENDIANNESS' : undefined, - wasMixedEndianness: config.wasMixedEndianness + warning: wasMixedEndianness ? 'MIXED_ENDIANNESS' : undefined, + wasMixedEndianness } } diff --git a/src/shared/migrations/shared.ts b/src/shared/migrations/shared.ts index 64983805..b9a368c3 100644 --- a/src/shared/migrations/shared.ts +++ b/src/shared/migrations/shared.ts @@ -31,6 +31,44 @@ export function repairPersistedParity(state: Record, ...path: s const isRecord = (value: unknown): value is Record => typeof value === 'object' && value !== null +/** + * What a config file has to be, and the version it claims, for either migration. + * + * Both opened with `JSON.parse(raw)` and then `parsed.version ?? 1`. On a + * number, a string, a boolean or an array that is `undefined`, so 1, so the v1 + * to v2 step ran, and every field the server migration needs sits behind a + * `??`, so `5`, `[]`, `"hello"` and `true` each came out a valid empty v2 + * config and the user was told the configuration had been updated from an older + * format. `null` reached them as + * `Cannot read properties of null (reading 'version')` instead. The client + * migration refuses the same input, but only because `migrateClientV1toV2` + * feeds the whole blob into `registerMapping` and the final `safeParse` catches + * it, which is an accident of that one field rather than a rule. + * + * An array is refused with the rest: `Array.isArray` is the half `typeof` does + * not cover, and a config is a map of named fields either way. + * + * The version is checked here too. `JSON.parse` answers `any`, so nothing had + * asked what was in that field, and a `version` of `"3"` or `true` took the v1 + * path with the rest. + */ +export function parseConfigFile(raw: string): { + parsed: Record + detectedVersion: number +} { + const parsed: unknown = JSON.parse(raw) + if (!isRecord(parsed) || Array.isArray(parsed)) { + throw new Error('This file does not hold a Modbux configuration') + } + + const version = parsed.version + if (version !== undefined && (typeof version !== 'number' || !Number.isInteger(version))) { + throw new Error(`This file claims version ${JSON.stringify(version)}, which is not a version`) + } + + return { parsed, detectedVersion: version ?? 1 } +} + /** The object entries of `value`, and nothing at all when it is not an object. */ const recordEntries = (value: unknown): [string, Record][] => isRecord(value) From 15c36932d59ac213e7dc5f0b89f35d66e9c7e80a Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 07:45:49 +0200 Subject: [PATCH 10/19] fix: a server main refuses costs that server and no other `/code-review` on `53fcc7c` found four things, and the first of them means that commit did not do what it said. The `try` stood around the whole loop, so a refusal for the first uuid threw out of it and `createServer` was never called for the second at all: no listener on its port, none of its registers in main, and `ready` false with no message. The commit message and the changelog both claimed a refusal cost the one server it belonged to, and now it does. The catch sits in `openServer`, per uuid. That `try` also covered three mutative recipes and `cleanOrphanedServerState`, and swallowed a throw out of any of them with no line anywhere. The narrowed one covers the invokes its comment describes, and prints what it caught, because a throw out of the store's own recipes is the other kind and nothing else says so. `markRehydratedUuidsReady` wrote `true` over every uuid rather than the ones the rehydrate introduced. A uuid this window's own `init` wrote `false` for is one main refused, and handing the three setters back for a server main has no listener for is the defect in the other direction. It writes only where the entry is missing. The rehydrate chain takes a `catch`. `rehydrate` reads the key and runs `migrateServerState` over what it finds, and either can throw on a blob a hand edit left there. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- .../__tests__/serverStoreRehydrate.test.ts | 17 ++ .../serverViewDrawsAfterInit.test.ts | 53 ++++++ src/renderer/src/context/server.zustand.ts | 170 ++++++++++-------- 3 files changed, 170 insertions(+), 70 deletions(-) diff --git a/src/renderer/src/context/__tests__/serverStoreRehydrate.test.ts b/src/renderer/src/context/__tests__/serverStoreRehydrate.test.ts index ff402bf6..bf61a7bb 100644 --- a/src/renderer/src/context/__tests__/serverStoreRehydrate.test.ts +++ b/src/renderer/src/context/__tests__/serverStoreRehydrate.test.ts @@ -129,6 +129,23 @@ describe('the server window closing', () => { expect(useServerZustand.getState().unitId['made-in-the-split-window']).toBe('7') }) + // A uuid this window's own `init` wrote `false` for is one main refused, and + // writing `true` over it hands the three setters back for a server main has + // no listener for. + it('leaves a uuid this window already knows about alone', async () => { + localStorage.setItem(SERVER_ZUSTAND_STORAGE_KEY, store([0])) + const { useServerZustand } = await import('../server.zustand') + await settle() + + useServerZustand.setState({ ready: { u: false } }) + + fireEvent('window_update', { main: true, server: true }) + fireEvent('window_update', { main: true, server: false }) + await settle() + + expect(useServerZustand.getState().ready.u).toBe(false) + }) + // `rtuServerActive` is not persisted, so re-reading the key leaves it where // it was, and the split out window is the one that heard the last change. it('asks main for the RTU status, at load and at the close', async () => { diff --git a/src/renderer/src/context/__tests__/serverViewDrawsAfterInit.test.ts b/src/renderer/src/context/__tests__/serverViewDrawsAfterInit.test.ts index cb28b833..207547d7 100644 --- a/src/renderer/src/context/__tests__/serverViewDrawsAfterInit.test.ts +++ b/src/renderer/src/context/__tests__/serverViewDrawsAfterInit.test.ts @@ -118,3 +118,56 @@ describe('the flag the server view draws on', () => { expect(useServerZustand.getState().initialized).toBe(true) }) }) + +// The `try` stood around the whole loop, so a refusal for the first uuid threw +// out of it and `createServer` was never called for the second: no listener on +// its port, none of its registers in main, and `ready` false with no message. +describe('one server main refuses', () => { + const SECOND = 'a-second-server' + + /** Records which uuids reached `createServer`, and refuses the first one. */ + const refuseTheFirstSync = (created: string[]): void => { + const boundary = window.api as unknown as Record + window.api = new Proxy( + {}, + { + get: (_target, method: string): unknown => { + if (method === 'isServerWindow') return boundary[method] + if (method === 'createServer') { + return (params: { uuid: string; port: number }): Promise => { + created.push(params.uuid) + return Promise.resolve(params.port) + } + } + if (method === 'setServerEndianness') { + return (params: { uuid: string }): Promise => + params.uuid === MAIN_SERVER_UUID + ? Promise.reject(new Error('setServerEndianness threw')) + : Promise.resolve() + } + return boundary[method] + } + } + ) as never + } + + it('costs that server and no other', async () => { + const blob = JSON.parse(persisted()) + blob.state.uuids.push(SECOND) + blob.state.port[SECOND] = '503' + blob.state.unitId[SECOND] = '0' + blob.state.littleEndian[SECOND] = false + blob.state.serverRegisters[SECOND] = {} + localStorage.setItem(SERVER_ZUSTAND_STORAGE_KEY, JSON.stringify(blob)) + + const created: string[] = [] + refuseTheFirstSync(created) + const { useServerZustand } = await import('../server.zustand') + await settle() + + expect(created).toEqual([MAIN_SERVER_UUID, SECOND]) + expect(useServerZustand.getState().ready[MAIN_SERVER_UUID]).toBe(false) + expect(useServerZustand.getState().ready[SECOND]).toBe(true) + expect(useServerZustand.getState().initialized).toBe(true) + }) +}) diff --git a/src/renderer/src/context/server.zustand.ts b/src/renderer/src/context/server.zustand.ts index 1fe8b120..305c6c20 100644 --- a/src/renderer/src/context/server.zustand.ts +++ b/src/renderer/src/context/server.zustand.ts @@ -141,92 +141,111 @@ export const useServerZustand = create< get().clean(uuid) }, /** - * Hands main what the store holds, and marks the store initialized - * whatever came of that. + * Hands main what the store holds, one server at a time, and marks the + * store initialized once it has been through them all. * * `containers/Server.tsx` draws nothing until the flag is set, so a - * rejected invoke below used to cost the whole server view rather than - * the one uuid it belonged to, on that launch and on every one after it. - * The uuid keeps `ready` false, which is what its three setters refuse - * on, and that is the whole cost now. - * - * The `catch` is empty because this runs from module scope with nothing - * awaiting it, and a rejection there is an unhandled one. Main reports - * its own failures through `backend_message`; a rejected invoke carries - * the channel name and nothing the user can act on. + * rejected invoke used to cost the whole server view rather than the one + * uuid it belonged to, on that launch and on every one after it. The uuid + * keeps `ready` false, which is what its three setters refuse on, and + * that is the whole cost now. `openServer` is where the catch sits, so + * the servers after it still get opened. */ init: async (uuid) => { - try { - set((state) => { - if (uuid) state.ready[uuid] = false - else for (const u of state.uuids) state.ready[u] = false - }) - const state = get() - const mode = state.serverMode ?? 'tcp' + set((state) => { + if (uuid) state.ready[uuid] = false + else for (const u of state.uuids) state.ready[u] = false + }) + const state = get() + const mode = state.serverMode ?? 'tcp' - // Ensure every uuid has a unitId and littleEndian entry (for backward compatibility) - set((state) => { - for (const uuid of state.uuids) { - if (state.unitId[uuid] === undefined) { - state.unitId[uuid] = '0' - } - if (state.littleEndian[uuid] === undefined) { - state.littleEndian[uuid] = false // Default to Big-Endian - } + // Ensure every uuid has a unitId and littleEndian entry (for backward compatibility) + set((state) => { + for (const uuid of state.uuids) { + if (state.unitId[uuid] === undefined) { + state.unitId[uuid] = '0' } - }) - - if (mode === 'rtu') { - // RTU serves the main server's registers, so that is the only uuid - // there is anything to open for. - const serialConfig = state.serialConfig ?? getDefaultSerialConfig() - - // Only start if COM port is configured - if (serialConfig.com.trim()) { - try { - await window.api.startRtuServer({ uuid: MAIN_SERVER_UUID, serialConfig }) - } catch { - // Error is reported via backend_message event - } + if (state.littleEndian[uuid] === undefined) { + state.littleEndian[uuid] = false // Default to Big-Endian } + } + }) - await syncUuidToBackend(set, get, MAIN_SERVER_UUID) + /** + * Opens one server and hands main what it holds, and answers whether + * that got through. + * + * The `catch` is per uuid, because that is the unit of the cost: it + * stood around the loop, and a refusal for the first uuid then meant + * `createServer` was never called for the second at all, so it had no + * listener on its port, none of its registers in main, and `ready` + * false with no message. + * + * Empty, because this runs from module scope with nothing awaiting it + * and a rejection there is an unhandled one. Main reports its own + * failures through `backend_message`; a rejected invoke carries the + * channel name and nothing the user can act on. `console.error` is what + * says the other kind happened, a throw out of the store's own recipes. + */ + const openServer = async (syncUuid: string, port: number): Promise => { + try { + const actualPort = await window.api.createServer({ uuid: syncUuid, port }) + if (actualPort === undefined) return set((state) => { - state.selectedUuid = MAIN_SERVER_UUID + state.port[syncUuid] = String(actualPort) }) - } else { - // TCP mode: existing flow - const uuidsToSync = uuid ? [uuid] : state.uuids - for (const syncUuid of uuidsToSync) { - const port = Number(state.port[syncUuid]) - const actualPort = await window.api.createServer({ uuid: syncUuid, port }) - if (actualPort === undefined) continue - - set((state) => { - state.port[syncUuid] = String(actualPort) - }) + await syncUuidToBackend(set, get, syncUuid) + } catch (error) { + console.error(`Server ${syncUuid} was not opened:`, error) + } + } - await syncUuidToBackend(set, get, syncUuid) + if (mode === 'rtu') { + // RTU serves the main server's registers, so that is the only uuid + // there is anything to open for. + const serialConfig = state.serialConfig ?? getDefaultSerialConfig() + + // Only start if COM port is configured + if (serialConfig.com.trim()) { + try { + await window.api.startRtuServer({ uuid: MAIN_SERVER_UUID, serialConfig }) + } catch { + // Error is reported via backend_message event } + } - if (state.uuids.length === 0) { - state.createServer({ port: 502, uuid: MAIN_SERVER_UUID }) - set((state) => { - state.ready[MAIN_SERVER_UUID] = true - }) - } + try { + await syncUuidToBackend(set, get, MAIN_SERVER_UUID) + } catch (error) { + console.error(`Server ${MAIN_SERVER_UUID} was not opened:`, error) } - get().cleanOrphanedServerState() - } catch { - // Reported by main, and unawaited here. See the note above. - } finally { set((state) => { - state.initialized = true + state.selectedUuid = MAIN_SERVER_UUID }) + } else { + // TCP mode: existing flow + const uuidsToSync = uuid ? [uuid] : state.uuids + + for (const syncUuid of uuidsToSync) { + await openServer(syncUuid, Number(state.port[syncUuid])) + } + + if (state.uuids.length === 0) { + state.createServer({ port: 502, uuid: MAIN_SERVER_UUID }) + set((state) => { + state.ready[MAIN_SERVER_UUID] = true + }) + } } + + get().cleanOrphanedServerState() + + set((state) => { + state.initialized = true + }) }, setSelectedUuid: (uuid) => set((state) => { @@ -788,7 +807,7 @@ readRtuServerStatus() let serverWindowOwnsTheKey = false /** - * Mark every uuid the rehydrate brought back as one main knows. + * Mark the uuids the rehydrate introduced as ones main knows. * * `ready` is not in `partialize`, so a rehydrate brings back `uuids`, `port` * and `selectedUuid` and leaves this window's own `ready` where `init` left it. @@ -796,12 +815,18 @@ let serverWindowOwnsTheKey = false * `setPort`, `setUnitId` and `setLittleEndian` each refuse on that with no * message, until a restart. The window that made it ran `createServer` for it, * so main does know it. + * + * Only where the entry is missing. A uuid this window's own `init` wrote + * `false` for is one main refused, and writing `true` over that would hand the + * three setters back for a server main has no listener for, and flip the flag + * `PrivilegedPortModal` runs its check on. */ const markRehydratedUuidsReady = (): void => { const { uuids, ready } = useServerZustand.getState() - if (uuids.every((uuid) => ready[uuid])) return + const introduced = uuids.filter((uuid) => ready[uuid] === undefined) + if (introduced.length === 0) return useServerZustand.setState({ - ready: Object.fromEntries(uuids.map((uuid) => [uuid, true])) + ready: { ...ready, ...Object.fromEntries(introduced.map((uuid) => [uuid, true])) } }) } @@ -823,6 +848,11 @@ onEvent('window_update', ({ server }) => { } if (!serverWindowOwnsTheKey) return serverWindowOwnsTheKey = false - void Promise.resolve(useServerZustand.persist.rehydrate()).then(markRehydratedUuidsReady) + void Promise.resolve(useServerZustand.persist.rehydrate()) + .then(markRehydratedUuidsReady) + // `rehydrate` reads the key and runs `migrateServerState` over what it + // finds, and either can throw on a blob a hand edit left there. Unhandled, + // that is the window's only sign that the re-read did not happen. + .catch((error) => console.error('Re-reading the server store failed:', error)) readRtuServerStatus() }) From 5fb24425a07096591af18e3dca93dc1af1d492e7 Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 07:51:39 +0200 Subject: [PATCH 11/19] fix: a config from a newer Modbux is parsed before it is loaded The `detectedVersion > CURRENT_..._CONFIG_VERSION` branch cast the parsed JSON to the config type and returned it, in both migrations, so it was the one door into the app that no schema stood in. `LoadButton` calls `replaceRegisterMapping(config.registerMapping)` with no gate at all, so `{ version: 9, registerMapping: 'nope' }` went into the persisted store and was flushed to main. The snackbar read "This config was created with a newer version of Modbux. Some features may not work correctly.", which is a compatibility notice for something nothing had checked. Both branches had a test and both fixtures were well formed, so the suite was green on the half that works. `repairPersisted` is the answer, which is what a persisted store already gets: parse field by field against the current schema, keep what matches, default the rest and name it. A config from a newer Modbux shares its fields with this one, and the fields it shares are the ones worth keeping. `MigrationResult.reset` carries what did not come across, and the two snackbars read `resetMessage`, which already had the wording for a blob a newer version wrote, down to the case where it came across in full. `FIELD_LABELS` gains the three fields a file carries and a store does not, because the two name some of the same things differently: `serverRegistersPerUnit` against `serverRegisters`, plus the two version fields. `emptyRegisterMapping` moves to `shared/default.ts`. The client store had it and the client repair needs the same thing to default the field to. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- CHANGELOG.md | 6 ++ .../RegisterGridToolbar/LoadButton.tsx | 13 ++-- .../src/components/server/OpenSaveClear.tsx | 13 ++-- src/renderer/src/context/client.zustand.ts | 10 +-- src/shared/__tests__/migrations.test.ts | 70 +++++++++++++++++++ src/shared/default.ts | 15 ++++ src/shared/migrations/client/config.ts | 28 +++++++- src/shared/migrations/server/config.ts | 27 ++++++- src/shared/migrations/types.ts | 12 ++++ src/shared/repairPersisted.ts | 12 +++- 10 files changed, 177 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b7f80d2..19d74f86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A configuration from a newer Modbux is now checked before it is loaded.** It + was the one file that went in unread: the fields were taken as they came, so a + register map a newer version writes differently was written straight into the + app. The warning said some features may not work correctly. The file is now + read against what this version understands, what matches is kept, and the + message says which parts did not come across. - **A file that is not a configuration is now refused instead of opened.** Picking a JSON file holding a number, a string, a list or nothing at all emptied the server and reported that the configuration had been updated from diff --git a/src/renderer/src/components/client/ClientGrids/RegisterGrid/RegisterGridToolbar/LoadButton.tsx b/src/renderer/src/components/client/ClientGrids/RegisterGrid/RegisterGridToolbar/LoadButton.tsx index e6e9efdc..0f62c6f0 100644 --- a/src/renderer/src/components/client/ClientGrids/RegisterGrid/RegisterGridToolbar/LoadButton.tsx +++ b/src/renderer/src/components/client/ClientGrids/RegisterGrid/RegisterGridToolbar/LoadButton.tsx @@ -2,7 +2,7 @@ import FileOpen from '@mui/icons-material/FileOpen' import Box from '@mui/material/Box' import IconButton from '@mui/material/IconButton' import { useClientZustand } from '@renderer/context/client.zustand' -import { migrateClientConfig } from '@shared' +import { migrateClientConfig, resetMessage } from '@shared' import { useSnackbar } from 'notistack' import { useRef, useState, useCallback } from 'react' import { showMapping } from '@renderer/context/data.zustand' @@ -28,7 +28,7 @@ const LoadButton = meme((): JSX.Element => { try { // Use migration framework to handle all config versions const migrationResult = migrateClientConfig(content) - const { config, migrated, warning } = migrationResult + const { config, migrated, warning, reset } = migrationResult // Set name, endianness and register mapping if (config.name) clientZustand.setName(config.name) @@ -49,12 +49,13 @@ const LoadButton = meme((): JSX.Element => { }) } - // Show warning for future version - if (warning === 'FUTURE_VERSION') { + // A config from a newer Modbux is parsed against the current schema + // and keeps what matches, so the warning says which fields did not come + // across rather than that some feature may not work. + if (warning === 'FUTURE_VERSION' && reset) { enqueueSnackbar({ variant: 'warning', - message: - 'This config was created with a newer version of Modbux. Some features may not work correctly.', + message: resetMessage('Client', reset), persist: true }) } diff --git a/src/renderer/src/components/server/OpenSaveClear.tsx b/src/renderer/src/components/server/OpenSaveClear.tsx index 9dba1650..8fcf4fb9 100644 --- a/src/renderer/src/components/server/OpenSaveClear.tsx +++ b/src/renderer/src/components/server/OpenSaveClear.tsx @@ -7,7 +7,7 @@ import { downloadJson } from '@renderer/components/shared/downloadJson' import { meme } from '@renderer/components/shared/inputs/meme' import { useLayoutZustand } from '@renderer/context/layout.zustand' import { useServerZustand } from '@renderer/context/server.zustand' -import { checkHasConfig, migrateServerConfig } from '@shared' +import { checkHasConfig, migrateServerConfig, resetMessage } from '@shared' import { CURRENT_SERVER_CONFIG_VERSION, ServerConfig, @@ -47,7 +47,7 @@ const useOpen: UseOpenHook = () => { // Use migration framework to handle all config versions const migrationResult = migrateServerConfig(content) - const { config, migrated, warning, wasMixedEndianness } = migrationResult + const { config, migrated, warning, wasMixedEndianness, reset } = migrationResult // A unit the file does not name is not written over on the way in, so // whatever the previous config left on it would answer a master after @@ -96,12 +96,13 @@ const useOpen: UseOpenHook = () => { }) } - // Show warning for future version - if (warning === 'FUTURE_VERSION') { + // A config from a newer Modbux is parsed against the current schema and + // keeps what matches, so the warning says which fields did not come + // across rather than that some feature may not work. + if (warning === 'FUTURE_VERSION' && reset) { enqueueSnackbar({ variant: 'warning', - message: - 'This config was created with a newer version of Modbux. Some features may not work correctly.', + message: resetMessage('Server', reset), persist: true }) } diff --git a/src/renderer/src/context/client.zustand.ts b/src/renderer/src/context/client.zustand.ts index 050c164f..95f6c908 100644 --- a/src/renderer/src/context/client.zustand.ts +++ b/src/renderer/src/context/client.zustand.ts @@ -15,7 +15,7 @@ import { migrateClientState, carryFormerClientState, CLIENT_ZUSTAND_STORAGE_KEY, - RegisterMapping + emptyRegisterMapping } from '@shared' import { useDataZustand } from './data.zustand' import { loadSerialPorts } from './serialPorts' @@ -52,14 +52,6 @@ export const flushRegisterMappingToMain = (): void => { window.api.setRegisterMapping(useClientZustand.getState().registerMapping) } -/** A mapping with nothing configured, one empty record per register type. */ -const emptyRegisterMapping = (): RegisterMapping => ({ - coils: {}, - discrete_inputs: {}, - holding_registers: {}, - input_registers: {} -}) - /** * Drop the rows on screen, unless something is about to replace them. * diff --git a/src/shared/__tests__/migrations.test.ts b/src/shared/__tests__/migrations.test.ts index 34753862..335add1f 100644 --- a/src/shared/__tests__/migrations.test.ts +++ b/src/shared/__tests__/migrations.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from 'vitest' import { migrateServerConfig } from '../migrations/server/config' import { migrateClientConfig } from '../migrations/client/config' +import { resetMessage } from '../repairPersisted' import { migrateServerRegistersState, migrateBoolShape } from '../migrations/server/zustand' import { readFileSync } from 'fs' import { join } from 'path' @@ -164,6 +165,49 @@ describe('configMigration', () => { expect(result.migrated).toBe(false) expect(result.fromVersion).toBe(3) expect(result.warning).toBe('FUTURE_VERSION') + expect(result.reset?.savedByNewerVersion).toBe(true) + expect(result.reset?.fields).toEqual([]) + }) + + // The branch cast the parsed JSON straight to `ServerConfig` and returned + // it, so this was the one door into the app no schema stood in, and the + // snackbar read as a compatibility notice rather than "this was not + // checked". Both fixtures the suite had were well formed, so it was green + // on the half that works. + it('keeps the fields a future config still shares and names the rest', () => { + const result = migrateServerConfig( + JSON.stringify({ + version: 9, + modbuxVersion: '9.0.0', + name: 'Future', + littleEndian: true, + serverRegistersPerUnit: 'not a unit' + }) + ) + + expect(result.warning).toBe('FUTURE_VERSION') + expect(result.config.name).toBe('Future') + expect(result.config.littleEndian).toBe(true) + expect(result.config.serverRegistersPerUnit).toEqual({}) + expect(result.reset?.fields).toEqual(['serverRegistersPerUnit']) + }) + + it('says so in a sentence naming the field', () => { + const result = migrateServerConfig( + JSON.stringify({ + version: 9, + modbuxVersion: '9.0.0', + name: 'Future', + littleEndian: false, + serverRegistersPerUnit: 'not a unit' + }) + ) + const reset = result.reset + if (!reset) throw new Error('a future config always answers a reset') + + expect(resetMessage('Server', reset)).toBe( + 'Server configuration was saved by a newer version of Modbux. the registers did not come across and was reset. Everything else was kept.' + ) }) }) @@ -390,6 +434,32 @@ describe('configMigration', () => { expect(result.fromVersion).toBe(3) expect(result.warning).toBe('FUTURE_VERSION') }) + + // `LoadButton` calls `replaceRegisterMapping(config.registerMapping)` + // with no gate at all, so `'nope'` went into the persisted store and was + // flushed to main. + it('keeps the fields a future config still shares and names the rest', () => { + const result = migrateClientConfig( + JSON.stringify({ + version: 9, + modbuxVersion: '9.0.0', + name: 'Future', + littleEndian: true, + registerMapping: 'nope' + }) + ) + + expect(result.warning).toBe('FUTURE_VERSION') + expect(result.config.name).toBe('Future') + expect(result.config.littleEndian).toBe(true) + expect(result.config.registerMapping).toEqual({ + coils: {}, + discrete_inputs: {}, + holding_registers: {}, + input_registers: {} + }) + expect(result.reset?.fields).toEqual(['registerMapping']) + }) }) }) diff --git a/src/shared/default.ts b/src/shared/default.ts index 6dc5ec6e..095c33ae 100644 --- a/src/shared/default.ts +++ b/src/shared/default.ts @@ -4,6 +4,7 @@ import { RegisterConfig, RegisterData, RegisterDataWords, + RegisterMapping, SerialPortOptions } from './types' @@ -46,6 +47,20 @@ export const defaultRegisterConfig: RegisterConfig = { addressBase: '0' } +/** + * A mapping with nothing configured, one empty record per register type. + * + * Built fresh at each call, because the store writes into it. Two callers: the + * client store's Clear, and the repair a config from a newer Modbux goes + * through, which needs something to default the field to when it fails. + */ +export const emptyRegisterMapping = (): RegisterMapping => ({ + coils: {}, + discrete_inputs: {}, + holding_registers: {}, + input_registers: {} +}) + export const defaultClientState: ClientState = { connectState: 'disconnected', polling: false, diff --git a/src/shared/migrations/client/config.ts b/src/shared/migrations/client/config.ts index 36377a4f..fe094f40 100644 --- a/src/shared/migrations/client/config.ts +++ b/src/shared/migrations/client/config.ts @@ -1,4 +1,6 @@ import { RegisterMapConfigSchema, RegisterMapConfig, RegisterMapping } from '../../types/client' +import { emptyRegisterMapping } from '../../default' +import { repairPersisted } from '../../repairPersisted' import { MigrationResult, Migration } from '../types' import { formatZodError, parseConfigFile, renameLegacyRegisterTypeKeys } from '../shared' @@ -59,13 +61,33 @@ export function migrateClientConfig(raw: string): MigrationResult CURRENT_CLIENT_CONFIG_VERSION) { + const repair = repairPersisted( + RegisterMapConfigSchema, + parsed, + { + version: detectedVersion, + modbuxVersion: '', + name: undefined, + littleEndian: false, + registerMapping: emptyRegisterMapping() + } satisfies RegisterMapConfig, + true + ) + return { - config: parsed as RegisterMapConfig, + config: repair.state, migrated: false, fromVersion: detectedVersion, - warning: 'FUTURE_VERSION' + warning: 'FUTURE_VERSION', + reset: repair.reset } } diff --git a/src/shared/migrations/server/config.ts b/src/shared/migrations/server/config.ts index dd035216..d27cf906 100644 --- a/src/shared/migrations/server/config.ts +++ b/src/shared/migrations/server/config.ts @@ -15,6 +15,7 @@ import { renameLegacyRegisterTypeKeys } from '../shared' import { V1ServerConfig, extractGlobalEndianness } from './shared' +import { repairPersisted } from '../../repairPersisted' /** * The version the Save button writes into a server config file. @@ -137,13 +138,33 @@ export function migrateServerConfig(raw: string): MigrationResult } } - // Future version + // A config from a newer Modbux keeps the fields it still shares with this + // one. The branch cast the parsed JSON straight to `ServerConfig` and + // returned it, so `{ version: 9, serverRegistersPerUnit: 'not a unit' }` went + // into the store with the snackbar reading "Some features may not work + // correctly", which is a compatibility notice for something no schema had + // looked at. `repairPersisted` is the same answer a persisted store already + // gets, and `reset` is what the warning now says. if (detectedVersion > CURRENT_SERVER_CONFIG_VERSION) { + const repair = repairPersisted( + ServerConfigSchema, + parsed, + { + version: detectedVersion, + modbuxVersion: '', + name: '', + littleEndian: false, + serverRegistersPerUnit: {} + } satisfies ServerConfig, + true + ) + return { - config: parsed as ServerConfig, + config: repair.state, migrated: false, fromVersion: detectedVersion, - warning: 'FUTURE_VERSION' + warning: 'FUTURE_VERSION', + reset: repair.reset } } diff --git a/src/shared/migrations/types.ts b/src/shared/migrations/types.ts index b1f16cbd..aa4b6a54 100644 --- a/src/shared/migrations/types.ts +++ b/src/shared/migrations/types.ts @@ -1,9 +1,21 @@ +import type { ConfigReset } from '../repairPersisted' + export interface MigrationResult { config: T migrated: boolean fromVersion: number warning?: 'FUTURE_VERSION' | 'MIXED_ENDIANNESS' wasMixedEndianness?: boolean + /** + * What a config from a newer Modbux did not bring across, and undefined when + * it brought everything. + * + * Set on the `FUTURE_VERSION` path alone. That branch used to cast the parsed + * JSON straight to the config type, so it was the one door into the app no + * schema stood in, and the warning the user read was a compatibility notice + * for something that had not been checked at all. + */ + reset?: ConfigReset } export type Migration = (config: unknown) => T diff --git a/src/shared/repairPersisted.ts b/src/shared/repairPersisted.ts index 7c3ddfc8..de115c94 100644 --- a/src/shared/repairPersisted.ts +++ b/src/shared/repairPersisted.ts @@ -62,10 +62,18 @@ export function repairPersisted( } /** - * What each persisted field is called on screen. A field with no entry is named - * as it is stored, which is worth more in a bug report than a guess. + * What each field is called on screen. A field with no entry is named as it is + * stored, which is worth more in a bug report than a guess. + * + * Both a persisted store and a config file from a newer Modbux are read + * through here, and the two name some of the same things differently: the store + * keeps `serverRegisters`, the file `serverRegistersPerUnit`, and a file also + * carries the two version fields. */ const FIELD_LABELS: Record = { + serverRegistersPerUnit: 'the registers', + version: 'the config version', + modbuxVersion: 'the Modbux version it was saved by', connectionConfig: 'the connection settings', registerConfig: 'the register settings', registerMapping: 'the register mapping', From e869759ebdf5d84ab50cabbee732419f2d3a4c7e Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 07:52:49 +0200 Subject: [PATCH 12/19] docs: three claims about a bad version field, measured `/code-review` on `f9184b8` measured what a `version` field that is not a number actually did, and the docblock, the test comment and the changelog line all said something else. A relational operator coerces: `"3" > 2` and `2.5 > 2` are both true, so those two took the future version branch and came back `migrated: false` with the persistent newer-version warning, never the v1 migration. `true` and `null` compared false against both and took the v1 path. Four values, three outcomes. The changelog claimed one symptom for all of them, and `null` was a fourth: the migration threw on it, so the user saw a red failure rather than "updated from an older format", after the server had already been emptied. `migrateClientConfig` reaches the same guard and had no case sending it a bad version. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- CHANGELOG.md | 9 +++++---- src/shared/__tests__/migrations.test.ts | 15 +++++++++++++-- src/shared/migrations/shared.ts | 7 +++++-- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19d74f86..1771417b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,10 +16,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 read against what this version understands, what matches is kept, and the message says which parts did not come across. - **A file that is not a configuration is now refused instead of opened.** - Picking a JSON file holding a number, a string, a list or nothing at all - emptied the server and reported that the configuration had been updated from - an older format. A file claiming a version that is not a number took the same - path. + Picking a JSON file holding a number, a string or a list emptied the server + and reported that the configuration had been updated from an older format. A + file holding nothing at all emptied it too and then failed with a message + about a property of null. A file claiming a version that is not a number got + one of those or the newer-version warning, depending on the value. - **Opening the wrong file no longer empties the server.** Picking a client config, or any file Modbux cannot read as a server config, cleared the setup that was on screen before anything had read the file, and then reported the diff --git a/src/shared/__tests__/migrations.test.ts b/src/shared/__tests__/migrations.test.ts index 335add1f..7d76e9e8 100644 --- a/src/shared/__tests__/migrations.test.ts +++ b/src/shared/__tests__/migrations.test.ts @@ -255,13 +255,24 @@ describe('configMigration', () => { ) }) - // `JSON.parse` answers `any`, so nothing asked what was in this field and - // a version that is not one took the v1 path with the rest. + // `JSON.parse` answers `any`, so nothing asked what was in this field, + // and a relational operator coerces: `"3" > 2` and `2.5 > 2` are both + // true, so those two took the future version branch and came back + // `migrated: false` with the newer-version warning, while `true` and + // `null` compared false against both and took the v1 path. it.each(['"3"', 'true', '2.5', 'null'])('refuses a version of %s', (version) => { const raw = `{"version": ${version}, "serverRegistersPerUnit": {}}` expect(() => migrateServerConfig(raw)).toThrow('which is not a version') }) + it.each(['"3"', 'true', '2.5', 'null'])( + 'refuses a client config claiming version %s', + (version) => { + const raw = `{"version": ${version}, "registerMapping": {}}` + expect(() => migrateClientConfig(raw)).toThrow('which is not a version') + } + ) + it('still takes a file with no version at all', () => { const result = migrateServerConfig(JSON.stringify({ name: 'v1', coils: {} })) expect(result.fromVersion).toBe(1) diff --git a/src/shared/migrations/shared.ts b/src/shared/migrations/shared.ts index b9a368c3..f217a827 100644 --- a/src/shared/migrations/shared.ts +++ b/src/shared/migrations/shared.ts @@ -49,8 +49,11 @@ const isRecord = (value: unknown): value is Record => * not cover, and a config is a map of named fields either way. * * The version is checked here too. `JSON.parse` answers `any`, so nothing had - * asked what was in that field, and a `version` of `"3"` or `true` took the v1 - * path with the rest. + * asked what was in that field, and a relational operator coerces: `"3" > 2` + * and `2.5 > 2` are both true, so those two took the future version branch and + * came back with `migrated: false` and the newer-version warning, while `true` + * and `null` compared false against both and took the v1 path. Four values, + * three outcomes, none of them the one the file asked for. */ export function parseConfigFile(raw: string): { parsed: Record From 34dffe9354d83967bc1d697100130311ae3cde73 Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 08:05:14 +0200 Subject: [PATCH 13/19] fix: one register a newer config carries does not cost the rest `/code-review` on `e8d01d9` found six things, and the first two mean that commit traded one defect for another. `repairPersisted` reads a field whole, and `serverRegistersPerUnit` is the one field a server config is about. `DataTypeSchema` is a closed enum and `RegisterParamsSchema` a closed union, so a config from a newer Modbux carrying one register with a data type this version does not name failed the field, the field became `{}`, and every register on every unit was gone, after `useOpen` had emptied the store and main. Under the cast those registers landed. `registerMapping` is the same shape on the client, and it is the one thing in the client store built by hand. `dropUnservableConfigRegisters` and `dropUnreadableConfigMapping` run before the field repair, which is what `dropUnservableRegisters` and `dropUnmappableRegisters` already do for the persisted store. They walk `serverRegistersPerUnit[unit]` and `registerMapping[type]` where those walk one level deeper, which is the difference `migrateBoolShapeForUnit` already carries. `warning: 'FUTURE_VERSION'` and `reset` were only ever set together, so the caller asked twice and the second question was one no input answered no. One field, `futureVersion`, carrying what did not come across. Three on the wording. The field list opened a sentence, so it read "Modbux. the registers did not come across"; the sentence is one clause now. `version: 'the config version'` was a label nothing can reach, because `parseConfigFile` refuses a version that is not an integer and an absent one gives 1. And a notice that everything came across is not one the user has to dismiss. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- .../RegisterGridToolbar/LoadButton.tsx | 12 ++- .../src/components/server/OpenSaveClear.tsx | 12 ++- src/shared/__tests__/migrations.test.ts | 90 ++++++++++++++++--- src/shared/migrations/client/config.ts | 15 +++- src/shared/migrations/server/config.ts | 10 ++- src/shared/migrations/shared.ts | 72 ++++++++++++--- src/shared/migrations/types.ts | 19 ++-- src/shared/repairPersisted.ts | 7 +- 8 files changed, 190 insertions(+), 47 deletions(-) diff --git a/src/renderer/src/components/client/ClientGrids/RegisterGrid/RegisterGridToolbar/LoadButton.tsx b/src/renderer/src/components/client/ClientGrids/RegisterGrid/RegisterGridToolbar/LoadButton.tsx index 0f62c6f0..ab66cac4 100644 --- a/src/renderer/src/components/client/ClientGrids/RegisterGrid/RegisterGridToolbar/LoadButton.tsx +++ b/src/renderer/src/components/client/ClientGrids/RegisterGrid/RegisterGridToolbar/LoadButton.tsx @@ -28,7 +28,7 @@ const LoadButton = meme((): JSX.Element => { try { // Use migration framework to handle all config versions const migrationResult = migrateClientConfig(content) - const { config, migrated, warning, reset } = migrationResult + const { config, migrated, futureVersion } = migrationResult // Set name, endianness and register mapping if (config.name) clientZustand.setName(config.name) @@ -52,11 +52,15 @@ const LoadButton = meme((): JSX.Element => { // A config from a newer Modbux is parsed against the current schema // and keeps what matches, so the warning says which fields did not come // across rather than that some feature may not work. - if (warning === 'FUTURE_VERSION' && reset) { + if (futureVersion) { enqueueSnackbar({ variant: 'warning', - message: resetMessage('Client', reset), - persist: true + message: resetMessage('Client', futureVersion), + // A notice that everything came across is not one the user has to + // dismiss, and it sits on top of "Configuration opened + // successfully" either way. + persist: futureVersion.fields.length > 0, + autoHideDuration: 8000 }) } } catch (error) { diff --git a/src/renderer/src/components/server/OpenSaveClear.tsx b/src/renderer/src/components/server/OpenSaveClear.tsx index 8fcf4fb9..35fd816a 100644 --- a/src/renderer/src/components/server/OpenSaveClear.tsx +++ b/src/renderer/src/components/server/OpenSaveClear.tsx @@ -47,7 +47,7 @@ const useOpen: UseOpenHook = () => { // Use migration framework to handle all config versions const migrationResult = migrateServerConfig(content) - const { config, migrated, warning, wasMixedEndianness, reset } = migrationResult + const { config, migrated, wasMixedEndianness, futureVersion } = migrationResult // A unit the file does not name is not written over on the way in, so // whatever the previous config left on it would answer a master after @@ -99,11 +99,15 @@ const useOpen: UseOpenHook = () => { // A config from a newer Modbux is parsed against the current schema and // keeps what matches, so the warning says which fields did not come // across rather than that some feature may not work. - if (warning === 'FUTURE_VERSION' && reset) { + if (futureVersion) { enqueueSnackbar({ variant: 'warning', - message: resetMessage('Server', reset), - persist: true + message: resetMessage('Server', futureVersion), + // A notice that everything came across is not one the user has to + // dismiss, and it sits on top of "Configuration opened + // successfully" either way. + persist: futureVersion.fields.length > 0, + autoHideDuration: 8000 }) } } catch (error) { diff --git a/src/shared/__tests__/migrations.test.ts b/src/shared/__tests__/migrations.test.ts index 7d76e9e8..7a9b729e 100644 --- a/src/shared/__tests__/migrations.test.ts +++ b/src/shared/__tests__/migrations.test.ts @@ -164,9 +164,9 @@ describe('configMigration', () => { expect(result.migrated).toBe(false) expect(result.fromVersion).toBe(3) - expect(result.warning).toBe('FUTURE_VERSION') - expect(result.reset?.savedByNewerVersion).toBe(true) - expect(result.reset?.fields).toEqual([]) + expect(result.futureVersion).toBeDefined() + expect(result.futureVersion?.savedByNewerVersion).toBe(true) + expect(result.futureVersion?.fields).toEqual([]) }) // The branch cast the parsed JSON straight to `ServerConfig` and returned @@ -185,11 +185,55 @@ describe('configMigration', () => { }) ) - expect(result.warning).toBe('FUTURE_VERSION') + expect(result.futureVersion).toBeDefined() expect(result.config.name).toBe('Future') expect(result.config.littleEndian).toBe(true) expect(result.config.serverRegistersPerUnit).toEqual({}) - expect(result.reset?.fields).toEqual(['serverRegistersPerUnit']) + expect(result.futureVersion?.fields).toEqual(['serverRegistersPerUnit']) + }) + + // `repairPersisted` reads a field whole, and `serverRegistersPerUnit` is + // the one field a server config is about. `DataTypeSchema` is a closed + // enum, so one register with a type this version does not name failed the + // field and cost every register on every unit, after `useOpen` had + // already emptied the store and main. That is the cost + // `dropUnservableRegisters` exists to avoid on the persisted side. + it('keeps the registers a future config shares and drops the one it does not', () => { + const register = (address: number, dataType: string): unknown => ({ + value: 1, + params: { + address, + registerType: 'holding_registers', + dataType, + comment: '', + value: 1 + } + }) + const result = migrateServerConfig( + JSON.stringify({ + version: 9, + modbuxVersion: '9.0.0', + name: 'Future', + littleEndian: false, + serverRegistersPerUnit: { + '1': { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: { + '0': register(0, 'uint16'), + '10': register(10, 'a_type_from_3_0'), + '20': register(20, 'int32') + } + } + } + }) + ) + + expect( + Object.keys(result.config.serverRegistersPerUnit['1']?.holding_registers ?? {}) + ).toEqual(['0', '20']) + expect(result.futureVersion?.fields).toEqual([]) }) it('says so in a sentence naming the field', () => { @@ -202,11 +246,11 @@ describe('configMigration', () => { serverRegistersPerUnit: 'not a unit' }) ) - const reset = result.reset + const reset = result.futureVersion if (!reset) throw new Error('a future config always answers a reset') expect(resetMessage('Server', reset)).toBe( - 'Server configuration was saved by a newer version of Modbux. the registers did not come across and was reset. Everything else was kept.' + 'Server configuration was saved by a newer version of Modbux, and the registers did not come across. Everything else was kept.' ) }) }) @@ -443,12 +487,38 @@ describe('configMigration', () => { const result = migrateClientConfig(v3Config) expect(result.fromVersion).toBe(3) - expect(result.warning).toBe('FUTURE_VERSION') + expect(result.futureVersion).toBeDefined() }) // `LoadButton` calls `replaceRegisterMapping(config.registerMapping)` // with no gate at all, so `'nope'` went into the persisted store and was // flushed to main. + // `registerMapping` is one field too, and it is the one thing in the + // client store built by hand, so one address this version cannot read + // cost the whole mapping. `LoadButton` then flushed that nothing to main. + it('keeps the mapping entries a future config shares and drops the one it does not', () => { + const result = migrateClientConfig( + JSON.stringify({ + version: 9, + modbuxVersion: '9.0.0', + littleEndian: false, + registerMapping: { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: { + '0': { dataType: 'uint16', comment: 'kept' }, + '10': { dataType: 'a_type_from_3_0' }, + '20': { dataType: 'int32' } + } + } + }) + ) + + expect(Object.keys(result.config.registerMapping.holding_registers)).toEqual(['0', '20']) + expect(result.futureVersion?.fields).toEqual([]) + }) + it('keeps the fields a future config still shares and names the rest', () => { const result = migrateClientConfig( JSON.stringify({ @@ -460,7 +530,7 @@ describe('configMigration', () => { }) ) - expect(result.warning).toBe('FUTURE_VERSION') + expect(result.futureVersion).toBeDefined() expect(result.config.name).toBe('Future') expect(result.config.littleEndian).toBe(true) expect(result.config.registerMapping).toEqual({ @@ -469,7 +539,7 @@ describe('configMigration', () => { holding_registers: {}, input_registers: {} }) - expect(result.reset?.fields).toEqual(['registerMapping']) + expect(result.futureVersion?.fields).toEqual(['registerMapping']) }) }) }) diff --git a/src/shared/migrations/client/config.ts b/src/shared/migrations/client/config.ts index fe094f40..0604b08e 100644 --- a/src/shared/migrations/client/config.ts +++ b/src/shared/migrations/client/config.ts @@ -2,7 +2,12 @@ import { RegisterMapConfigSchema, RegisterMapConfig, RegisterMapping } from '../ import { emptyRegisterMapping } from '../../default' import { repairPersisted } from '../../repairPersisted' import { MigrationResult, Migration } from '../types' -import { formatZodError, parseConfigFile, renameLegacyRegisterTypeKeys } from '../shared' +import { + dropUnreadableConfigMapping, + formatZodError, + parseConfigFile, + renameLegacyRegisterTypeKeys +} from '../shared' /** * The version the Save button writes into a client config file. @@ -69,6 +74,11 @@ export function migrateClientConfig(raw: string): MigrationResult CURRENT_CLIENT_CONFIG_VERSION) { + // Entry by entry first. `registerMapping` is one field, and it is the one + // thing in the client store built by hand, so one address this version + // cannot read would cost the whole mapping. + dropUnreadableConfigMapping(parsed) + const repair = repairPersisted( RegisterMapConfigSchema, parsed, @@ -86,8 +96,7 @@ export function migrateClientConfig(raw: string): MigrationResult // looked at. `repairPersisted` is the same answer a persisted store already // gets, and `reset` is what the warning now says. if (detectedVersion > CURRENT_SERVER_CONFIG_VERSION) { + // Register by register first, because `repairPersisted` reads a field + // whole and `serverRegistersPerUnit` is the one field a server config is + // about: one register with a data type this version does not name would + // cost every register on every unit. + dropUnservableConfigRegisters(parsed) + const repair = repairPersisted( ServerConfigSchema, parsed, @@ -163,8 +170,7 @@ export function migrateServerConfig(raw: string): MigrationResult config: repair.state, migrated: false, fromVersion: detectedVersion, - warning: 'FUTURE_VERSION', - reset: repair.reset + futureVersion: repair.reset } } diff --git a/src/shared/migrations/shared.ts b/src/shared/migrations/shared.ts index f217a827..60c41462 100644 --- a/src/shared/migrations/shared.ts +++ b/src/shared/migrations/shared.ts @@ -1,5 +1,6 @@ import type { ZodError, ZodIssue } from 'zod' import { RegisterAddressKeySchema } from '../types/ranges' +import { RegisterMapValueSchema } from '../types/client' import { RegisterParamsSchema } from '../types/server' import { NumberRegistersSchema, ParitySchema, UnitIdStringSchema } from '../types' import { getUsedAddresses } from '../utils' @@ -104,18 +105,6 @@ const recordAt = ( return made } -/** - * A register map is keyed by address, and a register entry repeats its whole - * parameter set, so both have to hold for the entry to be servable. A boolean - * entry carries the key alone. - */ -const isServable = (address: string, entry: Record): boolean => { - if (!RegisterAddressKeySchema.safeParse(address).success) return false - const params = entry.params - if (!isRecord(params)) return true - return RegisterParamsSchema.safeParse(params).success -} - /** The addresses one unit's surviving registers occupy, by register type. */ const usedAddressesOfUnit = ( registersByType: Record @@ -181,6 +170,65 @@ export function dropUnservableRegisters(state: Record): void { } } +/** + * A register map is keyed by address, and a register entry repeats its whole + * parameter set, so both have to hold for the entry to be servable. A boolean + * entry carries the key alone. + */ +const isServable = (address: string, entry: Record): boolean => { + if (!RegisterAddressKeySchema.safeParse(address).success) return false + const params = entry.params + if (!isRecord(params)) return true + return RegisterParamsSchema.safeParse(params).success +} + +/** + * Drop the registers a config file from a newer Modbux carries that this one + * cannot serve, so the ones it can survive. + * + * `repairPersisted` reads a field whole, and `serverRegistersPerUnit` is the + * one field a server config is about, so without this a single register with a + * data type this version does not name cost every register on every unit. That + * is the cost `dropUnservableRegisters` exists to avoid on the persisted store, + * and a config from a newer version is where a register the enum does not name + * actually comes from. + * + * Walks `serverRegistersPerUnit[unit]` where the store walks + * `serverRegisters[uuid][unit]`, which is the same difference + * `migrateBoolShapeForUnit` already carries. + */ +export function dropUnservableConfigRegisters(parsed: Record): void { + for (const [, registersByType] of recordEntries(parsed.serverRegistersPerUnit)) { + for (const entriesByAddress of objectValues(registersByType)) { + for (const [address, entry] of Object.entries(entriesByAddress)) { + if (isRecord(entry) && isServable(address, entry)) continue + delete entriesByAddress[address] + } + } + } +} + +/** + * Drop the mapping entries a client config from a newer Modbux carries that + * this one cannot read. + * + * `registerMapping` is one field too, and it is the one thing in the client + * store built by hand, which is the reason `dropUnmappableRegisters` gives for + * doing the same on the persisted side. `LoadButton` calls + * `replaceRegisterMapping` on whatever comes back and flushes it to main, so a + * mapping of thousands of rows was replaced by nothing on account of one entry. + */ +export function dropUnreadableConfigMapping(parsed: Record): void { + for (const entriesByAddress of objectValues(parsed.registerMapping)) { + for (const [address, entry] of Object.entries(entriesByAddress)) { + if (RegisterAddressKeySchema.safeParse(address).success) { + if (RegisterMapValueSchema.safeParse(entry).success) continue + } + delete entriesByAddress[address] + } + } +} + /** * Drop persisted mapping entries at an address outside the 16 bit map. * diff --git a/src/shared/migrations/types.ts b/src/shared/migrations/types.ts index aa4b6a54..b7d53d59 100644 --- a/src/shared/migrations/types.ts +++ b/src/shared/migrations/types.ts @@ -4,18 +4,21 @@ export interface MigrationResult { config: T migrated: boolean fromVersion: number - warning?: 'FUTURE_VERSION' | 'MIXED_ENDIANNESS' + warning?: 'MIXED_ENDIANNESS' wasMixedEndianness?: boolean /** - * What a config from a newer Modbux did not bring across, and undefined when - * it brought everything. + * Set when the file claims a version newer than this Modbux writes, and says + * what it did not bring across. * - * Set on the `FUTURE_VERSION` path alone. That branch used to cast the parsed - * JSON straight to the config type, so it was the one door into the app no - * schema stood in, and the warning the user read was a compatibility notice - * for something that had not been checked at all. + * One field rather than a `warning` of `'FUTURE_VERSION'` beside a `reset` + * that is only ever set with it: the pair made the caller ask twice, and the + * second question is one no input answers no. + * + * That branch used to cast the parsed JSON straight to the config type, so it + * was the one door into the app no schema stood in, and the warning the user + * read was a compatibility notice for something that had not been checked. */ - reset?: ConfigReset + futureVersion?: ConfigReset } export type Migration = (config: unknown) => T diff --git a/src/shared/repairPersisted.ts b/src/shared/repairPersisted.ts index de115c94..d1c56afd 100644 --- a/src/shared/repairPersisted.ts +++ b/src/shared/repairPersisted.ts @@ -72,8 +72,7 @@ export function repairPersisted( */ const FIELD_LABELS: Record = { serverRegistersPerUnit: 'the registers', - version: 'the config version', - modbuxVersion: 'the Modbux version it was saved by', + modbuxVersion: 'the version it was saved by', connectionConfig: 'the connection settings', registerConfig: 'the register settings', registerMapping: 'the register mapping', @@ -115,9 +114,9 @@ export function resetMessage(store: 'Client' | 'Server', reset: ConfigReset): st return `${store} configuration was saved by a newer version of Modbux and was read in full.` } - return `${store} configuration was saved by a newer version of Modbux. ${listFields( + return `${store} configuration was saved by a newer version of Modbux, and ${listFields( reset.fields - )} did not come across and ${reset.fields.length === 1 ? 'was' : 'were'} reset. ${kept}` + )} did not come across. ${kept}` } /** From dab660e14190d2bd3f649feeef73b0fb9c618248 Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 08:09:32 +0200 Subject: [PATCH 14/19] fix: a 64 bit register holds every one of its bits `ServerRegisterEntrySchema.value` was `z.number()`, and `applyRegisterValue` read the composite back with `getBigUint64` and stored `Number(...)`. Four words of 0xFFFF came out 18446744073709552000 rather than 18446744073709551615, and the next single word write read that back through `BigInt`, which `setBigUint64` takes modulo 2 ** 64 rather than throwing, so the entry collapsed to the low word alone. Measured: one more word after the flush left it holding 1. The batcher held the exact composite, so the loss only showed once a flush had cleared it and the fallback was the entry. A decimal string for the three types whose composite fills 64 bits as an integer, `ServerRegisterValue`. A `bigint` is not an option: the store is persisted through `JSON.stringify`, which refuses one outright. `double` is the fourth type four registers wide and stays a number, because its composite is a float64, which is exactly what a JS number is. `parseIEC870DateTimeValue` takes a `bigint` now. It read the four words back out of a number with `Math.floor` and `& 0xffff`, so the encoding of 2099/12/31 23:59:59.999 rounded up by a millisecond and the all-0xFFFF sentinel arrived as 2 ** 64, which the range gate answered rather than the sentinel branch. That branch is reachable from this side now. Store version 8 rewrites the value every blob on disk carries for those three types. A number still parses, so nothing is dropped by leaving them: the step is so the first word write after a launch reads an exact value. A value already past 2 ** 53 cannot be recovered, and the string says what was stored rather than what the device holds. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- CHANGELOG.md | 6 + .../ServerBitMapDetail/ServerBitMapDetail.tsx | 2 +- .../__tests__/ServerBitMapDetail.test.tsx | 4 +- .../ServerRegisters/ServerRegisters.tsx | 7 +- .../context/__tests__/exact64BitValue.test.ts | 121 ++++++++++++++++++ src/renderer/src/context/server.zustand.ts | 21 ++- .../src/context/server.zustand.types.ts | 5 +- src/shared/__tests__/conversion.test.ts | 34 ++++- src/shared/conversion.ts | 20 +-- .../__tests__/registerAddress.test.ts | 56 ++++++++ src/shared/migrations/server/zustand.ts | 15 ++- src/shared/migrations/shared.ts | 34 ++++- src/shared/types/server.ts | 11 +- src/shared/utils.ts | 42 ++++++ 14 files changed, 345 insertions(+), 33 deletions(-) create mode 100644 src/renderer/src/context/__tests__/exact64BitValue.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 1771417b..d422b31e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A 64-bit server register now shows the value a master wrote.** A master + writing an INT64, UINT64 or DATETIME register sends four words, and the server + folded them through a number that carries 53 bits, so anything above that came + back rounded. Writing one more word to that register afterwards collapsed it + to the last word alone. A DATETIME at the far end of the range read back one + millisecond late for the same reason. - **A configuration from a newer Modbux is now checked before it is loaded.** It was the one file that went in unread: the fields were taken as they came, so a register map a newer version writes differently was written straight into the diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/ServerBitMapDetail.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/ServerBitMapDetail.tsx index 214e5ff5..22bca36d 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/ServerBitMapDetail.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/ServerBitMapDetail.tsx @@ -105,7 +105,7 @@ const ServerBitMapDetail = meme(({ register }: ServerBitMapDetailProps): JSX.Ele void handleToggle(bitIndex)} onCommentChange={(c) => handleCommentChange(bitIndex, c)} diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/__tests__/ServerBitMapDetail.test.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/__tests__/ServerBitMapDetail.test.tsx index 61a676ad..8a6b5cea 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/__tests__/ServerBitMapDetail.test.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/__tests__/ServerBitMapDetail.test.tsx @@ -19,7 +19,7 @@ const serverState = { /** Answers the entry's own value, which is what the real one does with an empty batcher. */ const mockPendingRegisterValue = vi.fn( - (_uuid: string, _unitId: string, entry: ServerRegisterEntry): number => entry.value + (_uuid: string, _unitId: string, entry: ServerRegisterEntry): number => Number(entry.value) ) vi.mock('@renderer/context/server.zustand', () => ({ @@ -64,7 +64,7 @@ const writtenParams = (call = 0): ServerRegisterEntry['params'] => { beforeEach(() => { mockAddRegister.mockReset() mockPendingRegisterValue.mockReset() - mockPendingRegisterValue.mockImplementation((_uuid, _unitId, entry) => entry.value) + mockPendingRegisterValue.mockImplementation((_uuid, _unitId, entry) => Number(entry.value)) }) describe('which bits the panel shows as on', () => { diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx index 7078d369..4ed331f5 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx @@ -9,6 +9,7 @@ import { formatUnixSeconds, NumberRegisters, parseIEC870DateTimeValue, + toExact64Bits, ServerRegister } from '@shared' import { useServerZustand } from '@renderer/context/server.zustand' @@ -46,10 +47,12 @@ const RowEdit = meme(({ register }: RowProps) => { const getDisplayValue = (register: ServerRegister[number]): string | number => { const { dataType } = register.params if (dataType === 'utf8') return register.params.stringValue ?? '' - if (dataType === 'unix') return formatUnixSeconds(register.value) + // A `unix` register is a uint32, so its composite is a number. The three + // types that hold a string are read through `toExact64Bits`. + if (dataType === 'unix') return formatUnixSeconds(Number(register.value)) // The shared decoder answers '' for a register no date can be read out of, // which is what a register the server has not written yet holds. - if (dataType === 'datetime') return parseIEC870DateTimeValue(register.value) || '—' + if (dataType === 'datetime') return parseIEC870DateTimeValue(toExact64Bits(register.value)) || '—' return register.value } diff --git a/src/renderer/src/context/__tests__/exact64BitValue.test.ts b/src/renderer/src/context/__tests__/exact64BitValue.test.ts new file mode 100644 index 00000000..cdd17334 --- /dev/null +++ b/src/renderer/src/context/__tests__/exact64BitValue.test.ts @@ -0,0 +1,121 @@ +// @vitest-environment happy-dom +// +// `ServerRegisterEntrySchema.value` was `z.number()`. `applyRegisterValue` read +// the composite back with `getBigUint64` and stored `Number(...)`, so four +// words of 0xFFFF came out 18446744073709552000 rather than +// 18446744073709551615. The next single word write read that back through +// `BigInt`, which `setBigUint64` takes modulo 2 ** 64 rather than throwing, so +// the entry collapsed to the low word alone. `ServerRegisters.tsx` renders +// `register.value`, and the four `register_value` senders in main send one +// event per word, so a client writing a 64-bit holding register reaches it. +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + MAIN_SERVER_UUID, + ServerRegisterEntry, + ServerRegisterValue, + ServerRegisters +} from '@shared' +import { stubRenderer } from './stubRenderer' + +type Store = typeof import('../server.zustand') + +beforeEach(() => { + vi.resetModules() + vi.useFakeTimers() + localStorage.clear() + stubRenderer() +}) +afterEach(() => vi.useRealTimers()) + +/** One 64 bit register at address 10, holding `value`. */ +const entry = (dataType: 'uint64' | 'int64', value: ServerRegisterValue): ServerRegisterEntry => ({ + value, + params: { + address: 10, + registerType: 'holding_registers' as const, + dataType, + comment: '', + value: 0, + min: undefined, + max: undefined, + interval: undefined + } +}) + +const seeded = async (dataType: 'uint64' | 'int64'): Promise => { + const store = await import('../server.zustand') + store.useServerZustand.getState().clean(MAIN_SERVER_UUID) + store.useServerZustand.getState().replaceServerRegisters('0', { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: { 10: entry(dataType, 0) } + }) + return store +} + +const stored = (store: Store): ServerRegisterValue => { + const registers: ServerRegisters | undefined = + store.useServerZustand.getState().serverRegisters[MAIN_SERVER_UUID]?.['0'] + const found = registers?.holding_registers[10] + if (!found) throw new Error('the register at address 10 is not there') + return found.value +} + +const write = (store: Store, offset: number, word: number): void => + store.applyRegisterValue({ + uuid: MAIN_SERVER_UUID, + unitId: '0', + registerType: 'holding_registers', + address: 10 + offset, + value: word + }) + +describe('a uint64 register a client writes', () => { + it('holds every one of its 64 bits', async () => { + const store = await seeded('uint64') + for (const [offset, word] of [0xffff, 0xffff, 0xffff, 0xffff].entries()) { + write(store, offset, word) + } + await vi.advanceTimersByTimeAsync(100) + + expect(stored(store)).toBe('18446744073709551615') + }) + + // The flush clears the batcher, so the next word write reads the entry. That + // read used to be the rounded number. + it('keeps the other three words when one more word is written after a flush', async () => { + const store = await seeded('uint64') + for (const [offset, word] of [0xffff, 0xffff, 0xffff, 0xffff].entries()) { + write(store, offset, word) + } + await vi.advanceTimersByTimeAsync(100) + + write(store, 3, 0x0001) + await vi.advanceTimersByTimeAsync(100) + + expect(stored(store)).toBe('18446744073709486081') + }) + + it('holds a value above 2 ** 53 exactly', async () => { + const store = await seeded('uint64') + for (const [offset, word] of [0x0102, 0x0304, 0x0506, 0x0708].entries()) { + write(store, offset, word) + } + await vi.advanceTimersByTimeAsync(100) + + expect(stored(store)).toBe('72623859790382856') + }) +}) + +describe('an int64 register a client writes', () => { + it('holds minus one', async () => { + const store = await seeded('int64') + for (const [offset, word] of [0xffff, 0xffff, 0xffff, 0xffff].entries()) { + write(store, offset, word) + } + await vi.advanceTimersByTimeAsync(100) + + expect(stored(store)).toBe('-1') + }) +}) diff --git a/src/renderer/src/context/server.zustand.ts b/src/renderer/src/context/server.zustand.ts index 305c6c20..90e65165 100644 --- a/src/renderer/src/context/server.zustand.ts +++ b/src/renderer/src/context/server.zustand.ts @@ -22,7 +22,10 @@ import { registerWidth, ModbusBaudRate, RegisterType, - RegisterValue + RegisterValue, + ServerRegisterValue, + holdsExact64Bits, + toExact64Bits } from '@shared' import { onEvent } from '@renderer/events' import { round } from 'lodash' @@ -702,11 +705,11 @@ export const applyRegisterValue = (payload: RegisterValue): void => { view.setFloat32(0, Number(currentValue) || 0, littleEndian) break case 'int64': - view.setBigInt64(0, BigInt(currentValue) || 0n, littleEndian) + view.setBigInt64(0, toExact64Bits(currentValue), littleEndian) break case 'uint64': case 'datetime': - view.setBigUint64(0, BigInt(currentValue) || 0n, littleEndian) + view.setBigUint64(0, toExact64Bits(currentValue), littleEndian) break case 'double': view.setFloat64(0, Number(currentValue) || 0, littleEndian) @@ -766,7 +769,17 @@ export const applyRegisterValue = (payload: RegisterValue): void => { return } - const value = round(Number(newComposite), ['float', 'double'].includes(dataType) ? 3 : 0) + // A decimal string where the composite fills 64 bits as an integer, because + // `Number` carries 53 of them: four words of 0xFFFF came out + // 18446744073709552000 rather than 18446744073709551615, and the next single + // word write read that back through `BigInt`, which `setBigUint64` took + // modulo 2 ** 64, leaving the entry holding the low word alone. + // + // The cache above held the exact composite already, so the loss only showed + // after a flush cleared it and the fallback was the entry. + const value: ServerRegisterValue = holdsExact64Bits(dataType) + ? newComposite.toString() + : round(Number(newComposite), ['float', 'double'].includes(dataType) ? 3 : 0) delayedRegister.setValue(cacheKey, newComposite) diff --git a/src/renderer/src/context/server.zustand.types.ts b/src/renderer/src/context/server.zustand.types.ts index db2daba2..739f907b 100644 --- a/src/renderer/src/context/server.zustand.types.ts +++ b/src/renderer/src/context/server.zustand.types.ts @@ -16,7 +16,8 @@ import { Parity, ConfigReset, DataBits, - StopBits + StopBits, + ServerRegisterValue } from '@shared' import { AsyncMaskSetFn, MaskSetFn } from './client.zustand.types' import { z } from 'zod' @@ -53,7 +54,7 @@ export interface SetBoolParameters { export interface SetRegisterValueParameters { registerType: NumberRegisters address: number - value: number + value: ServerRegisterValue optionalUuid?: string optionalUnitId?: UnitIdString } diff --git a/src/shared/__tests__/conversion.test.ts b/src/shared/__tests__/conversion.test.ts index 76f91be8..b028384a 100644 --- a/src/shared/__tests__/conversion.test.ts +++ b/src/shared/__tests__/conversion.test.ts @@ -142,7 +142,10 @@ describe('parseIEC870DateTimeValue', () => { words.forEach((word, i) => buf.writeUInt16BE(word, i * 2)) return buf } - const composite = words.reduce((total, word, i) => total + word * 2 ** (48 - i * 16), 0) + const composite = words.reduce( + (total, word, i) => total + BigInt(word) * 2n ** BigInt(48 - i * 16), + 0n + ) it('reads the same date out of the composite as out of the buffer', () => { expect(parseIEC870DateTimeValue(composite)).toBe(parseIEC870DateTime(buffer())) @@ -150,14 +153,31 @@ describe('parseIEC870DateTimeValue', () => { }) it('returns empty string for a composite of zero', () => { - expect(parseIEC870DateTimeValue(0)).toBe('') + expect(parseIEC870DateTimeValue(0n)).toBe('') }) - // The all-0xFFFF sentinel is 2 ** 64 - 1, which no Number holds: the store - // narrows the composite with Number() and it arrives here as 2 ** 64. The - // sentinel branch is unreachable from this side, and the range gate answers. - it('returns empty string for the sentinel the store rounded past 64 bits', () => { - expect(parseIEC870DateTimeValue(2 ** 64)).toBe('') + // The all-0xFFFF sentinel is 2 ** 64 - 1, which no Number holds. The store + // kept the composite as one and it arrived here as 2 ** 64, so the sentinel + // branch was unreachable from this side and the range gate answered instead. + // A bigint reaches it. + it('reads the sentinel as the sentinel', () => { + expect(parseIEC870DateTimeValue(2n ** 64n - 1n)).toBe('') + }) + + // The end of the window the format carries, which is what rounded up by one + // millisecond through `Number`. + it('reads the last millisecond the format holds', () => { + const buf = Buffer.alloc(8) + buf.writeUInt16BE(99, 0) + buf.writeUInt16BE((12 << 8) | 31, 2) + buf.writeUInt16BE((23 << 8) | 59, 4) + buf.writeUInt16BE(59999, 6) + const exact = buf.readBigUInt64BE(0) + + expect(parseIEC870DateTimeValue(exact)).toBe(parseIEC870DateTime(buf)) + expect(parseIEC870DateTimeValue(BigInt(Number(exact)))).not.toBe( + parseIEC870DateTimeValue(exact) + ) }) }) diff --git a/src/shared/conversion.ts b/src/shared/conversion.ts index 4f772dc0..cb28076c 100644 --- a/src/shared/conversion.ts +++ b/src/shared/conversion.ts @@ -70,20 +70,22 @@ export const parseIEC870DateTime = (buf: Buffer): string => { } /** - * The same datetime, read off the composite number the server store keeps. + * The same datetime, read off the composite the server store keeps. * * The store folds the four registers into one `value`, so the row had its own * copy of the layout above, without the invalid flag and without the range gate. - * The number carries all 64 bits up to `Number.MAX_SAFE_INTEGER` and no further: - * the encoding of 2099/12/31 23:59:59.999 rounds up by one millisecond on the - * way through `Number`, and the year offset is what pushes it that far. + * + * A `bigint` because the composite fills all 64 bits and a JS number carries 53 + * of them. Through `Number` the encoding of 2099/12/31 23:59:59.999 rounded up + * by one millisecond, and the all-0xFFFF sentinel came out as 2 ** 64, which is + * out of range rather than the sentinel. */ -export const parseIEC870DateTimeValue = (packed: number): string => +export const parseIEC870DateTimeValue = (packed: bigint): string => parseIEC870Words( - Math.floor(packed / 2 ** 48) & 0xffff, - Math.floor(packed / 2 ** 32) & 0xffff, - Math.floor(packed / 2 ** 16) & 0xffff, - packed & 0xffff + Number((packed >> 48n) & 0xffffn), + Number((packed >> 32n) & 0xffffn), + Number((packed >> 16n) & 0xffffn), + Number(packed & 0xffffn) ) /** The `yyyy/MM/dd HH:mm:ss` a unix register's seconds stand for. */ diff --git a/src/shared/migrations/__tests__/registerAddress.test.ts b/src/shared/migrations/__tests__/registerAddress.test.ts index c890a015..2218c4c9 100644 --- a/src/shared/migrations/__tests__/registerAddress.test.ts +++ b/src/shared/migrations/__tests__/registerAddress.test.ts @@ -470,6 +470,62 @@ describe('a persisted register the encoder cannot serve', () => { }) }) +// `ServerRegisterEntrySchema.value` was `z.number()`, so every blob on disk +// carries a number for the three types whose composite fills 64 bits as an +// integer. The first word write after a launch reads that value, and a number +// carries 53 bits. +describe('a persisted 64 bit value', () => { + const persisted = (dataType: string, value: unknown): Record => ({ + serverRegisters: { + u: { + '1': { + holding_registers: { + '10': { + value, + params: { + address: 10, + registerType: 'holding_registers', + dataType, + comment: '', + value: 0 + } + } + } + } + } + } + }) + + const heldValue = (state: Record): unknown => { + const perUuid = state.serverRegisters as Record> + const unit = perUuid.u?.['1'] as Record + const holding = unit.holding_registers as Record> + return holding['10']?.value + } + + it.each(['uint64', 'int64', 'datetime'])('becomes a string for %s', (dataType) => { + const state = migrateServerState(persisted(dataType, 72623859790382850), 7) + + expect(heldValue(state)).toBe('72623859790382850') + }) + + it.each(['uint16', 'int32', 'double', 'float', 'unix'])('stays a number for %s', (dataType) => { + const state = migrateServerState(persisted(dataType, 1234), 7) + + expect(heldValue(state)).toBe(1234) + }) + + it('leaves a value already stored as a string alone', () => { + const state = migrateServerState(persisted('uint64', '18446744073709551615'), 7) + + expect(heldValue(state)).toBe('18446744073709551615') + }) + + it('is behind a version the store has moved past', () => { + expect(CURRENT_SERVER_ZUSTAND_VERSION).toBeGreaterThan(7) + }) +}) + describe('a persisted mapping entry outside the map', () => { /** The last client store version whose blobs can carry any numeric key. */ const LAST_VERSION_ACCEPTING_ANY_KEY = 3 diff --git a/src/shared/migrations/server/zustand.ts b/src/shared/migrations/server/zustand.ts index 7e2b5965..59068d4c 100644 --- a/src/shared/migrations/server/zustand.ts +++ b/src/shared/migrations/server/zustand.ts @@ -1,7 +1,12 @@ import { V1RegisterParams, V1ServerRegistersPerUnit, extractGlobalEndianness } from './shared' -import { dropUnservableRegisters, migrateBoolShapeForUnit, repairPersistedParity } from '../shared' +import { + dropUnservableRegisters, + migrateBoolShapeForUnit, + repairPersistedParity, + stringifyExact64BitValues +} from '../shared' -export const CURRENT_SERVER_ZUSTAND_VERSION = 7 +export const CURRENT_SERVER_ZUSTAND_VERSION = 8 /** Where the server store keeps its state. */ export const SERVER_ZUSTAND_STORAGE_KEY = 'server.zustand' @@ -151,5 +156,11 @@ export function migrateServerState( dropUnservableRegisters(state) } + // v7→v8: a 64 bit composite is a decimal string, so the first word write + // after a launch reads it exactly rather than rounded + if (version < 8) { + stringifyExact64BitValues(state) + } + return state } diff --git a/src/shared/migrations/shared.ts b/src/shared/migrations/shared.ts index 60c41462..8fa5ef0a 100644 --- a/src/shared/migrations/shared.ts +++ b/src/shared/migrations/shared.ts @@ -2,8 +2,8 @@ import type { ZodError, ZodIssue } from 'zod' import { RegisterAddressKeySchema } from '../types/ranges' import { RegisterMapValueSchema } from '../types/client' import { RegisterParamsSchema } from '../types/server' -import { NumberRegistersSchema, ParitySchema, UnitIdStringSchema } from '../types' -import { getUsedAddresses } from '../utils' +import { DataType, NumberRegistersSchema, ParitySchema, UnitIdStringSchema } from '../types' +import { getUsedAddresses, holdsExact64Bits } from '../utils' /** * Replace a stored parity that `ParitySchema` no longer names, at `path` from @@ -229,6 +229,36 @@ export function dropUnreadableConfigMapping(parsed: Record): vo } } +/** + * Rewrite a persisted 64 bit register value as the decimal string it is now. + * + * `ServerRegisterEntrySchema.value` was `z.number()`, so every blob on disk + * carries a number for the three types whose composite fills 64 bits as an + * integer. The schema takes a string as well now, and a number still parses, + * so nothing is dropped by leaving them: this is so the first word write after + * a launch reads an exact value rather than the rounded one. A value already + * past 2 ** 53 cannot be recovered, and the string says what was stored rather + * than what the device holds. + */ +export function stringifyExact64BitValues(state: Record): void { + for (const [, registersPerUnit] of recordEntries(state.serverRegisters)) { + for (const [, registersByType] of recordEntries(registersPerUnit)) { + for (const entriesByAddress of objectValues(registersByType)) { + for (const entry of Object.values(entriesByAddress)) { + if (!isRecord(entry)) continue + if (typeof entry.value !== 'number') continue + const params = entry.params + if (!isRecord(params)) continue + const dataType = params.dataType + if (typeof dataType !== 'string') continue + if (!holdsExact64Bits(dataType as DataType)) continue + entry.value = String(entry.value) + } + } + } + } +} + /** * Drop persisted mapping entries at an address outside the 16 bit map. * diff --git a/src/shared/types/server.ts b/src/shared/types/server.ts index 4b282628..90271429 100644 --- a/src/shared/types/server.ts +++ b/src/shared/types/server.ts @@ -180,9 +180,16 @@ export type ServerBoolEntry = z.infer export const ServerBoolSchema = z.record(RegisterAddressKeySchema, ServerBoolEntrySchema) export type ServerBool = z.infer -// Schema for a single register entry +/** + * One register entry: what it holds, and the parameters that say what it is. + * + * `value` takes a string as well as a number, for the three types whose + * composite fills 64 bits as an integer. See `ServerRegisterValue`. A string + * that is not a decimal integer is refused here, so `toExact64Bits` has a + * number or digits to read. + */ export const ServerRegisterEntrySchema = z.object({ - value: z.number(), + value: z.union([z.number(), z.string().regex(/^-?\d+$/)]), params: RegisterParamsSchema }) export type ServerRegisterEntry = z.infer diff --git a/src/shared/utils.ts b/src/shared/utils.ts index b68a43b1..bffb955d 100644 --- a/src/shared/utils.ts +++ b/src/shared/utils.ts @@ -228,6 +228,48 @@ export const getMinMaxValues = (dataType: DataType): { min: number; max: number } } +/** + * What a server register entry holds, which is not always a number. + * + * A decimal string for the three types whose composite fills 64 bits as an + * integer. It was `z.number()` for all of them: `applyRegisterValue` read the + * composite back with `getBigUint64` and stored `Number(...)`, so four words of + * 0xFFFF came out 18446744073709552000 rather than 18446744073709551615, and + * `setBigUint64` takes its argument modulo 2 ** 64 rather than throwing, so the + * next single word write on that entry collapsed it to the low word alone. + * Measured: one more word after the flush left the entry holding 1. + * + * A string rather than a `bigint` because the store is persisted through + * `JSON.stringify`, which refuses a bigint outright. + */ +export type ServerRegisterValue = number | string + +/** + * The types whose composite fills 64 bits as an integer. + * + * `applyRegisterValue` reads these three back with `getBigInt64` or + * `getBigUint64`. `double` is the fourth type four registers wide and is not + * here: its composite is a float64, which is exactly what a JS number is. + */ +export const holdsExact64Bits = (dataType: DataType): boolean => + dataType === 'int64' || dataType === 'uint64' || dataType === 'datetime' + +/** + * The exact composite a stored value stands for, for a 64 bit integer type. + * + * Takes a `bigint` as well, because `ServerDelayedSetter` holds the composite + * it last folded and that is the value the next word write reads first. + */ +export const toExact64Bits = (value: ServerRegisterValue | bigint): bigint => { + try { + return BigInt(value) + } catch { + // A blob a hand edit left there, or the empty string. `dropUnservableRegisters` + // reads `params` rather than `value`, so this is the one that has no meter. + return 0n + } +} + /** * The types `createRegisters` throws on for a value outside their range. * From edd0fc9a8eed8b4c7e99f89105b979d3fac6fe8e Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 08:15:52 +0200 Subject: [PATCH 15/19] fix: a unit id scan asks for a quantity the protocol allows Nothing between the keyboard and the wire clamped. `LengthField` passed no `max`, so `UintInput`'s default of 65535 was typeable; `setLength` stored it as given; `ScanUnitIDParametersSchema.length` was `z.number().int().positive()`; and `_scanUnitIds` handed it to `this._readers[registerType](address, length)`, which `modbus-serial` writes into the quantity field with nothing between. A device answers illegal-data-value or says nothing, and what the scan told you was about the request rather than about the device. The ceiling is the protocol's, stated once in `ranges.ts`. MODBUS Application Protocol Specification V1.1b3 section 6: FC01 and FC02 answer at most 2000 bits, FC03 and FC04 at most 125 registers, which is the 253 byte PDU minus the function code and the byte count. `maxReadQuantity` reads the register types selected, because one length goes out for every one of them and the strictest is the one the request has to fit. Three places ask it now. The scan's Length field masks to it, `ScanButton` clamps to it where the request is built, because a length typed under one set of types stays in the store when another is selected, and the schema is the backstop. `ScanRegisters.tsx ChunkSizeField` computed the same pair by hand and reads it instead. `LengthInput`, which caps a coil read at 125 in the other direction, is `CU-11` and stays for the annoying branch: too narrow is not illegal. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- CHANGELOG.md | 6 ++ e2e/specs/01-main/12-scan-unitids.spec.ts | 40 +++++++++++ .../client/ScanRegisters/ScanRegisters.tsx | 6 +- .../client/ScanUnitIds/ScanUnitIds.tsx | 18 ++++- src/shared/__tests__/schemas.test.ts | 67 +++++++++++++++++++ src/shared/types/ranges.ts | 32 +++++++++ src/shared/types/scan.ts | 35 +++++++--- 7 files changed, 191 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d422b31e..f2d5d7ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **The unit ID scan no longer asks for more than a device can answer.** Its + Length field took anything up to 65535 and sent it as the quantity, which is + past what the protocol allows. Every device on the bus then refused the + request or said nothing, so what the scan told you was about the request + rather than about the device. The field now stops at 125 registers, or 2000 + bits when only bit types are selected. - **A 64-bit server register now shows the value a master wrote.** A master writing an INT64, UINT64 or DATETIME register sends four words, and the server folded them through a number that carries 53 bits, so anything above that came diff --git a/e2e/specs/01-main/12-scan-unitids.spec.ts b/e2e/specs/01-main/12-scan-unitids.spec.ts index 84941075..bb4b237d 100644 --- a/e2e/specs/01-main/12-scan-unitids.spec.ts +++ b/e2e/specs/01-main/12-scan-unitids.spec.ts @@ -95,6 +95,46 @@ test.describe.serial('Scan Unit IDs', () => { await expect(input).toHaveValue('500') }) + // ─── Length field ────────────────────────────────────────────────── + + // The field passed no `max`, so `UintInput`'s default of 65535 was typeable + // and `modbus-serial` wrote it into the quantity field. A device answers + // illegal-data-value or says nothing, and the answer reads as a fact about + // the bus rather than about the request. FC03 answers at most 125 registers. + test('the length field stops at what one read of a register answers', async ({ mainPage }) => { + const input = mainPage.getByTestId('scan-unitid-length-input').locator('input') + await input.click() + await input.selectText() + await input.pressSequentially('65535') + await input.blur() + + await expect(input).toHaveValue('125') + }) + + // FC01 answers 2000 bits, so selecting a bit type widens the same field. + test('and at 2000 once a bit type is selected on its own', async ({ mainPage }) => { + const modal = mainPage.locator('.MuiModal-root') + await modal.getByTestId('scan-unitid-type-coils').click() + await modal.getByTestId('scan-unitid-type-holding-registers').click() + + const input = mainPage.getByTestId('scan-unitid-length-input').locator('input') + await input.click() + await input.selectText() + await input.pressSequentially('65535') + await input.blur() + + await expect(input).toHaveValue('2000') + + // Back to the defaults the tests below read. + await modal.getByTestId('scan-unitid-type-holding-registers').click() + await modal.getByTestId('scan-unitid-type-coils').click() + await input.click() + await input.selectText() + await input.pressSequentially('2') + await input.blur() + await expect(input).toHaveValue('2') + }) + // ─── Register type toggle buttons ────────────────────────────────── test('Holding Registers is selected by default', async ({ mainPage }) => { diff --git a/src/renderer/src/components/client/ScanRegisters/ScanRegisters.tsx b/src/renderer/src/components/client/ScanRegisters/ScanRegisters.tsx index af359d66..d9e112f2 100644 --- a/src/renderer/src/components/client/ScanRegisters/ScanRegisters.tsx +++ b/src/renderer/src/components/client/ScanRegisters/ScanRegisters.tsx @@ -20,6 +20,7 @@ import { ScanTimeoutField } from '../ScanProgress/ScanProgress' import { meme } from '@renderer/components/shared/inputs/meme' +import { maxReadQuantity } from '@shared' import { useScanRegistersZustand } from './scanRegisters.zustand' // @@ -105,8 +106,9 @@ const ChunkSizeField = meme((): JSX.Element => { const scanning = useClientZustand((z) => z.clientState.scanningRegisters) const chunkSize = useScanRegistersZustand((z) => String(z.chunkSize)) const type = useClientZustand((z) => z.registerConfig.type) - const isCoilType = ['coils', 'discrete_inputs'].includes(type) - const max = isCoilType ? 2000 : 125 + // The protocol's pair, stated once in `ranges.ts`: this field computed it by + // hand and the unit id scan's Length field computed nothing at all. + const max = maxReadQuantity([type]) const setChunkSize = useScanRegistersZustand.getState().setChunkSize diff --git a/src/renderer/src/components/client/ScanUnitIds/ScanUnitIds.tsx b/src/renderer/src/components/client/ScanUnitIds/ScanUnitIds.tsx index dc9ba4af..757fec95 100644 --- a/src/renderer/src/components/client/ScanUnitIds/ScanUnitIds.tsx +++ b/src/renderer/src/components/client/ScanUnitIds/ScanUnitIds.tsx @@ -12,7 +12,7 @@ import AddressBaseInput from '@renderer/components/shared/inputs/AddressBaseInpu import { maskInputProps } from '@renderer/components/shared/inputs/types' import UIntInput from '@renderer/components/shared/inputs/UintInput' import { useClientZustand } from '@renderer/context/client.zustand' -import { RegisterType } from '@shared' +import { maxReadQuantity, RegisterType } from '@shared' import { ElementType, useCallback, useMemo } from 'react' import useScanUnitIdColumns from './_columns' import { useScanUnitIdZustand } from './scanUnitIds.zustand' @@ -102,6 +102,13 @@ const LengthField = meme((): JSX.Element => { const scanning = useClientZustand((z) => z.clientState.scanningUnitIds) const length = useScanUnitIdZustand((z) => String(z.length)) + // The field passed no `max`, so `UintInput`'s default of 65535 was typeable + // and went to the socket as the quantity. `maxReadQuantity` is the protocol's + // own pair, and it reads the types selected because one length goes out for + // every one of them. + const registerTypes = useScanUnitIdZustand((z) => z.registerTypes) + const max = maxReadQuantity(registerTypes) + const setLength = useScanUnitIdZustand.getState().setLength return ( @@ -116,7 +123,7 @@ const LengthField = meme((): JSX.Element => { slotProps={{ input: { inputComponent: UIntInput as unknown as ElementType, - inputProps: maskInputProps({ set: setLength }) + inputProps: maskInputProps({ set: setLength, max }) } }} /> @@ -213,9 +220,14 @@ const ScanButton = meme((): JSX.Element => { const { address, length, startUnitId, count, registerTypes, timeout } = scanUnitIdZustand + // Clamped where the request is built rather than left to the boundary. The + // field masks to the same ceiling, and a length typed under one set of + // register types stays in the store when another is selected, so this is + // the moment the pair is finally known. The boundary refuses what it is + // given and the results are already cleared by then. window.api.scanUnitIds({ address, - length, + length: Math.min(length, maxReadQuantity(registerTypes)), range: [startUnitId, startUnitId + count - 1], registerTypes, timeout diff --git a/src/shared/__tests__/schemas.test.ts b/src/shared/__tests__/schemas.test.ts index a22c00e9..410be37c 100644 --- a/src/shared/__tests__/schemas.test.ts +++ b/src/shared/__tests__/schemas.test.ts @@ -9,6 +9,9 @@ import { BitColorSchema, BitMapEntrySchema, BitMapConfigSchema } from '../types/ import { RegisterParamsSchema, RemoveRegisterParamsSchema } from '../types/server' import { DataBitsSchema, SerialPortOptionsSchema, StopBitsSchema } from '../types/serial' import { MAX_UTF8_LENGTH } from '../utils' +import { ScanUnitIDParametersSchema } from '../types/scan' +import { MAX_READ_BITS, MAX_READ_REGISTERS, maxReadQuantity } from '../types/ranges' +import type { RegisterType } from '../types/register' describe('RegisterMapObjectSchema', () => { it('accepts numeric string keys', () => { @@ -491,6 +494,70 @@ describe('Server RegisterParamsSchema — what a generator draws between', () => }) }) +// Nothing between the keyboard and the wire clamped: `LengthField` passed no +// `max`, `UintInput` defaults to 65535, `setLength` stores the number as given, +// `length` was `z.number().int().positive()`, and `_scanUnitIds` hands it to +// `this._readers[registerType](address, length)`, which writes it into the +// quantity field. A device answers illegal-data-value or says nothing, and the +// answer the user reads is about the request rather than about the bus. +describe('ScanUnitIDParametersSchema — the quantity that reaches the wire', () => { + const parameters = (over: Record): unknown => ({ + range: [1, 5], + address: 0, + length: 2, + registerTypes: ['holding_registers'], + timeout: 500, + ...over + }) + + const takes = (over: Record): boolean => + ScanUnitIDParametersSchema.safeParse(parameters(over)).success + + it('refuses more registers than one read answers', () => { + expect(takes({ length: 126 })).toBe(false) + expect(takes({ length: 65535 })).toBe(false) + expect(takes({ length: MAX_READ_REGISTERS })).toBe(true) + }) + + it('refuses more bits than one read answers', () => { + expect(takes({ registerTypes: ['coils'], length: 2001 })).toBe(false) + expect(takes({ registerTypes: ['coils'], length: MAX_READ_BITS })).toBe(true) + }) + + // One length goes out for every type selected, so the strictest of them is + // the one the request has to fit. + it('holds a mixed selection to the stricter of the two', () => { + expect(takes({ registerTypes: ['coils', 'holding_registers'], length: 2000 })).toBe(false) + expect(takes({ registerTypes: ['coils', 'holding_registers'], length: 125 })).toBe(true) + }) + + it('names the length', () => { + const result = ScanUnitIDParametersSchema.safeParse(parameters({ length: 65535 })) + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.path).toEqual(['length']) + }) +}) + +describe('maxReadQuantity', () => { + // MODBUS Application Protocol Specification V1.1b3 section 6: FC01 and FC02 + // answer at most 2000 bits, FC03 and FC04 at most 125 registers. + it.each([ + [['coils'], MAX_READ_BITS], + [['discrete_inputs'], MAX_READ_BITS], + [['coils', 'discrete_inputs'], MAX_READ_BITS], + [['input_registers'], MAX_READ_REGISTERS], + [['holding_registers'], MAX_READ_REGISTERS], + [['coils', 'holding_registers'], MAX_READ_REGISTERS] + ] as [RegisterType[], number][])('answers %j with %i', (registerTypes, expected) => { + expect(maxReadQuantity(registerTypes)).toBe(expected) + }) + + it('states the protocol rather than a habit', () => { + expect(MAX_READ_BITS).toBe(2000) + expect(MAX_READ_REGISTERS).toBe(125) + }) +}) + describe('SerialPortOptionsSchema', () => { const withOptions = (options: Record): unknown => ({ baudRate: '9600', diff --git a/src/shared/types/ranges.ts b/src/shared/types/ranges.ts index ee5d1db0..9941649e 100644 --- a/src/shared/types/ranges.ts +++ b/src/shared/types/ranges.ts @@ -1,4 +1,5 @@ import z from 'zod' +import { NumberRegistersSchema, RegisterType } from './register' /** * Ranges the protocol and the socket fix, so a schema states them once. @@ -20,6 +21,37 @@ export const RegisterAddressSchema = z.number().int().min(0).max(65535) export const UnitIdSchema = z.number().int().min(0).max(255) export const PortSchema = z.number().int().min(0).max(65535) +/** + * The most a single read can ask for, by what it is reading. + * + * MODBUS Application Protocol Specification V1.1b3, section 6: FC01 and FC02 + * answer at most 2000 bits, FC03 and FC04 at most 125 registers. The PDU is 253 + * bytes, and a read response is the function code, a byte count and the data, + * so 251 bytes are left: 125 registers of two bytes, and 250 bytes of bits. + * + * A write is a different pair, 1968 bits and 123 registers, because the request + * carries the data as well as the address and the quantity. Nothing reads that + * here, because every field these bound builds a read. + */ +export const MAX_READ_BITS = 2000 +export const MAX_READ_REGISTERS = 125 + +/** + * The ceiling one read of these register types has. + * + * A unit id scan asks for several types with one length, so the strictest of + * them is the one the request has to fit. `ScanRegisters.tsx ChunkSizeField` + * computed this pair by hand; the scan's own Length field computed nothing, so + * `UintInput`'s default of 65535 was typeable and `modbus-serial` wrote it + * into the quantity field unchecked. A device answers illegal-data-value or + * says nothing, and the answer the user reads is about the request rather than + * about the bus. + */ +export const maxReadQuantity = (registerTypes: readonly RegisterType[]): number => + registerTypes.some((registerType) => NumberRegistersSchema.safeParse(registerType).success) + ? MAX_READ_REGISTERS + : MAX_READ_BITS + /** * The same range for a register map keyed by address. * diff --git a/src/shared/types/scan.ts b/src/shared/types/scan.ts index bbb67788..00964585 100644 --- a/src/shared/types/scan.ts +++ b/src/shared/types/scan.ts @@ -1,5 +1,5 @@ import { RegisterType, RegisterTypeSchema } from './register' -import { RegisterAddressSchema, UnitIdSchema } from './ranges' +import { maxReadQuantity, RegisterAddressSchema, UnitIdSchema } from './ranges' import z from 'zod' // Scan Registers @@ -13,13 +13,32 @@ export type ScanRegistersParameters = z.infer { + const ceiling = maxReadQuantity(parameters.registerTypes) + if (parameters.length <= ceiling) return + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['length'], + message: `One read of these register types answers at most ${ceiling}` + }) + }) export type ScanUnitIDParameters = z.infer const ScanUnitIdErrorMessageSchema = z.object({ From 6e34d02c5e9967d81567d9b301cb02d49ef09e86 Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 08:20:10 +0200 Subject: [PATCH 16/19] refactor: ServerBit's read-only branches, which no render could reach `readOnly` was set from one place, `readOnly={collapse}` in `ServerBoolRow`, and `ServerBooleans` renders that list only under `{!collapse && ...}`. Both read the same store and React renders the parent first, so no committed render carried `readOnly: true`. `collapse` is persisted, so a restart into a collapsed section starts on the same side. The same half guarded `ServerBoolRow`'s hover styles and its remove button, and two tests in `ServerBit.test.tsx` asserted behaviour nothing in the app reached, which is the severity table's own clause about dead code that makes something look covered. The prop, the `collapse` selector in `ServerBoolRow` and the two tests go. `collapse.test.tsx` states why instead: the section shows its bits while it is open and renders no bit at all once collapsed. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- .../server/ServerGrid/ServerBooleans.tsx | 52 ++++++++++-------- .../ServerGrid/__tests__/collapse.test.tsx | 55 +++++++++++++++++++ .../server/ServerGrid/shared/ServerBit.tsx | 31 ++++------- .../shared/__tests__/ServerBit.test.tsx | 18 ------ 4 files changed, 94 insertions(+), 62 deletions(-) create mode 100644 src/renderer/src/components/server/ServerGrid/__tests__/collapse.test.tsx diff --git a/src/renderer/src/components/server/ServerGrid/ServerBooleans.tsx b/src/renderer/src/components/server/ServerGrid/ServerBooleans.tsx index 72f2302f..8dec2b86 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerBooleans.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerBooleans.tsx @@ -28,8 +28,17 @@ interface ServerBoolRowProps { type: BooleanRegisters } +/** + * One coil or discrete input, with its comment and its remove button. + * + * It read `collapse` and passed it on as `readOnly`, and `ServerBooleans` + * renders this list only under `{!collapse && ...}`. Both read the same store + * and React renders the parent first, so no committed render carried + * `readOnly: true`, and the hover styles and the remove button were guarded on + * the same unreachable half. Two tests in `ServerBit.test.tsx` asserted + * behaviour nothing in the app reached. + */ const ServerBoolRow = meme(({ address, type }: ServerBoolRowProps) => { - const collapse = useServerGridZustand((z) => z.collapse[type]) const entry = useServerZustand((z) => { const uuid = z.selectedUuid const unitId = z.getUnitId(uuid) @@ -63,11 +72,9 @@ const ServerBoolRow = meme(({ address, type }: ServerBoolRowProps) => { alignItems: 'center', borderRadius: 1, transition: 'background-color 0.15s', - ...(!collapse && { - '&:hover .remove-btn': { opacity: 1 }, - '&:hover': { backgroundColor: alpha(theme.palette.primary.dark, 0.1) }, - '&:has(.remove-btn:hover)': { backgroundColor: alpha(theme.palette.error.main, 0.1) } - }) + '&:hover .remove-btn': { opacity: 1 }, + '&:hover': { backgroundColor: alpha(theme.palette.primary.dark, 0.1) }, + '&:has(.remove-btn:hover)': { backgroundColor: alpha(theme.palette.error.main, 0.1) } })} > @@ -81,26 +88,23 @@ const ServerBoolRow = meme(({ address, type }: ServerBoolRowProps) => { padDigits={5} dimUnmapped={false} hoverHighlight={false} - readOnly={collapse} /> - {!collapse && ( - ({ - opacity: 0, - transition: 'opacity 0.15s', - p: 0.25, - color: theme.palette.text.secondary, - '&:hover': { color: theme.palette.error.main } - })} - > - - - )} + ({ + opacity: 0, + transition: 'opacity 0.15s', + p: 0.25, + color: theme.palette.text.secondary, + '&:hover': { color: theme.palette.error.main } + })} + > + + ) }) diff --git a/src/renderer/src/components/server/ServerGrid/__tests__/collapse.test.tsx b/src/renderer/src/components/server/ServerGrid/__tests__/collapse.test.tsx new file mode 100644 index 00000000..c8b94fce --- /dev/null +++ b/src/renderer/src/components/server/ServerGrid/__tests__/collapse.test.tsx @@ -0,0 +1,55 @@ +// @vitest-environment happy-dom +/// +// +// `ServerBit` carried a `readOnly` prop, set from one place, +// `readOnly={collapse}` in `ServerBoolRow`. `ServerBooleans` renders that list +// only under `{!collapse && ...}`, and both read the same store, so React +// renders the parent first and no committed render could carry `readOnly: true`. +// The same half guarded `ServerBoolRow`'s hover styles and its remove button, +// and two tests in `ServerBit.test.tsx` asserted behaviour nothing reached. +import { render, screen } from '@testing-library/react' +import { describe, it, expect, vi } from 'vitest' +import type { ServerBool } from '@shared' + +const coils: ServerBool = { 0: { value: true, comment: 'run' } } +const serverState = { + selectedUuid: 'main', + getUnitId: (): string => '0', + unitId: { main: '0' }, + serverRegisters: { main: { '0': { coils } } } +} + +vi.mock('@renderer/context/server.zustand', () => ({ + useServerZustand: Object.assign( + (selector: (state: typeof serverState) => unknown) => selector(serverState), + { getState: () => serverState } + ) +})) + +import ServerBooleans from '../ServerBooleans' +import useServerGridZustand from '../serverGrid.zustand' + +const allOpen = { + coils: false, + discrete_inputs: false, + input_registers: false, + holding_registers: false +} + +describe('a collapsed boolean section', () => { + it('shows its bits while it is open', () => { + useServerGridZustand.setState({ collapse: allOpen }) + render() + + expect(screen.getByTestId('server-bool-coils-circle-0')).toBeInTheDocument() + expect(screen.getByTestId('remove-bool-coils-0')).toBeInTheDocument() + }) + + it('renders no bit at all once collapsed', () => { + useServerGridZustand.setState({ collapse: { ...allOpen, coils: true } }) + render() + + expect(screen.queryByTestId('server-bool-coils-circle-0')).toBeNull() + expect(screen.queryByTestId('server-bool-row-coils-0')).toBeNull() + }) +}) diff --git a/src/renderer/src/components/server/ServerGrid/shared/ServerBit.tsx b/src/renderer/src/components/server/ServerGrid/shared/ServerBit.tsx index 5f37e1bf..d948b621 100644 --- a/src/renderer/src/components/server/ServerGrid/shared/ServerBit.tsx +++ b/src/renderer/src/components/server/ServerGrid/shared/ServerBit.tsx @@ -18,8 +18,6 @@ export interface ServerBitProps { dimUnmapped?: boolean /** Show hover background on this row (default: true) */ hoverHighlight?: boolean - /** Disable toggle & comment editing (default: false) */ - readOnly?: boolean } const ServerBit = meme( @@ -32,8 +30,7 @@ const ServerBit = meme( testIdPrefix = 'server-bit', padDigits = 2, dimUnmapped = true, - hoverHighlight = true, - readOnly = false + hoverHighlight = true }: ServerBitProps): JSX.Element => { const [editing, setEditing] = useState(false) const [draft, setDraft] = useState(comment ?? '') @@ -75,19 +72,17 @@ const ServerBit = meme( // On and off differ only in background and shadow, so the circle // renders identically to a test either way. This is the state itself. data-active={active} - onClick={readOnly ? undefined : onToggle} + onClick={onToggle} sx={(theme) => ({ width: 12, height: 12, borderRadius: '50%', flexShrink: 0, - cursor: readOnly ? 'default' : 'pointer', + cursor: 'pointer', background: active ? theme.palette.success.main : theme.palette.action.disabled, boxShadow: active ? `0 0 5px ${theme.palette.success.main}` : 'none', transition: 'background 0.15s, box-shadow 0.15s', - ...(!readOnly && { - '&:hover': { outline: `2px solid ${theme.palette.primary.main}`, outlineOffset: 1 } - }) + '&:hover': { outline: `2px solid ${theme.palette.primary.main}`, outlineOffset: 1 } })} /> @@ -107,7 +102,7 @@ const ServerBit = meme( {/* Comment — inline editable */} - {!readOnly && editing ? ( + {editing ? ( { - setDraft(comment ?? '') - setEditing(true) - } - } + onClick={(): void => { + setDraft(comment ?? '') + setEditing(true) + }} sx={{ flexGrow: 1, minWidth: 0, lineHeight: '18px', - cursor: readOnly ? 'default' : 'text', - ...(!readOnly && { '&:hover': { textDecoration: 'underline dotted' } }) + cursor: 'text', + '&:hover': { textDecoration: 'underline dotted' } }} > {hasMapped ? comment : '...'} diff --git a/src/renderer/src/components/server/ServerGrid/shared/__tests__/ServerBit.test.tsx b/src/renderer/src/components/server/ServerGrid/shared/__tests__/ServerBit.test.tsx index cf720171..ce413c45 100644 --- a/src/renderer/src/components/server/ServerGrid/shared/__tests__/ServerBit.test.tsx +++ b/src/renderer/src/components/server/ServerGrid/shared/__tests__/ServerBit.test.tsx @@ -38,15 +38,6 @@ describe('the toggle circle', () => { expect(onToggle).toHaveBeenCalledOnce() }) - - it('does not toggle when the row is read only', async () => { - const onToggle = vi.fn() - renderBit({ onToggle, readOnly: true }) - - await userEvent.click(screen.getByTestId('server-bit-circle-3')) - - expect(onToggle).not.toHaveBeenCalled() - }) }) describe('the comment', () => { @@ -60,13 +51,4 @@ describe('the comment', () => { expect(onCommentChange).toHaveBeenCalledWith('motor running') }) - - it('is not editable when the row is read only', async () => { - const onCommentChange = vi.fn() - renderBit({ comment: 'run', onCommentChange, readOnly: true }) - - await userEvent.click(screen.getByTestId('server-bit-comment-3')) - - expect(screen.queryByRole('textbox')).not.toBeInTheDocument() - }) }) From 08d98c25f812f002fd716fbff2f085ea65110595 Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 08:29:46 +0200 Subject: [PATCH 17/19] fix: the config file says a 64 bit value is a string, and the rewrite cannot break a blob `/code-review` on `2376f64` found four things, and the first two matter. `ServerRegisterEntrySchema` is read by the persisted store and by `ServerConfigSchema`, and only the store version moved. So a config this build writes still claimed v2 while carrying a decimal string, and v2.3.0 takes its `detectedVersion === CURRENT` branch, fails `ServerConfigSchema` on `z.number()` and refuses the whole file rather than the one register. In the other direction a v2 file kept the rounded number, because no config migration ran the rewrite, so File then Open put the defect back. `CURRENT_SERVER_CONFIG_VERSION` is 3 and `migrateServerV2toV3` runs the same helper. `migrateBoolShapeInConfig` goes with it, because it ran on the current-version branch alone and a v2 file carrying the old `boolean` entries takes this step now. `String(entry.value)` could produce a string the new schema refuses: `String(0.5)` is `"0.5"` and `String(1e21)` is `"1e+21"`, and the union takes either as a number and neither as a string. `repairPersisted` reads `serverRegisters` whole, so one such entry cost every server and every unit, and `usedAddresses` survived beside it. The rewrite tests the digits, so it can never turn a blob that parsed into one that does not. The test is the digits rather than `Number.isSafeInteger`, because an integer above 2 ** 53 is exactly what this step is for. `toExact64Bits` answers `undefined` rather than `0n`. Returning zero turned an aborted merge into a composite rebuilt from zero, so one word write would have cost the other three registers. `applyRegisterValue` returns and the row shows a dash, which is what a register the server has not written shows. `ServerRegisters.test.tsx` feeds the datetime cell the decimal string the store now holds, including the far end of the window, which read 2100/01/01 through `Number`, and a fractional value no composite can be read out of. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- e2e/specs/01-main/05-file-io.spec.ts | 5 +- .../ServerRegisters/ServerRegisters.tsx | 7 ++- .../__tests__/ServerRegisters.test.tsx | 48 ++++++++++++--- src/renderer/src/context/server.zustand.ts | 19 ++++-- src/shared/__tests__/migrations.test.ts | 58 +++++++++++++++---- .../__tests__/legacyRegisterTypeKeys.test.ts | 1 - src/shared/migrations/server/config.ts | 34 +++++++++-- src/shared/migrations/shared.ts | 11 +++- src/shared/utils.ts | 15 +++-- 9 files changed, 161 insertions(+), 37 deletions(-) diff --git a/e2e/specs/01-main/05-file-io.spec.ts b/e2e/specs/01-main/05-file-io.spec.ts index 5e867207..0004b62c 100644 --- a/e2e/specs/01-main/05-file-io.spec.ts +++ b/e2e/specs/01-main/05-file-io.spec.ts @@ -168,7 +168,10 @@ test.describe.serial('File I/O — open, save, clear server and client configs', const content = await fs.readFile(savePath, 'utf-8') const config = JSON.parse(content) - expect(config.version).toBe(2) + // `CURRENT_SERVER_CONFIG_VERSION`, written out because importing `@shared` + // here pulls the barrel and with it modules that need Electron. It moved to + // 3 when a 64 bit value became a decimal string. + expect(config.version).toBe(3) expect(config.name).toBe('Main Server') expect(config.littleEndian).toBe(false) expect(config.serverRegistersPerUnit).toBeDefined() diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx index 4ed331f5..cc82d38b 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx @@ -52,7 +52,12 @@ const getDisplayValue = (register: ServerRegister[number]): string | number => { if (dataType === 'unix') return formatUnixSeconds(Number(register.value)) // The shared decoder answers '' for a register no date can be read out of, // which is what a register the server has not written yet holds. - if (dataType === 'datetime') return parseIEC870DateTimeValue(toExact64Bits(register.value)) || '—' + if (dataType === 'datetime') { + // Nothing to read a date out of is the same answer as a register the + // server has not written yet. + const packed = toExact64Bits(register.value) + return (packed !== undefined && parseIEC870DateTimeValue(packed)) || '—' + } return register.value } diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/__tests__/ServerRegisters.test.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/__tests__/ServerRegisters.test.tsx index 9a3a71b7..2c11e371 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/__tests__/ServerRegisters.test.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/__tests__/ServerRegisters.test.tsx @@ -2,7 +2,7 @@ /// import { render, screen } from '@testing-library/react' import { describe, it, expect, vi } from 'vitest' -import type { ServerRegister, ServerRegisterEntry } from '@shared' +import type { ServerRegister, ServerRegisterEntry, ServerRegisterValue } from '@shared' // ─── Store stub ────────────────────────────────────────────────────── // The real server store persists through window.api on import, and the rows @@ -25,15 +25,26 @@ vi.mock('@renderer/context/server.zustand', () => ({ import ServerRegisters from '../ServerRegisters' -/** A register of `dataType` at address 6, holding the composite the store folds. */ -const registerAt6 = (dataType: 'unix' | 'datetime', value: number): ServerRegisterEntry => ({ +/** + * A register of `dataType` at address 6, holding the composite the store folds. + * + * A `datetime` entry holds that composite as a decimal string, because it fills + * 64 bits as an integer and a JS number carries 53 of them. `unix` is a uint32 + * and holds a number. + */ +const registerAt6 = ( + dataType: 'unix' | 'datetime', + value: ServerRegisterValue +): ServerRegisterEntry => ({ value, params: { address: 6, registerType: 'holding_registers', dataType, comment: 'timestamp', - value, + // `params.value` is the fixed value the dialog stored and stays a number; + // the entry's own `value` is the composite the store folds. + value: 0, min: undefined, max: undefined, interval: undefined @@ -47,9 +58,14 @@ const valueCellFor = async (entry: ServerRegisterEntry): Promise => return screen.findByTestId('server-reg-value-holding_registers-6') } -/** One composite value out of the four IEC 870-5 words, the way the store folds them. */ -const packed = (word1: number, word2: number, word3: number, word4: number): number => - word1 * 2 ** 48 + word2 * 2 ** 32 + word3 * 2 ** 16 + word4 +/** One composite value out of the four IEC 870-5 words, as the store holds it. */ +const packed = (word1: number, word2: number, word3: number, word4: number): string => + ( + BigInt(word1) * 2n ** 48n + + BigInt(word2) * 2n ** 32n + + BigInt(word3) * 2n ** 16n + + BigInt(word4) + ).toString() // 2024/06/01 12:34:56.789 UTC const JUNE_2024 = packed(0x0018, 0x0601, 0x0c22, 0xddd5) @@ -61,7 +77,7 @@ describe('what the server row shows for a timestamp', () => { }) it('shows a dash for a datetime register the server has not written', async () => { - const cell = await valueCellFor(registerAt6('datetime', 0)) + const cell = await valueCellFor(registerAt6('datetime', '0')) expect(cell).toHaveTextContent('—') }) @@ -89,4 +105,20 @@ describe('what the server row shows for a timestamp', () => { const cell = await valueCellFor(registerAt6('unix', 1700000000)) expect(cell).toHaveTextContent('2023/11/14 22:13:20') }) + + // The end of the window the format carries, which rounded up by one + // millisecond through `Number` and read 2100/01/01 00:00:00. + it('reads the last second the format holds', async () => { + const cell = await valueCellFor( + registerAt6('datetime', packed(99, (12 << 8) | 31, (23 << 8) | 59, 59999)) + ) + expect(cell).toHaveTextContent('2099/12/31 23:59:59') + }) + + // `ServerRegisterEntrySchema` takes a fractional number for a 64 bit type, so + // a hand-edited config reaches a value no composite can be read out of. + it('shows a dash for a datetime value that is not a whole number', async () => { + const cell = await valueCellFor(registerAt6('datetime', 0.5)) + expect(cell).toHaveTextContent('—') + }) }) diff --git a/src/renderer/src/context/server.zustand.ts b/src/renderer/src/context/server.zustand.ts index 90e65165..c15abe42 100644 --- a/src/renderer/src/context/server.zustand.ts +++ b/src/renderer/src/context/server.zustand.ts @@ -704,13 +704,24 @@ export const applyRegisterValue = (payload: RegisterValue): void => { case 'float': view.setFloat32(0, Number(currentValue) || 0, littleEndian) break - case 'int64': - view.setBigInt64(0, toExact64Bits(currentValue), littleEndian) + // `undefined` is a stored value no composite can be read out of, which a + // hand-edited config reaches because `ServerRegisterEntrySchema` takes a + // fractional number for these types. Aborting leaves the entry alone; + // merging the one word into a composite of zero would cost the other + // three registers. + case 'int64': { + const composite = toExact64Bits(currentValue) + if (composite === undefined) return + view.setBigInt64(0, composite, littleEndian) break + } case 'uint64': - case 'datetime': - view.setBigUint64(0, toExact64Bits(currentValue), littleEndian) + case 'datetime': { + const composite = toExact64Bits(currentValue) + if (composite === undefined) return + view.setBigUint64(0, composite, littleEndian) break + } case 'double': view.setFloat64(0, Number(currentValue) || 0, littleEndian) break diff --git a/src/shared/__tests__/migrations.test.ts b/src/shared/__tests__/migrations.test.ts index 7a9b729e..960ce81b 100644 --- a/src/shared/__tests__/migrations.test.ts +++ b/src/shared/__tests__/migrations.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { migrateServerConfig } from '../migrations/server/config' +import { CURRENT_SERVER_CONFIG_VERSION, migrateServerConfig } from '../migrations/server/config' import { migrateClientConfig } from '../migrations/client/config' import { resetMessage } from '../repairPersisted' import { migrateServerRegistersState, migrateBoolShape } from '../migrations/server/zustand' @@ -20,7 +20,7 @@ describe('configMigration', () => { expect(result.migrated).toBe(true) expect(result.fromVersion).toBe(1) - expect(result.config.version).toBe(2) + expect(result.config.version).toBe(CURRENT_SERVER_CONFIG_VERSION) expect(result.config.littleEndian).toBe(false) expect(result.config.name).toBe('Test Server Big Endian') expect(result.warning).toBeUndefined() @@ -137,33 +137,67 @@ describe('configMigration', () => { }) }) - describe('v2 pass-through', () => { - it('does not migrate v2 config (pass-through)', () => { + // The v2 fixture is a migration now: a 64 bit value is a decimal string, + // and a file this build writes has to say so, or v2.3.0 takes the + // `detectedVersion === CURRENT` branch and refuses the whole file on + // `z.number()` rather than the one register. + describe('v2 to v3 migration', () => { + it('migrates a v2 config to the current version', () => { const v2Config = loadFixture('server-config-v2-current.json') const result = migrateServerConfig(v2Config) - expect(result.migrated).toBe(false) + expect(result.migrated).toBe(true) expect(result.fromVersion).toBe(2) - expect(result.config.version).toBe(2) + expect(result.config.version).toBe(CURRENT_SERVER_CONFIG_VERSION) expect(result.config.littleEndian).toBe(false) expect(result.warning).toBeUndefined() }) + + it('rewrites a 64 bit value a v2 file holds as a number', () => { + const register = (dataType: string, value: number): unknown => ({ + value, + params: { address: 0, registerType: 'holding_registers', dataType, comment: '', value: 0 } + }) + const result = migrateServerConfig( + JSON.stringify({ + version: 2, + modbuxVersion: '2.3.0', + name: 'v2', + littleEndian: false, + serverRegistersPerUnit: { + '1': { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: { + '0': register('uint64', 72623859790382850), + '10': register('uint16', 7) + } + } + } + }) + ) + + const holding = result.config.serverRegistersPerUnit['1']?.holding_registers + expect(holding?.['0']?.value).toBe('72623859790382850') + expect(holding?.['10']?.value).toBe(7) + }) }) describe('Future version handling', () => { - it('handles future version (v3) with warning', () => { - const v3Config = JSON.stringify({ - version: 3, - modbuxVersion: '2.0.0', + it('handles a future version with warning', () => { + const futureConfig = JSON.stringify({ + version: 9, + modbuxVersion: '9.0.0', name: 'Future', littleEndian: false, newField: 'something', serverRegistersPerUnit: {} }) - const result = migrateServerConfig(v3Config) + const result = migrateServerConfig(futureConfig) expect(result.migrated).toBe(false) - expect(result.fromVersion).toBe(3) + expect(result.fromVersion).toBe(9) expect(result.futureVersion).toBeDefined() expect(result.futureVersion?.savedByNewerVersion).toBe(true) expect(result.futureVersion?.fields).toEqual([]) diff --git a/src/shared/migrations/__tests__/legacyRegisterTypeKeys.test.ts b/src/shared/migrations/__tests__/legacyRegisterTypeKeys.test.ts index 9e879247..423849e4 100644 --- a/src/shared/migrations/__tests__/legacyRegisterTypeKeys.test.ts +++ b/src/shared/migrations/__tests__/legacyRegisterTypeKeys.test.ts @@ -53,7 +53,6 @@ describe('the text a config carries', () => { it.each(legacyNames)('keeps a server config named after %s', (name) => { const result = migrateServerConfig(serverConfigNamed(`${name} bank A`)) - expect(result.migrated).toBe(false) expect(result.config.name).toBe(`${name} bank A`) }) diff --git a/src/shared/migrations/server/config.ts b/src/shared/migrations/server/config.ts index 23beadae..eea22ad1 100644 --- a/src/shared/migrations/server/config.ts +++ b/src/shared/migrations/server/config.ts @@ -13,7 +13,8 @@ import { objectValues, dropUnservableConfigRegisters, parseConfigFile, - renameLegacyRegisterTypeKeys + renameLegacyRegisterTypeKeys, + stringifyExact64BitValues } from '../shared' import { V1ServerConfig, extractGlobalEndianness } from './shared' import { repairPersisted } from '../../repairPersisted' @@ -25,11 +26,12 @@ import { repairPersisted } from '../../repairPersisted' * the loop below counts the steps and this number only says where to stop, so * v1 to v2 writes 2 whatever this becomes. */ -export const CURRENT_SERVER_CONFIG_VERSION = 2 +export const CURRENT_SERVER_CONFIG_VERSION = 3 const SERVER_CONFIG_MIGRATIONS: Record> = { - 1: migrateServerV1toV2 - // Future: 2: migrateServerV2toV3 + 1: migrateServerV1toV2, + 2: migrateServerV2toV3 + // Future: 3: migrateServerV3toV4 } /** @@ -107,6 +109,30 @@ function migrateServerV1toV2(v1Config: unknown): ServerConfig & { wasMixedEndian return v2Config } +/** + * Migrate server config from v2 to v3: a 64 bit value is a decimal string. + * + * The three types whose composite fills 64 bits as an integer held a number, + * and a number carries 53 bits. The version moves because the file shape moved: + * without it a config this build writes still claims v2, and v2.3.0 takes the + * `detectedVersion === CURRENT` branch, fails `ServerConfigSchema` on + * `z.number()`, and refuses the whole file rather than the one register. + * + * `stringifyExact64BitValues` walks a persisted store, `serverRegisters[uuid]`, + * and a config is keyed one level up, so the blob is wrapped in the shape that + * helper reads. That is the same difference `migrateBoolShapeForUnit` carries. + * + * The bool shape goes with it. `migrateBoolShapeInConfig` ran on the + * `detectedVersion === CURRENT` branch alone, and a v2 file carrying the old + * `boolean` entries now takes this step instead of that branch. + */ +function migrateServerV2toV3(v2Config: unknown): ServerConfig { + const config = v2Config as ServerConfig & Record + migrateBoolShapeInConfig(config) + stringifyExact64BitValues({ serverRegisters: { config: config.serverRegistersPerUnit } }) + return { ...config, version: 3 } +} + /** * Convert old `boolean` bool entries to `{ value: boolean }` in a parsed config * object. Mutates in place. Safe to call on already-migrated data. diff --git a/src/shared/migrations/shared.ts b/src/shared/migrations/shared.ts index 8fa5ef0a..0e638bc3 100644 --- a/src/shared/migrations/shared.ts +++ b/src/shared/migrations/shared.ts @@ -252,7 +252,16 @@ export function stringifyExact64BitValues(state: Record): void const dataType = params.dataType if (typeof dataType !== 'string') continue if (!holdsExact64Bits(dataType as DataType)) continue - entry.value = String(entry.value) + // Only where `String` gives digits the schema takes. `String(0.5)` + // is `"0.5"` and `String(1e21)` is `"1e+21"`, and the union takes + // either as a number and neither as a string, so rewriting one would + // turn a blob that parsed into one that does not, and + // `repairPersisted` reads `serverRegisters` whole: every server, + // every unit. An integer above 2 ** 53 is the case this step is for, + // so the test is the digits rather than `Number.isSafeInteger`. + const asString = String(entry.value) + if (!/^-?\d+$/.test(asString)) continue + entry.value = asString } } } diff --git a/src/shared/utils.ts b/src/shared/utils.ts index bffb955d..f31097d7 100644 --- a/src/shared/utils.ts +++ b/src/shared/utils.ts @@ -255,18 +255,23 @@ export const holdsExact64Bits = (dataType: DataType): boolean => dataType === 'int64' || dataType === 'uint64' || dataType === 'datetime' /** - * The exact composite a stored value stands for, for a 64 bit integer type. + * The exact composite a stored value stands for, or nothing when it stands for + * none. * * Takes a `bigint` as well, because `ServerDelayedSetter` holds the composite * it last folded and that is the value the next word write reads first. + * + * `undefined` rather than `0n`, so a caller merging one word into a composite + * aborts rather than rebuilding it from zero: the other three registers are + * what zero would cost. `BigInt` throws on a fraction and on anything that is + * not digits, and `ServerRegisterEntrySchema` takes a fractional number for a + * 64 bit type, so a hand-edited config reaches this. */ -export const toExact64Bits = (value: ServerRegisterValue | bigint): bigint => { +export const toExact64Bits = (value: ServerRegisterValue | bigint): bigint | undefined => { try { return BigInt(value) } catch { - // A blob a hand edit left there, or the empty string. `dropUnservableRegisters` - // reads `params` rather than `value`, so this is the one that has no meter. - return 0n + return undefined } } From c3733babca6383b772e7c750d0ae7a6402f6677b Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 08:36:21 +0200 Subject: [PATCH 18/19] fix: the transaction log reads its address off the request frame `_logTransaction` read `rawTransaction.nextDataAddress`, and `shared/types/client.ts` declared it required. modbus-serial files it on two of its twelve transaction records, inside `writeFC4` at index.js:880 and `writeFC6` at 983, and `writeFC1` delegates to `writeFC2` while `writeFC3` delegates to `writeFC4`. So FC3, FC4 and FC6 carry an address and FC1, FC2, FC5, FC15 and FC16 do not, and `_columns.tsx` binds `field: 'address'` with no formatter, so the Addr cell was blank for every coil read, every discrete input read and every write Modbux sends. `_requestedAddress` reads it off the frame instead. Every `writeFCx` builds one as the unit id, the function code, and the data address at offset 2 as a big-endian word: measured over FC2, FC4, FC5, FC6, FC15 and FC16, which is every code Modbux sends. `_writeBufferToPort` stashes the frame while debug mode is on, and `_connect` sets `isDebugEnabled`, so a frame that went out has one. `nextDataAddress` is the fallback. The type stops asserting what the library does not file, and `TransactionSchema.address` is optional with it, because a request that never went out carries neither and a blank cell is the honest answer there. Reading 0 would be an address. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- CHANGELOG.md | 4 ++ e2e/specs/01-main/15-client-toolbar.spec.ts | 23 +++++++++ .../modules/__tests__/modbusClient.test.ts | 48 +++++++++++++++++++ src/main/modules/modbusClient.ts | 24 +++++++++- src/shared/types/client.ts | 24 +++++++++- 5 files changed, 120 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2d5d7ec..df0f987a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **The transaction log now shows the address of a coil read and of a write.** + The Addr column was blank for every coil read, every discrete input read and + every write Modbux sends, so a log of coil traffic said nothing about which + address each request went to. - **The unit ID scan no longer asks for more than a device can answer.** Its Length field took anything up to 65535 and sent it as the quantity, which is past what the protocol allows. Every device on the bus then refused the diff --git a/e2e/specs/01-main/15-client-toolbar.spec.ts b/e2e/specs/01-main/15-client-toolbar.spec.ts index 276a7124..e637a923 100644 --- a/e2e/specs/01-main/15-client-toolbar.spec.ts +++ b/e2e/specs/01-main/15-client-toolbar.spec.ts @@ -255,6 +255,29 @@ test.describe.serial('Client toolbar — display options and utilities', () => { await expect(logPanel).not.toBeVisible() }) + // modbus-serial files `nextDataAddress` on two of its twelve transaction + // records, so FC3, FC4 and FC6 carried an address and FC1, FC2, FC5, FC15 and + // FC16 did not: the Addr cell was blank for every coil read, every discrete + // input read and every write Modbux sends. + test('the log shows the address of a coil read, which FC1 files none for', async ({ + mainPage + }) => { + await selectRegisterType(mainPage, 'Coils') + await mainPage.getByTestId('show-log-btn').click() + + await readRegisters(mainPage, '4', '2') + + const logPanel = mainPage.getByTestId('transaction-log-panel') + const addressCell = logPanel + .locator('.MuiDataGrid-row') + .first() + .locator('[data-field="address"]') + await expect(addressCell).toHaveText('4', { timeout: 5000 }) + + await mainPage.getByTestId('show-log-btn').click() + await selectRegisterType(mainPage, 'Holding Registers') + }) + // ─── Register read config toggle ────────────────────────────────── for (const regType of ['Holding Registers', 'Input Registers']) { diff --git a/src/main/modules/__tests__/modbusClient.test.ts b/src/main/modules/__tests__/modbusClient.test.ts index 5386c24e..7d2f375b 100644 --- a/src/main/modules/__tests__/modbusClient.test.ts +++ b/src/main/modules/__tests__/modbusClient.test.ts @@ -1484,6 +1484,54 @@ describe('ModbusClient', () => { expect(tx.errorMessage).toBeUndefined() }) + // modbus-serial files `nextDataAddress` on two of its twelve transaction + // records, inside `writeFC4` and `writeFC6`, and `writeFC1` delegates to + // `writeFC2` while `writeFC3` delegates to `writeFC4`. So FC3, FC4 and FC6 + // carry one and FC1, FC2, FC5, FC15 and FC16 do not, and the Addr column + // was blank for every coil read, every discrete input read and every write. + // Every `writeFCx` puts the address at bytes 2 and 3 of the frame. + it('reads the address off the request frame when the library files none', async () => { + await connectClient() + appState.updateRegisterConfig({ type: 'coils', address: 300, length: 4 }) + mockModbusRTU.readCoils.mockImplementation(async () => { + fileTransaction({ + nextAddress: 1, + nextCode: 1, + nextLength: 6, + _timeoutFired: false, + // Unit id, function code, then the data address as a big-endian word. + request: Buffer.from([0x01, 0x01, 0x01, 0x2c, 0x00, 0x04]), + responses: [Buffer.from([0x01, 0x01, 0x01, 0x00])] + }) + return { data: [false, false, false, false], buffer: Buffer.from([0x00]) } + }) + + await client.read() + + const tx = getWindowCalls('transaction')[0]?.[1] + expect(tx.code).toBe(1) + expect(tx.address).toBe(300) + }) + + // A frame that never went out has neither, and the cell is blank rather + // than reading 0, which is an address. + it('answers no address for a request that carries neither', async () => { + await connectClient() + mockModbusRTU.readHoldingRegisters.mockImplementation(async () => { + fileTransaction({ + nextAddress: 1, + nextCode: 3, + nextLength: 10, + _timeoutFired: false + }) + return { data: new Array(10).fill(0), buffer: Buffer.alloc(20) } + }) + + await client.read() + + expect(getWindowCalls('transaction')[0]?.[1].address).toBeUndefined() + }) + it('removes the transaction it logged', async () => { await connectClient() setupHoldingRegisterReadMock([0]) diff --git a/src/main/modules/modbusClient.ts b/src/main/modules/modbusClient.ts index 3ba8a08b..4076fde3 100644 --- a/src/main/modules/modbusClient.ts +++ b/src/main/modules/modbusClient.ts @@ -680,6 +680,28 @@ export class ModbusClient { */ private _nextTransactionIdKey = (): string => String(this._internals()._port?._transactionIdWrite) + /** + * The data address the request asked for. + * + * Read off the request frame, not off `nextDataAddress`. modbus-serial files + * that field on two of its twelve transaction records, inside `writeFC4` and + * `writeFC6`, and `writeFC1` delegates to `writeFC2` while `writeFC3` + * delegates to `writeFC4`. So FC3, FC4 and FC6 carry one and FC1, FC2, FC5, + * FC15 and FC16 do not, and the Addr column was blank for every coil read, + * every discrete input read and every write Modbux sends. + * + * Every `writeFCx` builds its frame as unit id, function code, then the data + * address at offset 2 as a big-endian word: measured over FC2, FC4, FC5, FC6, + * FC15 and FC16, which is every code Modbux sends. `request` is stashed by + * `_writeBufferToPort` while debug mode is on, and `modbusClient` sets + * `isDebugEnabled` at construction, so a frame that went out has one. + */ + private _requestedAddress = (rawTransaction: RawTransaction): number | undefined => { + const request = rawTransaction.request + if (request && request.length >= 4) return request.readUInt16BE(2) + return rawTransaction.nextDataAddress + } + /** * Log the request filed under `transactionIdKey`, and only that one. * @@ -703,7 +725,7 @@ export class ModbusClient { id: `${transactionIdKey}__${v4()}`, timestamp: DateTime.now().toMillis(), unitId: rawTransaction.nextAddress, - address: rawTransaction.nextDataAddress, + address: this._requestedAddress(rawTransaction), code: rawTransaction.nextCode, responseLength: rawTransaction.nextLength, timeout: rawTransaction._timeoutFired, diff --git a/src/shared/types/client.ts b/src/shared/types/client.ts index 390f88eb..b9f77c02 100644 --- a/src/shared/types/client.ts +++ b/src/shared/types/client.ts @@ -64,7 +64,17 @@ export const TransactionSchema = z.object({ id: z.string(), timestamp: z.number(), unitId: UnitIdSchema, - address: RegisterAddressSchema, + /** + * The data address the request asked for, and undefined where it cannot be + * read. + * + * `_requestedAddress` takes it off bytes 2 and 3 of the request frame, which + * every function code Modbux sends puts it at, and falls back on + * `nextDataAddress`, which modbus-serial files for three of them. Both are + * gone only for a frame that never went out, and `_columns.tsx` binds + * `field: 'address'` with no formatter, so the cell is blank there. + */ + address: RegisterAddressSchema.optional(), code: z.number(), responseLength: z.number(), timeout: z.boolean(), @@ -247,7 +257,17 @@ export interface RegisterDataWords { export interface RawTransaction { nextAddress: number - nextDataAddress: number + /** + * The data address, filed by two of modbus-serial's twelve transaction + * records. + * + * `writeFC4` at index.js:880 and `writeFC6` at 983 set it, and `writeFC1` + * delegates to `writeFC2` and `writeFC3` to `writeFC4`, so FC3, FC4 and FC6 + * carry one and FC1, FC2, FC5, FC15 and FC16 do not. This said `number` and + * `_logTransaction` read it, so the Addr cell was blank for every coil read, + * every discrete input read and every write Modbux sends. + */ + nextDataAddress?: number nextCode: number nextLength: number // next: [Function: cb], From e7c56da7b9098a945dd8131f6744cb4021db7e5a Mon Sep 17 00:00:00 2001 From: Harted Date: Thu, 17 Sep 2026 08:50:50 +0200 Subject: [PATCH 19/19] fix: a broken 64 bit value repairs itself, and a v3 file gets the rewrite `/code-review` on `3bcb7e3..a0e32b6` found four things, all low, and two of them are the same defect from two sides. `toExact64Bits` answering nothing aborts `applyRegisterValue` before it rewrites the entry, so a stored value no composite can be read out of froze that register in the grid for good: a master writes all four words, main serves the new value, and the row keeps showing the old one with nothing saying why. The `0n` it answered before self-healed on the first write. `stringifyExact64BitValues` writes `'0'` where the digits are not there, which is what a register the server has not written holds, and a fraction on a 64 bit type is not a value any device wrote. The `detectedVersion === CURRENT_SERVER_CONFIG_VERSION` branch skipped the rewrite the version bump exists for. `ServerConfigSchema` takes a number for a 64 bit value as well as a string, and the persisted store's own step is gated on the store version and does not run on a config load, so opening a v3 file holding the number folded the first single word write from the rounded value. `_requestedAddress`'s docblock said `isDebugEnabled` is set at construction. It is set in `connect`, which matters: the disconnect timeout builds a replacement `ModbusRTU`, and that one carries debug off until the next connect. `getDisplayValue` stated the same claim twice, two lines apart. Claude-Session: https://claude.ai/code/session_018YrcWbyHtV9JPT4ppY4EZP --- src/main/modules/modbusClient.ts | 6 ++- .../ServerRegisters/ServerRegisters.tsx | 2 - src/shared/__tests__/migrations.test.ts | 38 +++++++++++++++++++ .../__tests__/registerAddress.test.ts | 12 ++++++ src/shared/migrations/server/config.ts | 7 ++++ src/shared/migrations/shared.ts | 20 +++++++--- 6 files changed, 75 insertions(+), 10 deletions(-) diff --git a/src/main/modules/modbusClient.ts b/src/main/modules/modbusClient.ts index 4076fde3..684b352b 100644 --- a/src/main/modules/modbusClient.ts +++ b/src/main/modules/modbusClient.ts @@ -693,8 +693,10 @@ export class ModbusClient { * Every `writeFCx` builds its frame as unit id, function code, then the data * address at offset 2 as a big-endian word: measured over FC2, FC4, FC5, FC6, * FC15 and FC16, which is every code Modbux sends. `request` is stashed by - * `_writeBufferToPort` while debug mode is on, and `modbusClient` sets - * `isDebugEnabled` at construction, so a frame that went out has one. + * `_writeBufferToPort` while debug mode is on, and `connect` sets + * `isDebugEnabled` on every client it opens, including the replacement the + * disconnect timeout builds, so a frame that went out over a connection this + * file made has one. `nextDataAddress` is the fallback either way. */ private _requestedAddress = (rawTransaction: RawTransaction): number | undefined => { const request = rawTransaction.request diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx index cc82d38b..77dc693e 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx @@ -53,8 +53,6 @@ const getDisplayValue = (register: ServerRegister[number]): string | number => { // The shared decoder answers '' for a register no date can be read out of, // which is what a register the server has not written yet holds. if (dataType === 'datetime') { - // Nothing to read a date out of is the same answer as a register the - // server has not written yet. const packed = toExact64Bits(register.value) return (packed !== undefined && parseIEC870DateTimeValue(packed)) || '—' } diff --git a/src/shared/__tests__/migrations.test.ts b/src/shared/__tests__/migrations.test.ts index 960ce81b..9e5459e2 100644 --- a/src/shared/__tests__/migrations.test.ts +++ b/src/shared/__tests__/migrations.test.ts @@ -153,6 +153,44 @@ describe('configMigration', () => { expect(result.warning).toBeUndefined() }) + // The persisted store's own step is gated on the store version and does + // not run on a config load, so without this the first single word write + // after opening a v3 file folds from the rounded value. + it('rewrites a 64 bit value a file of the current version holds as a number', () => { + const result = migrateServerConfig( + JSON.stringify({ + version: CURRENT_SERVER_CONFIG_VERSION, + modbuxVersion: '3.0.0', + name: 'hand edited', + littleEndian: false, + serverRegistersPerUnit: { + '1': { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: { + '0': { + value: 72623859790382850, + params: { + address: 0, + registerType: 'holding_registers', + dataType: 'uint64', + comment: '', + value: 0 + } + } + } + } + } + }) + ) + + expect(result.migrated).toBe(false) + expect(result.config.serverRegistersPerUnit['1']?.holding_registers['0']?.value).toBe( + '72623859790382850' + ) + }) + it('rewrites a 64 bit value a v2 file holds as a number', () => { const register = (dataType: string, value: number): unknown => ({ value, diff --git a/src/shared/migrations/__tests__/registerAddress.test.ts b/src/shared/migrations/__tests__/registerAddress.test.ts index 2218c4c9..e566b6a1 100644 --- a/src/shared/migrations/__tests__/registerAddress.test.ts +++ b/src/shared/migrations/__tests__/registerAddress.test.ts @@ -515,6 +515,18 @@ describe('a persisted 64 bit value', () => { expect(heldValue(state)).toBe(1234) }) + // `toExact64Bits` answers nothing for a value no composite can be read out + // of, and `applyRegisterValue` then leaves the entry where it is, so the grid + // would show that value for every write from then on while main served the + // new one. `String(0.5)` is `"0.5"` and `String(1e21)` is `"1e+21"`, and the + // schema takes either as a number and neither as a string, so neither can be + // written back as it is. + it.each([0.5, 1e21, -0.25])('repairs a value of %p that no composite reads', (value) => { + const state = migrateServerState(persisted('uint64', value), 7) + + expect(heldValue(state)).toBe('0') + }) + it('leaves a value already stored as a string alone', () => { const state = migrateServerState(persisted('uint64', '18446744073709551615'), 7) diff --git a/src/shared/migrations/server/config.ts b/src/shared/migrations/server/config.ts index eea22ad1..bfeb4aea 100644 --- a/src/shared/migrations/server/config.ts +++ b/src/shared/migrations/server/config.ts @@ -152,6 +152,13 @@ export function migrateServerConfig(raw: string): MigrationResult // Current version - migrate bool shape if needed, then validate if (detectedVersion === CURRENT_SERVER_CONFIG_VERSION) { migrateBoolShapeInConfig(parsed) + // A file this version writes carries a decimal string for a 64 bit value, + // and a hand-edited one can carry the number: `ServerConfigSchema` takes + // either. The persisted store's own step is gated on the store version and + // does not run on a config load, so without this the first single word + // write after opening such a file folds from the rounded value, which is + // the defect the v2 to v3 step removes for a v2 file. + stringifyExact64BitValues({ serverRegisters: { config: parsed.serverRegistersPerUnit } }) const result = ServerConfigSchema.safeParse(parsed) if (!result.success) { throw new Error( diff --git a/src/shared/migrations/shared.ts b/src/shared/migrations/shared.ts index 0e638bc3..dd27823c 100644 --- a/src/shared/migrations/shared.ts +++ b/src/shared/migrations/shared.ts @@ -252,16 +252,24 @@ export function stringifyExact64BitValues(state: Record): void const dataType = params.dataType if (typeof dataType !== 'string') continue if (!holdsExact64Bits(dataType as DataType)) continue - // Only where `String` gives digits the schema takes. `String(0.5)` - // is `"0.5"` and `String(1e21)` is `"1e+21"`, and the union takes - // either as a number and neither as a string, so rewriting one would - // turn a blob that parsed into one that does not, and + + // The digits, or `'0'` where there are none. `String(0.5)` is + // `"0.5"` and `String(1e21)` is `"1e+21"`, and the union takes + // either as a number and neither as a string, so writing one back + // would turn a blob that parsed into one that does not, and // `repairPersisted` reads `serverRegisters` whole: every server, // every unit. An integer above 2 ** 53 is the case this step is for, // so the test is the digits rather than `Number.isSafeInteger`. + // + // `'0'` rather than left alone, because `toExact64Bits` answers + // nothing for a value no composite can be read out of, and + // `applyRegisterValue` then leaves the entry where it is: the grid + // would show that value for every write from then on while main + // served the new one. Zero is what a register the server has not + // written holds, and a fraction on a 64 bit type is not a value any + // device wrote. const asString = String(entry.value) - if (!/^-?\d+$/.test(asString)) continue - entry.value = asString + entry.value = /^-?\d+$/.test(asString) ? asString : '0' } } }