feat(geometry): suggest a verified via elbow for clean-flow routing diagnostics - #308
Open
julianwirawan-hash wants to merge 1 commit into
Open
Conversation
…iagnostics
clean-flow/edge-through-node and clean-flow/endpoint-side-direction only
ever offered generic prose ("adjust fromSide/toSide, set route/via or
channel coordinates...") even though authoring a correct 2-point via
elbow for a cross-lane or reverse-flow edge is a well-known, mechanical
recipe: leave the source anchor and enter the target anchor
perpendicular to their sides, with one shared coordinate strictly
between the two lanes. In practice this recipe was hand-applied and
hand-verified per edge, per diagram, every time.
Add suggestClearingVia(): given the two endpoint rects, their
fromSide/toSide, and the obstacle set, it samples candidate midpoints
for that elbow and returns the first one whose three segments are
verified clear of every obstacle (via the same segmentIntersectsRect
primitive the diagnostics themselves use) and that honors both
endpoint-side direction contracts (via routeHonorsEndpointSides).
Returns null -- not a guess -- when the two sides aren't both
vertical or both horizontal, when source and target share the
elbow's cross-axis coordinate (would collapse to a degenerate
zero-length via), or when no sampled midpoint clears every obstacle.
Wired into cleanFlowProblems (all renderers, since obstacles was
already a required parameter there) and into cleanEndpointSideProblems
via a new optional "obstacles" parameter (backward compatible --
existing callers that don't pass it keep today's generic-hint
behavior unchanged). Enabled the endpoint-side-direction suggestion
specifically for the lifecycle renderer's transitions, since that's
where this diagnostic is most commonly hit in cross-lane/reverse-flow
diagrams.
Testing: added 7 tests to geometry.test.mjs covering suggestClearingVia
directly (a genuine obstacle it routes around, an unsolvable case that
correctly returns null rather than guessing, and a declined
mixed-side-combination case) plus both diagnostic functions' new
supportedFixes/evidence.suggestedVia output and backward-compatible
fallback. Caught and fixed a real bug during testing: an early version
could suggest a degenerate via (two identical points) when source and
target shared the same cross-axis coordinate -- now explicitly guarded
against.
Verified no regression in every other test file that exercises these
shared functions: repair-receipt.test.mjs + workflow-compiler.test.mjs
(97/97, covers the architecture and workflow renderers' use of the
same functions -- including a real fixture where this change legitimately
improved an existing diagnostic's supportedFixes text, requiring a
one-line assertion update to match), architecture-delta.test.mjs +
sequence-column-fit.test.mjs (24/24). Note: running the full suite as
one "node --test test/*.mjs" glob hangs indefinitely in my sandboxed
environment (unrelated to this change -- reproduces the same way on
unpatched main, likely the same live-server-lifecycle issue behind the
pre-existing 'preview runs from an installed skill' flake); every
targeted file run above completes normally and cleanly.
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.
Problem and value
clean-flow/edge-through-node(an edge crossing an unrelated node) andclean-flow/endpoint-side-direction(a route not leaving/entering perpendicular to its authored side) only ever offer generic prose assupportedFixes— e.g. "adjust fromSide/toSide, set route/via or channel coordinates, or move the obstacle." Solving a correct 2-pointviaelbow for a cross-lane or reverse-flow edge by hand is a well-known, mechanical recipe (leave/enter perpendicular to the authored sides, one shared coordinate strictly between the two lanes) — but it had to be hand-solved and hand-verified per edge, every time, with no help from the tool beyond the generic hint. No existing issue link — found this authoring denselifecyclediagrams with cross-lane transitions.Scoped narrower than a full fix on purpose: this does not address arrow-vs-arrow crossings (
composition/proper-crossing/composition/ambiguous-corridor) — only an edge's relationship to nodes, not to other edges. It's also not auto-layout (see Roadmap's "Not planned" — auto-layout is explicitly declined): it only computes a repair for an already-diagnosed violation on an already-authored edge, the same category as the existinglabelAtsuggestion forlayout/constraint, not a general positioning engine.Scope
suggestClearingVia()torenderers/shared/geometry.mjs— given the two endpoint rects,fromSide/toSide, and the obstacle set, it samples candidate midpoints for the standard 2-point elbow and returns the first one whose three segments are verified clear of every obstacle (reusingsegmentIntersectsRect) and that honors both endpoint-side direction contracts (viarouteHonorsEndpointSides). Wired intocleanFlowProblems(active for every renderer, sinceobstacleswas already required there) and intocleanEndpointSideProblemsvia a new optionalobstaclesparameter, enabled for thelifecyclerenderer's transitions.cleanEndpointSideProblemscall sites still don't passobstacles(same one-line change as lifecycle's — left out of this PR to keep it reviewable and scoped to where I could verify the real-world benefit; happy to extend here or in a follow-up, maintainer's call). No change tocomposition/proper-crossing/ambiguous-corridor. No auto-layout, no general routing engine.renderers/shared/geometry.mjs,renderers/lifecycle/render-lifecycle.mjs, andtest/geometry.test.mjs.Stability impact
cleanFlowProblems's signature is unchanged; behavior only changes (message/evidence content) when a verified via is actually found, otherwise falls back to the exact previous generic hint.cleanEndpointSideProblemsgains one new optional parameter (obstacles, default[]); omitting it keeps today's behavior byte-for-byte, confirmed by a dedicated backward-compatibility test.cleanFlowProblems), so I specifically re-ran thearchitectureandworkflowrenderers' own test suites (see below) rather than only the new tests.suggestClearingViareturnsnull(not a guess) whenever it can't verify a candidate — including a real degenerate case I found and fixed during testing (below) — so a failure mode here is "no suggestion," never a wrong one.Tests run
From
archify/:node --test test/geometry.test.mjs— 60/60 pass (7 new tests:suggestClearingViadirectly against a real obstacle — verified viasegmentIntersectsRect/routeHonorsEndpointSideson the returned points, not a hardcoded expected coordinate; an unsolvable case correctly returningnull; a declined mixed-side-combination case; both diagnostics' new message/evidence output; the backward-compatible no-obstaclesfallback).node --test test/repair-receipt.test.mjs test/workflow-compiler.test.mjs— 97/97 pass. One existing assertion inrepair-receipt.test.mjsneeded a one-line update: this change legitimately improved that test's own real fixture's diagnostic (a verified via is now suggested instead of the old generic text), so the assertion now accepts either form.node --test test/architecture-delta.test.mjs test/sequence-column-fit.test.mjs— 24/24 pass.npm test(full suite): could not complete — hangs indefinitely in my sandboxed environment (near-zero CPU for 1.5+ hours; confirmed via process inspection, not legitimate progress). Reproduces identically on unpatchedmain, so this predates and is unrelated to this change. Every targeted file above runs and completes normally and cleanly.A real bug caught during testing: an early version of
suggestClearingViacould return a degenerate via (two identical points) when source and target anchors shared the same row (horizontal elbow) or column (vertical elbow) — found by running the patch againstrepair-receipt.test.mjs's own real fixture, not by inspection. Fixed with an explicit guard (Math.abs(start[crossIndex] - end[crossIndex]) < 1) return null), commented in place with the derivation.Visual evidence
Not applicable — diagnostic message/evidence content only; no renderer output, SVG, or viewer change.
Generated artifacts
archify.zip: left unchanged.scripts/build-zip.shrequires Node 22 exactly for canonical byte-identical output; my environment runs Node 24.19.0, so rebuilding here would produce non-canonical bytes. Happy to rebuild if useful, or a maintainer can on merge.Checklist
npm testinarchify/. — targeted tests (geometry, repair-receipt, workflow-compiler, architecture-delta, sequence-column-fit) all pass;npm test's full suite hangs in my environment (pre-existing, see above).archify.zipneeds a Node-22 rebuild I can't produce correctly here.