The second audit's non-blocking half, and assignments A to E

.claude/skills/prose/SKILL.md

@@ -107,6 +107,12 @@ way to tell which is the false claim is to go and run the thing.
 **In a test file, the sentence naming what the test discriminates stays and the
 incident that produced it goes.**
 
+**What the code was is not what it does.** A prop that is gone, a ref that no
+longer exists, a test that was deleted, an alternative nobody is about to retry:
+that is the file's diary, and git holds it. The commit message is where it goes.
+What survives in the comment is the mechanism as it stands, and a past state
+earns a line only where a reader would otherwise undo the change.
+
 ## Then: can this be written with fewer sentences?
 
 Ask of each sentence:

.claude/skills/test/SKILL.md

@@ -76,7 +76,10 @@ git status --porcelain <file>             # empty, or the restore did not take
 - **Edit by line number or by a unique string, not by the first match.** `sed`,
   `replace(old, new, 1)` and a first-hit search all take the first one, and a
   codebase repeats lines. If a mutation reports _no tests_ rather than a failure,
-  it broke the file: that is your quoting, not the code.
+  it broke the file: that is your quoting, not the code. **A mutation that never
+  applied reports every test green**, which reads exactly like a mutant that
+  survived, and the wrong answer to that is to write a second test. Diff the file
+  against the copy before believing a green run.
 - **Assert what the code did, not what it said.** The value in the store, the
   cell in the grid, the bytes on the wire. Not that a handler was called.
 - **Assert what must appear, not what must be absent.** A thing never produced

.github/workflows/e2e.yml

@@ -146,14 +146,28 @@ jobs:
       - name: Build
         run: yarn electron-vite build
 
-      # --trace retain-on-failure, because the repository config sets
-      # on-first-retry against retries: 0 and so records nothing. A failure here
+      # E2E_TRACE=1 makes the fixture trace the app's own context and keep the
+      # trace of a failed test; --trace retain-on-failure records only the
+      # contexts Playwright creates, which holds the assertions. A failure here
       # cannot be reproduced on the machine you are reading this from.
       - name: E2E against out/
         if: inputs.mode != 'packaged'
+        env:
+          E2E_TRACE: '1'
         shell: bash
         run: ${{ runner.os == 'Linux' && 'xvfb-run -a' || '' }} yarn playwright test --trace retain-on-failure
 
+      # The dist/ run empties test-results/ before it starts, so a failure here
+      # is uploaded now or not at all.
+      - uses: actions/upload-artifact@v7
+        if: failure()
+        with:
+          name: playwright-out-${{ matrix.runner }}
+          path: |
+            test-results/
+            playwright-report/
+          if-no-files-found: ignore
+
       # not cancelled(), rather than success(): a failure in the dev run says
       # nothing about the packaged app, and the packaged app is what ships. The
       # job still ends red, and now it ends red with both answers in it.
@@ -166,6 +180,8 @@ jobs:
 
       - name: E2E against dist/
         if: "!cancelled() && inputs.mode != 'dev'"
+        env:
+          E2E_TRACE: '1'
         shell: bash
         run: ${{ runner.os == 'Linux' && 'xvfb-run -a' || '' }} yarn playwright test --config playwright.packaged.config.ts --trace retain-on-failure
 

CONTRIBUTING.md

@@ -64,12 +64,24 @@ population holds no violation, because a meter that reads no files passes every
 rule it has.
 
 **One store selector per field.** `useClientZustand((z) => z.a)` and then
-`((z) => z.b)`, never one selector returning an object. An object literal is a
-new reference on every render, so a selector that returns one re-renders its
-component on every flush of any field. The same goes for a call with no selector
-and for `(z) => z`, which take the whole store the long way round. The renderer
-has zero of all three and zero `useShallow`, and that is why it draws a
-two-thousand-row grid without either.
+`((z) => z.b)`, never one selector answering something it built. zustand runs
+the selector under `useSyncExternalStore` and compares its answer with
+`Object.is`, so a fresh reference is a new snapshot every time React reads it,
+and React reads it again on the render that follows: the component does not
+render more often, it throws `Maximum update depth exceeded`. Measured at
+zustand 5.0.11 on an object literal and on `Object.keys(z.servers)`, which is
+the spelling that cost the whole server view on 22 Sep 2026. Anything built in
+the selector counts, an array as much as an object, and so do a call with no
+selector and `(z) => z`, which take the whole store the long way round. Where a
+component wants a list of keys, select a value that compares equal, such as the
+joined string, and split it in a `useMemo`. The renderer has zero of all of
+these and zero `useShallow`, and that is why it draws a two-thousand-row grid
+without either.
+
+`conformance.test.ts` reads the object literal, the bare call and `(z) => z`.
+It cannot read a call that builds one, because whether `f(x)` answers a fresh
+reference is not in the AST, so a selector calling anything is a reviewer's
+question.
 
 **An action is fetched where it runs, not subscribed to.** A selector that hands
 back a store function puts that function in the dependency list, and a
@@ -92,9 +104,16 @@ with nothing to read.
 
 **Every component is wrapped in `meme`.** Props or not, one rule with no
 exception to remember. A declaration counts as a component when it is rendered
-as JSX somewhere or exported as its file's default. React's bare `memo` does not
-satisfy it: `meme` is `memo` with `deepEqual`, and the shallow comparator is what
-a mutated row defeats.
+as JSX somewhere, exported as its file's default, or handed to MUI as a slot.
+React's bare `memo` does not satisfy it: `meme` is `memo` with `deepEqual`, and
+the shallow comparator is what a mutated row defeats.
+
+A component handed over as a prop carries no JSX tag and need not be a default
+export, so the first two alone would leave it unchecked. Three spellings count:
+`slots={{ footer: X }}`, `inputComponent: X` nested inside `slotProps`, and
+`inputComponent={X}` straight on the element. The shorthand of the first names
+its local for the slot, which is lowercase, and the rule takes capitalised
+declarations only.
 
 **A local store is named after its component.** `<name>.zustand.ts`, matching
 the global stores in `context/`, and named after the component rather than the
@@ -122,13 +141,23 @@ counts as carrying it.
 
 **The three lists that name a channel agree, and so do the two that name an
 event.** `IPC_CHANNELS`, `IpcHandlerSpec` and the `ipcHandle` calls in
-`main/ipc.ts`; `IPC_EVENTS` and `IpcEventPayloadMap`. Typecheck catches one
-direction only: a channel in `IPC_CHANNELS` and not in the spec fails to index,
+`main/ipc.ts`; `EVENTS_TO_RENDERER` and `EVENTS_TO_MAIN` together against
+`IpcEventPayloadMap`. Typecheck catches one direction only: a channel in
+`IPC_CHANNELS` and not in the spec fails to index,
 while a channel in the spec and not in `IPC_CHANNELS` is dropped by the mapped
 type in silence, and one in both with no handler rejects at runtime instead. A
 channel and an event name is lowercase segments, because that is where
 `snakeToCamel` and `CamelCase` cannot disagree: `set_2wire` would be the method
-`set_2wire` and the type `set2wire`.
+`set_2wire` and the type `set2wire`. Over those segments `snakeToCamel` is
+injective, and the method names are counted against each other and against
+`isServerWindow`, which is the one key on `window.api` that is not a channel's.
+
+**An event is in the list for the direction it travels, and both ends exist.**
+`EVENTS_TO_RENDERER` is what `windows.send` pushes and `onEvent` hears;
+`EVENTS_TO_MAIN` is what `sendEvent` pushes and `onIpcEvent` hears. One list for
+both directions typechecked a send nothing listens for, and left "every event
+has a sender and a listener" nothing to measure. A `send` counts only on
+`windows`, because `send` is a name the platform uses too.
 
 **Every channel that carries an object declares a schema.** TypeScript covers a
 bare primitive, and a channel taking no argument has nothing to guard. The rest
@@ -265,7 +294,7 @@ generic driver claims the device. Every `test:all:*` ends with this round, and
 - **New features need tests.** Unit tests for logic, e2e tests for UI behavior.
 - **Bug fixes need a regression test.** Prove it was broken, prove it's fixed.
 - **Tests must be deterministic.** No flaky tests. No "works on my machine". Use `waitForTimeout()` for UI settling and animations, but use `toPass()` when asserting on data that needs time to arrive.
-- **e2e tests run serially** with `maxFailures: 1`. One failure stops the entire suite. This is intentional.
+- **e2e tests run serially**, and every spec file starts with a `beforeAll` that calls `resetApp`, which the fixture refuses to run a test without. A failure skips the rest of a `describe.serial` and touches no other file, so one run reports every failing file.
 
 ### Writing e2e tests
 

e2e/fixtures/arduino-port.ts

@@ -17,9 +17,9 @@ export type PortChoice = { port: string; reason?: undefined } | { port?: undefin
 /**
  * The Arduino's serial port, or why there isn't one.
  *
- * Replaces the page.pause() these specs used to open, which needed a person to
- * pick the port by hand and so kept the hardware round out of the unattended
- * suites. The vendor ID is what identifies the board: `manufacturer` reads
+ * The board is found here rather than through a `page.pause()` that waits for
+ * a person to pick the port, which is what keeps the hardware round in the
+ * unattended suites. The vendor ID is what identifies it: `manufacturer` reads
  * "Microsoft" on Windows, where the generic usbser driver claims the device.
  *
  * Returns a reason rather than throwing, so the caller decides between skipping

e2e/fixtures/electron-app.ts

@@ -1,58 +1,108 @@
-import {
-  test as base,
-  _electron as electron,
-  type ElectronApplication,
-  type Page
-} from '@playwright/test'
-import { launchOptions } from './launch'
-import { createWriteStream, mkdirSync } from 'fs'
-import { join } from 'path'
+import { test as base, expect, type ElectronApplication, type Page } from '@playwright/test'
+import { launchElectron, evaluateMain } from './launch'
+import { cleanServerState, navigateToHome } from './helpers'
+
+export type ElectronFixtures = {
+  electronApp: ElectronApplication
+  mainPage: Page
+}
+
+/** The spec files whose `beforeAll` ran `resetApp` in this worker. */
+const resetFiles = new Set<string>()
+
+const NO_MOTION =
+  '*, *::before, *::after { animation-duration: 0s !important; transition-duration: 0s !important; }'
 
 /**
- * Everything the app writes, kept next to the traces.
+ * Brings the worker's app back to what a fresh launch shows, for the spec
+ * file whose `beforeAll` calls it.
  *
- * Playwright owns the child process and nothing reads its stdio, so a run that
- * ends with "Target page, context or browser has been closed" says only that
- * the app is gone. The exit line below is the point of this: a code means the
- * app came down on its own, a signal means something else took it.
+ * The app is shared by every file a worker runs, so each file inherited what
+ * the one before it left: 27 left the client on RTU, and 12, 15 and 18 passed
+ * in the suite and failed on their own. Every file under this fixture calls it
+ * first, and a test in a file that did not is refused.
  *
- * The workflow uploads test-results/ when a run fails, so the file travels with
- * the trace it belongs to.
+ * Main holds what the renderer's storage does not: the split out server
+ * window, the client's connection, and every server with its listener. Those
+ * go first, through the app, and then the storage is cleared and the window
+ * reloaded, which hands main the default client configuration through the
+ * store's `init`.
  */
-export function keepOutput(app: ElectronApplication): void {
-  const dir = join(process.cwd(), 'test-results')
-  mkdirSync(dir, { recursive: true })
-
-  const worker = process.env.TEST_WORKER_INDEX ?? '0'
-  const log = createWriteStream(join(dir, `electron-main-${worker}.log`), { flags: 'a' })
-  const stamp = (): string => new Date().toISOString()
-
-  // The persistence spec launches its own app beside the worker's, and both
-  // write here, so every line says which process it came from.
-  const proc = app.process()
-  const pid = proc.pid
-  proc.stdout?.on('data', (c: Buffer) => log.write(`[${stamp()}] ${pid} out ${c.toString()}`))
-  proc.stderr?.on('data', (c: Buffer) => log.write(`[${stamp()}] ${pid} err ${c.toString()}`))
-  proc.on('exit', (code, signal) => {
-    log.write(`[${stamp()}] ${pid} exit code=${code} signal=${signal}\n`)
-  })
+export async function resetApp(app: ElectronApplication, page: Page): Promise<void> {
+  await evaluateMain(() =>
+    app.evaluate(({ BrowserWindow }) => {
+      BrowserWindow.getAllWindows()
+        .filter((w) => w.getTitle() !== 'Modbux')
+        .forEach((w) => w.close())
+    })
+  )
+  await page.evaluate(() => window.api.disconnect())
+
+  await cleanServerState(page)
+  await page.getByTestId('server-mode-tcp-btn').click()
+
+  await page.evaluate(() => localStorage.clear())
+  await page.reload()
+  await page.waitForLoadState('domcontentloaded')
+  await page.addStyleTag({ content: NO_MOTION })
+  await navigateToHome(page)
+  await expect(page.getByTestId('home-client-btn')).toBeVisible()
+
+  resetFiles.add(base.info().file)
 }
 
-export type ElectronFixtures = {
-  electronApp: ElectronApplication
-  mainPage: Page
-}
+/**
+ * A screenshot of every open window for every test that fails, and a trace of
+ * the app beside it when `E2E_TRACE=1`.
+ *
+ * Playwright's own `trace` option records the contexts it creates itself, and
+ * the app's is not one of them, so a failed test's trace held its assertions
+ * and no DOM. Tracing runs on the app's context for the whole worker, one
+ * chunk per test, and a chunk is written only when its test failed. An app
+ * that is already gone has no chunk to stop, and its log says why.
+ *
+ * Recording cost 06, 11 and 21 together 1.2 and 1.3 min against 1.0 min
+ * without, so the workflow turns it on and a local run leaves it off: a spec
+ * that fails locally is run again on its own with `E2E_TRACE=1`.
+ */
+const tracing = process.env.E2E_TRACE === '1'
+
+type ElectronTestFixtures = { electronTrace: void }
 
-// eslint-disable-next-line @typescript-eslint/ban-types
-export const test = base.extend<{}, ElectronFixtures>({
+export const test = base.extend<ElectronTestFixtures, ElectronFixtures>({
+  electronTrace: [
+    async ({ electronApp }, use, testInfo): Promise<void> => {
+      if (!resetFiles.has(testInfo.file)) {
+        throw new Error(`${testInfo.file} has no beforeAll that calls resetApp`)
+      }
+      const context = tracing ? electronApp.context().tracing : undefined
+      await context?.startChunk({ title: testInfo.title })
+      await use()
+      if (testInfo.status === testInfo.expectedStatus) {
+        await context?.stopChunk().catch(() => undefined)
+        return
+      }
+      for (const [i, window] of electronApp.windows().entries()) {
+        await window
+          .screenshot({ path: testInfo.outputPath(`window-${i}.png`) })
+          .catch(() => undefined)
+      }
+      await context
+        ?.stopChunk({ path: testInfo.outputPath('electron-trace.zip') })
+        .catch(() => undefined)
+    },
+    { auto: true }
+  ],
   electronApp: [
     // eslint-disable-next-line no-empty-pattern
     async ({}, use): Promise<void> => {
-      const app = await electron.launch(launchOptions())
-      keepOutput(app)
+      const app = await launchElectron()
+      if (tracing) await app.context().tracing.start({ snapshots: true })
 
-      await app.evaluate((ctx) =>
-        ctx.session.defaultSession.clearStorageData({ storages: ['localstorage'] })
+      await evaluateMain(() =>
+        app.evaluate((ctx) =>
+          ctx.session.defaultSession.clearStorageData({ storages: ['localstorage'] })
+        )
       )
 
       await use(app)
@@ -70,8 +120,10 @@ export const test = base.extend<{}, ElectronFixtures>({
         searchCount++
 
         // Check BrowserWindow title (HTML <title> was removed to not override it)
-        const found = await electronApp.evaluate(({ BrowserWindow }) =>
-          BrowserWindow.getAllWindows().some((w) => w.getTitle() === 'Modbux')
+        const found = await evaluateMain(() =>
+          electronApp.evaluate(({ BrowserWindow }) =>
+            BrowserWindow.getAllWindows().some((w) => w.getTitle() === 'Modbux')
+          )
         )
 
         if (found && electronApp.windows().length === 1) {
@@ -88,10 +140,7 @@ export const test = base.extend<{}, ElectronFixtures>({
       await page.waitForTimeout(500)
 
       // Disable CSS animations/transitions globally for faster test execution
-      await page.addStyleTag({
-        content:
-          '*, *::before, *::after { animation-duration: 0s !important; transition-duration: 0s !important; }'
-      })
+      await page.addStyleTag({ content: NO_MOTION })
 
       await use(page)
     },

e2e/fixtures/helpers.ts

@@ -34,6 +34,28 @@ export async function selectRegisterType(p: Page, name: string): Promise<void> {
   await p.waitForTimeout(200)
 }
 
+/**
+ * Clear the client's register configuration.
+ *
+ * `confirms` says whether the dialog is expected, which the caller knows and a
+ * sample after a fixed wait only guesses: the button asks whenever the mapping
+ * or the name holds anything, and clears at once when neither does. Both
+ * answers are asserted, so a configuration that is not in the state the caller
+ * thinks it is fails here rather than several lines later.
+ */
+export async function clearClientConfig(p: Page, confirms = true): Promise<void> {
+  await p.getByTestId('clear-config-btn').click()
+  const confirm = p.getByTestId('clear-config-confirm-btn')
+
+  if (!confirms) {
+    await expect(confirm).toHaveCount(0)
+    return
+  }
+
+  await confirm.click()
+  await expect(confirm).toHaveCount(0)
+}
+
 /** Locate a specific cell in the MUI DataGrid (client side) */
 export function cellLocator(p: Page, rowId: number, field: string): Locator {
   return p.locator(`.MuiDataGrid-row[data-id="${rowId}"] [data-field="${field}"]`)
@@ -840,3 +862,21 @@ export async function splitOutServerWindow(app: ElectronApplication, p: Page): P
   await serverPage.waitForLoadState('domcontentloaded')
   return serverPage
 }
+
+/**
+ * Close the add register dialog with Escape, pressed inside it.
+ *
+ * MUI 9.3.1 puts the Escape handler on the modal root: `getRootProps` in
+ * `Modal/useModal.js` hands `onKeyDown` to that element, so the key closes the
+ * dialog only while focus is inside it. Add & Next disables the button under
+ * the pointer while the submit is in flight, which blurs it to `body`, and
+ * `FocusTrap` puts focus back from an interval of 50 ms. Measured with a probe:
+ * `document.activeElement` is `BODY` immediately after the click and a DIV
+ * inside the modal 300 ms later. `keyboard.press` between those two lands on
+ * `body` and the dialog stays open. Pressing on the address field focuses it
+ * first, which is what the assertion below then holds.
+ */
+export async function closeAddRegisterModal(p: Page): Promise<void> {
+  await p.getByTestId('add-reg-address-input').locator('input').press('Escape')
+  await expect(p.getByTestId('add-reg-address-input')).not.toBeVisible()
+}