The conventions round, and the audit that followed it - #55
Draft
Harted wants to merge 72 commits into
Draft
Conversation
Three write channels validate their payload before the handler runs: write, add_replace_server_register and set_bool. A payload that fails comes back as a message and a console entry, and the handler is never called. The schemas were missing rather than unwired. Twelve of the thirteen IPC argument types were hand-written interfaces, so WriteParameters, AddRegisterParams and SetBooleanParameters are now inferred from Zod like everything else that crosses a process boundary. A guarded handler is given the parsed payload, so unknown keys are stripped before anything acts on them. Nothing throws across the boundary, because an error there surfaces in the renderer as an unhandled rejection carrying the channel name and nothing else. A schema is only accepted on a channel that returns void. create_server answers with the port it actually bound and the renderer writes that into the port field, so those validate in their own handler instead. A write of an unparseable value used to send NaN to modbus-serial and now stops with a message.
ValueGenerators described its maps with the ValueGenerator class itself, which meant shared imported from main. shared is imported by all three processes and is the one layer that may not do that. Only dispose is ever called on a generator from outside, so the interface says exactly that and ValueGenerator implements it. Type-only either way, so no bundle changes.
Two channels were being called from a store and a component for the same reason, which put two writers on one piece of state. setRegisterMapping had a debounced sync in the store and an undebounced one in RegisterConfig, there because turning on read configuration reads immediately and cannot wait 150 ms. The store now exposes that flush, so the component asks for it instead of sending the mapping itself. getAppVersion was read into the store at startup and then asked for again by both save paths. They read the store, which already holds it. UpdateBanner keeps its own call on purpose: it is tested on its own, and coupling it to the root store to save one read of a value that cannot change is a bad trade. The other two the audit flagged are not duplicates. In the store, read is a consequence of flipping endianness and stopScanningUnitIds is a reload cleanup; in the components, both are the user pressing a button. Same channel, different concerns.
ServerRtuConfig, AddRegister, ServerBitMapDetail, BitIndicator and BitSettingsPopover sat next to a parent instead of in a folder of their own. AddRegister takes its store, its helpers and their test along, so everything that belongs to it is in one place. Nothing but import paths changed: eight renames, eight lines, and no content edit in any moved file. The wrapping in meme comes next and would be unreviewable on top of a move this wide.
At 900 lines and 27 components it was more than twice the next file in the renderer on both counts, and the only file the size rule actually catches. The split is by subject, not one file per component: AddRegister.tsx the dialog and its layout registerFields.tsx where the register lives and what it is called valueParameters.tsx what value it produces, fixed or generated addRegisterActions.tsx the buttons and the submit they share maskedInputs.tsx the six IMask wrappers The masked inputs sit together because they are six near-identical pairs of a forwardRef wrapper and its memo. Whatever gets done to one of them should be done to all six, which is easier to see in one file than spread through the dialog. Whether they want to be one parameterised input, next to the ones shared/inputs already has, is a separate question. Every declaration moved unchanged; only the import blocks are new.
It was 89 lines in a component file doing two unrelated things, and the only function declaration in the renderer, which is a fair hint it never landed anywhere on purpose. The pure half is a translation: what the dialog holds, all of it strings, into the params the server stores. That is toRegisterParams in the helpers, next to getRegisterSize and isAddressInUse, with eight tests over the conversions that were carrying real rules and no coverage. An interval is typed in seconds and stored in milliseconds. A unix timestamp is stored in seconds while a datetime picked in the same field is stored in milliseconds. A utf8 register falls back to ten registers. A generated timestamp reads the clock, so its min and max are pinned to zero. The other half writes through the server store, which makes it a state mutation, so it is now a submit action on the form store rather than something a button does for itself. While there: the next-address button was calculating a register size inline with the same four branches getRegisterSize already holds.
Two save handlers stayed async after reading the version off the store took their only await away. Both hang on an onClick, which discards what they return, so nothing else had to change. The split left its section banners behind. They sit above the component they introduce, and the splitter cut on the const line, so each banner stayed with the component before it. Min Max components ended up over the interval mask, Fixed Or Generator over the comment field, and three named things that had moved to another file or stopped existing: MAIN, Comment, Shared submit logic, and a note about an endianness button that was removed a while ago. Every banner now introduces something that is actually under it.
Ported from ~/rust/ploxc, which has ten skills and a CLAUDE.md that fits on a screen. Most of that is about Rust and about a compiler's own documents; this is the half that is not. CLAUDE.md holds the writing rule, the four invariants a session breaks when it does not know them, and what the two test loops cost. It imports CONTRIBUTING.md rather than repeating it. /prose is the rule with its trigger table: a sentence shape on the left, the command that settles it on the right. Its gate is not "did you measure this" but "which set did the command count, and is that the noun in the sentence" — the failures it is built from all passed the first question. Its two reference files carry this branch's own failures rather than the ones it was ported with. A component census that counted `^const` declarations under a sentence about components. A step sized as wiring when twelve of thirteen argument types had no schema to wire. Six section banners that stopped naming what was under them after a split, none of which failed anything. The measurement in CLAUDE.md was itself written from memory first, at fifteen seconds, and the loop takes seventy-five.
A skill runs only when someone remembers it, and the moment prose needs checking is a write, which no description can name. So the trigger is the write itself: a markdown file, or an edit that adds a comment. It is a reminder and never a block. The first firing of a session states the rule and every one after asks two questions, because a hook that argues on every edit becomes wallpaper. The hooks live under .claude/ rather than in tools/, which holds scripts that belong to the app. Nothing under .claude/ is part of the build: no package.json entry, no postinstall, no CI. They are .mjs on node for the same reason — the least important thing here should not be what decides that this project has a second runtime. Both directions are pinned in the suite. The negative cases matter as much as the positive ones: a matcher narrowed to kill a false positive is how the false negatives get made. The payloads are built rather than typed, because a hand-escaped one in a shell went through unparseable and the hook read that as nothing to say, which looks exactly like a matcher declining.
The routine this branch has been running by hand: read the diff, lint, typecheck, the unit suite, the e2e specs the change reaches, report, then commit. It was in nobody's file. Ordered by what gets skipped rather than by when it happens, so reading the diff comes before running anything and the prose pass is a step rather than an afterthought. The e2e rule is split in two on purpose: the specs a change touches while you work, the suite once at the end of a branch. Running it per commit is how a branch stops being worked on. Two references carry this branch's own incidents. A build artefact that rode in on `git add -A` and was caught a commit later while reading a file list for another reason. Six section banners a split left naming the wrong thing, where lint, typecheck, 591 unit tests and 86 e2e specs were all green — a banner is prose, and prose has no suite. The claim that `yarn test` does not typecheck is pasted as what the two commands print on the same file rather than asserted.
Which tests a change needs, and the proof each one can fail. Two rules carry it: every fix ships a pair, and a test written after the fix proves nothing until you have watched it go red. Its own rule was applied to work from earlier on this branch. The eight tests over toRegisterParams were written against code that had just been extracted, and passed on the first run — which proves only that they agree with what they were copied from. Mutating each rule separately turned exactly one test red per mutation, which is what makes the set discriminating rather than overlapping. The e2e section is about this suite rather than about testing. One app, one worker, no retries, every spec serial and cleaning up before it starts rather than after it ends. The number in the filename is the order, and a spec that needs what an earlier one built breaks when run alone. The table of what each suite can see is there because the fast loop cannot protect a behaviour only Playwright reaches, and that is worth saying while the test is being written rather than when it breaks.
A working list for whatever is in flight. Modbux ships, so a todo here is tied to the branch being worked rather than to a roadmap, and anything that should outlive the branch belongs in a GitHub issue or the changelog. Not tracked, for the same reason tmp/ is not.
Two gaps, both found by using it. The reminder degraded to a one-line question after its first firing. That short form is the one that gets read past, and the sentence being written is what is at stake, so it states the whole rule every time now. The session marker it needed for that is no longer imported here. And it never saw a commit message. Those are written with `git commit -F -` through Bash, and the matcher was `Write|Edit`, so five commit messages on this branch went past it while precommit says every commit hands over unconditionally. Bash is on the matcher now. The commit match is anchored to the start of a command or to a separator rather than found anywhere in the string. Matching anywhere fired on the very run that added it: the command was a heredoc writing a test *about* `git commit`. That shape is now one of the cases.
Four destinations, and the first step is not one of them: is the fix small enough that it belongs in this commit instead. Writing a defect down converts it into a backlog item, and backlog items feel handled. The routing differs from the repo it came from because the destinations do. There is no docs/decisions/ here, so a reason goes to the memory directory, which is where every decision this branch made already lives. TODO.md is untracked, so anything another person needs is a GitHub issue rather than a note in it. The tense is the test. "Enable require-await" is a task; the paragraph explaining how two handlers came to be async without an await is a record. If the sentence explains, it is not a task. Its reference carries the gauge that caught the same drift in ploxc, with both figures attributed rather than restated as measurements: a fact from outside this repository cites its source or it goes.
The checkpoint settled one rule: every component, props or not. A meter that accepts a declaration rendered as JSX somewhere or exported as its file default reads 104 wrapped and 86 bare, identically on main and on this branch, and a half-applied rule is what produced that split. The 86 are wrapped here, in one commit, because doing it in several is how the split happens again. The comparator is untouched. The four forwardRef inputs take the shape the rest of the codebase already uses: a named Forward component carrying the displayName, wrapped by meme. Assigning displayName to the memo instead would have left the inner component anonymous. One test changed. SerialGroupModal's store stub was a plain object read during render, so the test for a newly plugged adapter drove it by re-rendering the parent with identical props, which memo refuses. The stub is a real zustand store now, and the change reaches the component the way it does in the app. Dropping ports from that effect's dependency list turns that one test red and no other.
The checkpoint settled the filename: <name>.zustand.ts, matching the global stores in context/, and named after the component rather than the folder. Three files were still called _zustand.ts. They are serialGroupModal, privilegedPortModal and scanUnitIds now, and their seven importers say so. WriteModal carried its store inline, which made it the exception the rule would have had to remember. It comes out to writeModal.zustand.ts, and the eslint-disable header goes with it: it was there for the store's setters, and WriteModal.tsx lints clean without it. The typo in ValueInputZusand moves unchanged. Renaming it is step 8's, and doing it here would hide a rename inside a move.
The checkpoint settled deep imports. It counted 42 sites, which was @mui/material alone; the sweep also takes @mui/icons-material, @mui/x-data-grid and @mui/x-date-pickers, because the rule is about barrels and those are barrels. 80 barrel imports across 57 files. Every target was chosen by compiling it rather than by the shape of the name, and three of them would have gone wrong the other way: GridRowProps lives in components, not models, and DateTimePicker and LocalizationProvider are named exports rather than defaults. Five sites keep the barrel. The exports map in @mui/x-data-grid/package.json declares thirteen subpaths besides the root, and useGridApiContext and useGridApiRef are exported by none of them, so the root is the only place they can be had.
Eight frozen typos: RootZusand, ValueInputZusand, typestampColumn, Connnection, Remeber, mobusServer.ts, lillteBigEndian.md, and the one a user reads, "Single register only supported fot 16 bit values". Six interfaces called Props are named after the component they belong to. Ten interactive elements carry a data-testid that did not. @main, @preload and @backend are gone from the vite config. All three resolved nothing: no file imports through any of them. src/backend was real between ec4898c and 3dea261, and the alias outlived the directory by more than a year. #2A2A2A was not a colour someone picked. The grid computes its own row background in dark mode as color-mix(in srgb, #1F1F1F 95%, #fff), which is 0.95 * 0x1F + 0.05 * 0xFF = 42.2, so #2A2A2A, and the two panels behind the server lists were a hand-copy of that. The value is named once as gridSurface and pinned into palette.DataGrid.bg, so the grid and the panels move together. x-data-grid augments PaletteOptions but not Palette, which is why the panels read the export rather than the theme. Both #cccccc sites read palette.info.main now, and the Ploxc logo's other fill reads palette.primary.main beside it. Found while measuring: the DateTimePicker already had add-reg-datetime-input, nested in slotProps where a meter looking at JSX attributes does not see it. Anything counting data-testid has to look there too.
32 sites, all of them in src. It reads a flag beside scanningRegisters and is set by the same scan that stopScanningUnitIds ends, so the plural is what the neighbours already say. No migration. partialize in root.zustand.ts persists name, connectionConfig, registerConfig and registerMapping, and clientState is not among them, so the old key was never written to storage and no saved config carries it.
Seven rules the run settled, each one a test: every component wrapped in meme, nothing in shared importing from main, no selector returning an object and no useShallow, every store file named <name>.zustand.ts, MUI imported deep, a data-testid on every interactive element, and every configured alias imported through. Each rule asserts twice. The second assertion is that the population holds no violation; the first is that the population is not empty, because a meter that reads no files passes every rule it has. It found two things the manual sweeps missed, before it was committed. ScanRegisters.tsx held a seventh local store inline, which is exactly what WriteModal.tsx was taken apart for one commit earlier. And vitest.config.mts still declared @main, @preload and @backend after they left the vite config. Both are fixed here, which is why this commit is green. Every rule was mutated once and watched go red: the meme wrapper removed from HomeButton, an @main import added to shared/default.ts, a selector rewritten to return an object, useShallow imported, a deep MUI import put back on the barrel, a data-testid deleted, a store file renamed off .zustand.ts, and a @ghost alias added to vitest.config.mts. Each turned exactly one test red and left the other fourteen green. The data-testid rule reads the whole attribute text rather than the top-level JSX attributes, because a picker carries the attribute inside slotProps and a meter that looked only at the top level reported the DateTimePicker as missing one it has had all along.
Seven rules were enforceable only by pointing at existing code. They are under Code style now, each one saying what it is and why, next to the test that fails when it is broken. An eighth is there under its own heading, because no test can see it: the store owns IPC that changes state and a component owns IPC the user asked for. The same channel can be called from both and be right both times, which is what makes it a reviewer's judgement rather than an assertion. Two corrections. The path alias line listed @main, @preload and @backend, none of which exist; it is @renderer/* and @shared, and there are no others. And the data-testid bullet said every interactive element without saying which elements are not one, so a ToggleButtonGroup and a Select's options kept turning up as findings when they are addressed through their children.
The other fourteen. Three write paths were already guarded; the rest of the seventeen channels taking an object or a union now declare a schema beside their handler, so a payload that does not parse never reaches the socket. The count is a test: a channel added without one fails yarn test. Ten types had no schema: nine hand-written interfaces and a string union. The leaf schemas were already sitting beside them, so the new ones are assembly rather than invention. Two ranges the protocol fixes are named once in shared/types/ranges.ts and read from there: a register address is 16 bit, a unit id is one byte. A TCP port is 16 bit too and means something else, so it has its own name. The constraint on where a guard may go changed, because the old one was drawn in the wrong place. It read "only a channel returning void", on the grounds that a rejected payload leaves nothing to return. What actually matters is whether undefined is an honest answer, so it reads that now, and the three value-returning channels say so in their own types. create_server and set_server_port answer Promise<number | undefined>. That is not cosmetic: the renderer wrote String(actualPort) straight into the port field, and String(undefined) is valid TypeScript, so the type alone would not have caught it. All three call sites check before they write. Two tests per channel, through initIpc rather than around it. The valid payload must reach the listener, which a swapped schema breaks. The invalid one must come back as a message naming the channel, which a missing schema breaks: a channel with no guard accepts everything, so passing it a valid payload proves nothing.
A review of the two step 9 commits found seven claims that did not hold. Three were the suite falling short of what the doc promised, and those are fixed in the suite rather than walked back in the prose. The suite accepted React's bare memo as satisfying the meme rule, so a component could take the shallow comparator the rule exists to rule out and stay green. Only meme counts now. It also asserted nothing about whole-store subscriptions while the doc said the renderer has none: a call with no selector and one returning (z) => z are neither an object literal, so both walked past the check that was there. And CONTRIBUTING.md said every configured path points at something while nothing read the tsconfigs, where tsconfig.node.json still included src/backend, deleted in 3dea261 along with the alias step 8 removed. Reading the directory part off a glob got electron.vite.config.* wrong twice, once in each direction, so the include check expands the glob instead. Four were the doc overreaching. The folder-per-component rule was under the heading that says these are asserted, and it is neither asserted nor true: seventeen components sit flat, and columns/ holds a WriteModal folder. It moves down beside the IPC rule, as the judgement it is, saying what it actually distinguishes. The data-testid paragraph read as though a Select needed no attribute when the suite requires one on it and exempts only its options. Both files said seven rules when the eighth landed in 24285f8 and a ninth here. And "each rule asserts twice" was wrong for the selector rule, which had three and now has four. The sentence says what is actually true of all nine: the population is checked before the violations are.
version and setVersion are the running app's own version, read once at startup from get_app_version and written into a save file's metadata. Nothing about them is a client. They sit on the layout store now, which is the other app-wide one and which persists nothing, so the move touches no storage. Its three readers say so: the two save paths and the version in the corner of the home screen.
44 of its 46 members were client state, and the other two left in the commit before this one. It is client.zustand.ts now, useClientZustand, ClientZustand, and migrateClientState in migrations/client/ where the folder already said so. The storage key moved too, and that part is not a rename. persist reads one key and builds an empty store when it finds nothing, so an upgrade would have come up with no connection config, no register config and no register mapping. There is no version bump that fixes that: migrate runs on what was read, and nothing was read. carryFormerStorageKey copies root.zustand to client.zustand before the store is built, and leaves the old key where it is so a build that goes back still finds its config. Five unit tests over the copy, each watched failing: the move itself, the old key surviving it, the guard that stops a second launch overwriting what the user changed since, the empty case, and storage that throws while the module graph is still loading. Whether that runs before persist reads is a question about module load order, so only the running app can answer it. 02-standalone/01-persistence writes a config, puts it back under the old key, restarts and finds it. Removing the one call from client.zustand.ts turns exactly that test red.
It was a file of its own, client.zustand.storage.ts, and the reason was the test: the store's own module builds the store on import, so a function inside it cannot be called without one. That is the test choosing the file layout, which is backwards, and it invented a filename shape the project does not use. It sits in migrations/client/zustand.ts now, beside migrateClientState and the version it belongs to. The storage is a parameter rather than a reach for localStorage, because shared is imported by main too, and that also makes the test hand it a Map instead of needing a DOM. Same five cases, each watched failing again after the move.
43 variables held a whole store under six different names: state 26 times, z 12, dataState twice, and one each of server and currentRootState. They are clientZustand, serverZustand, addRegisterZustand, dataZustand, layoutZustand, scanUnitIdZustand and scanRegistersZustand now, after the hook that produced them. z keeps the job it is good at. In a selector it is the state accessor, (z) => z.pollRate, where a longer name would bury the field being read. Holding the whole store is the other case, and there the name says which one. Renamed through the TypeScript language service rather than by search. state is also the mutative draft inside set((state) => ...), 35 times in client.zustand.ts and 40 in server.zustand.ts, often in the same file as a store variable of the same name. A textual replace takes the draft with it; a scope-aware rename knows which binding it is on. The drafts are untouched.
Six areas, split by what the code shares rather than by directory. Eight criteria, of which one is new here: an assumption about modbus-serial is a finding unless you opened the file in node_modules that implements it. The two sharpest things this project knows about that library came from doing exactly that, and neither was visible from src. The conformance suite asserts nine conventions and is green, so the audit does not look for those. It looks for what a test cannot see. Findings are proposals, not instructions, and the skill says what each part of one is worth. The recipe and the proposal are the two least trustworthy fields: a recipe describing an input that does not trigger the behaviour is the most common defect in an audit, and a proposal is a guess by someone who did not read the rest of the file. Two agents per area, and the second one is not a reviewer. It is handed a claim and asked to demonstrate it wrong, which is a different job with a different failure mode. The bar for rejection is high: a wrong finding costs one look, a dropped correct one costs a defect in a build. No verdict counts as kept. Ported from ploxc's audit-area, which needs a docs/map/ this repo does not have, so the output goes to tmp/. Both references carry this branch's own failures: the four meters that miscounted here, and the two modbus-serial findings. A ported anecdote about a compiler teaches nothing in a Modbus client.
Six areas audited in parallel gave 28 blocking findings that were thirteen causes. Three of the thirteen could not be seen from any single area document: one defect was reported three times, from the function that clears, the button that calls it, and the helper that answers the width, and none of the three names the file where the fix belongs. So the skill gains a step between auditing and fixing. Group the blocking findings by cause, write it to tmp/AUDIT-clusters.md, and check with a loop that every finding reached it. The grouping is itself a new way to be wrong: a cluster that is really two causes gets fixed as one and the half nobody looked at survives with the ticket closed. The refutation reviewer is handed the grouping along with the claims and asked to break that first. And a cluster's size is not the largest size inside it. Every finding was sized within one area by someone who could not see the others, so four of these thirteen need a decision that fits in no single area.
The audit's first blocking finding. Both persisted stores validate at module scope and reset themselves when the schema refuses, and both reported that through notistack from there. notistack assigns its standalone enqueueSnackbar inside the SnackbarProvider constructor, and main.tsx builds that provider in createRoot().render(), which runs after every module has evaluated. So the call threw out of module scope and took everything below it: no init, no event listeners, and no render either. A user with a corrupt config got a blank window with no UI left to clear it from. The stores record the reset on themselves now. MessageReceiver reads the flag and tells the user, from under the provider where there is something to tell. It acknowledges after telling, because it mounts inside Client and Server rather than at the root: without that, walking to Home and back reports the same reset again. Five tests in context/__tests__, which the audit noted was empty. Restoring the old call turns two of them red and leaves the other three green. Not run: the e2e suite. This changes the renderer's startup order, which is the wrong place to skip it, but five audit agents were writing scratch specs into src/ and e2e/ while this was being fixed and a run would have covered their files rather than this one. It is the first thing to run against this commit.
The opening thanked the reader and said the guidelines exist to keep the review smooth. Neither is a claim, an order or a measurement, which is what this repo asks of a sentence. One line replaces both, and it points at the conformance test. Two of the ten rules told what went wrong once instead of what the rule is. The path-alias rule named @main, @preload and @backend, none of which appear in any tsconfig or in electron.vite.config.ts any more, and src/backend is gone, so its whole body described a state nobody can check. It now names the two aliases that exist and says when one retires. The include rule kept its second sentence, which says how the test works, and lost its first. An action is fetched where it runs now names both shapes, after the refactor that made a pass-through prop take the action itself. Three em dashes and one -- left over from stripping one, each rewritten as its own sentence. The Arduino paragraph said twice that the board is found by USB vendor ID and skips without one. Still standing: What will get your PR rejected repeats ground rule 3 and two lines of Code style.
The harness reads a non-zero code as something to show the user, and 2 as a refusal, which on PreToolUse blocks the tool call. Nothing pinned that here, so a hook that dereferenced a bare payload would have blocked whatever it fired on and no test would have said so. The list of hooks under test is checked against .claude/settings.json rather than kept beside it, or a hook wired and not listed sits outside the claim. Both directions proven: wiring a sixth hook turns the list test red, and dropping one optional chain in precommit-trigger turns its exit test red. Ported from scripts/hooks/hooks.test.ts in the ploxc repo, which pins the same rule.
persist merges shallowly, so a persisted connectionConfig of {} replaced
the whole default sub-object rather than being filled in, and the check
over the whole state then failed on the merged result. That failure was
answered by clearStorage, which took every sibling with it: a register
mapping built by hand went along with the field that broke, and nothing
was kept to recover it from.
repairPersisted checks each field the schema names on its own, keeps the
ones that parse, defaults the ones that do not, and returns their names.
Both stores now report a ConfigReset instead of a boolean, and
MessageReceiver says which fields went rather than that something did.
The blob is copied to <key>.corrupt-<timestamp> before the reset. The
reset is what makes the app usable again and also what destroyed the
evidence, and a mapping worth hundreds of rows is worth having in a bug
report even once it is unreadable.
A state saved by a newer version went through migrate untouched and was
cast to the schema's type without ever meeting it. persist calls migrate
for any version that is not the current one, so the number is recorded
there and read once: the fields that still fit are kept, the rest reset,
and the message says where the config came from. A newer config that
loses nothing is still reported, because a field this version does not
know is a field it silently dropped.
Nine mutations, one per rule, each red on its own test. The non-object
guard survived the first attempt: its rival read a string as a record and
answered the same for every input the tests give it.
Opening the server in its own window dropped every connected master. The second window loads the same renderer entry, so `server.zustand.ts` runs its module-scope `init()` again, and in TCP mode that calls `createServer` per uuid. `createServer` closed the existing `ServerTCP` and bound a new one, and `ServerTCP.close` destroys every socket in `modbus.socks`. Measured in the running app with a raw socket on 502: `["connected","FIN","closed hadError=false"]`, and `["connected"]` after this change. `createServer` now answers with the port when `_servers` already holds a listener for that uuid on it. The rebind bought nothing: the vectors read `_serverData` when a request arrives, so a port change is the only reason to rebind and `setPort` owns that. `resetServer` therefore keeps its listener too, which is the same fix for clearing a server's registers. A refused bind was invisible in the same way. `new ServerTCP()` returns before `listen` has finished and reports a failure as a `serverError` event rather than a throw, so `createServer` returned a port while `_servers` held a listener that was never up. `_bindServer` waits for `initialized` or `serverError` before writing either map, with a timeout so a library that sends neither cannot hang the caller. `createServer` moves to the next port and `setPort` puts the server back where it was, saying so. `10-split-view` connects a socket before the window opens and asserts it survives. With the guard removed that test reports `Array [ "closed" ]`, and it takes two windows, so no unit test can see it.
FC15 sends every coil from the opened address to the end of the range, and the dialog seeded that list with Array(length).fill(false) without ever reading registerData. Writing one coil therefore switched off every coil above it that the user had not touched. seedCoils fills the list from the rows the grid holds, so what is on screen is what goes back out. FC5 is unaffected: main takes value[0], the coil at the opened address. The coil buttons now carry aria-pressed, which is what the e2e helper reads to decide whether a click is needed. Without it the helper clicked whenever the caller asked for TRUE, which now means it would send the opposite for a coil the device already had on. Verified with a mutation in each direction. seedCoils returning false for every row turns four of the five unit tests red, and indexing from zero instead of from the first address turns two red. Putting Array(length).fill(false) back in the component and rebuilding turns the new e2e test red on coil 6, with the 22 tests before it still green.
The mask sets `valid` and only the box read it, so an emptied field went out
as `Number('')`, which is 0. A lone minus, which is what the mask holds on
the way to a negative number, went out as NaN the same way. Both buttons are
now off while the field is not a value.
`handleClose` reset the field with `setValue('0')`, and `setValue` takes its
second argument as the verdict, so the reset marked a plain 0 as invalid.
That was invisible while nothing read the flag, and it would have left the
buttons off on the next address opened. The reset is now its own action.
Verified with three mutations against the new tests: dropping the guard turns
two red, replacing it with the neighbouring rule `value === ''` turns the
lone-minus test red, and a reset that marks the field invalid turns the store
test red.
The effect returned early when the register mapping named no type for the address, and this store outlives the dialog, so the address kept whatever the one before it used. A value then went out encoded as a type the address is not. writeDataTypeFor answers for both cases: the mapped type where there is one, int16 where there is not. It reads `none` as a mapping without a type rather than as a type, which the select does not offer either, and it takes the default from the same constant the store starts at. Verified with two mutations: putting the early return back turns the unmapped address test red, and letting `none` through turns its own test red.
`errorMessage` was declared outside `_read`'s group loop, so `_logTransaction` logged every group after a failed one with the earlier group's error. It is `_logTransaction` that reads it outside the catch: the placeholder rows and the snackbar are inside, and see their own group either way. Declaring it inside the loop is the whole fix. Two tests read two groups, one failing the first and one failing neither, and assert which error each group's transaction carries. Hoisting the declaration back out turns the first red and leaves the second green.
…ng one The disconnect timeout replaces `this._client` with a fresh `ModbusRTU`, and the constructor was the only place registering `error` and `close` on it. After one close that never called back, connection errors went unreported and auto-reconnect never fired again for the rest of the session. The two registrations move into `_attachClientHandlers`, called from the constructor and from the replacement site. `isDebugEnabled` is reset by the replacement too and needs nothing: `connect` sets it again. `modbusClient.test.ts` mocks the constructor to hand out one shared object, so the replacement is the same instance with its handlers still attached and the existing timeout test cannot see any of this. The two new tests empty the handler record first, so what it holds afterwards is what the replacement got: one asserts both handlers come back and that a close on the replacement still reconnects, the other that a close answering in time re-registers nothing.
`_logTransaction` assigned `{}` over modbus-serial's transaction table, which
takes the entries for requests still in flight with it. `_onReceive` returns
early when the response's entry is gone, so an overlapping request timed out
rather than resolving: the grid kept its old values and the view showed a
timeout that never happened. `write` ends with an un-awaited `this.read()`, and
a second write arriving while that read is outstanding is the way to meet it.
Deleting the key just logged is what the assignment was for: the same entry is
not logged twice on the next call.
Three tests: the logged entry is gone, a second read logs nothing again, and an
entry the read did not log is still there afterwards. Putting the assignment
back turns the third red and leaves the first two green.
`ConnectionConfigSchema.deepPartial()` accepts a payload whose field is
explicitly `undefined`, Zod keeps the key, Electron's structured clone carries
it over the IPC hop, and `deepmerge` copied it over the stored value. The
process then ran on a config its own schema refuses:
safeParse({ tcp: { host: undefined } }) -> success, keys of tcp: ["host"]
host after merge: undefined
whole config still valid: false
`withoutUndefined` drops those keys before the merge, in
`updateConnectionConfig` and in `updateRegisterConfig`, which has the same
shape. No call site was found that sends one today: the reviewer checked all
twenty `update*Config` call sites and this closes the shape rather than a
reproduced bug.
The helper is exported because neither config holds an array, so the branch that
hands one back whole is not reachable through the two update methods. Four tests
drive it directly and four drive the methods. Removing the call turns four red,
stripping only the top level turns three, recursing into arrays turns the array
one, and dropping the null guard turns the null one.
`get_connection_config` and `get_client_state` had no caller anywhere. Measured over all 38 channels in IPC_CHANNELS, each one's camelCase method grepped as `.<method>(` across src/renderer and e2e: population: 38 no caller: ["get_connection_config","get_client_state"] Every other occurrence of either name was a definition, a doc comment example or the sample string in a `snakeToCamel` test. Those three now name a live channel. `get_connection_config` held the app's only revalidation of the stored config, and it never ran. The shape it was guarding against is closed at the merge in the commit before this one, so nothing is lost with it. `get_client_state` validated a value main had just produced and typed. Thirty-six channels left. `yarn test` unchanged at 822.
`window.api` is generated from IPC_CHANNELS, so a channel nobody calls still
gets a method, a handler and a spec entry, and nothing says so. Two of them sat
that way long enough for the audit to find them, with the app's only config
repair branch inside one.
The rule reads IPC_CHANNELS, converts each name the way the preload does, and
looks for that name called as a property anywhere in src/renderer. The caller
has to be in the renderer: a channel only the e2e suite drives is one the app
does not use, and today there are none, so this costs nothing and makes that a
decision rather than a drift.
Both directions were run. Putting `get_client_state` back in IPC_CHANNELS turns
the rule red naming the channel and its method; renaming IPC_CHANNELS so the
reader finds nothing turns the population test red instead of passing empty.
CONTRIBUTING.md and CLAUDE.md now say eleven, which `grep -c '^describe('` over
the file agrees with.
`setRegisterValue` wrote straight into the entry, where `setBool` beside it branches on whether there is one. Against a store with no entry at that address: TypeError Cannot set properties of undefined (setting 'value') Reachable through `delayedSetRegister`, the only caller, which batches on a 50 ms timer. The `register_value` event proves the entry exists when it is enqueued, not when it flushes, and `removeRegister` and `resetRegisters` are both one click. The throw happens inside a `setTimeout` callback, so it surfaces as an uncaught renderer error rather than a message. The race itself is read, not reproduced. Dropped rather than recreated the way `setBool` does. A bool entry is a value; a register entry carries the params that say what it is, and a batched number is nothing to build one from. The address the user deleted stays deleted. Three tests: an entry that is there is written, one that is gone is dropped without a throw, and the entries beside it are still written. The original line turns two red, and recreating the entry instead turns the middle one red.
`strict` does not include it, so `record[key]` and `array[i]` were typed as `T`
and TypeScript said nothing about an index that is not there. That is why
`setRegisterValue` could write into an entry that had been deleted while
`setBool` beside it branched: the type said the entry was always there.
Enabled in all three tsconfigs. It reported 227 errors over 144 sites, which
these changes answer. Three of them were real:
_writeCoil gave value[0] to FC5 and the schema accepts an empty coil list,
so an empty one put undefined on the wire. It now sends nothing and says so.
readGroupEntry and readGroupByGid read the gid out of line.split(':'), and an
/etc/group line with fewer fields gave Number.parseInt(undefined), so a
malformed line answered NaN where the caller expects a gid. Skipped now.
deleteServer set selectedUuid to uuids[0]. The delete button is off for the
main server so the list never empties, but that guarantee lived in the
component rather than in the store, and now it is written down.
No non-null assertions were added, and an index that is provably in range got
no guard either: a branch no input reaches is a branch no test covers, no
mutation turns red, and a reader takes for a real case. The index goes away
instead. groupAddressInfos and the copy of it in the huawei spec became one
forward pass over an open block, which the 26 existing grouping tests hold to
its old answers; createRegisters returns [number, ...number[]], so the
guarantee that used to be a comment is the type FC6 reads registers[0] from.
In tests the reads became optional chaining, so a missing element fails the
assertion. Two places could not take it without hiding a failure and got a
helper that throws instead: fireClientEvent, because `?.()` on a handler that
was never registered is a test that quietly passes, and firstCallOrder.
Both new guards are reached by a test. Removing the coil guard turns its test
red; replacing the gid guard with a throw turns its test red, which is what
says the line runs.
CONTRIBUTING gains the rule under Code style.
yarn lint 0, yarn typecheck 0, yarn test 830, yarn test:e2e 572 passed.
Three things Jens asked for after the flag went on. `@typescript-eslint/no-non-null-assertion` is an error now. Turning it on reported 80 assertions, 69 of them in `modbusServer.test.ts`. All are gone. `lastVector` resolves the six handlers a server vector carries and fails naming the one that is missing, where `getHoldingRegister!(...)` said only that something was undefined; `lastInstance` does the same for the constructor's result. `bindResults.length > 0 ? bindResults.shift()! : true` is `bindResults.shift() ?? true`, which answers an empty queue the same way. Three e2e specs pulled a count out of a section title with the same regex and the same `expect(match).toBeTruthy()` in front of the assertion; they share `sectionCount` now, which fails with the text it did read. CONTRIBUTING and CLAUDE no longer count the conformance rules. The figure was edited on every rule that landed, which is how it goes quietly wrong. Same for "sixteen channels take no argument", in the same file. Names are spelled out. In `_read`: `gi`, `a`, `l` and `r` are `groupIndex`, `groupAddress`, `groupLength` and `row`, and the inner `address` that shadowed the outer one is `mappedAddress`. `d` and `p` further down are `row` and `port`. In `serialGroup`, `gid` is `groupId` and `readGroupByGid` is `readGroupById`. Node's own `Stats.gid` keeps its name, which three red tests said before I did. yarn lint 0, yarn typecheck 0, yarn test 830, yarn test:e2e 572 passed.
An adapter unplugged between requests reaches the port as `close` and as nothing else: `@serialport/stream` answers a failed read with `close(undefined, new DisconnectedError(...))` and pushes nothing into the stream. `startRtuServer` listened for `error` alone, so the view kept showing a server whose port was gone. The new listener sits beside the `error` one. It stays quiet for a close this process caused, which `stopRtuServer` marks by clearing `_rtuServer` and `_rtuActive` before closing the port, and for the write path, where one unplug emits both events and the `error` listener has already reported it. The `ServerSerial` mock had no `_serverPath`, so neither port listener was reachable from a test. It has one now, and `fireSerialPathEvent` fails naming the event where a missing handler would only have said something is not a function. Verified by mutation: removing the listener, each clause of its guard, and the `_rtuActive` it clears, each turns a different one of the five new tests red. Killing socat does not reproduce the trigger, because the pty outlives the process holding it, so the disconnect itself rests on the library's documentation of `close` rather than on a measurement here.
Removing the rule count from it left "three of them these" behind, which reads as nothing. The clause also claimed the four bullets above are all asserted, and `window.api` is generated is not one of the rules: the suite checks that a channel carrying an object has a schema and that a channel has a caller, not that the method appears by itself. Dropping the clause says what is true and cannot go stale.
"Four things that break if you do not know them" and "Two rules no test can see" both go stale the moment a bullet is added, and neither number tells the reader anything the list does not. The `audit` skill names the second heading, so it moves with it. The subpath count in the MUI rule goes the same way: the claim is that no subpath exports those two hooks, and how many there are does not carry it.
`PrivilegedPortModal` returned on `window.api.isServerWindow`, so in split view the check ran in neither window: `layout.zustand` puts the main window on the client view for as long as a server window exists, and the server window was the one told to stay quiet. A user who splits from Home never had it run at all, which is the path `10-split-view.spec.ts` drives. The modal is mounted in `containers/Server.tsx` and nowhere else, and only one window renders that container at a time, so the mount point already answers which window owns the check and the guard only cancelled it. The unit test that asserted the old behaviour now asserts the new one, and putting the guard back turns exactly that test red. The manual Linux spec gets the split path as a case; it needs a blocked port floor and someone at the keyboard, so it has not been run.
`DeleteButton` read the address out of the form field rather than out of the register the dialog was opened on. Changing the address and then pressing Remove left that register alone, removed whatever sat at the typed address, and closed the dialog as though it had worked. The `?? 'uint16'` fallback beside it then chose the span to erase for a register that was not there. Which of the two buttons applies follows from what the dialog holds, so they now say so. Submit Change is off until a field is touched, Remove goes off the moment one is, and a value typed back to what it was returns the dialog to where it opened. A user who came to edit submits; a user who came to delete deletes. Dirty is measured against what the edit effect wrote into the fields, not against the register, because that effect converts on the way in and a conversion that did not round-trip would leave the dialog dirty on open. Removing moves to `remove()` beside `submit()` in the store, for the reason `submit` is there: it reads the dialog and writes through the server store. It takes the address, type and length from the register being edited, so the lookup by form address and its fallback are gone. Seven mutations, each red on the tests that name it: dropping either button's `disabled`, dropping `capturePristine` from the edit effect, letting `remove` read the form address, dropping its edit-mode guard, keeping the old snapshot across a second open, and calling a missing snapshot dirty. yarn lint 0, yarn typecheck 0, yarn test 853 passed, 04-add-register-modal.spec.ts 20 passed.
The edit effect called the masked setters with one argument. Their second is the validity of the value, and the setters do `state.valid.x = !!valid`, so `undefined` landed as false and every field opened marked wrong. What that looks like depends on whether the field is on screen. A visible one paints its label as an error until the mask under it mounts and reports back, which is the red that fades on open. A hidden one keeps the false: a fixed register has no min, max or interval, and switching it to Generator showed those fields empty and wrong with Submit Change disabled until both were retyped. A generator has no value and does the same the other way. So the flag is passed, and a field the register has nothing for is seeded from `FIELD_DEFAULTS`, which the store's initial state and `resetToDefaults` now read as well. Six mutations, each red on the test that names it: dropping the flag on value, on min and max, or on interval, and dropping either seeding. Dropping the flag on the address survives, because `setDataType` at the end of the effect recomputes `valid.address` without it. It stays for the contract: omitting it says the address is invalid, and that it is rescued afterwards is an ordering accident. yarn lint 0, yarn typecheck 0, yarn test 857 passed, and the three specs that drive this dialog, 03-server-config, 04-add-register-modal and 05-file-io, 76 passed.
Two tests in the bitmap spec asserted `toBeVisible` on a bit's circle and on the register row's Edit button. A circle is visible whatever its bit is and the Edit button whatever the value is, so the per-bit rendering of `ServerBit` and `ServerBitMapDetail` looked covered and was not. A circle differs between on and off in `background` and `boxShadow` only, so it now carries `data-active`, and the register row's value carries `server-reg-value-<type>-<address>`. Both tests read those instead. The comment gets its testid in its read state too, which retires the `span.MuiTypography-root` selector that CONTRIBUTING forbids. `ServerBit` and `ServerBitMapDetail` get unit tests: which bits a value shows as on, what a toggle writes back in both directions, that a toggle drops the generator fields, and where an edited comment lands. Seven mutations, each red on the test that names it: the circle's state made constant, `readOnly` dropped from the toggle, the read-state testid removed, the set and clear branches swapped, the generator fields carried over, the bit order reversed, and an edited comment written under a fixed index. Two more in the e2e specs: a circle that stops carrying its state fails the bit states test, and a row value that never refreshes fails the toggle test. lint 0, typecheck 0, `yarn test` 870 passed, and `03-server-config`, `21-bitmap-settings` and `26-server-layout` together 65 passed.
`SerialPortOptionsSchema` permitted `mark` and `space`, and the RTU client
and the RTU server both offered them. `serialport_unix.cpp` has a case for
none, odd and even and a default that returns -1, and `binding.gyp` builds
that file on mac and on linux. Against a socat pty on macOS 23.6.0:
none opened
even opened
odd opened
mark ERROR: Invalid parity setting 2
space ERROR: Invalid parity setting 5
`ParitySchema` now holds the three, `SerialPortOptionsSchema` takes it, and
`ParitySelect` maps its options rather than a hand copy. The five strings were
written out in four places: the schema, that copy, the cast in
`setServerParity`, and `modbus-serial`'s own type, which is what `setParity`
was typed against and is why a cast could hand it anything. Both setters take
`Parity` now, and both casts are gone.
A stored config can carry either. `repairPersisted` works a top level field
at a time, so without a migration a stored `mark` costs the com port and the
baud rate beside it: with the v3 to v4 step removed, the server store comes
up on `com: ''`. Both stores bump a version and call `repairPersistedParity`,
which writes `none` only where the stored value is one the schema no longer
names.
`corruptedConfig` wrote `version: 2` into five fixtures, which is the current
version until it is not. They read the constant now, so the file keeps
testing repair rather than a migration.
Ten mutations, each red on the test that names it: `mark` back in the
schema, either migration step dropped, either version constant put back, the
repair made unconditional, its absent-parity guard removed, its walk cut
short, and each of the two guards in that walk. Two more in the e2e: the
select reading a hand list again fails on the option count, `5` against `3`,
and a parity setter that ignores its argument fails the server RTU spec.
lint 0, typecheck 0, `yarn test` 885 passed, `16-client-rtu` and
`23-server-rtu` together 62 passed.
`RegisterParamsBasePartSchema.address` was a bare number and
`RemoveRegisterParamsSchema.address` was `RegisterAddressSchema`, so the two
paths disagreed. Measured through the schemas:
address -1 add ACCEPTS remove refuses sync ACCEPTS
address 1.5 add ACCEPTS remove refuses sync ACCEPTS
address 70000 add ACCEPTS remove refuses sync ACCEPTS
address 65535 add ACCEPTS remove ACCEPTS sync ACCEPTS
That is also the schema a saved config file arrives on, through
`ServerRegistersPerUnitSchema`, and `syncRegistersWithBackend` hands the
parameters to `addRegister` unchanged. `addRegister` writes
`serverData[registerType][address + index]` into the array `resetRegisters`
builds as `new Array(65536)`, so a file naming 70000 stretches it:
`holding_registers` comes back 70001 long, and no Modbus request reaches the
part past the end.
`RegisterAddressSchema` now covers the field. The map is keyed by address as
well, and a boolean entry carries nothing else, so `ranges.ts` gains
`RegisterAddressKeySchema` and `ServerBoolSchema` and `ServerRegisterSchema`
take it. `WriteParametersSchema` and `SetBooleanParametersSchema` wrote the
same range out by hand and now import the name; that pair is a rename, so no
test names it.
The persisted store carries the same shape. `repairPersisted` works a top
level field at a time, so one register at 70000 fails `serverRegisters` and
returns it empty, which is every register on every server and unit. The store
bumps a version and calls `dropUnservableRegisters`, which deletes the entries
the schema no longer names and leaves their neighbours.
Twelve mutations, each red on the test that names it: the address back to a
bare number, either map key back to any digits, the key ceiling one short, the
migration step dropped, the version constant put back, either half of
`isServable` removed, an entry with no parameters dropped rather than kept, the
walk no longer filtering out a null, the entry shape clause removed, and the
entry emptied rather than deleted.
lint 0, typecheck 0, `yarn test` 900 passed, `03-server-config`,
`04-add-register-modal` and `05-file-io` together 76 passed.
`applyLegacyStringReplacements` ran four `replaceAll` calls over the raw file
text before `JSON.parse`, and `migrateServerConfig` and `migrateClientConfig`
both called it first. The four names read as ordinary English, so every
occurrence in a config name and in a comment went with them. Measured on a
valid v2 file, which reports `migrated: false`:
name -> "coils bank A" (was "Coils bank A")
coil comment -> "coils enable" (was "Coils enable")
reg comment -> "read input_registers here, discrete_inputs too"
That text is what the store persists and what the next save writes back, so a
load and a save lose it for good.
`RegisterType` was an enum of camelCase members until `b3474fe`, and those
members were the register map's keys, so a config saved before that is the only
one naming them the old way. Versioned configs arrived at `b4f558b`, eight
months later. `renameLegacyRegisterTypeKeys` walks the parsed object and
renames keys alone, and the two v1 to v2 migrations are its only callers. What
a user types is a value in a config: a name, a comment, a string register's
contents. The keys are field names, unit ids, addresses and bit indices.
name -> "Coils bank A"
coil comment -> "Coils enable"
reg comment -> "read InputRegisters here, DiscreteInputs too"
v1 map keys -> coils, discrete_inputs, input_registers, holding_registers
Ten mutations, each red on the test that names it: the rename put back over the
file text, in either config path; the call dropped from either v1 migration;
the walk no longer going down; the old key left beside the new one; a coil
renamed to the neighbouring type; the walk returning rather than continuing at
the first key it does not name; an array no longer walked; and the object check
dropped.
The test that covered the old rule asserted only that a config of four empty
maps did not throw, so it went with it.
lint 0, typecheck 0, `yarn test` 915 passed, `05-file-io` and
`14-client-config-io` together 33 passed.
`windows.main` was set nowhere but at creation. `windows.server` is nulled on close and on `window-all-closed`; the main window had no such handler, so the handle outlived the object it named. Every method on a destroyed `BrowserWindow` throws, and `second-instance` calls two of them. Only macos reaches it: `window-all-closed` quits everywhere else, so nowhere else does the app outlive its windows. Close the window, leave Modbux in the dock, launch it again from Finder or Spotlight, and the second process loses the single instance lock and hands over to the first, which calls `isMinimized()` on the dead handle. What the user gets is a modal error dialog reading "A JavaScript error occurred in the main process" and no window. A `closed` handler now nulls the handle, beside the server window's. `close` would be too early: the window is alive there and a listener may still cancel it. With the handle honest, `second-instance` can answer the case it was written for and open a window when there is none, which is what a second launch was asking for. `createWindow` builds into a local and assigns once, so the guard inside `ready-to-show` that read `windows.main === null` goes. It was written for a null the code could not produce. `_sendUpdate` was `send` with the per window guard removed, both carrying the same comment about macos. One throw ended its loop, and `Object.values` puts `main` first, so a stale main handle cost the server window its `window_update`. It calls `send` now: one path, one guard, one comment. The suite could not see either of these. `10-split-view` closes the server window and leaves the main one standing, and nothing anywhere closes the last window. `02-macos-dock` does, with a device counting the polls it is sent and a master holding a connection to the server, so what keeps running with no window open is measured rather than assumed: the client polls on, the server keeps its master, and the window comes back. Mutations, each red on the test that names it. Dropping the null-out and dropping the `createWindow` branch both fail `launching modbux again brings the window back`; the first also puts the error dialog back on screen. Restoring `_sendUpdate`'s own loop fails `a destroyed main window does not cost the server window its update`. lint 0, typecheck 0, `yarn test` 43 files and 918 passed, `10-split-view`, `01-persistence` and `02-macos-dock` together 27 passed.
`ConnectionConfigSchema.unitId` and `TcpPortOptionsSchema.port` were bare
numbers while `ranges.ts` exported `UnitIdSchema` and `PortSchema`. The server
states the same range on everything that arrives: all six entries of its service
vector open with `UnitIdStringSchema.safeParse`, and it is the same byte going
the other way.
What the bare schema accepted, and what the byte did with it:
999 accepted writeUInt8 ERR_OUT_OF_RANGE
-5 accepted writeUInt8 ERR_OUT_OF_RANGE
Infinity accepted writeUInt8 ERR_OUT_OF_RANGE
3.7 accepted writeUInt8 wrote 3
The promise API wraps each call in a `new Promise` executor, so the three
RangeErrors come back as a rejection and the read path reports them. `3.7` is
the one nothing reports: the client polls unit 3 and says so nowhere.
No shipped path produces either value, so there is no CHANGELOG entry.
`UnitIdInput` masks 0 to 255 and the client's port takes `UIntInput`, whose
default ceiling is 65535. The saved client config file carries neither field:
`SaveButton` puts the unit id in the filename and nothing else. What is left for
the schema to refuse is the persisted store, where `repairPersisted` resets the
connection settings and names them, and `update_connection_config`, which
answers "Invalid request, nothing was changed".
Four mutations, each red on the test that names it: either field back to a bare
number, the unit id given `RegisterAddressSchema`, which leaves 256 and 999 as
the values that discriminate, and the port given `UnitIdSchema`, which turns the
accepting tests red instead of the refusing ones.
lint 0, typecheck 0, `yarn test` 934 passed, and `yarn test:e2e` 581 passed.
Connect a client over TCP, close the window so the app sits in the dock, open it again: the button reads Connect while main is connected and polling, and pressing it answers `Already connected`. Main pushes `client_state` on a change, so a window that opens after the last push has nothing to catch up on and starts on the store's initial disconnected literal. `init` asks now. `get_client_state` is the channel `51bd5a6` dropped for having no caller. It comes back with its caller in the same commit, which is what the eleventh conformance rule asks for. The handler hands back `client.state` and validates nothing, because that is a value main produced and typed. A push that lands while the answer is in flight is the newer of the two and keeps its value. The ask sits behind a `try` because `init` runs from module scope with nothing awaiting it, so a rejection there is an unhandled one, and falling through leaves the literal the window already showed. `02-macos-dock` builds the scenario and asserted only that the device kept being read. It looks at the button now, and reads Connect without the fix. The store tests join the four the conventions round put in `context/__tests__/`: the answer landing, a push beating it, a rejection changing nothing, and the configs `init` pushes before it asks. yarn test 939 passed. yarn test:e2e 582 passed.
Three specs split the server out of Home the same way: click, then
`waitForEvent('window')`. The listener goes up when `waitForEvent` is called, so
the window can open inside the gap the awaited click leaves, and the event is
gone before anything is listening. `98-privileged-port` timed out there, at `the
split-out server window asks`.
Outside the spec, with the listener armed after the click: 2 of 3 runs timed
out, and the Server window was open in all three. Armed beside the click: 3 of
3.
The three call sites are one helper now, `splitOutServerWindow`, which also
waits for the load state each of them waited for separately.
lint 0, typecheck 0, yarn test 44 files and 939 passed. 98-privileged-port 8
passed, 10-split-view 7 passed, 03-presentation 43 passed with no screenshot
changed.
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
1 participant
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
Two rounds on one branch. The first wrote down the conventions this codebase had
already settled and made a test assert each one, so breaking a rule fails
yarn testinstead of waiting for a reviewer. The second audited the app byarea, and this branch carries a fix for everything that came back blocking.
The audit reported 28 blocking findings, and grouping them by cause left 21.
All 21 are built here, one commit each, every one 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 of the 70 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. By subject:The server answers only for what it hosts. On a shared RS-485 line it used
to reply for every address on the bus, including the real devices on it. It now
says nothing for an address it does not host, and a write to a unit you never
configured no longer creates one. Unit 0 is the broadcast address on RTU and
behaves like one.
Writing from the client does what the dialog shows. Writing one coil no
longer switches off the ones beside it, an empty value field no longer goes out
as a zero, and the dialog no longer keeps the data type of the address you
opened before.
A configuration comes back the way you left it. Loading one no longer
rewrites your own words in names and comments, a register at an address past
65535 is refused and named instead of loaded, a config that does not parse now
loses only the field that failed, and the parity list offers only what the
serial binding accepts on every platform.
Connections survive the things that used to drop them. Opening the server in
its own window no longer restarts every running server, a server that fails to
bind says so instead of looking up, unplugging the serial adapter stops the RTU
server in the view, a hung disconnect no longer costs you auto-reconnect, and on
macOS opening Modbux again while it sits in the Dock brings the window back
rather than a main-process error dialog.
What is asserted now
src/__tests__/conformance.test.tsasserts the conventions inCONTRIBUTING.mdunder Code style: one store selector per field, an actionfetched where it runs, every component wrapped in
meme, a local store namedafter its component, MUI imported deep, nothing in
src/sharedreaching intosrc/main, every interactive element carrying adata-testid, every channelthat carries an object declaring a schema, every channel having a caller in the
renderer, every configured path alias imported through, and every configured
include pointing at something. Each rule asserts its population is not empty
before it asserts the population holds no violation, because a meter that reads
no files passes every rule it has.
noUncheckedIndexedAccessis on, and!is a lint error. Where an index wasprovably in range the index went away rather than gaining an assertion.
What this does not do
The audit also reported 74 annoying and 44 cosmetic findings. None of them are
triaged and none are in here. Three annoying ones were picked up in passing
because they sat in code a blocking fix was already touching.
The audit documents live in
tmp/, which is gitignored, so they are on mymachine rather than in the repository.
How to test
yarn verify. Every one of the 21 causes carries a test in its own commit, ande2e/specs/02-standalone/02-macos-dock.spec.tsis new: it holds a Modbus masteron Modbux's own server and counts the reads a device is sent while every window
is closed, which is how the macOS Dock behaviour is measured rather than assumed.
Locally on macOS: lint 0, typecheck 0,
yarn test934 passed,yarn test:e2e581 passed.
CI on this pull request runs lint, typecheck and the unit tests, on Linux.
e2e.ymlisworkflow_dispatchonly, so the Playwright suites on Windows andmacOS need a run started by hand before this merges.