CONTRIBUTING.md

Contributing to Modbux

Everything below is a rule rather than a suggestion. The ones under The conventions this codebase has already settled are asserted by src/__tests__/conformance.test.ts, so breaking one fails yarn test instead of waiting for a reviewer.

Ground rules

1. Open an issue first. Describe the bug or the feature before writing code, so a change that does not fit the project's direction is found before you build it.
2. One PR, one concern. Don't mix a bug fix with a refactor, and don't sneak in "while I was here" changes.
3. Don't break the build. Run yarn verify before pushing. If it doesn't pass, your PR won't be reviewed.
4. Match the existing style. Don't introduce new patterns, conventions, or abstractions without discussing them first.

Getting started

git clone https://github.com/ploxc/modbux.git
cd modbux
yarn
yarn dev

Prerequisites: Node.js (LTS) and Yarn.

Project structure

src/
  main/            Electron main process (server, IPC, state)
  renderer/src/    React UI (components, hooks, containers, theme)
  preload/         Electron preload script
  shared/          Types, utilities, migrations (used by both processes)
e2e/
  specs/01-main/        Core feature tests
  specs/02-standalone/  Persistence & restart tests
  specs/03-presentation/ Screenshot & demo generation
  specs/99-hardware/    Hardware integration tests (Arduino)
  fixtures/             Test data and helpers

Path aliases: @renderer/* and @shared. There are no others. Use them instead of deep relative imports.

Code style

Formatting and linting are enforced by ESLint, Prettier, and TypeScript strict mode. Run yarn lint to auto-fix issues. Don't fight the tooling, don't disable rules inline, and don't modify .eslintrc, .prettierrc, .editorconfig, or tsconfig files without prior discussion.

Beyond what the linter catches:

• No any, no @ts-ignore. If the types are fighting you, your approach is wrong.
• No ! either, and no guard a test cannot reach. noUncheckedIndexedAccess is on, so record[key] and array[i] are T | undefined and every index asks a question. @typescript-eslint/no-non-null-assertion is an error, because an assertion answers that question without leaving the reasoning behind. The other wrong answer passes lint: if (!x) break on an index that is provably in range is a branch no input reaches, no test covers and no mutation can turn red, and it tells the reader the case is possible. Take the index away instead: readUInt16BE, for..of, .entries(), slice. Where something really can be missing, handle it and write the test that reaches it. In a test, data[0]?.id lets a missing element fail the assertion, but handler?.() makes it pass quietly, so that one gets a helper that throws by name.
• Zod for validation. External data (configs, IPC payloads) is validated with Zod schemas. Don't trust unvalidated input.
• Zustand + Mutative for state. Follow the existing store patterns. Don't introduce new state management approaches.
• MUI only. Don't add other UI libraries.
• Spell out variable names. resetButton, not rstBtn. registerAddress, not regAddr. Abbreviations make code harder to read. The only exceptions are well-known conventions like i in loops, el in DOM callbacks, z for Zod schemas and Zustand state accessors, and established project abbreviations like e2e.
• Match existing patterns. Look at how the codebase does it, do it the same way.

The conventions this codebase has already settled

src/__tests__/conformance.test.ts asserts every rule below, so a PR that breaks one fails yarn test rather than waiting for a reviewer to notice. Every rule asserts that the population it reads is not empty before it asserts the 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 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 dependency list naming something the component does not own is a list no reader can check. What the component holds goes in the list; what the store holds is read through getState(). That also covers a value the component wants at a moment rather than on every change: read that way, it causes no render.

Two shapes, and which one you write depends on whether the component adds anything. A handler that does its own work is a useCallback whose first line reads the store, const clientZustand = useClientZustand.getState(). A prop that only forwards takes the action itself, const setHost = useClientZustand.getState().setHost, because wrapping it in a useCallback that calls it with the same arguments only gives it a second name.

Either way the thing has a name and the prop takes the name, so a getState() written into a JSX attribute breaks the rule from the other side: the call sits where the reader is looking at layout, and a handler with no name is a handler 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, 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 folder it sits in.

MUI is imported deep. @mui/material/Button, not @mui/material. The same for @mui/icons-material, @mui/x-data-grid and @mui/x-date-pickers, because the rule is about barrels and those are barrels. Two exceptions are the package's doing rather than a choice: useGridApiContext and useGridApiRef are exported by none of the subpaths @mui/x-data-grid declares, so they come from the root.

Nothing in src/shared imports from src/main, or from electron. All three processes import shared; it is the one layer that may not reach back. Electron is the same rule read through the other specifier: Windows held two BrowserWindows from inside shared and passed, because it named electron rather than @main.

Every interactive element carries a data-testid. Buttons, fields, sliders, selects and grid action cells. Containers do not, because the e2e suite reaches what is inside them instead: a ToggleButtonGroup through its ToggleButtons, and a Select's options through getByRole('option'). The Select itself carries one. A picker takes the attribute through slotProps, which still 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; 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. 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 take an object or a union, and that is where a hand-edited config file arrives. The schema goes beside the handler in main/ipc.ts, and it is only accepted where undefined is an honest answer: a rejected payload has nothing else to give back, so a channel returning a value has to say so in its type.

Every channel has a caller in the renderer. 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 sat that way with the app's only config repair branch inside one of them, which is worse than no repair at all: it reads like a guard. The caller has to be in src/renderer, because a channel only the e2e suite drives is one the app does not use, and that is a decision to take rather than to let happen.

Every channel that reaches main's client is placed. main/index.ts constructs one ModbusClient and no client channel carries an addressee, so any window can aim one at it, and both windows load the same renderer bundle. CLIENT_CHANNELS in main/ipc.ts is the list createIpcHandle refuses from a window that is not windows.main, and it is written by hand. A channel added later is a name, a spec entry and a handler, and every other rule here stays green while it escapes that list. The signal the test reads is the handler's own body: a listener naming client or state reaches what one window owns. Everything else drives a server, which is addressed by uuid, or asks app or a Linux helper. Three reach the client and are deliberately not refused, and the test names them with the reason: list_serial_ports and validate_serial_port enumerate hardware and the RTU server's COM field asks for the first of the two from the server window, and get_client_state answers a value rather than undefined, so a refusal would have nothing to hand back.

Every configured path alias is imported through. @renderer/* and @shared are the two, in the tsconfigs and in electron.vite.config.ts alike. An alias nobody imports through resolves whatever it points at, including a directory that is gone, so the last import leaving is what retires it.

Every configured include points at something. A glob that matches nothing costs nothing to keep and says nothing when it stops being true, so the test expands it rather than reading its shape.

The rules no test can see

The store owns IPC that changes state; a component owns IPC the user asked for. Writing through another store is a mutation, and the store owns those. A button press is the component's.

The same channel can be called from both and be right both times, which is why this is a reviewer's judgement and not an assertion: read is a consequence of flipping endianness in the store, and a button in the toolbar. Same channel, two concerns.

A component that owns something gets a folder. Its store, its helpers, its subcomponents and their tests go in with it, and the folder takes its name. A __tests__ of its own is enough, because the alternative is its test sitting in the parent's folder among the tests of everything beside it. A component that owns nothing stays a file: SliderComponent.tsx and HomeButton.tsx are leaves, columns/ and shared/inputs/ are collections of them, and neither wants a folder each. Where the line falls is a judgement, so no test draws it.

Commits

Follow Conventional Commits. Lowercase, no period at the end.

feat: add RTU support for serial connections
fix: prevent grid clear on address base switch
test: add bitmap schema tests and e2e spec
refactor: move readConfiguration to standalone state
docs: update changelog for v2.0.0
chore: bump version to 2.0.0

Rules:

• feat = entirely new functionality
• fix = something was broken, now it's not
• refactor = same behavior, different code
• test = test-only changes
• docs = documentation-only changes
• chore = tooling, deps, version bumps

Don't use feat for a bug fix. Don't use fix for a refactor. Mean what you say.

Testing

What to run

Command	What it does
yarn test	Unit tests (Vitest)
yarn test:watch	Unit tests in watch mode
yarn test:e2e	Build + the e2e suite (Playwright)
yarn test:e2e:packaged	Same specs against the packaged app. Run before releasing.
yarn test:e2e:hardware	The 99-hardware specs. Needs an Arduino; skips without one.
yarn presentation	Build + regenerate the documentation screenshots
yarn verify	Lint + typecheck + unit + e2e. Run this before pushing.
yarn test:e2e:scan-perf	What a mounted grid costs during a scan. A measurement, not a check.
yarn test:e2e:privileged-port	The port 502 modal. Linux, and someone at the keyboard.
yarn test:all:mac	Everything this platform can run, ending with the hardware specs.
yarn test:all:windows	Everything this platform can run. No socat, so the socat serial specs skip.
yarn test:all:linux	Everything, including the one that waits for a person.

test:e2e covers 01-main and 02-standalone. Two suites sit outside it and are invoked on purpose:

• 99-hardware needs an Arduino on a serial port. It finds the board by USB vendor ID and skips the suite when none is attached, so it runs unattended. CI has no board, which is why it stays out of test:e2e.
• 03-presentation is a documentation utility, not a check. It clicks through the app and captures what it sees without asserting much, so it costs two minutes to tell you little that 01-main does not already cover. Run it when the UI changed and the manual needs new screenshots.

verify deliberately leaves out test:e2e:packaged, which adds a full packaging step and runs far longer than is worth doing on every push. The test:all:* rounds do include it, and those are for cutting a release rather than for a PR. It is the only check that exercises what actually ships: electron-vite externalizes whatever sits in dependencies and electron-builder packs only those into app.asar, so a runtime dependency that drifts into devDependencies passes every normal test and breaks only once installed. Packaged runs use a throwaway user-data directory and never touch an installed Modbux's config.

playwright.config.ts ignores 99-hardware, so neither test:e2e nor test:e2e:packaged picks those specs up. They need an Arduino running tools/arduino/iem3000.ino on a serial port. The board is found by USB vendor ID rather than by manufacturer, which reads "Microsoft" on Windows where the generic driver claims the device. Every test:all:* ends with this round, and yarn test:e2e:hardware runs it alone.

Test expectations

• 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, 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

• Use the helpers from e2e/fixtures/helpers.ts. Don't reinvent them.
• Assert grid contents with expectCell() / expectCellContains(), which retry until the value arrives. Reach for the raw cell() only when the test wants a deliberate point-in-time sample, such as comparing two consecutive polls of a generator. A cell can still hold the previous read while a new read or a freshly loaded mapping is on its way, and a snapshot gets no second chance.
• Use data-testid attributes for selectors. Never select by CSS class or DOM structure.
• Spec files are numbered and ordered. New specs go at the end of their directory.
• Presentation scenes (03-presentation/) generate the documentation screenshots and are excluded from the default run. If you change UI, update the relevant scenes and run yarn presentation.

Pull requests

1. Branch from main. Use feature/description or fix/description.
2. Keep commits clean. Squash fixups before requesting review.
3. Write a clear PR description: what changed, why, and how to test it.
4. yarn verify must pass. No exceptions.
5. Screenshots for UI changes. Before and after.
6. Don't bump the version number. That's done at release time.

What will get your PR rejected

• Failing yarn verify
• any types or disabled lint rules
• Missing tests for new functionality
• Unrelated changes mixed into the diff
• New dependencies without prior discussion
• Changes to .editorconfig, .prettierrc, or .eslintrc without prior discussion
• Commit messages that don't follow the convention

Reporting bugs

Use the bug report template. Include:

• Modbux version
• OS and version
• Steps to reproduce (exact, not approximate)
• Expected vs. actual behavior
• Screenshots if it's a UI issue

Feature requests

Use the feature request template. Explain the problem you're solving, not just the solution you want.

License

By contributing, you agree that your contributions will be licensed under the MIT License.