From 9f46101e3ef30af7e18c1dc21e7921e728f40e34 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Wed, 24 Jun 2026 02:28:26 -0400 Subject: [PATCH] fix(ladder): correct handle-branch handling across add, delete, duplicate and XML Four related handle-branch defects in the ladder editor: - Adding an element to a block's primary output (e.g. a coil on a counter's Q/QU) spliced it into a secondary-handle branch edge, dropping the element and breaking the branch (the originally reported "coil doesn't show on Q, contact-to-R connection breaks"). The serial predecessor was resolved by array index, which picks the handle-branch contact interleaved between the block and the right rail. getPreviousElement now skips branchContext nodes (mirroring getPreviousElementsByEdge) and is keyed by element id. - handleBranches is runtime-only state (not persisted in .ld); rebuild it from the graph on project load via deriveHandleBranches so the first branch-aware edit no longer corrupts diagrams that contain handle branches. - Duplicating a rung rebuilt blocks at default dimensions without re-running the layout solver, and dropped branchContext remapping on branch parallel nodes; re-run updateDiagramElementsPosition and remap. - PLCopen XML serialized every block-output branch to the primary output pin (QU); resolve the actual edge sourceHandle so QU/QD are distinguished. Adds a regression test for getPreviousElement. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../elements/handle-branch/index.ts | 34 +++++++++ .../ladder-utils/elements/serial/index.ts | 5 +- .../__tests__/get-previous-element.test.ts | 74 +++++++++++++++++++ .../rung/ladder-utils/elements/utils/index.ts | 31 ++++++-- src/frontend/store/slices/ladder/slice.ts | 14 +++- .../store/slices/ladder/utils/index.ts | 22 +++++- .../codesys/language/ladder-xml.ts | 20 ++++- 7 files changed, 184 insertions(+), 16 deletions(-) create mode 100644 src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/utils/__tests__/get-previous-element.test.ts diff --git a/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/handle-branch/index.ts b/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/handle-branch/index.ts index c703198ca..62bd627e0 100644 --- a/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/handle-branch/index.ts +++ b/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/handle-branch/index.ts @@ -1326,3 +1326,37 @@ export function reconcileBranchNodeIds(rung: RungLadderState, branch: HandleBran return nodeIds } + +/** + * Rebuild the handle-branch index from a rung's graph. + * + * `handleBranches` is runtime-only state — it is NOT persisted in the .ld file, + * so a project loaded from disk that contains handle branches (contacts/coils + * wired to a block's secondary input/output handles, e.g. CTUD CD/QD) comes + * back with an empty index. Branch-aware operations (deletion cleanup, + * `getBranch`, nodeId reconciliation) then can't find those branches and + * corrupt the diagram on the first edit. We reconstruct the index from the + * `branchContext` stamped on each branch node plus the block's handle + * directions, deriving the ordered nodeIds via `reconcileBranchNodeIds`. + */ +export function deriveHandleBranches(rung: RungLadderState): HandleBranch[] { + const seen = new Map() + for (const node of rung.nodes) { + const ctx = (node.data as BasicNodeData).branchContext + if (ctx?.blockId && ctx?.handleId) { + seen.set(`${ctx.blockId}::${ctx.handleId}`, { blockId: ctx.blockId, handleId: ctx.handleId }) + } + } + + const branches: HandleBranch[] = [] + for (const { blockId, handleId } of seen.values()) { + const block = rung.nodes.find((n) => n.id === blockId) + if (!block) continue + const data = block.data as BasicNodeData + const isOutput = (data.outputHandles ?? []).some((h) => h.id === handleId) + const direction: 'input' | 'output' = isOutput ? 'output' : 'input' + const nodeIds = reconcileBranchNodeIds(rung, { blockId, handleId, direction, nodeIds: [] }) + if (nodeIds.length > 0) branches.push({ blockId, handleId, direction, nodeIds }) + } + return branches +} diff --git a/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/serial/index.ts b/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/serial/index.ts index 6a0b785df..4369047ac 100644 --- a/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/serial/index.ts +++ b/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/serial/index.ts @@ -57,10 +57,7 @@ export const appendSerialConnection = ( /** * Get the previous node */ - let previousNode = getPreviousElement( - { ...rung, nodes: newNodes, edges: newEdges }, - newNodes.findIndex((n) => n.id === newElement.id), - ) + let previousNode = getPreviousElement({ ...rung, nodes: newNodes, edges: newEdges }, newElement.id) /** * If the related node is a parallel, check if it is an open or close parallel diff --git a/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/utils/__tests__/get-previous-element.test.ts b/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/utils/__tests__/get-previous-element.test.ts new file mode 100644 index 000000000..292ae7ee6 --- /dev/null +++ b/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/utils/__tests__/get-previous-element.test.ts @@ -0,0 +1,74 @@ +import type { Node } from '@xyflow/react' + +import type { RungLadderState } from '@root/frontend/store/slices' + +import { getPreviousElement } from '../index' + +/** + * Minimal node/rung factories — getPreviousElement only reads node.type, + * node.id and node.data.branchContext. + */ +const node = ( + id: string, + type: string, + branchContext?: { blockId: string; handleId: string; direction: 'input' | 'output' }, +): Node => + ({ + id, + type, + position: { x: 0, y: 0 }, + data: branchContext ? { branchContext } : {}, + }) as unknown as Node + +const rung = (nodes: Node[]): RungLadderState => ({ nodes }) as unknown as RungLadderState + +describe('getPreviousElement', () => { + // Regression: adding a coil to a counter block's primary output corrupted a + // handle branch (e.g. a contact on the R input). Branch elements are + // interleaved in the node array between the block and the right rail, so a + // freshly-inserted main-line coil's placeholder sits AFTER the branch contact. + // The old index-based walk picked the branch contact as the serial + // predecessor and spliced the coil into the branch edge (dropping the coil and + // breaking the branch). The predecessor must be the block, skipping the branch. + it('returns the main-line predecessor, skipping handle-branch elements', () => { + const r = rung([ + node('left-rail', 'powerRail'), + node('cu-contact', 'contact'), + node('block', 'block'), + node('r-contact', 'contact', { blockId: 'block', handleId: 'R', direction: 'input' }), + node('new-coil', 'coil'), + node('right-rail', 'powerRail'), + ]) + + const previous = getPreviousElement(r, 'new-coil') + + expect(previous.id).toBe('block') + expect(previous.id).not.toBe('r-contact') + }) + + it('skips placeholders and variables when resolving the predecessor', () => { + const r = rung([ + node('left-rail', 'powerRail'), + node('contact-1', 'contact'), + node('a-variable', 'variable'), + node('a-placeholder', 'placeholder'), + node('new-contact', 'contact'), + node('right-rail', 'powerRail'), + ]) + + expect(getPreviousElement(r, 'new-contact').id).toBe('contact-1') + }) + + it('skips multiple consecutive branch elements before the inserted node', () => { + const r = rung([ + node('left-rail', 'powerRail'), + node('block', 'block'), + node('cd-contact', 'contact', { blockId: 'block', handleId: 'CD', direction: 'input' }), + node('r-contact', 'contact', { blockId: 'block', handleId: 'R', direction: 'input' }), + node('new-coil', 'coil'), + node('right-rail', 'powerRail'), + ]) + + expect(getPreviousElement(r, 'new-coil').id).toBe('block') + }) +}) diff --git a/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/utils/index.ts b/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/utils/index.ts index ad78836e6..646bc8307 100644 --- a/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/utils/index.ts +++ b/src/frontend/components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/utils/index.ts @@ -65,19 +65,36 @@ export const getPreviousElementsByEdge = ( } /** - * Get the previous node when adding a new element - * It works when removing the placeholder and variables elements + * Get the previous main-line element when adding a new serial element. + * + * Walks the rung's node list skipping placeholders, variables and branch + * elements, then returns the element immediately before the just-inserted one. + * + * Branch elements (contacts/coils wired to a block's secondary handles, e.g. a + * contact on a counter's R input) are interleaved in the node array between the + * block and the right rail, but they are NOT part of the serial spine. A plain + * index walk that included them would treat such a branch element as the serial + * predecessor of a main-line element — so a coil added to a block's primary + * output (whose placeholder sits after the branch element in the array) would be + * wired into the branch edge instead, dropping the coil and breaking the branch. + * Skipping branchContext nodes here mirrors getPreviousElementsByEdge. Keying off + * the element id (rather than an externally-computed index) keeps the lookup + * consistent with this filtered spine. * * @param rung: RungLadderState - * @param nodeIndex: number + * @param newElementId: string * * @returns Node */ -export const getPreviousElement = (rung: RungLadderState, nodeIndex: number): Node => { - const nodesWithNoPlaceholderAndVariables = rung.nodes.filter( - (n) => n.type !== 'placeholder' && n.type !== 'parallelPlaceholder' && n.type !== 'variable', +export const getPreviousElement = (rung: RungLadderState, newElementId: string): Node => { + const serialSpine = rung.nodes.filter( + (n) => + n.type !== 'placeholder' && + n.type !== 'parallelPlaceholder' && + n.type !== 'variable' && + !(n.data as BasicNodeData).branchContext, ) - return nodesWithNoPlaceholderAndVariables[nodeIndex - 1] + return serialSpine[serialSpine.findIndex((n) => n.id === newElementId) - 1] } /** diff --git a/src/frontend/store/slices/ladder/slice.ts b/src/frontend/store/slices/ladder/slice.ts index 469e99f6d..983ad059d 100644 --- a/src/frontend/store/slices/ladder/slice.ts +++ b/src/frontend/store/slices/ladder/slice.ts @@ -9,6 +9,7 @@ import { } from '../../../components/_atoms/graphical-editor/ladder/node-builders' import type { LadderBlockConnectedVariables } from '../../../components/_atoms/graphical-editor/ladder/utils/types' import { removeElements } from '../../../components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements' +import { deriveHandleBranches } from '../../../components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/handle-branch' import { LadderFlowSlice, LadderFlowState } from './types' import { duplicateLadderRung } from './utils' @@ -56,9 +57,20 @@ export const createLadderFlowSlice: StateCreator ({ ...rung, selectedNodes: [] })) + // handleBranches (the index of contacts/coils wired to a block's + // secondary handles, e.g. CTUD CD/QD) is runtime-only state — it is + // NOT persisted in the .ld. Without rebuilding it on load, a project + // containing handle branches comes back with an empty index and the + // first branch-aware edit (e.g. deleting a coil on a block output) + // corrupts the diagram. Reconstruct it from the graph here. + const rungsWithBranches = rungs.map((rung) => ({ + ...rung, + handleBranches: deriveHandleBranches(rung), + })) + // Reset updated to false on load — the flow is being loaded from a saved project. // Only mark as updated if legacy data was migrated so the next save writes the new format. - const newFlow = { ...flow, rungs, updated: needsMigration } + const newFlow = { ...flow, rungs: rungsWithBranches, updated: needsMigration } if (flowIndex === -1) { ladderFlows.push(newFlow) diff --git a/src/frontend/store/slices/ladder/utils/index.ts b/src/frontend/store/slices/ladder/utils/index.ts index 6e5bbdda0..964151d8d 100644 --- a/src/frontend/store/slices/ladder/utils/index.ts +++ b/src/frontend/store/slices/ladder/utils/index.ts @@ -15,6 +15,7 @@ import type { PowerRailNode, VariableNode, } from '../../../../components/_atoms/graphical-editor/ladder/utils/types' +import { updateDiagramElementsPosition } from '../../../../components/_molecules/graphical-editor/ladder/rung/ladder-utils/elements/diagram' import { generateNumericUUID } from '../../../../utils/generate-uuid' import { newGraphicalEditorNodeID } from '../../../../utils/new-graphical-editor-node-id' import { RungLadderState } from '../types' @@ -129,6 +130,7 @@ export const duplicateLadderRung = (editorName: string, rung: RungLadderState): } as ContactNode } case 'parallel': { + const parallelData = node.data as BasicNodeData return { ...node, id: nodeMaps[node.id].id, @@ -141,6 +143,16 @@ export const duplicateLadderRung = (editorName: string, rung: RungLadderState): parallelOpenReference: (node as ParallelNode).data.parallelOpenReference ? nodeMaps[(node as ParallelNode).data.parallelOpenReference ?? ''].id : undefined, + // Branch parallel nodes (a parallel inside a handle branch) carry a + // branchContext whose blockId must be remapped to the duplicated + // block — same as coils/contacts. Without this the copy's parallel + // nodes point at the original block id, breaking branch operations. + ...(parallelData.branchContext && { + branchContext: { + ...parallelData.branchContext, + blockId: nodeMaps[parallelData.branchContext.blockId]?.id ?? parallelData.branchContext.blockId, + }, + }), }, } as ParallelNode } @@ -208,7 +220,15 @@ export const duplicateLadderRung = (editorName: string, rung: RungLadderState): }), } - return newRung + // Blocks/coils/contacts are rebuilt at their DEFAULT dimensions by + // nodesBuilder, so a block that was branch-expanded in the source rung comes + // back compact while its branch elements keep the expanded positions — + // leaving the duplicated diagram visually broken. Re-run the (branch-aware) + // layout solver so the block re-expands and every element is repositioned + // against the actual node set. + const laidOut = updateDiagramElementsPosition(newRung, newRung.defaultBounds as [number, number]) + + return { ...newRung, nodes: laidOut.nodes, edges: laidOut.edges } } /** diff --git a/src/frontend/utils/PLC/xml-generator/codesys/language/ladder-xml.ts b/src/frontend/utils/PLC/xml-generator/codesys/language/ladder-xml.ts index 457bc0bde..c5d0cdf1f 100644 --- a/src/frontend/utils/PLC/xml-generator/codesys/language/ladder-xml.ts +++ b/src/frontend/utils/PLC/xml-generator/codesys/language/ladder-xml.ts @@ -90,6 +90,20 @@ const findConnections = ( ) => { const { nodes: rungNodes, edges: rungEdges } = rung + // Resolve the formal parameter (block output pin) for a source node that + // feeds into a parallel chain. A block's `outputConnector` only records its + // PRIMARY output (e.g. QU on a CTUD_DINT), so reading it directly maps EVERY + // branch back to that one pin — which is why coils wired to a secondary + // output (QD) were serialized as connected to QU. Instead, use the handle of + // the edge that actually leaves the node into the parallel chain (QU vs QD). + // Falls back to the default output connector for single-output elements; + // 'OUT' (the unnamed function return) maps to an empty formal parameter. + const formalParameterFromParallel = (srcNode: Node, parallelChain: ParallelNode[]): string => { + const edge = rungEdges.find((e) => e.source === srcNode.id && parallelChain.some((p) => p.id === e.target)) + const handle = edge?.sourceHandle ?? srcNode.data.outputConnector?.id ?? '' + return handle === 'OUT' ? '' : handle + } + const connectedEdges = rungEdges.filter( (edge) => edge.target === node.id && (targetHandle === undefined || edge.targetHandle === targetHandle), ) @@ -148,7 +162,7 @@ const findConnections = ( if (lastParallelSerialEdge && lastParallelSerialEdge.target === node.id) { return nodes.map((node, index) => ({ '@refLocalId': node.data.numericId, - '@formalParameter': node.data.outputConnector?.id === 'OUT' ? '' : node.data.outputConnector?.id || '', + '@formalParameter': formalParameterFromParallel(node, parallels), position: index === 0 ? [ @@ -220,7 +234,7 @@ const findConnections = ( return nodes.map((node) => { return { '@refLocalId': node.data.numericId, - '@formalParameter': node.data.outputConnector?.id === 'OUT' ? '' : node.data.outputConnector?.id || '', + '@formalParameter': formalParameterFromParallel(node, parallels), position: [ // Final edge destination { @@ -255,7 +269,7 @@ const findConnections = ( const closeConnections = nodes.map((node, index) => { return { '@refLocalId': node.data.numericId, - '@formalParameter': node.data.outputConnector?.id === 'OUT' ? '' : node.data.outputConnector?.id || '', + '@formalParameter': formalParameterFromParallel(node, parallels), position: index === 0 ? [