Repository navigation
The audit's non-blocking half - #56
Merged
Merged
Conversation
Every setter in `client.zustand.ts` wrote the store and called main afterwards, so a payload the schemas refuse left the two sides on different values with one snackbar to say so, and C7 removed the channel that could read main's config back. `1,5` typed into a mask field is `NaN`, `UnitIdSchema` refuses it, `JSON.stringify` persisted it as `null`, and the next launch reset the connection settings to their defaults. `update_connection_config` and `update_register_config` now answer whether the payload was taken, and each setter writes only after that answer. `server.zustand.ts setPort` already carried the pattern. An invalid host or length is still written locally and never sent, because those two fields read their text out of the store. The three copies of the reset-the-rows condition became `clearRegisterDataWhenIdle`. The four test stubs answered `undefined` to every channel, which under this change refuses every payload, and one of them put that `undefined` in `clientState`. They are one helper now, answering the two config channels through the schemas `main/ipc.ts` guards them with. Measured in the built app: a round trip has a median of 0.2 ms and a maximum of 0.5 ms idle, 1.4 ms while the client polls a connected server, against 2.5 ms between two keystrokes typed as fast as Playwright types them. No field waits on the answer. lint 0, typecheck 0, `yarn test` 45 files and 945 passed, `yarn test:e2e` 582 passed.
`IMask.MaskedNumber` defaults to a scale of 2 and a radix of ',' with '.' mapped onto it, so a field that took the defaults accepted `1,5` and handed its setter a string `Number()` answers `NaN` to. That is where the config reset `0f53ced` guards against comes from, and the guard is the second line of defence rather than the first: nothing about the field said the value could not be a number. One `integerMask` now carries the rule, and the six fields that took the defaults spread it: the unit id, the length, the uint fields, the server port, the scan timeout and the add-register address. The interval and the register length stated the same thing by hand and now name it. Measured before the change, typing `1,5` into the unit id left `1,5` in the field. The unit tests for it go red on removing the scale, and the one that also refuses a dot is the only one a rival `radix: '.'` turns red, which is what tells the two rules apart.
`createServer` called `get().clean(uuid)` from inside its own recipe, and a `set` that runs inside a running recipe is discarded when that recipe finalises. Measured before the change: after `createServer`, `port` and `uuids` carried the new server and `serverRegisters`, `usedAddresses` and `unitId` were all `undefined`. It stood there since 352e4df. Nothing showed, because fifteen lines over eight actions rebuilt what `clean` had failed to build. That prologue is gone. One `unitRegisters` and one `unitUsedAddresses` make a unit the first time something is written into it, and the three actions that only read take the optional chain, so removing a bool from a unit that holds nothing no longer builds one. `clean` writes two empty maps rather than an entry for each of the 256 unit ids, which is what it filled before. Measured on the old shape: 21395 bytes of `serverRegisters` and 13203 bytes of `usedAddresses` per uuid in localStorage, for units the save filters out again through `checkHasConfig`. A config written by an older build still carries its 256 and still loads, which has a test. `getUnitId` is a read now. Two of its callers ask from inside a running recipe, where the `set` it made was discarded and attempted again on every call, and `init` and `clean` already write the '0' it was repairing. `addBool`, `removeBool`, `setBool` and `resetBools` send their IPC after the recipe rather than inside it, which is what `removeRegister` and `resetRegisters` already did. No test can see that one: the call goes out either way.
`littleEndian` travelled with every `add_replace_server_register` and every `sync_server_register`, and all three senders read the same field of the same server: `state.littleEndian[uuid]`. The v1 to v2 migration had already taken the setting off each register, so what the payload carried was the server's own stand, sent again on every write. Main keeps it now. `set_server_endianness` is the one channel that sets it, `addRegister` and `syncServerRegisters` read it where they encode, and a server main has not been told about is big-endian. `deleteServer` drops it, so a server built again on the same uuid starts big-endian too. The two places that encode say it first: `init` before it syncs a uuid, and `setLittleEndian` before it re-sends the registers. That order is what the payload used to guarantee for free, and it is what the store test asserts. `OpenSaveClear` does not await `setLittleEndian`, and the `init` at the end of its load is what settles the order for a config that carries the other one. Nothing persisted changed shape: the store still keeps `littleEndian` per uuid through `partialize`, and a saved config file still carries its own. No migration, and an existing config loads unchanged. Main had no test that passed a little-endian register: all 31 calls in `modbusServer.test.ts` passed false. It has two now, and they go red on main ignoring what it was told and on `deleteServer` keeping it.
`checkHasConfig` asked whether any coil or discrete input was on, where the
question is whether the user put one there. `addBool` writes `{ value: false }`,
so a coil added and left off is an entry like any other, and a unit holding
only those answered no.
Four callers ask it, and all four were wrong in the same way: the save filter
left the unit out of the file, the load skipped it on the way back in, the unit
id menu drew it as empty, and `extractUnitIdsWithData` did not sync it to main
at startup.
Counting entries rather than values fixes all four. `serverUtils.test.ts` had
the old answer pinned as `returns false when coils only have falsy values`;
that test now states the opposite and names the coil it is about. The e2e scene
adds two coils, saves, clears and reopens, and goes red on the old rule.
`refreshSerialPorts` and `refreshServerSerialPorts` were one function under two names, and they had diverged: only the server copy released its loading flag when the call failed. That flag disables both refresh buttons, so a flag left true is a button that never comes back. One `loadSerialPorts` serves both now, and the server store's two fields and its action drop the `server` prefix that guarded a clash between two separate stores. `validateSerialPort` had the same shape and gets the same `finally`. Nothing a user reaches changes: `listSerialPorts` in `modbusClient` catches its own failures and answers `[]`, so today only a change in main reaches the `finally`. That is also what kept the divergence invisible. The two branches of `init` held the same 26 lines, differing by a blank line and a two-line comment. `syncUuidToBackend` is those lines once. RTU and TCP differ in what they do to open the transport, not in what they put on it. The four serial setters were one five-line function written four times. `setSerialOption` is that function once. The restart they asked for was spelled one way in `restartRtuIfActive` and another in `applyServerCom`, and `restartRtuServer` is now both, taking `applyServerCom`'s shape: the stop is unconditional, because `stopRtuServer` returns at once when nothing is running and an empty COM field is a server that has to come down. `restartRtuIfActive` had no `.catch`, so a rejected `stopRtuServer` was an unhandled rejection. `defaultSerialPortOptions` states the four serial defaults once. The server store restated what `defaultConnectionConfig.rtu.options` already held, four fields with four identical values. Two specs for two paths that had none: the flag both stores release, and the RTU branch of `init` with the setters that follow it. Every rule was mutated one at a time and each went red for its own test. The `.catch` is the exception, because no assertion sees it, but vitest exits 1 on the unhandled rejection left without it. Measured: lint 0, typecheck 0, `yarn test` 50 files / 982 passed, and 100 e2e passed across `03-server-config`, `16-client-rtu` and `23-server-rtu`. `stores/F7`, `F8`, `F12` and `F14`, grouped as A19 in `tmp/AUDIT-triage.md`. A19's fifth finding, `F13`, named the two `clear` functions, and `8ca2b10` removed those.
Both stores ended their module with the same eleven lines: `repairPersisted`, and on a reset a `console.warn`, a `keepCorrupt` and a `setState` carrying `configReset`. Four of the eleven were byte-identical and the other seven differed only in the schema, the store, the version constant, the storage key and the noun in the warn. C3 put them there when it replaced the two `clear` functions, which is `stores/F13` in the audit and why that finding is retired rather than built. `repairPersistedStore` holds the read, the warn and the copy, and answers a repair only when something was lost. The write stays with each store: `setState` takes the store's own state type and the schema infers the persisted half of it, so a helper that wrote would have to be generic over both to say those agree, and the caller already holds both. That is what keeps this free of a cast. The warn names the storage key rather than the store, so it reads `client.zustand repaired`. No test asserted the old text. The explanation of why this reports through `console` and a store field instead of notistack now sits once, on the helper, where the server store used to point at the client store for it. `corruptedConfig.test.ts` already covered both stores across the lost field, the newer version and the whole blob, and it discriminates this: answering a repair when nothing was lost, dropping `keepCorrupt`, and comparing the versions the other way each turn it red, two of the three in both stores. Measured: lint 0, typecheck 0, `yarn test` 50 files / 982 passed, and 50 e2e passed across `03-server-config` and `02-standalone/01-persistence`.
`setRegisterMapping` sends main a copy per cell edit. `replaceRegisterMapping` and `clearRegisterMapping` sent nothing, so main went on grouping its reads out of the mapping the renderer had thrown away. What kept that from showing was LoadButton and ClearConfigButton each turning read configuration off by hand after the swap, and a poll firing in between read the old mapping anyway. The store does both now, in one action and in the order that leaves no window: read configuration off first, because it is the one thing that makes main read the mapping at all, then the mapping itself, now rather than after the debounce. Clearing is replacing with an empty mapping, so it is said once, and those two by-hand lines are gone. A config file that fails to load now leaves read configuration where it was. The line that turned it off ran whether the load threw or not, and a load that threw replaced nothing. `registerMappingToMain.test.ts` names both directions: what main is handed and in what order goes red on the previous store, and what the store itself holds plus the cell edit debounce stays green either way. Turning the toggle off after the flush instead of before reddens only the two order tests, and the debounced sync in place of the flush reddens the same five as the revert. Measured: lint 0, typecheck 0, `yarn test` 51 files / 991 passed, and 113 e2e across 05-file-io, 14-client-config-io, 15-client-toolbar and 17-huawei-smartlogger.
The scan's lifetime was written in two dialogs and in main, and neither side owned any of it. Both dialogs stopped polling before scanning, where main answered the same question by refusing the scan. A drop mid-scan stopped polling and left both scans running. Cancelling a unit id scan set a flag the loop never read, so every remaining iteration sent a client_state message across the boundary and did nothing else. Main owns it now. scanUnitIds and scanRegisters refuse a scan at a client that is not connected, which is the question _read already asks before a read, and they stop polling themselves. Both loops break on the connect state, so a connection that goes mid-scan ends the scan at the read in flight rather than after the whole range. _setDisconnected stops both scans alongside stopPolling, so the flag the dialogs read to enable Close is false in the same client_state that reports the disconnect. The unit id loop breaks on its own cancel flag. The dialogs lose their stopPolling calls, and ScanRegisters loses the clearScanUnitIdResults that belongs to the other dialog. Each rule above was mutated alone against modbusClient.test.ts, and each turned red exactly the test that names it.
ScanUnitIds greyed its Start button out while the client was polling and ScanRegisters did not, so the same question got two answers. Since main stops polling for a scan itself, the disabled button was the one place that still said no: the call that would have stopped the poll sat behind a button you could not press. The gate is not gone for good. Once logging lands, a poll that breaks off without saying so is the wrong behaviour and refusing the scan is the right one, but then it belongs on the menu buttons that open the dialogs rather than on Start.
…sconnect The mock hid this. Windows.send hands webContents.send a payload that Electron structured-clones, and _sendClientState sends _clientState itself, so the test mock kept a live reference where the renderer gets a copy. Every recorded client_state was the same object, and an assertion on an earlier send read the state as it is now. The mock clones what it records, and the two disconnect tests assert on the state the renderer receives instead of on the flag at the moment disconnect returns. That is what the defect needed to show. stopPolling sends a client_state, and inside _setDisconnected it ran while both scan flags were still true, so a drop mid-scan put out one state saying disconnected with the scan still running and another right behind it saying otherwise. The stops go before it now. Only _sendClientState sends a field rather than a literal it just built, which is why the other window mocks are left alone.
`startPolling` called again while a read was in flight started a second chain, so two reads sat on one client and the earlier chain's tail overwrote `_pollTimeout` with a handle `stopPolling` could no longer reach. Both callers can arrive there: `start_polling` passes on whatever the renderer sends, and the reconnect resume fires a second after the connect that scheduled it. A start now returns when the client is already polling, and a chain carries a generation that both `stopPolling` and the next start move past, so the tail of a chain that is no longer current arms nothing. Three tests in `modbusClient.test.ts` hold it. Deleting the start guard reddens the one about a second read, deleting the generation check reddens the other two, and putting the polling flag back in the generation's place reddens the one where a stop and a start land during a read.
`write` took a request during a poll or a scan and put it on the wire beside the loop's own reads. The renderer was the only thing saying no, and only about the poll: the write action cell and the bitmap panel disable on `polling`, so nothing shipped reached it, while a scan was covered nowhere. `read` refused a poll and not a scan. `_readLoopOwner` names the loop that is running, the poll or either scan, and both public entries refuse over it and say which one. The read after a write asks the same question, so a loop that started during the write gets no read, and the user no snackbar they did not ask for. `Already polling` reads `Cannot read during a poll` now, one shape with `Cannot write during a register scan`. Each clause and each call site was mutated alone. Dropping a clause reddens the tests that name that loop, dropping a guard reddens both refusals for that verb, and putting the polling flag back at the read after a write reddens the one where a scan starts during it.
`1bc3d64` is reachable with two clicks, Stop and then Poll while a read is on its way, so it gets an Unreleased line like the other user-visible fixes on this branch. `a4096b6` gets none: the Read button and both write paths are disabled while polling, so neither refusal it adds has a path to a user.
… was asked for The four read wrappers were two pairs differing in one token, and `_scanUnitIds` held four copies of one try, catch, progress block. A record keyed by register type holds the four reads now. `_read` and `_scanRegister` take the rows behind a reply; the unit id scan takes the same four functions and reads only whether the call threw. The scan iterates the register types it was asked for, so the cancel flag is read once at the top of that loop instead of four times between the blocks, and the two comments that named the wrong register type went with the blocks they sat on. Four tests, one per removed flag check, become one that cancels during the second of four types and asserts that neither of the remaining two is read and that the unit sends no result. Verified with `yarn lint`, `yarn typecheck`, `yarn test` and the e2e specs 06, 07, 08, 11, 12, 20 and 21.
`convertBitData` took `registerConfig.length` where the read took its own, so the row count came from the toolbar rather than from the request. For a read out of the toolbar those are one number and nothing changed. For a register scan they are two: the chunk size defaults to 100 against a read length of 10, so a coil scan sent ten rows for every hundred coils it read and the loop then advanced by a hundred. `_toBits` takes the length now. Two more callers pass a length of their own. A group from the register mapping carries its own count, which only a hand-edited config reaches for a bit type, because the grid mounts the data type and group end columns for input and holding registers alone and `groupAddressInfos` drops an entry with no data type. And the unit id scan built `registerConfig.length` rows per bit probe to drop them; it builds the probe's own now. The pair: a scan of a hundred coils in one chunk sends a row per address, and a toolbar read of three sends three rows rather than the eight booleans modbus-serial answers a one-byte read with. The first goes red on the old argument, the second on `result.data.length`. Verified with `yarn lint`, `yarn typecheck`, `yarn test` and the e2e specs 06, 08, 11, 12, 20 and 21.
The button's guard counted keys in the mapping. A comment is a key, so labelling one address switched it on, and read configuration reads data types: `showMapping` skips an entry without one and `groupAddressInfos` drops it. What the user got for pressing it was an empty grid and the address and length fields disabled until they pressed it again. Coils and discrete inputs reach it first, because the grid mounts the data type column for input and holding registers alone and a comment is all it writes into a bit mapping. It is not a bit-type defect though: a holding register mapping of comments only does the same, measured in the built app. So the guard asks the question the other two already ask rather than naming the types. Five specs, and each direction was mutated alone. The three that refuse go red on the key count, and the one that offers goes red on a guard that always refuses. The e2e specs that press the button cover the offering direction in the app and none of them moved: 14, 17, 19, 20 and 21, 94 passed. Verified with `yarn lint`, `yarn typecheck` and `yarn test`, 52 files and 1009 passed.
…ries `_read` grouped out of the register mapping for every type. Read configuration groups by data type and a bit type carries none, because the grid mounts the data type column for input and holding registers alone and a comment is all it writes into a coil. A config file is the exception: `RegisterMappingSchema` uses one object schema for all four types, so a hand-written data type on a coil address parsed and main grouped a bit read out of it. That was the only door left after the button stopped offering itself on such a mapping, and this is the second line behind it, the same order CU-05 took. Ignoring the mapping here rather than refusing it at the schema keeps the migrations out of it. The pair is the group test that already stood, holding registers at 0 and 100 reading twice, and a new one where the same two addresses under `coils` read once at the toolbar's address and length. The new one goes red without the type check. Verified with `yarn lint`, `yarn typecheck`, `yarn test` at 1010 passed, and the e2e specs 14, 19, 20 and 21 at 61 passed.
… a register value `modbusServer` said the same thing six times. The four vector getters were two pairs of identical bodies and the two setters a pair of their own. `_get` and `_set` take the register type and the getter's fallback, so `_getVector` names all six in six lines. `_unitData(uuid, unitId)` is the lookup that stood before each of six mutators. `_ensureInnerMap` reaches its return type through its parameter rather than through `new Map() as T`, which accepted any `T`. `boolean_value` is gone. `RegisterValue` is a union over the register type whose `value` is `ServerData[P][number]`, so one payload describes a coil and a holding register alike and one channel carries both. `ServerData`, `ServerRegisters` and `ValueGenerators` are mapped over `RegisterType` rather than written out. The shared types split three ways: `register.ts` holds the register-type schemas, `serial.ts` the baud rate, the parity and the port options, and `unitid.ts` the schema beside the list it is built from. The server store's helpers all sit in `server.zustand.helpers.ts`. A9, modbus/F13, F15 and F20.
`ServerDelayedSetter` reads `_updateCount` and nothing raised it. The two module-level batchers it replaced incremented on the test itself, `updateRegisterCount++ > updateRegisterCountMax` and the same line for bools, so folding them into the class dropped the `++` and left a comparison that is always false. `trigger()` then only ever cleared the pending timeout and armed a new one, which is a debounce with no ceiling: a stream whose gaps are all under 50 ms delivers nothing until it stops. Nothing reached it. `grep -rln "ServerDelayedSetter\|maxCount" src e2e` names the two source files and no test, which is why the three suites stayed green over it. Four specs, with the ceiling at three so a burst is ten triggers rather than two hundred and fifty. Two of them go red on the missing `++`, the burst that must flush and the second burst that must flush again; the other two hold the directions that already worked, the timer flush and the pending read. No changelog line: the defect was introduced in `a24454b` on this branch and never shipped, because 2.3.0 carries the two counters that worked.
… said twice Three comments the review read back. The one on `usedAddressesOf` restated the rule that `serverRegistersOf` states two declarations below it, and broke off mid-sentence doing it, so it is one line naming what the helper returns. The two on the `syncUuidToBackend` call sites were the first line of that function's own doc comment, copied, which made three places to keep in step. The aliases above `_getVector` claimed modbus-serial changing would be noticed. They are written out rather than derived from `IServiceVector`, so the assignment in `_getVector` is what checks them, and a version that widens a signature stays assignable. The comment says that now, and it no longer says "getters" while sitting above the setter too.
…the event it emits `ServerSerial.d.ts` declares `on(event: 'error')`, and `serverserial.js` emits `error` only from `sockWriter`'s `if (err)`. Its only caller `_callbackFactory` passes null on both branches, having turned the error into an exception frame by then, so that listener never fired. `socketError`, which a failure of the pipe under the server emits, is what it listens for now. The same typings declare neither `_serverPath` nor the `getPort()` that returns it, and that is where the `as any` and the inline `eslint-disable` came from. `ServerSerialWithPort` names the method, and the guard asking whether the port had an `on` goes with the cast. The open failure moves to `openCallback`, which the typings do declare and `serverserial.js` passes straight to `SerialPort`. `@serialport/stream` hands a failed open to it when one is given and emits `error` on the port when none is. The port's own `error` listener stays for the write path, where an unplugged port fails in `_write` and the stream emits. Nothing a user can press changes, so there is no changelog line. New unit tests cover the failed open, the open that succeeds and `socketError`. Verified: lint and typecheck clean, `yarn test` 1016 passed, and `23-server-rtu` with `25-disconnect-messages` against socat, 52 passed.
… was configured with `serverserial.js` builds its own option object out of `path`, `baudRate`, `parity`, `debug`, `unitID` and `binding`, and assigns it over a third constructor argument. The two it does not name reach `SerialPort` by no other route, and `startRtuServer` passed them in the second argument, where `SerialPortOpenOptions` accepts them and nothing reads them. So every serial server opened at the binding's own defaults, `dataBits: 8` and `stopBits: 1`, whatever `ServerRtuConfig` showed. `ServerSerial.d.ts` stops at two arguments, so a local `ServerSerialConstructor` names the third the way `ServerSerialWithPort` names `getPort()`. The client is clean: `rtubufferedport.js` hands its whole options object to `SerialPort`, so `connectRTUBuffered` has always carried both. No e2e can see it. A socat pty ignores line settings and the Arduino in the hardware specs is fixed at 8N1, so the unit test asserting what the constructor received is the only one that discriminates. It asks for 7 and 2 because 8 and 1 are what the binding falls back to. Verified: lint and typecheck clean, `yarn test` 1017 passed, and `23-server-rtu` with `25-disconnect-messages` against socat, 52 passed.
… place `stopRtuServer` cannot close a port that never opened. `ServerSerial.close` delegates to `SerialPortStream.close`, which takes its `!isOpen` branch and answers "Port is not open", and `stopRtuServer` swallows that by design. So a start on a port that is still opening leaves the old server's open in flight across the next start, and its `openCallback`, its port `error` and its `socketError` all wrote `_rtuActive` and sent `rtu_server_status`. The view then read Stopped for a server that was bound and answering. Its `initialized` was the same defect the other way: a port that opens late reported a server up that nothing would ever stop. The `close` listener was the only one asking, through `this._rtuServer !== rtuServer`, and an object identity is a question `openCallback` cannot ask because it is written before the object exists. `startRtuServer` takes a generation instead, `stopRtuServer` moves past it, and `_reportRtuDown` refuses a stale one for all four paths at once. `initialized` asks it for itself, because it is the one that reports up. No changelog line. The open failures this reaches in the shipped app return fast enough that the race was not measured, only read out of the two files. Verified: lint and typecheck clean, `yarn test` 1020 passed, and `23-server-rtu` with `25-disconnect-messages` against socat, 52 passed. Four mutations, each run alone: the guard in `_reportRtuDown` deleted, the guard in `initialized` deleted, the bump in `stopRtuServer` removed, and the old object identity put back on the close listener alone. Each reddened only the tests that name it, and the last one leaves the close tests green, which is what says the old guard was the right question asked in one place.
`stty -a` on a socat pty reads back what `tcsetattr` wrote, so a spec can see the frame a port opened with rather than the frame the UI shows. Measured on a pair: opening one end at 19200 7E2 gives `speed 19200` and `cs7 parenb cstopb` while the other end stays at the 9600 cs8 socat left it, and a round trip through the pair still works after the read. That corrects `82a7bed`, which said no e2e could see this. It can, and this spec is the one that would have caught the defect that commit fixed: with the data bits and stop bits back in the second constructor argument, the first case reads `["cs8", "parenb", "-parodd", "-cstopb"]` where it wants `cs7`, so the baud rate and the parity arrive and the other two do not. Four cases, each changing every axis the one before it set, so no assertion can pass on a frame the previous open left behind. Together they cover all four data bit widths, both stop bit counts, all three parities and four baud rates. A fifth test reads the client's end of the same pair, at a frame none of the server cases use. `parodd` is asserted only where `parenb` is on. Measured: the binding clears `parenb` for no parity and leaves the parity bit where the previous open put it, so a `-parodd` there would be asserting the case above. Verified: lint and typecheck clean, and `23-server-rtu`, `25-disconnect-messages`, `26-server-layout` and `27-serial-line-settings` run in order, 68 passed. Four mutations, each alone: the two options back in the second argument, the baud rate dropped, the parity dropped, and the client's two dropped in `modbusClient`. Each reddened the assertion that names it and left the rest green.
The Connect button is a Cancel for as long as the state is 'connecting', and pressing it sends `disconnect`. On a serial port that changed nothing: the open was still in flight, so `disconnect` took its `!isOpen` branch, and the connect resumed afterwards and wrote 'connected' over the cancel. Measured over socat, the button read Disconnect and the snackbar read "Connected over Modbus RTU". A generation says which connect the port that opens belongs to. `disconnect` bumps it, and a connect that comes back stale closes the port it opened and reports nothing. `close` is what takes modbus-serial's close relay off a port, so that port goes quiet as well as shut. Over TCP the cancel already worked: `TcpPort.destroy` destroys the socket, and `RTUBufferedPort` declares no `destroy` at all, so the branch `ModbusRTU.destroy` takes for a serial port only calls back. Both unit tests go red on the guard alone, and so does the e2e spec, on the "Connected over" the cancelled connect raises.
`ModbusRTU.destroy` destroys a socket and returns a serial port untouched: `RTUBufferedPort` declares no `destroy`, so destroy takes the branch that only calls back. The port stays open with modbus-serial's close relay on it, and the client the timeout replaces kept this file's handlers, so that port's own close would have arrived on the live `ModbusClient`, as a connection lost on the connection that replaced it. Forced through the destroy branch over socat, the app reported "Disconnected from server" and the next connect to the same port answered "Resource temporarily unavailable Cannot lock port". The message said "client destroyed" for a client that was not, and now says the port may stay open until Modbux closes. `removeAllListeners` needs a type written here, because `ModbusRTU.d.ts` declares `on` and nothing else of the `EventEmitter` the class extends. Nothing inside modbus-serial listens on the client object, so what comes off is what this file put on. The mock handed out one object for every `new ModbusRTU()`, which is what made an abandoned client indistinguishable from the one replacing it. It builds one per construction now, each keeping the handlers registered on it, and the timeout test that read the replacement's `destroy` reads the abandoned client's.
`showMapping` puts the mapping in the grid with `dummyWords` in every row, so a connected user was left reading zeros off a device nothing had asked. The store asks main for a read when the flag goes on, which is what the comment above `flushRegisterMappingToMain` already said would happen. `readWhenMainCan` holds the four states `read` refuses in, plus the one where nothing is connected, so neither this nor the byte order costs the user a warning it did not ask for. `setLittleEndian` had three of them written out and takes the same rule now, which adds the unit id scan it was missing. The read configuration toggle goes off while a read runs, because main refuses a second one. The effect below it that turns the flag off keys on an unreadable mapping rather than on `disabled`: a read in flight greys the same button, and turning the flag off there would empty the grid the read is about to fill. `recordApiCalls` moves into `stubRenderer`, where the second test that reads the order of the calls a setter makes can reach it. Measured: `29-read-on-read-configuration` reads 1 and 51 out of the server with nothing pressing Read, and holds the mapping with no value when disconnected. Reverting the one line leaves the first at 0 and the second green.
`write` called `_logTransaction` whatever the writers answered, and three refusals answered `undefined` the way a clean write does: the empty coil list in `_writeCoil`, and the single register and the UTF-8 or NONE data type in `_writeRegister`. `_logTransaction` took the last entry in `_transactions`, which belongs to whoever did reach the port, logged it as that write and deleted it. `_onReceive` returns on a transaction that is gone, so the request it belonged to timed out. That is C6b's shape on a path C6b did not take. The writers answer a `WriteAttempt` now, which says whether anything reached the port, and only a write that did is logged. `write` also refuses while nothing is connected. `writeFC5`, `writeFC6`, `writeFC15` and `writeFC16` answer a closed port with a `PortNotOpenError` before they file a transaction, so that path logged somebody else's transaction too, with the library's words in the snackbar. The coil write mock files a transaction the way `index.js` does and refuses a closed port, because a mock that files nothing hides what the fix is about. Measured: replacing the `attempt.sent` check with an unconditional log turns the three that assert a refused write logs nothing red; removing the connect guard turns the fourth red on the transaction it logged; switching the log off turns the two that assert a real write is logged red. `09-write-operations` and `21-bitmap-settings` pass.
`_logTransaction` took the last entry in `_transactions`, and the caller had no say in which one that was. `ec19c12` stopped a refused write from taking it, and left the half where a write that did go out takes somebody else's: nothing refuses a poll the user starts while a write is on the wire, so the poll's read files a second transaction and the write logs that one. Measured before the fix with a write in flight and `startPolling` behind it: the write logged `3__` with `code: 3`, the read's own request, and left its own entry 2 in the table. Then `_onReceive` returns on a transaction that is gone, so that read times out. Every caller names the request it filed. `_nextTransactionIdKey` reads `_port._transactionIdWrite`, which is the key `writeFCx` files under and the port increments once the buffer is out, so reading it immediately before the call names the request that goes next. `RTUBufferedPort` defines no such field, which is where the string "undefined" every serial transaction files under comes from, so one expression covers both transports. `ModbusRTUInternals` types the two fields `ModbusRTU.d.ts` does not declare. The empty coil list is refused for FC15 too. `writeFC15` writes `array.length` into the frame as the quantity of coils, so an empty list asked a device to write none, where FC5 had been refused since the guard was written. One guard above the branch answers both. The mock files transactions the way modbus-serial does, under the port's write id, and FC15 gets the same implementation FC5 had. The tests that keyed their transactions by hand file them through that helper now, because a key a test picks says nothing about which request it belongs to. Measured: addressing the last entry again turns the four refusal tests and the overlap test red; a read that stops naming its key turns nine red and the register scan one; restricting the coil guard to FC5 turns the FC15 one red; removing the connect guard turns the not-connected one red. `09-write-operations`, `21-bitmap-settings`, `11-scan-registers` and `12-scan-unitids` pass.
`dropUnservableRegisters` deleted the entry and left `usedAddresses`, which is persisted beside it. The only thing recomputing that map on launch is `syncUuidToBackend`, and it runs per unit `extractUnitIdsWithData` answers yes for, so a unit the drop emptied kept its entry, and so did every unit of a server whose port `createServer` refuses. Measured on a v4 blob through the store: a generator at 200 the interval floor refuses left `holding_registers: [200]` standing in both cases, and `isAddressInUse` is what reads that map, so the Add dialog refused an address the grid showed nothing at. The drop now rewrites the used addresses of every unit it took something from, computed from the registers that survived, so a register spanning two addresses keeps both. The rewrite sits behind the same version 6 gate as the drop. `v2.3.0` ships store version 3, so no released blob has run the drop yet.
Four findings from `/code-review` on `ff2ede8`. The rewrite was gated on having deleted something, so a blob that already ran the drop kept its stale map and never got a second chance. Store version 5 ran the address drop and not the interval one, between `5fc739d` and `5211399`, and it is below the gate this step sits behind: it runs again with nothing left to delete. Every unit walked is written back now, computed from the registers that survived, which answers the same for a unit nothing went from. A unit id `UnitIdStringSchema` refuses is skipped. `usedAddresses` is one persisted field, so writing `'300'` into it would cost the addresses of every unit beside it, where that key costs `serverRegisters` alone today. A `usedAddresses` that is not an object is left where it is. `repairPersisted` reads the field whole, resets it and names it through `FIELD_LABELS`, and a value replaced here is one it never sees. `usedAddressesOfUnit` reads `NumberRegistersSchema.options` rather than naming the two types again.
`addRegister` wrote the store and then told main, so a payload `AddRegisterParamsSchema` refuses left the grid drawing a register the server does not serve, with a snackbar beside it saying nothing changed. A3's shape, on the one server channel that carries a whole register. The channel answers the word now at the address rather than a yes. `modbusServer.addRegister` sends the `register_value` for a fixed register from inside the same call, and that event reaches the renderer before the answer does, where `setRegisterValue` drops a value for an address it has no entry for. Measured: with the channel answering `true`, a bitmap register added with value 5 drew every bit off, and `21-bitmap-settings` failed on bit 0. `submit` waits on that answer and says whether anything was written, because Add & Next asks for the next free address off the map the store writes. Measured: without the wait, `04-add-register-modal` failed on the address at 65535 staying unmarked, which is A27's case. The dialog's own test mocked `addRegister` with a bare `vi.fn()`, which answers undefined where the action answers whether main took it. No changelog line: every field the Add dialog sends is bounded by its mask and the button is gated on each one, so nothing shipped reaches the refusal.
The comment asked a question. The promise it sits under takes `resolve` alone,
so nothing rejects it and the catch runs only if `ModbusRTU.close` throws where
it stands, leaving a client whose close never started. `_setDisconnected` is
the answer, and the comment says that now.
T8 is retired rather than built. `write`'s parameter is `WriteParameters`,
which `5a78199` made a union over `z.literal('coils')` and
`z.literal('holding_registers')`, so the switch is exhaustive on the type.
Measured by deleting the `coils` case: `attempt` is then used before being
assigned and typecheck reports it at the log below.
Six findings from `/code-review` on `012cabd`. `ValueGenerator` writes its first value from its own constructor, so answering 0 for a generator threw that value away: the `register_value` it sends reaches a store with no entry yet and is dropped, and the grid read 0 until the next tick, which is a minute for a one minute interval. Both paths answer what the address holds now. The test that locked this in asked for a range of 0 to 10, where 0 is a word a generator can draw. An edit that moves a register removes the old address first, because the two spans can overlap and `removeRegister` erases everything the old one occupied. A refused write left the user with neither register, so the old one goes back when the new one is refused. The submit buttons go off while a submit is in flight. Both read the dialog through `getState()`, so two presses before the first answer wrote one register where the user asked for two. The e2e helper filled the next register before Add & Next had cleared the fields, which the clear then overwrote. It waits for the address to leave the one just written. Three snapshots of that field through `inputValue()` get no second chance either and are `toHaveValue` now.
… does `ServerPartTitleName` read `z.unitId[uuid]` and answered 0 when it was not there, where `ServerBoolRow`, `ServerBoolList`, `AddBoolInline`, `ServerRegisterRows`, `addRegister.zustand.ts` and the store's own mutators all go through `getUnitId`, which writes the '0' default in.
`SelectServer`'s add threw out of an async click handler, which becomes an unhandled rejection the user never sees. `findAvailablePort` walks 502 to 10502 and the button is off at ten servers, so nothing shipped reaches it. The count behind that button read `Object.keys(z.uuids).length` over an array.
`ServerRegisterValue` held the value in state behind a 10 ms timer. `ServerDelayedSetter` already batches every register write on a 50 ms timer, so the second delay debounced nothing, and `register` is a fresh object per write, which re-ran the effect whatever else was in its dependency list.
The load loop waited a millisecond after each unit, and nothing said what for. `replaceServerRegisters` is a store write with nothing behind it to wait on. Measured without it: `05-file-io` and `19-large-config`, 36 passed.
Both save buttons built the same seven DOM calls in the same order, each with its filename and its `JSON.stringify` interleaved. `downloadJson` in `components/shared/` is the anchor, and the two callers keep their own names.
`containers/Client.tsx` mounted both, six levels down inside `RegisterGridToolbar/MenuButton/`, where only the buttons that open them belong. `ScanRegisters/`, `ScanUnitIds/` and the `ScanProgress/` the two share now sit in `components/client/`, beside `RegisterConfig` and `ConnectionConfig`. `ScanUnitIds.tsx` exported two components with different owners. The button is its own file in `MenuButton/`, next to the register one. `SetAnchorProps` was exported by `ScanRegistersButton` and imported by three things, none of them about scanning registers. `MenuButton` owns it, and the three take it from there as a type.
`register.value` is the decoded composite, folded a word at a time from the `register_value` events. Answering the word at the base address put a raw word there instead: a fixed float of 3.14 read 16456, an int16 of -5 read 65531, and a unix register read as a 1970 date. Nothing corrected it, because those events go out before the answer does and are dropped for want of an entry. The channel answers the words it wrote, and the store folds them through the merge the event feeds, which is a named `applyRegisterValue` now. Same decoder, same 50 ms batcher, so what lands is what landed before `012cabd`. The three tests that missed this were uint16 and a bitmap, one word each. The submit buttons go back on in a `finally`. `invoke` rejects when a main handler throws, and only a refusal is answered rather than thrown, so a throw out of `createRegisters` or the `ValueGenerator` constructor left every button off until the dialog was closed and opened again. `04-add-register-modal` pressed Escape and waited 300 ms. Measured twice in a six spec run: the backdrop was still up when the next test clicked, and the click never landed. It waits for the modal to go now.
`_scanUnitIds` called the readers and never `_logTransaction`, so the scan's request per unit id and per register type never reached the transaction log, and what it filed in modbus-serial's `_transactions` stayed there for the session. The key is now read before the probe and the transaction logged after it, the way `_scanRegister` does.
`ServerBitMapDetail.handleToggle` writes the whole word back, and it read that word from `register.value`, which is two steps behind: the store writes it only once main answers, and hands it to the grid after 50 ms of quiet. Two clicks inside that window both derived from the word before the first, so the second cleared the bit the first had set. Measured at every gap from 0 to 80 ms: two bits back to back gave 2 where 3 was asked for, and a double click on one bit left it on. It reads `pendingRegisterValue` now, which answers the word the batcher is holding and falls back on the entry, the way `applyRegisterValue` already read the same pair. That leaves the round trip, which a double click is inside, so the panel keeps the word it last sent until its own write is answered.
`_read`, `write` and `_canScan` each asked `connectState !== 'connected' || !isOpen` and answered it differently: `_read` called `_setDisconnected`, the other two returned with a message and left the state saying connected. So a write or a scan refused for a port that closed underneath left the app showing a connection it no longer had, and which button you pressed decided whether it noticed. `_requireConnected` takes the verb its message names and corrects the state for all three. `_canScan` is gone, because that was the whole of it. The one caller with nobody to answer is a poll's own read, which passes `quiet`: the close that ended the poll has already spoken.
Both windows hold the server store and both persist it to one key, so the copy that writes last is the one the next launch reads. The window not showing the server never hears about a register the other one added, and one generator tick puts its copy back over the key. `sendTo` folds into `send(event, payload, to?)`, and `register_value` and `rtu_server_status` now go to the window showing the server. Those two are the only recurring writes a window not drawing the server makes: the rest are `init` at module load, the config reset acknowledgement, and actions on components that window has unmounted. Closing the split window hands the events back to a copy that has not moved since load, so the store re-reads the key at that point. Measured on a 1 s generator: without this, five of eight samples half a second apart had lost the register while the split was up, and after the close the key held it in none of eight.
… doing `rtu_server_status` fires on a change and now goes to the window showing the server, `rtuServerActive` is not in `partialize`, and no channel asked for it. So a window that was not the one showing the server at the last change had nothing to catch up on, and `ServerRtuConfig` is the one reader of the flag. `get_rtu_server_status` answers it, and the store asks at the two moments the flag can be wrong: a window coming up, and the main window taking the view back from the split out one. The first of those two predates the addressing. The handle now goes null on `closed` rather than `close`. It is what tells the main window to re-read the key, and on `close` the window being closed is still running and can persist once more after that read. `recordApiCalls` keeps the guard that names a channel it has no stub for, and answers `isServerWindow` the way the preload does. `valueGenerator`'s mock drops the `sendTo` the class no longer has, and the relaunch in the e2e spec clears the page it is about to look for.
Six client events reached every window: client_state, register_data, transaction, scan_unit_id_result, address_groups and scan_progress. Both windows load the same renderer, so the split out server window runs client.zustand as well, and its copy stops moving at what it loaded. Zustand's persist writes the whole partialized state on every setState, in node_modules/zustand/middleware.js, so an event delivered there puts that frozen copy over the shared key. Probed with the store on window in each renderer and a write counter per window. While the main window polled, the key held the read length it had just stored in 62 of 64 samples, and three restarts read it back, so the event path rarely wins the race. One write in the split out window with the main window quiet put its frozen length of 10 over the 7 the main window had stored, and the next launch read 10. All six now go to the main window, the way backend_message already did. The client view never leaves it. In the split out window Server renders MessageReceiver, the only reader of the client store there, and it reads configReset, which no event writes, so nothing has to re-read the key when the split closes. No CHANGELOG line: nothing a user can press lost a setting across the three restarts measured.
Twenty-six folders under src/renderer/src/components held one file and nothing else. CONTRIBUTING says a component that owns nothing stays a file, and names SliderComponent.tsx and HomeButton.tsx as the example, so those twenty-six were the rule's only exceptions. They are files beside their parent now, and no folder under components holds one file and nothing else. The other rule in play was 94d9304's, that a component does not lie loose beside its parent. Three of the five components that commit moved own nothing today, so it created three of the twenty-six. Ownership is the rule that is written down, and that is the one kept. CONTRIBUTING gains the clause that decided the count. A __tests__ of its own is ownership, which leaves RegisterConfig, ScanProgress, ServerBitMapDetail and ServerGrid/shared as folders. Without it they would have folded too, and each of their tests would have moved into the parent's folder among the tests of everything beside it. Nothing but import paths changed: twenty-six renames, no content edit in any moved file, and every other line in the diff is an import. Lint 0, typecheck 0, 1238 unit tests, and 104 e2e tests across 01-home-navigation, 03-server-config, 10-split-view, 15-client-toolbar and 26-server-layout.
Four of the five stores in context/ held their state type in a <name>.zustand.types.ts sibling and bitmap.zustand.ts declared it inline. All five do now. BitMapZustand moves with its two doc comments and nothing else changes. The finding, stores/F27, argued for leaving it: three members, nothing outside needs the type, so the file was right and the pattern was what was inconsistent. Jens took the other side. One shape for five stores is worth more than the one file that was already fine. Lint 0, typecheck 0, 1238 unit tests. No e2e: nothing but a type moved.
Every finding id in the six area documents was checked against tmp/AUDIT-triage.md, area prefix included. Twenty are absent: B-19, B-20, B-23, B-24, CU-16, CU-18, CU-19, CU-20, modbus/F21, modbus/F22, modbus/F24, server-ui/F18, server-ui/F19, stores/F18, F19, F20, F21, F22, F23 and F26. All twenty are criterion 3, size S, and each was measured against the code before it was touched. What was dead is gone. The preload's `process.contextIsolated` else branch could not run, and it held the repo's only two @ts-ignore, so the count is zero now. `offIpcEvent`, the `State` interface and the `shared/inputs` barrel had no importer; the barrel re-exported three defaults, which `export *` does not carry. `ipcInvoke`, SCAN_TIMEOUT_MIN, SCAN_TIMEOUT_MAX, outcomeOf, outcomeClass and ScanOutcome keep their bodies and lose the `export` nothing used. The two shift key listeners on Home fed `homeShiftKeyDown`, which nothing read. The `open_server_window` block at module scope read `hideHomeButton && !isServerWindow`, where `hideHomeButton` is initialised to `isServerWindow`, so it asked whether false was true; the macOS split it meant to restore needs a persisted flag, and nothing persists one. `PersistedLayoutZustandSchema` named a store with no persist at all, and the two fields it listed move into LayoutZustand. `bitmap.zustand`'s `collapse` had no caller. The client store's `version: '-'` was not in ClientZustand and duplicated the layout store's. Seven of the ten Modbus exception codes were referenced nowhere. Keeping them as the protocol's table costs a lint disable, which CONTRIBUTING forbids, so the three this server sends stay and the rest go. The commented-out socket-destroying debug harness in `createServer` goes with them. Four places said one thing and did another. `IpcHandlerSpec` named a promise for three channels and a value for the rest; it names the value everywhere now, because every call crosses the boundary and the preload wraps the lot. `process.argv.slice(2)` assumed the flag never lands in the first two slots, and main passes `--is-server-window` now. `rowHeight={40}` sat beside `getRowHeight={() => 'auto'}` in two grids, and getRowHeight wins. `lenght` is `length`, in the store, its type, the component that reads it and the test. Lint 0, typecheck 0, 1238 unit tests, and 167 e2e tests over 06-client-server, 10-split-view, 12-scan-unitids, 15-client-toolbar and the three standalone specs.
Harted
force-pushed
the
chore/audit-annoying
branch
from
September 23, 2026 21:32
ffabbc2 to
141061b
Compare
Harted
marked this pull request as ready for review
September 23, 2026 21:33
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Everything the audit found that was not blocking. PR #55 carries the blocking
causes and this branch is built on it, so the base here is
chore/auditratherthan
main.One commit per cause, each carrying the regression test that was red before the
fix.
Merge this with a rebase, not a squash. There are no fixup commits to hide:
each commit is one concern with its own message and its own test. A squash puts
all of it behind one subject line and takes the reasoning with it.
What a user gets
Full list in
CHANGELOG.mdunder Unreleased. This branch adds 27 entries to it,by subject:
Polling and reading do what the buttons say. Turning read configuration on
now reads instead of waiting for the next poll, and it is offered only where an
address carries a data type. Nothing else goes out in the middle of a read.
Pressing Stop and then Poll used to leave two read loops on one connection, so
the device was asked twice as often. A scan now stops at the read that failed
when the connection goes, and a scan of coils or discrete inputs shows every one
it read.
The transaction log shows what actually went out. A write took the last
request that went out, listed it as the write and took its entry away, so a read
still waiting for an answer timed out. A unit id scan sends a request per unit id
and per register type and the log listed none of them. A write or a scan refused
for a lost connection said it could not reach the device and left the app showing
it as connected.
The serial port opens the way you set it. The RTU server ignored the data
bits and stop bits. Cancelling a connect to a serial port did not cancel it.
The split out server window stands on its own. A register added there
survives a restart, the RTU status light asks what the server is doing instead of
waiting for a message that already went, and a message about the server reaches
the window showing the server.
Timestamps say what the registers hold. A DATETIME write carries the date you
typed, where its field's range was stated in seconds against a register encoded
from milliseconds. The date picker writes the date it shows and refuses a year
the register cannot carry. The server's DATETIME row reads the format's invalid
flag now, so whatever a client wrote into those four registers no longer comes
back as a plausible date. Add & Next no longer writes over the register it just
added, and an address freed on load is offered again.
A configuration file keeps what you put in it. A refusal names the register
and the field. A unit whose only content was coils or discrete inputs, none of
them switched on, was written to the file as nothing at all. A comma typed into
the unit ID, port, address or length field was stored as no number, and the next
start fell back to the defaults.
UTF-8 reads and writes its own characters. A value ran one register past the
group it was read in, so it showed two characters from the register after it.
Writing a value as UTF-8 sent one register of 0 over whatever the address held.
The grid draws what the server holds. Two bit toggles in a row both land,
where the second read the word from before the first. A row with a bitmap panel
open takes the height the panel needs, so the bottom of the list is reachable.
Under it
53 of the commits are
fixand 26 arerefactor. The refactors are the causesthe audit named as construction rather than behaviour: one generic accessor per
register type, one read per register type, the server owning the byte order it
encodes in, the two stores saying once what they both do, and the single file
folders folded back into files beside their parents.
How to test
Run it on the tip before this leaves draft.