fix(ethercat): prevent slave name collisions across masters [DOPE-281] - #754
Merged
Conversation
- 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>
Contributor
WalkthroughThis PR introduces utility functions for deterministic EtherCAT slave name generation and collection, integrates them into store rename validation and device-add flows to enforce globally-unique names, and displays ESI-derived device identifiers in the editor header. ChangesEtherCAT Slave Unique Naming
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
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
reviewed
May 14, 2026
JoaoGSP
left a comment
JoaoGSP
left a comment
Member
There was a problem hiding this comment.
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.renameand in the two creation paths inindex.tsx. If anything else ever writes toethercatConfig.devices[].name(paste, import, undo replay), the name-keyed-slice bug returns. Worth a comment oncollectAllSlaveNamessaying "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).
| let candidate = base | ||
| let i = 0 | ||
| while (taken.has(candidate)) { | ||
| i++ |
Member
There was a problem hiding this comment.
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.
The previous chain assumed <Type> always carried text, but the ESI schema only requires its ProductCode/RevisionNo attributes. Vendors emitting self-closing <Type/> were silently dropping to the long localized <Name>, defeating the readability win of short product codes. Introduce getShortDeviceName with a tiered fallback that validates <Type> shape, extracts SKU-shaped tokens from <Name>, and falls back to the canonical (productCode, revisionNo) identity when nothing readable is available. Also clarify the unique-slave-name padStart docstring and document the rename guard as the sole rejecting enforcement point for slave-name uniqueness across masters. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/components/_features/[workspace]/editor/device/ethercat/index.tsx (1)
473-503:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse latest store state when generating/appending repository-added devices.
After
await(Line 476), Line 486 and Line 502 still use render-time snapshots (project.data.remoteDevices,configuredDevices). Rapid consecutive adds can produce duplicate names/positions or drop a previously added device.💡 Proposed fix
const handleAddDeviceFromBrowser = useCallback( async (ref: ESIDeviceRef, device: ESIDeviceSummary, repoItem: ESIRepositoryItemLight) => { @@ - const nextPosition = - configuredDevices.length > 0 ? Math.max(...configuredDevices.map((d) => d.position ?? 0)) + 1 : 1 + const { project: latestProject } = useOpenPLCStore.getState() + const latestRemoteDevices = latestProject.data.remoteDevices ?? [] + const latestMaster = + latestRemoteDevices.find((d) => d.name === deviceName)?.ethercatConfig?.devices ?? [] + + const nextPosition = + latestMaster.length > 0 ? Math.max(...latestMaster.map((d) => d.position ?? 0)) + 1 : 1 const baseName = getShortDeviceName(device) - const uniqueName = generateUniqueSlaveName(baseName, collectAllSlaveNames(project.data.remoteDevices)) + const uniqueName = generateUniqueSlaveName(baseName, collectAllSlaveNames(latestRemoteDevices)) @@ - syncDevicesToStore([...configuredDevices, newDevice]) + syncDevicesToStore([...latestMaster, newDevice])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/_features/`[workspace]/editor/device/ethercat/index.tsx around lines 473 - 503, In handleAddDeviceFromBrowser: after the await that loads the full device, re-read the current store state instead of using the render-time snapshots (project.data.remoteDevices and configuredDevices) so name/position generation and the final sync use the latest list; specifically, call the store/getter you use elsewhere to obtain the up-to-date remoteDevices and configuredDevices, then pass those into collectUsedIecAddresses, collectAllSlaveNames/generateUniqueSlaveName and compute nextPosition, and finally call syncDevicesToStore with the fresh list merged with the new device (rather than spreading the stale configuredDevices variable).
🧹 Nitpick comments (1)
src/frontend/store/slices/shared/slice.ts (1)
9-9: ⚡ Quick winUse path alias instead of relative import.
Replace the relative import with the
@root/*path alias for consistency with project conventions.♻️ Proposed fix
-import { collectAllSlaveNames } from '../../../utils/unique-slave-name' +import { collectAllSlaveNames } from '`@root/frontend/utils/unique-slave-name`'As per coding guidelines, prefer path alias
@root/*for imports instead of relative paths to./src/*.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/store/slices/shared/slice.ts` at line 9, Replace the relative import of collectAllSlaveNames with the project path alias: locate the import statement that currently reads "import { collectAllSlaveNames } from '../../../utils/unique-slave-name'" in slice.ts and change it to use the `@root` alias (e.g. import { collectAllSlaveNames } from '`@root/utils/unique-slave-name`') so it follows the project's path-alias convention; ensure the module specifier matches the existing alias pattern used elsewhere in the repo and that the symbol name collectAllSlaveNames remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/index.tsx:
- Around line 473-503: In handleAddDeviceFromBrowser: after the await that loads
the full device, re-read the current store state instead of using the
render-time snapshots (project.data.remoteDevices and configuredDevices) so
name/position generation and the final sync use the latest list; specifically,
call the store/getter you use elsewhere to obtain the up-to-date remoteDevices
and configuredDevices, then pass those into collectUsedIecAddresses,
collectAllSlaveNames/generateUniqueSlaveName and compute nextPosition, and
finally call syncDevicesToStore with the fresh list merged with the new device
(rather than spreading the stale configuredDevices variable).
---
Nitpick comments:
In `@src/frontend/store/slices/shared/slice.ts`:
- Line 9: Replace the relative import of collectAllSlaveNames with the project
path alias: locate the import statement that currently reads "import {
collectAllSlaveNames } from '../../../utils/unique-slave-name'" in slice.ts and
change it to use the `@root` alias (e.g. import { collectAllSlaveNames } from
'`@root/utils/unique-slave-name`') so it follows the project's path-alias
convention; ensure the module specifier matches the existing alias pattern used
elsewhere in the repo and that the symbol name collectAllSlaveNames remains
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0349893b-bf66-4316-8bfa-bce0901432de
📒 Files selected for processing (8)
src/frontend/components/_features/[workspace]/editor/device/ethercat/ethercat-device-editor.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/index.tsxsrc/frontend/store/__tests__/shared-slice.test.tssrc/frontend/store/slices/shared/slice.tssrc/frontend/utils/__tests__/short-device-name.test.tssrc/frontend/utils/__tests__/unique-slave-name.test.tssrc/frontend/utils/short-device-name.tssrc/frontend/utils/unique-slave-name.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
<Type>(short, e.g.EL1809) instead of the long<Name LcId>descriptor._NNat creation when the base name already exists in any master.Linked: DOPE-281
Test plan
EL1809,EL1809_01).EL1809on bus A and on bus B → second becomesEL1809_01.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes