feat(compile): one table list, every server counted, and no output driven twice

[JulioSergioFS]

Stacked on #1093 — targets that branch, so the diff here is only group B's own commit.

Four things that belong together, because the first is what makes the rest safe to add.

One canonical table list

"Which tables exist, which IEC prefix each stores, in which unit, on which runtime" was written out in five places — the two area sets, image.conf's keys, the bare-metal macros and the S7comm buffer enum — and a sixth was about to be added for the S7comm sizer. Every way five copies can disagree is silent: a table in one list and not another is an area sized to zero, a prefix paired wrongly sizes the wrong storage, a unit written differently is a factor of eight nobody sees until an address stops answering.

All five now derive from middleware/shared/utils/io-image/tables.ts. Adding a table is one line there.

No output moves. The list is in image_tables.h declaration order, and filtering it to the entries carrying a macro yields exactly the order defines.h already emitted — checked by a test, because a gratuitous reorder rebuilds every firmware for nothing.

Every server counts, not just Modbus

serverExposure looked for protocol === 'modbus-tcp' and nothing else, so a project with an S7comm server and no Modbus one sized nothing at all from its servers.

Still the first server of each protocol, deliberately: both config emitters ship one file built from the first one carrying a config, so a second never reaches the device and sizing for it would reserve memory nothing can use.

An S7comm data block sizes the table it names, converting sizeBytes to that table's elements and adding startBuffer. Two conversions meet there and pull opposite ways, so it is written once and asserted from both directions.

An S7comm block backs what it covers, inputs included

The opposite of the Modbus answer, and the difference is a fact about the protocols rather than a preference. Modbus discrete inputs and input registers are read-only to the master by the protocol, so exposing an %IX through Modbus cannot put anything into it. S7comm has no such restriction and the plugin implements none — write_buffer_to_openplc_journal dispatches every buffer type, BUFFER_TYPE_*_INPUT included. An S7 client writes an input table as readily as an output one, so BR14's question answers yes in both directions here.

Two POUs cannot drive one output

checkIfLocationExists reads one variable list, so each POU passes on its own while IEC located addresses are global. The generated code then assigns to the same storage from two places and the last write in the scan wins, decided by POU order.

Checked per slot, so overlapping located arrays are caught at the slot they share — and reported once per pair, because a 4000-element array declared twice is one mistake. Outputs only: two POUs reading one input is ordinary, and sharing a memory address is what memory is for.

The same fact is answered while editing, through the amber glyph the alias scan already uses.

Plus: a VPP manifest channel's address prefix is now checked against the eight the package schema admits. It arrives as an unchecked string through an addressMapping typed unknown, and a manifest declaring %MW would have memory allocated for it as if a module produced it.

Verification

3317 tests across the areas touched, 100% statements/lines/functions on compute-io-image.ts, tsc and validate:arch clean, compare-surfaces "match": true over 1117 files.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)

• development

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 73a15f63-ba58-4f68-86d4-e7df5149eabf

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[marconetsf]

Severity on every point, here and inline: 🔴 Major (must fix before merge) · 🟡 Changes required (should be addressed) · 🟢 Nit (optional) · ❓ Question. Each carries a confidence out of ten.

One 🔴 Major, three 🟡 Changes required, four 🟢 Nits and one ❓ Question, all inline.

I ran the validation this PR does not get from CI — it targets a feature branch, so ci.yml never fired and only CodeRabbit ran:

• npx tsc --noEmit — clean.
• npx jest src/backend/shared/compile src/middleware/shared/utils/io-image — 396 passed, 15 suites.

The consolidation in (a) does what it claims, and I checked it rather than trusting the test. I reproduced all five derived lists from the diff's pre-image and compared them against what IMAGE_TABLES now produces:

derived list	membership	order	verdict
IMAGE_AREAS_BAREMETAL	identical	identical	no change
IMAGE_AREAS_RUNTIME_V4	identical	changed	no behavioural change — only ever consumed through .has()
image.conf TABLES	identical	identical	key/prefix/unit identical row for row, byte-identical output
PROCESS_IMAGE_MACROS	identical	identical	no macro moves, so no gratuitous firmware rebuild
S7CommBufferTypeSchema	identical	changed	no validation change — z.enum is a set test and .options has no consumer

The two reorders are harmless for the reasons above, but the PR body does not mention them and the S7CommBufferTypeSchema one is worth a sentence there, since a reader seeing a persisted enum reorder will want to know someone checked.

The byte-to-element conversion is written once and is correct for every width. extentForDataBlock divides by BYTES_PER_ELEMENT then multiplies by ADDRESSES_PER_ELEMENT: 128 bytes is 1024 bits / 128 bytes / 64 words / 32 dwords / 16 lwords. Both directions are asserted, and startBuffer is correctly added in elements before the address multiply, which matches the unit the schema comment on startBuffer documents. A partial trailing element is floored, which is the right direction.

Duplicate outputs: I traced the cases end to end rather than reading test names. Confirmed caught: two POUs on one %QX0.0; two declarations in one POU; a POU-local against a configuration global; overlapping located arrays at a shared slot that is neither base address; and the ST text path, which writes back into the same pou.data.variables so there is no second declaration channel to bypass. Correctly silent for %IX, %IW, %MW and %MX. "Once per pair" is genuinely implemented — the probe breaks on the first colliding slot.

Two gaps I could not close, neither of which I am filing as findings: variables in globalVariableLists are not walked by locatedVariables (pre-existing, shared with the BR14 check, and the docstring defends it); and there is no test pairing a literal %QW5 against an alias that resolves to %QW5.

One thing I could not verify at all: extentForDataBlock ignores bitAddressing. If the runtime plugin reinterprets start_buffer as a bit index when that flag is set, a BOOL block would be sized eightfold. The plugin source is not in this repo and no test covers bitAddressing: true.

🟢 Nit · confidence 9/10 — the edit-time glyph is missing on the side the compile check explicitly covers. Raising it here because the file is not in this diff, which is the finding. useDuplicateOutputLocations deliberately scans configuration globals, and the compile check has a passing test for refuses a POU-local colliding with a configuration global. But only _molecules/variables-table/editable-cell.tsx consumes the hook — _molecules/global-variables-table/editable-cell.tsx still calls useProjectAliasBindings alone. So the user editing the global side of exactly the collision this feature is about sees nothing until they build. It is the same three-line change as in the sibling cell.

[marconetsf]

🔴 Major · confidence 9/10 · an S7comm block backs addresses it does not cover

const extent = extentForDataBlock(table, block.mapping.startBuffer, block.sizeBytes)
if (extent <= 0) continue
claim(sizes, table.prefix, extent)
markBacked(backed, table.prefix, 0, extent)

extent is a high-water mark — startBuffer + elements, scaled to addresses. That is exactly right for claim, because the image is contiguous. It is not the covered range, and markBacked(..., 0, extent) marks [0, extent).

So a block { type: 'int_input', startBuffer: 100, sizeBytes: 8 } backs %IW0 through %IW103, when the plugin only ever writes int_input[100..103]. AT %IW0 : INT then compiles clean with nothing producing it and reads zero forever on the machine — which is precisely the declaration BR14 exists to refuse.

This contradicts the file's own rule in two places. producerClaims marks per slot (markBacked(backed, prefixOf(parsed.cls), parsed.linear, 1)) and its comment says "a producer at slot 9 alone still needs ten slots — but slots 0 through 8 have nothing behind them". The unbacked loop says "an address in a gap between two producers is within the image and still has nothing behind it, which is the case BR14 exists for". And this function's own docstring claims per-slot semantics: "A block mapped onto int_input is a producer for the %IWs it covers".

The Modbus path a few lines up may legitimately mark from 0 — Modbus segments always start at IEC index 0. S7comm blocks do not, which is what startBuffer is for.

Fix: back only the covered range. Having extentForDataBlock return { start, end } is cleaner than exporting the scale factor, then markBacked(backed, table.prefix, start, extent - start).

No test catches this: still refuses an input address the block does not reach uses startBuffer: 0 and probes an address above the extent, and adds the start buffer to the extent asserts only sizes. The missing case is a block at startBuffer: 100 leaving AT %IW0 : INT in unbacked.

[marconetsf]

🟡 Changes required · confidence 9/10 · reintroduces the O(N) blow-up the memory path was just fixed to avoid

for (let slot = parsed.linear; slot < parsed.linear + slotCount; slot++) {
  if (!owners.has(slot)) owners.set(slot, { scope, variableName: name })
}

This runs unconditionally for every %Q declaration. Thirty lines below, the memory branch carries the comment explaining why the identical pattern was removed there:

No markBacked here, deliberately. […] it was dead at a cost: the loop ran once per declared element, so AT %MW0 : ARRAY [0..10000000] OF WORD inserted ten million Set entries in the main process before the platform compiler ever got to refuse the size.

AT %QW0 : ARRAY [0..10000000] OF WORD now does exactly that, and worse — a Map entry with an object value per slot rather than a Set entry — in the Electron main process, before the compiler is reached. The probe loop above is safe because it breaks at the first collision; this one has no escape.

The repo already has the right primitive, and it currently has no non-test callers: slotRangesOverlap(a, aSlots, b, bSlots) in src/middleware/shared/utils/iec-address/registry/address-space.ts. Keeping a per-prefix list of { start, end, scope, name } intervals and comparing ranges is O(declarations²) at worst, which is nothing, and it matches the helper's documented semantics including the %QX0.0 ARRAY[0..9] against %QX0.6 case.

[marconetsf]

🟡 Changes required · confidence 9/10 · the message reports a number the user cannot map back to their code

`"${issue.second.variableName}" (${where}) both cover slot ${issue.slot} of ` +

issue.slot is parsed.linear, and for bit classes parseAddress computes byte * 8 + bit. So two variables at %QX3.2 produce "both cover slot 26 of %QX" — the user has to divide by eight in their head to get back to the address they typed. %QX is the most common duplicate-output case by far.

The address is already being carried: DuplicateOutput.location is populated at the push site and read nowhere. The unbacked message next door gets this right by echoing issue.location.

Fix: format the slot back into an address so a bit slot renders as %QX3.2, or at minimum include issue.location for the second declaration and add the first's location to the DTO. The tests assert only that both names appear, so nothing catches this.

[marconetsf]

🟡 Changes required · confidence 9/10 · the tooltip empties itself in the most common case

const otherWriters = (duplicateOutputs.get(locationValue) ?? []).filter(
  (name) => name !== variable?.name,
)

DuplicateOutputMap is ReadonlyMap<string, readonly string[]> — bare names — so this filters the other writers by name. But the whole point of the feature is that the two declarations are usually in different POUs, where the same name is entirely ordinary: run, motor_on, out.

When both are called the same thing the filter removes every entry and the tooltip renders as "Output %QX0.0 is also driven by . IEC located addresses are global, so the last write in the scan would win — the compiler refuses this." — while the amber glyph still shows.

Fix: have the hook store { scope, name } instead of bare names and exclude the current row by scope and name together. That also lets the tooltip say which POU, which is the information the user actually needs to go and fix it. Neither this hook nor this branch has any test.

[marconetsf]

🟢 Nit · confidence 9/10 · cache fields typed unknown, and no test for the hook

interface Cache {
  pous: unknown
  globals: unknown
  map: DuplicateOutputMap
}

The hook this is modelled on, use-project-alias-bindings.ts, types the same fields PLCPou[] and PLCVariable[] | undefined. unknown costs nothing at runtime but gives up the compiler's check that the identity being compared is the identity being read, which is the only thing keeping this cache correct.

There is also no test file for this hook, so the cache invalidation, the %Q-only predicate and the tooltip branch above are all unasserted. One test building the map for two POUs — an input pair that must be absent and an output pair that must be present — would cover the useful part.

[marconetsf]

🟢 Nit · confidence 9/10 · two new exports with no production callers

grep -rn 'elementsFor\|tableForPrefix' src outside this file returns only the new test. Neither is used by generate-image-conf.ts, generate-defines.ts or compute-io-image.ts.

Worth noting it is not a missed call site: generate-defines.ts keeps its own firmwareCount (Math.ceil(slots / 8) * 8, bits padded to a byte multiple), which is a genuinely different conversion from elementsFor (bits to bytes). The functions simply have no consumer yet. Either wire them into one, or drop them so the module's exported surface is the surface something actually depends on.

[marconetsf]

🟢 Nit · confidence 7/10 · the warning repeats on every render

Dropping the channel rather than throwing is the right call and the comment defends it well. But resolveModuleChannels is a pure resolver called per slot and per render, not a one-shot load step, so a manifest with one bad channel produces an unbounded stream of identical console lines — which makes the log less useful rather than more, and works against the stated goal that "without this line there is nothing anywhere saying why".

Dedupe with a module-level Set keyed on ${moduleId}:${channel.name}:${prefix}, or surface it once where the manifest is loaded.

Separately, I could not verify the eight-prefix list against the package schema — grep -rn addressPrefix --include=*.json finds no manifest schema in this repo, so the claim that the schema admits exactly those eight rests on the comment. If the schema lives in openplc-packages, a pointer to it in the comment would let the next reader check.

[marconetsf]

❓ Question · confidence 6/10 · should a disabled S7comm server back input addresses?

serverExposure picks the first server with protocol === 's7comm' && s7commSlaveConfig, mirroring generateS7commConfig's predicate exactly — I checked, they are identical — and config.server.enabled is never consulted. For Modbus the existing docstring defends that choice, and it is harmless there because Modbus exposure only backs outputs.

With (c), an S7comm block now vouches for %IX, %IW, %ID and %IL as well. So a server with server.enabled: false — which ships a config the runtime will not serve — makes AT %IW7 : INT compile clean with genuinely nothing producing it.

Is that intended? Every test uses server: { enabled: true }, so either answer is currently unasserted. If the answer is that enabled should gate the S7comm branch, it is a one-line change plus a test; if it is that a disabled server still counts, that belongs in the docstring beside the Modbus reasoning.