refactor(ethercat): extract concrete EthercatConfig / EtherCATMasterConfig types#737
Merged
Merged
Conversation
…rConfig types Replace the inline anonymous shape of PLCRemoteDevice.ethercatConfig with two named interfaces in the shared ports layer, so consumers can reference them directly instead of duplicating the shape or widening to Record<string, unknown>. Drops the `as unknown as ConfiguredEtherCATDevice[]` casts in the EtherCAT editor components and the `as EthercatConfig` casts around masterConfig syncing in the project slice. The updateEthercatConfig action now receives an EthercatConfig directly rather than the Record fallback. Stacked on feat/ethercat-esi-backend (PR #731) — the EtherCAT base shape only lives on that branch until #731 lands in development. Mirrored in openplc-web on development directly (that side already carries the base shape). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThis pull request introduces comprehensive EtherCAT (Ethernet-based Fieldbus) support to the OpenPLC Editor, including ESI (EtherCAT Slave Information) XML parsing and repository management, device discovery and matching against repository entries, per-slave configuration UI, and runtime JSON generation for the firmware build pipeline. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/backend/editor/compiler/compiler-module.ts (1)
1431-1431:⚠️ Potential issue | 🟠 Major
#getBoardRuntimenow throws but callers don't catch — message port leaks.
#getBoardRuntimewas tightened to throw when the board is missing fromhals.json(Line 187-193), but both callers (compileProgramat Line 1431 andcompileForDebuggerat Line 2206) invoke it before entering atry/catchblock. A missing-board error now rejects the outercompileProgram/compileForDebuggerpromise without closing_mainProcessPortor emitting anerrorlog to the renderer — the UI will just hang. Wrap these calls so the existing error-reporting/port-close pattern runs.🛡️ Proposed fix
- const boardRuntime = await this.#getBoardRuntime(boardTarget) // Get the board runtime from the hals.json file + let boardRuntime: string + try { + boardRuntime = await this.#getBoardRuntime(boardTarget) + } catch (error) { + _mainProcessPort.postMessage({ + logLevel: 'error', + message: `${getErrorMessage(error)}\nStopping compilation process.`, + }) + _mainProcessPort.close() + return + }Apply the equivalent guard around the
compileForDebuggercall at Line 2206.Also applies to: 2206-2206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/editor/compiler/compiler-module.ts` at line 1431, The call to the private method `#getBoardRuntime` now throws when a board is missing, but compileProgram and compileForDebugger call it outside their existing try/catch so errors can escape and leak _mainProcessPort and fail to emit the renderer error; update both callers (compileProgram and compileForDebugger) to wrap the `#getBoardRuntime`(boardTarget) invocation in the same try/catch/error-report/port-close pattern used elsewhere: catch the thrown error, call the existing error-emission logic to the renderer and ensure _mainProcessPort is closed (or cleaned up) before rethrowing or returning, matching the surrounding error-handling flow so no port or UI state is left hanging.src/frontend/components/_molecules/project-tree/index.tsx (1)
424-447:⚠️ Potential issue | 🟠 MajorMake EtherCAT leaf identifiers required for EtherCAT actions.
busNameanddeviceIdare optional, so anethercatDeviceleaf can render Rename/Delete but silently do nothing when either prop is missing. Use a discriminated props union so the compiler enforces both identifiers for EtherCAT leaves.Proposed type shape
-type IProjectTreeLeafProps = ComponentPropsWithoutRef<'li'> & { - leafLang: +type ProjectTreeLeafLang = | 'il' | 'st' | 'python' | 'cpp' @@ | 'server' | 'remoteDevice' | 'ethercatDevice' + +type IProjectTreeLeafProps = ComponentPropsWithoutRef<'li'> & + ( + | { + leafLang: Exclude<ProjectTreeLeafLang, 'ethercatDevice'> + leafType: WorkspaceProjectTreeLeafType + label?: string + busName?: never + deviceId?: never + } + | { + leafLang: 'ethercatDevice' + leafType: WorkspaceProjectTreeLeafType + label?: string + busName: string + deviceId: string + } + ) - leafType: WorkspaceProjectTreeLeafType - label?: string - busName?: string - deviceId?: string -}Also applies to: 523-582, 621-664
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_molecules/project-tree/index.tsx` around lines 424 - 447, The props type IProjectTreeLeafProps currently makes busName and deviceId optional, allowing an ethercatDevice leaf (leafLang === 'ethercatDevice') to render actions that silently no-op; change IProjectTreeLeafProps to a discriminated union keyed on leafLang/WorkspaceProjectTreeLeafType so that when leafLang is 'ethercatDevice' the type requires busName and deviceId, while other variants keep them optional or absent; update any other similar leaf prop types/usages (the other project-tree leaf definitions referenced in the review) to the same discriminated-union pattern so the compiler enforces the identifiers for EtherCAT actions.src/frontend/store/slices/project/slice.ts (1)
1062-1108:⚠️ Potential issue | 🟡 MinorAdd task name collision check before renaming system task.
deleteRemoteDevicecorrectly cleans up associated system tasks, andupdateRemoteDeviceNameupdates the task name before the device rename so thefindby old name still succeeds. However, the pre-check at line 1086 only guards against duplicate device names. If a non-system task already exists with the name produced byethercatTaskName(newName)(e.g.,EtherCAT_Master1), the rename will create a duplicate task name, violating the constraint enforced bycreateTaskat line 605.Consider adding a task name collision check in
updateRemoteDeviceNamebefore updatingsystemTask.name:if (device.protocol === 'ethercat') { const newTaskName = ethercatTaskName(newName) if (slice.project.data.configurations.resource.tasks.some((t) => t.name === newTaskName)) { // Handle collision: skip rename or return error return } // ... existing logic }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/store/slices/project/slice.ts` around lines 1062 - 1108, The rename flow in updateRemoteDeviceName must also guard against task-name collisions: before assigning systemTask.name = ethercatTaskName(newName) (inside the device.protocol === 'ethercat' branch), compute const newTaskName = ethercatTaskName(newName) and check slice.project.data.configurations.resource.tasks.some(t => t.name === newTaskName) (excluding the current systemTask if necessary); if a collision exists, abort the operation (return fail('Task name already exists') or otherwise surface an error) rather than performing the rename, ensuring you reference updateRemoteDeviceName, ethercatTaskName, systemTask and slice.project.data.configurations.resource.tasks to locate the logic to change.
🟠 Major comments (22)
.claude/skills/sync-shared-surfaces/SKILL.md-81-87 (1)
81-87:⚠️ Potential issue | 🟠 MajorCompare local
HEADblobs, notorigin/<branch>, for the CRLF check.The playbook hashes the working trees being synced, but this caveat checks
origin/<branch>. A clean local branch can still be ahead of origin, so this can falsely report “already in sync” and skip a real shared-surface drift.Proposed fix
-git -C <editor> ls-tree origin/<branch> -- <path> -git -C <web> ls-tree origin/<branch> -- <path> +git -C <editor> ls-tree HEAD -- <path> +git -C <web> ls-tree HEAD -- <path>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/sync-shared-surfaces/SKILL.md around lines 81 - 87, The CRLF-check logic currently compares blob hashes against origin/<branch> using the git ls-tree commands shown; change those checks to compare against the local HEAD instead (replace origin/<branch> with HEAD in the git -C <editor> ls-tree and git -C <web> ls-tree invocations) so the playbook detects local commits ahead of origin and does not falsely report "already in sync" for functions/steps that compute working-tree blob hashes.src/frontend/components/_features/[workspace]/editor/device/ethercat/components/interface-selector.tsx-161-164 (1)
161-164:⚠️ Potential issue | 🟠 MajorFix the ref callback to return
void.The assignment expression in a ref callback returns the assigned element, but
RefCallbackin React 18 requires the callback to returnvoid. Use a block body to avoid returning the element.Proposed fix
- ref={(el) => (optionRefs.current[index] = el)} + ref={(el) => { + optionRefs.current[index] = el + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/interface-selector.tsx around lines 161 - 164, The ref callback currently returns the assigned element (via expression arrow), which violates React 18's RefCallback signature; change the inline ref from (el) => (optionRefs.current[index] = el) to a block-body callback that performs the assignment and returns void so the function does not return the element — i.e., use (el) => { optionRefs.current[index] = el; } for the ref on the element keyed by option.value (see optionRefs and optionRefs.current usage in the Option rendering inside interface-selector.tsx).src/middleware/shared/providers/types.ts-36-36 (1)
36-36:⚠️ Potential issue | 🟠 MajorOptional
esiport — consumers lack undefined guards and use non-null assertions.The optional
esi?: EsiPortdeclaration is fine, but several consumers bypass type safety by assuming it's always present. Specifically:
use-device-configuration.ts(line 67): UsesesiPort!.loadDeviceFull()without checking ifesiPortis defined; useEffect guards only checkdeviceandenabled, notesiPortesi-upload.tsx(line 92): Usesesi!.parseAndSaveFile()without prior validationesi-repository.tsx(line 35): Usesesi!.deleteRepositoryItem()without prior validationThese should either add explicit undefined checks before calling methods (e.g.,
if (!esiPort) return;oresiPort?.method()) or addesiPortto effect dependencies and the guard condition to properly handle cases where the adapter isn't registered (e.g., web platform).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/shared/providers/types.ts` at line 36, The optional esi?: EsiPort is declared but callers assume it's always present; update consumers to guard against undefined instead of using non-null assertions: in use-device-configuration.ts ensure the effect checks esiPort (add esiPort to the dependency array and include it in the guard) before calling esiPort!.loadDeviceFull(), and in esi-upload.tsx and esi-repository.tsx replace esi!.parseAndSaveFile() and esi!.deleteRepositoryItem() with safe calls (either early return if !esiPort or use optional chaining like esiPort?.parseAndSaveFile()) so methods are only invoked when the EsiPort instance exists. Ensure references to the EsiPort type and the methods loadDeviceFull, parseAndSaveFile, and deleteRepositoryItem are updated accordingly.src/types/PLC/units/task.ts-6-6 (1)
6-6:⚠️ Potential issue | 🟠 MajorFix
triggeringcasing in the Zod schema — it diverges from all actual usage.The schema defines
triggeringasz.enum(['CYCLIC', 'INTERRUPT']), but the shared port type (src/middleware/shared/ports/types.ts:59) and every test and production use throughout the codebase expect'Cyclic' | 'Interrupt'(capitalized). Any data validated against this schema will fail because the allowed values do not match what the rest of the application produces.Update the Zod enum to:
triggering: z.enum(['Cyclic', 'Interrupt']),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/PLC/units/task.ts` at line 6, The Zod schema's triggering enum uses uppercase values that don't match the rest of the app; update the triggering definition in the Zod schema (the triggering field using z.enum) to use capitalized entries 'Cyclic' and 'Interrupt' instead of 'CYCLIC' and 'INTERRUPT' so validation matches the shared port type and existing usages.src/frontend/components/_features/[workspace]/editor/device/ethercat/components/advanced-tab.tsx-47-103 (1)
47-103:⚠️ Potential issue | 🟠 MajorAvoid persisting invalid numeric drafts on every keystroke.
Number(e.target.value)writes0for an empty field and can writeNaN/out-of-range values beforeonBlurclamps. Keep local draft state or clamp/validate before callingonUpdateMasterConfig, so store/task sync never sees invalid master config values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/advanced-tab.tsx around lines 47 - 103, The inputs (InputWithRef instances using masterConfig.cycleTimeUs, taskPriority, watchdogTimeoutCycles and onUpdateMasterConfig) are writing invalid numeric drafts on every keystroke because Number(e.target.value) returns 0/NaN for empty or partial input; change them to manage a local controlled draft state for each field (e.g., cycleTimeDraft, taskPriorityDraft, watchdogTimeoutDraft) that is updated onChange and only call onUpdateMasterConfig from onBlur after parsing, clamping (100–100000 for cycleTimeUs, 1–31 for taskPriority, 1–100 for watchdogTimeoutCycles) and validating the value; alternatively, onChange may update the draft as a string and onBlur perform Number(parse) + clamp before calling onUpdateMasterConfig so store/task sync never receives transient invalid numbers.src/main/modules/ipc/main.ts-1539-1678 (1)
1539-1678:⚠️ Potential issue | 🟠 MajorValidate EtherCAT runtime responses with Zod before returning them.
These handlers parse untrusted runtime responses and then cast to typed objects. A malformed plugin response can still be forwarded as valid data. Add Zod schemas for the EtherCAT status/scan/test/validate/runtime-status payloads and parse the JSON through those schemas. As per coding guidelines,
**/*.{ts,tsx}should “UseZodfor schema validation across the codebase”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/modules/ipc/main.ts` around lines 1539 - 1678, Add Zod validation for all parsed runtime/plugin responses used by handleEtherCATScan, handleEtherCATTest, handleEtherCATValidate, handleEtherCATGetRuntimeStatus (and the EtherCAT status parser earlier) by defining Zod schemas (e.g., EtherCATStatusResponseSchema, EtherCATScanResponseSchema, EtherCATTestResponseSchema, EtherCATValidateResponseSchema, EtherCATRuntimeStatusResponseSchema) and using schema.parse or schema.safeParse inside each makeRuntimeApiPostRequest response callback instead of blind JSON casts; on validation failure throw an Error with the validation message so makeRuntimeApiPostRequest returns a proper error. Ensure you import Zod, place the schemas near these handlers or in a nearby types/validation section, and replace the current inline casts (the lambda callbacks passed to makeRuntimeApiPostRequest in handleEtherCATScan, handleEtherCATTest, handleEtherCATValidate, handleEtherCATGetRuntimeStatus, and the initial status parser) with schema parsing logic.src/frontend/components/_organisms/explorer/project.tsx-330-335 (1)
330-335:⚠️ Potential issue | 🟠 MajorUse a unique tab identity for EtherCAT child devices.
Line 332 passes
child.name, buthandleCreateTabresolves editors byname. Two slaves with the same device name can collide and open/reuse the wrong editor. Includedevice.name/child.idin the tab identity, or switch editor lookup to the uniquepath.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_organisms/explorer/project.tsx` around lines 330 - 335, The tab identity for EtherCAT child devices currently uses child.name which can collide; update the call to handleCreateTab so the tab identity is unique (e.g., include device.name and child.id in the name or supply a separate unique id field) — for example, build the tab name as `${device.name}:${child.id}:${child.name}` or use the path `/devices/remote/${device.name}/devices/${child.id}` as the editor lookup key; ensure the same unique identifier is used wherever tabs are resolved (handleCreateTab and any editor-resolve logic that currently matches by name) and keep the elementType payload (type: 'ethercat-device', busName: device.name, deviceId: child.id) unchanged.src/frontend/components/_features/[workspace]/editor/device/ethercat/components/advanced-tab.tsx-1-7 (1)
1-7:⚠️ Potential issue | 🟠 MajorImport
EtherCATMasterConfigfrom the shared ports surface.This frontend component imports a backend type directly, which breaks the frontend/backend boundary and conflicts with the ports refactor. Import the named config type from the shared ports module instead. As per coding guidelines,
src/frontend/**/*.{ts,tsx}should “Never import backend or Electron APIs directly in frontend code; use port interfaces through dependency injection instead”.🏗️ Proposed fix
-import type { EtherCATMasterConfig } from '@root/backend/shared/types/PLC/open-plc' +import type { EtherCATMasterConfig } from '@root/middleware/shared/ports'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/advanced-tab.tsx around lines 1 - 7, The component imports the backend type EtherCATMasterConfig directly; update the import so the frontend consumes the shared ports surface instead of backend APIs—replace the current import of EtherCATMasterConfig with the equivalent named export from the shared ports module and keep AdvancedTabProps and the onUpdateMasterConfig signature unchanged so the component uses the ports-facing type.src/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-channels-table.tsx-58-65 (1)
58-65:⚠️ Potential issue | 🟠 MajorPass filtered channel IDs to “select all”.
allSelectedis scoped tofilteredChannels, butonChannelSelectAll(!allSelected)gives the parent no way to apply the action to only the visible rows. This can select/deselect hidden channels when a direction/search filter is active.Proposed fix
type ESIChannelsTableProps = { channels: ESIChannel[] onChannelSelect?: (channelId: string, selected: boolean) => void - onChannelSelectAll?: (selected: boolean) => void + onChannelSelectAll?: (channelIds: string[], selected: boolean) => void selectedChannels?: Set<string> showSelection?: boolean } @@ const handleSelectAll = () => { if (onChannelSelectAll) { - onChannelSelectAll(!allSelected) + onChannelSelectAll( + filteredChannels.map((channel) => channel.id), + !allSelected, + ) } }Also applies to: 131-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/esi-channels-table.tsx around lines 58 - 65, The select-all handler currently toggles selection state with onChannelSelectAll(!allSelected) which doesn't limit the operation to visible/filtered rows; change the call to pass the IDs of the visible channels so the parent can apply the action only to those rows (e.g. call onChannelSelectAll(!allSelected, filteredChannels.map(c => c.id)) in handleSelectAll), and update the other similar invocation in this component (the duplicate select-all handler around the second block) to the same signature so both use filtered channel IDs when toggling selection.src/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-upload.tsx-39-108 (1)
39-108:⚠️ Potential issue | 🟠 MajorGuard against overlapping uploads.
processFilescan start twice before the disabled UI state takes effect; both runs append to the same capturedrepository, so the later completion can drop items from the earlier one. Add a synchronous ref guard around the processing loop.Proposed fix
const ESIUpload = ({ onFilesLoaded, repository, isLoading = false }: ESIUploadProps) => { const esi = useEsi() const [isDragging, setIsDragging] = useState(false) @@ }) const fileInputRef = useRef<HTMLInputElement>(null) + const processingRef = useRef(false) const processFiles = useCallback( async (files: FileList) => { + if (processingRef.current) return + processingRef.current = true + const xmlFiles = Array.from(files).filter((file) => file.name.toLowerCase().endsWith('.xml')) if (xmlFiles.length === 0) { + processingRef.current = false onFilesLoaded(repository, [{ filename: '', error: 'No XML files found. Please upload .xml ESI files.' }]) return } - setParseProgress({ - active: true, - currentFile: xmlFiles[0].name, - currentFileIndex: 0, - totalFiles: xmlFiles.length, - percentage: 0, - }) + try { + setParseProgress({ + active: true, + currentFile: xmlFiles[0].name, + currentFileIndex: 0, + totalFiles: xmlFiles.length, + percentage: 0, + }) - const newItems: ESIRepositoryItemLight[] = [] - const errors: Array<{ filename: string; error: string }> = [] + const newItems: ESIRepositoryItemLight[] = [] + const errors: Array<{ filename: string; error: string }> = [] - const MAX_FILE_SIZE = 100 * 1024 * 1024 // 100MB + const MAX_FILE_SIZE = 100 * 1024 * 1024 // 100MB - // Process files one at a time to avoid memory issues - for (let i = 0; i < xmlFiles.length; i++) { + // Process files one at a time to avoid memory issues + for (let i = 0; i < xmlFiles.length; i++) { const file = xmlFiles[i] @@ - setParseProgress({ - active: false, - currentFileIndex: 0, - totalFiles: 0, - percentage: 100, - }) + setParseProgress({ + active: false, + currentFileIndex: 0, + totalFiles: 0, + percentage: 100, + }) - onFilesLoaded([...repository, ...newItems], errors.length > 0 ? errors : undefined) + onFilesLoaded([...repository, ...newItems], errors.length > 0 ? errors : undefined) + } finally { + processingRef.current = false + } },Also applies to: 122-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/esi-upload.tsx around lines 39 - 108, Add a synchronous guard to prevent overlapping runs of processFiles: create a ref (e.g., processingRef via useRef(false)), check at the top of processFiles and return immediately if processingRef.current is true, then set processingRef.current = true before processing and reset it to false in a finally block; also capture the repository into a local snapshot (e.g., const repositorySnapshot = repository) at the start and use that snapshot when calling onFilesLoaded to avoid lost updates due to the closure over the changing repository; reference processFiles, processingRef, repositorySnapshot, parseProgress, esi, and onFilesLoaded when applying the change.src/backend/shared/ethercat/generate-ethercat-config.ts-133-147 (1)
133-147:⚠️ Potential issue | 🟠 MajorFail invalid SDO values instead of writing zero.
A typo like
"0xGG"or"abc"becomes0, which can push an unintended startup SDO value to a slave. Treat invalid values as configuration errors so the build fails safely.Proposed fix
-function parseNumericValue(str: string): number { - if (!str || str.trim() === '') return 0 +function parseNumericValue(str: string, context: string): number { + if (!str || str.trim() === '') { + throw new Error(`Missing SDO value for ${context}`) + } const trimmed = str.trim() @@ if (/^(0x|#x)/i.test(trimmed)) { const hexStr = trimmed.replace(/^#x/i, '0x') const parsed = Number(hexStr) - return isNaN(parsed) ? 0 : parsed + if (Number.isNaN(parsed)) throw new Error(`Invalid SDO value for ${context}: ${str}`) + return parsed } const parsed = Number(trimmed) - return isNaN(parsed) ? 0 : parsed + if (Number.isNaN(parsed)) throw new Error(`Invalid SDO value for ${context}: ${str}`) + return parsed } @@ index: entry.index, subindex: entry.subIndex, - value: parseNumericValue(entry.value), + value: parseNumericValue(entry.value, `${entry.index}:${entry.subIndex}`), data_type: entry.dataType,Also applies to: 206-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/shared/ethercat/generate-ethercat-config.ts` around lines 133 - 147, The current parseNumericValue function swallows parse errors and returns 0 for invalid inputs; change it to throw a descriptive Error for any non-empty string that cannot be parsed (including bad hex like "0xGG" or non-numeric "abc"), while keeping the current behavior of returning 0 for null/empty/whitespace; update the hex-path and decimal-path checks in parseNumericValue to throw when isNaN(parsed) and include the original input in the error message so the build fails with a clear configuration error; apply the same change to the duplicate parsing implementation present later (the other parseNumericValue-like block referenced around lines 206-215) to ensure consistent failure semantics.src/backend/shared/ethercat/generate-ethercat-config.ts-182-197 (1)
182-197:⚠️ Potential issue | 🟠 MajorDo not emit runtime channels with empty IEC locations.
When a channel has no mapping, this currently writes
iec_location: ''. That produces a runtime config entry the plugin cannot bind reliably. Filter to enabled/mapped channels or fail generation before emitting invalid config.Proposed fix
function buildChannels( channelInfo: PersistedChannelInfo[], - channelMappings: { channelId: string; iecLocation: string }[], + channelMappings: { channelId: string; iecLocation: string; enabled?: boolean }[], ): RuntimeChannel[] { - const mappingMap = new Map(channelMappings.map((m) => [m.channelId, m.iecLocation])) + const mappingMap = new Map( + channelMappings + .filter((m) => m.enabled !== false && m.iecLocation.trim() !== '') + .map((m) => [m.channelId, m.iecLocation]), + ) - return channelInfo.map((ch, index) => ({ - index, - name: ch.name, - type: deriveChannelType(ch.direction, ch.bitLen), - bit_length: ch.bitLen, - iec_location: mappingMap.get(ch.channelId) ?? '', - pdo_index: ch.pdoIndex, - pdo_entry_index: ch.entryIndex, - pdo_entry_subindex: hexToInt(ch.entrySubIndex), - })) + let runtimeIndex = 0 + return channelInfo.flatMap((ch) => { + const iecLocation = mappingMap.get(ch.channelId) + if (!iecLocation) return [] + + return [ + { + index: runtimeIndex++, + name: ch.name, + type: deriveChannelType(ch.direction, ch.bitLen), + bit_length: ch.bitLen, + iec_location: iecLocation, + pdo_index: ch.pdoIndex, + pdo_entry_index: ch.entryIndex, + pdo_entry_subindex: hexToInt(ch.entrySubIndex), + }, + ] + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/shared/ethercat/generate-ethercat-config.ts` around lines 182 - 197, The buildChannels function currently emits RuntimeChannel entries with iec_location set to an empty string when a mapping is missing; change buildChannels to either filter out channels that lack a mapping or throw a generation error before emitting any runtime channels with empty IEC locations. Specifically, update the buildChannels function (and its use sites if needed) to consult mappingMap.get(ch.channelId) and only include channels where that value is defined/non-empty (or raise an explicit error mentioning ch.channelId and ch.name), ensuring no RuntimeChannel is produced with iec_location == ''.src/frontend/store/slices/shared/slice.ts-338-361 (1)
338-361:⚠️ Potential issue | 🟠 MajorDo not key EtherCAT child state only by slave display name.
files[slave.name]andupdateFile({ name: oldName, newName })assume global name uniqueness, but EtherCAT slaves commonly share default names and can repeat across buses. That can overwrite file entries on project open and collide tabs/editor models during rename. Use a stable key that includesbusName/deviceId, or enforce global uniqueness before registering/renaming.Minimum guard if keeping name-based identity
rename: (busName, deviceId, newName) => { const state = getState() const remoteDevice = state.project.data.remoteDevices?.find((d) => d.name === busName) if (!remoteDevice) return { ok: false, message: 'Bus not found' } const devices = remoteDevice.ethercatConfig?.devices ?? [] const device = devices.find((d) => d.id === deviceId) if (!device) return { ok: false, message: 'EtherCAT device not found' } const oldName = device.name + const existingFile = state.fileActions.getFile({ name: newName }).file + const duplicateSibling = devices.some((d) => d.id !== deviceId && d.name === newName) + if (oldName !== newName && (existingFile || duplicateSibling)) { + return { ok: false, message: 'EtherCAT device name already exists' } + } + const updatedDevices = devices.map((d) => (d.id === deviceId ? { ...d, name: newName } : d))Also applies to: 633-636
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/store/slices/shared/slice.ts` around lines 338 - 361, The rename handler currently keys per-device state by mutable display name (see rename, getState, remoteDevice, device.id and updateFile calls), which can collide when multiple EtherCAT slaves share the same name; change the file/tab/editor keying to use a stable identifier that includes busName and deviceId (e.g. composeKey(busName, deviceId)) instead of just device.name, and update all places that read/write files/tabs/editors (including updateFile({ name: oldName, newName }), state.editorActions.updateEditorName and state.tabsActions.updateTabName) to accept and use that composite key (or to accept both {key, name} so the human name still updates). Ensure newKey is derived from busName+deviceId when renaming so save-state tracking and lookups no longer depend on globally unique display names.src/frontend/components/_molecules/project-tree/index.tsx-241-292 (1)
241-292:⚠️ Potential issue | 🟠 MajorNarrow
ProjectTreeExpandableLeafto'remoteDevice'only.The type
leafLang: IProjectTreeLeafProps['leafLang']permits'ethercatDevice', but the implementation only callsrenameRemoteDeviceanddeleteRemoteDeviceRequestfromremoteDeviceActions. There is no conditional logic to route'ethercatDevice'toethercatDeviceActions(which requiresbusNameanddeviceIdparameters).While current usage in
explorer/project.tsxonly passesleafLang='remoteDevice'to this component, the type signature allows incorrect usage. EtherCAT slave nodes are correctly handled by the non-expandableProjectTreeLeafcomponent instead.Restrict the type to prevent silent failures:
Type narrowing fix
type IProjectTreeExpandableLeafProps = ComponentPropsWithoutRef<'li'> & { - leafLang: IProjectTreeLeafProps['leafLang'] - leafType: WorkspaceProjectTreeLeafType + leafLang: 'remoteDevice' + leafType: Extract<WorkspaceProjectTreeLeafType, 'remote-device'> label?: string children?: ReactNode }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_molecules/project-tree/index.tsx` around lines 241 - 292, The component ProjectTreeExpandableLeaf is typed to accept any IProjectTreeLeafProps['leafLang'] (including 'ethercatDevice') but its logic only calls remoteDeviceActions (renameRemoteDevice, deleteRequest) which will mis-handle non-remoteDevice leaves; change the prop type for leafLang in IProjectTreeExpandableLeafProps to the literal 'remoteDevice' so the component can only be used for remote devices (update the type alias on IProjectTreeExpandableLeafProps and any callers if needed), and confirm usages (e.g., in explorer/project.tsx) pass only 'remoteDevice' to ProjectTreeExpandableLeaf; leave other handling (ethercatDevice) to ProjectTreeLeaf.src/backend/shared/ethercat/device-matcher.ts-15-45 (1)
15-45:⚠️ Potential issue | 🟠 Major
parseHexToNumberdoes not actually reject empty/whitespace IDs — contradicts the docstring.The docstring (Lines 18–19) and the guard at Lines 41–45 are predicated on
parseHexToNumberreturningNaNfor unparseable input. ButNumber("")andNumber(" ")return0, notNaN. If any ESI entry has an emptyvendor.id,type.productCode, ortype.revisionNo, it will silently coerce to0and falsely match a scanned device whose vendor/product/revision happens to be0— which is exactly the failure mode the comment claims to prevent.🛡️ Proposed fix: explicitly reject empty/whitespace input
function parseHexToNumber(hexString: string): number { - const cleaned = hexString.replace(/#x/gi, '0x') - return Number(cleaned) + const cleaned = hexString.trim().replace(/#x/gi, '0x') + if (cleaned === '') return NaN + return Number(cleaned) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/shared/ethercat/device-matcher.ts` around lines 15 - 45, The parseHexToNumber helper currently coerces empty or whitespace strings to 0; update parseHexToNumber so it first trims hexString and if the trimmed string is empty return NaN, then perform the existing replacement (replace /#x/gi with '0x') and finally return Number(cleaned); this ensures getMatchQuality's NaN checks (esiVendorNum, esiProductNum, esiRevisionNum) correctly reject empty/whitespace ESI IDs (refer to parseHexToNumber and getMatchQuality names to locate the change).src/frontend/hooks/use-device-configuration.ts-48-75 (1)
48-75:⚠️ Potential issue | 🟠 MajorReset the full-device load guard when the ESI ref changes.
fullDeviceLoadedRefstaystrueafter the first successful load. If this hook instance receives anotherdevice.esiDeviceRef, the effect runs but immediately returns, leaving stale channels/CoE data from the previous slave. Track the loaded key instead of a boolean.Proposed fix
- const fullDeviceLoadedRef = useRef(false) + const loadedDeviceKeyRef = useRef<string | null>(null)useEffect(() => { - if (!enabled || !device || fullDeviceLoadedRef.current) return + const deviceKey = `${device.esiDeviceRef.repositoryItemId}:${device.esiDeviceRef.deviceIndex}` + if (!enabled || !device || loadedDeviceKeyRef.current === deviceKey) return const loadFullDevice = async () => { setIsLoadingChannels(true) setChannelLoadError(null) @@ const deviceChannels = pdoToChannels(result.device) setChannels(deviceChannels) setCoeObjects(result.device.coeObjects) - fullDeviceLoadedRef.current = true + loadedDeviceKeyRef.current = deviceKeyAlso applies to: 103-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/hooks/use-device-configuration.ts` around lines 48 - 75, fullDeviceLoadedRef being a boolean causes stale data when device.esiDeviceRef changes; change the guard to track the loaded key (e.g., a string from device.esiDeviceRef.repositoryItemId + ':' + device.esiDeviceRef.deviceIndex) instead of true/false, compare that key at the top of the useEffect (and return if it matches), and when a new load succeeds set fullDeviceLoadedRef.current = key; also reset fullDeviceLoadedRef.current to null/empty when device or device.esiDeviceRef becomes undefined so subsequent effects will reload; apply the same key-based approach to the similar check referenced at lines 103-104 (the other useEffect/guard).src/frontend/hooks/use-device-configuration.ts-1-3 (1)
1-3:⚠️ Potential issue | 🟠 MajorMove these EtherCAT helpers to a frontend-safe shared surface.
This frontend hook imports parser/enrichment/defaulting helpers from
@root/backend/.... Those utilities should live in a shared module that is safe for renderer/web consumption, or be accessed through the ESI port.As per coding guidelines, "
src/frontend/**/*.{ts,tsx}: Never import backend or Electron APIs directly in frontend code; use port interfaces through dependency injection instead".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/hooks/use-device-configuration.ts` around lines 1 - 3, The hook use-device-configuration.ts currently imports backend-only helpers (enrichDeviceData, generateDefaultChannelMappings, pdoToChannels, extractDefaultSdoConfigurations); move these utilities into a frontend-safe shared surface (e.g., a new src/shared/ethercat module) or expose them via the ESI port and inject them into the hook; then update use-device-configuration.ts to import the functions from the new shared module or receive them from the injected port (keep the same function names: enrichDeviceData, generateDefaultChannelMappings, pdoToChannels, extractDefaultSdoConfigurations) so frontend code no longer imports from `@root/backend/`*.src/frontend/components/_features/[workspace]/editor/device/ethercat/components/device-configuration-form.tsx-1-1 (1)
1-1:⚠️ Potential issue | 🟠 MajorKeep SDO default extraction out of the frontend/backend boundary.
This frontend component imports a backend module directly. Move
extractDefaultSdoConfigurationsto the shared ports/shared utility surface, or expose it through the ESI port.As per coding guidelines, "
src/frontend/**/*.{ts,tsx}: Never import backend or Electron APIs directly in frontend code; use port interfaces through dependency injection instead".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/device-configuration-form.tsx at line 1, The component device-configuration-form.tsx is importing a backend-only symbol extractDefaultSdoConfigurations; remove that direct backend import and either (A) move extractDefaultSdoConfigurations into the shared frontend/backend utility surface (e.g., a new shared module under src/shared/... and update imports in both frontend and backend to use it), or (B) expose the functionality via the existing ESI/port interface and call it through the injected port (add a method like getDefaultSdoConfigurations to the port interface and implement it in the backend), then update device-configuration-form.tsx to use the shared import or call the injected port method instead of importing the backend symbol directly. Ensure you update all references to extractDefaultSdoConfigurations accordingly and remove any direct backend module imports from frontend files.src/frontend/components/_features/[workspace]/editor/device/ethercat/index.tsx-2-6 (1)
2-6:⚠️ Potential issue | 🟠 MajorMove frontend imports off the backend surface.
This frontend component imports EtherCAT helpers and
EtherCATMasterConfigdirectly from@root/backend/.... Move frontend-safe utilities/types into the shared ports layer, or expose them through injected ports, so the renderer/web bundle does not depend on backend modules.As per coding guidelines, "
src/frontend/**/*.{ts,tsx}: Never import backend or Electron APIs directly in frontend code; use port interfaces through dependency injection instead".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/editor/device/ethercat/index.tsx around lines 2 - 6, This frontend file imports backend-only utilities/types (collectUsedIecAddresses, createDefaultSlaveConfig, getBestMatchQuality, matchDevicesToRepository, enrichDeviceData and EtherCATMasterConfig) which breaks the rule against importing backend/Electron APIs in src/frontend; move or expose frontend-safe equivalents through the shared ports layer or dependency-injection: create frontend-facing port interfaces (e.g. EthercatDevicePort) that declare the needed functions/types, implement those ports in the backend module that currently exports these symbols, and update this component to import only the port interface or injected instance; ensure the port surface exposes the same function names/types so callers (collectUsedIecAddresses, createDefaultSlaveConfig, getBestMatchQuality, matchDevicesToRepository, enrichDeviceData, EtherCATMasterConfig) can be used unchanged by the component while keeping backend modules out of the renderer bundle.src/backend/editor/ethercat/esi-service.ts-423-451 (1)
423-451:⚠️ Potential issue | 🟠 MajorAvoid dropping legacy index entries when appending a new ESI file.
If
existingIndexis v1 or a partial v2 missingdevices, Line 450 returns[], and Line 451 rewrites the index with only the newly uploaded item. Gate this path on migration first, or migrate/load existing entries before appending.Proposed guard
const existingIndex = await this.loadRepositoryIndex(projectPath) + if ( + existingIndex && + (existingIndex.version !== 2 || existingIndex.items.some((i) => i.devices === undefined)) + ) { + return { + success: false, + error: 'Repository index needs migration before adding new ESI files', + } + } + const existingFilenames = new Set(existingIndex?.items.map((i) => i.filename) ?? [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/editor/ethercat/esi-service.ts` around lines 423 - 451, The code is overwriting legacy/v1 or partial v2 index entries when appending a new ESI file because loadLightItemsFromIndex() can return [] for an existingIndex that lacks migrated device data; fix by ensuring migration or full-load before appending: detect when existingIndex (from loadRepositoryIndex(projectPath)) is v1 or has missing `devices` and in that case call the migration/load routine to produce a complete light-item list (or convert existingIndex.items into ESIRepositoryItemLight) rather than using loadLightItemsFromIndex()’s empty result, then pass the merged array to saveRepositoryIndexV2(projectPath, [...currentItems, item]); update logic around existingIndex, loadLightItemsFromIndex, and saveRepositoryIndexV2 to perform the migration/load step when necessary.src/backend/editor/ethercat/esi-service.ts-263-275 (1)
263-275:⚠️ Potential issue | 🟠 MajorUpdate the index before deleting the XML file.
Line 267 removes the XML first. If Line 275 then fails to save
repository.json, the repository still references a missing XML file. Prefer persisting the filtered index first, then deleting the XML; an orphaned XML is safer than a broken index reference.Proposed fix
- // Delete the XML file - const deleteResult = await this.deleteXmlFile(projectPath, itemId) - if (!deleteResult.success) { - return deleteResult - } - - // Update the v2 index without the deleted item const currentItems = await this.loadLightItemsFromIndex(projectPath) const updatedItems = currentItems.filter((i) => i.id !== itemId) - return this.saveRepositoryIndexV2(projectPath, updatedItems) + const saveResult = await this.saveRepositoryIndexV2(projectPath, updatedItems) + if (!saveResult.success) { + return saveResult + } + + return this.deleteXmlFile(projectPath, itemId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/editor/ethercat/esi-service.ts` around lines 263 - 275, In deleteRepositoryItemV2 (inside withIndexLock) persist the updated index before removing the XML: call loadLightItemsFromIndex and then saveRepositoryIndexV2(projectPath, updatedItems) first; if that save fails return the error immediately. Only after a successful save should you call deleteXmlFile(projectPath, itemId). Keep existing return types (ESIServiceResponse) and ensure errors from saveRepositoryIndexV2 or deleteXmlFile are propagated/logged appropriately so the repository index never references a missing XML.src/frontend/components/_features/[workspace]/editor/device/ethercat/index.tsx-398-413 (1)
398-413:⚠️ Potential issue | 🟠 MajorUse a unique slave key for files, models, and tabs.
Identical EtherCAT slaves commonly share the same ESI name. Using
device.name/newDevice.nameforaddFile,removeModel, andremoveTabcan collide and close/remove the wrong slave editor. Includepositionoridin the editor/file key, and keep the display label separate if needed.Example direction
+ const uniqueDeviceName = `${device.name} @${nextPosition}` const newDevice: ConfiguredEtherCATDevice = { id: uuidv4(), position: nextPosition, - name: device.name, + name: uniqueDeviceName, esiDeviceRef: ref,- fileActions.addFile({ name: newDevice.name, type: 'ethercat-device', filePath: deviceName }) + fileActions.addFile({ name: newDevice.name, type: 'ethercat-device', filePath: deviceName })Apply the same uniqueness rule for scan-added devices before they are persisted.
Also applies to: 448-466, 471-484
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/editor/device/ethercat/index.tsx around lines 398 - 413, The newDevices creation uses device.name (bestMatch.esiDevice.name or match.device.name) for UI/file keys which can collide for identical slaves; change the editor/file/tab keys in the code paths that call addFile, removeModel, and removeTab to use a unique key composed of the new device id (uuidv4()) or position (e.g., `${id}:${position}`) instead of the display name, and keep the existing name field as the visible label; apply this uniqueness change for the scan-added device block where newDevices.push(...) is constructed and in the same pattern at the other mentioned spots (the callsites that build file/model/tab keys for scan-added devices and for functions interacting with addFile/removeModel/removeTab) so keys are stable before persistence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3b4cb29c-7f43-4895-957c-0711a237fcdc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (83)
.claude/skills/sync-shared-surfaces/SKILL.mddocs/ethercat-architecture.mdpackage.jsonsrc/App.tsxsrc/backend/editor/compiler/compiler-module.tssrc/backend/editor/ethercat/esi-service.tssrc/backend/editor/ethercat/index.tssrc/backend/shared/ethercat/collect-used-iec-addresses.tssrc/backend/shared/ethercat/device-config-defaults.tssrc/backend/shared/ethercat/device-matcher.tssrc/backend/shared/ethercat/enrich-device-data.tssrc/backend/shared/ethercat/esi-parser-main.tssrc/backend/shared/ethercat/esi-parser.tssrc/backend/shared/ethercat/ethercat-task-helpers.tssrc/backend/shared/ethercat/generate-ethercat-config.tssrc/backend/shared/ethercat/index.tssrc/backend/shared/ethercat/sdo-config-defaults.tssrc/backend/shared/types/PLC/open-plc.tssrc/frontend/components/_atoms/tab/index.tsxsrc/frontend/components/_features/[workspace]/create-element/element-card/index.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/advanced-tab.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/channel-mapping-table.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/configured-device-row.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/configured-devices.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/device-browser-modal.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/device-configuration-form.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/device-detail-panel.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/device-scan-table.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/devices-tab.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/diagnostics-tab.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/discovered-device-table.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-channels-table.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-device-info.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-parse-progress.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-repository-table.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-repository.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-upload.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/global-settings-tab.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/interface-selector.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/repository-tab.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/runtime-status-panel.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/scan-bus-tab.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/sdo-parameters-table.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/ethercat-device-editor.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/index.tsxsrc/frontend/components/_molecules/breadcrumbs/index.tsxsrc/frontend/components/_molecules/project-tree/index.tsxsrc/frontend/components/_molecules/task-table/index.tsxsrc/frontend/components/_organisms/explorer/project.tsxsrc/frontend/components/_organisms/task-editor/index.tsxsrc/frontend/hooks/use-device-configuration.tssrc/frontend/hooks/use-store-selectors.tssrc/frontend/screens/workspace-screen.tsxsrc/frontend/services/save-actions.tssrc/frontend/store/__tests__/shared-slice.test.tssrc/frontend/store/slices/editor/types.tssrc/frontend/store/slices/file/types.tssrc/frontend/store/slices/project/slice.tssrc/frontend/store/slices/project/types.tssrc/frontend/store/slices/shared/slice.tssrc/frontend/store/slices/shared/types.tssrc/frontend/store/slices/shared/utils.tssrc/frontend/store/slices/tabs/types.tssrc/frontend/store/slices/tabs/utils.tssrc/frontend/store/slices/workspace/types.tssrc/frontend/utils/PLC/xml-generator/codesys/instances-xml.tssrc/frontend/utils/PLC/xml-generator/old-editor/instances-xml.tssrc/frontend/utils/parse-resource-configuration-to-string.tssrc/frontend/utils/parse-resource-string-to-configuration.tssrc/main/modules/ipc/main.tssrc/main/modules/ipc/renderer.tssrc/middleware/adapters/editor/esi-adapter.tssrc/middleware/adapters/editor/runtime-adapter.tssrc/middleware/editor-platform.tssrc/middleware/shared/ports/esi-port.tssrc/middleware/shared/ports/esi-types.tssrc/middleware/shared/ports/platform-capabilities.tssrc/middleware/shared/ports/runtime-port.tssrc/middleware/shared/ports/types.tssrc/middleware/shared/providers/platform-context.tsxsrc/middleware/shared/providers/types.tssrc/types/PLC/units/task.tssrc/types/ethercat/index.ts
| @@ -0,0 +1,128 @@ | |||
| import { getBestMatchQuality } from '@root/backend/shared/ethercat/device-matcher' | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether getBestMatchQuality is pure and could be relocated to a frontend-safe shared module
rg -nP --type=ts -C3 'export\s+(function|const)\s+getBestMatchQuality\b'
# Find all frontend files that import from `@root/backend/`...
rg -nP "from '@root/backend/" -g 'src/frontend/**' -lRepository: Autonomy-Logic/openplc-editor
Length of output: 1591
Frontend directly importing runtime function from @root/backend/shared/ethercat/device-matcher.
getBestMatchQuality is a runtime function imported from backend code in a frontend component, violating the architecture boundary. The function is pure and should be exposed through a port interface and consumed via dependency injection, following the guideline: "Never import backend or Electron APIs directly in frontend code; use port interfaces through dependency injection instead" and consuming ports through convenience hooks from @root/middleware/shared/providers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/discovered-device-table.tsx
at line 1, The component discovered-device-table.tsx imports the backend runtime
function getBestMatchQuality directly; instead create (or use) a port that
exposes a pure matching utility (e.g., DeviceMatcherPort with a
getBestMatchQuality method), register/implement that port on the backend side,
then inject/consume it in the frontend via the provided convenience hook from
`@root/middleware/shared/providers` (e.g., useDeviceMatcher or similar) and
replace direct imports of getBestMatchQuality with calls to the injected port
method inside the component; ensure the port interface matches the original
function signature so discovered-device-table.tsx can call the injected
getBestMatchQuality without importing backend code.
Comment on lines
+664
to
+681
| // ===================== ETHERCAT DISCOVERY ===================== | ||
| this.registerHandle('ethercat:get-interfaces', this.handleEtherCATGetInterfaces) | ||
| this.registerHandle('ethercat:get-status', this.handleEtherCATGetStatus) | ||
| this.registerHandle('ethercat:scan', this.handleEtherCATScan) | ||
| this.registerHandle('ethercat:test', this.handleEtherCATTest) | ||
| this.registerHandle('ethercat:validate', this.handleEtherCATValidate) | ||
| this.registerHandle('ethercat:get-runtime-status', this.handleEtherCATGetRuntimeStatus) | ||
|
|
||
| // ===================== ESI REPOSITORY ===================== | ||
| this.registerHandle('esi:load-repository-index', this.handleESILoadRepositoryIndex) | ||
| this.registerHandle('esi:save-xml-file', this.handleESISaveXmlFile) | ||
| this.registerHandle('esi:load-xml-file', this.handleESILoadXmlFile) | ||
| this.registerHandle('esi:delete-xml-file', this.handleESIDeleteXmlFile) | ||
| this.registerHandle('esi:parse-and-save-file', this.handleESIParseAndSaveFile) | ||
| this.registerHandle('esi:clear-repository', this.handleESIClearRepository) | ||
| this.registerHandle('esi:load-device-full', this.handleESILoadDeviceFull) | ||
| this.registerHandle('esi:load-repository-light', this.handleESILoadRepositoryLight) | ||
| this.registerHandle('esi:migrate-repository', this.handleESIMigrateRepository) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check that new EtherCAT/ESI IPC channels registered in main.ts are referenced by renderer wrappers.
rg -n -C2 --glob '*.{ts,tsx}' "'(ethercat|get-interfaces|ethercat:get-status|ethercat:scan|ethercat:test|ethercat:validate|ethercat:get-runtime-status|esi:load-repository-index|esi:save-xml-file|esi:load-xml-file|esi:delete-xml-file|esi:parse-and-save-file|esi:clear-repository|esi:load-device-full|esi:load-repository-light|esi:migrate-repository)'"Repository: Autonomy-Logic/openplc-editor
Length of output: 28244
Add missing renderer wrapper for ethercat:get-interfaces.
The IPC handler is registered in main.ts (line 665) but the renderer wrapper is missing from renderer.ts. All other channels have their async wrappers properly exposed. Add the etherCATGetInterfaces function to renderer.ts to maintain parity between main-process registration and renderer-process invocation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/modules/ipc/main.ts` around lines 664 - 681, The main process
registers the IPC channel 'ethercat:get-interfaces' via registerHandle but
renderer.ts lacks the corresponding wrapper; add an async exported function
named etherCATGetInterfaces in renderer.ts that calls the IPC invoke for
'ethercat:get-interfaces' (matching the pattern used by other wrappers like
etherCATGetStatus/etherCATScan) so renderer code can call the channel
consistently.
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
PLCRemoteDevice.ethercatConfiginto two named interfaces in the shared ports layer:EthercatConfigandEtherCATMasterConfig.Record<string, unknown>:ProjectActions.updateEthercatConfigtakes anEthercatConfig(wasRecord<string, unknown>).as EthercatConfigcast aroundmasterConfigsyncing and theas typeof device.ethercatConfigcast on assignment.@root/backend/shared/types/PLC/open-plcimport ofEthercatConfigfromsrc/frontend/store/slices/project/slice.ts— consumers now get the type transitively via the ports-declared action signature, which keeps the frontend off the backend import path.as unknown as ConfiguredEtherCATDevice[]casts fromconfiguredDevices.Stacking
This PR is stacked on
feat/ethercat-esi-backend(#731) becausedevelopmentdoes not yet carry the EtherCAT base shape that the refactor touches. Once #731 merges intodevelopment, we rebase this branch onto the refresheddevelopment.The web-side companion lives in Autonomy-Logic/openplc-web#373 (based on web
development, which already carries the base shape from #360). The Shared Surface Sync CI on both PRs will remain red until #731 (and its web-side mirror openplc-web#371) land.Test plan
npx tsc --noEmitcleannpx eslinton the 5 refactor files — no new warnings/errorsjestproject-slice suite 239/239 passingdevelopmentand re-run full check🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Improvements