sync(ladder): reimplement handle branches on ladder#750
Closed
dcoutinho1328 wants to merge 7 commits into
Closed
Conversation
Mirrors openplc-web feature/reimplement-handle-branches-on-ladder (PR #386) onto editor: - Persistent handle-branch index on rung state (Zod schema, slice, reconciliation hooks). - Drop-on-handle creation, compact branch placement next to FB, serial chaining + parallels (OR-paths) inside branches. - Layout: nested-parallel X cascade collapse, sibling-block vertical extent rolls up nested content, branch-specific BRANCH_BLOCK_SIDE_GAP for short FB-side wires, FB-shift anchored against any predecessor. - Compile-time wiring: blockToXml (codesys + old-editor) picks up edges targeting each secondary FB input handle so branch booleans reach the FB at runtime. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a denormalized per-handle "handleBranches" subsystem and threads it through types, Zod schemas, the ladder slice (new actions and normalization), layout pipeline, element add/remove/drag flows, node/edge utilities, components, duplication remapping, and XML export helpers. ChangesHandle Branches Feature Implementation
Sequence Diagram(s)sequenceDiagram
participant EditorUI as Editor UI
participant Add as addNewElement
participant Layout as updateDiagramElementsPosition
participant Slice as ladderFlowSlice
participant Store as Redux Store
EditorUI->>Add: drop node / add element
Add->>Layout: compute nodes, edges, handleBranches
Layout->>Slice: return layout result
Slice->>Store: updateRungData(nodes, edges, handleBranches)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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 |
Mirror of openplc-web fix: the ports barrel didn't re-export the ladder-branch types, so TS resolved them as unknown across the rung layer and the build / lint jobs lit up. Adding the two type re-exports clears the cascade; the autofix sweep removes type assertions that were only needed while the types were broken. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/edges.ts (1)
75-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
targetHandleandsourceHandlelose fallbacks when callers pass partial options.The ternary at lines 75–76 checks
!options(whether options exist) rather than per-key nullish checks. A caller providingsourceEdgeLookupHandlewithouttargetHandleorsourceHandlewill silently getundefinedfor both handles, breaking the auto-derivation from connectors. Update to use nullish coalescing:Proposed fix
- const targetHandle = !options ? targetNodeData.inputConnector?.id : options.targetHandle - const sourceHandle = !options ? targetNodeData.outputConnector?.id : options.sourceHandle + const targetHandle = options?.targetHandle ?? targetNodeData.inputConnector?.id + const sourceHandle = options?.sourceHandle ?? targetNodeData.outputConnector?.id🤖 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/components/_molecules/graphical-editor/ladder/rung/ladder-utils/edges.ts` around lines 75 - 76, The current ternaries check only !options and thus drop connector fallbacks when callers pass a partial options object; change the assignments for targetHandle and sourceHandle to use per-key nullish coalescing so each handle falls back to the node connector when its specific option is undefined (e.g., set targetHandle = options?.targetHandle ?? targetNodeData.inputConnector?.id and sourceHandle = options?.sourceHandle ?? targetNodeData.outputConnector?.id), updating the lines that assign targetHandle and sourceHandle accordingly.
🧹 Nitpick comments (7)
src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/drag-n-drop/index.ts (1)
116-118: ⚡ Quick winUse
findIndex+findinstead ofObject.entrieson an array.
Object.entries(rung.nodes)coerces the array's indices to string keys and forces the subsequenttoInteger(selectedPlaceholderIndex)cast to recover them. A direct array search reads better and keeps the type narrowing intact:♻️ Proposed refactor
- const [selectedPlaceholderIndex, selectedPlaceholder] = Object.entries(rung.nodes).find( - ([, n]) => (n.type === 'placeholder' || n.type === 'parallelPlaceholder') && n.selected, - ) ?? [undefined, undefined] - if (!selectedPlaceholder || selectedPlaceholderIndex === undefined) return null + const selectedPlaceholderIndex = rung.nodes.findIndex( + (n) => (n.type === 'placeholder' || n.type === 'parallelPlaceholder') && n.selected, + ) + if (selectedPlaceholderIndex === -1) return null + const selectedPlaceholder = rung.nodes[selectedPlaceholderIndex]…and drop the
toInteger(selectedPlaceholderIndex)call at line 135 along with thelodashimport if unused elsewhere.🤖 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/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/drag-n-drop/index.ts` around lines 116 - 118, Replace the Object.entries(...) usage with direct array searches: use rung.nodes.findIndex(...) to set selectedPlaceholderIndex and rung.nodes.find(...) to set selectedPlaceholder (matching the predicate used now for placeholder/parallelPlaceholder && n.selected); remove the subsequent toInteger(selectedPlaceholderIndex) usage and associated lodash import if nothing else uses it so indices remain numeric and the type narrowing for selectedPlaceholder is preserved.src/frontend/store/slices/ladder/utils/index.ts (1)
26-38: 💤 Low valueOptional: short-circuit
remapBranchHandleIdon missing prefix match.
remapBranchHandleIdalready filters bybranch_prefix at line 30, but the inner loop scans every entry ofblockIdMap. When duplicating a rung with many blocks but few branch-rooted handles, this isO(handles × blocks). Since the next path segment afterbranch_is the block id, you could parse it once and look it up directly:♻️ Proposed refactor
- if (typeof handleId !== 'string' || !handleId.startsWith('branch_')) return handleId - for (const [oldBlockId, newBlockId] of Object.entries(blockIdMap)) { - const prefix = `branch_${oldBlockId}_` - if (handleId.startsWith(prefix)) { - return `branch_${newBlockId}_${handleId.slice(prefix.length)}` - } - } - return handleId + if (typeof handleId !== 'string' || !handleId.startsWith('branch_')) return handleId + const rest = handleId.slice('branch_'.length) + const sep = rest.indexOf('_') + if (sep === -1) return handleId + const oldBlockId = rest.slice(0, sep) + const newBlockId = blockIdMap[oldBlockId] + return newBlockId ? `branch_${newBlockId}_${rest.slice(sep + 1)}` : handleId🤖 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/store/slices/ladder/utils/index.ts` around lines 26 - 38, The function remapBranchHandleId currently loops over blockIdMap for every handle; instead, after confirming handleId is a string and startsWith('branch_'), parse the block id directly from the handle (the segment between 'branch_' and the next underscore), look it up in blockIdMap, and if a mapping exists return the reconstructed handle using the mapped id and the remaining suffix; if no mapping exists return the original handleId. Update remapBranchHandleId to avoid the Object.entries loop and preserve the current input/output types and short-circuit behavior.src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/placeholder/index.ts (1)
50-56: 🏗️ Heavy liftBranch-rail discrimination by id prefix is fragile.
Identifying branch rails via
node.id.startsWith('branch-rail-')couples placeholder rendering to the id-construction convention used elsewhere (rail builder, branch reconciler, persistence). A stray rename or a future id refactor would silently disable branch-aware placeholder suppression. Consider adding a structural marker (e.g.,data.branchContexton the rail node, like contacts/coils/parallels above) so all sites can branch on a single typed property.🤖 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/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/placeholder/index.ts` around lines 50 - 56, The current branch-rail check in the placeholder filter uses node.id.startsWith('branch-rail-') which is fragile; change it to test a structural marker (e.g., node.data?.branchContext === true or node.branchContext === 'handle') instead of relying on id format, and update the rail construction code (where rails are created/serialized) to set that marker so renderHandleBranchCreationPlaceholders / renderInBranchSplicePlaceholders keep working; update the conditional in the placeholder logic (the if in the placeholder module that checks node.type === 'powerRail' && ...) to use the new structural property and ensure any downstream consumers read the same property.src/frontend/store/slices/ladder/types.ts (1)
95-108: 💤 Low valueConsider narrowing the new action payloads to
RungNode[]for consistency.The new
updateRungDatakeepsnodes: Node[]even though the rung-levelRungLadderState.nodesis nowRungNode[]. Existing actions (setNodes,updateNode, etc.) also still accept the genericNode. This is fine for ReactFlow callback compatibility, but if the goal of the type narrowing on lines 29–32 is to remove casts at call sites, consider applying the same narrowing to these new action signatures so persisted store state stays in the discriminated-union shape end-to-end.Also applies to: 155-156
🤖 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/store/slices/ladder/types.ts` around lines 95 - 108, The action payload types for rung-level node state are too broad (use Node[]) while RungLadderState.nodes is RungNode[]; update the signatures of updateRungData and the actions setNodes, updateNode, addNode, removeNodes (and the other duplicated signatures noted) to accept RungNode / RungNode[] instead of Node/Node[] so the store stays in the discriminated-union shape end-to-end and callers won’t need casts; locate and change the types in the function/type declarations named updateRungData, setNodes, updateNode, addNode, and removeNodes to use RungNode/RungNode[] consistently.src/frontend/components/_atoms/graphical-editor/ladder/variable.tsx (1)
155-167: 💤 Low valueCasing-repair
updateNodeinside the sync effect risks re-render churn.Calling
updateNodefrom the synchronization effect during render-driven mounts can re-fire the effect whendata.variable?.nameflips after the store update. Today this terminates because the next pass seescaseDiffers === false, but it's a fragile invariant — any futureupdateRelatedNodechange that touchesdata.variable.namecasing on the related block could cause an effect/store ping-pong. Consider gating the repair behind aname !== nodeVariableNamestrict compare and/or moving it out of the effect body (e.g., a one-shot ref).🤖 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/components/_atoms/graphical-editor/ladder/variable.tsx` around lines 155 - 167, The sync effect currently calls updateNode/updateRelatedNode whenever variable.name !== nodeVariableName which can cause render churn; introduce a one-shot ref (e.g., hasRepairedCaseRef) and gate the repair so it only runs once for a given node: inside the synchronization effect check if variable.name !== nodeVariableName AND !hasRepairedCaseRef.current, then call updateNode(...) and updateRelatedNode(...), and set hasRepairedCaseRef.current = true; this keeps the existing strict compare behavior but prevents repeat effect/store ping-pong after the store update (refer to updateNode, updateRelatedNode, variable.name, nodeVariableName, VariableNode and the sync effect).src/frontend/components/_atoms/graphical-editor/ladder/utils/types.ts (1)
4-4: ⚡ Quick winUse the
@rootalias for this shared type import.This new cross-package import should come through the repo alias instead of a six-level relative path.
As per coding guidelines,
Prefer path alias@root/* for imports instead of relative paths to ./src/*.🤖 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/components/_atoms/graphical-editor/ladder/utils/types.ts` at line 4, Replace the six-level relative import in types.ts that imports HandleBranch and PLCVariable from '../../../../../../middleware/shared/ports' with the repo path alias: import those symbols from '@root/middleware/shared/ports' so the single import statement uses the `@root` alias (update the import line that references HandleBranch and PLCVariable accordingly).src/frontend/store/slices/ladder/slice.ts (1)
5-5: ⚡ Quick winUse the repo alias for this new shared-schema import.
Prefer
@root/middleware/shared/ports/flow-schemashere instead of another deep relative climb fromsrc/frontend.As per coding guidelines,
Prefer path alias@root/* for imports instead of relative paths to ./src/*.🤖 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/store/slices/ladder/slice.ts` at line 5, The import uses a deep relative path; change it to the repo path alias to follow guidelines: replace the current import of zodLadderFlowSchema in slice.ts with the aliased import from `@root/middleware/shared/ports/flow-schemas` so that the symbol zodLadderFlowSchema is imported via the `@root/`* alias rather than '../../../../middleware/...'.
🤖 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/frontend/components/_atoms/graphical-editor/ladder/autocomplete/index.tsx`:
- Around line 169-188: The clear-path for blockType === 'variable' only resets
variableNode.data.variable and doesn't sync the parent block's
data.connectedVariables, so subsequent rebuilds (renderVariableBlock /
updateVariableBlockPosition) restore the old value; modify the clear branch
(where getLadderPouVariablesRungNodeAndEdges and updateNode are used) to also
remove the corresponding entry from the parent block's data.connectedVariables
(keep identifiers from the found rung/node to locate the parent block) and
include that updated connectedVariables in the same updateNode call so both the
variable node and its parent block are updated in sync (mirroring what
submitVariableToBlock does).
In `@src/frontend/components/_atoms/graphical-editor/ladder/block.tsx`:
- Around line 189-201: Replace the blocking window.confirm flow in the block
rename logic (where invalidatedBranches is computed using
findInvalidatedBranches and state setters setBlockNameValue/validBlockNameValue
are used) with a non-blocking in-app confirmation modal using the project's
Modal/ModalContent pattern: when invalidatedBranches.length > 0, open the Modal
and render the message (include handle names from invalidatedBranches.map(b =>
`${b.handleId} (${b.direction})`). Provide Confirm/Cancel buttons where Confirm
proceeds with the rename and closes the modal and Cancel calls
setBlockNameValue(validBlockNameValue) then closes the modal; route all
displayed strings through i18next (use translation keys for the headline, body
text with a pluralized branch/pin count, and button labels) so no hardcoded
English remains. Ensure the new flow preserves the original early-return
behavior when cancelled and does not block the renderer thread.
In `@src/frontend/components/_atoms/graphical-editor/ladder/variable.tsx`:
- Around line 199-222: The code force-casts emptyVariable to PLCVariable before
calling updateRelatedNode, which can cause undefined fields downstream; change
updateRelatedNode's parameter type to accept Partial<PLCVariable> (or the same
shape used by updateNodeVariable, e.g. Partial<PLCVariable> | { name: string }),
update its implementation and all callers accordingly, or alternatively
construct and pass a properly typed sentinel PLCVariable with all required
fields populated (instead of casting) and remove the force-cast on
emptyVariable; update references: emptyVariable, updateRelatedNode,
updateNodeVariable, and variableNode to keep types consistent.
In
`@src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/index.ts`:
- Around line 72-75: The toast call currently hardcodes English strings in the
toast invocation (toast({ title: 'Cannot add block to handle branch',
description: 'Handle branches only accept contacts and coils.', variant: 'fail'
})); update it to use i18next translation keys instead: import and use the
i18next t function (or useTranslation hook) to replace the title and description
with translated strings (e.g., t('ladder.toast.cannotAddBlock') and
t('ladder.toast.handleBranchDescription')), add corresponding keys to the
translation JSONs, and keep variant unchanged so the toast displays localized
text.
In `@src/frontend/store/slices/ladder/utils/index.ts`:
- Around line 216-220: The current creation of newHandleBranches (involving
HandleBranch and rung.handleBranches) keeps original node ids when nodeMaps
lookup fails (nodeMaps[nodeId]?.id ?? nodeId), which can leak stale ids; update
the branch.nodeIds transformation to only include mapped ids (e.g., map to
nodeMaps[nodeId]?.id and filter out undefined) or throw/assert when a mapping is
missing, and optionally emit a warning (use the existing logger or console.warn)
indicating the branch and missing nodeId(s); also keep the blockId mapping using
blockIdMap[branch.blockId] ?? branch.blockId as-is.
In `@src/frontend/utils/PLC/xml-generator/rung-graph.ts`:
- Around line 42-103: The functions findNodeBasedOnParallelOpen and
findNodesBasedOnParallelClose currently assume graph lookups succeed; add
null-checks for sourceNodeOfParallelNode, serialNode, and bottomNode after their
rungNodes.find(...) calls and handle missing results by either skipping that
branch (do not push or recurse) or throwing a controlled serialization error
(e.g., a descriptive Error like "Broken parallel graph: missing node for edge to
parallel <id>") so XML generation fails predictably; ensure path.parallels is
only pushed when the parallelNode exists, and return the current path when a
required node is null to avoid dereferencing .type on undefined.
- Around line 122-299: The incoming-edge filter uses targetHandle but
destination waypoints still read node.data.inputConnector; resolve the correct
targetConnector once at the top (use targetHandle to pick the matching connector
from node.data.inputConnector(s) or fall back to the default inputConnector) and
then replace all uses of node.data.inputConnector /
actualNode.data.inputConnector in the serial, open and close branches (including
where findNodeBasedOnParallelOpen, findNodesBasedOnParallelClose, nodes and
parallels are used) with that resolved targetConnector variable so emitted
waypoints anchor to the requested targetHandle.
In `@src/middleware/shared/ports/flow-schemas.ts`:
- Around line 61-63: The schema currently uses z.array(z.number()) for
defaultBounds and reactFlowViewport which allows arrays of any length; change
both to fixed 2-tuples so they always parse as [number, number]. Update the Flow
schema entries for defaultBounds and reactFlowViewport (the symbols named
defaultBounds and reactFlowViewport in flow-schemas.ts) to use
z.tuple([z.number(), z.number()]) and keep the existing DEFAULT_RUNG_BOUNDS as
the .default(...) value; this guarantees exactly two numeric elements and
prevents [] / [1530] / longer arrays from passing validation.
---
Outside diff comments:
In
`@src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/edges.ts`:
- Around line 75-76: The current ternaries check only !options and thus drop
connector fallbacks when callers pass a partial options object; change the
assignments for targetHandle and sourceHandle to use per-key nullish coalescing
so each handle falls back to the node connector when its specific option is
undefined (e.g., set targetHandle = options?.targetHandle ??
targetNodeData.inputConnector?.id and sourceHandle = options?.sourceHandle ??
targetNodeData.outputConnector?.id), updating the lines that assign targetHandle
and sourceHandle accordingly.
---
Nitpick comments:
In `@src/frontend/components/_atoms/graphical-editor/ladder/utils/types.ts`:
- Line 4: Replace the six-level relative import in types.ts that imports
HandleBranch and PLCVariable from '../../../../../../middleware/shared/ports'
with the repo path alias: import those symbols from
'@root/middleware/shared/ports' so the single import statement uses the `@root`
alias (update the import line that references HandleBranch and PLCVariable
accordingly).
In `@src/frontend/components/_atoms/graphical-editor/ladder/variable.tsx`:
- Around line 155-167: The sync effect currently calls
updateNode/updateRelatedNode whenever variable.name !== nodeVariableName which
can cause render churn; introduce a one-shot ref (e.g., hasRepairedCaseRef) and
gate the repair so it only runs once for a given node: inside the
synchronization effect check if variable.name !== nodeVariableName AND
!hasRepairedCaseRef.current, then call updateNode(...) and
updateRelatedNode(...), and set hasRepairedCaseRef.current = true; this keeps
the existing strict compare behavior but prevents repeat effect/store ping-pong
after the store update (refer to updateNode, updateRelatedNode, variable.name,
nodeVariableName, VariableNode and the sync effect).
In
`@src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/drag-n-drop/index.ts`:
- Around line 116-118: Replace the Object.entries(...) usage with direct array
searches: use rung.nodes.findIndex(...) to set selectedPlaceholderIndex and
rung.nodes.find(...) to set selectedPlaceholder (matching the predicate used now
for placeholder/parallelPlaceholder && n.selected); remove the subsequent
toInteger(selectedPlaceholderIndex) usage and associated lodash import if
nothing else uses it so indices remain numeric and the type narrowing for
selectedPlaceholder is preserved.
In
`@src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/placeholder/index.ts`:
- Around line 50-56: The current branch-rail check in the placeholder filter
uses node.id.startsWith('branch-rail-') which is fragile; change it to test a
structural marker (e.g., node.data?.branchContext === true or node.branchContext
=== 'handle') instead of relying on id format, and update the rail construction
code (where rails are created/serialized) to set that marker so
renderHandleBranchCreationPlaceholders / renderInBranchSplicePlaceholders keep
working; update the conditional in the placeholder logic (the if in the
placeholder module that checks node.type === 'powerRail' && ...) to use the new
structural property and ensure any downstream consumers read the same property.
In `@src/frontend/store/slices/ladder/slice.ts`:
- Line 5: The import uses a deep relative path; change it to the repo path alias
to follow guidelines: replace the current import of zodLadderFlowSchema in
slice.ts with the aliased import from `@root/middleware/shared/ports/flow-schemas`
so that the symbol zodLadderFlowSchema is imported via the `@root/`* alias rather
than '../../../../middleware/...'.
In `@src/frontend/store/slices/ladder/types.ts`:
- Around line 95-108: The action payload types for rung-level node state are too
broad (use Node[]) while RungLadderState.nodes is RungNode[]; update the
signatures of updateRungData and the actions setNodes, updateNode, addNode,
removeNodes (and the other duplicated signatures noted) to accept RungNode /
RungNode[] instead of Node/Node[] so the store stays in the discriminated-union
shape end-to-end and callers won’t need casts; locate and change the types in
the function/type declarations named updateRungData, setNodes, updateNode,
addNode, and removeNodes to use RungNode/RungNode[] consistently.
In `@src/frontend/store/slices/ladder/utils/index.ts`:
- Around line 26-38: The function remapBranchHandleId currently loops over
blockIdMap for every handle; instead, after confirming handleId is a string and
startsWith('branch_'), parse the block id directly from the handle (the segment
between 'branch_' and the next underscore), look it up in blockIdMap, and if a
mapping exists return the reconstructed handle using the mapped id and the
remaining suffix; if no mapping exists return the original handleId. Update
remapBranchHandleId to avoid the Object.entries loop and preserve the current
input/output types and short-circuit behavior.
🪄 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: 0348271b-24e1-49d4-b9bc-7b067e90a667
📒 Files selected for processing (25)
src/__architecture__/validate.tssrc/frontend/components/_atoms/graphical-editor/ladder/autocomplete/index.tsxsrc/frontend/components/_atoms/graphical-editor/ladder/block.tsxsrc/frontend/components/_atoms/graphical-editor/ladder/buildNodes.tsxsrc/frontend/components/_atoms/graphical-editor/ladder/power-rail.tsxsrc/frontend/components/_atoms/graphical-editor/ladder/utils/types.tssrc/frontend/components/_atoms/graphical-editor/ladder/variable.tsxsrc/frontend/components/_atoms/highlighted-textarea/index.tsxsrc/frontend/components/_molecules/graphical-editor/ladder/rung/body.tsxsrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/edges.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/diagram/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/drag-n-drop/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/handle-branch/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/placeholder/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/utils/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/variable-block/index.tssrc/frontend/store/slices/ladder/slice.tssrc/frontend/store/slices/ladder/types.tssrc/frontend/store/slices/ladder/utils/index.tssrc/frontend/utils/PLC/xml-generator/codesys/language/ladder-xml.tssrc/frontend/utils/PLC/xml-generator/old-editor/language/ladder-xml.tssrc/frontend/utils/PLC/xml-generator/rung-graph.tssrc/middleware/shared/ports/flow-schemas.tssrc/middleware/shared/ports/types.ts
Comment on lines
+189
to
+201
| const invalidatedBranches = findInvalidatedBranches(rung, node.id, libraryBlock as BlockVariant) | ||
| if (invalidatedBranches.length > 0) { | ||
| const handleNames = invalidatedBranches.map((b) => `${b.handleId} (${b.direction})`).join(', ') | ||
| const confirmed = window.confirm( | ||
| `Changing this block to "${blockNameValue}" will drop ${invalidatedBranches.length} handle ` + | ||
| `branch${invalidatedBranches.length === 1 ? '' : 'es'} on the following pin${invalidatedBranches.length === 1 ? '' : 's'}: ${handleNames}.\n\n` + | ||
| `Continue?`, | ||
| ) | ||
| if (!confirmed) { | ||
| setBlockNameValue(validBlockNameValue) | ||
| return | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
window.confirm blocks the renderer and bypasses i18n.
window.confirm is a synchronous, blocking call that halts the renderer thread and is generally discouraged in Electron/React apps (some Electron renderer configs disable it altogether, returning false and silently dropping the prompt). The hardcoded English copy also violates the i18n guideline for frontend components.
Consider replacing with an in-app modal (the project already has Modal/ModalContent patterns) and routing the strings through i18next.
As per coding guidelines: "Use i18next for internationalization in frontend components".
🤖 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/components/_atoms/graphical-editor/ladder/block.tsx` around
lines 189 - 201, Replace the blocking window.confirm flow in the block rename
logic (where invalidatedBranches is computed using findInvalidatedBranches and
state setters setBlockNameValue/validBlockNameValue are used) with a
non-blocking in-app confirmation modal using the project's Modal/ModalContent
pattern: when invalidatedBranches.length > 0, open the Modal and render the
message (include handle names from invalidatedBranches.map(b => `${b.handleId}
(${b.direction})`). Provide Confirm/Cancel buttons where Confirm proceeds with
the rename and closes the modal and Cancel calls
setBlockNameValue(validBlockNameValue) then closes the modal; route all
displayed strings through i18next (use translation keys for the headline, body
text with a pluralized branch/pin count, and button labels) so no hardcoded
English remains. Ensure the new flow preserves the original early-return
behavior when cancelled and does not block the renderer thread.
Comment on lines
+199
to
+222
| // Allow clearing a variable from a block handle by submitting an empty name. | ||
| // Resets the variable node to an empty placeholder so the user can pick a | ||
| // different variable (or, with handle branches, place contacts/coils on | ||
| // the handle instead). | ||
| if (!variableNameToSubmit.trim()) { | ||
| const emptyVariable = { id: '', name: '' } | ||
| setVariableValue('') | ||
| setIsAVariable(false) | ||
| setInputError(false) | ||
| updateNode({ | ||
| editorName: editor.meta.name, | ||
| rungId: rung.id, | ||
| nodeId: variableNode.id, | ||
| node: { | ||
| ...variableNode, | ||
| data: { | ||
| ...variableNode.data, | ||
| variable: emptyVariable, | ||
| }, | ||
| }, | ||
| }) | ||
| updateRelatedNode(rung, variableNode, emptyVariable as PLCVariable) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid casting an incomplete object to PLCVariable.
emptyVariable is { id: '', name: '' } but is force-cast to PLCVariable when handed to updateRelatedNode. PLCVariable carries additional fields (type, class, etc.), so any downstream consumer that reads them on the cleared entry will hit undefined. Consider widening updateRelatedNode's parameter type to accept the same Partial<PLCVariable> | { name: string } shape used by updateNodeVariable elsewhere in this file, or normalize to a typed sentinel. As per coding guidelines: "Use TypeScript strict mode and avoid any types".
🤖 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/components/_atoms/graphical-editor/ladder/variable.tsx` around
lines 199 - 222, The code force-casts emptyVariable to PLCVariable before
calling updateRelatedNode, which can cause undefined fields downstream; change
updateRelatedNode's parameter type to accept Partial<PLCVariable> (or the same
shape used by updateNodeVariable, e.g. Partial<PLCVariable> | { name: string }),
update its implementation and all callers accordingly, or alternatively
construct and pass a properly typed sentinel PLCVariable with all required
fields populated (instead of casting) and remove the force-cast on
emptyVariable; update references: emptyVariable, updateRelatedNode,
updateNodeVariable, and variableNode to keep types consistent.
Comment on lines
+72
to
+75
| toast({ | ||
| title: 'Cannot add block to handle branch', | ||
| description: 'Handle branches only accept contacts and coils.', | ||
| variant: 'fail', |
Contributor
There was a problem hiding this comment.
Localize the new toast copy.
These strings are user-facing and live under src/frontend/components, so they should go through i18next instead of being hardcoded in English.
As per coding guidelines, Use i18next for internationalization in frontend components.
🤖 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/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/index.ts`
around lines 72 - 75, The toast call currently hardcodes English strings in the
toast invocation (toast({ title: 'Cannot add block to handle branch',
description: 'Handle branches only accept contacts and coils.', variant: 'fail'
})); update it to use i18next translation keys instead: import and use the
i18next t function (or useTranslation hook) to replace the title and description
with translated strings (e.g., t('ladder.toast.cannotAddBlock') and
t('ladder.toast.handleBranchDescription')), add corresponding keys to the
translation JSONs, and keep variant unchanged so the toast displays localized
text.
Comment on lines
+216
to
220
| const newHandleBranches: HandleBranch[] = (rung.handleBranches ?? []).map((branch) => ({ | ||
| ...branch, | ||
| blockId: blockIdMap[branch.blockId] ?? branch.blockId, | ||
| nodeIds: branch.nodeIds.map((nodeId) => nodeMaps[nodeId]?.id ?? nodeId), | ||
| })) |
Contributor
There was a problem hiding this comment.
handleBranches.nodeIds may retain stale ids when a node id is missing from nodeMaps.
The fallback nodeMaps[nodeId]?.id ?? nodeId preserves the original node id when the lookup fails, which can leak a reference to a node that no longer exists in the duplicated rung (e.g., a stale branch-context entry). Consider filtering out unmapped ids and warning, or asserting the mapping is complete:
- nodeIds: branch.nodeIds.map((nodeId) => nodeMaps[nodeId]?.id ?? nodeId),
+ nodeIds: branch.nodeIds.flatMap((nodeId) => (nodeMaps[nodeId] ? [nodeMaps[nodeId].id] : [])),📝 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
| const newHandleBranches: HandleBranch[] = (rung.handleBranches ?? []).map((branch) => ({ | |
| ...branch, | |
| blockId: blockIdMap[branch.blockId] ?? branch.blockId, | |
| nodeIds: branch.nodeIds.map((nodeId) => nodeMaps[nodeId]?.id ?? nodeId), | |
| })) | |
| const newHandleBranches: HandleBranch[] = (rung.handleBranches ?? []).map((branch) => ({ | |
| ...branch, | |
| blockId: blockIdMap[branch.blockId] ?? branch.blockId, | |
| nodeIds: branch.nodeIds.flatMap((nodeId) => (nodeMaps[nodeId] ? [nodeMaps[nodeId].id] : [])), | |
| })) |
🤖 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/store/slices/ladder/utils/index.ts` around lines 216 - 220, The
current creation of newHandleBranches (involving HandleBranch and
rung.handleBranches) keeps original node ids when nodeMaps lookup fails
(nodeMaps[nodeId]?.id ?? nodeId), which can leak stale ids; update the
branch.nodeIds transformation to only include mapped ids (e.g., map to
nodeMaps[nodeId]?.id and filter out undefined) or throw/assert when a mapping is
missing, and optionally emit a warning (use the existing logger or console.warn)
indicating the branch and missing nodeId(s); also keep the blockId mapping using
blockIdMap[branch.blockId] ?? branch.blockId as-is.
Comment on lines
+42
to
+103
| export const findNodeBasedOnParallelOpen = ( | ||
| parallelNode: ParallelNode, | ||
| rung: RungLadderState, | ||
| path: { | ||
| nodes: Node<BasicNodeData>[] | ||
| parallels: ParallelNode[] | ||
| } = { nodes: [], parallels: [] }, | ||
| ) => { | ||
| const { nodes: rungNodes, edges: rungEdges } = rung | ||
|
|
||
| const edgeToParallelNode = rungEdges.find((edge) => edge.target === parallelNode.id)?.source | ||
| const sourceNodeOfParallelNode = rungNodes.find((node) => node.id === edgeToParallelNode) as Node<BasicNodeData> | ||
| path.parallels.push(parallelNode) | ||
|
|
||
| if (sourceNodeOfParallelNode.type !== 'parallel') { | ||
| path.nodes.push(sourceNodeOfParallelNode) | ||
| return path | ||
| } else if ((sourceNodeOfParallelNode as ParallelNode).data.type === 'close') { | ||
| return findNodesBasedOnParallelClose(sourceNodeOfParallelNode as ParallelNode, rung, path) | ||
| } else { | ||
| return findNodeBasedOnParallelOpen(sourceNodeOfParallelNode as ParallelNode, rung, path) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Walk backward from a parallel-close node, collecting both the serial-spine | ||
| * upstream and the parallel-path upstream nodes plus all parallel nodes seen. | ||
| */ | ||
| export const findNodesBasedOnParallelClose = ( | ||
| parallelNode: ParallelNode, | ||
| rung: RungLadderState, | ||
| path: { | ||
| nodes: Node<BasicNodeData>[] | ||
| parallels: ParallelNode[] | ||
| } = { nodes: [], parallels: [] }, | ||
| ) => { | ||
| const { nodes: rungNodes, edges: rungEdges } = rung | ||
|
|
||
| const edgesToParallelNode = rungEdges.filter((edge) => edge.target === parallelNode.id) | ||
| const serialNode = rungNodes.find((node) => | ||
| edgesToParallelNode.find( | ||
| (edge) => edge.source === node.id && edge.targetHandle === parallelNode.data.inputConnector?.id, | ||
| ), | ||
| ) as Node<BasicNodeData> | ||
|
|
||
| if (!path.nodes.includes(serialNode)) path.nodes.push(serialNode) | ||
|
|
||
| const bottomNode = rungNodes.find((node) => | ||
| edgesToParallelNode.find( | ||
| (edge) => edge.source === node.id && edge.targetHandle === parallelNode.data.parallelInputConnector?.id, | ||
| ), | ||
| ) as Node<BasicNodeData> | ||
|
|
||
| path.parallels.push(parallelNode) | ||
|
|
||
| if (bottomNode.type !== 'parallel') { | ||
| path.nodes.push(bottomNode) | ||
| return path | ||
| } | ||
|
|
||
| return findNodesBasedOnParallelClose(bottomNode as ParallelNode, rung, path) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard missing graph lookups before dereferencing them.
Both traversal helpers cast lookup results and immediately read .type. A single orphaned edge or mismatched OPEN/CLOSE pair will throw during XML generation instead of failing cleanly. Please null-check sourceNodeOfParallelNode, serialNode, and bottomNode and either skip that broken branch or raise a controlled serialization error.
🤖 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-generator/rung-graph.ts` around lines 42 - 103,
The functions findNodeBasedOnParallelOpen and findNodesBasedOnParallelClose
currently assume graph lookups succeed; add null-checks for
sourceNodeOfParallelNode, serialNode, and bottomNode after their
rungNodes.find(...) calls and handle missing results by either skipping that
branch (do not push or recurse) or throwing a controlled serialization error
(e.g., a descriptive Error like "Broken parallel graph: missing node for edge to
parallel <id>") so XML generation fails predictably; ensure path.parallels is
only pushed when the parallelNode exists, and return the current path when a
required node is null to avoid dereferencing .type on undefined.
Comment on lines
+122
to
+299
| const { targetHandle, formatFormalParameter = defaultFormatFormalParameter } = options | ||
|
|
||
| const connectedEdges = rungEdges.filter( | ||
| (edge) => edge.target === node.id && (targetHandle === undefined || edge.targetHandle === targetHandle), | ||
| ) | ||
| if (!connectedEdges.length) return [] | ||
|
|
||
| const connections = connectedEdges.map((edge) => { | ||
| const sourceNode = rungNodes.find((node) => node.id === edge.source) as Node<BasicNodeData> | ||
| // If the source node is not found or it is a variable node, return undefined | ||
| if (!sourceNode || sourceNode.type === 'variable') return undefined | ||
|
|
||
| // Node is not a parallel node | ||
| if (sourceNode.type !== 'parallel') { | ||
| return { | ||
| '@refLocalId': sourceNode.data.numericId, | ||
| '@formalParameter': formatFormalParameter(sourceNode.data.outputConnector?.id), | ||
| position: [ | ||
| // Final edge destination | ||
| { | ||
| '@x': node.data.inputConnector?.glbPosition.x ?? 0, | ||
| '@y': (node.data.inputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| // Initial edge source | ||
| { | ||
| '@x': sourceNode.data.outputConnector?.glbPosition.x ?? 0, | ||
| '@y': (sourceNode.data.outputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| ], | ||
| } | ||
| } | ||
|
|
||
| // Node is a parallel node | ||
| const parallelNode = sourceNode as ParallelNode | ||
|
|
||
| /** | ||
| * TODO: This is a temporary solution to find the connections of a parallel node. | ||
| * This should be refactored so that the lines are placed correctly | ||
| */ | ||
|
|
||
| // If the parallel node is opening the connection | ||
| if (parallelNode.data.type === 'open') { | ||
| // Find the previous node of the parallel node | ||
| const { nodes, parallels } = findNodeBasedOnParallelOpen(parallelNode, rung) | ||
| const actualNode = node | ||
|
|
||
| const lastParallelNode = parallels | ||
| .filter((parallel) => parallel.data.type === 'open') | ||
| .reverse() | ||
| .copyWithin(0, 1)[0] | ||
| const lastParallelSerialEdge = rungEdges.find( | ||
| (edge) => | ||
| edge.source === lastParallelNode.id && edge.sourceHandle === lastParallelNode.data.outputConnector?.id, | ||
| ) | ||
|
|
||
| // If the node is connected serially to the parallel node | ||
| if (lastParallelSerialEdge && lastParallelSerialEdge.target === actualNode.id) { | ||
| return nodes.map((node, index) => ({ | ||
| '@refLocalId': node.data.numericId, | ||
| '@formalParameter': formatFormalParameter(node.data.outputConnector?.id), | ||
| position: | ||
| index === 0 | ||
| ? [ | ||
| // Final edge destination | ||
| { | ||
| '@x': actualNode.data.inputConnector?.glbPosition.x ?? 0, | ||
| '@y': (actualNode.data.inputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| // Initial edge source | ||
| { | ||
| '@x': node.data.outputConnector?.glbPosition.x ?? 0, | ||
| '@y': (node.data.outputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| ] | ||
| : [ | ||
| // Final edge destination | ||
| { | ||
| '@x': actualNode.data.inputConnector?.glbPosition.x ?? 0, | ||
| '@y': (actualNode.data.inputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| // Final position of parallel | ||
| { | ||
| '@x': lastParallelNode.data.parallelInputConnector?.glbPosition.x ?? 0, | ||
| '@y': (actualNode.data.inputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| // Initial position of parallel | ||
| { | ||
| '@x': lastParallelNode.data.parallelInputConnector?.glbPosition.x ?? 0, | ||
| '@y': (node.data.outputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| // Initial edge source | ||
| { | ||
| '@x': node.data.outputConnector?.glbPosition.x ?? 0, | ||
| '@y': (node.data.outputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| ], | ||
| })) | ||
| } | ||
|
|
||
| return nodes.map((node) => { | ||
| return { | ||
| '@refLocalId': node.data.numericId, | ||
| '@formalParameter': formatFormalParameter(node.data.outputConnector?.id), | ||
| position: [ | ||
| // Final edge destination | ||
| { | ||
| '@x': actualNode.data.inputConnector?.glbPosition.x ?? 0, | ||
| '@y': (actualNode.data.inputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| // Final position of parallel | ||
| { | ||
| '@x': lastParallelNode.data.parallelInputConnector?.glbPosition.x ?? 0, | ||
| '@y': (actualNode.data.inputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| // Initial position of parallel | ||
| { | ||
| '@x': lastParallelNode.data.parallelInputConnector?.glbPosition.x ?? 0, | ||
| '@y': (node.data.outputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| // Initial edge source | ||
| { | ||
| '@x': node.data.outputConnector?.glbPosition.x ?? 0, | ||
| '@y': (node.data.outputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| ], | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| // If the parallel node is closing the connection | ||
| const { nodes, parallels } = findNodesBasedOnParallelClose(parallelNode, rung) | ||
| const actualNode = node | ||
|
|
||
| const firstParallelNode = parallels[0] | ||
| const closeConnections = nodes.map((node, index) => { | ||
| return { | ||
| '@refLocalId': node.data.numericId, | ||
| '@formalParameter': formatFormalParameter(node.data.outputConnector?.id), | ||
| position: | ||
| index === 0 | ||
| ? [ | ||
| // Final edge destination | ||
| { | ||
| '@x': actualNode.data.inputConnector?.glbPosition.x ?? 0, | ||
| '@y': (actualNode.data.inputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| // Initial edge source | ||
| { | ||
| '@x': node.data.outputConnector?.glbPosition.x ?? 0, | ||
| '@y': (node.data.outputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| ] | ||
| : [ | ||
| // Final edge destination | ||
| { | ||
| '@x': actualNode.data.inputConnector?.glbPosition.x ?? 0, | ||
| '@y': (actualNode.data.inputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| // Final position of parallel | ||
| { | ||
| '@x': firstParallelNode.data.parallelInputConnector?.glbPosition.x ?? 0, | ||
| '@y': (actualNode.data.inputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| // Initial position of parallel | ||
| { | ||
| '@x': firstParallelNode.data.parallelInputConnector?.glbPosition.x ?? 0, | ||
| '@y': (node.data.outputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| // Initial edge source | ||
| { | ||
| '@x': node.data.outputConnector?.glbPosition.x ?? 0, | ||
| '@y': (node.data.outputConnector?.glbPosition.y ?? 0) + offsetY, | ||
| }, | ||
| ], | ||
| } | ||
| }) | ||
|
|
||
| return closeConnections |
Contributor
There was a problem hiding this comment.
Anchor serialized waypoints to the requested targetHandle.
targetHandle only filters the incoming edges right now. Every emitted destination waypoint still uses node.data.inputConnector, so a branch wired to a secondary boolean input is serialized to the primary input’s coordinates. That will put the wire on the wrong pin in the exported graph.
[suggested fix: resolve the destination handle once, then reuse it in the serial/open/close branches.]
Suggested fix
export const findConnections = (
node: Node<BasicNodeData>,
rung: RungLadderState,
offsetY: number = 0,
options: FindConnectionsOptions = {},
): RungConnection[] => {
const { nodes: rungNodes, edges: rungEdges } = rung
const { targetHandle, formatFormalParameter = defaultFormatFormalParameter } = options
+ const targetConnector =
+ targetHandle === undefined
+ ? node.data.inputConnector
+ : node.data.inputHandles.find((handle) => handle.id === targetHandle) ?? node.data.inputConnector
const connectedEdges = rungEdges.filter(
(edge) => edge.target === node.id && (targetHandle === undefined || edge.targetHandle === targetHandle),
)Then replace every actualNode.data.inputConnector / node.data.inputConnector destination reference in this function with targetConnector.
🤖 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-generator/rung-graph.ts` around lines 122 - 299,
The incoming-edge filter uses targetHandle but destination waypoints still read
node.data.inputConnector; resolve the correct targetConnector once at the top
(use targetHandle to pick the matching connector from
node.data.inputConnector(s) or fall back to the default inputConnector) and then
replace all uses of node.data.inputConnector / actualNode.data.inputConnector in
the serial, open and close branches (including where
findNodeBasedOnParallelOpen, findNodesBasedOnParallelClose, nodes and parallels
are used) with that resolved targetConnector variable so emitted waypoints
anchor to the requested targetHandle.
Comment on lines
61
to
+63
| comment: z.string().default(''), | ||
| defaultBounds: z.array(z.number()), | ||
| reactFlowViewport: z.array(z.number()), | ||
| nodes: z.array(nodeSchema), | ||
| edges: z.array(edgeSchema), | ||
| defaultBounds: z.array(z.number()).default(DEFAULT_RUNG_BOUNDS), | ||
| reactFlowViewport: z.array(z.number()).default(DEFAULT_RUNG_BOUNDS), |
Contributor
There was a problem hiding this comment.
Validate rung bounds as fixed 2-tuples.
defaultBounds and reactFlowViewport are consumed as [number, number], but z.array(z.number()) will also accept [], [1530], or longer arrays. That lets malformed snapshots through the parser and pushes undefined/NaN into layout code that indexes [0] and [1].
Suggested fix
+const boundsSchema = z.tuple([z.number(), z.number()])
+
const zodRungLadderStateSchema = z.object({
id: z.string(),
comment: z.string().default(''),
- defaultBounds: z.array(z.number()).default(DEFAULT_RUNG_BOUNDS),
- reactFlowViewport: z.array(z.number()).default(DEFAULT_RUNG_BOUNDS),
+ defaultBounds: boundsSchema.default(DEFAULT_RUNG_BOUNDS),
+ reactFlowViewport: boundsSchema.default(DEFAULT_RUNG_BOUNDS),
nodes: z.array(nodeSchema).default([]),
edges: z.array(edgeSchema).default([]),
handleBranches: z.array(zodHandleBranchSchema).default([]),
})🤖 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/ports/flow-schemas.ts` around lines 61 - 63, The schema
currently uses z.array(z.number()) for defaultBounds and reactFlowViewport which
allows arrays of any length; change both to fixed 2-tuples so they always parse
as [number, number]. Update the Flow schema entries for defaultBounds and
reactFlowViewport (the symbols named defaultBounds and reactFlowViewport in
flow-schemas.ts) to use z.tuple([z.number(), z.number()]) and keep the existing
DEFAULT_RUNG_BOUNDS as the .default(...) value; this guarantees exactly two
numeric elements and prevents [] / [1530] / longer arrays from passing
validation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/serial/index.ts (1)
52-52: ⚡ Quick winConsider extracting the repeated RungLadderState construction pattern.
The pattern
{ ...rung, nodes: newNodes, edges: newEdges } as RungLadderStateis repeated four times. While the type assertion is necessary, extracting it to a variable would improve maintainability and reduce duplication.♻️ Proposed refactor to reduce repetition
newNodes.splice(placeholder.index, 1, newElement) newNodes = removePlaceholderElements(newNodes) + + const updatedRung = { ...rung, nodes: newNodes, edges: newEdges } as RungLadderState /** * Get the related element of the placeholder * If there is no related element, return the new nodes and edges * Get the previous elements (based on the related node) */ const relatedNode = placeholder.selected.data.relatedNode as Node const { nodes: relatedNodePreviousNodes, edges: relatedNodePreviousEdges } = getPreviousElementsByEdge( - { ...rung, nodes: newNodes, edges: newEdges } as RungLadderState, + updatedRung, relatedNode, ) if (!relatedNodePreviousNodes || !relatedNodePreviousEdges) return { nodes: newNodes, edges: newEdges } /** * Get the previous node */ let previousNode = getPreviousElement( - { ...rung, nodes: newNodes, edges: newEdges } as RungLadderState, + updatedRung, newNodes.findIndex((n) => n.id === newElement.id), ) // ... if ( // ...conditions... ) { previousNode = relatedNodePreviousNodes.serial[0] newEdges = connectNodes( - { ...rung, nodes: newNodes, edges: newEdges } as RungLadderState, + updatedRung, previousNode.id, newElement.id, 'parallel', ) } else { newEdges = connectNodes( - { ...rung, nodes: newNodes, edges: newEdges } as RungLadderState, + updatedRung, previousNode.id, newElement.id, 'serial', ) }Also applies to: 61-61, 84-84, 91-91
🤖 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/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/serial/index.ts` at line 52, Extract the repeated construction `{ ...rung, nodes: newNodes, edges: newEdges } as RungLadderState` into a single typed variable (e.g., `const updatedRung: RungLadderState = { ...rung, nodes: newNodes, edges: newEdges };`) and replace the four occurrences with that variable; make sure to use the same `rung`, `newNodes`, `newEdges` identifiers and keep the RungLadderState assertion on the single declaration to preserve typing.src/frontend/components/_features/[workspace]/editor/graphical/elements/ladder/block/index.tsx (2)
177-184: ⚡ Quick winType assertion may hide type safety issues.
The type assertion
as BlockNode<object>on line 184 forces TypeScript to accept the spread result without verifying that all requiredBlockNode<object>properties are present. While the code appears correct (spreadingnewNodeand addingexecutionOrderandvariable), type assertions bypass compile-time safety.Consider refactoring to avoid the assertion by explicitly typing the intermediate result or using a type guard, or at minimum add a runtime validation that the shape matches expectations.
🤖 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/components/_features/`[workspace]/editor/graphical/elements/ladder/block/index.tsx around lines 177 - 184, The current setNode call casts the spread result to BlockNode<object> which can hide missing properties; replace the type assertion by constructing or validating a proper BlockNode<object> before calling setNode — e.g., create an explicitly typed variable (updatedNode: BlockNode<object>) that spreads newNode but ensures all required BlockNode fields exist and sets data.executionOrder = Number(formState.executionOrder) and data.variable = selectedNode.data.variable, or implement a small runtime type guard that verifies the shape and throws/logs if invalid, then pass that validated updatedNode into setNode instead of using "as".
346-349: ⚖️ Poor tradeoffDuplicate type assertion pattern.
Same type assertion pattern as line 184. This creates maintenance risk—if
BlockNode<object>requirements change, these assertions won't catch mismatches. Consider extracting a typed helper function likebuildNodeWithVariable(newNode, variable, executionOrder?)to centralize the logic and eliminate both assertions.🤖 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/components/_features/`[workspace]/editor/graphical/elements/ladder/block/index.tsx around lines 346 - 349, The repeated explicit type assertion to BlockNode<object> in the setNode call is brittle; extract a small typed helper (e.g., buildNodeWithVariable(node, variable, executionOrder?)) that accepts the untyped/newNode and variable and returns a properly typed BlockNode<object>, move the merge logic (data: { ...node.data, variable }) into that helper, then call setNode(buildNodeWithVariable(newNode, selectedNode.data.variable)); this centralizes the type shape, removes the duplicate "as BlockNode<object>" assertions, and ensures the returned value is strongly typed for use across the file.src/frontend/store/slices/ladder/slice.ts (1)
157-157: ⚖️ Poor tradeoffMultiple type assertions reduce type safety.
Seven type assertions were added across node-mutation actions (lines 157, 255, 295, 312, 326, 354, 371). While these work around Immer's
Draft<T>types, they bypass TypeScript's ability to catch shape mismatches.Each
as RungLadderState['nodes']assertion tells the compiler "trust me" without verification. IfRungLadderState['nodes']requirements evolve (e.g., a new required property), these assertions won't flag missing fields.Since the assertions appear necessary for Immer compatibility and follow a consistent pattern, this is acceptable for now, but consider:
- Adding a helper function
asRungNodes(nodes: Node[]): RungLadderState['nodes']with runtime validation- Or creating a Zod schema
rungNodesSchemato validate before assertingAlso applies to: 255-255, 295-295, 312-312, 326-326, 354-354, 371-371
🤖 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/store/slices/ladder/slice.ts` at line 157, The repeated use of unsafe assertions like "as RungLadderState['nodes']" (seen where nodes is set from railNodes and in other node-mutation actions) bypasses type checking; replace these ad-hoc casts by introducing a small runtime-checked converter such as a helper function (e.g., asRungNodes(nodes: Node[]): RungLadderState['nodes']) or a Zod schema (rungNodesSchema.parse(nodes)) and call that helper wherever you currently cast (locations referencing railNodes and the other node-mutation assignments) so you validate shape at runtime before returning the typed value instead of asserting it away.
🤖 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/frontend/components/_atoms/graphical-editor/fbd/utils/utils.ts`:
- Around line 25-26: Replace the incorrect LadderFlowType edge references with
the FBD-specific type: change the types of the source and target parameters that
currently use LadderFlowType['rungs'][0]['edges'] to
FBDFlowType['rung']['edges'] so they match the existing rung parameter (typed as
FBDFlowType['rung']) and the implementation that reads rung?.edges; update the
parameter type annotations where source and target are declared accordingly.
In
`@src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/nodes.ts`:
- Around line 8-16: The declared return type for findNode says position can be
number | undefined but findIndex always returns a number; update the
implementation in findNode so the runtime matches the type by computing const
idx = rung.nodes.findIndex(n => n.id === nodeId) and returning position: idx ===
-1 ? undefined : idx (keep node as rung.nodes.find(...)); this ensures the
function's signature and behavior (RungLadderState, RungNode, findNode) are
consistent and callers can reliably check for undefined when not found.
In `@src/frontend/store/slices/ladder/slice.ts`:
- Around line 16-44: parseFlowOrPassthrough currently returns the raw unparsed
flow on zodLadderFlowSchema.safeParse failure, which lets corrupted shapes
propagate; instead, on parse failure build and return a sanitized fallback: keep
flow.name and flow.updated, set missing top-level fields (e.g., handleBranches:
[], connectedVariables: {} or the shape expected by addLadderFlow migration),
and normalize rungs to an array where each rung has required properties and
selectedNodes defaulted to [] (use flow.rungs values where safe but coerce
types/empty arrays as needed); log the full parse error with context via
console.error and return this defensively-defaulted flow rather than the
original unvalidated object so downstream code sees a consistent, safe shape
(update references in parseFlowOrPassthrough and any tests that rely on its
behavior).
---
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/editor/graphical/elements/ladder/block/index.tsx:
- Around line 177-184: The current setNode call casts the spread result to
BlockNode<object> which can hide missing properties; replace the type assertion
by constructing or validating a proper BlockNode<object> before calling setNode
— e.g., create an explicitly typed variable (updatedNode: BlockNode<object>)
that spreads newNode but ensures all required BlockNode fields exist and sets
data.executionOrder = Number(formState.executionOrder) and data.variable =
selectedNode.data.variable, or implement a small runtime type guard that
verifies the shape and throws/logs if invalid, then pass that validated
updatedNode into setNode instead of using "as".
- Around line 346-349: The repeated explicit type assertion to BlockNode<object>
in the setNode call is brittle; extract a small typed helper (e.g.,
buildNodeWithVariable(node, variable, executionOrder?)) that accepts the
untyped/newNode and variable and returns a properly typed BlockNode<object>,
move the merge logic (data: { ...node.data, variable }) into that helper, then
call setNode(buildNodeWithVariable(newNode, selectedNode.data.variable)); this
centralizes the type shape, removes the duplicate "as BlockNode<object>"
assertions, and ensures the returned value is strongly typed for use across the
file.
In
`@src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/serial/index.ts`:
- Line 52: Extract the repeated construction `{ ...rung, nodes: newNodes, edges:
newEdges } as RungLadderState` into a single typed variable (e.g., `const
updatedRung: RungLadderState = { ...rung, nodes: newNodes, edges: newEdges };`)
and replace the four occurrences with that variable; make sure to use the same
`rung`, `newNodes`, `newEdges` identifiers and keep the RungLadderState
assertion on the single declaration to preserve typing.
In `@src/frontend/store/slices/ladder/slice.ts`:
- Line 157: The repeated use of unsafe assertions like "as
RungLadderState['nodes']" (seen where nodes is set from railNodes and in other
node-mutation actions) bypasses type checking; replace these ad-hoc casts by
introducing a small runtime-checked converter such as a helper function (e.g.,
asRungNodes(nodes: Node[]): RungLadderState['nodes']) or a Zod schema
(rungNodesSchema.parse(nodes)) and call that helper wherever you currently cast
(locations referencing railNodes and the other node-mutation assignments) so you
validate shape at runtime before returning the typed value instead of asserting
it away.
🪄 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: 1cafaf5d-a38f-41e7-b3e0-e2fa7e1c9ee6
📒 Files selected for processing (27)
src/frontend/components/_atoms/graphical-editor/fbd/block.tsxsrc/frontend/components/_atoms/graphical-editor/fbd/utils/utils.tssrc/frontend/components/_atoms/graphical-editor/ladder/autocomplete/index.tsxsrc/frontend/components/_atoms/graphical-editor/ladder/block.tsxsrc/frontend/components/_atoms/graphical-editor/ladder/coil.tsxsrc/frontend/components/_atoms/graphical-editor/ladder/contact.tsxsrc/frontend/components/_atoms/graphical-editor/ladder/utils/types.tssrc/frontend/components/_features/[workspace]/editor/graphical/elements/ladder/block/index.tsxsrc/frontend/components/_molecules/graphical-editor/ladder/rung/body.tsxsrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/edges.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/diagram/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/drag-n-drop/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/handle-branch/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/parallel/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/placeholder/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/serial/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/utils/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/variable-block/index.tssrc/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/nodes.tssrc/frontend/store/slices/ladder/slice.tssrc/frontend/store/slices/ladder/types.tssrc/frontend/utils/PLC/xml-generator/codesys/language/ladder-xml.tssrc/frontend/utils/PLC/xml-generator/old-editor/language/ladder-xml.tssrc/frontend/utils/PLC/xml-generator/rung-graph.tssrc/middleware/shared/ports/ai-port.tssrc/middleware/shared/ports/index.ts
✅ Files skipped from review due to trivial changes (2)
- src/frontend/components/_atoms/graphical-editor/ladder/coil.tsx
- src/frontend/components/_atoms/graphical-editor/ladder/contact.tsx
🚧 Files skipped from review as they are similar to previous changes (14)
- src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/variable-block/index.ts
- src/frontend/utils/PLC/xml-generator/codesys/language/ladder-xml.ts
- src/frontend/components/_molecules/graphical-editor/ladder/rung/body.tsx
- src/frontend/utils/PLC/xml-generator/old-editor/language/ladder-xml.ts
- src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/diagram/index.ts
- src/frontend/components/_atoms/graphical-editor/ladder/block.tsx
- src/frontend/components/_atoms/graphical-editor/ladder/utils/types.ts
- src/frontend/components/_atoms/graphical-editor/ladder/autocomplete/index.tsx
- src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/drag-n-drop/index.ts
- src/frontend/store/slices/ladder/types.ts
- src/frontend/utils/PLC/xml-generator/rung-graph.ts
- src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/index.ts
- src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/edges.ts
- src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/utils/index.ts
Comment on lines
25
to
26
| source: LadderFlowType['rungs'][0]['edges'] | undefined | ||
| target: LadderFlowType['rungs'][0]['edges'] | undefined |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check TypeScript compilation and examine edge type definitions
# Check for TypeScript errors in this file
echo "=== Checking for TypeScript errors ==="
npx tsc --noEmit --project tsconfig.json 2>&1 | rg -C3 "fbd/utils/utils.ts"
# Find and display FBD and Ladder edge type definitions
echo -e "\n=== FBD edge type structure ==="
rg -n -A5 "type FBDFlowType.*=|interface FBDFlowType" --type=ts
echo -e "\n=== Ladder edge type structure ==="
rg -n -A5 "type LadderFlowType.*=|interface LadderFlowType" --type=ts
# Check usages of this function to see what type consumers expect
echo -e "\n=== Function usages ==="
rg -n -B2 -A2 "getFBDPouVariablesRungNodeAndEdges" --type=ts --type=tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 941
🏁 Script executed:
# Read the utils.ts file to see the full function definition
cat -n src/frontend/components/_atoms/graphical-editor/fbd/utils/utils.ts | head -100Repository: Autonomy-Logic/openplc-editor
Length of output: 4807
🏁 Script executed:
# Get the FBDRungState type definition
rg -n -A15 "type FBDRungState|interface FBDRungState" src/frontend/store/slices/fbd/types.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Get the RungLadderState type definition for comparison
rg -n -A15 "type RungLadderState|interface RungLadderState" src/frontend/store/slices/ladder/types.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 408
🏁 Script executed:
# Search for edge-related type definitions in both FBD and Ladder
rg -n "edges.*:" src/frontend/store/slices/fbd/types.ts src/frontend/store/slices/ladder/types.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 543
🏁 Script executed:
# Get FBDRungState type definition
rg -n -A20 "type FBDRungState|interface FBDRungState" src/frontend/store/slices/fbd/types.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Also check PortRungLadderState to see the full edge definition
rg -n "edges" src/frontend/store/slices/ladder/types.ts | head -20Repository: Autonomy-Logic/openplc-editor
Length of output: 456
🏁 Script executed:
# Read the entire FBD types file to find FBDRungState
cat -n src/frontend/store/slices/fbd/types.ts | head -150Repository: Autonomy-Logic/openplc-editor
Length of output: 3957
🏁 Script executed:
# Also search for edge definitions in FBD context more broadly
rg -n "edges" src/frontend/store/slices/fbd/types.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 248
🏁 Script executed:
# Find and read the FBDRungState definition
rg -n -A20 "type FBDRungState|interface FBDRungState" src/middleware/shared/ports/types.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 759
🏁 Script executed:
# Also check for RungLadderState in the same location for comparison
rg -n -A20 "type RungLadderState|interface RungLadderState" src/middleware/shared/ports/types.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 936
🏁 Script executed:
# Verify the correct type path for FBD edges
rg -n "FBDFlowType\['rung'\]\['edges'\]" src/Repository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Double-check that the return values are actually Edge[] and assignment works
rg -n -B5 -A5 "edgesThatNodeIsSource\|edgesThatNodeIsTarget" src/frontend/components/_atoms/graphical-editor/fbd/utils/utils.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 55
Correct the edge type references to use FBDFlowType instead of LadderFlowType.
Lines 25–26 should reference FBDFlowType['rung']['edges'] instead of LadderFlowType['rungs'][0]['edges']. The function is FBD-specific, the rung parameter is already typed as FBDFlowType['rung'], and the implementation retrieves edges from rung?.edges. This inconsistency mirrors the fix already applied to line 28 for the node type. While both type references resolve to Edge[] structurally, using FBDFlowType maintains consistency with the function's actual context.
🤖 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/components/_atoms/graphical-editor/fbd/utils/utils.ts` around
lines 25 - 26, Replace the incorrect LadderFlowType edge references with the
FBD-specific type: change the types of the source and target parameters that
currently use LadderFlowType['rungs'][0]['edges'] to
FBDFlowType['rung']['edges'] so they match the existing rung parameter (typed as
FBDFlowType['rung']) and the implementation that reads rung?.edges; update the
parameter type annotations where source and target are declared accordingly.
Comment on lines
8
to
16
| export const findNode = ( | ||
| rung: RungLadderState, | ||
| nodeId: string, | ||
| ): { node: Node | undefined; position: number | undefined } => { | ||
| ): { node: RungNode | undefined; position: number | undefined } => { | ||
| return { | ||
| node: rung.nodes.find((node) => node.id === nodeId), | ||
| position: rung.nodes.findIndex((node) => node.id === nodeId), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
position return type is inaccurate — findIndex never returns undefined.
Array.prototype.findIndex always returns a number (using -1 as the not-found sentinel), so the declared position: number | undefined is broader than the implementation can ever produce. Callers that try to detect "not found" via position !== undefined will silently treat the -1 case as a hit. Either tighten the type to number (and document the -1 sentinel) or change the implementation to return undefined so the type matches reality.
🩹 Option A — tighten the type to match implementation
export const findNode = (
rung: RungLadderState,
nodeId: string,
-): { node: RungNode | undefined; position: number | undefined } => {
+): { node: RungNode | undefined; position: number } => {
return {
node: rung.nodes.find((node) => node.id === nodeId),
position: rung.nodes.findIndex((node) => node.id === nodeId),
}
}🩹 Option B — make the implementation match the declared type
export const findNode = (
rung: RungLadderState,
nodeId: string,
): { node: RungNode | undefined; position: number | undefined } => {
- return {
- node: rung.nodes.find((node) => node.id === nodeId),
- position: rung.nodes.findIndex((node) => node.id === nodeId),
- }
+ const position = rung.nodes.findIndex((node) => node.id === nodeId)
+ return {
+ node: position === -1 ? undefined : rung.nodes[position],
+ position: position === -1 ? undefined : position,
+ }
}📝 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
| export const findNode = ( | |
| rung: RungLadderState, | |
| nodeId: string, | |
| ): { node: Node | undefined; position: number | undefined } => { | |
| ): { node: RungNode | undefined; position: number | undefined } => { | |
| return { | |
| node: rung.nodes.find((node) => node.id === nodeId), | |
| position: rung.nodes.findIndex((node) => node.id === nodeId), | |
| } | |
| } | |
| export const findNode = ( | |
| rung: RungLadderState, | |
| nodeId: string, | |
| ): { node: RungNode | undefined; position: number } => { | |
| return { | |
| node: rung.nodes.find((node) => node.id === nodeId), | |
| position: rung.nodes.findIndex((node) => node.id === nodeId), | |
| } | |
| } |
🤖 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/components/_molecules/graphical-editor/ladder/rung/ladder-utils/nodes.ts`
around lines 8 - 16, The declared return type for findNode says position can be
number | undefined but findIndex always returns a number; update the
implementation in findNode so the runtime matches the type by computing const
idx = rung.nodes.findIndex(n => n.id === nodeId) and returning position: idx ===
-1 ? undefined : idx (keep node as rung.nodes.find(...)); this ensures the
function's signature and behavior (RungLadderState, RungNode, findNode) are
consistent and callers can reliably check for undefined when not found.
Comment on lines
+16
to
+44
| /** | ||
| * Run an incoming flow through Zod so every `.default(...)` clause on the | ||
| * schema fires before the data lands in the store. Caller-side TypeScript | ||
| * types document the expected shape, but they can't catch drift coming from | ||
| * disk (legacy projects), from snapshot restore, or from unknown third-party | ||
| * producers. This is the slice's runtime safety net — making the slice the | ||
| * single trust boundary instead of relying on every caller to remember to | ||
| * normalize first. | ||
| * | ||
| * On parse failure, returns the flow as-is. The legacy `connectedVariables` | ||
| * migration in addLadderFlow still runs, so worst case is the editor sees | ||
| * the unparsed shape and may surface a runtime error downstream — which is | ||
| * still better than silently corrupting state. | ||
| */ | ||
| const parseFlowOrPassthrough = (flow: LadderFlowType): LadderFlowType => { | ||
| const result = zodLadderFlowSchema.safeParse({ name: flow.name, rungs: flow.rungs }) | ||
| if (!result.success) { | ||
| console.warn(`Failed to parse ladder flow "${flow.name}":`, result.error.issues) | ||
| return flow | ||
| } | ||
| return { | ||
| name: result.data.name, | ||
| updated: flow.updated, | ||
| rungs: result.data.rungs.map((rung, i) => ({ | ||
| ...rung, | ||
| selectedNodes: flow.rungs[i]?.selectedNodes ?? [], | ||
| })) as RungLadderState[], | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Risky error handling: parse failure returns potentially corrupted data.
When zodLadderFlowSchema.safeParse fails (line 32), the function returns the unparsed flow as-is (line 34), which the JSDoc acknowledges "may surface a runtime error downstream." This creates a silent failure path where invalid data propagates through the store.
While the console warning provides visibility, continuing with unparsed data can:
- Cause cryptic runtime errors far from the root cause
- Make debugging harder (users won't connect a ladder crash to a missing
handleBranchesproperty) - Allow partial corruption to persist across save/reload cycles
Consider instead:
- Apply defensive defaults for known-missing fields (e.g.,
handleBranches: []) even when parse fails - Or throw an error with actionable guidance for the user (e.g., "Project file is corrupted or from an incompatible version")
🛡️ Proposed defensive defaults on parse failure
const parseFlowOrPassthrough = (flow: LadderFlowType): LadderFlowType => {
const result = zodLadderFlowSchema.safeParse({ name: flow.name, rungs: flow.rungs })
if (!result.success) {
console.warn(`Failed to parse ladder flow "${flow.name}":`, result.error.issues)
- return flow
+ // Apply defensive defaults for known-missing fields to prevent runtime crashes
+ return {
+ ...flow,
+ rungs: flow.rungs.map(rung => ({
+ ...rung,
+ handleBranches: rung.handleBranches ?? [],
+ selectedNodes: rung.selectedNodes ?? [],
+ }))
+ }
}🤖 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/store/slices/ladder/slice.ts` around lines 16 - 44,
parseFlowOrPassthrough currently returns the raw unparsed flow on
zodLadderFlowSchema.safeParse failure, which lets corrupted shapes propagate;
instead, on parse failure build and return a sanitized fallback: keep flow.name
and flow.updated, set missing top-level fields (e.g., handleBranches: [],
connectedVariables: {} or the shape expected by addLadderFlow migration), and
normalize rungs to an array where each rung has required properties and
selectedNodes defaulted to [] (use flow.rungs values where safe but coerce
types/empty arrays as needed); log the full parse error with context via
console.error and return this defensively-defaulted flow rather than the
original unvalidated object so downstream code sees a consistent, safe shape
(update references in parseFlowOrPassthrough and any tests that rely on its
behavior).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
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
R) via a compact branch with its own local rail.frontend/,middleware/shared/,backend/shared/,__architecture__/.Test plan
Rboolean input — it appears as a branch with a local rail next to the FB.res1contact to CTURvia a branch and compile — whenres1is TRUE, the counter resets.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements