feat(plcopen): PLCopen XML import parser + import/export UI (NODE-111, NODE-112) - #936
Merged
Merged
Conversation
…UI (NODE-111, NODE-112) - Port the PLCopen XML->project parser (textual POUs, dataTypes, task/instance config, FBD and LD graphical bodies) from autonomy-node's git history into the shared frontend/utils/PLC/xml-parser/ tree, retargeted to this app's native xyflow-based project model. - Wire "Export as PLCopen XML" (previously a disabled stub) and a new "Import PLCopen XML" menu item, backed by new ProjectPort methods (pickPlcopenImportFile, exportPlcopenFile) and a confirm-overwrite modal. - Editor adapter: implement the two new port methods via Electron IPC (native open/save file dialogs filtered to .xml). - Fix a pre-existing export bug where an unnamed ladder variable node was dropped from generated XML while a <connection> still referenced it. - Exclude three new shared test files from this repo's Jest run (testPathIgnorePatterns) that rely on vi.mock with a relative module path, a pre-existing Jest/vi-shim limitation already worked around for notify-no-write-permission.test.ts; these run correctly under web's Vitest. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughPLCopen XML import and export are added across parsing, project-state conversion, filesystem dialogs, IPC bridges, frontend services, menus, and confirmation UI. LD/FBD graph parsing, round-trip coverage, and handling for unsupported or dangling XML connections are included. ChangesPLCopen XML workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant FileMenu
participant FrontendService
participant ProjectPort
participant IPC
participant FilePicker
User->>FileMenu: choose PLCopen import or export
FileMenu->>FrontendService: execute import or export action
FrontendService->>ProjectPort: request XML file operation
ProjectPort->>IPC: invoke PLCopen channel
IPC->>FilePicker: read or write XML
FilePicker-->>IPC: return operation result
IPC-->>ProjectPort: return content or error
ProjectPort-->>FrontendService: complete workflow
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 3
🧹 Nitpick comments (2)
src/frontend/locales/en/menu.json (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNormalize the capitalization of PLCopen.
For consistency with the new
Import PLCopen XMLstring, you might want to consider updating the export string above it fromPLCOpentoPLCopen.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/locales/en/menu.json` at line 25, Update the adjacent export menu translation to use “PLCopen” instead of “PLCOpen,” matching the capitalization of the importFromPLCOpenXml label and the requested naming convention.src/frontend/utils/PLC/xml-parser/language/__tests__/ladder-xml.test.ts (1)
185-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
@formalParameter-less and empty-string connection cases.Every
connectionfixture in this file sets a non-empty@formalParameter(Lines 30, 42, 53, 212, 263, 294). Two documented branches inparseConnectionXml(ladder-xml.tsL64-74) and its consumer fallback (ladder-xml.tsL519) are never exercised:
- A connection with no
@formalParameterattribute at all (the "block input wired directly to an<inVariable>" case the code comments describe).- A connection with
@formalParameter=""referencing an unnamed function's return pin.As per coding guidelines, "Code in
src/frontend/utils/must maintain 100% test coverage when new code is added."✅ Suggested additional test cases
it('defaults the source handle to "output" when a connection omits `@formalParameter`', () => { const { body, warnings } = parseLadderXml('p', { inVariable: [ { '`@localId`': '1', '`@width`': '80', '`@height`': '30', position: { '`@x`': '0', '`@y`': '0' }, connectionPointOut: { relPosition: { '`@x`': '80', '`@y`': '15' } }, expression: 'LIT', }, ], block: [ { '`@localId`': '2', '`@typeName`': 'ADD', '`@executionOrderId`': '0', '`@width`': '100', '`@height`': '60', position: { '`@x`': '100', '`@y`': '0' }, inputVariables: { variable: [ { '`@formalParameter`': 'IN1', connectionPointIn: { relPosition: { '`@x`': '0', '`@y`': '10' }, connection: [{ '`@refLocalId`': '1' }] } }, ], }, outputVariables: '', }, ], }) expect(warnings).toEqual([]) expect(body.rungs[0].edges).toContainEqual(expect.objectContaining({ sourceHandle: 'output', targetHandle: 'IN1' })) }) it("resolves a connection referencing an unnamed function's return pin (`@formalParameter`=\"\")", () => { const { body } = parseLadderXml('p', { block: [ { '`@localId`': '1', '`@typeName`': 'ADD', '`@executionOrderId`': '0', '`@width`': '100', '`@height`': '60', position: { '`@x`': '0', '`@y`': '0' }, inputVariables: '', outputVariables: { variable: [{ '`@formalParameter`': '', connectionPointOut: { relPosition: { '`@x`': '100', '`@y`': '10' } } }] } }, ], coil: [ { '`@localId`': '2', '`@width`': '40', '`@height`': '40', position: { '`@x`': '150', '`@y`': '0' }, connectionPointIn: { relPosition: { '`@x`': '0', '`@y`': '20' }, connection: [{ '`@refLocalId`': '1', '`@formalParameter`': '' }] }, connectionPointOut: { relPosition: { '`@x`': '40', '`@y`': '20' } }, variable: ['Y'] }, ], }) expect(body.rungs[0].edges).toContainEqual(expect.objectContaining({ sourceHandle: 'OUT', target: 'COIL-2' })) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/utils/PLC/xml-parser/language/__tests__/ladder-xml.test.ts` around lines 185 - 227, Add tests covering both untested parseConnectionXml branches: a connection without `@formalParameter` from an inVariable to a block input must produce sourceHandle "output", and a connection with `@formalParameter`="" from an unnamed block output to a coil must resolve sourceHandle "OUT". Keep the assertions for empty warnings where applicable and verify the expected target handles or edge endpoints.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jest.config.json`:
- Around line 18-24: Remove the newly added test paths from Jest’s
testPathIgnorePatterns so they are discovered and executed by the test suite. If
any cannot run because of environment or mocking issues, update the
corresponding test files to explicitly skip them rather than excluding them in
jest.config.json.
In `@src/frontend/services/export-actions.ts`:
- Around line 17-22: Reorder the imports in export-actions.ts to match the
repository’s lint rules, preserving the existing import grouping and symbols;
run the lint autofix or formatter to confirm the exact order.
In `@src/middleware/shared/ports/platform-capabilities.ts`:
- Around line 154-157: Keep hasProjectExport and hasProjectImport set to false
in WEB_CAPABILITIES until the browser-based port adapters are implemented,
preventing these actions from being exposed in the web UI prematurely.
---
Nitpick comments:
In `@src/frontend/locales/en/menu.json`:
- Line 25: Update the adjacent export menu translation to use “PLCopen” instead
of “PLCOpen,” matching the capitalization of the importFromPLCOpenXml label and
the requested naming convention.
In `@src/frontend/utils/PLC/xml-parser/language/__tests__/ladder-xml.test.ts`:
- Around line 185-227: Add tests covering both untested parseConnectionXml
branches: a connection without `@formalParameter` from an inVariable to a block
input must produce sourceHandle "output", and a connection with
`@formalParameter`="" from an unnamed block output to a coil must resolve
sourceHandle "OUT". Keep the assertions for empty warnings where applicable and
verify the expected target handles or edge endpoints.
🪄 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: 819dda0f-411a-410c-981d-efcecafa6d20
📒 Files selected for processing (47)
jest.config.jsonsrc/__architecture__/validate.tssrc/backend/editor/utils/__tests__/path-picker.test.tssrc/backend/editor/utils/path-picker.tssrc/frontend/components/_molecules/menu-bar/menus/file.tsxsrc/frontend/components/_organisms/modals/__tests__/confirm-plcopen-import-modal.test.tsxsrc/frontend/components/_organisms/modals/confirm-plcopen-import-modal.tsxsrc/frontend/components/_templates/app-layout.tsxsrc/frontend/locales/en/menu.jsonsrc/frontend/services/__tests__/export-actions.test.tssrc/frontend/services/__tests__/import-actions.test.tssrc/frontend/services/export-actions.tssrc/frontend/services/import-actions.tssrc/frontend/store/slices/modal/types.tssrc/frontend/utils/PLC/build-plcopen-project-response.tssrc/frontend/utils/PLC/xml-generator/old-editor/language/__tests__/ladder-xml.test.tssrc/frontend/utils/PLC/xml-generator/old-editor/language/ladder-xml.tssrc/frontend/utils/PLC/xml-parser/__tests__/data-type-xml.test.tssrc/frontend/utils/PLC/xml-parser/__tests__/instances-xml.test.tssrc/frontend/utils/PLC/xml-parser/__tests__/parse-plcopen-xml.test.tssrc/frontend/utils/PLC/xml-parser/__tests__/parse-xml-document.test.tssrc/frontend/utils/PLC/xml-parser/__tests__/pou-xml.test.tssrc/frontend/utils/PLC/xml-parser/__tests__/type-xml.test.tssrc/frontend/utils/PLC/xml-parser/__tests__/variable-xml.test.tssrc/frontend/utils/PLC/xml-parser/__tests__/xml-node.test.tssrc/frontend/utils/PLC/xml-parser/data-type-xml.tssrc/frontend/utils/PLC/xml-parser/index.tssrc/frontend/utils/PLC/xml-parser/instances-xml.tssrc/frontend/utils/PLC/xml-parser/language/__tests__/fbd-xml.test.tssrc/frontend/utils/PLC/xml-parser/language/__tests__/geometry.test.tssrc/frontend/utils/PLC/xml-parser/language/__tests__/ladder-xml.test.tssrc/frontend/utils/PLC/xml-parser/language/fbd-xml.tssrc/frontend/utils/PLC/xml-parser/language/geometry.tssrc/frontend/utils/PLC/xml-parser/language/ladder-xml.tssrc/frontend/utils/PLC/xml-parser/parse-xml-document.tssrc/frontend/utils/PLC/xml-parser/pou-xml.tssrc/frontend/utils/PLC/xml-parser/type-xml.tssrc/frontend/utils/PLC/xml-parser/variable-xml.tssrc/frontend/utils/PLC/xml-parser/xml-node.tssrc/frontend/utils/__tests__/iec-types-registry.test.tssrc/frontend/utils/iec-types-registry.tssrc/main/modules/ipc/main.tssrc/main/modules/ipc/renderer.tssrc/middleware/adapters/editor/__tests__/project-adapter.test.tssrc/middleware/adapters/editor/project-adapter.tssrc/middleware/shared/ports/platform-capabilities.tssrc/middleware/shared/ports/project-port.ts
closeModal() only resets modals listed in ALL_MODAL_TYPES, so the File -> Import PLCopen XML confirm dialog stayed open after a successful import since its type was missing from that list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
Author
PR #936 ReviewSummaryEditor-side counterpart to openplc-web #601 (same byte-identical shared surface: parser tree, services, modal, ports). Adds the editor-specific IPC plumbing (native open/save dialogs) and adapter wiring for PLCopen export/import. Module: Editor-specific (backend/editor/utils/path-picker.ts, main/modules/ipc/{main,renderer}.ts, middleware/adapters/editor/project-adapter.ts)Checklist Results
Code Improvement FindingsNone. Module: Shared (byte-identical with openplc-web #601)Already reviewed there — no repo-specific findings to add. Cross-Module ObservationsNone beyond what's noted in #601. Verdict
|
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 tasks
Co-Authored-By: Claude Sonnet 5 <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
1 participant
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
Editor-side half of NODE-95 (Weidmüller: PLCopen import/export). autonomy-node's backend already owns PLCopen export and raw-import storage (a
.xmlupload is stored verbatim, flagged pending conversion); this PR adds the actual XML↔project conversion, which belongs here since this app owns the native project model.src/frontend/utils/PLC/xml-parser/tree, mirroring the existingxml-generator/(export) layout. The parser was originally prototyped against autonomy-node's own domain model, proven there, then ported to this repo's native xyflow-based types (see NODE-111 for the reference implementation history).ProjectPortmethods..xml).<connection>elsewhere still referenced it (dangling reference).Affected Surfaces
frontend/utils/PLC/xml-parser/(new),frontend/utils/PLC/xml-generator/old-editor/language/ladder-xml.ts(bug fix),frontend/services/{export,import}-actions.ts(new),frontend/components/_organisms/modals/confirm-plcopen-import-modal.tsx(new),frontend/components/_molecules/menu-bar/menus/file.tsx,middleware/shared/ports/{project-port,platform-capabilities}.ts,__architecture__/validate.ts(newKNOWN_EXCEPTIONSentry)backend/editor/utils/path-picker.ts,main/modules/ipc/{main,renderer}.ts,middleware/adapters/editor/project-adapter.ts,jest.config.json(excludes 3 shared test files that hit a pre-existing Jest/vi-shim relative-mock-path limitation, same workaround already applied tonotify-no-write-permission.test.ts)Validation
npx tsx src/__architecture__/validate.ts)npm run test— all suites pass (coverage-threshold shortfalls inbackend/shared/middleware/adapters/editorare pre-existing ondevelopment, confirmed via side-by-side run)npm run lint— 0 errorscompare_repos.py— 1168 files, all identical)Cross-repo
Generated with Claude Code
Summary by CodeRabbit