sync(ethercat): mirror openplc-web feat/ethercat-web-adapter#741
Merged
Conversation
Catches the editor up to the web's branch on the four shared surfaces.
The bulk of this is the new EtherCAT statistics card grid on the
Device → Orchestrators view; the rest is shared fixes that hadn't been
mirrored yet.
Surface changes (byte-identical to openplc-web):
- frontend/components/_features/[workspace]/editor/device/orchestrators/
orchestrators-list.tsx:
enables the new ethercat polling flag on mount, normalises the
runtime's two response shapes (multi-master `masters[]` and the flat
single-master legacy form) into one array, and renders one
`EtherCAT Statistics — <bus name>` card grid per master alongside
the existing Scan Cycle Statistics. Cards: Master State, Slave Count,
Cycle Count, WKC Errors (with consecutive subtitle), Cycle Time avg
(with max subtitle), Max Exchange Time, Recovery Attempts (when > 0).
- frontend/store/slices/device/{types,slice}.ts and
frontend/store/__tests__/device-slice.test.ts:
new `ethercatStatus` + `includeEthercatStatsInPolling` fields and
matching setters; both cleared on disconnect alongside the existing
timing-stats fields. Tests cover the new setters, default initial
state, and disconnect cleanup. Slice keeps 100% coverage.
- frontend/hooks/use-runtime-polling.ts:
when the consumer opts in via the new flag,
`getEthercatRuntimeStatus()` rides on the same 2s poll cycle as the
PLC status/logs pair (single Promise.all). Soft-fails on transient
errors so the UI doesn't flicker between populated and empty.
- frontend/components/_features/[workspace]/editor/device/ethercat/
components/runtime-status-panel.tsx:
removed the duplicated cycle-metric cards (and the local `MetricCard`
helper) so cycle stats live in one place. Panel now focuses on
master-state header + per-slave diagnostics table.
- frontend/components/_features/[workspace]/editor/device/ethercat/
components/discovered-device-table.tsx:
"No XML" badge palette switched from yellow (warning) to neutral gray
to match the rest of the table chrome. Tooltip remains the
load-bearing signal.
- frontend/components/_features/[workspace]/editor/device/ethercat/
index.tsx:
narrows `unmatched`/`unmatchedAddAttempt` to
`ScannedDeviceMatch['device'][]` to match the post-merge widening of
EtherCATDevice; drops the unused `getBestMatchQuality` import; minor
prettier reformat.
- frontend/components/_features/[workspace]/editor/device/ethercat/
ethercat-device-editor.tsx:
promotes "Channel Mappings" to the first tab and the default active
tab.
Editor-side runtime adapter (not shared) already implements
`getEthercatRuntimeStatus()`, so the new polling path works without
further code changes on the desktop side.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Picks up the EtherCAT card grid that openplc-web added to `board.tsx` (the Device → Configuration screen) on its `feat/ethercat-web-adapter` branch. Desktop users hit Configuration as part of their normal flow when picking a device and connecting; this is where they expect the EtherCAT stats to live, alongside the existing Scan Cycle Statistics. Surface change byte-identical to web. Same render shape as orchestrators-list: - enables `includeEthercatStatsInPolling` on mount, clears on unmount; - normalises `masters[]` and the flat single-master legacy form into one array via a `useMemo`; - renders one `EtherCAT Statistics — <bus name>` card grid per master inside the existing `isOpenPLCRuntimeTarget` branch, after the Scan Cycle Statistics block. Editor's runtime adapter (not shared) already implements `getEthercatRuntimeStatus()`, so the new polling path works without further code changes on the desktop side. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds end-to-end EtherCAT runtime-status support: new types/ports, store fields/actions, optional polling in the runtime poller, normalizer util, per-master stats component, UI wiring in board/orchestrators, frontend selection/editor updates, backend ESI/default decoding improvements, and related tests. ChangesEtherCAT runtime-status feature (end-to-end)
Sequence Diagram(s)sequenceDiagram
participant Board as Board Component
participant Orchs as Orchestrators Screen
participant Hook as use-runtime-polling Hook
participant Runtime as Runtime API
participant Store as Redux Store
participant UI as EthercatStatsSection
Board->>Store: setIncludeEthercatStatsInPolling(true) on mount
Orchs->>Store: subscribe/read ethercatStatus
Hook->>Store: read includeEthercatStatsInPolling
alt includeEthercatStatsInPolling = true
Hook->>Runtime: getEthercatRuntimeStatus()
Runtime-->>Hook: ethercatStatus or error
alt success
Hook->>Store: setEthercatStatus(data)
else error
Hook-->>Store: do not overwrite previous ethercatStatus
end
else
Hook->>Store: setEthercatStatus(null)
end
Store-->>Board: ethercatStatus updated
Board->>Board: normalizeEthercatStatus -> ethercatMasters
Board-->>UI: render EthercatStatsSection with ethercatMasters
Board->>Store: setIncludeEthercatStatsInPolling(false) on unmount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/frontend/components/_features/[workspace]/editor/device/ethercat/components/runtime-status-panel.tsx (2)
153-153:⚠️ Potential issue | 🟡 MinorNon-null assertion on optional port method can throw at runtime.
runtime.getEthercatRuntimeStatusis declared optional onRuntimePort(seesrc/middleware/shared/ports/runtime-port.ts:178). Calling it with!()will throwTypeError: runtime.getEthercatRuntimeStatus is not a functionif any adapter (current or future) omits the implementation, instead of being caught by the existing error branch.🛡️ Suggested guard
- const result = await runtime.getEthercatRuntimeStatus!() + if (!runtime.getEthercatRuntimeStatus) { + setPluginNotActive(true) + setStatus(null) + setError(null) + return + } + const result = await runtime.getEthercatRuntimeStatus()🤖 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/runtime-status-panel.tsx at line 153, The code uses a non-null assertion calling runtime.getEthercatRuntimeStatus!(), which will throw if that optional port method is not implemented; replace the assertion with a runtime guard (e.g., check typeof runtime.getEthercatRuntimeStatus === "function" or use optional chaining) and only call it when present, otherwise route to the existing error/unsupported branch (log or set appropriate error state) so missing adapters are handled by the existing error handling instead of raising a TypeError; reference the runtime.getEthercatRuntimeStatus symbol and ensure the call site in the runtime-status-panel honours the guard.
137-199:⚠️ Potential issue | 🟠 MajorDuplicate EtherCAT polling — consider reading
runtimeConnection.ethercatStatusinstead.With this PR,
useRuntimePollingalready fetchesgetEthercatRuntimeStatus()every 2 s and writes the result toruntimeConnection.ethercatStatuswheneverincludeEthercatStatsInPollingis on (whichboard.tsxandorchestrators-list.tsxenable on mount). This panel keeps its own 2 ssetIntervaldoing the same call, so when the panel is visible the runtime now sees ~2 round-trips per cycle for the same payload, and the two state sources can drift between renders.Since the panel only consumes per-slave diagnostics, it can read from the store and drop its local fetch/timer entirely —
pluginNotActive/errorhandling can move into the global hook (or be derived from the absence ofethercatStatus). Worth doing while the surface is being mirrored.🤖 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/runtime-status-panel.tsx around lines 137 - 199, RuntimeStatusPanel is duplicating polling by calling runtime.getEthercatRuntimeStatus in its local fetchStatus/intervalRef; remove the local timer and fetch logic and instead read the status from the global runtime connection (runtimeConnection.ethercatStatus provided by useRuntime/useRuntimePolling when includeEthercatStatsInPolling is enabled). Replace fetchStatus/intervalRef and the polling useEffect with a simple subscription/read of runtimeConnection.ethercatStatus (via useRuntime()), map that value into the component state/props the panel needs (status, pluginNotActive, error — derive pluginNotActive/error from ethercatStatus presence or error fields), and keep the component reactive to isConnected/jwtToken/ipAddress if you need to early-clear UI; ensure no local setInterval or calls to getEthercatRuntimeStatus remain in RuntimeStatusPanel.src/frontend/components/_features/[workspace]/editor/device/orchestrators/orchestrators-list.tsx (1)
615-762:⚠️ Potential issue | 🔴 CriticalBug: EtherCAT stats are hidden whenever scan-cycle stats are absent.
The
<div id='scan-cycle-stats-panel'>(Lines 619-761) is rendered only whenruntimeConnection.timingStats && timingStats.scan_count > 0. Because theethercatMasters.map(...)block at Lines 685-760 lives inside that div, EtherCAT statistics will not render until the runtime also reports scan-cycle timing stats — which is unrelated to whether an EtherCAT bus is configured/operational. Fresh connections (no scan yet) and runtimes where timing stats are disabled will silently omit the EtherCAT card grid.
board.tsx(Lines 563-695) renders the two as siblings under independentconnectionStatus === 'connected'checks; orchestrators-list should do the same.🐛 Suggested restructure
- {/* Right side panel - Scan Cycle Statistics */} - {runtimeConnection.connectionStatus === 'connected' && - runtimeConnection.timingStats && - runtimeConnection.timingStats.scan_count > 0 && ( - <div - id='scan-cycle-stats-panel' - className='flex h-full w-1/2 flex-col gap-4 overflow-y-auto overflow-x-hidden p-4 lg:px-8 lg:py-4' - > - <h2 ...>Scan Cycle Statistics</h2> - <div id='scan-cycle-stats-cards' ...> - ... - </div> - - {ethercatMasters.map((master, idx) => { ... })} - </div> - )} + {runtimeConnection.connectionStatus === 'connected' && + ((runtimeConnection.timingStats && runtimeConnection.timingStats.scan_count > 0) || + ethercatMasters.length > 0) && ( + <div + id='runtime-stats-panel' + className='flex h-full w-1/2 flex-col gap-4 overflow-y-auto overflow-x-hidden p-4 lg:px-8 lg:py-4' + > + {runtimeConnection.timingStats && runtimeConnection.timingStats.scan_count > 0 && ( + <> + <h2 ...>Scan Cycle Statistics</h2> + <div id='scan-cycle-stats-cards' ...> + ... + </div> + </> + )} + + {ethercatMasters.map((master, idx) => { ... })} + </div> + )}🤖 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/orchestrators/orchestrators-list.tsx around lines 615 - 762, The EtherCAT stats block is nested inside the scan-cycle panel conditional so it hides when timing stats or scan_count are absent; extract the ethercatMasters.map(...) section out of the <div id='scan-cycle-stats-panel'> conditional and render it as a sibling (under the same runtimeConnection.connectionStatus === 'connected' guard) so EtherCAT cards display independently; ensure you keep the existing section header id/ids (e.g., sectionId, `${sectionId}-title`, `${sectionId}-cards`) and the busLabel logic (master.name || `Bus ${idx + 1}`) intact, and only gate rendering of the ethercatMasters list by connection status (and optionally by ethercatMasters.length > 0) rather than timingStats/scan_count.
🧹 Nitpick comments (4)
src/frontend/hooks/use-runtime-polling.ts (1)
123-131: Add observability for soft-failed EtherCAT polls.The “soft-fail” branch silently drops
ethercatResult.success === falseanderrorpayloads. Operators won't know an EtherCAT-specific endpoint problem is happening (e.g. plugin disabled, auth issue) since both the status and the UI keep their last good values forever. A singleconsole.warn(ordebug-level structured log) on!ethercatResult?.successwould be enough to make this diagnosable without changing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/hooks/use-runtime-polling.ts` around lines 123 - 131, The soft-fail branch in use-runtime-polling silently ignores failed EtherCAT polls; update the block that checks includeEthercatStatsInPolling so that when ethercatResult exists but ethercatResult.success is false you emit a diagnostic log (e.g., console.warn or a debug structured log) including identifying context and the ethercatResult.error/payload, then keep the current behavior of not calling setEthercatStatus; reference the symbols ethercatResult, includeEthercatStatsInPolling and setEthercatStatus so the log is added where the current soft-fail comment is.src/frontend/components/_features/[workspace]/editor/device/orchestrators/orchestrators-list.tsx (1)
305-342: Refactor: extract the EtherCAT normalization helper.The
useMemoat Lines 319-342 is a verbatim copy of the same block inboard.tsx(Lines 337-359). Two things to consolidate while the surface is being mirrored:
- Move the legacy → multi-master normalization into a shared util (e.g.
src/frontend/utils/ethercat.ts) so both screens stay in lock-step if the response shape evolves.- The mount/unmount
setIncludeEthercatStatsInPollingeffect (Lines 308-313) is also duplicated; consider a smalluseEthercatPollingOptIn()hook.Not blocking, but the duplication is the kind of thing the surface-sync PR pairing makes easy to drift on next time.
🤖 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/orchestrators/orchestrators-list.tsx around lines 305 - 342, Extract the EtherCAT normalization and polling opt-in into shared utilities: move the logic inside the useMemo that builds ethercatMasters (which reads runtimeConnection.ethercatStatus and handles status.masters vs legacy plugin_state) into a new exported helper like normalizeEthercatStatus(status) in a utils file (e.g. src/frontend/utils/ethercat.ts) and replace the useMemo with a call to that helper; similarly extract the mount/unmount effect that calls deviceActions.setIncludeEthercatStatsInPolling(true/false) into a small hook useEthercatPollingOptIn() and call that hook from this file (and from board.tsx) so both files reuse normalizeEthercatStatus and useEthercatPollingOptIn instead of duplicating the logic.src/frontend/components/_features/[workspace]/editor/device/ethercat/index.tsx (2)
377-446: Add-flow handles missing-XML cleanly; minor dependency-array hygiene.The split between
newDevicesandunmatched, the in-loopusedAddressesreservation, and keeping unmatched positions selected post-add all look correct. Two small points worth a second look:
projectPathis listed in the deps array (Line 444) but isn't read in the callback body. It can be dropped.esiis referenced viaesi!.loadDeviceFull(...)(Line 401) but isn't in the deps. IfuseEsi()ever returns a non-stable reference, the callback could capture a stale handle. Adding it (or asserting stability via the provider's contract) would make the closure robust.These are non-blocking; the user-visible behavior is correct.
♻️ Proposed dep-array tweak
}, [ selectedScannedDevices, deviceMatches, repository, configuredDevices, syncDevicesToStore, - projectPath, project.data.remoteDevices, + esi, ])🤖 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 377 - 446, The callback handleAddSelectedFromScan currently lists projectPath in its dependency array but never uses it, and it references esi via esi!.loadDeviceFull(...) without including esi in deps; remove projectPath from the deps and add esi to the dependency array (or, if the useEsi() hook guarantees a stable reference, add a comment asserting that stability instead) so the closure won't capture a stale esi; update the array that currently includes selectedScannedDevices, deviceMatches, repository, configuredDevices, syncDevicesToStore, projectPath, project.data.remoteDevices to instead include esi and omit projectPath.
583-665: Warning modal looks good; considertype="button"on the action buttons.Modal wiring (controlled
openviaunmatchedAddAttempt.length > 0, reset on close, "Go to Repository" switchingactiveTab) is clean, and the table presentation with hex-formattedvendor_id/product_codematches the rest of the EtherCAT UI. As a small defensive nit, the two<button>elements (Lines 648 and 657) default totype="submit"; if this subtree is ever rendered inside a<form>later, that would trigger an accidental submit. Addingtype="button"is cheap insurance.🛡️ Proposed defensive tweak
- <button + <button + type='button' onClick={() => { setUnmatchedAddAttempt([]) setActiveTab('repository') }} className='h-8 rounded-md border border-neutral-300 bg-white px-3 text-xs font-medium text-neutral-700 hover:bg-neutral-100 dark:border-neutral-700 dark:bg-neutral-900 dark:text-neutral-300 dark:hover:bg-neutral-800' > Go to Repository </button> - <button + <button + type='button' onClick={() => setUnmatchedAddAttempt([])} className='h-8 rounded-md bg-brand px-3 text-xs font-medium text-white hover:bg-brand-medium-dark' > OK </button>🤖 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 583 - 665, The two action buttons inside the modal (the "Go to Repository" button that calls setUnmatchedAddAttempt([]) and setActiveTab('repository', and the "OK" button that calls setUnmatchedAddAttempt([])) should explicitly set type="button" to avoid accidental form submissions if this component is rendered inside a form; locate the buttons in the JSX that reference setUnmatchedAddAttempt and setActiveTab and add type="button" to both elements.
🤖 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/frontend/components/_features/`[workspace]/editor/device/ethercat/components/discovered-device-table.tsx:
- Around line 73-98: The row (<tr role='button' tabIndex={0}
aria-pressed={isSelected} ...>) and the Checkbox component are both
keyboard-focusable and announced, causing duplicate tab stops; make the Checkbox
presentational so the row retains the single interactive control: update the
Checkbox instance (checked={isSelected} onCheckedChange={...} onClick={...}) to
include tabIndex={-1} and aria-hidden={true} (keep onCheckedChange/onClick for
mouse interactions or delegate clicks to the row), so screen readers and
keyboard users only encounter the row/button semantics and the checkbox is
ignored by assistive tech.
---
Outside diff comments:
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/runtime-status-panel.tsx:
- Line 153: The code uses a non-null assertion calling
runtime.getEthercatRuntimeStatus!(), which will throw if that optional port
method is not implemented; replace the assertion with a runtime guard (e.g.,
check typeof runtime.getEthercatRuntimeStatus === "function" or use optional
chaining) and only call it when present, otherwise route to the existing
error/unsupported branch (log or set appropriate error state) so missing
adapters are handled by the existing error handling instead of raising a
TypeError; reference the runtime.getEthercatRuntimeStatus symbol and ensure the
call site in the runtime-status-panel honours the guard.
- Around line 137-199: RuntimeStatusPanel is duplicating polling by calling
runtime.getEthercatRuntimeStatus in its local fetchStatus/intervalRef; remove
the local timer and fetch logic and instead read the status from the global
runtime connection (runtimeConnection.ethercatStatus provided by
useRuntime/useRuntimePolling when includeEthercatStatsInPolling is enabled).
Replace fetchStatus/intervalRef and the polling useEffect with a simple
subscription/read of runtimeConnection.ethercatStatus (via useRuntime()), map
that value into the component state/props the panel needs (status,
pluginNotActive, error — derive pluginNotActive/error from ethercatStatus
presence or error fields), and keep the component reactive to
isConnected/jwtToken/ipAddress if you need to early-clear UI; ensure no local
setInterval or calls to getEthercatRuntimeStatus remain in RuntimeStatusPanel.
In
`@src/frontend/components/_features/`[workspace]/editor/device/orchestrators/orchestrators-list.tsx:
- Around line 615-762: The EtherCAT stats block is nested inside the scan-cycle
panel conditional so it hides when timing stats or scan_count are absent;
extract the ethercatMasters.map(...) section out of the <div
id='scan-cycle-stats-panel'> conditional and render it as a sibling (under the
same runtimeConnection.connectionStatus === 'connected' guard) so EtherCAT cards
display independently; ensure you keep the existing section header id/ids (e.g.,
sectionId, `${sectionId}-title`, `${sectionId}-cards`) and the busLabel logic
(master.name || `Bus ${idx + 1}`) intact, and only gate rendering of the
ethercatMasters list by connection status (and optionally by
ethercatMasters.length > 0) rather than timingStats/scan_count.
---
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/index.tsx:
- Around line 377-446: The callback handleAddSelectedFromScan currently lists
projectPath in its dependency array but never uses it, and it references esi via
esi!.loadDeviceFull(...) without including esi in deps; remove projectPath from
the deps and add esi to the dependency array (or, if the useEsi() hook
guarantees a stable reference, add a comment asserting that stability instead)
so the closure won't capture a stale esi; update the array that currently
includes selectedScannedDevices, deviceMatches, repository, configuredDevices,
syncDevicesToStore, projectPath, project.data.remoteDevices to instead include
esi and omit projectPath.
- Around line 583-665: The two action buttons inside the modal (the "Go to
Repository" button that calls setUnmatchedAddAttempt([]) and
setActiveTab('repository', and the "OK" button that calls
setUnmatchedAddAttempt([])) should explicitly set type="button" to avoid
accidental form submissions if this component is rendered inside a form; locate
the buttons in the JSX that reference setUnmatchedAddAttempt and setActiveTab
and add type="button" to both elements.
In
`@src/frontend/components/_features/`[workspace]/editor/device/orchestrators/orchestrators-list.tsx:
- Around line 305-342: Extract the EtherCAT normalization and polling opt-in
into shared utilities: move the logic inside the useMemo that builds
ethercatMasters (which reads runtimeConnection.ethercatStatus and handles
status.masters vs legacy plugin_state) into a new exported helper like
normalizeEthercatStatus(status) in a utils file (e.g.
src/frontend/utils/ethercat.ts) and replace the useMemo with a call to that
helper; similarly extract the mount/unmount effect that calls
deviceActions.setIncludeEthercatStatsInPolling(true/false) into a small hook
useEthercatPollingOptIn() and call that hook from this file (and from board.tsx)
so both files reuse normalizeEthercatStatus and useEthercatPollingOptIn instead
of duplicating the logic.
In `@src/frontend/hooks/use-runtime-polling.ts`:
- Around line 123-131: The soft-fail branch in use-runtime-polling silently
ignores failed EtherCAT polls; update the block that checks
includeEthercatStatsInPolling so that when ethercatResult exists but
ethercatResult.success is false you emit a diagnostic log (e.g., console.warn or
a debug structured log) including identifying context and the
ethercatResult.error/payload, then keep the current behavior of not calling
setEthercatStatus; reference the symbols ethercatResult,
includeEthercatStatsInPolling and setEthercatStatus so the log is added where
the current soft-fail comment is.
🪄 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: 1181be0c-8ae5-4298-ba14-2838e1796e54
📒 Files selected for processing (10)
src/frontend/components/_features/[workspace]/editor/device/configuration/board.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/discovered-device-table.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/runtime-status-panel.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/ethercat-device-editor.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/index.tsxsrc/frontend/components/_features/[workspace]/editor/device/orchestrators/orchestrators-list.tsxsrc/frontend/hooks/use-runtime-polling.tssrc/frontend/store/__tests__/device-slice.test.tssrc/frontend/store/slices/device/slice.tssrc/frontend/store/slices/device/types.ts
Mirrors openplc-web's fix that re-exports `EtherCATRuntimeStatusResponse` from `middleware/shared/ports/runtime-port.ts` so the device store slice can import it without crossing the Store → Types layer boundary that the architecture validator forbids. Surface stays byte-identical with web. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors openplc-web's prettier reformat on the new EtherCAT card grid hunks in the Configuration screen. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
JoaoGSP
reviewed
Apr 30, 2026
Member
|
The new EtherCAT branches in |
JoaoGSP
reviewed
Apr 30, 2026
JoaoGSP
reviewed
Apr 30, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/frontend/store/__tests__/device-slice.test.ts (1)
3-3: ⚡ Quick winUse
@root/*alias for this test import.Line 3 introduces a new deep relative import; switch it to the project alias to stay consistent with repo import rules.
Proposed change
-import type { EtherCATRuntimeStatusResponse } from '../../../middleware/shared/ports/runtime-port' +import type { EtherCATRuntimeStatusResponse } from '@root/middleware/shared/ports/runtime-port'As per coding guidelines: "
**/*.{ts,tsx}: Prefer path alias@root/*for imports instead of relative paths to./src/*."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/store/__tests__/device-slice.test.ts` at line 3, Replace the deep relative import of EtherCATRuntimeStatusResponse in device-slice.test.ts with the project path alias; locate the import of EtherCATRuntimeStatusResponse and change its module specifier to use the `@root/`* alias (e.g., `@root/middleware/shared/ports/runtime-port`) so the test uses the repository's preferred path aliasing.
🤖 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/frontend/components/_features/`[workspace]/editor/device/configuration/board.tsx:
- Around line 565-689: The UI strings in board.tsx (e.g., "Scan Cycle
Statistics", "Scan Count", "Overruns", "Scan Time (avg)", "Cycle Time (avg)",
"Cycle Latency (avg)", "EtherCAT Statistics", "Master State", "Slave Count",
"Cycle Count", "WKC Errors", "Cycle Time (avg)" label blocks, "Max Exchange
Time", "Recovery Attempts", and bus label suffix) are hardcoded; replace them
with i18next calls: import and use the useTranslation hook (const { t } =
useTranslation()) in the component and wrap each visible string with
t('devices.board.scanCycle.title')-style keys (choose descriptive keys matching
each label/heading such as devices.board.scanCount, devices.board.overruns,
devices.board.ethercat.title, devices.board.ethercat.masterState, etc.); ensure
interpolation for dynamic bits like — {busLabel} or pluralization as needed and
add corresponding keys to the translation JSON files.
- Around line 326-331: The useEffect currently calls
setIncludeEthercatStatsInPolling unconditionally; gate this so it only enables
polling for runtime targets by checking the runtime-target condition (e.g., an
isRuntimeTarget flag, currentTarget.type === 'runtime', or presence in
runtimeTargets from context/props) before calling
setIncludeEthercatStatsInPolling(true) and still call
setIncludeEthercatStatsInPolling(false) on cleanup; update the effect that
references useEffect and setIncludeEthercatStatsInPolling to perform this
runtime check and only enable polling when the check passes.
- Around line 625-631: The current sectionId uses master.name directly which can
produce invalid DOM IDs; update the generation of sectionId (and related id
attributes like `${sectionId}-title`) to produce a deterministic, safe ID by
normalizing master.name (e.g., trim, toLowerCase, replace any non-alphanumeric
characters with hyphens or remove them) and always append the index for
uniqueness (e.g., `ethercat-stats-${normalizedName}-${idx}`); apply this in the
code that computes busLabel/sectionId so IDs remain valid and unique for
arbitrary bus names.
In `@src/frontend/store/__tests__/device-slice.test.ts`:
- Around line 1176-1212: Add unit tests for the useRuntimePolling hook to cover
the EtherCAT branches: write tests that (1) verify disabling polling via
runtimeConnection.includeEthercatStatsInPolling clears EtherCAT-related state
(use deviceActions.setIncludeEthercatStatsInPolling and verify
deviceActions.setEthercatStatus behavior), (2) exercise the optional-method
guard by mounting useRuntimePolling with a store that lacks the write/flush
methods and ensuring no exceptions occur, (3) simulate a successful write path
(mock the write method used by useRuntimePolling and assert the poll cycle
updates state as expected), and (4) simulate soft-fail behavior where a write
rejects and confirm the hook handles the failure gracefully without throwing and
sets the appropriate error/soft-fail state; use renderHook or a small test
component with makeStore and reference useRuntimePolling,
runtimeConnection.includeEthercatStatsInPolling, and deviceActions.* to locate
relevant code.
---
Nitpick comments:
In `@src/frontend/store/__tests__/device-slice.test.ts`:
- Line 3: Replace the deep relative import of EtherCATRuntimeStatusResponse in
device-slice.test.ts with the project path alias; locate the import of
EtherCATRuntimeStatusResponse and change its module specifier to use the `@root/`*
alias (e.g., `@root/middleware/shared/ports/runtime-port`) so the test uses the
repository's preferred path aliasing.
🪄 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: 80f64e21-c6e2-4eb7-986c-8571e8559e74
📒 Files selected for processing (4)
src/frontend/components/_features/[workspace]/editor/device/configuration/board.tsxsrc/frontend/store/__tests__/device-slice.test.tssrc/frontend/store/slices/device/types.tssrc/middleware/shared/ports/runtime-port.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/frontend/store/slices/device/types.ts
Comment on lines
+1176
to
+1212
| // ----------------------------------------------------------------------- | ||
| // setEthercatStatus | ||
| // ----------------------------------------------------------------------- | ||
| describe('setEthercatStatus', () => { | ||
| it('sets ethercat status', () => { | ||
| const store = makeStore() | ||
| const status = makeEthercatStatus() | ||
| store.getState().deviceActions.setEthercatStatus(status) | ||
| expect(store.getState().runtimeConnection.ethercatStatus).toEqual(status) | ||
| }) | ||
|
|
||
| it('clears ethercat status', () => { | ||
| const store = makeStore() | ||
| store.getState().deviceActions.setEthercatStatus(makeEthercatStatus()) | ||
| store.getState().deviceActions.setEthercatStatus(null) | ||
| expect(store.getState().runtimeConnection.ethercatStatus).toBeNull() | ||
| }) | ||
| }) | ||
|
|
||
| // ----------------------------------------------------------------------- | ||
| // setIncludeEthercatStatsInPolling | ||
| // ----------------------------------------------------------------------- | ||
| describe('setIncludeEthercatStatsInPolling', () => { | ||
| it('enables ethercat stats in polling', () => { | ||
| const store = makeStore() | ||
| store.getState().deviceActions.setIncludeEthercatStatsInPolling(true) | ||
| expect(store.getState().runtimeConnection.includeEthercatStatsInPolling).toBe(true) | ||
| }) | ||
|
|
||
| it('disables ethercat stats in polling', () => { | ||
| const store = makeStore() | ||
| store.getState().deviceActions.setIncludeEthercatStatsInPolling(true) | ||
| store.getState().deviceActions.setIncludeEthercatStatsInPolling(false) | ||
| expect(store.getState().runtimeConnection.includeEthercatStatsInPolling).toBe(false) | ||
| }) | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
Coverage is still missing for the polling-hook EtherCAT branches.
These new tests validate slice setters/resets, but they do not cover the useRuntimePolling logic branches introduced by this feature (disable-clears, optional-method guard, success write paths, and soft-fail behavior). Please add dedicated hook tests to prevent regressions in the poll cycle behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/store/__tests__/device-slice.test.ts` around lines 1176 - 1212,
Add unit tests for the useRuntimePolling hook to cover the EtherCAT branches:
write tests that (1) verify disabling polling via
runtimeConnection.includeEthercatStatsInPolling clears EtherCAT-related state
(use deviceActions.setIncludeEthercatStatsInPolling and verify
deviceActions.setEthercatStatus behavior), (2) exercise the optional-method
guard by mounting useRuntimePolling with a store that lacks the write/flush
methods and ensuring no exceptions occur, (3) simulate a successful write path
(mock the write method used by useRuntimePolling and assert the poll cycle
updates state as expected), and (4) simulate soft-fail behavior where a write
rejects and confirm the hook handles the failure gracefully without throwing and
sets the appropriate error/soft-fail state; use renderHook or a small test
component with makeStore and reference useRuntimePolling,
runtimeConnection.includeEthercatStatsInPolling, and deviceActions.* to locate
relevant code.
Mirror of openplc-web@3b6c397f to keep the shared frontend surface byte-identical. Bundles four review-feedback fixes from PR #741: - discovered-device-table: drop the duplicate tab stop and screen-reader announcement on each row. The row carries the button semantics; the checkbox is now presentational (tabIndex={-1}, aria-hidden, pointer-events-none). - use-runtime-polling: swallow a rejected EtherCAT runtime-status call to null inside Promise.all so a transient ethercat error no longer rejects the whole poll cycle and tears down a healthy runtime connection. - ethercat/index.tsx (handleAddSelected): rebuild the scanned-devices selection as `prev minus added` instead of replacing it with the unmatched set so already-configured selections and concurrent state changes are preserved. - Extract the duplicated EtherCAT stats UI from board.tsx and orchestrators-list.tsx into ethercat-stats-section.tsx plus a normalizeEthercatStatus() helper (with unit-test suite). The two call sites consume className/cardsClassName/withSectionId props for their layout variants. Net -171 lines across the two callers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror of openplc-web@61fea7cb to keep the shared frontend surface byte-identical: - ethercat/index.tsx: rebuild the scanned-device selection cleanup so it iterates newDevices directly with a `position !== undefined` guard, fixing the tsc error that broke the web build CI on the earlier mirror commit. - discovered-device-table, ethercat-stats-section: prettier-formatted to satisfy the format CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror of openplc-web@ac7bb264: - board.tsx: gate setIncludeEthercatStatsInPolling(true) behind isOpenPLCRuntimeTarget(currentBoardInfo) so non-runtime board flows don't kick the EtherCAT poll on every cycle. - ethercat-stats-section: slugify the bus name and append the index when composing each section's DOM id, so runtime-supplied names with spaces/special chars produce valid + unique ids. - Add a vitest suite for useRuntimePolling covering the four EtherCAT branches (flag-off clears, missing optional method skips, success writes payload, transient rejection no longer tears down the poll). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/frontend/components/_features/[workspace]/editor/device/ethercat/index.tsx (2)
4-4: 🏗️ Heavy liftAvoid adding another
@root/backend/*dependency to this frontend component.Pulling
matchDevicesToRepositorystraight from the backend layer deepens the frontend→backend coupling here. Please expose this through a frontend-safe shared module or a port instead of importing backend code directly.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 at line 4, The component currently imports matchDevicesToRepository from a backend module which violates the frontend-only import rule; replace that direct import by depending on a frontend-safe port or injected interface (e.g., an IDeviceMatcher or a frontend/shared/ethercat adapter) and update the consumer to call the injected matcher instead of matchDevicesToRepository; implement the adapter/port in the backend bootstrap (or Electron bridge) to forward to the existing matchDevicesToRepository implementation and wire it into the component via props or a DI/context provider so the frontend code no longer directly imports backend modules.
593-675: ⚡ Quick winLocalize the new unmatched-device modal copy.
This modal adds a lot of new visible strings and button labels, but they are still hardcoded in the component.
As per coding guidelines: "
src/frontend/**/*.{ts,tsx}: Usei18nextfor internationalization in frontend components`."🤖 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 593 - 675, The modal renders many hardcoded user-facing strings (title, paragraph, table headers, "How to fix" block, button labels) in the EtherCAT component (references: unmatchedAddAttempt, Modal, ModalContent, ModalTitle, setActiveTab); replace them with i18next translations by importing useTranslation (react-i18next), calling const { t } = useTranslation('ethercat' /* or appropriate namespace */), then swap each literal with t('key') (use t('missingESIXml.title'), t('missingESIXml.description', { count: unmatchedAddAttempt.length }) for pluralization, t('table.pos'), t('table.name'), t('table.vendor'), t('table.product'), t('howToFix.title'), t('howToFix.step1')..., and t('buttons.goToRepository') / t('buttons.ok')), and add corresponding keys to the translation resource files; ensure numeric/plural forms use the count option where needed.src/frontend/components/_features/[workspace]/editor/device/ethercat/components/ethercat-stats-section.tsx (1)
63-103: ⚡ Quick winLocalize the shared EtherCAT stats labels.
This component hardcodes the section heading and every card label, so both board and orchestrator views inherit the same localization gap.
As per coding guidelines: "
src/frontend/**/*.{ts,tsx}: Usei18nextfor internationalization in frontend components`."🤖 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/ethercat-stats-section.tsx around lines 63 - 103, The component EthercatStatsSection currently hardcodes all labels; import and use i18next's React hook (const { t } = useTranslation()) at the top of the component and replace every hardcoded string (e.g., "EtherCAT Statistics", "Master State", "Slave Count", "Cycle Count", "WKC Errors", "consecutive:", "Cycle Time (avg)", "us", "max:", "Max Exchange Time", "Recovery Attempts") with t(...) calls using clear keys (e.g., t('ethercat.stats.title'), t('ethercat.stats.masterState'), etc.), keeping numeric values from master and master.metrics as-is; ensure repeated small units/words like "us" and "consecutive:" have their own keys; update any JSX spans that render those labels to use the t() values so both board and orchestrator views get localized.
🤖 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/frontend/components/_features/`[workspace]/editor/device/ethercat/components/discovered-device-table.tsx:
- Around line 73-98: The current <tr> uses role='button', tabIndex and key
handlers which breaks native table semantics; instead remove role='button',
tabIndex, aria-pressed and the onClick/onKeyDown from the <tr> and make the
Checkbox the interactive control: attach the selection handlers to the Checkbox
(use onClick and onKeyDown or onChange) to call
onSelectDevice(dm.device.position, !isSelected), set the Checkbox's checked to
isSelected and expose selection via aria-checked (not aria-hidden), and keep the
<tr> as a plain row for screen-reader/table navigation; reference the Checkbox
component, isSelected and onSelectDevice(dm.device.position, ...) when making
these changes.
In `@src/frontend/hooks/__tests__/use-runtime-polling.test.ts`:
- Around line 122-132: Update the test for useRuntimePolling to also exercise
the legacy flat success payload shape: when
runtimeMocks.getEthercatRuntimeStatus resolves with the legacy single-master
object, ensure the hook still calls storeMocks.setEthercatStatus with the legacy
payload. Specifically, in the test that sets
storeMocks.state.runtimeConnection.includeEthercatStatsInPolling and calls
renderHook(() => useRuntimePolling()), add or modify a sub-case where
runtimeMocks.getEthercatRuntimeStatus is mocked to return the legacy payload
(e.g., a single master object) and assert runtimeMocks.getEthercatRuntimeStatus
was called and storeMocks.setEthercatStatus received that legacy payload;
reference the test file use-runtime-polling.test.ts, the useRuntimePolling hook,
runtimeMocks.getEthercatRuntimeStatus, and storeMocks.setEthercatStatus to
locate where to add this coverage.
---
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/ethercat-stats-section.tsx:
- Around line 63-103: The component EthercatStatsSection currently hardcodes all
labels; import and use i18next's React hook (const { t } = useTranslation()) at
the top of the component and replace every hardcoded string (e.g., "EtherCAT
Statistics", "Master State", "Slave Count", "Cycle Count", "WKC Errors",
"consecutive:", "Cycle Time (avg)", "us", "max:", "Max Exchange Time", "Recovery
Attempts") with t(...) calls using clear keys (e.g., t('ethercat.stats.title'),
t('ethercat.stats.masterState'), etc.), keeping numeric values from master and
master.metrics as-is; ensure repeated small units/words like "us" and
"consecutive:" have their own keys; update any JSX spans that render those
labels to use the t() values so both board and orchestrator views get localized.
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/index.tsx:
- Line 4: The component currently imports matchDevicesToRepository from a
backend module which violates the frontend-only import rule; replace that direct
import by depending on a frontend-safe port or injected interface (e.g., an
IDeviceMatcher or a frontend/shared/ethercat adapter) and update the consumer to
call the injected matcher instead of matchDevicesToRepository; implement the
adapter/port in the backend bootstrap (or Electron bridge) to forward to the
existing matchDevicesToRepository implementation and wire it into the component
via props or a DI/context provider so the frontend code no longer directly
imports backend modules.
- Around line 593-675: The modal renders many hardcoded user-facing strings
(title, paragraph, table headers, "How to fix" block, button labels) in the
EtherCAT component (references: unmatchedAddAttempt, Modal, ModalContent,
ModalTitle, setActiveTab); replace them with i18next translations by importing
useTranslation (react-i18next), calling const { t } = useTranslation('ethercat'
/* or appropriate namespace */), then swap each literal with t('key') (use
t('missingESIXml.title'), t('missingESIXml.description', { count:
unmatchedAddAttempt.length }) for pluralization, t('table.pos'),
t('table.name'), t('table.vendor'), t('table.product'), t('howToFix.title'),
t('howToFix.step1')..., and t('buttons.goToRepository') / t('buttons.ok')), and
add corresponding keys to the translation resource files; ensure numeric/plural
forms use the count option where needed.
🪄 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: be69749d-ccba-4f87-ab90-29f9d456d5ee
📒 Files selected for processing (9)
src/frontend/components/_features/[workspace]/editor/device/configuration/board.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/discovered-device-table.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/ethercat-stats-section.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/index.tsxsrc/frontend/components/_features/[workspace]/editor/device/orchestrators/orchestrators-list.tsxsrc/frontend/hooks/__tests__/use-runtime-polling.test.tssrc/frontend/hooks/use-runtime-polling.tssrc/frontend/utils/__tests__/ethercat-status.test.tssrc/frontend/utils/ethercat-status.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/frontend/components/_features/[workspace]/editor/device/orchestrators/orchestrators-list.tsx
- src/frontend/hooks/use-runtime-polling.ts
Comment on lines
+122
to
+132
| it('writes the runtime payload into the store on a successful ethercat poll', async () => { | ||
| Object.assign(storeMocks.state.runtimeConnection as object, { includeEthercatStatsInPolling: true }) | ||
| const payload = { masters: [{ name: 'BusA', plugin_state: 'OPERATIONAL' }] } | ||
| runtimeMocks.getEthercatRuntimeStatus = vi.fn().mockResolvedValue({ success: true, data: payload }) | ||
|
|
||
| renderHook(() => useRuntimePolling()) | ||
| await flushAll() | ||
|
|
||
| expect(runtimeMocks.getEthercatRuntimeStatus).toHaveBeenCalledTimes(1) | ||
| expect(storeMocks.setEthercatStatus).toHaveBeenCalledWith(payload) | ||
| }) |
Contributor
There was a problem hiding this comment.
Add the legacy flat success payload here too.
This only exercises the modern { masters: [...] } response. PR feedback asked for success coverage of both runtime shapes, so a regression that stops storing the legacy single-master payload would still pass this suite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/hooks/__tests__/use-runtime-polling.test.ts` around lines 122 -
132, Update the test for useRuntimePolling to also exercise the legacy flat
success payload shape: when runtimeMocks.getEthercatRuntimeStatus resolves with
the legacy single-master object, ensure the hook still calls
storeMocks.setEthercatStatus with the legacy payload. Specifically, in the test
that sets storeMocks.state.runtimeConnection.includeEthercatStatsInPolling and
calls renderHook(() => useRuntimePolling()), add or modify a sub-case where
runtimeMocks.getEthercatRuntimeStatus is mocked to return the legacy payload
(e.g., a single master object) and assert runtimeMocks.getEthercatRuntimeStatus
was called and storeMocks.setEthercatStatus received that legacy payload;
reference the test file use-runtime-polling.test.ts, the useRuntimePolling hook,
runtimeMocks.getEthercatRuntimeStatus, and storeMocks.setEthercatStatus to
locate where to add this coverage.
Mirror of openplc-web@c746f7cc: - discovered-device-table: drop role='button'/tabIndex/aria-pressed/ onKeyDown from <tr>; restore native table-row semantics. Move selection interactivity onto the Checkbox (onCheckedChange, aria-label, click stopPropagation). Matches the existing pattern in esi-channels-table.tsx. - EtherCATRuntimeStatusResponse: drop the optional flat single-master fields. The runtime now natively reports masters[]; the inline shape was carried only for backward compat with a runtime build that never shipped. - normalizeEthercatStatus collapses to `status?.masters ?? []`; the fallback that synthesised a master from flat root fields is gone, and the test cases covering it follow. - runtime-status-panel.resolveMasterStatus loses its second branch and the surrounding doc-comments referring to the legacy shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/frontend/components/_features/[workspace]/editor/device/ethercat/components/discovered-device-table.tsx (2)
87-87: ⚡ Quick winLocalize the new selection and “No XML” copy.
The new
aria-label, tooltip, and badge text are hardcoded English strings. Please route them throughi18nextbefore this lands.As per coding guidelines, "Use i18next for internationalization in frontend components".
Also applies to: 95-104
🤖 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 87, Hardcoded English strings (the aria-label `Select device at position ${dm.device.position}`, the tooltip text and the badge "No XML") must be localized via i18next: import and call useTranslation() (or the existing t function) in the component (e.g., in DiscoveredDeviceTable) and replace the literal strings with t(...) calls using meaningful keys (pass dm.device.position into the aria-label key as an interpolation), and update the tooltip and badge text to use their own i18n keys; ensure keys are added to the appropriate translation namespace and that interpolation/HTML-escaping is used where needed.
66-70: ⚡ Quick winDerive
hasNoXmllocally and drop the backend helper here.This component only needs a zero-match check. Using
dm.matches.length === 0keeps the intent clearer and lets this frontend file stop depending on@root/backend/shared/....Suggested refactor
-import { getBestMatchQuality } from '@root/backend/shared/ethercat/device-matcher' @@ - const bestQuality = getBestMatchQuality(dm.matches) const bestMatch = dm.matches.length > 0 ? dm.matches[0] : null const displayName = bestMatch?.esiDevice?.name || dm.device.name - const hasNoXml = bestQuality === 'none' + const hasNoXml = dm.matches.length === 0As per coding guidelines, "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/discovered-device-table.tsx around lines 66 - 70, The frontend should stop depending on the backend helper for XML absence: instead of computing hasNoXml via getBestMatchQuality(dm.matches), derive it locally as a zero-match check (e.g., set hasNoXml based on dm.matches.length === 0), update any logic that relied on getBestMatchQuality in this component (like the hasNoXml variable and related display logic), and remove the import/usage of the backend/shared helper in discovered-device-table.tsx so the component only uses dm.matches, bestMatch, displayName, and selectedDevices.
🤖 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/frontend/components/_features/`[workspace]/editor/device/ethercat/components/discovered-device-table.tsx:
- Around line 43-46: The bulk-select header Checkbox (the Checkbox element using
props checked={someSelected && !allSelected ? 'indeterminate' : allSelected},
onCheckedChange={handleSelectAll}, disabled={deviceMatches.length === 0}) is
missing an accessible name; add an aria-label prop (for example
aria-label="Select all scanned devices") to that Checkbox so screen readers can
announce its purpose, keeping the existing checked, onCheckedChange, and
disabled behavior intact.
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/runtime-status-panel.tsx:
- Around line 80-84: When masterName is provided but not found we should not
fall back to another bus; in the code block in runtime-status-panel.tsx that
currently does "if (masterName) { const match = response.masters.find((m) =>
m.name === masterName) if (match) return match } return response.masters[0]",
change the logic so that if masterName is truthy and no match is found you
return null (instead of response.masters[0]); keep returning response.masters[0]
only when masterName is not provided. This affects the lookup using masterName,
match, and response.masters.
---
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/discovered-device-table.tsx:
- Line 87: Hardcoded English strings (the aria-label `Select device at position
${dm.device.position}`, the tooltip text and the badge "No XML") must be
localized via i18next: import and call useTranslation() (or the existing t
function) in the component (e.g., in DiscoveredDeviceTable) and replace the
literal strings with t(...) calls using meaningful keys (pass dm.device.position
into the aria-label key as an interpolation), and update the tooltip and badge
text to use their own i18n keys; ensure keys are added to the appropriate
translation namespace and that interpolation/HTML-escaping is used where needed.
- Around line 66-70: The frontend should stop depending on the backend helper
for XML absence: instead of computing hasNoXml via
getBestMatchQuality(dm.matches), derive it locally as a zero-match check (e.g.,
set hasNoXml based on dm.matches.length === 0), update any logic that relied on
getBestMatchQuality in this component (like the hasNoXml variable and related
display logic), and remove the import/usage of the backend/shared helper in
discovered-device-table.tsx so the component only uses dm.matches, bestMatch,
displayName, and selectedDevices.
🪄 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: faa9d175-e354-4b10-b4fc-c295d4e13921
📒 Files selected for processing (6)
src/frontend/components/_features/[workspace]/editor/device/ethercat/components/discovered-device-table.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/ethercat-stats-section.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/runtime-status-panel.tsxsrc/frontend/utils/__tests__/ethercat-status.test.tssrc/frontend/utils/ethercat-status.tssrc/types/ethercat/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/frontend/utils/ethercat-status.ts
- src/frontend/utils/tests/ethercat-status.test.ts
- src/frontend/components/_features/[workspace]/editor/device/ethercat/components/ethercat-stats-section.tsx
Comment on lines
43
to
+46
| <Checkbox | ||
| checked={someSelected && !allSelected ? 'indeterminate' : allSelected} | ||
| onCheckedChange={handleSelectAll} | ||
| disabled={selectableDevices.length === 0} | ||
| disabled={deviceMatches.length === 0} |
Contributor
There was a problem hiding this comment.
Add an accessible name to the bulk-select checkbox.
This control is still unlabeled, so screen readers will announce an unnamed checkbox in the header. Please add an aria-label such as “Select all scanned devices”.
Suggested fix
<Checkbox
checked={someSelected && !allSelected ? 'indeterminate' : allSelected}
onCheckedChange={handleSelectAll}
+ aria-label='Select all scanned devices'
disabled={deviceMatches.length === 0}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Checkbox | |
| checked={someSelected && !allSelected ? 'indeterminate' : allSelected} | |
| onCheckedChange={handleSelectAll} | |
| disabled={selectableDevices.length === 0} | |
| disabled={deviceMatches.length === 0} | |
| <Checkbox | |
| checked={someSelected && !allSelected ? 'indeterminate' : allSelected} | |
| onCheckedChange={handleSelectAll} | |
| aria-label='Select all scanned devices' | |
| disabled={deviceMatches.length === 0} | |
| /> |
🤖 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
around lines 43 - 46, The bulk-select header Checkbox (the Checkbox element
using props checked={someSelected && !allSelected ? 'indeterminate' :
allSelected}, onCheckedChange={handleSelectAll}, disabled={deviceMatches.length
=== 0}) is missing an accessible name; add an aria-label prop (for example
aria-label="Select all scanned devices") to that Checkbox so screen readers can
announce its purpose, keeping the existing checked, onCheckedChange, and
disabled behavior intact.
Comment on lines
+80
to
+84
| if (masterName) { | ||
| const match = response.masters.find((m) => m.name === masterName) | ||
| if (match) return match | ||
| } | ||
|
|
||
| // Fallback: flat fields (single-master backward compat) | ||
| if (response.plugin_state && response.slaves && response.metrics) { | ||
| return { | ||
| name: masterName ?? 'default', | ||
| plugin_state: response.plugin_state, | ||
| slave_count: response.slave_count ?? 0, | ||
| expected_wkc: response.expected_wkc ?? 0, | ||
| slaves: response.slaves, | ||
| metrics: response.metrics, | ||
| } | ||
| } | ||
|
|
||
| return null | ||
| return response.masters[0] |
Contributor
There was a problem hiding this comment.
Avoid falling back to another bus when masterName is specified.
If masterName is provided but missing, returning response.masters[0] can display the wrong bus diagnostics in this panel. Return null in that path so the UI can show “Waiting for status...” instead of mismatched data.
Suggested fix
function resolveMasterStatus(
response: EtherCATRuntimeStatusResponse,
masterName?: string,
): EtherCATMasterStatus | null {
if (!response.masters || response.masters.length === 0) return null
if (masterName) {
const match = response.masters.find((m) => m.name === masterName)
- if (match) return match
+ return match ?? null
}
return response.masters[0]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (masterName) { | |
| const match = response.masters.find((m) => m.name === masterName) | |
| if (match) return match | |
| } | |
| // Fallback: flat fields (single-master backward compat) | |
| if (response.plugin_state && response.slaves && response.metrics) { | |
| return { | |
| name: masterName ?? 'default', | |
| plugin_state: response.plugin_state, | |
| slave_count: response.slave_count ?? 0, | |
| expected_wkc: response.expected_wkc ?? 0, | |
| slaves: response.slaves, | |
| metrics: response.metrics, | |
| } | |
| } | |
| return null | |
| return response.masters[0] | |
| if (masterName) { | |
| const match = response.masters.find((m) => m.name === masterName) | |
| return match ?? null | |
| } | |
| return response.masters[0] |
🤖 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/runtime-status-panel.tsx
around lines 80 - 84, When masterName is provided but not found we should not
fall back to another bus; in the code block in runtime-status-panel.tsx that
currently does "if (masterName) { const match = response.masters.find((m) =>
m.name === masterName) if (match) return match } return response.masters[0]",
change the logic so that if masterName is truthy and no match is found you
return null (instead of response.masters[0]); keep returning response.masters[0]
only when masterName is not provided. This affects the lookup using masterName,
match, and response.masters.
End-to-end fix for the empty Startup Parameters tab on devices whose ESI uses the <DefaultData> hex-LE convention (Beckhoff Slave Information Tool style) instead of <DefaultValue>: - esi-parser-main.ts: read <DefaultData> as a fallback wherever the parser was reading <DefaultValue> (DataType subitems, object-level default, and the Object Info subitem override). Without this every default from a Beckhoff-style ESI silently became undefined, which caused extractDefaultSdoConfigurations to drop every entry. - sdo-config-defaults.ts: skip PDO-mapped objects (0x6xxx / 0x7xxx inputs/outputs are streamed cyclically, never configured at startup), and decode <DefaultData> hex-LE wire bytes into a type-aware display string per industry convention -- decimal for SINT/INT/DINT/.../USINT/ UINT/UDINT, hex 0xNN for bitmask types BYTE/WORD/DWORD/LWORD, TRUE/FALSE for BOOL, IEEE-754 decimal for REAL/LREAL. - generate-ethercat-config.ts: parseNumericValue now recognises TRUE/FALSE (case-insensitive) so the BOOL defaults the decoder produces don't collapse to 0 in the runtime config. - sdo-parameters-table.tsx: drop the silent clamp on blur and replace it with reactive validation that shows a red border + tooltip when a value is unparseable or out of range for its declared data type. The raw user input still propagates so the project file reflects what was typed; the runtime's strict_sdo gate catches anything that slips past the visual warning. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/frontend/components/_features/[workspace]/editor/device/ethercat/components/sdo-parameters-table.tsx (1)
92-107: 💤 Low valueuseMemo dependency on
rangedefeats memoization.
getDataTypeRangereturns a new object reference on every render, causing thevalidationuseMemo to recompute on every render despite inputs being unchanged. Either memoizerangeseparately or use the primitive dependencies directly.♻️ Suggested fix
- const range = getDataTypeRange(entry.dataType, entry.bitLength) - - const validation = useMemo<{ valid: boolean; message?: string }>(() => { + const validation = useMemo<{ valid: boolean; message?: string }>(() => { + const range = getDataTypeRange(entry.dataType, entry.bitLength) if (localValue.trim() === '') return { valid: true } const num = parseUserInput(localValue) if (isNaN(num)) { return { valid: false, message: `Cannot parse "${localValue}" as ${entry.dataType}` } } if (range && (num < range.min || num > range.max)) { return { valid: false, message: `${num} out of range for ${entry.dataType} (allowed: ${range.min} .. ${range.max})`, } } return { valid: true } - }, [localValue, entry.dataType, range]) + }, [localValue, entry.dataType, entry.bitLength])🤖 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/sdo-parameters-table.tsx around lines 92 - 107, getDataTypeRange returns a fresh object each render so including the derived "range" object in the validation useMemo dependencies defeats memoization; fix by memoizing range itself (e.g., wrap getDataTypeRange(entry.dataType, entry.bitLength) in useMemo) or by removing range from the validation deps and depending only on primitives (entry.dataType and entry.bitLength) and/or range.min/range.max values; update the code around the range variable and the validation useMemo so validation depends on stable primitives or the memoized range (referencing getDataTypeRange, the range constant, and the validation useMemo).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/sdo-parameters-table.tsx:
- Around line 92-107: getDataTypeRange returns a fresh object each render so
including the derived "range" object in the validation useMemo dependencies
defeats memoization; fix by memoizing range itself (e.g., wrap
getDataTypeRange(entry.dataType, entry.bitLength) in useMemo) or by removing
range from the validation deps and depending only on primitives (entry.dataType
and entry.bitLength) and/or range.min/range.max values; update the code around
the range variable and the validation useMemo so validation depends on stable
primitives or the memoized range (referencing getDataTypeRange, the range
constant, and the validation useMemo).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8c21e77d-e2b8-4f16-a1c3-a118c22f521c
📒 Files selected for processing (4)
src/backend/shared/ethercat/esi-parser-main.tssrc/backend/shared/ethercat/generate-ethercat-config.tssrc/backend/shared/ethercat/sdo-config-defaults.tssrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/sdo-parameters-table.tsx
Run prettier on esi-parser-main.ts and sdo-config-defaults.ts to fix the line-break style flagged by `format / Format Check` on PR #741. No behavioural changes; long expressions just collapse back to single lines under the project's print width. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ESIs may declare configurable RW SDOs without a vendor default, expecting the operator to supply one. The extractor now surfaces those entries with an empty default so they are visible in the UI (commit f8fa19a). The exporter must skip them when the operator left them blank, otherwise parseNumericValue('') silently coerces to 0 and the slave receives a wrong SDO write -- 0 is an unsafe value for many parameters (motor torque limits, axis IDs, fault reactions). Filtering at export keeps the slave's internal default in effect when the operator did not configure the entry, matching what TwinCAT and CODESYS do. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror of openplc-web's surface refactor so the cross-repo surface stays byte-identical. Same change: src/types/ethercat/ -> src/middleware/ shared/ports/ethercat-types.ts. Editor-only consumers in main/modules/ ipc/ also updated to the new path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror of openplc-web's autofix so the cross-repo surface stays byte- identical. simple-import-sort/imports flagged the same three files on both sides. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Editor-only file missed in the previous import-sort pass; ethercat- types must precede types alphabetically. Pure mechanical fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror of openplc-web's validation gate. The validator is shared surface (byte-identical with web) and ensures two EtherCAT masters cannot share the same network interface before the config reaches the runtime. Editor-only wiring lands in compiler-module's handleGenerateEthercatConfig: when the validator returns errors, throw with the joined messages so the upstream catch surfaces a single diagnostic line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Mirrors the four shared surfaces from openplc-web's
feat/ethercat-web-adapterbranch and brings the new EtherCAT statistics card grid to the desktop.The headline feature is EtherCAT Statistics on the Device screens (Orchestrators view + Configuration view), rendered alongside the existing Scan Cycle Statistics in the same card style. Each EtherCAT master gets its own section labelled
EtherCAT Statistics — <bus name>so users can tell which bus the metrics belong to when more than one is configured.Paired PR
openplc-web#372 — same branch name, same base (
development), same shared-surface diff. Thesurface-syncjob should pair this PR with #372 automatically and pass once both branches are in lockstep.What's in this PR
sync(ethercat): mirror openplc-web feat/ethercat-web-adapter(2fc42e7)ethercatStatus+includeEthercatStatsInPollingfields on the device store slice, plus matching setters and disconnect cleanup. Tests cover defaults, setters, and cleanup; slice keeps 100% coverage.useRuntimePollingnow opts intogetEthercatRuntimeStatus()on the same 2s cycle as PLC status/logs (singlePromise.all). Soft-fails on transient errors so the UI doesn't flicker.orchestrators-list.tsxenables the polling flag on mount, normalises the runtime's two response shapes (masters[]and the flat single-master legacy form) into one array, and renders one card grid per master.runtime-status-panel.tsxdrops the duplicated cycle-metric cards (and the localMetricCardhelper). Panel now focuses on master state header + per-slave diagnostics table.discovered-device-table.tsx"No XML" badge switched to neutral gray;ethercat/index.tsxnarrowedunmatched/unmatchedAddAttempttoScannedDeviceMatch['device'][], dropped unusedgetBestMatchQualityimport, prettier reformat;ethercat-device-editor.tsxpromotes "Channel Mappings" to first/default tab.sync(ethercat): mirror Configuration screen stats from openplc-web(476d8cb)board.tsx(Device → Configuration). Desktop users hit Configuration as part of their normal flow when picking a device and connecting; this is where they expect the stats. Lives inside the existingisOpenPLCRuntimeTargetbranch, after the Scan Cycle Statistics block. Both polling effects coexist on the same screen via independentuseEffects.Notes
src/middleware/adapters/editor/) already implementsgetEthercatRuntimeStatus()so no editor-side adapter changes were needed.surface-syncchecks:match=true, total_files=876, total_diffs=0.Test plan
surface-syncjob passes by pairing with openplc-web#372deps-sync,script-sync,tooling-syncjobs pass (no changes to those areas)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements