fix: suppress default anchor fragments in describe#321
Conversation
AI Code ReviewCritical IssuesNone Moderate IssuesNone Minor IssuesNone What Looks Good
RecommendationApprove the PR. The fix is correct, well-tested, and maintains backward compatibility for non-default cases while improving roundtrip fidelity by omitting semantically redundant default values. No changes requested. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor IssuesNone found. What Looks Good
RecommendationApprove. The PR correctly fixes issue #320 by suppressing redundant default anchor fragments in DESCRIBE MICROFLOW output while preserving non-default annotations. The changes are minimal, well-tested, and maintain backward compatibility for cases where non-default anchors exist. All checklist items are satisfied for this bug fix. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
ako
left a comment
ako
left a comment
There was a problem hiding this comment.
Review: fix: suppress default anchor fragments in describe
Summary: Correct fix for noisy describe output. Suppressing @anchor(from: right, to: left) on regular flows and the equivalent default combinations on split branches eliminates cosmetic BSON drift. The approach is sound; a few design points worth discussing.
Hard-coded string literals instead of constants/keywords
emitAnchorAnnotation now does:
if from != "" && from != "right" {
if to != "" && to != "left" {The side names are computed by anchorSideKeyword(AnchorRight) / anchorSideKeyword(AnchorLeft) earlier in the same function. Using the literal strings here creates a hidden coupling — if the keyword mapping ever changed, the suppression would silently stop working. Expressing the defaults as anchorSideKeyword(AnchorRight) and anchorSideKeyword(AnchorLeft) would make the link explicit and survive a keyword rename without a bug.
The same concern applies to the string slices passed to branchAnchorFragmentWithDefaultSides ([]string{"right", "bottom"} etc.).
Secondary suppression logic in branchAnchorFragmentWithDefaultSides
After primary default-side suppression, there's a second pass:
case "false":
if to == "" && from == "top" { from = "" }
if from == "" && (to == "bottom" || to == "right") { to = "" }
case "true":
if from == "" && to == "bottom" { to = "" }These suppress side combinations observed to be "layout-equivalent" in Studio Pro. This logic is hard to reason about without knowing the layout rules, and there's no comment explaining why these combinations are equivalent (e.g., "Studio Pro auto-routes the false branch bottom-right when the destination is directly below"). The test TestEmitSplitAnchor_OmitsSingleSidedLayoutEquivalentBranchAnchors covers the cases but doesn't explain the reasoning. A short comment with the layout rule would help a future maintainer judge whether a new case belongs here.
Also: the secondary suppression only fires when one side was already primary-suppressed, making the logic order-dependent. Worth a comment noting that the input to the switch block is already post-primary-suppression.
Existing test updates
Several tests were updated by swapping default anchor values (e.g. AnchorRight/AnchorLeft → AnchorTop/AnchorTop) so they continue to emit something after the suppression. This is the right mechanical fix. One note: the old tests were implicitly verifying that default anchors were emitted; now they verify that non-default anchors are emitted. The suppression behaviour itself is tested by the new OmitsDefault tests, so coverage is complete — just worth being aware that the semantic intent of the old tests shifted.
containsString helper
A small linear-search helper over a ≤2-element slice. Fine for the use case; could alternatively be expressed inline with `||" at the call site to avoid an extra function. Not a blocker.
Tests
Good coverage: default regular flow omitted, split default branches omitted, builder no-else defaults omitted, single-sided layout-equivalent combinations omitted, non-default destination still emitted. The concurrent race test update is correct.
Verdict
Approve with a recommendation to replace the hard-coded default-side strings with anchorSideKeyword(AnchorRight/Left) expressions, and to add a comment explaining the layout semantics behind the secondary suppression switch. Both are readability improvements, not correctness issues.
Replaces the hard-coded "right"/"left"/"top"/"bottom" string literals in emitAnchorAnnotation, emitSplitAnchorAnnotation, and branchAnchorFragmentWithDefaultSides with anchorSideKeyword() lookups so the suppression logic stays in lockstep with the keyword mapping if it ever changes. Adds an extended doc comment to branchAnchorFragmentWithDefaultSides explaining the two suppression passes and their order dependency. Addresses ako's review on PR mendixlabs#321. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@hjotha can you resolve the merge conflicts in this PR? |
|
@ako yes fixes are on the way. |
Symptom: microflow describe emitted explicit @anchor fragments for flows that used the MDL default connection sides, creating cosmetic drift in round-trip output. Root cause: anchor formatting printed every known from/to side even when the value matched the statement default: regular flows use right-to-left, true branches use right-to-left, and false branches use bottom-to-top. Fix: omit default anchor fragments and skip the annotation entirely when all sides are defaults, while preserving explicit non-default sides. Tests: added regression coverage for omitted regular and split defaults, and adjusted existing split-anchor tests to assert that non-default branch anchors are still emitted.
Adds an MDL script under mdl-examples/bug-tests/ verifying that linear flows produce zero @anchor lines (all defaults) while non-default sides like `to: top` survive a describe → exec → describe cycle. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Symptom: describe/exec/describe could add @anchor metadata to IF statements whose branch flows only used the layout that the microflow builder generated. Root cause: split anchor emission only treated one hard-coded true/false branch layout as default. IF statements without an ELSE use a different generated layout, with the true branch going down and the false branch continuing horizontally, so the describer exposed those generated anchors as authored metadata. Fix: suppress branch fragments when their from/to sides match either supported builder-default split layout, while still emitting non-default destination sides. Tests: added split-anchor regressions for the no-ELSE builder defaults and for a non-default destination anchor; ran make build, make lint-go, and make test.
Symptom: describe/exec/describe still reported anchor-only drift for branch fragments such as false: (from: top), false: (to: bottom), and true: (to: bottom). The executor rebuilt equivalent flows, but the fixed default list made the first describe look more explicit than the second. Root cause: default branch anchors vary with the relative layout of split and branch destinations; suppressing only one fixed side pair left builder-equivalent single-sided fragments visible. Fix: treat those single-sided branch fragments as defaults only when the opposite side is already default, while preserving paired manual anchors such as false: (from: left, to: right). Tests: add split-anchor and IF describer regressions for the layout-equivalent fragments; make build and make test pass.
Symptom: describe could emit a statement-level from: anchor for a loop-body tail flow that exec cannot recreate without invalid Studio Pro sequence flows. Root cause: simple activity anchor emission treated the implicit body-to-loop tail edge as a writable outgoing flow on the body statement. Fix: when activity context is available, ignore outgoing body-to-loop tail flows for simple statement anchors; loop-level iterator/tail rendering remains unchanged. Tests: added a loop-body tail anchor emission regression and ran make test.
Replaces the hard-coded "right"/"left"/"top"/"bottom" string literals in emitAnchorAnnotation, emitSplitAnchorAnnotation, and branchAnchorFragmentWithDefaultSides with anchorSideKeyword() lookups so the suppression logic stays in lockstep with the keyword mapping if it ever changes. Adds an extended doc comment to branchAnchorFragmentWithDefaultSides explaining the two suppression passes and their order dependency. Addresses ako's review on PR mendixlabs#321. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
hjotha
force-pushed
the
submit/describer-suppress-default-anchor-fragments
branch
from
3b84dd5 to
2041812
Compare
|
Review follow-up status on the current head:
Local validation after the update was green: make build
make test
make lint-go |
3 participants
Part of #332.
Fixes #320.
Summary
DESCRIBE MICROFLOWnow omits anchor fragments when all emitted sides match the MDL defaults.Root cause
Anchor formatting printed every known
from/toside even when the side matched the statement default. These explicit defaults were semantically redundant and created cosmetic roundtrip drift.Fix
Suppress default fragments for regular flows and split branches. Non-default sides are still emitted.
This intentionally changes describe output for flows that only used default sides.
Tests
Added regression coverage for omitted defaults and non-default anchor preservation.
Validation
make buildmake testmake lint-gomake test-integrationAgentic Code Testing
Test plan