Conversation
The semantic sigil is drawn inset in a node's top corner while the node label is centred in the same vertical band, so a label wide enough to reach that corner painted over the icon. Validation stayed clean because the text-fit rule measured labels against the whole node width and never subtracted the sigil's footprint. Both halves of the existing text-fit contract now cover the sigil, in the same shape as the brand top rail beside it: - render: reserve the sigil's footprint and re-centre the label into the space next to it, so an ordinary label shifts and shrinks instead of overlapping. The shift applies only when the label would otherwise reach the icon, so short labels keep the node centre and render byte-identically. - validate: report the label that still cannot clear the sigil once it has shrunk to its legible minimum, i.e. when no label position satisfies both "clears the icon" and "stays inside the node". Only the sigil's own footprint is reserved, not twice it. Reserving both sides to keep a strictly centred label leaves too little width for ordinary labels and rejects diagrams that have no repairable defect. Scope: dataflow, lifecycle and sequence. Two renderers are deliberately left alone, both documented in code: - workflow, because the fixed-v1 contract freezes its compiler output byte-for-byte, so the label cannot move, and reporting the overlap instead would reject the packaged incident-response example by ~6px. Fixing it needs a maintainer decision on re-freezing that baseline. - architecture, because it centres its label vertically, so it shares the sigil band only on very short components and a blanket reserve would shrink labels that never collide. The clearance comparison carries a 1px tolerance because widthFactor is an approximation of advance width; without it the rule rejects the CJK labels in test/fixtures/automatic-routing-node-border-clearance.workflow.json by 0.6px. Real defects miss by far more. Refs tt-a1i#199
fd4b111 to
008c93e
Compare
tt-a1i
left a comment
tt-a1i
left a comment
There was a problem hiding this comment.
Thanks for documenting the narrowed scope and the remaining workflow decision. Reviewed exact head008c93eefc0a4e8e01d281f99ffd0b934645753b.
[P2] Keep the new sigil validation compatible with standard/v1 (renderers/dataflow/render-dataflow.mjs:146-147 and corresponding lifecycle/sequence calls). In product-analytics.dataflow.json, a node with width50 and label Ingester validates under standard on fixed main2bfb471, but this head rejects it because the new17px reservation leaves33px for a39px label. The guard runs with no quality-profile condition; omitted profile also regresses. Preserve the documented standard compatibility and exercise standard/showcase side by side. The new failure currently also has only generic layout/constraint, an imprecise subject, empty evidence and no supportedFixes; return actionable structured geometry diagnostics.
[P2] Rebuild the published ZIP after the source is settled. Seven payload files differ from canonical source, so ZIP installs retain the old renderer/fitting behavior. This is the archive gap already disclosed in the PR, not a claim that local geometric tests covered package delivery.
Focused sigil/layout tests101/101 passed. Fixed-main versus head public CLI reproduction confirms the compatibility failure; source/ZIP compared member-by-member. Workflow exclusion is explicitly disclosed and remains a scope/maintainer decision, not a separate hidden defect. No source edits or merge.
|
Warning Review limit reachedNext included review available in 38 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 participants
Refs #199
The user problem
The semantic sigil is drawn inset in a node's top corner and the node label is centred in the same vertical band, so a label wide enough to reach that corner paints over the icon. Validation stays clean, because the text-fit rule measures a label against the whole node width and never subtracts the sigil's footprint.
Still reproducible on
maintoday. Reverting onlyarchify/renderers/and keeping this PR's tests:Lifecycle is the mirrored case: a state without a brand mark carries its sigil in the top-right corner.
One shipped artifact is affected today —
examples/sequence-cache-miss-request.html, wherePostgresstarts at 575.6 against a sigil ending at 576.0. The rest of the current examples are clear, because v2.16.0 widened workflow nodes to 132px; the defect is latent for ordinary user diagrams with longer labels rather than visible across the gallery.What changes
The sigil now participates in the same two-part contract as the brand top rail beside it, mirroring
brandLabelFitWidth/brandTopRailProblem:Geometry has one home:
SEMANTIC_SIGIL_INSET/_SIZE/_FOOTPRINTare exported fromrenderers/shared/utils.mjsbesiderenderSemanticSigil, so the fitting logic measures what is actually drawn. Measured against the shapes themselves, the widest reaches x=14 of the 16-unit box →6 + 14 × 0.6875 ≈ 15.6px, plus non-scaling stroke ≈ 16.4px, so the 17px footprint is the nominal box and slightly conservative rather than inflated.Two deliberate choices, both to avoid rejecting legitimate diagrams:
What deliberately does not change
Workflow. Both halves are blocked there, so it is untouched and the reason is recorded in
workflow-compiler.mjs:fixed-v1contract assertssha256(result.svg) === '4e493db…'intest/workflow-compiler.test.mjs, so the label cannot be re-centred without breaking that hash;incident-response.workflow.json— nodepage, "Page On-call", needs ~65px at the 9px floor with 59px available.Re-freezing that baseline, and whether
incident-responseshould be widened, are your calls. Happy to do both in a follow-up once you say which way you want it.Architecture. It centres its label vertically (
c.y + c.height / 2), so it shares the sigil band only on very short components; a blanket reserve would shrink labels that never collide.Brand-bearing nodes. The existing 48px symmetric reserve already puts the label's left edge at
x + 24, clear of a 17px sigil, so those nodes are unchanged.No schema, no authored geometry, no diagnostic codes removed or renamed.
Compatibility
schema-v1unchanged;fixed-v1byte contract untouched. The new validation is ashowcase-visible layout problem in the same family as the sublabel/tag rules from #61.The clearance comparison carries a 1px tolerance, because
widthFactoris documented as an approximation of advance width. Without it the rule rejects the CJK labels intest/fixtures/automatic-routing-node-border-clearance.workflow.json("校验并注入输入") by 0.6px — finer than the model can justify asserting on, and that fixture exists to test routing. Real defects miss by far more: the new regression misses by ~5px.Testing
node --test test/semantic-sigil-clearance.test.mjs— new, 3 tests, one per affected renderer. Each reads the sigil transform and the label from the same node group in the final SVG and asserts they do not overlap. Labels are chosen per renderer so each case actually collides before the fix rather than passing vacuously.node --test test/layout-rules.test.mjs— 97 → 98, adding the validator case.Negative control, reverting only
archify/renderers/and keeping both tests:Restored: 3/3 and 98/98.
Full suite,
npm testfromarchify/, against cleanmainon the same machine:main(39a2113)Failure-set diff: no new failures. The pre-existing 38 are the symlink-privilege and CRLF-checkout families (see #144) and the Node-22 archive pin; all reproduce on clean
mainhere.Regenerated artifacts
npm run render:examplesfor both the repo and packaged copies. Only one artifact changed content in each location; the other four were byte-identical and are not in this diff.examples/sequence-cache-miss-request.htmlarchify/examples/sequence-cache-miss-request.htmlThe changed line is a single
<text data-node-label>x,602 → 602.4. No geometry, routing, font size, or other element moved.Not verified
archify.zipis stale and I could not rebuild it. The archive packagesrenderers/shared/text-fit.mjsandrenderers/shared/utils.mjs, both changed here.scripts/build-zip.shaccepts only Node 22 and this machine has Node 20.18.1 / 20.19.5, so the archive is left untouched rather than published with the wrong zlib. Please rebuild on merge, or tell me the release identity you want and I will add it.visual-checkrun. Chrome is unavailable here, sotest/desktop-reader-browser.test.mjsandvisual-checkwere skipped, not passed. The clearance assertions are geometric, read from the final SVG. A screenshot review of the one regenerated artifact would be a useful second pair of eyes on how a re-centred label reads.🤖 Assisted by an AI agent; every measurement, negative control, and suite comparison above was run and is reproducible.