mirror of
https://github.com/DayuanJiang/next-ai-draw-io.git
synced 2026-09-02 01:20:23 +08:00
526f1e14e7
Four reviewers went over the previous commit (three Claude, one Codex). Their
findings, verified independently before applying:
A REAL BUG. A vertical pool with milestone labels drew the label strip outside
the pool frame. The measure pass reserves width as padding + content + strip with
no gap between the last two; the renderer placed the strip one gap further out.
No test caught it because every vertical case omitted phases and every phases
case was horizontal — both regression cases added.
Duplicated logic, now single-sourced:
- messageCount existed byte-identically in layout.ts and render.ts. Two copies
that had to agree or the lifelines stop reaching the last message.
- sequenceMetrics was called twice per sequence container, once inside the
chrome builder and again for the message positions. Same drift hazard, in the
file whose own comment warns about it.
Dead code, each verified unreachable rather than assumed:
- Placed.extent: declared and documented, never written or read. Every .extent
access belongs to RadialTree.
- SequenceMetrics.top: computed, returned, no reader.
- spread()'s level parameter: threaded through the recursion, never used.
- radialReach's .slice(0, generations): widestPerLevel writes one entry per
generation, so its length IS the depth. Confirmed over 20,000 random trees;
removing it made RadialTree.depth dead too.
- Two of three cycle guards in radialHierarchy: self-links are already skipped
when the parent map is built, and that map holds one parent per node, so the
structure is a forest and the visited-set filter cannot fire. The rootOf
guard does fire and stays.
- GraphOptions.layerGap/nodeGap/idPrefix: no caller, not in the tool schema.
Simplifications:
- LayoutContext wrapped a single field; the link array now passes directly,
which also removes the NO_CONTEXT default no call site ever took.
- stretches() and the mirror-image check five lines below it expressed one rule
two ways; unified, with the rationale stated once.
- hasStencilFrame/isDirectional: one caller each, and isDirectional's name
contradicted its body, which the guarded branch then re-discriminated anyway.
- poolFrameStyle() took no arguments and had one caller.
- poolCellOf clamped a value already clamped at the model boundary and
unreachable-by-construction from the parser.
- A comment on stampPoolDecoration described container behaviour the function
does not implement.
Kept deliberately, with evidence:
- The best-arrangement tracking in the crossing reducer. Two reviewers
suspected it was dead weight. Measured: barycentre sweeping regressed below
its own running best in 180 of 500 random graphs, so without it a third of
flowcharts would keep a worse arrangement than one already found.
- Vertical pools. Two reviewers recommended deleting the feature as
undiscoverable. The bug was one line, and vertical swimlanes are a real
convention — documented to the model instead, which is what was actually
missing.
- styleValue duplicating readMarker, isLeaf, findPageIndex: all genuinely
redundant, all predating this branch. Left alone to keep the diff scoped.
525 unit tests and 11 diagram e2e tests pass.
412 lines
13 KiB
TypeScript
412 lines
13 KiB
TypeScript
import { describe, expect, it } from "vitest"
|
|
import { type Operation, restructureDiagram } from "@/lib/diagram-engine"
|
|
import { parseDiagram } from "@/lib/diagram-engine/parse"
|
|
import type { PoolNode } from "@/lib/diagram-engine/types"
|
|
import { findNode } from "@/lib/diagram-engine/types"
|
|
import {
|
|
absoluteRects,
|
|
escapesParent,
|
|
outsidePage,
|
|
overlaps,
|
|
parentOf,
|
|
rectOf,
|
|
} from "./fixtures/geometry"
|
|
|
|
/** An expense-approval swimlane: three roles, five steps, three milestone labels. */
|
|
const EXPENSE: Operation[] = [
|
|
{ op: "set_title", title: "Expense Approval" },
|
|
{
|
|
op: "add_pool",
|
|
id: "p",
|
|
label: "Expense claim",
|
|
lanes: ["Employee", "Manager", "Finance"],
|
|
phases: ["Submit", "Review", "Pay"],
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "fill",
|
|
parent: "p",
|
|
label: "Fill form",
|
|
lane: 0,
|
|
col: 0,
|
|
shape: "terminator",
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "send",
|
|
parent: "p",
|
|
label: "Submit claim",
|
|
lane: 0,
|
|
col: 1,
|
|
},
|
|
{ op: "add_box", id: "rev", parent: "p", label: "Review", lane: 1, col: 2 },
|
|
{
|
|
op: "add_box",
|
|
id: "ok",
|
|
parent: "p",
|
|
label: "Approved?",
|
|
lane: 1,
|
|
col: 3,
|
|
shape: "decision",
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "pay",
|
|
parent: "p",
|
|
label: "Pay out",
|
|
lane: 2,
|
|
col: 4,
|
|
},
|
|
{ op: "link", source: "fill", target: "send" },
|
|
{ op: "link", source: "send", target: "rev" },
|
|
{ op: "link", source: "rev", target: "ok" },
|
|
{ op: "link", source: "ok", target: "pay", label: "yes" },
|
|
]
|
|
|
|
const STEPS = ["fill", "send", "rev", "ok", "pay"]
|
|
|
|
describe("swimlane pool: layout", () => {
|
|
const result = restructureDiagram("", EXPENSE)
|
|
const xml = result.xml as string
|
|
const rects = absoluteRects(xml)
|
|
|
|
it("builds without errors", () => {
|
|
expect(result.errors).toEqual([])
|
|
})
|
|
|
|
it("stacks the lanes in the order they were declared", () => {
|
|
expect(rectOf(rects, "fill").y).toBeLessThan(rectOf(rects, "rev").y)
|
|
expect(rectOf(rects, "rev").y).toBeLessThan(rectOf(rects, "pay").y)
|
|
})
|
|
|
|
it("advances the columns left to right", () => {
|
|
expect(rectOf(rects, "fill").x).toBeLessThan(rectOf(rects, "send").x)
|
|
expect(rectOf(rects, "send").x).toBeLessThan(rectOf(rects, "rev").x)
|
|
expect(rectOf(rects, "rev").x).toBeLessThan(rectOf(rects, "ok").x)
|
|
})
|
|
|
|
it("puts two steps with the same column at the same x", () => {
|
|
const r = restructureDiagram("", [
|
|
{ op: "add_pool", id: "p", label: "", lanes: ["A", "B"] },
|
|
{
|
|
op: "add_box",
|
|
id: "x",
|
|
parent: "p",
|
|
label: "X",
|
|
lane: 0,
|
|
col: 1,
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "y",
|
|
parent: "p",
|
|
label: "Y",
|
|
lane: 1,
|
|
col: 1,
|
|
},
|
|
])
|
|
const rr = absoluteRects(r.xml as string)
|
|
expect(rectOf(rr, "x").x).toBe(rectOf(rr, "y").x)
|
|
expect(rectOf(rr, "x").y).not.toBe(rectOf(rr, "y").y)
|
|
})
|
|
|
|
it("draws one band per lane, with the role names beside them", () => {
|
|
expect([...xml.matchAll(/id="p__band(\d)"/g)].map((m) => m[1])).toEqual(
|
|
["0", "1", "2"],
|
|
)
|
|
expect(
|
|
[...xml.matchAll(/id="p__lane\d" value="([^"]*)"/g)].map(
|
|
(m) => m[1],
|
|
),
|
|
).toEqual(["Employee", "Manager", "Finance"])
|
|
})
|
|
|
|
it("draws the milestone labels", () => {
|
|
expect(
|
|
[...xml.matchAll(/id="p__phase\d" value="([^"]*)"/g)].map(
|
|
(m) => m[1],
|
|
),
|
|
).toEqual(["Submit", "Review", "Pay"])
|
|
})
|
|
|
|
it("omits the milestone band when no phases were given", () => {
|
|
const r = restructureDiagram("", [
|
|
{ op: "add_pool", id: "p", label: "", lanes: ["A"] },
|
|
{
|
|
op: "add_box",
|
|
id: "x",
|
|
parent: "p",
|
|
label: "X",
|
|
lane: 0,
|
|
col: 0,
|
|
},
|
|
])
|
|
expect(r.xml).not.toContain("p__phase")
|
|
})
|
|
|
|
it("parents each step to its lane band, not to the pool", () => {
|
|
// This is what records a role change when the user drags a step to another lane:
|
|
// draw.io rewrites `parent` to the band it was dropped on.
|
|
expect(parentOf(xml, "fill")).toBe("p__band0")
|
|
expect(parentOf(xml, "rev")).toBe("p__band1")
|
|
expect(parentOf(xml, "pay")).toBe("p__band2")
|
|
})
|
|
|
|
it("keeps everything inside the page and inside its parent", () => {
|
|
expect(outsidePage(xml, ["__title"])).toEqual([])
|
|
expect(escapesParent(xml)).toEqual([])
|
|
expect(overlaps(rects, STEPS)).toEqual([])
|
|
})
|
|
|
|
it("clamps a lane index past the last lane instead of drawing off the pool", () => {
|
|
const r = restructureDiagram("", [
|
|
{ op: "add_pool", id: "p", label: "", lanes: ["only"] },
|
|
{
|
|
op: "add_box",
|
|
id: "x",
|
|
parent: "p",
|
|
label: "X",
|
|
lane: 9,
|
|
col: 0,
|
|
},
|
|
])
|
|
expect(r.errors).toEqual([])
|
|
expect(escapesParent(r.xml as string)).toEqual([])
|
|
})
|
|
|
|
it("lays a vertical pool out with the lanes as columns", () => {
|
|
const r = restructureDiagram("", [
|
|
{
|
|
op: "add_pool",
|
|
id: "p",
|
|
label: "",
|
|
lanes: ["A", "B"],
|
|
orientation: "vertical",
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "x",
|
|
parent: "p",
|
|
label: "X",
|
|
lane: 0,
|
|
col: 0,
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "y",
|
|
parent: "p",
|
|
label: "Y",
|
|
lane: 1,
|
|
col: 0,
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "z",
|
|
parent: "p",
|
|
label: "Z",
|
|
lane: 0,
|
|
col: 1,
|
|
},
|
|
])
|
|
expect(r.errors).toEqual([])
|
|
const rr = absoluteRects(r.xml as string)
|
|
// Lanes side by side, the flow running downwards.
|
|
expect(rectOf(rr, "x").x).toBeLessThan(rectOf(rr, "y").x)
|
|
expect(rectOf(rr, "x").y).toBeLessThan(rectOf(rr, "z").y)
|
|
expect(escapesParent(r.xml as string)).toEqual([])
|
|
})
|
|
|
|
it("keeps the milestone strip inside a VERTICAL pool", () => {
|
|
// The measure pass reserves the pool's width as padding + content + strip, with no
|
|
// gap between content and strip. Rendering the strip one gap further out put it
|
|
// outside the frame — and no earlier test caught it, because every vertical case
|
|
// omitted phases and every phases case was horizontal.
|
|
const r = restructureDiagram("", [
|
|
{
|
|
op: "add_pool",
|
|
id: "p",
|
|
label: "V",
|
|
lanes: ["A", "B"],
|
|
phases: ["P1", "P2"],
|
|
orientation: "vertical",
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "x",
|
|
parent: "p",
|
|
label: "X",
|
|
lane: 0,
|
|
col: 0,
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "y",
|
|
parent: "p",
|
|
label: "Y",
|
|
lane: 1,
|
|
col: 0,
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "z",
|
|
parent: "p",
|
|
label: "Z",
|
|
lane: 0,
|
|
col: 1,
|
|
},
|
|
])
|
|
expect(r.errors).toEqual([])
|
|
expect(escapesParent(r.xml as string)).toEqual([])
|
|
expect(outsidePage(r.xml as string, ["__title"])).toEqual([])
|
|
})
|
|
|
|
it("keeps the milestone strip inside a HORIZONTAL pool", () => {
|
|
const r = restructureDiagram("", [
|
|
{
|
|
op: "add_pool",
|
|
id: "p",
|
|
label: "H",
|
|
lanes: ["A", "B"],
|
|
phases: ["P1", "P2", "P3"],
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "x",
|
|
parent: "p",
|
|
label: "X",
|
|
lane: 0,
|
|
col: 0,
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "y",
|
|
parent: "p",
|
|
label: "Y",
|
|
lane: 1,
|
|
col: 1,
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "z",
|
|
parent: "p",
|
|
label: "Z",
|
|
lane: 0,
|
|
col: 2,
|
|
},
|
|
])
|
|
expect(r.errors).toEqual([])
|
|
expect(escapesParent(r.xml as string)).toEqual([])
|
|
expect(outsidePage(r.xml as string, ["__title"])).toEqual([])
|
|
})
|
|
|
|
it("refuses a pool with no lanes", () => {
|
|
const r = restructureDiagram("", [
|
|
{ op: "add_pool", id: "p", label: "x", lanes: [] },
|
|
])
|
|
expect(r.xml).toBeNull()
|
|
expect(r.errors[0]).toContain("at least one lane")
|
|
})
|
|
})
|
|
|
|
describe("swimlane pool: round-trip", () => {
|
|
it("comes back with the same lanes, phases and cells", () => {
|
|
const first = restructureDiagram("", EXPENSE)
|
|
const { tree, warnings } = parseDiagram(first.xml as string)
|
|
expect(warnings).toEqual([])
|
|
const pool = findNode(tree, "p") as PoolNode
|
|
expect(pool.kind).toBe("pool")
|
|
expect(pool.lanes).toEqual(["Employee", "Manager", "Finance"])
|
|
expect(pool.phases).toEqual(["Submit", "Review", "Pay"])
|
|
expect(pool.orientation).toBe("horizontal")
|
|
expect(pool.children.map((c) => c.id)).toEqual(STEPS)
|
|
})
|
|
|
|
it("does not move anything on a re-layout", () => {
|
|
const first = restructureDiagram("", EXPENSE)
|
|
const second = restructureDiagram(first.xml as string, [])
|
|
expect(second.errors).toEqual([])
|
|
expect(second.warnings).toEqual([])
|
|
const a = absoluteRects(first.xml as string)
|
|
const b = absoluteRects(second.xml as string)
|
|
for (const id of STEPS) expect(b.get(id)).toEqual(a.get(id))
|
|
})
|
|
|
|
it("does not accumulate lane bands across round-trips", () => {
|
|
// The bands are chrome the renderer rebuilds. Preserving them verbatim would leave a
|
|
// stale set behind the new ones on every pass.
|
|
const first = restructureDiagram("", EXPENSE)
|
|
const second = restructureDiagram(first.xml as string, [])
|
|
const third = restructureDiagram(second.xml as string, [])
|
|
const count = (xml: string) => (xml.match(/__band\d/g) ?? []).length
|
|
expect(count(third.xml as string)).toBe(count(first.xml as string))
|
|
})
|
|
|
|
it("reads a step's new lane from the band the user dropped it on", () => {
|
|
const first = restructureDiagram("", EXPENSE)
|
|
// Simulate the drag: draw.io rewrites the cell's parent to the new band.
|
|
const moved = (first.xml as string).replace(
|
|
/(<mxCell id="pay"[^>]*parent=")p__band2(")/,
|
|
"$1p__band0$2",
|
|
)
|
|
expect(moved).not.toBe(first.xml)
|
|
const pool = findNode(parseDiagram(moved).tree, "p") as PoolNode
|
|
const pay = pool.children.find((c) => c.id === "pay")
|
|
expect(pay).toBeDefined()
|
|
// The band wins over the stale marker still on the cell.
|
|
expect((pay as { cell?: { lane: number } }).cell?.lane).toBe(0)
|
|
})
|
|
|
|
it("keeps a vertical pool vertical", () => {
|
|
const first = restructureDiagram("", [
|
|
{
|
|
op: "add_pool",
|
|
id: "p",
|
|
label: "V",
|
|
lanes: ["A", "B"],
|
|
orientation: "vertical",
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "x",
|
|
parent: "p",
|
|
label: "X",
|
|
lane: 1,
|
|
col: 0,
|
|
},
|
|
])
|
|
const pool = findNode(
|
|
parseDiagram(first.xml as string).tree,
|
|
"p",
|
|
) as PoolNode
|
|
expect(pool.orientation).toBe("vertical")
|
|
expect(pool.lanes).toEqual(["A", "B"])
|
|
})
|
|
|
|
it("survives a lane name containing a semicolon or an equals sign", () => {
|
|
// Those two characters delimit a draw.io style string, so a naive marker would break
|
|
// the whole cell.
|
|
const first = restructureDiagram("", [
|
|
{
|
|
op: "add_pool",
|
|
id: "p",
|
|
label: "",
|
|
lanes: ["a;b", "c=d", "plain"],
|
|
},
|
|
{
|
|
op: "add_box",
|
|
id: "x",
|
|
parent: "p",
|
|
label: "X",
|
|
lane: 0,
|
|
col: 0,
|
|
},
|
|
])
|
|
expect(first.errors).toEqual([])
|
|
const pool = findNode(
|
|
parseDiagram(first.xml as string).tree,
|
|
"p",
|
|
) as PoolNode
|
|
expect(pool.lanes).toEqual(["a;b", "c=d", "plain"])
|
|
})
|
|
})
|