fix(web): keep shared edge routes orthogonal#57
Conversation
Ensure layout routes never render diagonal road segments and let straight sibling edges delay branch points for shared source trunks.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughDefers promotion of back-edge layout edges, orthogonalizes intermediate route points (inserting 90° corners), refines shared-source trunk bundling to avoid shifting straight routes, snaps ELK node y-positions to a row grid (shifting routes accordingly), and exports SELF_LOOP_WIDTH/SELF_LOOP_HEIGHT. Tests updated/added accordingly. ChangesFlow Layout: topology, orthogonalization, bundling, snapping, self-loop constants
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 8/10 reviews remaining, refill in 9 minutes and 28 seconds. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/lib/flow-layout.ts`:
- Around line 322-347: The handle-inference step is still receiving the raw ELK
polylines, so call orthogonalizeRoutePoints (or the same normalization used in
buildOrthogonalPath, i.e., orthogonalizeRoutePoints(dedupeRoutePoints(...)))
before passing routes into inferPortAssignmentsFromRoutes; locate where
inferPortAssignmentsFromRoutes is invoked and replace the raw route argument(s)
with the normalized/deduped route(s) to ensure port inference uses the
orthogonalized path (reference functions: orthogonalizeRoutePoints,
buildOrthogonalPath, inferPortAssignmentsFromRoutes).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 423ae476-d7e2-4311-9daa-60f51547a4b2
📒 Files selected for processing (2)
packages/web/__tests__/flow-layout.test.tspackages/web/src/lib/flow-layout.ts
…row grid Two layout issues hit examples/order-flow.yaml's cron node: 1. pushLayoutEdge unified sources and targets in one layoutSeen set, so any back-edge whose target was already seen got dropped entirely. For a node like cron that only feeds an already-seen target (cron -> order-service), this dropped its only edge and left ELK with a floating node placed on its own row. Defer back-edges and promote them post-pass for any source that otherwise has no layout edge. This keeps cron in the layered layout without changing the behavior for normal forward edges. 2. Even with the back-edge restored, ELK's per-column node placement produced y values that did not align across columns (cron at y=941 vs events at y=1000), breaking the visual row grid. Add a snapPositionsToRowGrid step that rounds each node's y to the nearest multiple of NODE_HEIGHT + spacing (240px) anchored at the first row's y. Same-row nodes stay on the same row; outliers like cron get pulled onto the nearest grid row alongside events. Adds a regression test loading examples/order-flow.yaml and asserting: - cron has a layout edge into order-service - cron and events share the same y after layout Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
snapPositionsToRowGrid changed each node's y to align to the row grid, but ELK had already produced routes against the un-snapped positions. Source-side route segments stayed at the old port y while the node moved, opening a 10px gap between the cardinal handle (computed from the snapped position) and ELK's route start. For self-loop edges this was visible: buildSelfLoopRoute uses the snapped node position to anchor the ear at cardinal y=360, while adjacent forward edges (api->worker, api->cache) drew from ELK's y=370 port. The self-loop appeared on a separate parallel road instead of branching from the shared trunk. snapPositionsToRowGrid now returns per-node y deltas. shiftRoutesAfterSnap walks each route and shifts source-side points (y matching route[0].y) by the source node's delta and target-side points by the target's delta, so source and target ends stay anchored to the snapped nodes without breaking orthogonality of the H-V-H middle bend. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
inferPortAssignmentsFromRoutes inspects the first and last route segments to decide which side each edge exits/enters. When ELK produced a polyline whose endpoint segment had a diagonal nub (rare but possible on non-strict-orthogonal output), the inference saw the wrong direction and picked an off-axis port handle. Run orthogonalizeRoutePoints(dedupeRoutePoints(route)) on every route before passing them to inferPortAssignmentsFromRoutes so the inference operates on the same corrected geometry that buildOrthogonalPath ultimately renders. Addresses CodeRabbit review on PR #57. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Test plan
npx tsx -e "... buildOrthogonalPath assertion ..."npx tsx -e "... bundleSharedSourcePrefixes assertion ..."npx tsc --noEmit -p packages/webhttp://localhost:8788/flow/8AdyEvHrojv0Note:
npm test -w @openhop/web -- flow-layout.test.ts ...is currently blocked locally because@openhop/sharedis not resolvable in the workspace install, andnpm installhit a WindowsEACCESonnode_modules\@openhop\shared.Summary by CodeRabbit
Bug Fixes
Tests