fix(modbus): make IO Group size edits visible and reject invalid lengths (DOPE-439)

[JulioSergioFS]

Pull request info

Description of the changes proposed

The ticket's literal root cause — updateIOGroup only Object.assigning the
metadata and never regenerating ioPoints — was already fixed in b81f7ee5a
(v4.2.8). This PR closes the three remaining paths by which the ticket still
reproduced, none of which were in the store:

• The resize was invisible. The IO Group table had no Length column, and
  the Address cell showed only the first point's iecLocation — which does
  not move when the group grows. Testing the ticket on a collapsed row read as
  "nothing changed". Adds a Length column between Offset and Function Code,
  and turns Address into a span (%IW0 – %IW3), which also makes the
  project-wide address recompaction legible.
• FC 5 / FC 6 locked Length at 1 with no explanation — readOnly plus
  opacity-50 and no help text. Correct Modbus semantics (Write Single Coil /
  Write Single Register address exactly one element), but the UI never said so.
  Adds an inline hint that explains the rule and points at FC 15 / FC 16.
• A negative Length passed validation. parseInt(length, 10) || 1 does not
  catch -5 (the || 1 only covers 0 and NaN), so the group ended up with
  zero I/O points and len: -5 reached the generated Modbus master config.
  Adds input validation bounded by each function code's PDU limit (FC 3/4 = 125,
  FC 16 = 123, FC 1/2 = 2000, FC 15 = 1968), and a floor-only clamp as a
  store invariant on both addIOGroup and updateIOGroup.

New pure util src/frontend/utils/modbus/io-group.ts holds the rules
(clampIOGroupLength, validateIOGroupLength, formatIOGroupAddressRange,
isSingleElementFunctionCode), replacing the functionCode === '5' || === '6'
literal that was duplicated in the component.

Decisions worth flagging in review

• The clamp applies the floor only, never the per-FC maximum. Enforcing the
  maximum in the store would silently truncate a pre-existing FC 3 group of
  length 200 the moment a user edited only its name — data loss introduced by
  a bug fix. The maximum is an input rule; a test pins this.
• The store normalizes the persisted length, not just the point count,
  because that field is what generate-modbus-master-config.ts ships to the
  runtime as len. Two things fall out for free: switching a group to FC 5/6
  forces length: 1 even if a caller forgets, and a bad length loaded from an
  old project file self-heals on first edit.
• ModbusIOGroupSchema.length is deliberately left as z.number(). An
  .int().min(1) refinement fails PLCRemoteDeviceSchema.safeParse, and the
  load path responds by dropping the whole remote device with no
  diagnostic — trading a bad number for a vanished device. Normalization
  belongs in the store. A safe load-time coercion
  (z.number().transform(...)) is filed separately.
• Inline hint instead of the Tooltip atom: the field lives inside a Radix
  Modal, so a tooltip means a portal nested in a portal with z-[999]
  stacking to reason about — and a permanently locked control deserves an
  always-visible reason, not a hover-discoverable one.

Out of scope (filed separately)

• Shrinking a group silently deletes the dropped slots' aliases, orphaning any
  variable bound to them. Orphaning is deliberate policy — see the
  renameAlias comment in project/slice.ts — so what's missing is a warning,
  not a data-model change.
• Deleting an IO group has no confirmation at all and destroys every alias in
  the group; strictly worse than the shrink case, and should reuse the same
  modal.
• ModbusIOPoint.id is ${groupName}_${i}, so two same-named groups produce
  colliding ids (React keys, and the sourceRef used by the alias-uniqueness
  gate).
• DOPE-440 already covers address recalculation after gaps. This PR only makes
  visible what DOPE-440 fixes.

Verification

Scoped runs only, Node v22:

npx jest src/frontend/utils/modbus src/frontend/store/__tests__/project-slice.test.ts --no-coverage

• 341 tests green (editor, Jest) / 341 green (web, Vitest).
• New util: 100% statements, branches, functions and lines.
• tsc --noEmit, npm run validate:arch, eslint and prettier clean in both repos.
• scripts/compare-surfaces.py: total_diffs: 0 across 1029 files.

Manual, in the running app: created an FC 3 group of length 4 (%IW0 – %IW3) plus
a second of length 2 (%IW4 – %IW5); grew the first to 6 → reads %IW0 – %IW5
with the sibling sliding to %IW6 – %IW7, aliases t0–t3 intact and the two
new slots blank; -5, 0, 3.5 and 126 each block Save with a reason;
switching to FC 5 snaps Length to 1 with the hint and yields %QX0.0; the
generated master config has len >= 1 and never falls back to %MW0.

Summary by CodeRabbit

• New Features

  • Added configurable length handling for Modbus I/O groups.
  • Group lengths are automatically normalized and limited according to the selected function code.
  • Group rows now display complete address ranges and point counts.
  • Added clearer alignment and a dedicated Length column in the I/O group table.

• Bug Fixes

  • Invalid, zero, negative, or fractional lengths are corrected automatically.
  • Single-element function codes now consistently create one point.
  • Group creation is disabled when the entered length is invalid, with validation feedback displayed.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 06e0037c-897c-40a7-a640-5b81a10e21bd

📥 Commits

Reviewing files that changed from the base of the PR and between 6e7584c and a6c91b6.

📒 Files selected for processing (5)

• src/frontend/components/_features/[workspace]/editor/device/remote-device/index.tsx
• src/frontend/store/__tests__/project-slice.test.ts
• src/frontend/store/slices/project/slice.ts
• src/frontend/utils/modbus/__tests__/io-group.test.ts
• src/frontend/utils/modbus/io-group.ts

---

Walkthrough

The change adds shared Modbus I/O group length utilities, normalizes lengths in the project store, validates editor input, and displays complete address ranges and lengths in the remote-device table.

Changes

Modbus I/O group length handling

Layer / File(s)	Summary
Define Modbus I/O group rules src/frontend/utils/modbus/io-group.ts, src/frontend/utils/modbus/__tests__/io-group.test.ts	The utilities define function-code limits, normalize and validate lengths, and format occupied address ranges. Tests cover these rules and boundary cases.
Normalize persisted group lengths src/frontend/store/slices/project/slice.ts, src/frontend/store/__tests__/project-slice.test.ts	Group creation and updates persist normalized lengths and regenerate points with those lengths. Regression tests cover invalid, fractional, and single-element values.
Apply validation in the remote-device editor src/frontend/components/_features/[workspace]/editor/device/remote-device/index.tsx	The editor validates length input, blocks invalid submissions, restores single-element lengths, and displays address ranges and group lengths with updated table alignment.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RemoteDeviceEditor
  participant validateIOGroupLength
  participant ProjectStore
  RemoteDeviceEditor->>validateIOGroupLength: Validate function code and length input
  validateIOGroupLength-->>RemoteDeviceEditor: Return validation result
  RemoteDeviceEditor->>ProjectStore: Submit validated group length
  ProjectStore-->>RemoteDeviceEditor: Persist normalized group and points

Loading

Suggested reviewers: thiagoralves, joaogsp

Poem

A rabbit checks each Modbus row,
And trims the lengths where limits show.
One point stays for codes five and six,
Ranges line up with tidy tricks.
The table hops, validated and bright.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the Modbus IO Group visibility and invalid-length validation changes.
Description check	✅ Passed	The description thoroughly explains the changes, design decisions, scope, and verification results, but omits the template checklist and issue links.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch bugfix/DOPE-439-io-group-size-edit

---

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.}

[Gustavohsdp]

Review — approved from my side; nothing to change

Reviewed together with the web sync PR Autonomy-Logic/openplc-web#663 — all 5 files byte-identical, so this applies to both.

I read this against the ticket text (DOPE-439: "When editing an already-existing IO Group, it is not possible to change its size", High) rather than just against the diff.

Verified: 5/5 mirrored files byte-identical (git hash-object); 329 tests green (io-group, project-slice); CI 12/12 on the editor side, 9/9 on web.

The two claims in the description, checked

• "The ticket's literal root cause was already fixed in b81f7ee5a (v4.2.8)" — confirmed. git tag --contains b81f7ee5a returns v4.2.8 and v4.2.9. Saying so in the description instead of quietly re-fixing it is the right call; the alternative is closing the ticket as "cannot reproduce" and having the reporter come back with the same complaint.

• "A negative Length passed validation" — confirmed:

  parseInt('-5',10) || 1  = -5     ← passed
  parseInt('0',10)  || 1  = 1
  parseInt('abc',10)|| 1  = 1
  

  The || 1 only covers 0 and NaN, so a group ended up with zero I/O points and len: -5 reached the generated Modbus master config. That's the one with runtime consequences, and it's now bounded at the input plus floored as a store invariant.

The part I found most interesting

Two of the three remaining reproduce paths weren't store bugs at all — they were observability bugs. The table had no Length column, and the Address cell rendered only the first point's iecLocation, which doesn't move when the group grows. So the resize was happening and looked like it wasn't, when tested on a collapsed row. That's a hard thing to find, because the instinct is to keep digging in the store. Turning Address into a span (%IW0 – %IW3) also makes the project-wide recompaction legible, which pays off directly for DOPE-440.

Two decisions I'd have flagged if they weren't already handled

• PDU limits are correct per the Modbus application protocol — I checked each: FC 1/2 = 2000, FC 3/4 = 125, FC 15 = 1968, FC 16 = 123, FC 5/6 = 1.
• The clamp applies the floor only, never the per-FC maximum, and the comment explains why: enforcing the maximum in the store would truncate a pre-existing FC 3 group of length 200 the moment a user edited only its name — data loss introduced by a bug fix. There's a test pinning it. This is exactly the kind of decision that's cheap to get wrong and expensive to notice.

Normalising the persisted length rather than only the point count is also the right level, since that field is what generate-modbus-master-config.ts ships as len.

One coordination note

This PR and the DOPE-440 pair both touch src/frontend/store/slices/project/slice.ts — different regions (addIOGroup/updateIOGroup here, address allocation there), so the conflict should be mechanical, but whichever merges second needs a rebase. And keep each pair's two repos merging in short sequence: the shared-surface gate only tolerates divergence while an open PR exists in the other repo.

_{Review assisted by Claude Code.}