-
Notifications
You must be signed in to change notification settings - Fork 72
Arazzo extension fixed lay-outing issues #1421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR aims to fix multiple layout/rendering issues in the Arazzo designer visualizer’s vertical layout: reducing node/edge collisions, adding bridge rendering at line intersections, and improving label placement/styling. It also includes a change to portal creation logic for backward jumps.
Changes:
- Adjust vertical layout spacing constants and retry/condition branch positioning.
- Improve edge routing/collision avoidance and add “bridge” rendering for orthogonal edge intersections.
- Update portal creation rule for vertical-layout backward jumps and tweak label truncation/positioning.
Reviewed changes
Copilot reviewed 9 out of 15 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| workspaces/arazzo/arazzo-designer-visualizer/src/visitors/PositionVisitorVertical_v2.ts | Adds special Y placement for RETRY branch heads in vertical positioning. |
| workspaces/arazzo/arazzo-designer-visualizer/src/visitors/PortalCreator_v2.ts | Broadens portal creation for backward jumps in vertical layout. |
| workspaces/arazzo/arazzo-designer-visualizer/src/visitors/NodeFactoryVisitorVertical_v2.ts | Updates handle selection and waypoint routing; introduces shared geometry utilities and shift logic for collision avoidance. |
| workspaces/arazzo/arazzo-designer-visualizer/src/constants/nodeConstants.ts | Tweaks spacing constants and introduces label-related constants. |
| workspaces/arazzo/arazzo-designer-visualizer/src/components/edges/edgeUtils.ts | New shared geometry helpers for segment/rectangle intersection checks. |
| workspaces/arazzo/arazzo-designer-visualizer/src/components/edges/edgeIntersectDetect.ts | New helpers to detect intersections and generate SVG bridge arcs. |
| workspaces/arazzo/arazzo-designer-visualizer/src/components/edges/WaypointCreator.ts | Adds optional shift amount to adjust skip routing column and updates waypoint offsets. |
| workspaces/arazzo/arazzo-designer-visualizer/src/components/edges/PlannedPathEdge.tsx | Adds bridge-aware segment rendering and improves label positioning/truncation. |
| workspaces/arazzo/arazzo-designer-extension/src/extension.ts | Changes language server binary naming logic. |
| common/config/rush/pnpm-lock.yaml | Dependency lock updates. |
Files not reviewed (1)
- common/config/rush/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| nextY_RETRY = nodeY + node.viewState.h + C.RETRY_GAP_Y_ConditionBranch; | ||
| } else { | ||
| nextY = nodeY + node.viewState.h + C.NODE_GAP_Y_Vertical; | ||
| //nextY_RETRY = nextY; |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nextY_RETRY is declared as a number but is only assigned inside the node.type === 'CONDITION' branch. It’s later used when positioning branch heads, which will trigger a TS definite-assignment error (and could yield undefined at runtime if branches exist on a non-CONDITION node). Initialize nextY_RETRY to a safe default (e.g., same as nextY) in the non-CONDITION branch or immediately after computing nextY.
| //nextY_RETRY = nextY; | |
| nextY_RETRY = nextY; |
| const findshifts = (sourcePt: {x:number,y:number}, targetPt: {x:number,y:number}): number => { | ||
| let shifts = 0; | ||
| while (true) { | ||
| for (const nodePos of this.allNodePositions) { | ||
| if (nodePos.id === source.id || nodePos.id === target.id) continue; | ||
| const rect = { x: nodePos.x, y: nodePos.y, w: nodePos.w, h: nodePos.h }; | ||
| if (segmentIntersectsRect(sourcePt, targetPt, rect)) { | ||
| shifts++; | ||
| sourcePt.x += C.NODE_WIDTH; | ||
| targetPt.x += C.NODE_WIDTH; | ||
| break; | ||
| } | ||
| } | ||
| return shifts; | ||
| } | ||
|
|
||
| } |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findshifts currently returns shifts unconditionally at the end of the first while(true) iteration, so it can never perform more than one shift even when the shifted segment still intersects another node. This makes the collision-avoidance logic ineffective for multiple blockers. The return should only happen once a full pass finds no intersections (or the loop should be restructured to break/continue appropriately).
| import type { Point as P } from './WaypointCreator'; | ||
|
|
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import: import type { Point as P } from './WaypointCreator'; is not referenced in this module. Please remove it to avoid confusion and keep the module minimal.
| import type { Point as P } from './WaypointCreator'; |
| // Column: move right by approximately 1.5 * block.w from block.x | ||
| const columnX = block.x + block.w * C.WAYPOINT_SKIP_HORIZONTAL_OFFSET_MULTIPLIER; | ||
| const columnX = block.x + block.w + C.WAYPOINT_SKIP_HORIZONTAL_OFFSET + (C.NODE_WIDTH + C.NODE_GAP_X_Vertical) * (shiftAmount || 0); | ||
|
|
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment still says the routing column is "~1.5 * block.w", but the new columnX calculation is block.w + WAYPOINT_SKIP_HORIZONTAL_OFFSET + .... Update the comment to reflect the current formula so future adjustments don’t rely on outdated assumptions.
| import { Node, Edge, MarkerType } from '@xyflow/react'; | ||
| import * as C from '../constants/nodeConstants'; | ||
| import WaypointCreator from '../components/edges/WaypointCreator'; | ||
| import { pointInRect, segIntersectsSeg, segmentIntersectsRect } from '../components/edges/edgeUtils';; |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an extra semicolon at the end of this import (...edgeUtils';;). This is harmless at runtime but will typically fail lint/style checks; remove the duplicate ;.
| import { pointInRect, segIntersectsSeg, segmentIntersectsRect } from '../components/edges/edgeUtils';; | |
| import { pointInRect, segIntersectsSeg, segmentIntersectsRect } from '../components/edges/edgeUtils'; |
| // Gather other edges & nodes once (exclude current). | ||
| // NOTE: React Flow may not populate sourceX/targetX on edges returned by | ||
| // `useEdges()` in some render passes — fall back to node positions. | ||
| const allEdges = useEdges(); | ||
| const otherEdges = (allEdges || []).filter(e => e.id !== id); | ||
| // Get nodes from React Flow so we can reconstruct edge endpoints when | ||
| // `sourceX/sourceY` or `targetX/targetY` are not available on the edge object. | ||
| const nodes = useNodes() as any[] || []; | ||
| const nodeById = new Map((nodes || []).map((n: any) => [n.id, n])); | ||
|
|
||
| // Forwarding wrapper that uses the centralized helper (keeps behaviour) | ||
| const getPointsForEdge = (e: any) => getEdgePoints(e, nodeById); | ||
|
|
||
| // Emit a straight segment but inject bridge arcs when crossing | ||
| // orthogonal segments from other edges (supports vertical *and* horizontal). | ||
| let pen = { x: points[0].x, y: points[0].y }; | ||
| const emitLineWithBridges = (from: { x: number; y: number }, to: { x: number; y: number }) => { | ||
| const intersections = detectBridgesForSegment(from, to, otherEdges as any[], nodeById, BRIDGE_RADIUS, DEFAULT_EPS); | ||
| const frag = buildSegmentPathWithBridges(from, to, intersections, BRIDGE_RADIUS); | ||
| pen = { x: to.x, y: to.y }; | ||
| return frag; | ||
| }; |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bridge detection recomputes intersections by scanning all other edges for every straight segment, on every render of each edge. This is O(E² × segments) work and can become a bottleneck on larger graphs. Consider memoizing otherEdges/nodeById and/or caching the detected intersections per edge/segment (e.g., via useMemo) so the path doesn’t fully recompute unless edges/nodes/waypoints actually change.
| // Forwarding wrapper that uses the centralized helper (keeps behaviour) | ||
| const getPointsForEdge = (e: any) => getEdgePoints(e, nodeById); | ||
|
|
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable getPointsForEdge.
| // Forwarding wrapper that uses the centralized helper (keeps behaviour) | |
| const getPointsForEdge = (e: any) => getEdgePoints(e, nodeById); |
| ): DetectedIntersection[] { | ||
| const dx = to.x - from.x; | ||
| const dy = to.y - from.y; | ||
| const isVertical = Math.abs(dx) <= eps && Math.abs(dy) > eps; |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable isVertical.
| const isVertical = Math.abs(dx) <= eps && Math.abs(dy) > eps; |
| import { Node, Edge, MarkerType } from '@xyflow/react'; | ||
| import * as C from '../constants/nodeConstants'; | ||
| import WaypointCreator from '../components/edges/WaypointCreator'; | ||
| import { pointInRect, segIntersectsSeg, segmentIntersectsRect } from '../components/edges/edgeUtils';; |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused imports pointInRect, segIntersectsSeg.
| import { pointInRect, segIntersectsSeg, segmentIntersectsRect } from '../components/edges/edgeUtils';; | |
| import { segmentIntersectsRect } from '../components/edges/edgeUtils';; |
| return shifts; | ||
| } | ||
|
|
||
| } |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid automated semicolon insertion (97% of all statements in the enclosing function have an explicit semicolon).
| } | |
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 18 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- common/config/rush/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isTargetRight = target.viewState.x > source.viewState.x + source.viewState.w; | ||
| const isTargetRight = target.viewState.x + target.viewState.w/2 > source.viewState.x + source.viewState.w/2; | ||
| const isTargetAbove = target.viewState.y < source.viewState.y; | ||
| const isTargetLeft = target.viewState.x + target.viewState.w/2 < source.viewState.x + source.viewState.w/2; //add margeins |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: //add margeins should be // add margins.
| const isTargetLeft = target.viewState.x + target.viewState.w/2 < source.viewState.x + source.viewState.w/2; //add margeins | |
| const isTargetLeft = target.viewState.x + target.viewState.w/2 < source.viewState.x + source.viewState.w/2; // add margins |
| specifier: 5.8.3 | ||
| version: 5.8.3 | ||
| webpack: | ||
| specifier: ^5.94.0 | ||
| specifier: ^5.88.2 | ||
| version: 5.104.1([email protected]) | ||
| webpack-cli: |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lockfile hunk shows tooling dependency specifier/version changes (e.g., webpack specifier changes, and multiple version downgrades). These look unrelated to the PR’s stated layout goals; if they’re from an accidental lockfile regen, consider reverting/minimizing them or documenting why the dependency changes are needed, since they can impact builds across the repo.
| }else if(scenario === 'branch') { | ||
| // For branches, add a slight horizontal offset to the label position to avoid overlap with the node | ||
| computedWaypoints = WaypointCreator(sourcePt, targetPt, foundBlockingRect, 'branch'); | ||
| computedWaypoints = WaypointCreator(sourcePt, targetPt, foundBlockingRect, 'branch',); |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WaypointCreator(...) call has an extra trailing comma with no argument: WaypointCreator(sourcePt, targetPt, foundBlockingRect, 'branch',);. This is a syntax error and will fail to compile. Remove the extra comma or pass the intended 5th argument.
| computedWaypoints = WaypointCreator(sourcePt, targetPt, foundBlockingRect, 'branch',); | |
| computedWaypoints = WaypointCreator(sourcePt, targetPt, foundBlockingRect, 'branch'); |
| } | ||
| } | ||
|
|
||
| } |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid automated semicolon insertion (97% of all statements in the enclosing function have an explicit semicolon).
| } | |
| }; |
Goals
Approach