feat: EtherCAT ESI support

[marconetsf]

Summary

Adds full EtherCAT ESI (EtherCAT Slave Information) support to the desktop editor: ESI file upload/parsing, repository management, device configuration with channel mappings, and compilation integration.

Companion PRs

• autonomy-edge — Backend ESI endpoints (Autonomy-Logic/autonomy-edge#173)
• openplc-web — Web adapter + shared surface sync (Autonomy-Logic/openplc-web#360)

What this branch contains

Ports & Middleware

• EsiPort contract (6 methods) in shared ports surface
• esi-types.ts canonical ESI domain types in shared ports surface
• Editor adapter via Electron IPC (12 channels)
• useEsi() hook in platform providers

Backend (main process)

• ESIService — persistence in devices/esi/ per project (v1/v2 index, UUID-based XML storage)
• esi-parser-main.ts — parseESILight + parseESIDeviceFull using fast-xml-parser
• Channel utilities, enrichment, device matching, SDO extraction

Frontend (25 components)

• ESI upload with drag-and-drop + progress bar (100 MB limit)
• Repository table with vendor/device summary
• Device browser modal for adding slaves from repository
• Channel mapping table with IEC address generation
• Device configuration form (addressing, timeouts, watchdog, distributed clocks)
• SDO parameters table
• Scan bus integration with device matching

Store

• EtherCAT editor types in editor/tabs/workspace slices
• Project slice updates for ethercatConfig in remote devices

Test plan

• npm run dev — app starts, EtherCAT tab works
• Upload ESI XML → appears in repository
• Add device from repository → channels shown
• Configure channel mappings → IEC addresses generated
• Delete/clear repository works
• npm test passes
• npm run validate:arch passes

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Full EtherCAT support: network discovery (interfaces, scan, test, validate), runtime status/diagnostics, and automatic runtime config generation.
  • ESI repository UI and workflows: upload, browse, migrate, manage ESI files, and import devices with automatic channel/PD O/SDO enrichment.
  • Editor UI: Bus/Repository/Advanced tabs, per-device editor, channel mapping table, SDO parameter editor, device browser, and interface selector.
  • Project integration: EtherCAT devices in project tree, per-device editors/tabs, and save support for device files.

• Documentation

  • Added comprehensive EtherCAT architecture reference guide.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Added end-to-end EtherCAT support: ESI parsing and repository, main↔renderer IPC and runtime HTTP helpers, editor-side ESI/runtime adapters, frontend bus/device editors and components, device matching/enrichment utilities, runtime JSON generation, and compiler integration to emit conf/ethercat.json.

Changes

Cohort / File(s)	Summary
Documentation docs/ethercat-architecture.md	New architecture reference covering ESI model, repository layout, IPC bridge, renderer flows, enrichment, UI, and compile/runtime integration.
ESI parsing & persistence (main/backend) src/backend/shared/ethercat/esi-parser-main.ts, src/backend/shared/ethercat/esi-parser.ts, src/backend/editor/ethercat/esi-service.ts, src/backend/shared/ethercat/index.ts	Added light/full ESI XML parsers and a persisted project-scoped ESIService with v2 index, safe IO, CRUD, migration, and serialized index locking.
EtherCAT core utilities (shared) src/backend/shared/ethercat/* (device-matcher, enrich-device-data, device-config-defaults, sdo-config-defaults, generate-ethercat-config, ethercat-task-helpers, collect-used-iec-addresses)	New device matching/enrichment, default slave config factory, SDO default extraction, IEC-address collection, task helpers, and generateEthercatConfig producing runtime JSON.
Compiler integration (main/backend) src/backend/editor/compiler/compiler-module.ts	Integrated EtherCAT config generation into compile flow, adjusted runtime detection and v4 config generation order, and added error handling for config generation.
IPC / Main ↔ Renderer bridge src/main/modules/ipc/main.ts, src/main/modules/ipc/renderer.ts	Registered ethercat:* and esi:* IPC handlers, added runtime HTTP helpers, response wrapping, and renderer bridge invoke methods for ESI and EtherCAT operations.
Middleware adapters & platform src/middleware/adapters/editor/esi-adapter.ts, src/middleware/adapters/editor/runtime-adapter.ts, src/middleware/editor-platform.ts, src/middleware/shared/providers/platform-context.tsx	Added ESI adapter, extended runtime adapter with EtherCAT methods, project-path setter, and useEsi() hook; wired esi port into editorPorts.
Types & ports src/types/ethercat/*, src/middleware/shared/ports/esi-types.ts, src/middleware/shared/ports/esi-port.ts, src/backend/shared/types/PLC/open-plc.ts, src/middleware/shared/ports/runtime-port.ts, src/middleware/shared/ports/types.ts, src/middleware/shared/ports/platform-capabilities.ts, src/types/PLC/units/task.ts	Added EtherCAT/ESI domain types, ESI port interface, extended runtime/middleware port types, PLCTask Zod schema, and hasEthercat platform capability.
Frontend EtherCAT editor & components src/frontend/components/_features/.../ethercat/*, src/frontend/components/_features/[workspace]/editor/device/ethercat/index.tsx, src/frontend/components/_features/[workspace]/editor/device/ethercat/ethercat-device-editor.tsx	New bus editor and ~18 UI components (scan/diagnostics/repository/device browser, device detail/forms, SDO table, channel mappings, runtime status, etc.).
Frontend store & workspace integration src/frontend/store/slices/*, src/frontend/hooks/use-device-configuration.ts, src/frontend/hooks/use-store-selectors.ts	Added ethercat-device editor/file/tab types, updateEthercatConfig action with system-task sync, ethercatDeviceActions (rename/delete), per-slave file/tab registration, and useDeviceConfiguration hook.
Project tree, breadcrumbs, screen wiring src/frontend/components/_molecules/project-tree/index.tsx, src/frontend/components/_molecules/breadcrumbs/index.tsx, src/frontend/screens/workspace-screen.tsx, src/App.tsx	Integrated EtherCAT nodes and expandable leaves into project tree/breadcrumbs, protocol-based editor routing, and synced project path to middleware.
Save/serialization & XML utilities src/frontend/services/save-actions.ts, src/frontend/utils/PLC/xml-generator/*, src/frontend/utils/parse-resource-*	Added ethercat-device save branch, sorted tasks by priority for XML generation, and adjusted resource parse defaults.
Minor UI/type updates & misc src/frontend/components/_atoms/tab/index.tsx, src/frontend/components/_features/[workspace]/create-element/element-card/index.tsx, src/frontend/components/_molecules/task-table/index.tsx, other small files, package.json	Enabled EtherCAT protocol in creation dialog, added tab icon/type, made task-table cells non-editable for system tasks, and added fast-xml-parser dependency.

Sequence Diagram(s)

sequenceDiagram
    participant UI as Renderer (UI)
    participant Bridge as IPC Bridge
    participant Main as Main Process
    participant Runtime as EtherCAT Runtime
    participant FS as File System

    UI->>Bridge: etherCATScan(interface)
    Bridge->>Main: handleEtherCATScan
    Main->>Runtime: HTTP POST /scan {interface}
    Runtime-->>Main: scan response
    Main-->>Bridge: scan result
    Bridge-->>UI: scan result

    UI->>UI: matchDevicesToRepository(devices)
    UI->>Bridge: esiLoadDeviceFull(projectPath,itemId,idx)
    Bridge->>Main: handleESILoadDeviceFull
    Main->>FS: read XML file
    Main->>Main: parseESIDeviceFull(xml)
    Main-->>Bridge: ESIDevice
    Bridge-->>UI: ESIDevice

    UI->>UI: enrichDeviceData(...) & update project store
    UI->>Bridge: request compile
    Bridge->>Main: compile request
    Main->>Main: generateEthercatConfig(...)
    Main->>FS: write conf/ethercat.json

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• feat: add EtherCAT SDO/CoE support and device configuration UI #704 — Strong overlap: ESI parsing/service, IPC handlers, runtime discovery APIs, types, and frontend EtherCAT components.
• feat: OPC-UA Server Configuration #546 — Related: integrates runtime config generation into the compiler flow (similar compile-time config hooks).
• Integration with the OpenPLC Runtime #351 — Related: modifies compiler-module and IPC/runtime request handling in adjacent areas.

Suggested labels

feature

Suggested reviewers

• JoaoGSP

Poem

🐰 I hopped through XML fields by lamplight,
I matched each PDO and set addresses right.
I parsed and saved each vendor’s song,
Wrote conf JSON tidy and strong.
Now EtherCAT hums — go cycle on!

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/ethercat-esi-backend

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (3)

src/backend/editor/ethercat/esi-service.ts (3)

275-311: Unused _existingItems parameter — remove or honor it.

Both saveRepositoryItem and deleteRepositoryItem accept _existingItems: ESIRepositoryItem[] but deliberately ignore it (comment says "Read current index from disk instead of trusting caller snapshot"). Keeping an unused parameter in a public API is a footgun for callers. Either drop the parameter from the signature or document the intent in the JSDoc.

🤖 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 275 - 311, The
_existingItems parameter is unused in saveRepositoryItem (and its sibling
deleteRepositoryItem); remove the unused parameter from the public method
signatures (remove _existingItems: ESIRepositoryItem[] from saveRepositoryItem
and deleteRepositoryItem), update any interface/type declarations that declare
these methods (e.g., ESIService or related service interface), and update all
callers/tests to stop passing that argument; alternatively, if you want to
preserve the API, clearly document the parameter in the JSDoc and use a leading
underscore convention consistently, but prefer removing the parameter to avoid
the footgun.

---

79-84: withIndexLock leaks pending locks if fn is synchronous-throw-free but the preceding lock rejects.

current.then(fn) does not invoke fn when current rejects, but your .finally(resolve!) still runs and releases this.indexLock. That's fine. However, the returned promise from withIndexLock then rejects with the previous caller's error, not this caller's work — callers will see errors unrelated to their operation. Recommend isolating the chain so each caller only sees its own outcome:

🛠️ Suggested refactor

-  private withIndexLock<T>(fn: () => Promise<T>): Promise<T> {
-    const current = this.indexLock
-    let resolve: () => void
-    this.indexLock = new Promise<void>((r) => (resolve = r))
-    return current.then(fn).finally(() => resolve!())
-  }
+  private withIndexLock<T>(fn: () => Promise<T>): Promise<T> {
+    const previous = this.indexLock
+    let release!: () => void
+    this.indexLock = new Promise<void>((r) => (release = r))
+    // Wait for previous lock regardless of outcome, then run fn.
+    const run = previous.catch(() => undefined).then(fn)
+    run.finally(() => release())
+    return run
+  }

🤖 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 79 - 84,
withIndexLock currently uses current.then(fn) so if the previous lock (current)
rejects, fn is never invoked and the returned promise rejects with the previous
error; isolate each caller by always invoking fn regardless of current's
outcome. Change the chaining so you await current but swallow its rejection
before calling fn (e.g. use current.then(() => fn(), () => fn()) or
current.catch(() => {}).then(() => fn())), keep setting this.indexLock to a new
Promise whose resolver you call in finally to release the lock, and ensure
resolve is assigned before any possible early returns; refer to withIndexLock,
this.indexLock and current in your changes.

---

1-11: Use @root/* alias for internal imports.

The relative paths ../../shared/ethercat/esi-parser-main and ../utils should use the @root/* alias per repository guidelines.

🛠️ Suggested change

-import { parseESILight } from '../../shared/ethercat/esi-parser-main'
-import { fileOrDirectoryExists } from '../utils'
+import { parseESILight } from '@root/backend/shared/ethercat/esi-parser-main'
+import { fileOrDirectoryExists } from '@root/backend/editor/utils'

🤖 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 1 - 11, Replace the
two relative imports that violate the internal-alias policy: change the import
of parseESILight from '../../shared/ethercat/esi-parser-main' and the import of
fileOrDirectoryExists from '../utils' to use the `@root/`* alias form used
elsewhere in the repo (e.g. import parseESILight from
'@root/.../ethercat/esi-parser-main' and import { fileOrDirectoryExists } from
'@root/.../utils') so that the symbols parseESILight and fileOrDirectoryExists
are imported via the `@root` alias rather than relative paths.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/backend/editor/ethercat/esi-service.ts`:
- Around line 485-522: The duplicate-filename branch in the function using
withIndexLock returns { success: true } with no item which breaks callers;
change the duplicate handling in that function so it looks up the existing item
(use loadLightItemsFromIndex or loadRepositoryIndex to find the matching
ESIRepositoryItemLight) and return either { success: true, item: existingItem }
or explicitly { success: true, skipped: true, item: existingItem } so callers
can distinguish saved vs skipped; additionally, improve dedupe logic by
computing a content hash (sha256) or using a composite key
(vendor+product+revision from parseResult) when saving and checking duplicates
(update the duplicate check before calling saveXmlFile and ensure
saveRepositoryIndexV2 persists the chosen dedupe key).
- Around line 126-140: The JSON parse failure in loadRepositoryIndex currently
re-throws and can bubble out of withIndexLock causing unhandled rejections;
change loadRepositoryIndex to detect JSON parse errors and throw a distinct,
recoverable error type (e.g., RepositoryCorruptError) or return a recoverable
result object, then update callers that call loadRepositoryIndex inside
withIndexLock (notably saveRepositoryItem and deleteRepositoryItem) to catch
that specific error and surface a recoverable response (or call a new
repairRepository() entry point) instead of letting it escape; reference
loadRepositoryIndex, saveRepositoryItem, deleteRepositoryItem, withIndexLock and
implement either the new RepositoryCorruptError class or a repairRepository()
method and the corresponding try/catch in those callers to avoid permanently
bricking the repository UI.
- Around line 370-416: loadRepositoryLight currently detects a v2 index by only
checking index.items[0].devices which can hide partial/corrupt v2 data and
causes loadLightItemsFromIndex to silently drop items via filter((i) =>
i.devices). Update loadRepositoryLight to inspect all index.items (e.g., use
Array.every or Array.some) to ensure every item contains devices before treating
the index as v2; if any item is missing devices, return needsMigration: true (or
at minimum log/warn about dropped items) and avoid calling
loadLightItemsFromIndex, and keep loadLightItemsFromIndex unchanged except for
optional logging when items are filtered out.
- Around line 275-340: The bug is that saveRepositoryItem and
deleteRepositoryItem always rewrite the on-disk index in a v1 shape (dropping
devices) even when currentIndex.version === 2, causing v2 indices to lose their
inline devices; update both methods to detect currentIndex.version === 2 and
handle v2 writes: when version === 2, preserve the existing items' devices field
(copy devices from currentItems for untouched entries) and include a proper
devices array for the saved item (use item.devices) before JSON.stringify;
similarly, in deleteRepositoryItem when writing back for version === 2, write
the filtered items with their original devices preserved (copy from
currentItems) and keep version === 2 when calling promises.writeFile; reference
functions/methods: saveRepositoryItem, deleteRepositoryItem,
loadRepositoryIndex, parseAndSaveFile/deleteRepositoryItemV2 for v2 pattern.

---

Nitpick comments:
In `@src/backend/editor/ethercat/esi-service.ts`:
- Around line 275-311: The _existingItems parameter is unused in
saveRepositoryItem (and its sibling deleteRepositoryItem); remove the unused
parameter from the public method signatures (remove _existingItems:
ESIRepositoryItem[] from saveRepositoryItem and deleteRepositoryItem), update
any interface/type declarations that declare these methods (e.g., ESIService or
related service interface), and update all callers/tests to stop passing that
argument; alternatively, if you want to preserve the API, clearly document the
parameter in the JSDoc and use a leading underscore convention consistently, but
prefer removing the parameter to avoid the footgun.
- Around line 79-84: withIndexLock currently uses current.then(fn) so if the
previous lock (current) rejects, fn is never invoked and the returned promise
rejects with the previous error; isolate each caller by always invoking fn
regardless of current's outcome. Change the chaining so you await current but
swallow its rejection before calling fn (e.g. use current.then(() => fn(), () =>
fn()) or current.catch(() => {}).then(() => fn())), keep setting this.indexLock
to a new Promise whose resolver you call in finally to release the lock, and
ensure resolve is assigned before any possible early returns; refer to
withIndexLock, this.indexLock and current in your changes.
- Around line 1-11: Replace the two relative imports that violate the
internal-alias policy: change the import of parseESILight from
'../../shared/ethercat/esi-parser-main' and the import of fileOrDirectoryExists
from '../utils' to use the `@root/`* alias form used elsewhere in the repo (e.g.
import parseESILight from '@root/.../ethercat/esi-parser-main' and import {
fileOrDirectoryExists } from '@root/.../utils') so that the symbols
parseESILight and fileOrDirectoryExists are imported via the `@root` alias rather
than relative paths.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: dbc684f3-afae-4152-83b2-cfa4b5516127

📥 Commits

Reviewing files that changed from the base of the PR and between 6639776 and 8446a1f.

📒 Files selected for processing (2)

• src/backend/editor/ethercat/esi-service.ts
• src/middleware/shared/ports/esi-types.ts

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find callers to confirm whether saveRepositoryItem/deleteRepositoryItem are invoked when on-disk index is v2.
rg -nP -C3 '\b(saveRepositoryItem|deleteRepositoryItem)\s*\('

Repository: Autonomy-Logic/openplc-editor

Length of output: 4131

---

🏁 Script executed:

# First, let's check the full ESI service file to understand the index structure and v2 variants
wc -l src/backend/editor/ethercat/esi-service.ts

Repository: Autonomy-Logic/openplc-editor

Length of output: 119

---

🏁 Script executed:

# Let's look at the saveRepositoryIndexV2 and deleteRepositoryItemV2 methods mentioned in the review
rg -n "saveRepositoryIndexV2|deleteRepositoryItemV2|v2|devices" src/backend/editor/ethercat/esi-service.ts | head -50

Repository: Autonomy-Logic/openplc-editor

Length of output: 1967

---

🏁 Script executed:

# Let's examine the ESIRepositoryItem interface to understand its fields
rg -n "interface ESIRepositoryItem|type ESIRepositoryItem" -A 15

Repository: Autonomy-Logic/openplc-editor

Length of output: 2407

---

🏁 Script executed:

# Check for the loadLightItemsFromIndex and loadRepositoryLight methods to understand the impact
rg -n "loadLightItemsFromIndex|loadRepositoryLight" -A 10

Repository: Autonomy-Logic/openplc-editor

Length of output: 11438

---

🏁 Script executed:

# Check parseAndSaveFile to see if it routes based on index version
rg -n "parseAndSaveFile" -A 30 src/backend/editor/ethercat/esi-service.ts | head -60

Repository: Autonomy-Logic/openplc-editor

Length of output: 1430

---

🏁 Script executed:

# Check the full context around parseAndSaveFile to see v2 routing
sed -n '460,530p' src/backend/editor/ethercat/esi-service.ts

Repository: Autonomy-Logic/openplc-editor

Length of output: 2500

---

🏁 Script executed:

# Check esi-repository.tsx to see when saveRepositoryItem is invoked
rg -n "saveRepositoryItem" -B 5 -A 5 src/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-repository.tsx

Repository: Autonomy-Logic/openplc-editor

Length of output: 55

---

🏁 Script executed:

# Find the esi-repository file more broadly
fd -i "esi-repository" src/frontend

Repository: Autonomy-Logic/openplc-editor

Length of output: 276

---

🏁 Script executed:

# Search for where saveRepositoryItem is called from frontend code
rg -n "saveRepositoryItem" src/frontend -B 3 -A 3

Repository: Autonomy-Logic/openplc-editor

Length of output: 55

---

🏁 Script executed:

# Search for ESI save operations in frontend
rg "esi\." src/frontend -n | grep -i "save\|delete" | head -20

Repository: Autonomy-Logic/openplc-editor

Length of output: 55

---

🏁 Script executed:

# Check the IPC main.ts to understand the full flow of saveRepositoryItem
sed -n '1695,1710p' src/main/modules/ipc/main.ts

Repository: Autonomy-Logic/openplc-editor

Length of output: 677

---

V2 index corruption in saveRepositoryItem and deleteRepositoryItem — critical data loss bug.

Both methods preserve version: currentIndex?.version ?? 1 but rebuild/filter items without the devices field. When an on-disk index is v2, this strips the inline device summaries from all untouched items (and corrupts the saved item in saveRepositoryItem). After either operation, loadRepositoryLight → loadLightItemsFromIndex filters on i.devices (line 374) and silently drops all affected items, forcing re-parse of every XML on next load.

The root cause: saveRepositoryItem and deleteRepositoryItem do not detect the index version. They reconstruct items in a v1-compatible format regardless of index version. Since deleteRepositoryItemV2 and parseAndSaveFile already demonstrate the correct v2 pattern, these methods should either:

1. Detect currentIndex.version === 2 and route to v2-specific logic, or
2. Preserve the devices field when writing back to v2 indices.

Minimal fix for saveRepositoryItem: Detect v2 index and either call the v2 logic or preserve devices from currentItems. For deleteRepositoryItem, the filter operation itself is safe, but the subsequent write should also respect v2 structure.

🤖 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 275 - 340, The bug
is that saveRepositoryItem and deleteRepositoryItem always rewrite the on-disk
index in a v1 shape (dropping devices) even when currentIndex.version === 2,
causing v2 indices to lose their inline devices; update both methods to detect
currentIndex.version === 2 and handle v2 writes: when version === 2, preserve
the existing items' devices field (copy devices from currentItems for untouched
entries) and include a proper devices array for the saved item (use
item.devices) before JSON.stringify; similarly, in deleteRepositoryItem when
writing back for version === 2, write the filtered items with their original
devices preserved (copy from currentItems) and keep version === 2 when calling
promises.writeFile; reference functions/methods: saveRepositoryItem,
deleteRepositoryItem, loadRepositoryIndex,
parseAndSaveFile/deleteRepositoryItemV2 for v2 pattern.

[coderabbitai]

♻️ Duplicate comments (2)

src/frontend/store/slices/shared/slice.ts (2)

625-630: ⚠️ Potential issue | 🟠 Major

Unstable file key — slave.name still collides across slaves.

Registering files[slave.name] means two slaves sharing a display name (easy to hit when adding two of the same device type, e.g. two EL1008's, on the same or different buses) will overwrite each other's save-state entry, and the resulting tab/editor keys collide too. Matching cleanup in ethercatDeviceActions.delete (L328) and rename also operate on device.name, so the collision cascades through all three slices. Recommend a composite identity (e.g. ${d.name}:${slave.id}) propagated consistently through the file/tab/editor APIs, with slave.name reserved for display only.

🤖 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 625 - 630, The
current registration uses unstable files[slave.name] which collides for
identical display names; change the key to a stable composite id like
`${d.name}:${slave.id}` when creating the entry in the slice (replace
files[slave.name] with files[`${d.name}:${slave.id}`]) and propagate that
composite id through the related APIs (update ethercatDeviceActions.delete and
any rename handling that currently uses device.name/device.name-based lookups to
use the composite id or slave.id+bus name instead); keep slave.name only for UI
display and ensure all file/tab/editor lookups and cleanups use the new
composite identifier.

---

338-361: ⚠️ Potential issue | 🟠 Major

ethercatDeviceActions.rename is missing two things: duplicate-name check and fileActions.updateFile.

1. Unlike pouActions.rename / datatypeActions.rename, this action doesn't check whether newName is already used by another slave on the same bus. Because slave files/tabs/editors are keyed by slave.name (see Line 628 and L322-323/357-358 here), renaming to a colliding name will silently overwrite the other slave's file/tab/editor entries.
2. The file registry is never updated on rename. state.editorActions.updateEditorName and state.tabsActions.updateTabName are called, but state.fileActions.updateFile({ name: oldName, newName }) is not — so files[oldName] lingers and files[newName] is missing, breaking save-state tracking and later close/save flows for the renamed device. The sibling delete already calls fileActions.removeFile, so rename should mirror that.

🛠️ Proposed fix

     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' }
 
+      if (devices.some((d) => d.id !== deviceId && d.name === newName)) {
+        return { ok: false, message: 'EtherCAT device name already exists' }
+      }
+
       const oldName = device.name
       const updatedDevices = devices.map((d) => (d.id === deviceId ? { ...d, name: newName } : d))
       state.projectActions.updateEthercatConfig(busName, {
         masterConfig: remoteDevice.ethercatConfig?.masterConfig ?? {
           networkInterface: 'eth0',
           cycleTimeUs: 1000,
           watchdogTimeoutCycles: 3,
         },
         devices: updatedDevices,
       })
       state.editorActions.updateEditorName(oldName, newName)
+      state.fileActions.updateFile({ name: oldName, newName })
       state.tabsActions.updateTabName(oldName, newName)
 
       return { ok: true }
     },

🤖 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,
ethercatDeviceActions.rename must prevent name collisions and update the file
registry: before applying the update in rename, check the bus's devices
(remoteDevice.ethercatConfig.devices) for any device with name === newName and
id !== deviceId and return { ok: false, message: 'Name already in use' } if
found; after calling state.projectActions.updateEthercatConfig and before
returning, call state.fileActions.updateFile({ name: oldName, newName }) in
addition to the existing state.editorActions.updateEditorName and
state.tabsActions.updateTabName so the files map is moved to the new key and
save/close flows continue to work.

🧹 Nitpick comments (2)

src/frontend/store/slices/shared/slice.ts (1)

304-362: DRY: factor out the masterConfig default.

The hardcoded { networkInterface: 'eth0', cycleTimeUs: 1000, watchdogTimeoutCycles: 3 } fallback is duplicated between delete (L315-319) and rename (L350-354), and presumably exists elsewhere in the EtherCAT code where ethercatConfig is first created. If those defaults ever drift (e.g. cycle time bumped to 500µs), they'll silently diverge here. Consider extracting a DEFAULT_ETHERCAT_MASTER_CONFIG constant (or a helper ensureMasterConfig(existing)) in the EtherCAT types/utils and reusing it in both call sites.

Also worth asking: should these code paths ever actually materialize a default master config? If remoteDevice.ethercatConfig exists, masterConfig should too — a missing masterConfig may be a bug upstream that's being papered over here.

🤖 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 304 - 362, The
duplicated inline master config fallback in ethercatDeviceActions.delete and
ethercatDeviceActions.rename should be factored out: create a shared constant
DEFAULT_ETHERCAT_MASTER_CONFIG (or a helper ensureMasterConfig(existing) that
returns existing.masterConfig || DEFAULT_ETHERCAT_MASTER_CONFIG) in the EtherCAT
types/utils module, export it, and replace the duplicated object literals in
both delete and rename with that constant/helper (update calls to
state.projectActions.updateEthercatConfig to pass masterConfig from the new
constant/helper). Also review whether you should assert presence of masterConfig
instead of silently defaulting and adjust callers accordingly.

src/frontend/store/__tests__/shared-slice.test.ts (1)

711-744: Consider also asserting editor-model cleanup.

The new cascade test covers project.data.remoteDevices, files, and tabs, which is great. Since ethercatDeviceActions.delete also calls state.editorActions.removeModel(deviceName) and conditionally clearEditor(), consider adding an assertion on state.editors (e.g. expect(state.editors.some((m) => m.meta.name === 'EK1100' || m.meta.name === 'EL1008')).toBe(false)) to lock in that behavior — otherwise a future regression that drops removeModel would still pass this test.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/frontend/store/__tests__/shared-slice.test.ts` around lines 711 - 744,
Add an assertion that editor models for the EtherCAT children are removed: after
calling store.getState().remoteDeviceActions.delete('eth'), check the editor
state (state.editors) to ensure no model with meta.name 'EK1100' or 'EL1008'
remains (e.g. assert state.editors.some(m => m.meta.name === 'EK1100' ||
m.meta.name === 'EL1008') is false). This ties the test to the
editorActions.removeModel/clearEditor behavior invoked by the deletion path.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/frontend/store/slices/shared/slice.ts`:
- Around line 625-630: The current registration uses unstable files[slave.name]
which collides for identical display names; change the key to a stable composite
id like `${d.name}:${slave.id}` when creating the entry in the slice (replace
files[slave.name] with files[`${d.name}:${slave.id}`]) and propagate that
composite id through the related APIs (update ethercatDeviceActions.delete and
any rename handling that currently uses device.name/device.name-based lookups to
use the composite id or slave.id+bus name instead); keep slave.name only for UI
display and ensure all file/tab/editor lookups and cleanups use the new
composite identifier.
- Around line 338-361: ethercatDeviceActions.rename must prevent name collisions
and update the file registry: before applying the update in rename, check the
bus's devices (remoteDevice.ethercatConfig.devices) for any device with name ===
newName and id !== deviceId and return { ok: false, message: 'Name already in
use' } if found; after calling state.projectActions.updateEthercatConfig and
before returning, call state.fileActions.updateFile({ name: oldName, newName })
in addition to the existing state.editorActions.updateEditorName and
state.tabsActions.updateTabName so the files map is moved to the new key and
save/close flows continue to work.

---

Nitpick comments:
In `@src/frontend/store/__tests__/shared-slice.test.ts`:
- Around line 711-744: Add an assertion that editor models for the EtherCAT
children are removed: after calling
store.getState().remoteDeviceActions.delete('eth'), check the editor state
(state.editors) to ensure no model with meta.name 'EK1100' or 'EL1008' remains
(e.g. assert state.editors.some(m => m.meta.name === 'EK1100' || m.meta.name ===
'EL1008') is false). This ties the test to the
editorActions.removeModel/clearEditor behavior invoked by the deletion path.

In `@src/frontend/store/slices/shared/slice.ts`:
- Around line 304-362: The duplicated inline master config fallback in
ethercatDeviceActions.delete and ethercatDeviceActions.rename should be factored
out: create a shared constant DEFAULT_ETHERCAT_MASTER_CONFIG (or a helper
ensureMasterConfig(existing) that returns existing.masterConfig ||
DEFAULT_ETHERCAT_MASTER_CONFIG) in the EtherCAT types/utils module, export it,
and replace the duplicated object literals in both delete and rename with that
constant/helper (update calls to state.projectActions.updateEthercatConfig to
pass masterConfig from the new constant/helper). Also review whether you should
assert presence of masterConfig instead of silently defaulting and adjust
callers accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ebd9edc9-c907-4264-b5d1-42ae0659b669

📥 Commits

Reviewing files that changed from the base of the PR and between 8446a1f and 44c3515.

📒 Files selected for processing (2)

• src/frontend/store/__tests__/shared-slice.test.ts
• src/frontend/store/slices/shared/slice.ts

[coderabbitai]

Actionable comments posted: 7

♻️ Duplicate comments (2)

src/frontend/hooks/use-device-configuration.ts (1)

81-92: ⚠️ Potential issue | 🟠 Major

User-edited channelMappings are still overwritten by enrichment.

Past feedback flagged that enrichDeviceData(...) returns a payload whose ...rest still contains channelMappings, and the typical onEnrichDevice implementation merges via { ...device, ...data } — so if a user had already edited mappings (e.g. set aliases) before the first full-device load completed, those edits get replaced by freshly generated defaults on Line 82-83. The externalAddresses fix is in place, but the channelMappings clobber is not.

The existing Line 77-79 path already handles the "no mappings yet" case; the enrichment branch should omit channelMappings when the user already has some.

🐛 Suggested fix

-          if (!device.channelInfo || !device.rxPdos || !device.txPdos) {
-            const { sdoConfigurations, ...rest } = enrichDeviceData(result.device, externalAddresses)
-            onEnrichDeviceRef.current(device.sdoConfigurations !== undefined ? rest : { ...rest, sdoConfigurations })
+          if (!device.channelInfo || !device.rxPdos || !device.txPdos) {
+            const { sdoConfigurations, channelMappings, ...rest } = enrichDeviceData(
+              result.device,
+              externalAddresses,
+            )
+            const base = device.channelMappings.length > 0 ? rest : { ...rest, channelMappings }
+            onEnrichDeviceRef.current(device.sdoConfigurations !== undefined ? base : { ...base, sdoConfigurations })

🤖 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 81 - 92, The
enrichment branch is clobbering user-edited channelMappings because
enrichDeviceData(...) returns channelMappings inside ...rest which gets applied
via onEnrichDeviceRef.current; modify the branch that calls enrichDeviceData
(the block using const { sdoConfigurations, ...rest } =
enrichDeviceData(result.device, externalAddresses) and passing rest) to detect
if device.channelMappings exists/has entries and, if so, remove channelMappings
from rest before calling onEnrichDeviceRef.current so user edits are preserved;
apply the same omission logic wherever enrichment constructs a device object
(including the fallback branch that builds sdoConfigurations using
extractDefaultSdoConfigurations) to ensure channelMappings are only provided
when the device has no existing mappings.

src/frontend/store/slices/shared/slice.ts (1)

628-637: ⚠️ Potential issue | 🟠 Major

slave.name is still used as the file-registry key.

The loop at Line 635 registers each EtherCAT child as files[slave.name], so two slaves sharing a display name (same bus or across buses) will collide and overwrite each other's save-state. The same collision surfaces in tab/editor keys via ethercatDeviceActions.delete/rename, and the rename path does not guard against the new name already being in use by another slave, which makes the issue reachable by a plain rename. A stable composite identity such as `${d.name}:${slave.id}` (with slave.name kept for display/label only) would close this.

🤖 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 628 - 637, The file
registry uses slave.name as the key which causes collisions when display names
duplicate; change the registry to use a stable composite key like
`${d.name}:${slave.id}` when registering in the files map (replace
files[slave.name] with files[`${d.name}:${slave.id}`]) and keep slave.name as
the display label only, update any related flows that reference that key (e.g.,
ethercatDeviceActions.rename, ethercatDeviceActions.delete and
fileActions.updateFile) to read/write the composite key, and add a guard in the
rename path to detect if the target composite key already exists before renaming
to prevent overwriting another slave's entry.

🧹 Nitpick comments (4)

src/backend/editor/ethercat/esi-service.ts (2)

139-158: Corrupt-backup rename is not atomic against concurrent readers.

When multiple renderer calls race on a freshly corrupted file, two readers can both detect the parse error and both attempt rename(repoPath, backupPath); the second will log "backup rename failed" because the source no longer exists, even though recovery succeeded. Not a correctness bug — ENOENT on the rename is harmless — but the console.error with renameError will be noisy in that case. Consider treating renameError.code === 'ENOENT' as a benign "already recovered by another caller" and downgrading to warn without the error payload.

🤖 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 139 - 158, The
rename error handler in the catch block that runs after JSON parse failure
currently logs all rename failures as errors; treat an ENOENT from
promises.rename as benign (another concurrent reader already moved the file) by
checking renameError.code === 'ENOENT' and downgrading the log to console.warn
(without the error payload), while retaining console.error for other rename
failures; update the catch that handles renameError inside the parseError
handler (references: parseError, renameError, promises.rename, repoPath,
backupPath, ESIService) accordingly.

---

146-155: Main-process logging should use Winston, not console.*.

This module runs in the main process (invoked via IPC handlers in src/main/modules/ipc/main.ts) and the project already exposes the shared logger from backend/editor/services — which is used throughout main.ts. Replacing console.warn / console.error here (and in saveRepositoryIndexV2, saveXmlFile, loadXmlFile, deleteXmlFile, migrateRepositoryToV2) with logger.warn / logger.error keeps the repository corruption/recovery events in the structured log stream operators actually look at.

As per coding guidelines: "Use Winston for structured logging in the main process" (applies to src/main/**/*.ts). This file is under src/backend/ but is main-process-only code executed by those IPC handlers, so the spirit of the rule applies.

🤖 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 146 - 155, Replace
main-process console calls with the shared Winston logger: import and use the
project-wide logger (the shared logger used elsewhere in main) and change
console.warn/console.error occurrences in ESIService (notably in the
error-handling blocks inside saveRepositoryIndexV2 and the catch handling shown,
and likewise in saveXmlFile, loadXmlFile, deleteXmlFile, migrateRepositoryToV2)
to logger.warn/logger.error; include the existing parseError and renameError as
structured metadata in the logger call so the messages remain descriptive and
machine-parsable (e.g., descriptive message + { parseError, renameError }).

src/main/modules/ipc/main.ts (2)

318-324: wrapServiceCall swallows the stack trace with String(error).

String(error) on an Error instance produces "Error: <message>" and drops the stack. The codebase already imports getErrorMessage (Line 5) for exactly this purpose and uses it everywhere else in this file. Aligning here keeps renderer-side error messages consistent and avoids the "Error: " prefix that leaks the implementation.

♻️ Proposed fix

-    } catch (error) {
-      return { success: false, error: String(error) }
-    }
+    } catch (error) {
+      return { success: false, error: getErrorMessage(error) }
+    }

🤖 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 318 - 324, wrapServiceCall
currently converts thrown errors with String(error), which strips the stack and
prefixes "Error: "; change the error serialization to use the existing helper by
returning { success: false, error: getErrorMessage(error) } inside the catch
block of wrapServiceCall so renderer-side messages match the rest of the file
(getErrorMessage is already imported).

---

1698-1699: esi:delete-xml-file channel name is misleading — it also mutates the repository index.

The IPC channel esi:delete-xml-file is wired to esiService.deleteRepositoryItemV2, which both deletes the XML file and rewrites repository.json. Meanwhile esi:save-xml-file only writes the XML file (no index update), and the index-aware save path lives under esi:parse-and-save-file. The asymmetric naming invites bugs — e.g. a caller that only wants to drop the XML file without affecting the index has no way to do so, and anyone reading the IPC contract in renderer.ts would reasonably assume symmetric semantics with esi:save-xml-file.

Either rename the channel (e.g. esi:delete-repository-item) or expose a separate "delete XML only" operation. The renderer wrapper esiDeleteXmlFile in src/main/modules/ipc/renderer.ts (Line 434) inherits the same misleading name.

🤖 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 1698 - 1699, The IPC channel name
is misleading because handleESIDeleteXmlFile is wired to
esiService.deleteRepositoryItemV2 which both deletes the XML and mutates
repository.json; either rename the channel (e.g. change "esi:delete-xml-file" to
"esi:delete-repository-item") or add a new "delete XML only" service and
channel; to fix, update handleESIDeleteXmlFile (or create
handleESIDeleteXmlOnly) to call a new esiService.deleteXmlOnly(projectPath,
itemId) if you want file-only behavior, or rename the channel and the renderer
wrapper esiDeleteXmlFile (and all callers) to match repository-aware semantics,
and ensure consistent naming with esi:save-xml-file and esi:parse-and-save-file
so callers know whether the index is mutated (update IPC channel string, the
renderer wrapper in renderer.ts, and any callers invoking the channel).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/backend/editor/ethercat/esi-service.ts`:
- Around line 75-80: parseAndSaveFile and loadRepositoryIndex can race with
saveRepositoryIndexV2 because saveRepositoryIndexV2 writes the index file
non-atomically, allowing readers called outside withIndexLock to observe
truncated JSON; update saveRepositoryIndexV2 to perform an atomic write by
writing the JSON to a temporary file (unique tmp name in same directory), fsync
the temp file (and optionally its directory), then rename/move the temp file to
the target index path (atomic replace), preserving permissions — this ensures
loadRepositoryIndex and parseAndSaveFile never see half-written files.
- Around line 418-458: parseAndSaveFile (and similarly deleteRepositoryItemV2)
currently calls loadLightItemsFromIndex which returns [] for non-v2 indexes,
causing saveRepositoryIndexV2([...currentItems, item]) to overwrite and lose all
v1 entries; update parseAndSaveFile and deleteRepositoryItemV2 to first call
loadRepositoryIndex (or check the loaded index.version) and if version !== 2
either return a clear failure ({ success: false, error: 'Repository requires
migration' }) or perform an inline migration before calling
saveRepositoryIndexV2, or alternatively change loadLightItemsFromIndex to throw
when an index is v1 so callers must handle migration explicitly—ensure you
reference and use loadRepositoryIndex, loadLightItemsFromIndex,
saveRepositoryIndexV2, parseAndSaveFile, and deleteRepositoryItemV2 when
implementing the fix.

In `@src/backend/shared/ethercat/esi-parser.ts`:
- Around line 205-215: The function parseIecLocationToBitRange currently accepts
malformed bit-addresses like "%IX0" because the regex makes the ".bit" suffix
optional for all kinds; change the validation so bit suffix is required when
kind === 'X' and forbidden for kinds 'B','W','D','L': after extracting dir,
kind, byteStr and bitStr from the regex, explicitly reject the input (return
null) if kind === 'X' and bitStr === undefined, and likewise reject if kind !==
'X' and bitStr !== undefined; keep the rest of the logic (using IEC_WIDTH_BITS,
computing startBit/endBit and returning the IecBitRange) unchanged so only
properly-formed bit addresses are accepted.

In `@src/frontend/hooks/use-device-configuration.ts`:
- Around line 66-69: The non-null assertion on esiPort causes a runtime
TypeError when useEsi() returns undefined; update the async routine that calls
esiPort!.loadDeviceFull (in use-device-configuration.ts, the function that
performs loadFullDevice/loadDeviceFull) to first check for esiPort existence and
either return early (skip the effect) or throw a clear, typed error (e.g., new
Error("ESI port unavailable in this environment")) so the catch handler receives
a meaningful message instead of a TypeError; locate the call site of
esiPort!.loadDeviceFull and replace the non-null assertion with this guard
before invoking loadDeviceFull.
- Around line 58-104: The effect that calls loadFullDevice captures
externalAddresses and latches fullDeviceLoadedRef causing stale mappings and
skipped reloads when device identity changes; fix by: add externalAddresses to
the dependency array of the loading effect (or guard use of
generateDefaultChannelMappings with fullDeviceLoadedRef.current to avoid
re-running), and reset fullDeviceLoadedRef.current = false in a separate
useEffect keyed on device.esiDeviceRef.repositoryItemId and
device.esiDeviceRef.deviceIndex so each new device forces loadFullDevice to run
and uses up-to-date externalAddresses when generating default mappings
(referencing fullDeviceLoadedRef, generateDefaultChannelMappings,
loadFullDevice, and externalAddresses).

In `@src/frontend/store/slices/project/slice.ts`:
- Around line 197-215: The existence check for EtherCAT system tasks only
matches tasks with isSystemTask === true, causing duplicates for legacy tasks;
in the migration loop in slice.project handling (inside the slice logic), update
the lookup to also consider tasks with name === ethercatTaskName(device.name);
if a task by name exists but lacks isSystemTask/associatedDevice, mutate that
task to set isSystemTask = true and associatedDevice = device.name (and update
interval/priority from device.ethercatConfig as needed) instead of unshifting a
new task, otherwise add the new system task as currently done.

In `@src/main/modules/ipc/main.ts`:
- Around line 1686-1690: The handler handleESILoadRepositoryIndex passes the raw
value from esiService.loadRepositoryIndex (which can be null) into the IPC
response shape that expects data to be either undefined or an object; change the
handler so it coerces a null index to undefined before returning (e.g. const
data = index ?? undefined; return { success: true, data }), or alternatively
update the IPC contract types to allow data: ... | null; locate
handleESILoadRepositoryIndex and esiService.loadRepositoryIndex to implement the
coercion (preferred) or adjust the renderer-side response type to include null.

---

Duplicate comments:
In `@src/frontend/hooks/use-device-configuration.ts`:
- Around line 81-92: The enrichment branch is clobbering user-edited
channelMappings because enrichDeviceData(...) returns channelMappings inside
...rest which gets applied via onEnrichDeviceRef.current; modify the branch that
calls enrichDeviceData (the block using const { sdoConfigurations, ...rest } =
enrichDeviceData(result.device, externalAddresses) and passing rest) to detect
if device.channelMappings exists/has entries and, if so, remove channelMappings
from rest before calling onEnrichDeviceRef.current so user edits are preserved;
apply the same omission logic wherever enrichment constructs a device object
(including the fallback branch that builds sdoConfigurations using
extractDefaultSdoConfigurations) to ensure channelMappings are only provided
when the device has no existing mappings.

In `@src/frontend/store/slices/shared/slice.ts`:
- Around line 628-637: The file registry uses slave.name as the key which causes
collisions when display names duplicate; change the registry to use a stable
composite key like `${d.name}:${slave.id}` when registering in the files map
(replace files[slave.name] with files[`${d.name}:${slave.id}`]) and keep
slave.name as the display label only, update any related flows that reference
that key (e.g., ethercatDeviceActions.rename, ethercatDeviceActions.delete and
fileActions.updateFile) to read/write the composite key, and add a guard in the
rename path to detect if the target composite key already exists before renaming
to prevent overwriting another slave's entry.

---

Nitpick comments:
In `@src/backend/editor/ethercat/esi-service.ts`:
- Around line 139-158: The rename error handler in the catch block that runs
after JSON parse failure currently logs all rename failures as errors; treat an
ENOENT from promises.rename as benign (another concurrent reader already moved
the file) by checking renameError.code === 'ENOENT' and downgrading the log to
console.warn (without the error payload), while retaining console.error for
other rename failures; update the catch that handles renameError inside the
parseError handler (references: parseError, renameError, promises.rename,
repoPath, backupPath, ESIService) accordingly.
- Around line 146-155: Replace main-process console calls with the shared
Winston logger: import and use the project-wide logger (the shared logger used
elsewhere in main) and change console.warn/console.error occurrences in
ESIService (notably in the error-handling blocks inside saveRepositoryIndexV2
and the catch handling shown, and likewise in saveXmlFile, loadXmlFile,
deleteXmlFile, migrateRepositoryToV2) to logger.warn/logger.error; include the
existing parseError and renameError as structured metadata in the logger call so
the messages remain descriptive and machine-parsable (e.g., descriptive message
+ { parseError, renameError }).

In `@src/main/modules/ipc/main.ts`:
- Around line 318-324: wrapServiceCall currently converts thrown errors with
String(error), which strips the stack and prefixes "Error: "; change the error
serialization to use the existing helper by returning { success: false, error:
getErrorMessage(error) } inside the catch block of wrapServiceCall so
renderer-side messages match the rest of the file (getErrorMessage is already
imported).
- Around line 1698-1699: The IPC channel name is misleading because
handleESIDeleteXmlFile is wired to esiService.deleteRepositoryItemV2 which both
deletes the XML and mutates repository.json; either rename the channel (e.g.
change "esi:delete-xml-file" to "esi:delete-repository-item") or add a new
"delete XML only" service and channel; to fix, update handleESIDeleteXmlFile (or
create handleESIDeleteXmlOnly) to call a new
esiService.deleteXmlOnly(projectPath, itemId) if you want file-only behavior, or
rename the channel and the renderer wrapper esiDeleteXmlFile (and all callers)
to match repository-aware semantics, and ensure consistent naming with
esi:save-xml-file and esi:parse-and-save-file so callers know whether the index
is mutated (update IPC channel string, the renderer wrapper in renderer.ts, and
any callers invoking the channel).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5cf42e28-bf5d-410e-9565-bfaa7d2a2ecd

📥 Commits

Reviewing files that changed from the base of the PR and between 44c3515 and e414fbc.

📒 Files selected for processing (19)

• src/backend/editor/compiler/compiler-module.ts
• src/backend/editor/ethercat/esi-service.ts
• src/backend/shared/ethercat/device-matcher.ts
• src/backend/shared/ethercat/esi-parser.ts
• src/frontend/components/_features/[workspace]/editor/device/ethercat/components/device-scan-table.tsx
• src/frontend/components/_features/[workspace]/editor/device/ethercat/components/discovered-device-table.tsx
• src/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-upload.tsx
• src/frontend/components/_features/[workspace]/editor/device/ethercat/components/global-settings-tab.tsx
• src/frontend/components/_features/[workspace]/editor/device/ethercat/components/runtime-status-panel.tsx
• src/frontend/components/_features/[workspace]/editor/device/ethercat/ethercat-device-editor.tsx
• src/frontend/hooks/use-device-configuration.ts
• src/frontend/store/slices/project/slice.ts
• src/frontend/store/slices/shared/slice.ts
• src/main/modules/ipc/main.ts
• src/main/modules/ipc/renderer.ts
• src/middleware/adapters/editor/esi-adapter.ts
• src/middleware/shared/ports/esi-port.ts
• src/middleware/shared/ports/runtime-port.ts
• src/middleware/shared/ports/types.ts

✅ Files skipped from review due to trivial changes (1)

• src/middleware/shared/ports/esi-port.ts

🚧 Files skipped from review as they are similar to previous changes (10)

• src/frontend/components/_features/[workspace]/editor/device/ethercat/components/device-scan-table.tsx
• src/middleware/adapters/editor/esi-adapter.ts
• src/frontend/components/_features/[workspace]/editor/device/ethercat/ethercat-device-editor.tsx
• src/backend/editor/compiler/compiler-module.ts
• src/frontend/components/_features/[workspace]/editor/device/ethercat/components/runtime-status-panel.tsx
• src/frontend/components/_features/[workspace]/editor/device/ethercat/components/discovered-device-table.tsx
• src/middleware/shared/ports/runtime-port.ts
• src/frontend/components/_features/[workspace]/editor/device/ethercat/components/global-settings-tab.tsx
• src/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-upload.tsx
• src/backend/shared/ethercat/device-matcher.ts

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Data loss when uploading into a not-yet-migrated v1 index.

parseAndSaveFile calls loadLightItemsFromIndex(projectPath) before writing the new v2 index. loadLightItemsFromIndex returns [] for any index where version !== 2 (Line 292), so if a user uploads a new ESI file into a project whose repository.json is still v1 (or partially v2 with items missing devices), the subsequent saveRepositoryIndexV2(projectPath, [...currentItems, item]) overwrites the on-disk index with only the newly added item, silently discarding every pre-existing v1 entry. The same pattern in deleteRepositoryItemV2 (Line 271-273) deletes one XML but then truncates the index to [].

Callers are expected to invoke esi:migrate-repository first, but nothing in this service enforces that invariant, and the IPC flow has no guard either. Please either:

1. In parseAndSaveFile/deleteRepositoryItemV2, detect a v1-shaped index and either refuse with a clear error (e.g. { success: false, error: 'Repository requires migration' }) or migrate inline before mutating, or
2. Have loadLightItemsFromIndex throw on v1 so callers are forced to handle it.

🤖 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 418 - 458,
parseAndSaveFile (and similarly deleteRepositoryItemV2) currently calls
loadLightItemsFromIndex which returns [] for non-v2 indexes, causing
saveRepositoryIndexV2([...currentItems, item]) to overwrite and lose all v1
entries; update parseAndSaveFile and deleteRepositoryItemV2 to first call
loadRepositoryIndex (or check the loaded index.version) and if version !== 2
either return a clear failure ({ success: false, error: 'Repository requires
migration' }) or perform an inline migration before calling
saveRepositoryIndexV2, or alternatively change loadLightItemsFromIndex to throw
when an index is v1 so callers must handle migration explicitly—ensure you
reference and use loadRepositoryIndex, loadLightItemsFromIndex,
saveRepositoryIndexV2, parseAndSaveFile, and deleteRepositoryItemV2 when
implementing the fix.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

parseIecLocationToBitRange accepts malformed %IX0 (bit-address without .bit) as a 1-bit range.

The regex /^%([IQ])([XBWDL])(\d+)(?:\.(\d+))?$/ makes the bit suffix optional for every width prefix — including X. For %IX0 (no .bit), bit defaults to 0, so it's treated as a valid 1-bit address at bit 0 of byte 0. But in IEC 61131-3, bit-level addressing requires the explicit .bit suffix; %IX0 alone is malformed and should be rejected so callers can surface it instead of silently mis-overlapping.

Conversely, non-bit prefixes (B/W/D/L) should not accept a .bit suffix. Tightening the regex avoids false matches on user-mangled input in collectUsedIecAddresses.

🛠️ Suggested fix

-  const match = /^%([IQ])([XBWDL])(\d+)(?:\.(\d+))?$/.exec(location)
-  if (!match) return null
-  const [, dir, kind, byteStr, bitStr] = match
-  const width = IEC_WIDTH_BITS[kind]
-  if (width === undefined) return null
+  const match = /^%([IQ])(?:(X)(\d+)\.(\d+)|([BWDL])(\d+))$/.exec(location)
+  if (!match) return null
+  const dir = match[1]
+  const kind = match[2] ?? match[5]
+  const byteStr = match[3] ?? match[6]
+  const bitStr = match[4]
+  const width = IEC_WIDTH_BITS[kind]
+  if (width === undefined) return null

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/backend/shared/ethercat/esi-parser.ts` around lines 205 - 215, The
function parseIecLocationToBitRange currently accepts malformed bit-addresses
like "%IX0" because the regex makes the ".bit" suffix optional for all kinds;
change the validation so bit suffix is required when kind === 'X' and forbidden
for kinds 'B','W','D','L': after extracting dir, kind, byteStr and bitStr from
the regex, explicitly reject the input (return null) if kind === 'X' and bitStr
=== undefined, and likewise reject if kind !== 'X' and bitStr !== undefined;
keep the rest of the logic (using IEC_WIDTH_BITS, computing startBit/endBit and
returning the IecBitRange) unchanged so only properly-formed bit addresses are
accepted.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Stale externalAddresses / projectPath capture and skipped reloads.

Two related concerns on the effect:

1. externalAddresses is read inside the effect but not in the dependency array (Line 104). If the set of in-use IEC addresses changes between when the component mounts and when loadDeviceFull resolves (e.g. another device added in a different tab), the generated mappings at Line 78 / Line 82 use a stale snapshot and can collide. Since generateDefaultChannelMappings only runs once per device (fullDeviceLoadedRef latches), this is bounded, but worth acknowledging — or adding externalAddresses to deps with a guard on fullDeviceLoadedRef.

2. fullDeviceLoadedRef.current is never reset when device.esiDeviceRef.repositoryItemId / deviceIndex changes. When the parent re-renders the same hook instance with a different slave (e.g. user switches between configured devices in a detail panel), the effect sees the new deps but the ref is still true from the previous device, so the new device's full data is never fetched. Reset the ref when deps change.

🛠️ Suggested fix

   useEffect(() => {
-    if (!enabled || !device || fullDeviceLoadedRef.current) return
+    fullDeviceLoadedRef.current = false
+    if (!enabled || !device) return

Or, more idiomatically, reset the ref in a separate effect keyed on device identity, then let the load effect check it.

🤖 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 58 - 104, The
effect that calls loadFullDevice captures externalAddresses and latches
fullDeviceLoadedRef causing stale mappings and skipped reloads when device
identity changes; fix by: add externalAddresses to the dependency array of the
loading effect (or guard use of generateDefaultChannelMappings with
fullDeviceLoadedRef.current to avoid re-running), and reset
fullDeviceLoadedRef.current = false in a separate useEffect keyed on
device.esiDeviceRef.repositoryItemId and device.esiDeviceRef.deviceIndex so each
new device forces loadFullDevice to run and uses up-to-date externalAddresses
when generating default mappings (referencing fullDeviceLoadedRef,
generateDefaultChannelMappings, loadFullDevice, and externalAddresses).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Non-null assertion on esiPort will crash on web/non-electron platforms.

useEsi() can return undefined in environments where the ESI port isn't wired (e.g., when PlatformContext hasn't provided it). The non-null assertion esiPort! turns that into a TypeError thrown into the loadFullDevice async function and captured by the catch on Line 96 as "TypeError: ...undefined". Better to fail-fast with a typed error message and skip the effect.

🛡️ Suggested guard

-    if (!enabled || !device || fullDeviceLoadedRef.current) return
+    if (!enabled || !device || !esiPort || fullDeviceLoadedRef.current) return
...
-        const result = await esiPort!.loadDeviceFull(
+        const result = await esiPort.loadDeviceFull(

🤖 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 66 - 69, The
non-null assertion on esiPort causes a runtime TypeError when useEsi() returns
undefined; update the async routine that calls esiPort!.loadDeviceFull (in
use-device-configuration.ts, the function that performs
loadFullDevice/loadDeviceFull) to first check for esiPort existence and either
return early (skip the effect) or throw a clear, typed error (e.g., new
Error("ESI port unavailable in this environment")) so the catch handler receives
a meaningful message instead of a TypeError; locate the call site of
esiPort!.loadDeviceFull and replace the non-null assertion with this guard
before invoking loadDeviceFull.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Legacy projects without the isSystemTask flag will spawn duplicate tasks.

The existence check at Line 200-202 matches only tasks that already carry isSystemTask === true && associatedDevice === device.name. A project saved before this feature shipped will have a task named ethercatTaskName(device.name) but no isSystemTask flag, so setProject will unshift a second task with the same name on every open. Consider also matching by t.name === ethercatTaskName(device.name) and, when found, backfilling the isSystemTask/associatedDevice flags rather than inserting a duplicate.

🤖 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 197 - 215, The
existence check for EtherCAT system tasks only matches tasks with isSystemTask
=== true, causing duplicates for legacy tasks; in the migration loop in
slice.project handling (inside the slice logic), update the lookup to also
consider tasks with name === ethercatTaskName(device.name); if a task by name
exists but lacks isSystemTask/associatedDevice, mutate that task to set
isSystemTask = true and associatedDevice = device.name (and update
interval/priority from device.ethercatConfig as needed) instead of unshifting a
new task, otherwise add the new system task as currently done.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Response shape drops null from loadRepositoryIndex into a type that doesn't allow it.

esiService.loadRepositoryIndex returns ESIRepositoryIndex | null (null when repository.json doesn't exist). This handler returns { success: true, data: index }, which the renderer-side type in src/main/modules/ipc/renderer.ts (Line 415-419) declares as data?: { version: number; items: Array<...> } — i.e. undefined-or-object, not null. Consumers doing result.data.items without a null check will crash on a fresh project.

Either coerce null → undefined here, or type the IPC contract as data?: ... | null.

🛠️ Suggested coercion

-      const index = await this.esiService.loadRepositoryIndex(projectPath)
-      return { success: true as const, data: index }
+      const index = await this.esiService.loadRepositoryIndex(projectPath)
+      return { success: true as const, data: index ?? undefined }

🤖 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 1686 - 1690, The handler
handleESILoadRepositoryIndex passes the raw value from
esiService.loadRepositoryIndex (which can be null) into the IPC response shape
that expects data to be either undefined or an object; change the handler so it
coerces a null index to undefined before returning (e.g. const data = index ??
undefined; return { success: true, data }), or alternatively update the IPC
contract types to allow data: ... | null; locate handleESILoadRepositoryIndex
and esiService.loadRepositoryIndex to implement the coercion (preferred) or
adjust the renderer-side response type to include null.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (1)

src/main/modules/ipc/main.ts (1)

1684-1688: ⚠️ Potential issue | 🟡 Minor

Coerce a missing repository index out of data.

Line 1687 still returns a possible null index as data; keep the IPC response shape object-or-undefined so fresh projects do not surprise consumers.

🛠️ Proposed fix

   handleESILoadRepositoryIndex = async (_event: IpcMainInvokeEvent, projectPath: string) =>
     this.wrapServiceCall(async () => {
       const index = await this.esiService.loadRepositoryIndex(projectPath)
-      return { success: true as const, data: index }
+      return { success: true as const, data: index ?? undefined }
     })

🤖 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 1684 - 1688,
handleESILoadRepositoryIndex currently returns the raw index which can be null;
change the IPC response to keep the object-or-undefined shape by coercing null
to undefined before returning (use the result of
this.esiService.loadRepositoryIndex(projectPath) and return { success: true,
data: index ?? undefined } or equivalent) so consumers always receive either an
object or undefined; update the return inside handleESILoadRepositoryIndex (and
keep wrapServiceCall usage) to perform this coercion.

🧹 Nitpick comments (1)

src/main/modules/ipc/main.ts (1)

1507-1680: Add Zod validation for runtime/plugin payload responses in EtherCAT handlers.

These handlers use JSON.parse(data) as ... pattern, which only validates JSON syntax—not structure. If the runtime returns valid JSON with unexpected shape, malformed data flows to the renderer as success: true. Add Zod schemas for EtherCATServiceStatusResponse, EtherCATScanResponse, EtherCATTestResponse, EtherCATValidateResponse, EtherCATRuntimeStatusResponse, and NetworkInterface, then validate against them before returning successful results. Per coding guidelines, Zod must be used for schema validation.

🤖 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 1507 - 1680, The handlers
handleEtherCATGetInterfaces, handleEtherCATGetStatus, handleEtherCATScan,
handleEtherCATTest, handleEtherCATValidate and handleEtherCATGetRuntimeStatus
currently use JSON.parse(...) as Type which doesn't enforce structure; add Zod
schemas for NetworkInterface, EtherCATServiceStatusResponse,
EtherCATScanResponse, EtherCATTestResponse, EtherCATValidateResponse and
EtherCATRuntimeStatusResponse, import/create those schemas, then replace the
parse callbacks passed into makeRuntimeApiRequest/makeRuntimeApiPostRequest to:
parse JSON, validate with the appropriate zodSchema.parse or safeParse and throw
a descriptive Error on validation failure (so result.success is false), and only
return the typed data when validation passes; ensure plugin-response branches
(pluginResponse.error) still surface runtime errors before validation.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.claude/skills/sync-shared-surfaces/SKILL.md:
- Around line 93-107: The markdown has fenced code blocks without language
identifiers (the blocks starting with "Editor branch:
feat/ethercat-esi-backend..." and the block starting with "sync: mirror shared
surfaces from openplc-<source>") which triggers MD040; fix by updating the
opening fences to include a language token (e.g., change ``` to ```text) for
both blocks so linting accepts them; ensure you update every identical fenced
block instance in SKILL.md (the blocks containing "Editor branch..." and the
"sync: mirror..." content).

In `@src/main/modules/ipc/main.ts`:
- Around line 1690-1707: The mutating IPC handlers (handleESISaveXmlFile,
handleESIDeleteXmlFile, handleESIParseAndSaveFile, handleESIClearRepository)
accept renderer-supplied projectPath and must be guarded against
stale/unexpected roots; before calling this.esiService methods, fetch the
current project root from the app (e.g.,
this.projectService.getCurrentProjectRoot() or equivalent) and validate that the
incoming projectPath strictly matches it (or normalize and compare). If it does
not match, short-circuit the call and return a controlled error/result via
this.wrapServiceCall (do not perform the ESIService mutation). Apply this check
to save, delete, parse-and-save, and clear handlers.
- Around line 329-408: The makeRuntimeApiPostRequest implementation duplicates
low-level https.request logic (doRequest, timeout, TLS options, response
parsing, auth, and stripStatus) that also appears elsewhere; replace it by
calling a shared Axios-based HTTP client (e.g., createRuntimeHttpClient or
runtimeApiClient) that accepts method, endpoint, body, headers (Authorization),
and timeout, and returns parsed data and HTTP status; update
makeRuntimeApiPostRequest to use that client, preserve the existing behavior
around responseParser, isTokenExpiredError(token status/error) and
attemptTokenRefresh(token refresh flow) and sending 'runtime:token-refreshed' to
mainWindow.webContents, and remove the duplicated doRequest/stripStatus logic so
all runtime HTTP calls use the centralized Axios client with TLS options from
getRuntimeHttpsOptions().

---

Duplicate comments:
In `@src/main/modules/ipc/main.ts`:
- Around line 1684-1688: handleESILoadRepositoryIndex currently returns the raw
index which can be null; change the IPC response to keep the object-or-undefined
shape by coercing null to undefined before returning (use the result of
this.esiService.loadRepositoryIndex(projectPath) and return { success: true,
data: index ?? undefined } or equivalent) so consumers always receive either an
object or undefined; update the return inside handleESILoadRepositoryIndex (and
keep wrapServiceCall usage) to perform this coercion.

---

Nitpick comments:
In `@src/main/modules/ipc/main.ts`:
- Around line 1507-1680: The handlers handleEtherCATGetInterfaces,
handleEtherCATGetStatus, handleEtherCATScan, handleEtherCATTest,
handleEtherCATValidate and handleEtherCATGetRuntimeStatus currently use
JSON.parse(...) as Type which doesn't enforce structure; add Zod schemas for
NetworkInterface, EtherCATServiceStatusResponse, EtherCATScanResponse,
EtherCATTestResponse, EtherCATValidateResponse and
EtherCATRuntimeStatusResponse, import/create those schemas, then replace the
parse callbacks passed into makeRuntimeApiRequest/makeRuntimeApiPostRequest to:
parse JSON, validate with the appropriate zodSchema.parse or safeParse and throw
a descriptive Error on validation failure (so result.success is false), and only
return the typed data when validation passes; ensure plugin-response branches
(pluginResponse.error) still surface runtime errors before validation.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0b3ada65-3cb9-448f-b585-4bef14938738

📥 Commits

Reviewing files that changed from the base of the PR and between e414fbc and 435aae4.

📒 Files selected for processing (2)

• .claude/skills/sync-shared-surfaces/SKILL.md
• src/main/modules/ipc/main.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add language identifiers to fenced code blocks to satisfy markdownlint.

Line 93 and Line 158 use fenced blocks without a language, which triggers MD040 and can fail docs lint gates.

Proposed fix

-```
+```text
 Editor branch: feat/ethercat-esi-backend (HEAD abc1234)
 Web branch:    feat/ethercat-esi-backend (HEAD def5678)

 Proposed sync:
   editor → web  (N files)
     - src/frontend/...
     - src/backend/shared/...
   web → editor  (M files)
     - src/middleware/shared/...
   needs decision  (K files)
     - src/frontend/store/__tests__/shared-slice.test.ts
       Last touched in editor by commit <hash> (<subject>)
       Last touched in web    by commit <hash> (<subject>)

• sync: mirror shared surfaces from openplc-<source>
  
  Synced <N> file(s) from openplc-<source> <branch>@<short-hash>:
    - <file 1>
    - <file 2>
    ...
  
  Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
  

</details>


Also applies to: 158-167

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>

[warning] 93-93: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</details>

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @.claude/skills/sync-shared-surfaces/SKILL.md around lines 93 - 107, The
markdown has fenced code blocks without language identifiers (the blocks
starting with "Editor branch: feat/ethercat-esi-backend..." and the block
starting with "sync: mirror shared surfaces from openplc-") which
triggers MD040; fix by updating the opening fences to include a language token
(e.g., change totext) for both blocks so linting accepts them; ensure
you update every identical fenced block instance in SKILL.md (the blocks
containing "Editor branch..." and the "sync: mirror..." content).

</details>

<!-- fingerprinting:phantom:triton:hawk:185e9efd-f242-4e01-902c-11113fdca159 -->

<!-- This is an auto-generated comment by CodeRabbit -->

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify direct HTTP client usage that should be consolidated.
rg -nP "https\.request|axios\." --type ts -C3

Repository: Autonomy-Logic/openplc-editor

Length of output: 1653

---

Consolidate HTTP requests using Axios instead of duplicating native https.request implementations.

The file now has two separate https.request handlers (lines 121 and 341) that duplicate timeout, TLS, response parsing, and auth handling. Per coding guidelines, **/*.{ts,tsx}: Use Axios for HTTP requests. Refactor both to use a single shared Axios-based HTTP client with parameterized method, body, and timeout support to reduce maintenance drift.

🤖 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 329 - 408, The
makeRuntimeApiPostRequest implementation duplicates low-level https.request
logic (doRequest, timeout, TLS options, response parsing, auth, and stripStatus)
that also appears elsewhere; replace it by calling a shared Axios-based HTTP
client (e.g., createRuntimeHttpClient or runtimeApiClient) that accepts method,
endpoint, body, headers (Authorization), and timeout, and returns parsed data
and HTTP status; update makeRuntimeApiPostRequest to use that client, preserve
the existing behavior around responseParser, isTokenExpiredError(token
status/error) and attemptTokenRefresh(token refresh flow) and sending
'runtime:token-refreshed' to mainWindow.webContents, and remove the duplicated
doRequest/stripStatus logic so all runtime HTTP calls use the centralized Axios
client with TLS options from getRuntimeHttpsOptions().

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Guard mutating ESI operations against stale or unexpected project paths.

These IPC calls pass renderer-supplied projectPath directly into file mutations. Since ESIService derives devices/esi paths from that argument, validate it against the current project root before save/delete/parse/clear to avoid corrupting the wrong project.

🛡️ Suggested guard pattern

-  handleESISaveXmlFile = async (_event: IpcMainInvokeEvent, projectPath: string, itemId: string, xmlContent: string) =>
-    this.wrapServiceCall(() => this.esiService.saveXmlFile(projectPath, itemId, xmlContent))
+  handleESISaveXmlFile = async (_event: IpcMainInvokeEvent, projectPath: string, itemId: string, xmlContent: string) => {
+    if (!this.isCurrentProjectPath(projectPath)) {
+      return { success: false as const, error: 'Path is outside the current project' }
+    }
+    return this.wrapServiceCall(() => this.esiService.saveXmlFile(projectPath, itemId, xmlContent))
+  }

Apply the same check to delete, parse-and-save, and clear operations.

🤖 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 1690 - 1707, The mutating IPC
handlers (handleESISaveXmlFile, handleESIDeleteXmlFile,
handleESIParseAndSaveFile, handleESIClearRepository) accept renderer-supplied
projectPath and must be guarded against stale/unexpected roots; before calling
this.esiService methods, fetch the current project root from the app (e.g.,
this.projectService.getCurrentProjectRoot() or equivalent) and validate that the
incoming projectPath strictly matches it (or normalize and compare). If it does
not match, short-circuit the call and return a controlled error/result via
this.wrapServiceCall (do not perform the ESIService mutation). Apply this check
to save, delete, parse-and-save, and clear handlers.

[JoaoGSP]

The port docstring says duplicate: true fires on content-hash match, but this code only checks filename equality. Rename-then-reimport silently duplicates; different content under the same filename gets wrongly rejected. Either hash the content or update the port docstring to say "filename".

[JoaoGSP]

I think that this was an architectural decision made, if I am not wrong, right? If not, scan_time_ms: 0 is hardcoded while the full pluginResponse is in scope. The field is on the EtherCATScanResponse type and the UI may rely on it. Use (pluginResponse.scan_time_ms as number) ?? 0 instead.

[JoaoGSP]

projectPath is listed but never read inside the callback; esi (from useEsi()) is read but missing. Stale-closure risk. Drop projectPath, add esi — or add an eslint-disable with a note that the adapter is stable.

[JoaoGSP]

Atomicity gap — XML is written first and the index is then appended via a plain writeFile. If the index write fails (or the app crashes in between), the XML is orphaned and the index can be truncated. Prefer write-then-rename for the index, and delete the orphan XML when the index save fails.

[JoaoGSP]

JSON.parse(data) as EtherCATServiceStatusResponse is a bare cast — any runtime-API misbehavior yields a wrongly-typed value that crashes downstream on property access. A minimal shape guard (or zod) before the cast would surface a clean error here. If there is a clean approach to correctly type this, is better, type casting can be dangerous in Typescript.

[JoaoGSP]

// VPP package manager for board package operations sits above esiService and is misleading — it reads like a comment for a different field. Drop it (or move it). The // === PACKAGE MANAGER === section header in registerHandlers is also empty — remove or fill in. Stale comments can be dangerous with AI.

[JoaoGSP]

Use import type { RuntimeLogEntry } since the symbol is only used as a type in this file.

[JoaoGSP]

This condition flipped from boardTarget === 'OpenPLC Runtime v4' to boardRuntime === 'openplc-compiler' && !isRuntimeV3. Any existing v4 board whose runtime string isn't literally 'openplc-compiler' will now be misclassified as non-v4 and skip the conf/* generation. Will be nice if we can confirm that all current hals.json v4 entries use that runtime name.

[marconetsf]

Verifiquei: hoje as três entradas do hals.json com openplc-compiler são OpenPLC Runtime v4, OpenPLC Runtime v3 e Raspberry Pi — e a classificação atual
acerta as três (v3 é excluída explicitamente; Pi entra como v4 porque roda Linux e hospeda o Runtime v4 nativamente, igual aos VPP packages com
target.type === 'runtime-v4'). A heurística é intencional: qualquer board com esse compilador é capaz de executar o Runtime v4 embarcado, então herda os
artefatos conf/*.json — comportamento desejado para novos boards Linux que venham a ser adicionados.