fix(calm-suite): read flow transition labels from the schema description field - #2939
fix(calm-suite): read flow transition labels from the schema description field#2939gjs-opsflo wants to merge 4 commits into
Conversation
…ion field The flow meta-schema defines description on transitions; summary was a legacy CalmStudio field. Schema-only flow documents previously rendered badge tooltips as undefined. Prefer description, keep summary as a fallback for older files. Signed-off-by: Test User <gjs@opsflow.sh>
Transitions sharing a relationship now spread along the edge path ((k+1)/(n+1) of path length) instead of stacking on the midpoint, and destination-to-source transitions render hollow badges (white fill, blue ring) so request/response direction is readable without the animation. Signed-off-by: Test User <gjs@opsflow.sh>
Reverse (destination-to-source) badges shift perpendicular to the edge so request/response pairs stay separated even on edges shorter than a badge diameter. Verified visually on the tutorial inventory flow: all five sequence numbers legible, hollow badges beside their solid pair. Signed-off-by: Test User <gjs@opsflow.sh>
0f6f8f1 to
f48eaac
Compare
markscott-ms
left a comment
markscott-ms
left a comment
There was a problem hiding this comment.
Overview
flowOverlay.ts read transition.summary — a legacy CalmStudio field absent from CalmFlowTransitionSchema — so schema-valid flows rendered data-summary="undefined" and <title>undefined</title>. Preferring transition.description with a cast-based summary fallback is correct: calm-models/src/types/flow-types.ts:6 confirms description: string is the schema field and summary doesn't exist, which is also the source of the two typecheck errors this removes.
The diff also carries badge-placement and styling work: transitions sharing a relationship now spread along the path at (k+1)/(n+1) rather than stacking at the midpoint, reverse transitions shift into a perpendicular "return lane" and render hollow via a new flow-badge-reverse class, and two geometry helpers (pointAtFraction, pathNormalAt) support that. It's coherent with the bug being fixed — the same request/response demo files that exposed the undefined labels are exactly where stacked badges bite. Worth a line in the PR body so the visual change is discoverable from the changelog, but no objection to it riding along.
Correctness
Inline comments cover the specifics. In summary:
- The lane offset applies to every reverse badge, not just crowded ones — a lone
destination-to-sourcetransition's badge now floats 22px off its edge. - The perpendicular direction is arbitrary (always
(-ty, tx)), so which side a badge lands on depends on layout point ordering; nothing prevents it landing over a node. - Badge spread follows document order rather than
sequence-number. pointAtFractionemits raw floats, so output now containscx="140.86000000000001"-style values where layout integers used to be.- Pre-existing but adjacent:
${transition['sequence-number']}is interpolated into text content unescaped. It's typednumber, but these are parsed JSON documents, so a string value would inject markup — cheap to route throughescapeAttrwhile you're in this code.
Code quality
pointAtFractionandpathNormalAtduplicate the segment-length walk verbatim, and the copies diverge on degenerate input (pointAtFractionreturnsfirstwhentotal === 0;pathNormalAtreturnsundefined). One function returning{ point, normal }removes both the duplication and the divergence.- The attribute is still
data-summarywhile the value now comes fromdescription. I found no reader incalm-studio/— only the:global(.flow-badge)cursor rule atCalmDiagram.svelte:234— so either rename todata-descriptionor drop it, since<title>already supplies the tooltip. - Hard-coded
#fffffffor the hollow badge: the component supports:host([theme="dark"])with--calm-bg: #1e1e1e(CalmDiagram.svelte:228), so on dark the "hollow" reading inverts to a white disc. The file hard-codes#3b82f6throughout already, so this is existing debt rather than new — follow-up material. - A short note on the
(transition as { summary?: string })cast saying when the fallback can be retired would help future cleanup.
Test coverage
- The core regression test is well-targeted: schema-only flow, asserts the description text appears and
undefineddoesn't. - The
summaryfallback is untested. Nothing asserts a legacysummary-only transition still produces a label — existing fixtures usesummarybut only assert onanimateMotion,keyPoints, and badge counts, never label text. Backward compatibility is the stated reason for keeping the cast, so it should have a test, plus one assertingdescriptionwins when both are present. - The geometry tests assert only inequality (centres differ, distance >= 18) and would pass under badly wrong placement. Pinning an expected fraction point on a known polyline would lock the behaviour down.
- Minor: the new test counts
/class="flow-badge/g(unterminated) while the existing test near line 226 countsclass="flow-badge"(terminated), which now silently excludes reverse badges — it passes only because that fixture is forward-only. expect(svg).toContain('fill="#ffffff"')couples to a literal colour; assertingflow-badge-reverseis enough.- Fixtures still put
summaryonCalmFlow-typed literals, which remain excess-property type errors. The claimed two-error drop is real for the production file; the test file's errors could be cleared in the same pass.
Verdict
Approve-with-comments. The label fix is correct and the badge work is a reasonable companion. Worth addressing before merge: gate the lane offset to the crowded case, and add a test for the summary fallback. Float precision, helper duplication, and the data-summary naming are fine as follow-ups.
| if (a === undefined || b === undefined || len === 0) return undefined; | ||
| const tx = (b.x - a.x) / len; | ||
| const ty = (b.y - a.y) / len; | ||
| return { x: -ty, y: tx }; |
There was a problem hiding this comment.
The normal is always (-ty, tx), so which side of the edge a reverse badge lands on depends purely on the layout engine's point ordering — nothing stops it landing on top of a node, another edge, or off-canvas. A comment acknowledging that is probably enough for now; biasing away from the diagram centroid would be the fuller fix.
Separately, this function re-walks the segment lengths exactly as pointAtFraction does, and the two copies handle degenerate input differently (pointAtFraction returns first when total === 0, this returns undefined). One function returning { point, normal } would remove the duplication and the divergence.
Badge spread ranks by sequence-number rather than document order; the return-lane offset applies only when a shared edge is actually crowded (along-path spacing under a badge diameter), so a lone reverse badge stays on its line; coordinates round to 2dp; the badge data attribute is renamed data-description to match its content; the two path helpers share one polyline walk with consistent degenerate handling; and the label fallback chain gains legacy-summary and description-wins tests. Signed-off-by: Test User <gjs@opsflow.sh>
|
Thanks @markscott-ms — all six addressed in
Suite 64/64, typecheck unchanged. |
|
@markscott-ms gentle nudge — all six review points were addressed in 65c211d (sequence ordering, crowding gate, rounding, data-description, unified path walk, label tests). Ready for another look whenever you have time. No rush, and thanks again for the detailed first pass. |
rocketstack-matt
left a comment
rocketstack-matt
left a comment
There was a problem hiding this comment.
The six points from the earlier review are addressed in 65c211d (verified against the diff + tests: sequence-number ordering, crowding-gated lane offset, 2dp rounding, data-description rename, shared walkPolyline, and both label-fallback tests). Three smaller items below.
| ` <title>${escapeAttr(transition.summary)}</title>`, | ||
| `<g class="${badgeClass}" data-description="${escapeAttr(transitionLabel)}">`, | ||
| ` <circle cx="${midX}" cy="${midY}" r="10" fill="${circleFill}"${circleExtra}/>`, | ||
| ` <text x="${midX}" y="${midY}" fill="${numberFill}" font-size="9" font-weight="bold" text-anchor="middle" dominant-baseline="central">${transition['sequence-number']}</text>`, |
There was a problem hiding this comment.
transition['sequence-number'] goes into the badge text unescaped, while every other dynamic value here (and in nodeRenderer.ts) is routed through an escape helper. The type says number, but this is parsed JSON reaching {@html svgContent} in CalmDiagram.svelte:166 — a document with a string in that field injects markup. Raised in the original review body as a while-you're-here item; still open.
| ` <text x="${midX}" y="${midY}" fill="${numberFill}" font-size="9" font-weight="bold" text-anchor="middle" dominant-baseline="central">${transition['sequence-number']}</text>`, | |
| ` <text x="${midX}" y="${midY}" fill="${numberFill}" font-size="9" font-weight="bold" text-anchor="middle" dominant-baseline="central">${escapeAttr(String(transition['sequence-number']))}</text>`, |
| return Math.round(n * 100) / 100; | ||
| } | ||
|
|
||
| function escapeAttr(str: string): string { |
There was a problem hiding this comment.
Pre-existing, not introduced here: this duplicates nodeRenderer.ts's escapeXml but doesn't escape '. Since this file is already being touched, worth exporting escapeXml from nodeRenderer.ts and importing it here instead of maintaining two slightly different escaping helpers in the same package. Non-blocking.
| if (seg.len === 0) return undefined; | ||
| const tx = (seg.b.x - seg.a.x) / seg.len; | ||
| const ty = (seg.b.y - seg.a.y) / seg.len; | ||
| return { x: -ty, y: tx }; |
There was a problem hiding this comment.
Following up on the duplication point from the earlier review: walkPolyline now shares the segment/length precompute, but pointOnWalk and normalOnWalk still each re-walk the segment list to find the target fraction. A single function returning { point, normal } (as originally suggested) would still collapse that. Non-blocking.
|
@gjs-opsflo nudge |
3 participants
Description
The flow overlay in
@calmstudio/diagramread transition labels from a legacysummaryfield, but the flow meta-schema (calm/release/1.2/meta/flow.json) definesdescriptionon transitions. A schema-correct flow document with nosummaryrendered its sequence-badge tooltips asundefined.Fix: prefer the schema's
description, keepsummaryas a fallback for older files. Found while building the learn-tutorials PoC (#2928) — its demo files carried both fields as a workaround, which this makes unnecessary.Side effect: removes two pre-existing typecheck errors in
flowOverlay.ts(the typed access to the non-schemasummaryfield).Type of Change
Affected Components
calm-studio/) —packages/web-componentTesting
Evidence: new test renders a schema-only flow (no
summary) and asserts the badge carries thedescriptiontext with noundefinedin the output; suite 54/54; typecheck error count drops by two.Checklist