fix(architecture): document backend-shared exception for SoftMotion axis helpers - #937
Conversation
…xis helpers PR #928 introduced imports of generate-softmotion.ts's pure axis-naming helpers (sanitizeAxisName, softMotionAxisNames, serializeSoftMotionAxisGlobalsToST, isValidIecIdentifier) from three services/store files, violating the services/store -> backend-shared layer rule and failing `validate:arch` on development for every PR since. Documented as KNOWN_EXCEPTIONS entries, mirroring the existing ladder-slice/fbd-slice precedent: the pure discovery logic isn't duplicated, only reached across a layer boundary those files can't move. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
WalkthroughChangesSoftMotion axis pipeline
Surface comparison filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PLCProject
participant collectAxes
participant generateSoftMotionArtifacts
participant UserPOUs
participant PLCTasks
PLCProject->>collectAxes: discover configured CiA 402 axes
collectAxes-->>generateSoftMotionArtifacts: return axis plans
generateSoftMotionArtifacts->>PLCProject: add globals and bridge POU
generateSoftMotionArtifacts->>UserPOUs: inject axis externals
generateSoftMotionArtifacts->>PLCTasks: prepend bridge instance
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
PR #937 ReviewSummaryAdds three Checklist Results
Verdict
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/__architecture__/validate.ts (1)
282-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestrict these exceptions to the intended import path.
Because
validate()approves exceptions by destination layer only (exceptions.includes(toLayer)), each entry currently permits any futurebackend-sharedimport from these files—not just the documentedgenerate-softmotionhelpers. Match the exception against the specific module path, or add a regression test that rejects unrelated backend-shared imports.🤖 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/__architecture__/validate.ts` around lines 282 - 293, Update validate() exception handling so the listed frontend files permit only their documented generate-softmotion/backend-shared module imports, not every backend-shared destination. Match exceptions against the specific import module path while preserving the existing layer validation for other entries, and add or update regression coverage to reject unrelated backend-shared imports.
🤖 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.
Nitpick comments:
In `@src/__architecture__/validate.ts`:
- Around line 282-293: Update validate() exception handling so the listed
frontend files permit only their documented generate-softmotion/backend-shared
module imports, not every backend-shared destination. Match exceptions against
the specific import module path while preserving the existing layer validation
for other entries, and add or update regression coverage to reject unrelated
backend-shared imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c66b8b14-073c-44f9-a720-4b911155e33b
📒 Files selected for processing (1)
src/__architecture__/validate.ts
…Motion axis helpers" This reverts commit b1a60bf.
…ed/utils PR #928 introduced imports of generate-softmotion.ts's pure axis-naming helpers (sanitizeAxisName, softMotionAxisNames, serializeSoftMotionAxisGlobalsToST, isValidIecIdentifier) and cia402.ts directly from three frontend/services/ and frontend/store/ files, plus two frontend/components/ call sites (missed by the checker's @root/ alias blind spot) — all violating the services/store/components -> backend-shared layer rule and failing validate:arch on development for every PR since. Rather than special-casing the violation with a KNOWN_EXCEPTIONS entry, relocated the pure logic to middleware/shared/utils/ethercat/ — the existing home for domain logic reachable from every layer including the compile pipeline (same relationship target-capabilities/ and library/ already have to backend/shared/compile/pipeline.ts). backend/shared/ethercat/ generate-softmotion.ts now only owns the actual codegen (generateSoftMotionArtifacts/injectAxisExternals), importing collectAxes from the new utils module. Also fixed a pre-existing prettier violation from the same PR (preprocess-pous.test.ts) that was failing the format check on development. (openplc-web's matching softmotion-e2e.test.ts fix does not apply here — that test is web-adapter-specific and doesn't exist in this repo.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
compare-surfaces.py required byte-identical test files across both repos, same as program files. This is stricter than necessary and already broken in practice: openplc-web's backend/shared/ethercat/__tests__/softmotion-e2e.test.ts (added by #928) is genuinely web-adapter-specific (imports middleware/adapters/web/transpile-from-port + bundled-stlibs, neither of which exists in this repo) and has never had an editor counterpart — this has been failing the required Shared Surface Sync check on every PR since #928 merged, independent of anything in this branch. Test files are allowed to diverge (platform-specific mocks, adapter-only e2e suites) without indicating a real program-file drift — the same exclusion the architecture validator (__architecture__/validate.ts) already applies to its own scan. Now only program files are compared. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR #937 Review (updated again)Additional fixSame as openplc-web #602: found that Updated Verdict
|
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 (1)
src/backend/shared/ethercat/generate-softmotion.ts (1)
147-166: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSchedule the bridge for every task that can execute axis-using POUs.
The generated instance is bound only to
tasks[0]. In multi-task projects, POUs on other tasks read stale axis feedback and their commands are not bridged on that task’s scan. Generate a uniquely named bridge instance per relevant task, preserving execution before that task’s user instances, and cover the multi-task case.🤖 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/backend/shared/ethercat/generate-softmotion.ts` around lines 147 - 166, Update the bridge-instance generation in the project transformation to create a uniquely named SM3 bridge instance for every task that can execute axis-using POUs, rather than binding only to tasks[0]. Prepend each generated bridge instance to that task’s user instances while preserving task-local execution order, retain the fallback-task behavior, and add coverage for multi-task projects.
🧹 Nitpick comments (1)
scripts/compare-surfaces.py (1)
76-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a tuple directly with
endswith.In Python,
str.endswith()natively accepts a tuple of strings, which is more concise and slightly faster than usingany()with a generator expression.♻️ Proposed refactor
- name = path.name - return any(name.endswith(suffix) for suffix in (".test.ts", ".test.tsx", ".spec.ts", ".spec.tsx")) + return path.name.endswith((".test.ts", ".test.tsx", ".spec.ts", ".spec.tsx"))🤖 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 `@scripts/compare-surfaces.py` around lines 76 - 77, Update the suffix check in the visible path-filtering function to pass the existing suffix tuple directly to name.endswith, removing the any() generator while preserving the same test and spec extensions.
🤖 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 `@src/middleware/shared/utils/ethercat/softmotion-axis-naming.ts`:
- Around line 45-59: Update sanitizeAxisName and isValidIecIdentifier to reject
IEC 61131-3 reserved words case-insensitively, including PROGRAM, VAR, and
END_VAR. Apply the reserved-word check before accepting or returning an axis
name, while preserving the existing character and leading-character validation
for non-reserved identifiers.
---
Outside diff comments:
In `@src/backend/shared/ethercat/generate-softmotion.ts`:
- Around line 147-166: Update the bridge-instance generation in the project
transformation to create a uniquely named SM3 bridge instance for every task
that can execute axis-using POUs, rather than binding only to tasks[0]. Prepend
each generated bridge instance to that task’s user instances while preserving
task-local execution order, retain the fallback-task behavior, and add coverage
for multi-task projects.
---
Nitpick comments:
In `@scripts/compare-surfaces.py`:
- Around line 76-77: Update the suffix check in the visible path-filtering
function to pass the existing suffix tuple directly to name.endswith, removing
the any() generator while preserving the same test and spec extensions.
🪄 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: 47abcc36-2813-4bea-9967-cb9aea4625bc
📒 Files selected for processing (15)
scripts/compare-surfaces.pysrc/backend/shared/ethercat/__tests__/generate-softmotion.test.tssrc/backend/shared/ethercat/enrich-device-data.tssrc/backend/shared/ethercat/generate-softmotion.tssrc/backend/shared/utils/PLC/__tests__/preprocess-pous.test.tssrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/cia402-axis-tab.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/index.tsxsrc/frontend/services/st-lsp/goto-definition-redirect.tssrc/frontend/services/st-lsp/project-sync.tssrc/frontend/store/slices/shared/slice.tssrc/middleware/shared/utils/ethercat/__tests__/cia402.test.tssrc/middleware/shared/utils/ethercat/__tests__/softmotion-axis-naming.test.tssrc/middleware/shared/utils/ethercat/cia402.tssrc/middleware/shared/utils/ethercat/index.tssrc/middleware/shared/utils/ethercat/softmotion-axis-naming.ts
💤 Files with no reviewable changes (1)
- src/backend/shared/ethercat/tests/generate-softmotion.test.ts
| /** Sanitize a device name into a valid IEC 61131-3 identifier. */ | ||
| export function sanitizeAxisName(name: string): string { | ||
| let s = name.replace(/[^A-Za-z0-9_]/g, '_') | ||
| if (!/^[A-Za-z_]/.test(s)) s = `_${s}` | ||
| return s | ||
| } | ||
|
|
||
| /** | ||
| * True when `name` is already a valid IEC 61131-3 identifier — a letter or | ||
| * underscore followed by letters, digits, or underscores. A SoftMotion drive's | ||
| * name IS the axis variable name used in `MC_*(Axis := <name>)`, so it must | ||
| * satisfy this (no spaces, hyphens, or leading digits). | ||
| */ | ||
| export function isValidIecIdentifier(name: string): boolean { | ||
| return /^[A-Za-z_][A-Za-z0-9_]*$/.test(name) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate existing IEC identifier/keyword validation and relevant tests.
rg -n -C3 --type=ts '\b(isValidIecIdentifier|reserved.*word|IEC.*keyword|END_VAR|VAR_GLOBAL)\b' srcRepository: Autonomy-Logic/openplc-editor
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- softmotion-axis-naming.ts ---'
sed -n '1,220p' src/middleware/shared/utils/ethercat/softmotion-axis-naming.ts
echo
echo '--- keyword-aware validators ---'
rg -n -C2 --type=ts 'keyword|reserved|identifier|isValidIecIdentifier' src/middleware src/backend src/frontend | head -n 200Repository: Autonomy-Logic/openplc-editor
Length of output: 23269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- exact file ---'
cat -n src/middleware/shared/utils/ethercat/softmotion-axis-naming.ts
echo
echo '--- potential keyword lists / reserved-word helpers ---'
rg -n --type=ts '\b(END_VAR|PROGRAM|FUNCTION_BLOCK|VAR_GLOBAL|VAR_INPUT|VAR_OUTPUT|VAR_IN_OUT|VAR_EXTERNAL|VAR_TEMP|CLASS|METHOD|ACTION|isReserved|reservedWords|keywords?)\b' src | head -n 200Repository: Autonomy-Logic/openplc-editor
Length of output: 33529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '388,410p' src/frontend/store/slices/shared/slice.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 1463
Reject IEC reserved words too. sanitizeAxisName and isValidIecIdentifier still allow identifiers like PROGRAM, VAR, and END_VAR, which can be emitted into VAR_GLOBAL and fail ST compilation. Add a case-insensitive reserved-word check before accepting the axis name.
🤖 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/middleware/shared/utils/ethercat/softmotion-axis-naming.ts` around lines
45 - 59, Update sanitizeAxisName and isValidIecIdentifier to reject IEC 61131-3
reserved words case-insensitively, including PROGRAM, VAR, and END_VAR. Apply
the reserved-word check before accepting or returning an axis name, while
preserving the existing character and leading-character validation for
non-reserved identifiers.
1 participant
Summary
development'sarchitecture / Architecture Validationcheck has been failing since PR #928 ("feat: CODESYS-compatible SoftMotion") merged: it added imports ofgenerate-softmotion.ts's pure axis-naming helpers (sanitizeAxisName,softMotionAxisNames,serializeSoftMotionAxisGlobalsToST,isValidIecIdentifier) from threefrontend/services/andfrontend/store/files, which the layer rules forbid importing frombackend-shareddirectly. Because branch protection isstrict(requires the branch up to date + all checks green), this currently blocks merging any PR intodevelopment.Found while shipping an unrelated PLCopen import/export PR (openplc-editor #936) — traced the failure to this pre-existing, unrelated violation. Matching fix in openplc-web: https://github.com/Autonomy-Logic/openplc-web/pull/602
Fix
Documented the three call sites as
KNOWN_EXCEPTIONSentries insrc/__architecture__/validate.ts, the same escape hatch already used for the ladder/FBD slice'scomponentsdependency. Each entry explains exactly which pure helper is needed and why. No behavior change —generate-softmotion.tsitself is untouched.Validation
npx tsx src/__architecture__/validate.ts— passes (0 violations)eslint src/__architecture__/validate.ts— cleanGenerated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests