Refine workflow edge routing #507
Merged
Conversation
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
brandur
approved these changes
Feb 11, 2026
Collaborator
brandur
left a comment
brandur
left a comment
There was a problem hiding this comment.
Dang, this is a lot of code. Going to assume 100% LLM generated? A little scary that I definitely could not fix a problem in here without an LLM there to help.
Still, definitely good to have this fixed up. Thanks!
bgentry
force-pushed
the
bg/workflow-no-turn-zone-routing
branch
2 times, most recently
from
bd35f4f to
174b17b
Compare
Workflow dependency graphs with fan-in/fan-out could still be misleading in several cases: bends could occur under sibling cards, long horizontal segments could run through node bodies, and direct upstream dependencies could appear to terminate at separate nearby merge points. Those patterns made true dependency relationships harder to read at a glance. The routing logic now keeps the original Manhattan style by default, but validates each candidate lane against stricter constraints. Edge selection rejects lanes whose turns fall inside node no-turn zones and also rejects lanes whose orthogonal segments intersect non-endpoint nodes. For mixed fan-in targets, layout now adds a shared `preferredBendX` hint so off-row incoming edges converge on a common merge lane when safe. Coverage was expanded with focused routing tests for node-body crossing avoidance, near-target lane fallback in same-row sibling scenarios, shared preferred merge-lane convergence, and merge-hint assignment at the layout layer.
Workflow diagram logic had grown monolithic in `src/components/WorkflowDiagram.tsx`, mixing React wiring with graph construction, Dagre layout, and edge styling. It also crashed on some historical payloads where `metadata.deps` was missing, because the graph model read `job.metadata.deps` directly. This refactor moves workflow diagram code into `src/components/workflow-diagram/` and separates responsibilities across `workflowDiagramGraphModel.ts`, `workflowDiagramLayout.ts`, `workflowDiagramGeometry.ts`, and `workflowDiagramConstants.ts`, while keeping edge-path routing and merge hints in dedicated modules. The model builder now normalizes dependency reads with `job.metadata.deps ?? []` so missing deps are treated as empty lists. The change preserves rendering behavior and routing parity while improving maintainability and preventing runtime failures for malformed or historical rows. Coverage adds regressions for missing `metadata.deps` in `WorkflowDiagram.test.tsx` and `workflowDiagramGraphModel.test.ts`, alongside relocated routing and merge-hint tests in the new module folder.
Workflow diagram tests in `WorkflowDiagram.test.tsx` and `workflowDiagramGraphModel.test.ts` each defined a local `workflowJob` fixture with nearly identical defaults. That duplication made workflow metadata drift likely when fields like `deps` or `workflow_id` changed in one file but not the other. Add a shared `workflowJobFactory` in `src/test/factories/workflowJob.ts`, built on top of `jobFactory`. The factory centralizes workflow-specific metadata defaults (`deps`, `task`, `workflow_id`, `workflow_staged_at`) while still allowing per-test overrides for state and dependency scenarios. Both workflow diagram test files now use the shared factory and no longer carry their own fixture builder logic. This keeps existing behavior coverage while reducing fixture boilerplate and making future test updates easier to apply consistently.
bgentry
force-pushed
the
bg/workflow-no-turn-zone-routing
branch
from
174b17b to
4df7817
Compare
Contributor
Author
|
@brandur yep, I definitely share some concerns around maintainability and appreciate you raising it! ❤️ I did a bunch of extra refactoring in subsequent commits to:
Unfortunately these have the result of adding significantly more lines of code than we had as of my first commit, though most of it is tests and a fair bit of documentation comments. I think it's an overall win and is at least very well isolated from the rest of the repo. Definitely worth fixing the edge routing issues IMO, I was having such a hard time debugging my own work with them previously. |
Collaborator
|
@bgentry Awesome. Sounds good. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Workflow dependency graphs with fan-in/fan-out could still be misleading in several cases: bends could occur under sibling cards, long horizontal segments could run through node bodies, and direct upstream dependencies could appear to terminate at separate nearby merge points. Those patterns made true dependency relationships harder to read at a glance.
The routing logic now keeps the original Manhattan style by default, but validates each candidate lane against stricter constraints. Edge selection rejects lanes whose turns fall inside node no-turn zones and also rejects lanes whose orthogonal segments intersect non-endpoint nodes. For mixed fan-in targets, layout now adds a shared
preferredBendXhint so off-row incoming edges converge on a common merge lane when safe.Coverage was expanded with focused routing tests for node-body crossing avoidance, near-target lane fallback in same-row sibling scenarios, shared preferred merge-lane convergence, and merge-hint assignment at the layout layer.
Notice how in the before case there's a line connecting
AgentPlanExecutiondirectly toAgentVerifyDraft, but it disappears behind the intermediateAgentCallTooltask. That's solved now including test coverage.