fix(layout.DirectedGraph): better error message when dagre fails to connect to container #2838
Conversation
ee6df96 to
8204e21
Compare
|
This PR is stale because it has been open 60 days with no activity. Please remove stale label or comment or this will be closed in 14 days. |
|
This PR is stale because it has been open 60 days with no activity. Please remove stale label or comment or this will be closed in 14 days. |
…onnect to container
8204e21 to
7f0eb64
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds a new validation step to DirectedGraph.layout to catch invalid links between container and child elements and introduces tests for both validation-enabled and disabled modes.
- Introduces a
validateGraphmethod that throws a clear error when linking a child to its container. - Adds a
validateGraphoption to the layout API (defaulttrue) and wires it intolayout. - Adds QUnit tests covering scenarios where validation is on or off.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/joint-layout-directed-graph/test/index.js | New QUnit test for container–child connection errors |
| packages/joint-layout-directed-graph/DirectedGraph.mjs | Added validateGraph method and integrated its use |
Comments suppressed due to low confidence (1)
packages/joint-layout-directed-graph/DirectedGraph.mjs:123
- [nitpick] Enhance this error message with contextual information (e.g. source and target element IDs) to make debugging easier, for example:
DirectedGraph: cannot connect child ${source.id} to container ${target.id}.
throw new Error('DirectedGraph: It is not possible to connect a child to a container.');
There was a problem hiding this comment.
Pull Request Overview
This PR adds pre-layout validation to DirectedGraph to catch invalid parent/child connections earlier and provides clearer error messaging when such cases occur.
- Introduces
validateGraphto detect links between containers and children before invoking Dagre. - Updates
layoutto callvalidateGraphbased on thevalidateGraphoption. - Adds new tests in
test/index.jsto verify the custom error message and fallback behavior when validation is disabled.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/joint-layout-directed-graph/test/index.js | New QUnit tests covering invalid container/child links and error messages |
| packages/joint-layout-directed-graph/DirectedGraph.mjs | Added validateGraph method and hooked it into layout |
Comments suppressed due to low confidence (1)
packages/joint-layout-directed-graph/DirectedGraph.mjs:126
- [nitpick] The error message only mentions "child to a container" but this validator also catches "container to child" and "container to container" cases. Consider updating the message (or making it parametric) to accurately reflect all invalid connection types.
throw new Error('DirectedGraph: It is not possible to connect a child to a container.');
|
This PR is stale because it has been open 60 days with no activity. Please remove stale label or comment or this will be closed in 14 days. |
Description
Provides a better error message when dagre fails due to trying to connect a child to a container.
See #455 for more information and workarounds.