[pull] develop from mermaid-js:develop - #228
Merged
Merged
Conversation
After the Playwright migration every Applitools batch was fragmented and the ~1000 migrated .mmd fixtures all landed under a single mmd-snapshots.spec.ts entry. - The batch id was seeded with Date.now() at module load. Under Cypress the helper loaded once per spec file; under Playwright it loads once per worker, so with fullyParallel a spec's tests were split across as many batches as workers touched it. Seed the id once in playwright.config.ts (the runner process, before workers fork and inherit process.env); CI passes a per-dispatch id (sha + run_id) so re-dispatching on the same commit gets fresh batches. - Batch by the same grouping the Argos sheets use: mmd fixtures by their diagram folder (diagrams/flowchart, diagrams/flowchart/elk, ...), spec-based tests by spec path. - Name Applitools tests within the batch: mmd fixtures by base name, spec tests without the spec-file prefix Playwright prepends to titlePath (restoring the Cypress-era `describe title` names). Explicit names are unchanged. - Drop PLAYWRIGHT_COMMIT from the Argos workflows; it only ever fed the old Applitools seed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`redux-color` / `redux-dark-color` were forked from `redux` / `redux-dark` by
copy-paste and had drifted. Variables the base themes define were either missing
outright or silently re-derived from the grey `primaryColor`:
redux-color primaryBorderColor, clusterBkg, clusterBorder,
altBackground, compositeTitleBackground,
stateEdgeLabelBackground, requirementEdgeLabelBackground
redux-dark-color compositeBackground, altBackground,
compositeTitleBackground, stateEdgeLabelBackground,
requirementEdgeLabelBackground
Nothing crashed when that happened -- diagrams just picked up an untuned value
where the base theme had a deliberate one, which is invisible in review and only
shows up in a screenshot.
Also wire up the chart diagrams that read flat `pieN` / `fillTypeN` /
`sectionBkgColor` variables rather than the `cScale` array, and so had never
picked up the colour themes' palette at all:
- pie slices now come from the theme's categorical scale. Every `pieN` used to
be a tint of one pale lavender; `pie3` resolved to pure white in
`redux-color` and to near-black in `redux-dark-color`.
- gantt section bands use two palette hues. Both section colours were white
(light) or near-black (dark), and gantt paints them at 20% opacity, so there
was no visible banding.
- user-journey gets eight hue-distinct section fills: pale in the light theme,
dark in the dark theme, since journey task labels use the theme's
`textColor`.
Add `theme-redux-color-superset.spec.ts`, which pins that a colour theme defines
everything its base theme defines and diverges only on an explicitly listed
palette, so widening that list is a deliberate edit rather than something a loose
pattern lets through.
`docs:build` ran `rimraf ../../docs` as its *first* step:
rimraf ../../docs && docs:code && docs:spellcheck && docs.cli.mts
so a failure in typedoc or cspell left the whole committed `docs/` tree deleted
and never regenerated. The contributor got ~150 staged deletions with no obvious
cause, and -- because `.lintstagedrc.mjs` wires `docs:build` to any change under
`src/docs/**` -- a pre-commit hook that could not succeed until they worked out
what had happened and restored the directory by hand.
The deletion is still required so that pages whose source was removed do not
linger, so it now runs immediately before the step that regenerates the
directory, after the two steps that can realistically fail:
docs:code && docs:spellcheck && rimraf ../../docs && docs.cli.mts
Verified by appending an unrecognised word to a file under `src/docs/` and
running `docs:build`: it exits 1 at the spellcheck step with all 155 files in
`docs/` intact. Before this change the same failure removed every one of them.
`docs:verify` already used this order and is unchanged. `docs:pre:vitepress` has
the same shape but targets `src/vitepress`, which is gitignored scratch, so a
failure there costs nothing and it is left alone.
`intersectLine` comes from Graphics Gems, where the coordinates were INTEGERS and `denom / 2` was added to the numerator so the integer division rounded instead of truncating. JavaScript division does neither, so that term was not a rounding correction at all: `(num + denom / 2) / denom` is `num / denom + 0.5` — a constant half-unit displacement of every intersection, on both axes. `intersectPolygon` is how every non-rectangular shape finds its edge attachment — diamond, stadium, hexagon, trapezoid, subroutine. Half a pixel is invisible alone, but it moved the point off the axis the query ray travelled, which is enough to give an otherwise orthogonal edge a tiny diagonal opening segment and to push an attachment just inside the node it was meant to touch. `question.ts` had been compensating by subtracting 0.5 back off for diamonds; that compensation goes with it, or the attachment moves half a pixel the other way.
The ELK options that matter most come in combinations that only make sense together, and "which layering strategy goes with which node placement" is not something a diagram author should have to know. `elk.preset` names three: `default`, `legacy` (what shipped before this branch) and `depthFirst`. An explicit `elk.layeringStrategy`, `nodePlacementStrategy` or `cycleBreakingStrategy` beats the preset for that one option, so a preset is a starting point rather than a lock. The individual keys default to `undefined` rather than to a value, which is what makes `config.elk?.X ?? preset.Y` fall through to the preset. Note that `defaultConfig.ts` is hand-written: a `default:` in the schema alone never reaches `config.elk`.
Where two edges cross, the one that gives way is drawn with a small arc (or a gap) so it is clear which line passes over which. On by default; `elk.lineHops: false` for plain crossings, `'gap'` for gaps. Detection and both styles already existed for swimlanes. What is new is the `afterPaint` hook that lets ELK use them, and `applyLineJumpsToSvg` being exported so a layout package outside `mermaid` can reach it. Two defects in the hop geometry are fixed here, both found on real diagrams and both of which rendered as something that looked broken rather than merely untidy: - A hop with no room next to a bend was fitted into whatever was left, as little as 2.9px against a requested 6, opening exactly on the corner's tangent point. At that radius the arc does not clear the stroke it is hopping, so the lines still touch. Hops now keep a straight run clear of the bend, and one that would still shrink below 60% of the requested radius is dropped — an ordinary crossing is a much better failure than a broken-looking hop. - A crossing found inside the stretch where either edge is rounding a bend is now ignored. Crossings are computed on polylines, but a rounded edge is not drawn as its polyline: it leaves the line up to 7.07px before each bend and rejoins it that far after. A crossing found in there is somewhere the stroke never goes, so the arc arched over blank paper while the two lines carried on touching beside it.
…rminal jogs Two edge-endpoint fixes, both in `geometry.ts`. ELK models every node as a rectangle, so for a diamond, stadium or hexagon the port it chooses is on the bounding box rather than on the shape. The adapter used to resolve that by attaching along the ray from the node CENTRE, which lands on the outline at a different offset than the port and so opened every such edge with a diagonal segment. `outlineAttachPoint` now bisects along the edge's own departure axis using only `node.intersect`, so the edge leaves perpendicular and meets the shape where it was heading. `straightenTerminalJogs` removes the tiny step ELK leaves between a port and the channel an edge runs in — two rounded corners stacked on each other, often under a pixel apart, which reads as a kink under the arrowhead. It is removed by moving the channel onto the port's row, NOT the port onto the channel: sliding an attachment along a node's border leaves it somewhere the layout did not choose, and a node whose other edges are still evenly spread then looks lopsided. That constraint makes the pass conservative, and deliberately so. The WHOLE run has to move or the moved and unmoved halves meet at a diagonal, so an edge whose run ends at the far terminal is skipped rather than drag the other end's port. Straightening also runs as a post-pass over the finished layout and counts each candidate against every other edge, dropping any that would add a crossing.
A subgraph could carry 74px of padding on one side and 24px everywhere else, which reads as a mistake because nothing visible occupies it. The space is a routing lane, held open for an edge that runs against the flow of the layout and has to be routed back around the outside — so only groups containing such an edge were affected, which is why it looked arbitrary. The lane's width came from `spacing.baseValue`, which was doing two jobs. Every unset ELK spacing derives from it, so it had to stay large enough that an edge got a straight run before the node it enters — below about 40 the approach came out shorter than the 10px arrowhead and the turn read as happening underneath it. But an edge routed down the inside of a frame claims a lane the same width, so paying for the approach out of the base value also pushed groups clear of their own borders. Split them: base value down to 24, with the approach run, node separation and edge separation set explicitly. Two things worth recording, because both cost time: - The key that buys the approach is `elk.layered.spacing.edgeNodeBetweenLayers`. A previous attempt used `edgeEdgeBetweenLayers`, which is edge-to-edge and a different quantity, and the note left behind concluded that ELK ignored edge-node spacing "in every key form". It does not. - Node separation derives from the base value too, so lowering that alone pulled sibling nodes together until they were touching. `spacing.nodeNode` is now set on its own, spelled the same way as the rectpacking override so a container cannot hold two values for one option, and `clearContainerAlgorithmOptions` restores it alongside the base value. Subgraph nodes are also placed with NETWORK_SIMPLEX and PORT_POSITION flexibility, which keeps a group's nodes aligned with one another rather than drifting, and lets a node shift so an edge leaves straight instead of bending off the port. Measured on a 25-node six-subgraph diagram: the two lopsided groups are back to 24/24, and the shortest approach run over all 28 edges is 30px with none under 15, against 7 under 15 when the base value alone was lowered. Resolves #8150
The subgraph padding this branch set out to even up is still lopsided,
because the key that bought the approach run pays for the routing lane
as well. On a group with an edge routed down the inside of its frame,
`edgeNodeBetweenLayers` is charged once between the nodes and that
edge's lane, and again between the lane and the frame.
Measured across four values on a six-subgraph diagram, the group's extra
width is exactly `36 + 2x`:
x=20 padR 76 approach 20
x=25 padR 86 approach 25
x=30 padR 96 approach 30
x=40 padR 116 approach 30
40 was costing 40px of frame more than 20, and buying nothing over 30 —
the approach run stops improving there.
Nothing separates the two uses. `elk.spacing.edgeNode` at 10 and at 20
leaves both the lane and the frame unchanged, and so does
`elk.spacing.edgeEdge`. Buying the same approach out of
`spacing.baseValue` instead is worse, not better: approach 20 costs 84px
of frame that way against 76px here, and approach 25 costs 99px against
86px. So this key is the cheapest way to buy the approach, and 20 is as
low as it goes while keeping every edge clear of its own arrowhead —
below it, three edges drop under 15px against a 10px arrowhead.
Honest about what is left: this is a balance, not the decoupling the
previous commit claimed. A group with a back-edge still carries more
frame than one without, 76px against 24px here. Removing the rest would
mean not routing that edge inside the frame at all, which is ELK's
decision rather than a spacing one.
The previous commit got the lopsided padding down from 116px to 76px but could not make it even, because the space is a real routing lane: ELK sizes a container around everything it put inside, edges included, and an edge running against the flow of the layout is routed back around the outside. A group holding one grows on whichever side that edge leaves by. No spacing option separates the lane from the approach run — edgeNode and edgeEdge were both measured and neither moves it. So stop trying to reclaim the space and stop drawing the frame around it. The frame is pulled in to SUBGRAPH_PADDING from the group's own children on the left, right and bottom, and the edge keeps its lane just outside — which is what an edge routed around a group should look like anyway. The top is left exactly as ELK set it. It carries the subgraph's title strip, and there is no way from here to tell how much of that padding is the title and how much is spare, so tightening it risks clipping. Placed between applyElkNodePositions and applyElkEdgeLayout on purpose: boundsFor reads the box the first sets and cutter2 clips an edge that ends on a group against it, so the six edges that attach to a frame in the test diagram follow the frame when it moves. Runs deepest-first, so a parent measures against children already pulled in. Measured on the six-subgraph diagram, every group now reads padL=24 padR=24 padB=24, against 24/76/24 before. The title strips stay put at 48 and 82.
Changelog accuracy:
- Three changesets stated different things about node placement, and they
concatenate into one changelog. elk-non-rect-attachment.md described an
intermediate branch state that never ships (NETWORK_SIMPLEX as the root
default, with BRANDES_KOEPF as the way back), including corpus evidence
measured against that state. What actually ships is preset: 'default'
with nodePlacementStrategy: undefined, so the preset supplies
LINEAR_SEGMENTS. The net change from the last release is now stated
once, in elk-layout-presets.md, and the subgraph-level NETWORK_SIMPLEX
is named there as the separate container setting it is.
- Swimlanes call applyLineJumpsToSvg (adjustLayout.ts:25) and have hops
on today, so both lineJump changes move swimlane output for people not
using ELK at all. line-hop-corner-clearance.md now says so up front.
intersect-line-half-pixel.md likewise now leads with the fact that it
reaches every layout, since a dagre flowchart with a decision diamond
is affected as much as an ELK one.
The bias description was wrong, in the comments and the changeset. The
original nudged the numerator AWAY FROM ZERO:
const offset = Math.abs(denom / 2);
x = num < 0 ? (num - offset) / denom : (num + offset) / denom;
so the displacement is 0.5 * sign(num) * sign(denom), not a constant
+0.5. The magnitude is always half a unit but the sign is per axis,
because num is computed separately for x and y while denom is shared.
That also explains the old question.ts compensation properly: subtracting
a flat 0.5 from both axes only cancelled the bias when both signs came
out positive, and doubled the error to a full unit when they did not. A
test now pins it, walking the same crossing through all four sign
combinations; it fails on the old code.
Robustness:
- resolveElkPreset used an indexed lookup, so a preset named __proto__
returned Object.prototype — truthy, so the fallback never fired and
every strategy read off it came back undefined. The schema enum guards
config but not directives or programmatic config. Object.hasOwn now.
- applyLineJumpsToSvg built a selector per edge from an author-controlled
id. CSS.escape is not guaranteed outside a browser and the raw-id
fallback turns a trailing backslash into a SyntaxError that aborts the
render. Paths are indexed by reading data-id instead, so no selector is
built.
- lineHops.ts had two casts that asserted rather than described: a
`never` on the paint groups and a trailing `as EdgeGeom[]`. Replaced
with a named interface deriving the selection type from
applyLineJumpsToSvg's own signature, and a type predicate in the filter
so the map needs no assertion. JUMP_RADIUS now records why the radius
is fixed while the style is configurable.
- OUTLINE_RAY_STEPS 40 -> 20. Each step costs a node.intersect() call and
runs for both endpoints of every edge. 20 steps take a 200px bracket to
~2e-4px, four orders of magnitude below anything renderable. Geometry
over the test diagram is unchanged.
Review caught that evenGroupFrames measured a parent purely from its children's boxes, and it was right to be suspicious. Rendering a nested subgraph with a back edge shows the failure: P frame x=110..468 L_b_d_0 escapes at (474,158) (474,75) `b` is inside C and `d` is a sibling of C, so both endpoints are inside P. ELK routes that edge around C, and the lane is genuinely part of P's interior — but P was measured from C's tightened box plus padding, so the frame was pulled in past it and the edge ran outside a group it never leaves. The distinction is which endpoints an edge has, not where its lane sits. An edge leaving a group has a lane that belongs to the layout around it, and the frame should not be drawn round that. An edge with both endpoints inside never leaves, so its lane is interior and the frame keeps it. Nested groups get both readings of the same lane: outside C, inside P. Looking for this turned up a second, quieter bug. `calcOffset` resolves an edge's section against the origin of the container that owns it, and evenGroupFrames was overwriting that origin — so moving a frame sideways would have dragged every edge routed inside it. It never showed because the frames in the test diagram only shrank on the right, leaving posX untouched. ELK's origin is now kept as `elkOrigin` and calcOffset reads that, never the moved frame. Frames are also clamped so this pass can only ever pull one IN. ELK sized the container around everything it put there, so a measurement here wanting more room means this code got something wrong, not that ELK left something out. Nested case now reads P padR=54 — 24 of padding plus the 30px lane it actually contains — with C still at 24/24 and no internal edge outside its frame. The flat six-subgraph diagram is unchanged at 24/24/24.
Running the DDLT corpus against this branch — which I should have done
before marking the PR ready, not after — turned up a regression the
browser check could not see. At edgeNodeBetweenLayers: 20,
right-angles-not-curves starts tripping
edge-parallel-segment-too-close and the corpus goes to two invalid
fixtures instead of one.
The cause is that this spacing does a third job I had not accounted for:
as well as the approach run and the routing lane, it separates edges
running alongside each other in the layer gap. Over the corpus it is not
monotonic, so "as low as it goes" was the wrong way to pick it:
x=20 invalid 2/14 aggregate 9991.8
x=25 invalid 3/14 aggregate 9990.7
x=30 invalid 1/14 aggregate 9989.6
x=40 invalid 1/14 aggregate 9987.7
30 is the lowest value that leaves only the deliberate merge-edge
counterexample invalid.
Going back up costs nothing that matters now. The reason to be low was
the lane this value also pays for, which used to show as lopsided
padding — but evenGroupFrames pulls the frame in past that lane
regardless, so the only remaining cost is overall diagram size. The
approach run is unchanged either way: 40 measured the same 30px shortest
approach as 30 does, so it stops buying anything there.
Six-subgraph diagram still reads 24/24/24 on every group with a 30px
shortest approach, and the nested case still keeps its interior lane
inside the outer frame.
All five findings were real. Verified each against the code first; the
corpus is unchanged at 9989.6 with one invalid fixture (the deliberate
merge-edge counterexample) after all of them.
Major — two aliases for one ELK option. buildSubgraphLayoutOptions set
both `nodePlacement.strategy` (honouring the preset and an explicit
`nodePlacementStrategy`) and `elk.layered.nodePlacement.strategy`
(hardcoded NETWORK_SIMPLEX). ELK reads those as the same option, so the
container held two values for it with no say in which won, and an
explicit `elk.nodePlacementStrategy` was quietly ignored for subgraph
contents. This is the duplicate-key trap elkOptionCatalogue.ts already
documents, and I walked into it.
Now one fully-qualified key, and the container's default comes from the
preset rather than a literal: presets gained `containerPlacement`, so
`legacy` puts subgraph contents back on BRANDES_KOEPF too. Without that
`legacy` would have been a half-restore that still laid groups out the
new way. The root key is qualified as well — ELK's own docs warn that an
unqualified suffix can collide.
Minor — the label-width floor bypassed the "only ever pull a frame IN"
clamp, and the test I wrote encoded that as intended. A title wider than
the frame ELK sized means ELK did not reserve for its own title;
widening the frame here papers over that while breaking the one
guarantee the pass makes. Clamped, and the test now says why.
Minor — MIN_USEFUL_RADIUS_RATIO was applied before the adjacency pass
but not after, so a hop shrunk to keep clear of its neighbour could
still be drawn below the bar. At the shipped radius of 6, two crossings
4px apart would each get 2px, which does not clear the stroke being
hopped — the same defect the rule exists to prevent. Checked after the
clamp as well. The existing adjacency test moved to a 1.6 gap so it
still covers the non-inversion case it was written for, with a new test
for the too-close case.
Minor — outlineAttachPoint's comment claimed a diagonal departure was
declined, but the code forced it onto the dominant axis, which attaches
at a point the edge does not pass through and reintroduces the offset
the function exists to remove. It declines now, as documented, with
tests for 45 degrees and both dominant-axis diagonals, plus one proving
floating-point dust still counts as axis-aligned.
Minor — the straightenEdges schema text still described the original
behaviour ("moves the endpoint onto the channel row"). The rewrite
moves the channel onto the port's row precisely so ports do NOT move.
Corrected at the source and regenerated, which fixes config.type.ts and
the docs with it.
MIN_JUMP_RADIUS is gone, its degeneracy guard now covered by the useful
radius bar.
`evenGroupFrames` refuses to widen a frame past what ELK sized it to: a title wider than its own frame is a frame ELK did not reserve room for, and widening it here would paper over that. The clamp did its job on `group.width` — and then the layout node took `Math.max(width, labelData.width)` and handed the wider value straight back. That is the width the frame is painted at: `clusters.js` sizes the rect from `node.width`. So a 100-wide ELK frame under a 200-wide title painted 200 wide, spilling 100 units outside the bounds ELK reserved. The max only ever differed from `width` in exactly the case the clamp had just refused — everywhere else `width` has already honoured the label floor — so it existed solely to undo the clamp two lines above it. Every existing `evenGroupFrames` test passed an empty `nodeById`, which left the branch that writes the painted node uncovered. Both sides of it are now tested: the clamped case (the regression, which fails with 200 against the expected 100 before this change) and the case where ELK left room for the title, so the fix cannot pass by simply always shrinking.
`pathById` was exactly `pathByDataId` filtered to the ids present in `edges`, and the only lookup against it ran over `renderedEdges` — which is built from that same filter, so every id in it is already a hit in `pathByDataId`. The second map could never answer differently. Two near-identically named maps in one function is how a wrong-map bug gets written later, so the lookup now reads `pathByDataId` directly. The `if (!pathEl)` guard stays: `Map.get` is still `Element | undefined` to the type checker. Raised as an optional nit in review.
Adds the missing visual coverage and folds away the drift-shaped leftovers the review flagged. e2e coverage for the diagrams this change exists to fix. The existing redux/neo theme specs cover er, gitGraph, mindmap, requirement, sequence and timeline -- pie, gantt, user-journey and state were rendered under no redux colour theme anywhere, so the largest visual deltas had no safety net. `redux-color-chart- themes.spec.ts` renders all four across the four redux themes (16 snapshots), under the default `classic` look since none is neo-specific. Fixture sizes are deliberate: 12 pie slices to exercise the whole scale, 8 journey sections for all of fillType0..7, and 4 gantt sections because `.section1`/`.section3` share `altSectionBkgColor` and 3 would hide that it is half of every gantt's banding. Gantt bands now come off the categorical scale rather than duplicating two hex literals. Verified nothing between the gantt block and the `cScale` assignments reads `sectionBkgColor`, so the assignments move below the scale and use `cScale0` / `cScale1`; a future palette change now carries them along. `altSectionBkgColor` pulled into scope. Both base themes set it to 'white', which is correct on a white canvas -- at gantt's 20% band opacity it composites to nothing, so every other band reads as absent. On the #333 canvas the same literal composites to rgb(92,92,92), brighter than either tuned hue, so half of every gantt's banding fought the other half. Both colour themes now use the canvas colour, giving the intended absent band in either mode. Confirmed by compositing all four themes' bands and by rendering. Five dead assignments folded. The unconditional writes sat above pre-existing `|| ` fallbacks for the same variables, leaving two contradictory statements eight lines apart where the second looked live and was not. Folded into the existing `this.x = this.x || '#…'` lines. Behaviour is unchanged either way, since `calculate()` re-applies overrides after `updateColors()`. `bkgColorArray` asymmetry is now asserted rather than invisible. It is not populated for `redux-dark-color`: that array gates *fills* in `er/styles.ts`, `requirement/styles.js` and `sequence/svgDraw.js`, and the dark theme deliberately colours only borders there, so filling it would repaint ER entities, requirement boxes and sequence actors -- three diagrams outside this change. The spec now checks the length per theme and records why they differ, so the one asymmetry between the themes no longer passes the test written to catch drift between them. The spec header no longer claims both themes add the array. Reconciled the contradictory journey-label comments: labels resolve to the theme's `textColor`, and the hardcoded `.label text { fill: #333 }` rule in `user-journey/styles.js` does not win. The dark file already said this; the light one did not. Changeset shortened.
… depth-first Two changes to how ELK diagrams settle. `elk.spacing.portsSurrounding` was left at ELK's default of 0, which permits a port to sit exactly on a node's corner. A corner is the one boundary point with no side to leave from, so the edge left the vertex and then ran ALONG the frame's own edge before turning away. Subgraphs showed it first: an edge that crosses a subgraph boundary attaches to the frame rather than to a node inside it, and a frame is large enough for the result to be obvious. Reserving a margin at the ends of each side lets ELK keep ports off the corners itself, rather than the renderer correcting them afterwards — an endpoint fix-up was tried first and made it worse, moving the terminal to mid-side while leaving ELK's corner bend in place, so the edge hugged the border to get back to it. Over the elk-edge-cases corpus this takes fixtures with a corner endpoint from 8 of 30 to 3. The remaining three also occur under `legacy`, so they have another cause. 12 is chosen by measurement, not taste: the smallest value that clears the corner on that corpus. Not a free parameter — 30 reorders layers. `preset` also changes. Depth-first cycle breaking becomes `default`, since it gives shorter back edges on graphs that loop, which is most flowcharts that loop at all. The greedy-model-order triple that `default` named before is still reachable as `modelOrder`, and `depthFirst` stays as a name for what `default` now is, so a diagram can say depth-first rather than depend on the default staying put.
…palettes Both stylesheets generate one rule per palette slot, looping to THEME_COLOR_LIMIT and indexing the palette by the loop counter. Two things go wrong with that, and neither raises anything -- the browser discards the invalid declaration, so the only symptom is a shape rendering unstyled. Indexing raw means a palette with fewer entries than THEME_COLOR_LIMIT emits `stroke: undefined` for the overflow slots. Both now wrap at the palette length. `requirement` also emitted `fill: ;` -- a property with no value -- whenever there was no background palette. That is not hypothetical: `redux-dark-color` ships a border palette and an empty background palette so that it colours outlines only, which means every requirement diagram under that theme carries twelve invalid declarations today. The declaration is now omitted instead. `paletteCssGeneration.spec.ts` asserts the shape of the generated CSS rather than the colours, so it holds for any palette: no `undefined` values, no empty declarations, every slot resolving to a real colour when the palette is shorter than the limit, and no `fill` at all when there is no fill palette. Confirmed it fails four ways against the current code and passes with the fix. Split out of the redux-color default-theme stack: these are independent of that change and can land on their own.
Review feedback: still too verbose at three paragraphs. Cut to one, matching the 6-14 line norm of the existing changesets on develop. The detail lives in the commit messages and the PR description.
Review found that the wrap this PR introduces has its own hole. `i %
borderColorArray.length` is `i % 0` for an empty palette, which is NaN, and
`[][NaN]` is undefined -- so ER would emit `stroke: undefined` across all twelve
slots, the exact symptom the wrap was added to remove. `requirement/styles.js`
already bailed on an empty border palette; ER gated on the theme name only and
now checks both. Reachable through a `themeVariables` override.
The spec missed it because the short-palette case supplies two entries and the
empty case only emptied `bkgColorArray`. It now covers an empty border palette
explicitly, asserting that no palette rules are emitted at all -- the correct
outcome when there is no palette to render. Confirmed it fails when the new
guard alone is removed.
`toContain('stroke:')` was close to vacuous: it ran against the whole stylesheet,
where `.entityBox` and `.reqBox` already carry `stroke:`, so it passed even if
genColor returned nothing. Now scoped to the palette rule bodies, and confirmed
it fails when genColor is short-circuited to ''.
Moved the spec from `diagrams/common/` up to `diagrams/`, a sibling of the
diagram folders. It imports two diagram stylesheets, and `common/` is imported by
every diagram type, so a cross-diagram spec in there implies a dependency that
does not exist.
Dropped the casts in the helper: `theme` is typed as `MermaidConfig['theme']`
rather than asserted to one member of the union, and the options argument uses a
declared `PaletteOptions` shape instead of `as never`, which had been disabling
checking for the whole call.
Pulled forward from #8148 on request, so the fix lands on develop rather than waiting on the default-theme stack. Timeline was the last stylesheet still indexing `borderColorArray[i]` raw in a loop to THEME_COLOR_LIMIT, with no wrap and no palette guard. Written without the shared colour-theme gate, which does not exist on develop: the gate becomes `theme?.includes('color') && options.borderColorArray?.length > 0` and the index wraps at the palette length. #8148 supersedes this with `isColorTheme()` from `diagrams/common/colorThemeGate.js`, so expect a conflict in this function when that stack rebases -- resolution is to take #8148's version. The spec splits into two passes. The invalid-CSS assertions -- no `undefined`, no empty declarations, both for a short palette and an empty one -- apply to all three stylesheets. The slot-shaped assertions stay scoped to ER and requirement, since timeline colours `.section-N` classes directly rather than emitting `[data-color-id]` rules. Confirmed both timeline assertions fail against develop's version and pass with the fix. Verified: unit suite green at 5770, and the timeline / ER / requirement e2e specs green at 395.
All eleven actor drawers read `bkgColorArray[actorCount % borderColorArray.length]` -- one palette indexed by the other's length. The expression was copy-pasted across every actor type, stroke and fill, 22 sites in all. It is currently harmless, which is why it survived: both shipped palettes have twelve entries, so the wrong length happens to give the right answer. It breaks as soon as they differ -- a background palette shorter than the border palette leaves the overflow actors resolving to `undefined`, and because `selection.style(name, undefined)` takes d3's *remove* path (verified in d3-selection 3.0.0), the inline fill silently disappears for some actors and not others. That remove path is also load-bearing, so the fix keeps it. `redux-dark-color` ships a border palette and an *empty* background palette precisely so actors are outlined but not filled; a helper that substituted a fallback colour there would change how every sequence diagram renders under that theme. `paletteColor` returns `undefined` for an absent or empty palette for exactly that reason. Verified as a no-op for the shipped themes rather than assumed: the spec replays the old expression verbatim for 24 indices against both colour themes and asserts the new helper agrees at every one. That is what makes `patch` honest. The last two assertions read the module's own source. The helper tests cannot see a call site that goes back to the old expression, and that is the failure mode with history here -- it was copied eleven times before anyone noticed, so a new actor type copied from an existing one is the obvious way for it to return. Confirmed both fail when a single call site is reverted.
fix(elk): better default layout config
…fail The slot test bounded the stroke count with `toBeGreaterThan(2)`, which still passes when only two of the twelve slots are emitted -- the opposite of what the comment above it claimed. Count the palette blocks and strokes exactly instead, derived from THEME_COLOR_LIMIT rather than a literal. The timeline short-palette test only asserted the absence of `undefined`. But `slot ?? options.nodeBorder` means dropping the `% length` wrap sends the overflow slots to the classic fallback colour rather than leaving them undefined, so that assertion passed with or without the fix. Assert the wrapped sequence, which is what actually regresses: sections 3..12 silently losing their palette colours. Both new assertions were checked against a reverted fix and fail as intended.
fix(e2e): batch Applitools checks per diagram folder with a run-wide id
Review found the guard blind to the mistake it exists to catch. It collected only the palette argument, so a swapped pair -- `stroke` fed from `bkgColorArray` and `fill` from `borderColorArray` -- produced the same set and passed. That is precisely what a copy-paste between two adjacent `.style()` lines produces. Reproduced the reviewer's finding before fixing it: swapping the pair at all eleven drawers left every one of the 167 sequence unit tests passing, the 9 new ones included. The guard now captures the property alongside the palette and pins both pairings, and with it in place that same swap fails -- and is the only failure across the suite, so the gap is closed rather than merely narrowed. `*?raw` is now declared in `src/type.d.ts` instead of suppressed per-file with `@ts-expect-error`. Confirmed `build:types` is clean with the suppression gone, so the declaration is doing the work rather than hiding a real error. The `22` in the count assertion is written as `2 * 11` -- two properties for each of the eleven drawers -- since `expected 21 to be greater than or equal to 22` gave no hint where the number came from. Documented why `drawActivation` keeps `?? mainBkg` when the actor drawers do not: an activation rect spans the lifeline it sits on, so it needs an opaque fill or the line shows through. That is a question of opacity rather than of palette, which is why it belongs at that call site and not in the helper. Changeset no longer contradicts itself -- it claimed both shipped palettes have twelve entries two paragraphs before noting that `redux-dark-color`'s background palette is empty. Scoped to `redux-color`.
…neration fix(er, requirement, timeline): stop emitting invalid CSS for the colour-theme palettes
The two explanatory paragraphs restated the commit message and the PR description. The published changelog only needs the one-line summary.
…ect the khroma note Review found three of the four chart-palette assertions still passing against develop, which makes them decoration rather than tests. Two are rewritten to measure the property that was actually broken; the spec now fails 10 of 14 against the merge base, up from 6. Gantt bands: `not.toBe` was satisfied by two strings that differ while compositing to nothing. gantt/styles.js paints bands at 20% opacity, so the assertion now composites each band over the canvas and requires a max-channel separation of 16. Recomputed both sides: develop separates by 4 in the light theme and 0 in the dark; the tuned bands separate by 35 in both. Journey fills: `new Set(fills).size === 8` passed against develop, because the old tints *were* eight distinct values -- four hues repeated in pairs. Measuring hue instead discriminates 4 against 8. Worth recording that the obvious metric does not: minimum pairwise channel separation is 7.5 on develop against 5 here, because Tailwind-50 shades are deliberately close in value and differ in hue. The improvement is hue diversity, so that is what the test measures. The contrast-sign assertion is left as-is. It was never broken, but it is a real invariant and the thing that stops someone folding the dark journey fills back onto `cScale`. On the khroma directive: the review is right that the comment was false, and wrong that no directive is needed. khroma does declare these members (`dist/methods/index.d.ts`), but `dist/index.d.ts` re-exports them through a bare `export * from './methods'`, and under this repo's `module: nodenext` that resolves as ESM, where a specifier with no file extension does not resolve -- so TS sees the module as having no exports and every member errors. Verified by probe: removing the directive gives three TS2305 errors from `pnpm build:types`, and a namespace import fails the same way. An isolated `tsc --moduleResolution bundler` does compile it clean, which is what makes it look unnecessary. So the directive stays, with a comment that describes the real cause, and as `@ts-expect-error` rather than `@ts-ignore` -- confirmed it reports TS2578 the moment it stops being needed, so it cannot go stale the way the old one did. Also: the light theme's altSectionBkgColor goes back to the base's 'white' verbatim. Restating it as `this.background` was the same colour with a different string, which forced the variable into the drift net's exemption list for a pair that does not diverge. Only the dark theme changes it now, and the exemption says so. And the state fixture comment no longer claims to exercise `stateEdgeLabelBackground`. Nothing reads it -- `state/styles.js` uses the unprefixed `edgeLabelBackground` -- so that clause could never fail.
…embership Review found the guard passable by a half-converted file: `new Set(pairs)` drops duplicates and the total was a `>=`, so 21 `stroke<-borderColorArray` entries alongside a single `fill<-bkgColorArray` satisfied both assertions. Confirmed by reverting ten of the eleven fills to raw indexing -- the previous form accepted it. Now counts each side and asserts they stay balanced, since every drawer paints both. Reproduced the same half-conversion afterwards: it fails with `expected [...] to have a length of 1 but got 11`. Kept as a floor of eleven rather than the suggested exact count. An exact 22 would fail on a correctly-added twelfth actor drawer, and a test that cries wolf on a valid change teaches people to bump the number without reading it -- which is the reflex that let this bug reach eleven call sites in the first place. Balance catches the asymmetric case the exact count was aimed at, and the floor still catches drawers being dropped or left unconverted.
…ctive-order fix(docs): stop docs:build deleting docs/ when a later step fails
…xing fix(sequence): index each actor colour palette by its own length
…erset fix(themes): make the redux colour themes supersets of their base themes
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )