Fix routing of same-scope named-port edges and handle boundary-crossing#13
Merged
Merged
Conversation
…hrow on boundary-crossing port edges - HierarchicalLayoutAlgorithm.ClassifyEdges previously silently dropped any edge with a LayoutGraphPort endpoint, even when the edge did not cross a container boundary, whenever the scope contained any container node elsewhere. This broke named-port support (Phase 1) as soon as any containment existed anywhere in the graph. - Add portOwner map and TryResolveOwner helper to resolve port endpoints to their owning node for scope classification. - Same-scope port edges (both endpoints direct members of current scope) are now routed normally through to LayeredLayoutAlgorithm, which already supports LayoutGraphPort endpoints via TryResolveEndpoint. - Genuinely boundary-crossing port edges now throw a clear NotSupportedException instead of being silently dropped. - BuildSizedView/TryMapConnectable carry Ports onto view nodes and remap leaf edge endpoints; LayoutScope extracts LayoutPort nodes from leaf output; Translate gained a LayoutPort case for correct nested-scope translation. - Add 3 unit tests covering same-scope port routing, boundary-crossing exception, and nested container port translation. - Update ROADMAP.md, design doc, verification doc, and reqstream yaml.
There was a problem hiding this comment.
Pull request overview
Fixes a bug in HierarchicalLayoutAlgorithm where edges involving LayoutGraphPort endpoints were incorrectly dropped in hierarchical scopes, and formalizes unsupported boundary-crossing port-edge behavior by throwing a clear exception. This aligns hierarchical behavior with existing flat-graph port routing while making current limitations explicit across code, tests, requirements, and verification docs.
Changes:
- Correctly classify and route same-scope named-port edges via the selected leaf algorithm, including copying named ports into the sized view graph.
- Fail loudly for boundary-crossing named-port edges by throwing
NotSupportedExceptioninstead of silently dropping. - Add regression tests plus requirements/design/verification documentation updates describing both behaviors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/DemaConsulting.Rendering.Layout.Tests/HierarchicalLayoutAlgorithmTests.cs | Adds regression tests for same-scope port routing, boundary-crossing port-edge exception, and port coordinate translation during composition. |
| src/DemaConsulting.Rendering.Layout/HierarchicalLayoutAlgorithm.cs | Updates edge classification to resolve port ownership, copies ports into the sized view, includes emitted LayoutPorts in output, and translates ports during composition. |
| ROADMAP.md | Documents the fixed bug and clarifies the interim “throw on boundary-crossing port edges” limitation pending Phase 2. |
| docs/verification/rendering-layout/hierarchical-layout-algorithm.md | Extends verification scenarios and requirement-to-test mapping for same-scope port edges and boundary-crossing exceptions. |
| docs/reqstream/rendering-layout/hierarchical-layout-algorithm.yaml | Adds new requirements covering same-scope port routing and boundary-crossing port-edge exception behavior. |
| docs/design/rendering-layout/hierarchical-layout-algorithm.md | Updates design documentation to include port copying/routing behavior and the explicit exception for unsupported boundary-crossing port edges. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+578
to
+580
| const string boundaryPortMessage = | ||
| "Named ports on edges crossing a container boundary are not yet supported; see ROADMAP.md Phase 2."; | ||
|
|
| /// <param name="connectable">The edge endpoint to resolve.</param> | ||
| /// <param name="portOwner">Map from every descendant's named port to the node that owns it.</param> | ||
| /// <param name="owner">The resolved owning node, when resolution succeeds.</param> | ||
| /// <returns><see langword="true"/> when the endpoint resolves to a node in this scope's graph.</returns> |
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
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.
This pull request addresses a previously undocumented but significant bug in the
HierarchicalLayoutAlgorithmrelated to handling edges connected to named ports (LayoutGraphPort) within hierarchical graph layouts. The changes ensure that same-scope port edges are routed correctly and that unsupported boundary-crossing port edges now fail loudly with a clear exception. The documentation, requirements, and verification files have been updated to reflect and test these behaviors.Bug Fixes and Routing Behavior
NotSupportedExceptionwith a clear message, rather than silently dropping the edge. This makes the limitation explicit and safer for users. [1] [2] [3]Documentation and Requirements
Implementation Details