fix(ethercat): prevent slave name collisions across masters [DOPE-281] by marconetsf · Pull Request #754 · Autonomy-Logic/openplc-editor

marconetsf · 2026-05-11T08:43:57Z

Summary

Source slave name from <Type> (short, e.g. EL1809) instead of the long <Name LcId> descriptor.
Auto-suffix _NN at creation when the base name already exists in any master.
Reject rename to a name already taken by another slave, matching the pattern used by POU / datatype / master renames.
Re-add the long ESI descriptor as a subtitle below the slave header now that the title shows the short form.

Test plan

Scan Bus: add two slaves of the same model → second is auto-suffixed (EL1809, EL1809_01).
Repository tab: same behavior on add.
Cross-bus: add EL1809 on bus A and on bus B → second becomes EL1809_01.
Rename a slave to a name already used by another slave → operation rejected, label reverts.
Rename a slave to its own current name → no-op accepted.
Slave editor shows the long ESI descriptor as a subtitle under the short name.

🤖 Generated with Claude Code

- Source slave name from <Type> (short, e.g. "EL1809") instead of the long <Name LcId> descriptor. - Auto-suffix _NN at creation when the base collides with any existing slave in any master. - Reject rename to a name already taken by another slave, matching the pattern used by POU/datatype/master renames. - Re-add the long ESI descriptor as a subtitle below the slave header now that the title shows the short form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-05-11T08:44:18Z

Warning

Rate limit exceeded

@marconetsf has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 39 minutes and 47 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ef035801-98aa-4131-ba4f-a3f3dbf0ad91

📥 Commits

Reviewing files that changed from the base of the PR and between f06a6cd and 189c134.

📒 Files selected for processing (6)

src/frontend/components/_features/[workspace]/editor/device/ethercat/ethercat-device-editor.tsx
src/frontend/components/_features/[workspace]/editor/device/ethercat/index.tsx
src/frontend/store/__tests__/shared-slice.test.ts
src/frontend/store/slices/shared/slice.ts
src/frontend/utils/__tests__/unique-slave-name.test.ts
src/frontend/utils/unique-slave-name.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/ethercat-slave-name-collision

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

Generate code and open pull requests
Plan features and break down work
Investigate incidents and troubleshoot customer tickets together
Automate recurring tasks and respond to alerts with triggers
Summarize progress and report instantly

Built for teams:

Shared memory across your entire org—no repeating context
Per-thread sandboxes to safely plan and execute work
Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

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

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

The helper was misplaced under backend/shared/ethercat/, which made the store import violate the layer rule "Store must not import from Backend Shared". Move it to frontend/utils/ alongside next-name.ts and ethercat-status.ts — the conventional location for pure helpers shared between store and components. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Inline a single-element array literal that exceeded the explicit-wrap heuristic but fits within the 120-char width. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

JoaoGSP

Thanks for tackling this — the fix works end-to-end and the tests are solid. Before approving I want to flag one thing about scope:

The DOPE-281 description identifies the root cause as tabs, editor, and file slices keying by slave.name alone, and the AC reads "Two masters can hold slaves with the same name; clicking each opens its own editor with its own configuration, independently." This PR takes the inverse approach — it makes duplicate names impossible rather than making the slices handle them.

Both approaches close the user-visible bug, and this one is lower-risk. But two things still worry me:

Legacy projects. A project saved before this fix can still contain duplicate slave names across masters. Opening it will hit the original bug — no migration / load-time de-dup. Is there a known set of in-the-wild projects we should check, or do we want a one-time fixup on project load?
Other write paths. This guard runs in ethercatDeviceActions.rename and in the two creation paths in index.tsx. If anything else ever writes to ethercatConfig.devices[].name (paste, import, undo replay), the name-keyed-slice bug returns. Worth a comment on collectAllSlaveNames saying "all writes must funnel through here," or moving the check into a lower layer.

If the scope change is intentional, please update DOPE-281's description/AC so a future reviewer (or audit) doesn't trip on it. Otherwise happy to approve once we agree on (1).

JoaoGSP · 2026-05-14T13:53:38Z

+ */
+export function generateUniqueSlaveName(base: string, existing: Iterable<string>): string {
+  const taken = new Set(existing)
+  let candidate = base


Tiny doc nit: "Pad widens past 99 automatically" reads like the pad value changes — it doesn't. The behavior you're describing is that padStart(2, '0') doesn't truncate, so String(100) passes through as "100". Maybe: "Two-digit pad doesn't truncate, so 3+ digit indices pass through unchanged."

JoaoGSP · 2026-05-14T13:53:38Z

+  let candidate = base
+  let i = 0
+  while (taken.has(candidate)) {
+    i++


Minor: when existing is already a Set (which is the hot path inside the scan-bus loop), this copies it on every call. Not a real cost at our scale, but existing instanceof Set ? existing : new Set(existing) is free if you want it. Optional.

JoaoGSP · 2026-05-14T13:53:38Z

+      // Prefer the short product code from <Type> (e.g. "EL1809") over the
+      // long localized name from <Name LcId="1033"> — the long form is
+      // verbose and identical for any two units of the same model.
+      const baseName = bestMatch.esiDevice.type.name || bestMatch.esiDevice.name || match.device.name


Sanity-check question: is esiDevice.type.name reliably populated as the short product code (EL1809, EK1100, …) across all vendor ESI XMLs we've parsed — not just Beckhoff? If type.name is ever empty for a real ESI, the fallback chain drops back to the long localized name and we lose the readability win this PR is trying to deliver. A one-line confirmation (or a quick grep across the test fixtures) would settle it.

JoaoGSP · 2026-05-14T13:53:38Z

+      // Same-name rename is a no-op the UI short-circuits before us, but we
+      // still allow it here so the action stays idempotent.
+      if (newName !== oldName && collectAllSlaveNames(state.project.data.remoteDevices).has(newName)) {
+        return { ok: false, message: `An EtherCAT slave named "${newName}" already exists in this project` }


Worth a short comment here noting that this is the only enforcement point for slave-name uniqueness, and that the underlying name-keyed slices (tabs/editor/file) will silently misbehave if a duplicate ever slips through. That way future readers of this file know not to add a sibling write path without the same guard.

marconetsf and others added 3 commits May 11, 2026 10:45

Merge branch 'development' into fix/ethercat-slave-name-collision

5fd8358

style(ethercat): apply prettier to unique-slave-name test

189c134

Inline a single-element array literal that exceeded the explicit-wrap heuristic but fits within the 120-char width. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

marconetsf requested a review from JoaoGSP May 11, 2026 09:51

JoaoGSP reviewed May 14, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ethercat): prevent slave name collisions across masters [DOPE-281]#754

fix(ethercat): prevent slave name collisions across masters [DOPE-281]#754
marconetsf wants to merge 4 commits into
developmentfrom
fix/ethercat-slave-name-collision

marconetsf commented May 11, 2026 •

edited by atlassian Bot

Loading

Uh oh!

coderabbitai Bot commented May 11, 2026 •

edited

Loading

Rate limit exceeded

Uh oh!

JoaoGSP left a comment

Uh oh!

JoaoGSP May 14, 2026

Uh oh!

JoaoGSP May 14, 2026

Uh oh!

JoaoGSP May 14, 2026

Uh oh!

JoaoGSP May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

marconetsf commented May 11, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

coderabbitai Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Uh oh!

JoaoGSP left a comment

Choose a reason for hiding this comment

Uh oh!

JoaoGSP May 14, 2026

Choose a reason for hiding this comment

Uh oh!

JoaoGSP May 14, 2026

Choose a reason for hiding this comment

Uh oh!

JoaoGSP May 14, 2026

Choose a reason for hiding this comment

Uh oh!

JoaoGSP May 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

marconetsf commented May 11, 2026 •

edited by atlassian Bot

Loading

coderabbitai Bot commented May 11, 2026 •

edited

Loading