From 3916ac502ee22b54404eb649f493dc32a832f26c Mon Sep 17 00:00:00 2001 From: Ivan Cheung Date: Sun, 9 Aug 2026 05:43:30 +0000 Subject: [PATCH 1/3] =?UTF-8?q?feat(flow):=20remove=20dagre=20=E2=80=94=20?= =?UTF-8?q?SGCR=20is=20the=20only=20layout=20engine?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace all dagre calls with SGCR equivalents across the flow layout pipeline: inter-zone packing, swimlane global rank, ungrouped flat layout, and the measured re-pass. SGCR guarantees zero node overlaps (P1) and zero edge-over-node (P2) by construction — invariants dagre never provided. - Add grouped SGCR pipeline: two-level layout (per-zone → zone-packing → orthogonal edge routing) with container boxes for zone rendering - Remove @dagrejs/dagre dependency (~14KB gzip bundle savings) - Remove bestLayout direction search (SGCR is deterministic, no search needed) - Remove analyzeFlowGeometry heuristic lint (replaced by checkInvariants) - Geometry lint now uses sgcrReport for ALL flows (grouped and ungrouped) - engine:"dagre" accepted for backward compat, silently ignored - 703 tests pass, tsc clean --- packages/viewer/package.json | 1 - .../src/client/renderers/flow-layout.ts | 93 +-- packages/viewer/src/client/renderers/flow.tsx | 46 +- .../src/client/renderers/sgcr/layout.ts | 137 ++++ .../viewer/src/client/renderers/sgcr/types.ts | 24 + packages/viewer/src/flow-geometry.ts | 679 +++--------------- packages/viewer/test/flow-geometry.test.ts | 313 ++------ packages/viewer/test/readable-layout.test.ts | 65 +- packages/viewer/test/server.test.ts | 22 +- packages/viewer/test/sgcr.test.ts | 126 ++++ 10 files changed, 494 insertions(+), 1012 deletions(-) diff --git a/packages/viewer/package.json b/packages/viewer/package.json index a5016093..fb513a22 100644 --- a/packages/viewer/package.json +++ b/packages/viewer/package.json @@ -41,7 +41,6 @@ "isomorphic-dompurify": "^3.19.0" }, "devDependencies": { - "@dagrejs/dagre": "^3.0.0", "@ivanmkc/termchart-core": "*", "@mantine/core": "^9.3.0", "@mantine/hooks": "^9.3.0", diff --git a/packages/viewer/src/client/renderers/flow-layout.ts b/packages/viewer/src/client/renderers/flow-layout.ts index d1a4c446..2a1c1ac8 100644 --- a/packages/viewer/src/client/renderers/flow-layout.ts +++ b/packages/viewer/src/client/renderers/flow-layout.ts @@ -1,4 +1,5 @@ -import dagre from "@dagrejs/dagre"; +import { layoutSGCR } from "./sgcr/layout.js"; +import type { SGCRInput, Direction } from "./sgcr/types.js"; export interface FlowNode { id: string; @@ -55,10 +56,7 @@ export interface FlowSpec { * and dagre lays the whole graph out. "manual": honor explicit positions (hand-placed diagrams * like the sequence recipe's lifelines); only nodes missing a position are dagre-placed. */ layout?: "auto" | "manual"; - /** Layout engine. "dagre" (default): the established force-directed-ish layered layout + smoothstep - * edges. "sgcr": the provable-by-construction Slotted Grid + Orthogonal Channel Routing engine — - * node/edge overlap, edge-over-node, label spill and arrowhead stacking are impossible by - * construction (see renderers/sgcr/ + the design spec). Opt-in; ungrouped graphs only. */ + /** Layout engine. SGCR is the only engine. "dagre" is accepted for backward compat but silently ignored. */ engine?: "dagre" | "sgcr"; /** Optional overlay annotations (callouts/badges/notes), placed in the layout's free space by the * SGCR annotation engine — overlap-free (P10), on toggleable layers. Engine:"sgcr" only today. */ @@ -196,58 +194,23 @@ function withEdgeIds(edges: FlowEdge[]): FlowEdge[] { return edges.map((e, i) => (e.id ? e : { ...e, id: `e${i}-${e.source}-${e.target}` })); } -/** - * Run dagre over sized nodes and return each node's top-left position (keyed by id). - * Edges with an endpoint not in `nodes` are ignored (a dangling endpoint would make - * dagre synthesize a phantom node and distort the layout). Pure (no DOM). - */ -/** Dagre tuning knobs, overridable per call. A geometry experiment swept these across the recipe - * corpus and found spacing (nodesep/ranksep/edgesep) and ranker do NOT change edge crossings / - * over-node / near-node — those are topological (rank ordering), not spacing-driven. So the - * defaults below are the established readable values; the override hook stays for experiments and - * for the lint to evaluate alternative layouts. The real levers for crossings are flow DIRECTION - * and graph structure (grouping, fewer long-range edges) — see flow-geometry's recommendations. */ -export interface DagreOpts { - nodesep?: number; - ranksep?: number; - edgesep?: number; - ranker?: "network-simplex" | "tight-tree" | "longest-path"; -} -// Phase 5: tighten defaults. The old 70/110 spacing was tuned before SGCR handled the intra-zone -// layout (Phase 3). Now that zones use SGCR internally, the remaining dagre pass is only the -// inter-zone super-node packing (2-6 zones) + ungrouped fallbacks. Tighter defaults give ~30% -// better space utilization. The bestLayout search can still try even tighter (40/70) variants. -export const DAGRE_DEFAULTS: Required = { - nodesep: 70, - ranksep: 110, - edgesep: 28, - ranker: "network-simplex", -}; - -export function dagreLayout( +function sgcrPositions( nodes: { id: string; width?: number; height?: number }[], edges: { source: string; target: string }[], direction: string = "TB", - opts: DagreOpts = {}, ): Record { - const g = new dagre.graphlib.Graph(); - const { nodesep, ranksep, edgesep, ranker } = { ...DAGRE_DEFAULTS, ...opts }; - // ranksep/edgesep give orthogonal edges room to route between ranks instead of crossing - // over nodes; dagre reserves edge corridors via dummy nodes when there's space. ranker picks - // the rank-assignment algorithm (network-simplex gives the most compact, fewest-crossing ranks). - g.setGraph({ rankdir: direction, nodesep, ranksep, edgesep, ranker, marginx: 8, marginy: 8 }); - g.setDefaultEdgeLabel(() => ({})); + if (nodes.length === 0) return {}; const ids = new Set(nodes.map((n) => n.id)); - for (const n of nodes) g.setNode(n.id, { width: n.width ?? NODE_W, height: n.height ?? NODE_H }); - for (const e of edges) { - if (ids.has(e.source) && ids.has(e.target)) g.setEdge(e.source, e.target); - } - dagre.layout(g); + const input: SGCRInput = { + direction: (direction || "TB") as Direction, + nodes: nodes.map((n) => ({ id: n.id, width: n.width ?? 172, height: n.height ?? 44 })), + edges: edges + .filter((e) => ids.has(e.source) && ids.has(e.target)) + .map((e, i) => ({ id: `e${i}-${e.source}-${e.target}`, source: e.source, target: e.target })), + }; + const lay = layoutSGCR(input); const out: Record = {}; - for (const n of nodes) { - const p = g.node(n.id); - out[n.id] = { x: p.x - (n.width ?? NODE_W) / 2, y: p.y - (n.height ?? NODE_H) / 2 }; - } + for (const n of lay.nodes) out[n.id] = { x: n.x, y: n.y }; return out; } @@ -261,9 +224,6 @@ export function dagreLayout( * the inter-zone packing (the super-node dagre pass) stays unchanged. SGCR's output positions * are already normalized to (0, 0) via its own margin, so we just translate to min = 0. */ -import { layoutSGCR } from "./sgcr/layout.js"; -import type { SGCRInput, Direction } from "./sgcr/types.js"; - function layoutCluster( ms: FlowNode[], edges: FlowEdge[], @@ -336,7 +296,7 @@ function layoutGroupedFlow(spec: FlowSpec, nodes: FlowNode[], edges: FlowEdge[]) const s = metaId(e.source), t = metaId(e.target); if (s !== t && !seen.has(`${s} ${t}`)) { seen.add(`${s} ${t}`); metaEdges.push({ source: s, target: t }); } } - const metaPos = dagreLayout(metaNodes, metaEdges, dir); + const metaPos = sgcrPositions(metaNodes, metaEdges, dir); // 3. Assemble: zone containers first (paint behind), then members (parent-relative), then loose. const out: FlowNode[] = []; @@ -410,7 +370,7 @@ function layoutSwimlane(spec: FlowSpec, nodes: FlowNode[], edges: FlowEdge[]): { // position, so cross-lane edges stay short and roughly perpendicular. Laying out each lane // independently (the old approach) left cross-lane-only nodes at rank 0 of their lane, // producing long backward edges that ran over other nodes. - const full = dagreLayout(nodes.map((n) => ({ id: n.id, ...sizeOf.get(n.id)! })), edges, dir); + const full = sgcrPositions(nodes.map((n) => ({ id: n.id, ...sizeOf.get(n.id)! })), edges, dir); const alongRaw = (id: string) => (horizontal ? full[id]?.x ?? 0 : full[id]?.y ?? 0); let minAlong = Infinity; for (const n of nodes) minAlong = Math.min(minAlong, alongRaw(n.id)); @@ -588,12 +548,6 @@ function layoutTiers(spec: FlowSpec, nodes: FlowNode[], edges: FlowEdge[]): { no return { nodes: [...containers, ...childMembers], edges: tieredEdges }; } -/** - * Position nodes with dagre when any node lacks an explicit position; honor explicit - * positions as-is. Uses each node's declared width/height (or defaults). flow.tsx re-runs - * dagreLayout with MEASURED sizes after first render so long labels don't overlap. When nodes - * declare a `group`, defers to the grouped (zoned / lanes / tiers) layout instead. - */ /** * After a grouped layout produces parent-relative positions, compute the ABSOLUTE * position of each non-group node by walking up the parentId chain. Then feed those @@ -651,31 +605,22 @@ function routeGroupedEdges( } } -export function layoutFlow(spec: FlowSpec, opts: DagreOpts = {}): { nodes: FlowNode[]; edges: FlowEdge[] } { - // Drop any null/primitive entry before dispatching to a layout variant: every path below (grouped - // / swimlane / tiers / dagre) dereferences n.id/n.data/e.source and would otherwise throw. Malformed - // entries are already rejected as a 400 by the validator; this keeps the pure layout crash-proof. +export function layoutFlow(spec: FlowSpec): { nodes: FlowNode[]; edges: FlowEdge[] } { const nodes = (Array.isArray(spec.nodes) ? spec.nodes : []).filter((n): n is FlowNode => !!n && typeof n === "object"); const edges = (Array.isArray(spec.edges) ? spec.edges : []).filter((e): e is FlowEdge => !!e && typeof e === "object"); if (nodes.length && nodes.some((n) => groupOf(n))) { const grouped = spec.lanes ? layoutSwimlane(spec, nodes, edges) : spec.tiers ? layoutTiers(spec, nodes, edges) : layoutGroupedFlow(spec, nodes, edges); - // Route ALL edges (intra-zone + cross-zone) through SGCR's orthogonal channel - // router so edge-over-node is impossible by construction (P2). The node positions - // stay as the grouped layout placed them; only the edge polylines change. return routeGroupedEdges(grouped); } if (nodes.length === 0) return { nodes, edges: withEdgeIds(edges) }; - // Authoritative by default: ignore any spec-provided `node.position` and let dagre own the layout. - // `layout:"manual"` honors explicit positions (and only dagre-places nodes that lack one). const manual = spec.layout === "manual"; if (manual && !nodes.some((n) => !n.position)) return { nodes, edges: withEdgeIds(edges) }; - const pos = dagreLayout( - nodes.map((n) => ({ id: n.id, width: n.width, height: n.height })), + const pos = sgcrPositions( + nodes.map((n) => { const sz = estSize(n); return { id: n.id, width: n.width ?? sz.width, height: n.height ?? sz.height }; }), edges, spec.direction ?? "TB", - opts, ); const positioned = nodes.map((n) => (manual && n.position ? n : { ...n, position: pos[n.id] })); return { nodes: positioned, edges: withEdgeIds(edges) }; diff --git a/packages/viewer/src/client/renderers/flow.tsx b/packages/viewer/src/client/renderers/flow.tsx index 0af49e84..a5914c95 100644 --- a/packages/viewer/src/client/renderers/flow.tsx +++ b/packages/viewer/src/client/renderers/flow.tsx @@ -17,9 +17,8 @@ import { type Edge, } from "@xyflow/react"; import xyflowCss from "@xyflow/react/dist/style.css"; -import { layoutFlow, dagreLayout, estSize, readableFloorZoom, anchoredViewport, type FlowSpec, type LegendItem } from "./flow-layout.js"; +import { layoutFlow, estSize, readableFloorZoom, anchoredViewport, type FlowSpec, type LegendItem } from "./flow-layout.js"; import { normalizeFlowSpec } from "./flow-normalize.js"; -import { bestLayout } from "../../flow-geometry.js"; import { LifelineNode, PointNode, EntityNode, ChangeNode, GroupNode, dirHandles } from "./flow-nodes.js"; import { layoutSGCR } from "./sgcr/layout.js"; import { checkInvariants } from "./sgcr/check.js"; @@ -61,15 +60,9 @@ function Legend({ items }: { items: LegendItem[] }) { } function FlowInner({ spec, handle }: { spec: FlowSpec; handle?: PatchHandle }) { - // Capped, deterministic re-layout search picks the most readable arrangement (direction/compaction) - // before we render — requirement 1 ("lay it out so it reads"). For grouped/manual/pinned specs it - // returns the base layout unchanged. - const chosen = useMemo(() => bestLayout(spec), [spec]); - const dir = chosen.direction; - // Readable-zoom floor: the initial fit never shrinks the smallest content label below MIN_READABLE_PX - // (requirement 2). A graph too big to fit at that zoom overflows and pans instead of zooming out. + const dir = (spec.direction ?? "TB") as "TB" | "LR" | "BT" | "RL"; const floor = useMemo(() => readableFloorZoom(spec), [spec]); - const initial = useMemo(() => layoutFlow({ ...spec, direction: dir }, chosen.opts), [spec, dir, chosen.opts]); + const initial = useMemo(() => layoutFlow({ ...spec, direction: dir }), [spec, dir]); // Layout is authoritative unless the spec opts into hand-placed positions; the measured re-pass // (below) runs for auto-laid-out graphs so long labels never overlap. const autoLayout = spec.layout !== "manual"; @@ -146,28 +139,25 @@ function FlowInner({ spec, handle }: { spec: FlowSpec; handle?: PatchHandle }) { }; }, [handle, setNodes]); - // Re-run dagre with MEASURED node sizes once nodes are measured, so long labels don't - // overlap (the initial pass uses estimated/default sizes). Auto-laid-out graphs only — - // hand-placed diagrams (e.g. sequence) keep their explicit positions. useEffect(() => { - // Grouped graphs are positioned by the compound layout (with estimated sizes) and must NOT - // be re-flattened by the plain dagre re-pass, which would ignore zones. if (!autoLayout || grouped || !inited || relaidOut.current) return; relaidOut.current = true; - const sized = nodes.map((n) => ({ - id: n.id, - width: n.measured?.width ?? 172, - height: n.measured?.height ?? 44, + const measuredSpec: FlowSpec = { + ...spec, + direction: dir, + nodes: nodes.map((n) => ({ + ...(spec.nodes ?? []).find((sn) => sn.id === n.id) ?? { id: n.id }, + width: n.measured?.width ?? 172, + height: n.measured?.height ?? 44, + })), + }; + const re = layoutFlow(measuredSpec); + setNodes((nds) => nds.map((n) => { + const rn = re.nodes.find((r) => r.id === n.id); + return rn?.position ? { ...n, position: rn.position } : n; })); - const pos = dagreLayout( - sized, - edges as unknown as { source: string; target: string }[], - dir, - chosen.opts, - ); - setNodes((nds) => nds.map((n) => (pos[n.id] ? { ...n, position: pos[n.id] } : n))); requestAnimationFrame(anchorFit); - }, [autoLayout, grouped, inited, nodes, edges, anchorFit, setNodes, dir, chosen.opts]); + }, [autoLayout, grouped, inited, nodes, edges, anchorFit, setNodes, dir, spec]); // Re-fit when the container resizes (e.g. iPad rotation, sidebar collapse, maximize). useEffect(() => { @@ -459,7 +449,7 @@ export const mount: Mount = (el, content) => { // `engine:"dagre"` opts out. Grouped graphs still use the dagre render path (FlowInner) // because zone containers + parentId + smoothstep edges are dagre-path infrastructure — // but their INTRA-zone layout is already SGCR (Phase 3, layoutCluster). - const useSgcr = spec.engine !== "dagre" && !grouped; + const useSgcr = !grouped; const { teardown } = mountReact( el, diff --git a/packages/viewer/src/client/renderers/sgcr/layout.ts b/packages/viewer/src/client/renderers/sgcr/layout.ts index 1d239d17..3e0ece7f 100644 --- a/packages/viewer/src/client/renderers/sgcr/layout.ts +++ b/packages/viewer/src/client/renderers/sgcr/layout.ts @@ -25,10 +25,14 @@ import type { Direction, Pt, SGCRInput, SGCRInputNode, SGCRLayout, SGCROptions, PositionedNode, RoutedEdge, + SGCRContainer, } from "./types.js"; type ResolvedOpts = Required> & { viewport?: { width: number; height: number } }; +const GROUP_PAD = 16; +const GROUP_LABEL_H = 24; + const DEFAULTS: ResolvedOpts = { layout: "layered", routing: "orthogonal", @@ -966,6 +970,137 @@ function ringLayout(input: SGCRInput, o: ResolvedOpts): SGCRLayout { // ---- orchestration ------------------------------------------------------------------------------- +function layoutGrouped(input: SGCRInput, o: ResolvedOpts): SGCRLayout { + const dir: Direction = input.direction ?? "TB"; + const horizontal = dir === "LR" || dir === "RL"; + // Extract group definitions from input.groups or discover from node group fields + const groupDefs = new Map(); + if (input.groups) for (const g of input.groups) groupDefs.set(g.id, { label: g.label, color: g.color }); + const groupOrder: string[] = []; + const members = new Map(); + const ungrouped: typeof input.nodes = []; + const nodeGroup = new Map(); + for (const n of input.nodes) { + if (n.group) { + if (!members.has(n.group)) { members.set(n.group, []); groupOrder.push(n.group); if (!groupDefs.has(n.group)) groupDefs.set(n.group, {}); } + members.get(n.group)!.push(n); + nodeGroup.set(n.id, n.group); + } else { + ungrouped.push(n); + } + } + + // For each zone: run layoutSGCR on the zone's members + intra-zone edges + const zoneLayouts = new Map(); + for (const grp of groupOrder) { + const ms = members.get(grp)!; + const mids = new Set(ms.map(m => m.id)); + const intraEdges = input.edges.filter(e => mids.has(e.source) && mids.has(e.target)); + const stripped = ms.map(m => ({ ...m, group: undefined })); + const zoneInput: SGCRInput = { direction: dir, nodes: stripped, edges: intraEdges }; + zoneLayouts.set(grp, layoutSGCR(zoneInput, o)); + } + + // Create super-nodes from zone bounding boxes + const superNodes: SGCRInputNode[] = groupOrder.map(grp => { + const zl = zoneLayouts.get(grp)!; + return { id: `__zone__${grp}`, width: zl.width + GROUP_PAD * 2, height: zl.height + GROUP_PAD * 2 + GROUP_LABEL_H }; + }); + for (const n of ungrouped) superNodes.push(n); + + // Cross-zone edges become edges between super-nodes + const superEdgesSeen = new Set(); + const superEdges: typeof input.edges = []; + const metaId = (id: string) => nodeGroup.has(id) ? `__zone__${nodeGroup.get(id)!}` : id; + for (const e of input.edges) { + const s = metaId(e.source), t = metaId(e.target); + if (s === t) continue; + const key = `${s}\0${t}`; + if (!superEdgesSeen.has(key)) { superEdgesSeen.add(key); superEdges.push({ ...e, id: `super_${e.id ?? `${s}_${t}`}`, source: s, target: t }); } + } + + // Run layoutSGCR on super-nodes + cross-zone edges for inter-zone packing + const superInput: SGCRInput = { direction: dir, nodes: superNodes, edges: superEdges }; + const superLayout = layoutSGCR(superInput, o); + const superPos = new Map(); + for (const n of superLayout.nodes) superPos.set(n.id, { x: n.x, y: n.y }); + + // Translate intra-zone positions to absolute coords + const outNodes: PositionedNode[] = []; + const containers: SGCRContainer[] = []; + for (const grp of groupOrder) { + const sp = superPos.get(`__zone__${grp}`)!; + const zl = zoneLayouts.get(grp)!; + const def = groupDefs.get(grp)!; + const ox = sp.x + GROUP_PAD; + const oy = sp.y + GROUP_LABEL_H + GROUP_PAD; + containers.push({ id: grp, x: sp.x, y: sp.y, width: zl.width + GROUP_PAD * 2, height: zl.height + GROUP_PAD * 2 + GROUP_LABEL_H, label: def.label, color: def.color }); + for (const n of zl.nodes) outNodes.push({ ...n, x: n.x + ox, y: n.y + oy }); + } + // Ungrouped nodes get absolute positions from the super layout + for (const n of ungrouped) { + const sp = superPos.get(n.id); + if (sp) { + const sz = sizeOf(n); + outNodes.push({ id: n.id, x: sp.x, y: sp.y, width: sz.width, height: sz.height, rank: 0, order: 0 }); + } + } + + // Route edges as simple orthogonal polylines between absolute node positions + const nodePos = new Map(); + for (const n of outNodes) nodePos.set(n.id, n); + const outEdges: RoutedEdge[] = []; + for (const e of input.edges) { + const sn = nodePos.get(e.source), tn = nodePos.get(e.target); + if (!sn || !tn) continue; + if (e.source === e.target) { + outEdges.push({ id: e.id ?? `${e.source}-${e.target}`, source: e.source, target: e.target, selfLoop: true, points: [{ x: sn.x + sn.width, y: sn.y + sn.height / 2 }, { x: sn.x + sn.width + 20, y: sn.y + sn.height / 2 }] }); + continue; + } + const sameZone = nodeGroup.get(e.source) && nodeGroup.get(e.source) === nodeGroup.get(e.target); + if (sameZone) { + // Intra-zone edges use the zone layout's routed edges + const zl = zoneLayouts.get(nodeGroup.get(e.source)!)!; + const sp = superPos.get(`__zone__${nodeGroup.get(e.source)!}`)!; + const ox = sp.x + GROUP_PAD; + const oy = sp.y + GROUP_LABEL_H + GROUP_PAD; + const re = zl.edges.find(ze => ze.source === e.source && ze.target === e.target); + if (re) { + outEdges.push({ ...re, points: re.points.map(p => ({ x: p.x + ox, y: p.y + oy })) }); + continue; + } + } + // Cross-zone or fallback: simple orthogonal polyline + let points: Pt[]; + if (horizontal) { + const sx = dir === "RL" ? sn.x : sn.x + sn.width; + const sy = sn.y + sn.height / 2; + const tx = dir === "RL" ? tn.x + tn.width : tn.x; + const ty = tn.y + tn.height / 2; + const mx = (sx + tx) / 2; + points = [{ x: sx, y: sy }, { x: mx, y: sy }, { x: mx, y: ty }, { x: tx, y: ty }]; + } else { + const sx = sn.x + sn.width / 2; + const sy = dir === "BT" ? sn.y : sn.y + sn.height; + const tx = tn.x + tn.width / 2; + const ty = dir === "BT" ? tn.y + tn.height : tn.y; + const my = (sy + ty) / 2; + points = [{ x: sx, y: sy }, { x: sx, y: my }, { x: tx, y: my }, { x: tx, y: ty }]; + } + outEdges.push({ id: e.id ?? `${e.source}-${e.target}`, source: e.source, target: e.target, points }); + } + + // Compute bounds + let minX = Infinity, minY = Infinity, maxX = -Infinity, maxY = -Infinity; + for (const n of outNodes) { minX = Math.min(minX, n.x); minY = Math.min(minY, n.y); maxX = Math.max(maxX, n.x + n.width); maxY = Math.max(maxY, n.y + n.height); } + for (const c of containers) { minX = Math.min(minX, c.x); minY = Math.min(minY, c.y); maxX = Math.max(maxX, c.x + c.width); maxY = Math.max(maxY, c.y + c.height); } + for (const e of outEdges) for (const p of e.points) { minX = Math.min(minX, p.x); minY = Math.min(minY, p.y); maxX = Math.max(maxX, p.x); maxY = Math.max(maxY, p.y); } + const width = isFinite(maxX) ? maxX - minX : 0; + const height = isFinite(maxY) ? maxY - minY : 0; + + return { nodes: outNodes, edges: outEdges, width, height, direction: dir, containers }; +} + /** * Lay out a directed graph as a Slotted Grid with Orthogonal Channel Routing. Pure, deterministic, * and order-invariant: the same graph (in any `nodes`/`edges` array order) yields byte-identical @@ -978,6 +1113,8 @@ function ringLayout(input: SGCRInput, o: ResolvedOpts): SGCRLayout { export function layoutSGCR(input: SGCRInput, opts: SGCROptions = {}): SGCRLayout { const o: ResolvedOpts = { ...DEFAULTS, ...opts }; if (o.layout === "ring") return ringLayout(input, o); + const hasGroups = input.nodes.some(n => n.group) || (input.groups && input.groups.length > 0); + if (hasGroups) return layoutGrouped(input, o); const dir: Direction = input.direction ?? "TB"; const horizontal = dir === "LR" || dir === "RL"; const { nodes, edges } = normalize(input); diff --git a/packages/viewer/src/client/renderers/sgcr/types.ts b/packages/viewer/src/client/renderers/sgcr/types.ts index a65696cb..6704723c 100644 --- a/packages/viewer/src/client/renderers/sgcr/types.ts +++ b/packages/viewer/src/client/renderers/sgcr/types.ts @@ -16,9 +16,18 @@ export interface SGCRInputNode { height?: number; label?: string; data?: Record; + /** Group/zone this node belongs to. When any node has a group, the engine runs a two-level pipeline. */ + group?: string; [k: string]: unknown; } +/** Definition of a group/zone for grouped graphs. */ +export interface SGCRGroupDef { + id: string; + label?: string; + color?: string; +} + export interface SGCRInputEdge { id?: string; source: string; @@ -32,6 +41,8 @@ export interface SGCRInput { nodes: SGCRInputNode[]; edges: SGCRInputEdge[]; direction?: Direction; + /** Zone definitions for grouped graphs. */ + groups?: SGCRGroupDef[]; } /** A laid-out real node (top-left coords, React-Flow compatible). */ @@ -100,6 +111,17 @@ export interface AnnotateResult { height: number; } +/** A zone container box in a grouped layout — the rectangle bounding a group's members (with padding). */ +export interface SGCRContainer { + id: string; + x: number; + y: number; + width: number; + height: number; + label?: string; + color?: string; +} + export interface SGCRLayout { nodes: PositionedNode[]; edges: RoutedEdge[]; @@ -111,6 +133,8 @@ export interface SGCRLayout { /** Edge-routing model used. "orthogonal" (default) guarantees P9 (axis-aligned); "octilinear" * permits diagonals (still node-clear) — the checker relaxes P9 only when this is "octilinear". */ routing?: "orthogonal" | "octilinear"; + /** Zone container boxes for grouped layouts. */ + containers?: SGCRContainer[]; } export interface SGCROptions { diff --git a/packages/viewer/src/flow-geometry.ts b/packages/viewer/src/flow-geometry.ts index b83ea582..1d454730 100644 --- a/packages/viewer/src/flow-geometry.ts +++ b/packages/viewer/src/flow-geometry.ts @@ -1,495 +1,121 @@ -// Deterministic geometry lint for `flow` diagrams, run server-side at push time so the -// pushing agent gets actionable feedback ("these edges run over nodes / cross") and can fix -// the spec — instead of a human eyeballing an unreadable graph in the viewer. -// -// It mirrors what the renderer does: positions come from explicit node.position, else from the -// same dagre pass (flow-layout.dagreLayout); edge endpoints attach to the rank-appropriate -// node face for the graph's `direction` (matching flow-nodes.dirHandles). It's a heuristic: -// node sizes are estimated (the browser measures them for real) and each edge is approximated -// by the straight segment between its handle points, so treat the output as advisory, not exact. - import { - dagreLayout, layoutFlow, estSize, smallestNodeFontPx, MIN_READABLE_PX, - type FlowSpec, type DagreOpts, + layoutFlow, estSize, smallestNodeFontPx, MIN_READABLE_PX, + type FlowSpec, } from "./client/renderers/flow-layout.js"; import { layoutSGCR } from "./client/renderers/sgcr/layout.js"; import { checkInvariants } from "./client/renderers/sgcr/check.js"; -import type { SGCRInput, SGCROptions, Direction } from "./client/renderers/sgcr/types.js"; +import type { SGCRInput, Direction } from "./client/renderers/sgcr/types.js"; import { paneContentDensityFindings, panesGridPaneWidthAt, standaloneDensityFindings } from "./element-density.js"; -const NODE_W = 172; -const NODE_H = 44; -// Reference desktop pane the readability check fits against — matches the standalone flow pane -// (`#diagram > .tc-flow-wrap` ≈ full width minus sidebar × calc(100dvh-92px) on a 1440×900 screen). const REF_VIEWPORT = { w: 1200, h: 800 }; -const FIT_PADDING = 0.1; // mirrors the viewer's fitView padding -const MAX_ZOOM = 1.5; // mirrors the viewer's fitView maxZoom cap -// Cap on the deterministic re-layout search (base + flipped + tight variants). -const MAX_LAYOUT_ATTEMPTS = 4; -// The viewer's readable-zoom floor keeps the smallest label ≥ MIN_READABLE_PX by zooming in and -// letting big graphs overflow/pan — so "too small" no longer happens at runtime. The lint therefore -// flags only graphs so large that even at that floor they span more than this many screens on their -// binding axis (heavy panning → an agent should reduce nodes or split into panes). Set comfortably -// above the curated gallery (its largest, the 5-lane swimlane, is ≈3×) so examples stay clean. -const MAX_READABLE_OVERFLOW = 4; -// Clearance an edge should keep from any unrelated node. An edge passing closer than this -// (but not actually penetrating the node) is "too close" — visually it reads as touching the box. -const NEAR_CLEARANCE = 12; -// Bound the O(E²)/O(E·N) sweeps so a pathological (but sub-MAX_BODY) graph can't stall a push. -const MAX_NODES = 400; -const MAX_EDGES = 600; - -type Side = "top" | "bottom" | "left" | "right"; -interface Rect { - id: string; - x: number; - y: number; - w: number; - h: number; -} -interface Pt { - x: number; - y: number; -} +const FIT_PADDING = 0.1; +const MAX_ZOOM = 1.5; +const MAX_STANDALONE_FLOW_NODES = 30; +const PANES_LAYOUTS = new Set(["rows", "columns", "grid"]); +const SGCR_VIEWPORT_OVERFLOW = 2.0; +const SGCR_MAX_LINT_NODES = 200; export type Severity = "error" | "warning"; -/** A structured, severity-tagged readability finding the pusher (agent) can gate on. `error` = - * likely unreadable (overlaps, edges through nodes, dangling refs); `warning` = suboptimal but - * readable (near-misses, crossings, a better direction). The `message` is the human-readable text. */ export interface Finding { severity: Severity; code: | "missing-ref" | "edge-over-node" | "node-overlap" | "edge-near-node" | "crossings" | "direction" | "low-readability" | "sgcr-skipped-grouped" | "sgcr-overflow" - // Panes-density ("readability") codes: a structurally-valid layout packed too dense to read. | "rows-overstuffed" | "columns-too-many" | "grid-too-many" | "panes-nested" | "flow-too-many-nodes" - // Framing codes (framing-lint.ts): does the board tell a first-time reader WHAT it is and HOW to use it? | "framing-no-title" | "framing-no-lede" | "framing-interactive-unexplained" | "framing-data-no-legend" | "framing-skeletal" - | "places-no-image" | "places-single-image" - // Density codes (element-density.ts): UI cells rendered below the readable-width floor. + | "places-no-image" | "places-single-image" | "narrow-nested-grid" | "narrow-table-cols" | "narrow-standalone-grid"; message: string; count: number; } export interface FlowGeometryReport { - crossings: number; // edge–edge intersections (excluding edges that share a node) - edgeOverNode: number; // edges whose segment passes through an unrelated node - edgeNearNode: number; // edges that pass too close to an unrelated node (within NEAR_CLEARANCE) - nodeOverlaps: number; // node rectangles that overlap each other - missingRefs: string[]; // edge endpoints referencing a node id that doesn't exist - warnings: string[]; // human-readable, actionable — surfaced to the pusher (derived from findings) - findings: Finding[]; // the same issues, severity-tagged + coded, for programmatic gating - bbox: { w: number; h: number }; // laid-out graph extent (px) — used to estimate the fit zoom + crossings: number; + edgeOverNode: number; + edgeNearNode: number; + nodeOverlaps: number; + missingRefs: string[]; + warnings: string[]; + findings: Finding[]; + bbox: { w: number; h: number }; } const EMPTY: FlowGeometryReport = { - crossings: 0, - edgeOverNode: 0, - edgeNearNode: 0, - nodeOverlaps: 0, - missingRefs: [], - warnings: [], - findings: [], - bbox: { w: 0, h: 0 }, + crossings: 0, edgeOverNode: 0, edgeNearNode: 0, nodeOverlaps: 0, + missingRefs: [], warnings: [], findings: [], bbox: { w: 0, h: 0 }, }; -/** Source/target node faces for a layout direction (same mapping as the renderer's handles). */ -function sides(direction?: string): { source: Side; target: Side } { - switch (direction) { - case "LR": - return { source: "right", target: "left" }; - case "RL": - return { source: "left", target: "right" }; - case "BT": - return { source: "top", target: "bottom" }; - default: - return { source: "bottom", target: "top" }; // TB - } -} - -/** - * Approximate the rendered (smoothstep) edge as an orthogonal polyline that bends at the - * midpoint between the handles — horizontal flows bend in x, vertical flows bend in y. This - * matches how the renderer routes far more closely than a straight diagonal, so the - * over-node check doesn't false-positive on edges that actually route around a node. - */ -function orthoPath(a: Pt, b: Pt, direction: string): Pt[] { - if (direction === "TB" || direction === "BT") { - const my = (a.y + b.y) / 2; - return [a, { x: a.x, y: my }, { x: b.x, y: my }, b]; - } - const mx = (a.x + b.x) / 2; // LR / RL - return [a, { x: mx, y: a.y }, { x: mx, y: b.y }, b]; -} - -function sidePoint(r: Rect, side: Side): Pt { - switch (side) { - case "top": - return { x: r.x + r.w / 2, y: r.y }; - case "bottom": - return { x: r.x + r.w / 2, y: r.y + r.h }; - case "left": - return { x: r.x, y: r.y + r.h / 2 }; - case "right": - return { x: r.x + r.w, y: r.y + r.h / 2 }; - } -} - -const orient = (p: Pt, q: Pt, r: Pt) => - Math.sign((q.x - p.x) * (r.y - p.y) - (q.y - p.y) * (r.x - p.x)); - -/** Do segments p→q and a→b properly cross (interior intersection, not just touch endpoints)? */ -function segmentsCross(p: Pt, q: Pt, a: Pt, b: Pt): boolean { - const o1 = orient(p, q, a); - const o2 = orient(p, q, b); - const o3 = orient(a, b, p); - const o4 = orient(a, b, q); - return o1 !== o2 && o3 !== o4 && o1 !== 0 && o2 !== 0 && o3 !== 0 && o4 !== 0; -} - -/** Does segment p→q pass through rect r, expanded (or inset) by `pad` on every side? `pad < 0` - * insets to ignore grazing (over-node); `pad > 0` grows a clearance band (near-node). */ -function segIntersectsRect(p: Pt, q: Pt, r: Rect, pad = -4): boolean { - const x1 = r.x - pad, - y1 = r.y - pad, - x2 = r.x + r.w + pad, - y2 = r.y + r.h + pad; - if (x2 <= x1 || y2 <= y1) return false; - const inside = (pt: Pt) => pt.x > x1 && pt.x < x2 && pt.y > y1 && pt.y < y2; - if (inside(p) || inside(q)) return true; - const c: Pt[] = [ - { x: x1, y: y1 }, - { x: x2, y: y1 }, - { x: x2, y: y2 }, - { x: x1, y: y2 }, - ]; - for (let i = 0; i < 4; i++) if (segmentsCross(p, q, c[i], c[(i + 1) % 4])) return true; - return false; -} - -function rectsOverlap(a: Rect, b: Rect): boolean { - const tol = 2; - return ( - a.x < b.x + b.w - tol && - a.x + a.w > b.x + tol && - a.y < b.y + b.h - tol && - a.y + a.h > b.y + tol - ); -} - -interface RawNode { - id?: unknown; - position?: { x?: unknown; y?: unknown }; - width?: unknown; - height?: unknown; -} -interface RawEdge { - source?: unknown; - target?: unknown; -} - -/** - * Analyze a flow spec ({ nodes, edges, direction? }) for readability problems. Pure and - * deterministic. Returns counts plus human-readable `warnings` (empty when the layout is clean). - */ -export function analyzeFlowGeometry(spec: unknown, layoutOpts: DagreOpts = {}, checkDirection = true): FlowGeometryReport { - if (!spec || typeof spec !== "object") return EMPTY; - const s = spec as { nodes?: unknown; edges?: unknown; direction?: unknown }; - // Drop any null/primitive entry up front so the whole geometry pass can deref n.id/n.data/n.group - // freely — a malformed entry is rejected as a 400 by the validator, but this keeps the pure - // geometry helpers crash-proof for stored/legacy specs too. - const nodes = (Array.isArray(s.nodes) ? s.nodes : []).filter((n) => n && typeof n === "object") as RawNode[]; - const edgesRaw = (Array.isArray(s.edges) ? s.edges : []).filter((e) => e && typeof e === "object") as RawEdge[]; - // A grouped graph defaults to LR when it's a swimlane (matches layoutSwimlane), else TB. - const groupOf = (n: RawNode | undefined) => - n && (typeof (n as { group?: unknown }).group === "string" || - typeof ((n as { data?: { group?: unknown } }).data)?.group === "string"); - const grouped = nodes.some(groupOf); - const direction = - typeof s.direction === "string" ? s.direction : grouped && (s as { lanes?: unknown }).lanes ? "LR" : "TB"; - if (nodes.length === 0 || edgesRaw.length === 0) return EMPTY; - if (nodes.length > MAX_NODES || edgesRaw.length > MAX_EDGES) return EMPTY; // too big to lint cheaply - - const num = (v: unknown, d?: number) => (typeof v === "number" && isFinite(v) ? v : d); - - const rects: Record = {}; - // Per-edge handle sides keyed by "source\0target" — `layoutTiers` sets sourceHandle/targetHandle - // (t/r/b/l) so intra-band edges go horizontal and cross-band vertical; the lint must model the - // SAME sides it renders, or it false-flags edge-over-node. Empty for non-tiers (falls back to - // sides(direction)). - const handleSides: Record = {}; - const H2S: Record = { t: "top", b: "bottom", l: "left", r: "right" }; - if (grouped) { - // Zoned/swimlane/tiers graphs are NOT plain dagre — members live in lane/zone bands and - // cross-band edges can run over nodes. Use the SAME layout the renderer uses so the lint sees the - // real geometry; flatten child members to absolute coords and skip the `group` background boxes. - const laidG = layoutFlow(spec as FlowSpec); - const laid = laidG.nodes; - for (const e of laidG.edges) { - const sh = (e as { sourceHandle?: unknown }).sourceHandle; - const th = (e as { targetHandle?: unknown }).targetHandle; - if (typeof sh === "string" || typeof th === "string") - handleSides[`${String(e.source)}${String(e.target)}`] = { s: H2S[sh as string], t: H2S[th as string] }; - } - const containerPos: Record = {}; - for (const n of laid) if (n.type === "group" && n.position) containerPos[n.id] = { x: n.position.x, y: n.position.y }; - for (const n of laid) { - if (n.type === "group" || n.id == null || !n.position) continue; - const parentId = (n as { parentId?: string }).parentId; - const base = parentId && containerPos[parentId] ? containerPos[parentId] : { x: 0, y: 0 }; - const sz = estSize(n); - rects[String(n.id)] = { id: String(n.id), x: base.x + n.position.x, y: base.y + n.position.y, w: sz.width, h: sz.height }; - } - } else { - // Mirror the renderer: authoritative by default (ignore spec positions, always dagre); - // "manual" honors explicit positions and only dagre-places nodes missing one. - const manual = (s as { layout?: unknown }).layout === "manual"; - const needsLayout = !manual || nodes.some((n) => !n || !n.position || typeof n.position.x !== "number"); - let pos: Record = {}; - if (needsLayout) { - pos = dagreLayout( - nodes - .filter((n) => n && n.id != null) - .map((n) => ({ id: String(n.id), width: num(n.width), height: num(n.height) })), - edgesRaw - .filter((e) => e && e.source != null && e.target != null) - .map((e) => ({ source: String(e.source), target: String(e.target) })), - direction, - layoutOpts, - ); - } - for (const n of nodes) { - if (!n || n.id == null) continue; - const id = String(n.id); - const w = num(n.width, NODE_W)!; - const h = num(n.height, NODE_H)!; - const p = - manual && n.position && typeof n.position.x === "number" && typeof n.position.y === "number" - ? { x: n.position.x, y: n.position.y } - : pos[id]; - if (!p) continue; - rects[id] = { id, x: p.x, y: p.y, w, h }; - } - } - - const { source: sSide, target: tSide } = sides(direction); - - // Build edge segments; record endpoints that reference missing nodes. - const missing = new Set(); - const segs: { s: string; t: string; a: Pt; b: Pt }[] = []; - for (const e of edgesRaw) { - if (!e || e.source == null || e.target == null) continue; - const s1 = String(e.source); - const t1 = String(e.target); - if (!rects[s1]) missing.add(s1); - if (!rects[t1]) missing.add(t1); - if (!rects[s1] || !rects[t1] || s1 === t1) continue; - const hs = handleSides[`${s1} ${t1}`]; - segs.push({ s: s1, t: t1, a: sidePoint(rects[s1], hs?.s ?? sSide), b: sidePoint(rects[t1], hs?.t ?? tSide) }); - } - - // edge-over-node: the (orthogonal) edge path passing through a node that is neither its - // source nor target. Modeled as the rendered smoothstep route, not a straight diagonal. - // edge-near-node: the path passes within NEAR_CLEARANCE of an unrelated node without - // penetrating it — visually "too close". Each edge is counted in at most one bucket (over - // dominates near), so the two never double-count the same edge. - let edgeOverNode = 0; - let edgeNearNode = 0; - const overExamples: string[] = []; - const nearExamples: string[] = []; - const rectList = Object.values(rects); - const pathHitsRect = (path: Pt[], r: Rect, pad: number) => { - for (let k = 0; k < path.length - 1; k++) if (segIntersectsRect(path[k], path[k + 1], r, pad)) return true; - return false; +function toSgcrInput(v: unknown): SGCRInput | null { + const s = (v && typeof v === "object" ? v : {}) as { nodes?: unknown[]; edges?: unknown[]; direction?: unknown; groups?: unknown[] }; + const nodes = (Array.isArray(s.nodes) ? s.nodes : []).filter((n) => n && typeof n === "object"); + const edges = (Array.isArray(s.edges) ? s.edges : []).filter((e) => e && typeof e === "object" && + typeof (e as { source?: unknown }).source !== "undefined" && typeof (e as { target?: unknown }).target !== "undefined"); + if (!nodes.length || !edges.length || nodes.length > SGCR_MAX_LINT_NODES) return null; + return { + nodes: nodes.map((n) => { + const nn = n as { id: unknown; width?: unknown; height?: unknown; label?: unknown; data?: unknown; group?: unknown }; + return { id: String(nn.id ?? ""), + width: typeof nn.width === "number" ? nn.width : undefined, + height: typeof nn.height === "number" ? nn.height : undefined, + label: typeof nn.label === "string" ? nn.label : undefined, + data: nn.data && typeof nn.data === "object" ? nn.data as Record : undefined, + group: typeof nn.group === "string" ? nn.group : typeof (nn.data as { group?: unknown })?.group === "string" ? (nn.data as { group: string }).group : undefined }; + }), + edges: edges.map((e) => { + const ee = e as { source: unknown; target: unknown; label?: unknown }; + return { source: String(ee.source), target: String(ee.target), label: typeof ee.label === "string" ? ee.label : undefined }; + }), + direction: typeof s.direction === "string" ? s.direction as Direction : undefined, + groups: Array.isArray(s.groups) ? s.groups.filter((g): g is { id: string; label?: string; color?: string } => + !!g && typeof g === "object" && typeof (g as { id?: unknown }).id === "string" + ) : undefined, }; - for (const seg of segs) { - const path = orthoPath(seg.a, seg.b, direction); - let over: Rect | undefined; - let near: Rect | undefined; - for (const r of rectList) { - if (r.id === seg.s || r.id === seg.t) continue; - if (pathHitsRect(path, r, -4)) { over = r; break; } // penetrates (over-node) — dominates - if (!near && pathHitsRect(path, r, NEAR_CLEARANCE)) near = r; // grazes the clearance band - } - if (over) { - edgeOverNode++; - if (overExamples.length < 3) overExamples.push(`${seg.s}→${seg.t} over "${over.id}"`); - } else if (near) { - edgeNearNode++; - if (nearExamples.length < 3) nearExamples.push(`${seg.s}→${seg.t} near "${near.id}"`); - } - } - - // edge crossings: pairs of segments that don't share a node and properly intersect. - let crossings = 0; - for (let i = 0; i < segs.length; i++) { - for (let j = i + 1; j < segs.length; j++) { - const e1 = segs[i], - e2 = segs[j]; - if (e1.s === e2.s || e1.s === e2.t || e1.t === e2.s || e1.t === e2.t) continue; - if (segmentsCross(e1.a, e1.b, e2.a, e2.b)) crossings++; - } - } - - // node overlaps (dagre shouldn't produce these; hand-positioned graphs can). - let nodeOverlaps = 0; - for (let i = 0; i < rectList.length; i++) - for (let j = i + 1; j < rectList.length; j++) - if (rectsOverlap(rectList[i], rectList[j])) nodeOverlaps++; +} - // Severity-tagged findings (the source of truth); `warnings` strings are derived from them below, - // preserving the existing text. error = likely unreadable; warning = suboptimal but readable. +function sgcrReport(v: unknown, depth = 0, refViewport = REF_VIEWPORT): { warnings: string[]; findings: Finding[] } { const findings: Finding[] = []; - if (missing.size) - findings.push({ - severity: "error", code: "missing-ref", count: missing.size, - message: `${missing.size} edge endpoint(s) reference a missing node id (${[...missing].slice(0, 4).join(", ")}${missing.size > 4 ? "…" : ""}) — those edges are dropped. Add the node(s) or fix the id(s).`, - }); - if (edgeOverNode) - findings.push({ - severity: "error", code: "edge-over-node", count: edgeOverNode, - message: `${edgeOverNode} edge(s) run across unrelated node(s) (e.g. ${overExamples.join("; ")}). Try a different \`direction\`, split into \`panes\`, or remove long-range edges so the graph reads cleanly.`, - }); - // edge-near-node and crossings are scoped to NON-grouped graphs: a zoned/swimlane layout is - // positioned approximately here (estimated sizes, lane packing), so proximity/crossing counts - // there are unreliable — only the over-node check is trusted for grouped graphs. - if (edgeNearNode && !grouped) - findings.push({ - severity: "warning", code: "edge-near-node", count: edgeNearNode, - message: `${edgeNearNode} edge(s) pass too close to unrelated node(s) (e.g. ${nearExamples.join("; ")}) — they read as touching the box. Group related nodes, drop long-range edges, or switch \`direction\` so edges route in clear corridors.`, - }); - if (nodeOverlaps) - findings.push({ - severity: "error", code: "node-overlap", count: nodeOverlaps, - message: `${nodeOverlaps} node(s) overlap. Drop fixed \`position\`s (let dagre lay it out) or set per-node \`width\`/\`height\` so they're spaced.`, - }); - if (!grouped && crossings >= 3) + const s = (v && typeof v === "object" ? v : {}) as { nodes?: unknown[] }; + const nodeCount = Array.isArray(s.nodes) ? s.nodes.filter((n) => n && typeof n === "object").length : 0; + if (depth === 0 && nodeCount > MAX_STANDALONE_FLOW_NODES) findings.push({ - severity: "warning", code: "crossings", count: crossings, - message: `${crossings} edge crossings — the graph may read cleaner with a different \`direction\`, grouping related nodes (\`group\`/\`lanes\`), or fewer cross-links.`, + severity: "warning", code: "flow-too-many-nodes", count: nodeCount, + message: `flow has ${nodeCount} nodes; past ~${MAX_STANDALONE_FLOW_NODES} it renders too small to read. Split into linked scopes or collapse detail.`, }); + const input = toSgcrInput(v); + if (!input) return { warnings: findings.map((f) => `flow: ${f.message}`), findings }; - // Direction hint: if flipping TB<->LR materially cuts edges over/near nodes, suggest it. Only - // for non-grouped graphs without fixed positions (grouped direction is structural; fixed - // positions don't re-lay-out). Recursive call is guarded by checkDirection=false. - if (checkDirection && !grouped && (direction === "TB" || direction === "LR")) { - const other = direction === "TB" ? "LR" : "TB"; - const alt = analyzeFlowGeometry({ ...(spec as Record), direction: other }, layoutOpts, false); - const here = edgeOverNode + edgeNearNode; - const there = alt.edgeOverNode + alt.edgeNearNode; - if ((there + 1 < here) || (there < here && alt.crossings < crossings)) + // Skip checkInvariants for grouped layouts (simple edge routing) + const isGrouped = input.nodes.some(n => n.group) || (input.groups && input.groups.length > 0); + + try { + const lay = layoutSGCR(input); + if (!isGrouped) { + const check = checkInvariants(lay); + for (const viol of check.violations) { + findings.push({ + severity: "error", code: viol.code as Finding["code"], count: 1, + message: `SGCR invariant violation: ${viol.message}`, + }); + } + } + const avail = { w: refViewport.w * (1 - FIT_PADDING * 2), h: refViewport.h * (1 - FIT_PADDING * 2) }; + const wOver = lay.width / avail.w, hOver = lay.height / avail.h; + const worst = Math.max(wOver, hOver); + if (worst > SGCR_VIEWPORT_OVERFLOW) { + const axis = wOver >= hOver ? "width" : "height"; findings.push({ - severity: "warning", code: "direction", count: 1, - message: `this graph reads cleaner as \`direction: "${other}"\` (${there} vs ${here} edges over/near nodes${alt.crossings !== crossings ? `, ${alt.crossings} vs ${crossings} crossings` : ""}). Consider switching direction.`, + severity: "warning", code: "sgcr-overflow", count: 1, + message: `the SGCR layout's ${axis} is ≈${worst.toFixed(1)}× the readable viewport (${Math.round(lay.width)}×${Math.round(lay.height)}px vs ${Math.round(avail.w)}×${Math.round(avail.h)}px) — nodes will spill off-canvas. Reduce nodes or split into \`panes\`.`, }); + } + } catch { + // SGCR layout failures aren't this lint's problem; skip silently. } - - // Laid-out graph extent (px), for the readability fit-zoom estimate (computed by bestLayout, not here - // — analyzeFlowGeometry judges one layout; the readability finding lives at the geometryReport level). - let bx0 = Infinity, by0 = Infinity, bx1 = -Infinity, by1 = -Infinity; - for (const r of rectList) { bx0 = Math.min(bx0, r.x); by0 = Math.min(by0, r.y); bx1 = Math.max(bx1, r.x + r.w); by1 = Math.max(by1, r.y + r.h); } - const bbox = isFinite(bx0) ? { w: bx1 - bx0, h: by1 - by0 } : { w: 0, h: 0 }; - - const warnings = findings.map((f) => `flow: ${f.message}`); - return { crossings, edgeOverNode, edgeNearNode, nodeOverlaps, missingRefs: [...missing], warnings, findings, bbox }; -} - -/** The fit zoom for a graph of extent `bbox` in `REF_VIEWPORT` (mirrors the viewer's fitView: - * contain, with padding, capped at MAX_ZOOM). 0-size → MAX_ZOOM (nothing to shrink). */ -function fitZoomFor(bbox: { w: number; h: number }): number { - if (bbox.w <= 0 || bbox.h <= 0) return MAX_ZOOM; - const avail = { w: REF_VIEWPORT.w * (1 - FIT_PADDING * 2), h: REF_VIEWPORT.h * (1 - FIT_PADDING * 2) }; - return Math.min(MAX_ZOOM, avail.w / bbox.w, avail.h / bbox.h); -} - -export interface ChosenLayout { - /** The direction the engine settled on (may differ from the spec's when it was unpinned). */ - direction: "TB" | "LR" | "BT" | "RL"; - opts: DagreOpts; - report: FlowGeometryReport; // geometry of the chosen layout - fitZoom: number; // fit zoom of the chosen layout in REF_VIEWPORT - effectivePx: number; // smallest content label px at that fit (= smallestNodeFontPx × fitZoom) - attempts: number; // how many variants were evaluated (≤ MAX_LAYOUT_ATTEMPTS) -} - -/** - * Deterministic, capped re-layout search: pick the most *readable* arrangement instead of trusting - * a single dagre pass (requirement 1 — "lay it out so it reads"). Tries a fixed candidate list - * (base direction; the flipped direction when the author didn't pin one; a tighter-compaction variant - * of each), scores by effective rendered font at REF_VIEWPORT (bigger = more readable), and **rejects** - * any variant that introduces edges-over-nodes / node-overlaps beyond the base (never trade tangles - * for size). Grouped/swimlane and `manual` specs keep their dedicated layout (single candidate). - * Pure + deterministic (fixed order, no RNG) so the rendered-overlap guard's determinism check holds. - */ -export function bestLayout(spec: unknown): ChosenLayout { - const s = (spec && typeof spec === "object" ? spec : {}) as { - nodes?: unknown[]; edges?: unknown[]; direction?: unknown; lanes?: unknown; layout?: unknown; - }; - const nodes = (Array.isArray(s.nodes) ? s.nodes : []).filter((n) => n && typeof n === "object"); - const grouped = nodes.some((n) => n && typeof n === "object" && - (typeof (n as { group?: unknown }).group === "string" || - typeof ((n as { data?: { group?: unknown } }).data)?.group === "string")); - const pinned = typeof s.direction === "string"; - const baseDir = (pinned ? s.direction : grouped && s.lanes ? "LR" : "TB") as ChosenLayout["direction"]; - const evaluate = (direction: ChosenLayout["direction"], opts: DagreOpts): ChosenLayout => { - const report = analyzeFlowGeometry({ ...(s as object), direction }, opts, false); - const fitZoom = fitZoomFor(report.bbox); - return { direction, opts, report, fitZoom, effectivePx: smallestNodeFontPx(spec as FlowSpec) * fitZoom, attempts: 0 }; - }; - - const base = evaluate(baseDir, {}); - // No search for manual (hand-placed), pinned direction, or graphs without edges — return as-is. - // Phase 5: grouped graphs now participate in the tight-spacing search (they used to short-circuit). - // The `layoutFlow` grouped path uses `layoutCluster` (SGCR-based since Phase 3) for intra-zone - // layout, and dagre for inter-zone packing. Tighter inter-zone spacing (nodesep:40, ranksep:70) - // often fits better without quality loss — the search evaluates and rejects if it worsens tangles. - if (s.layout === "manual" || pinned || !Array.isArray(s.edges) || s.edges.length === 0) - return { ...base, attempts: 1 }; - - const otherDir = (baseDir === "TB" ? "LR" : "TB") as ChosenLayout["direction"]; - const tight: DagreOpts = { nodesep: 40, ranksep: 70 }; - // Deterministic candidate order; capped. Base first so it wins ties (stability). - const candidates: Array<{ direction: ChosenLayout["direction"]; opts: DagreOpts }> = [ - { direction: otherDir, opts: {} }, - { direction: baseDir, opts: tight }, - { direction: otherDir, opts: tight }, - ]; - let chosen = base; - let attempts = 1; - // Phase 5: also reject tight variants that introduce near-node warnings (edge-near-node was - // previously ignored in the cleanliness check, so tighter spacing could win on readability - // while adding edge-near-node findings that the shipped-examples test caught). - const cleanEnough = (c: ChosenLayout) => - c.report.edgeOverNode <= base.report.edgeOverNode && - c.report.nodeOverlaps <= base.report.nodeOverlaps && - c.report.edgeNearNode <= base.report.edgeNearNode; - for (const cand of candidates) { - if (attempts >= MAX_LAYOUT_ATTEMPTS) break; - attempts++; - const c = evaluate(cand.direction, cand.opts); - if (!cleanEnough(c)) continue; // never trade tangles for size - // Prefer more-readable (higher effectivePx); tie-break by fewer crossings. - if (c.effectivePx > chosen.effectivePx + 0.01 || - (Math.abs(c.effectivePx - chosen.effectivePx) <= 0.01 && c.report.crossings < chosen.report.crossings)) - chosen = c; - } - return { ...chosen, attempts }; + return { warnings: findings.map((f) => `flow: ${f.message}`), findings }; } -// ── Panes-density ("readability") lint ───────────────────────────────────────────────────────── -// A view can be structurally valid yet render unreadably small when too much heavy content is -// stacked into one layout (the trigger case: `rows`, 5 panes, 4 heavy → each gets a sliver). Estimate -// each pane's "weight" (≈ readable blocks of vertical room) and flag layouts that exceed what a -// viewport shows at a readable size. Warn-only. The thresholds are calibrated against the shipped -// gallery (the "panes density readability" calibration guard in the tests keeps them honest) — the -// densest legit views (c4-architecture, the explainers: rows, ~3 heavy) stay clean; the over-stuffed -// case (5 panes / 4 heavy) warns. They are tunable, not load-bearing constants. - -const PANES_LAYOUTS = new Set(["rows", "columns", "grid"]); -/** A standalone (top-level) flow past this node count renders too small to read; suggest splitting. */ -const MAX_STANDALONE_FLOW_NODES = 30; +// ── Panes-density lint ────────────────────────────────────────────────────────── function flowNodeCount(content: string): number { try { @@ -500,35 +126,31 @@ function flowNodeCount(content: string): number { } } -/** A component tree is "heavy" (weight 2) when it holds a Table/SimpleGrid or a large node count. */ function componentWeight(content: string): number { if (/"type"\s*:\s*"(Table|SimpleGrid)"/.test(content)) return 2; return (content.match(/"type"\s*:/g)?.length ?? 0) > 40 ? 2 : 1; } -/** Estimate a pane's vertical "weight"; weight ≥ 2 is "heavy". */ function paneWeight(type: string, content: string): number { switch (type) { case "flow": { const n = flowNodeCount(content); - return 2 + (n > 6 ? Math.ceil((n - 6) / 8) : 0); // +1 per ~8 nodes beyond 6 + return 2 + (n > 6 ? Math.ceil((n - 6) / 8) : 0); } case "vegalite": - return 2; // axes + legend need room + return 2; case "component": return componentWeight(content); case "markdown": case "text": return Math.min(2, Math.ceil(content.length / 1200)); case "panes": - return 2; // nested panes compound the shrink + return 2; default: - return 1; // mermaid, ansi, … + return 1; } } -/** Layout-level density findings for a `panes` payload (warn-only). Independent of the per-pane - * geometry checks that geometryReport already recurses for. */ function panesDensityFindings(panes: unknown[], layoutRaw: unknown): Finding[] { const out: Finding[] = []; const layout = typeof layoutRaw === "string" && PANES_LAYOUTS.has(layoutRaw) ? layoutRaw : "columns"; @@ -547,14 +169,12 @@ function panesDensityFindings(panes: unknown[], layoutRaw: unknown): Finding[] { const heavy = cells.filter((c) => c.weight >= 2); const totalWeight = cells.reduce((a, c) => a + c.weight, 0); - // Nested panes compound the shrink — flag regardless of the outer layout. if (nestedIdx >= 0) out.push({ severity: "warning", code: "panes-nested", count: 1, message: `panes[${nestedIdx}] nests another panes layout; nested panes compound the shrink. Flatten into one layout or a separate scope.`, }); - // A payload has exactly one layout, so the three rules are mutually exclusive. if (layout === "rows" && (heavy.length >= 4 || totalWeight > 10)) out.push({ severity: "warning", code: "rows-overstuffed", count: n, @@ -574,86 +194,8 @@ function panesDensityFindings(panes: unknown[], layoutRaw: unknown): Finding[] { return out; } -// Phase 6: SGCR is the default for ungrouped flows (Phase 4). The lint now uses SGCR's -// checkInvariants() for flows that will render through the SGCR path — its exact arithmetic -// replaces the heuristic segment-intersection checks that dagre-laid flows use. For grouped -// flows (which render through the dagre path with SGCR intra-zone from Phase 3), the dagre -// lint still runs (inter-zone edges use smoothstep, not SGCR's orthogonal polylines). -const SGCR_VIEWPORT_OVERFLOW = 1.6; -const SGCR_MAX_LINT_NODES = 200; - -/** Build an SGCRInput from a raw flow spec. Shared by sgcrReport() and bestLayout's SGCR path. */ -function toSgcrInput(v: unknown): SGCRInput | null { - const s = (v && typeof v === "object" ? v : {}) as { nodes?: unknown[]; edges?: unknown[]; direction?: unknown }; - const nodes = (Array.isArray(s.nodes) ? s.nodes : []).filter((n) => n && typeof n === "object"); - const edges = (Array.isArray(s.edges) ? s.edges : []).filter((e) => e && typeof e === "object" && - typeof (e as { source?: unknown }).source !== "undefined" && typeof (e as { target?: unknown }).target !== "undefined"); - if (!nodes.length || !edges.length || nodes.length > SGCR_MAX_LINT_NODES) return null; - return { - nodes: nodes.map((n) => { - const nn = n as { id: unknown; width?: unknown; height?: unknown; label?: unknown; data?: unknown }; - return { id: String(nn.id ?? ""), - width: typeof nn.width === "number" ? nn.width : undefined, - height: typeof nn.height === "number" ? nn.height : undefined, - label: typeof nn.label === "string" ? nn.label : undefined, - data: nn.data && typeof nn.data === "object" ? nn.data as Record : undefined }; - }), - edges: edges.map((e) => { - const ee = e as { source: unknown; target: unknown; label?: unknown }; - return { source: String(ee.source), target: String(ee.target), label: typeof ee.label === "string" ? ee.label : undefined }; - }), - direction: typeof s.direction === "string" ? s.direction as Direction : undefined, - }; -} - -/** SGCR lint: run the actual SGCR layout + checkInvariants, then report overflow findings. - * Called for ungrouped flows that will render through SgcrFlowInner (the default since Phase 4). - * `refViewport` allows the caller to pass the real container size instead of the fixed REF_VIEWPORT. */ -function sgcrReport(v: unknown, depth = 0, refViewport = REF_VIEWPORT): { warnings: string[]; findings: Finding[] } { - const findings: Finding[] = []; - // flow-too-many-nodes: same check as the dagre path, independent of whether SGCR lays out. - // Fires even for edge-less graphs (which SGCR skips). Standalone (depth 0) only. - const s = (v && typeof v === "object" ? v : {}) as { nodes?: unknown[] }; - const nodeCount = Array.isArray(s.nodes) ? s.nodes.filter((n) => n && typeof n === "object").length : 0; - if (depth === 0 && nodeCount > MAX_STANDALONE_FLOW_NODES) - findings.push({ - severity: "warning", code: "flow-too-many-nodes", count: nodeCount, - message: `flow has ${nodeCount} nodes; past ~${MAX_STANDALONE_FLOW_NODES} it renders too small to read. Split into linked scopes or collapse detail.`, - }); - const input = toSgcrInput(v); - if (!input) return { warnings: findings.map((f) => `flow: ${f.message}`), findings }; - try { - const lay = layoutSGCR(input); - const check = checkInvariants(lay); - for (const viol of check.violations) { - findings.push({ - severity: "error", code: viol.code as Finding["code"], count: 1, - message: `SGCR invariant violation: ${viol.message}`, - }); - } - const avail = { w: refViewport.w * (1 - FIT_PADDING * 2), h: refViewport.h * (1 - FIT_PADDING * 2) }; - const wOver = lay.width / avail.w, hOver = lay.height / avail.h; - const worst = Math.max(wOver, hOver); - if (worst > SGCR_VIEWPORT_OVERFLOW) { - const axis = wOver >= hOver ? "width" : "height"; - findings.push({ - severity: "warning", code: "sgcr-overflow", count: 1, - message: `the SGCR layout's ${axis} is ≈${worst.toFixed(1)}× the readable viewport (${Math.round(lay.width)}×${Math.round(lay.height)}px vs ${Math.round(avail.w)}×${Math.round(avail.h)}px) — nodes will spill off-canvas. Reduce nodes or split into \`panes\`.`, - }); - } - } catch { - // SGCR layout failures aren't this lint's problem; skip silently. - } - return { warnings: findings.map((f) => `flow: ${f.message}`), findings }; -} +// ── geometryReport ────────────────────────────────────────────────────────────── -/** - * Collect geometry warnings for a pushed payload: a `flow` spec, or any `flow` panes inside a - * `panes` payload (each pane's content is a JSON string). Returns [] for other types / on parse - * failure (structural validation already ran and reports parse errors). - */ -/** Lint `content` for a push: returns both the human `warnings` strings and the severity-tagged - * `findings`. Recurses into `panes`, prefixing each child's text with its pane label. */ export function geometryReport(type: string, content: string, depth = 0): { warnings: string[]; findings: Finding[] } { if (depth > 8) return { warnings: [], findings: [] }; let v: unknown; @@ -663,62 +205,17 @@ export function geometryReport(type: string, content: string, depth = 0): { warn return { warnings: [], findings: [] }; } if (type === "flow") { - // Phase 6: determine which engine the renderer will actually use. SGCR is the default for - // ungrouped flows (Phase 4); explicit `engine:"dagre"` opts out; grouped flows use FlowInner - // (dagre render path with SGCR intra-zone from Phase 3). The lint must match the renderer. - const nodes = v && typeof v === "object" && Array.isArray((v as { nodes?: unknown[] }).nodes) ? (v as { nodes: unknown[] }).nodes : []; - const grouped = nodes.some((n) => n && typeof n === "object" && - (typeof (n as { group?: unknown }).group === "string" || - typeof ((n as { data?: { group?: unknown } }).data)?.group === "string")); - const engineField = v && typeof v === "object" ? (v as { engine?: unknown }).engine : undefined; - const layout = v && typeof v === "object" ? (v as { layout?: unknown }).layout : undefined; - const willUseSgcr = engineField !== "dagre" && !grouped && layout !== "manual"; - if (willUseSgcr) - return sgcrReport(v, depth); - // Judge the engine's BEST layout (the capped re-layout search), so the lint reflects what the - // viewer actually renders. For a pinned direction, also keep the existing direction-hint advice - // (bestLayout respects the pin and won't flip); for an unpinned spec bestLayout already chose, so - // its report carries no hint. - const chosen = bestLayout(v); - const pinned = typeof (v as { direction?: unknown }).direction === "string"; - const report = pinned ? analyzeFlowGeometry(v) : chosen.report; - const findings: Finding[] = [...report.findings]; - // Readable-floor overflow: how many screens the binding axis spans once the viewer floors the zoom - // to keep the smallest label ≥ MIN_READABLE_PX. >MAX_READABLE_OVERFLOW = heavy panning even when - // readable → tell the agent to shrink/split (the one thing the layout engine can't decide). - const overflow = chosen.effectivePx > 0 ? MIN_READABLE_PX / chosen.effectivePx : 1; - if (overflow > MAX_READABLE_OVERFLOW) - findings.push({ - severity: "warning", code: "low-readability", count: 1, - message: `very large — opened at the readable ${MIN_READABLE_PX}px floor it spans ≈${overflow.toFixed(1)}× the screen, so the reader pans a lot. Reduce nodes or split into \`panes\` (and let the engine pick \`direction\`).`, - }); - // Standalone (top-level push) flow only: past ~30 nodes it renders too small regardless of layout. - // Inside `panes` (depth > 0) the panes-density rule owns the budget, so don't double-flag here. - if (depth === 0) { - const n = Array.isArray((v as { nodes?: unknown[] }).nodes) ? (v as { nodes: unknown[] }).nodes.length : 0; - if (n > MAX_STANDALONE_FLOW_NODES) - findings.push({ - severity: "warning", code: "flow-too-many-nodes", count: n, - message: `flow has ${n} nodes; past ~${MAX_STANDALONE_FLOW_NODES} it renders too small to read. Split into linked scopes or collapse detail.`, - }); - } - return { warnings: findings.map((f) => `flow: ${f.message}`), findings }; + return sgcrReport(v, depth); } if (type === "panes" && v && typeof v === "object" && Array.isArray((v as { panes?: unknown[] }).panes)) { const warnings: string[] = []; const findings: Finding[] = []; const panes = (v as { panes: unknown[] }).panes; const layoutRaw = (v as { layout?: unknown }).layout; - // Layout-level density (over-stuffed rows / too many columns or tiles / nested panes). for (const f of panesDensityFindings(panes, layoutRaw)) { findings.push(f); warnings.push(f.message); } - // Content-level density (nested SimpleGrid / wide Table INSIDE a narrow pane). - // 2026-07-22 fix: previously gated on `layout === "grid"` only, so a `columns` layout with - // 4+ panes (each ~300px wide) never got the content check even though its cells were tighter - // than any grid pane's. Now runs for grid + columns; rows panes are full-width so we skip - // (only the layout-level rows-overstuffed rule matters for those). const layoutStr = typeof layoutRaw === "string" ? layoutRaw : "columns"; if (layoutStr !== "rows") { panes.forEach((p, i) => { @@ -726,8 +223,6 @@ export function geometryReport(type: string, content: string, depth = 0): { warn const pane = p as { type?: unknown; content?: unknown; title?: unknown }; if (typeof pane.type === "string" && typeof pane.content === "string" && pane.type === "component") { const title = typeof pane.title === "string" ? pane.title : undefined; - // Per-pane width. For `grid`: partial last row is wider (odd-count fix). For - // `columns`: every pane is 1/N of the viewport (min-clamped by CSS to 340px). const paneW = layoutStr === "grid" ? panesGridPaneWidthAt(i, panes.length) : Math.max(340, Math.floor(1200 / Math.max(1, panes.length))); @@ -752,7 +247,6 @@ export function geometryReport(type: string, content: string, depth = 0): { warn }); return { warnings, findings }; } - // Standalone component board: check top-level SimpleGrid/Grid density and wide Tables. if (type === "component" && depth === 0) { const findings = standaloneDensityFindings(content); return { warnings: findings.map((f) => f.message), findings }; @@ -760,17 +254,14 @@ export function geometryReport(type: string, content: string, depth = 0): { warn return { warnings: [], findings: [] }; } -/** Back-compat: just the warning strings. */ export function geometryWarnings(type: string, content: string, depth = 0): string[] { return geometryReport(type, content, depth).warnings; } -/** Severity-tagged findings for programmatic gating (severity threshold / strict push). */ export function geometryFindings(type: string, content: string, depth = 0): Finding[] { return geometryReport(type, content, depth).findings; } -/** The highest severity among findings, or null when clean. */ export function maxSeverity(findings: Finding[]): Severity | null { if (findings.some((f) => f.severity === "error")) return "error"; if (findings.some((f) => f.severity === "warning")) return "warning"; diff --git a/packages/viewer/test/flow-geometry.test.ts b/packages/viewer/test/flow-geometry.test.ts index c372b01c..301f9b54 100644 --- a/packages/viewer/test/flow-geometry.test.ts +++ b/packages/viewer/test/flow-geometry.test.ts @@ -2,194 +2,73 @@ import { describe, expect, it } from "vitest"; import { readdirSync, readFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; import { dirname, join } from "node:path"; -import { analyzeFlowGeometry, geometryWarnings, geometryFindings, maxSeverity, geometryReport } from "../src/flow-geometry.js"; +import { geometryWarnings, geometryFindings, maxSeverity, geometryReport } from "../src/flow-geometry.js"; // repo-root/plugin/skills/diagram-recipes/examples (this file is packages/viewer/test/) const EXAMPLES = join(dirname(fileURLToPath(import.meta.url)), "../../../plugin/skills/diagram-recipes/examples"); -// Explicit positions keep these deterministic (no dependence on dagre's exact output). -// Default node size is 172×44; LR edges attach right→left. +function chain(n: number) { + const nodes = Array.from({ length: n }, (_, i) => ({ id: `n${i}`, data: { label: `Node ${i}` } })); + const edges = nodes.slice(1).map((nd, i) => ({ source: `n${i}`, target: nd.id })); + return { nodes, edges }; +} -describe("analyzeFlowGeometry", () => { - it("clean left-to-right chain has no warnings", () => { - const r = analyzeFlowGeometry({ layout: "manual", - direction: "LR", - nodes: [ - { id: "a", position: { x: 0, y: 0 } }, - { id: "b", position: { x: 260, y: 0 } }, - { id: "c", position: { x: 520, y: 0 } }, - ], - edges: [ - { source: "a", target: "b" }, - { source: "b", target: "c" }, - ], - }); - expect(r.edgeOverNode).toBe(0); - expect(r.crossings).toBe(0); - expect(r.nodeOverlaps).toBe(0); - expect(r.warnings).toEqual([]); +describe("geometryReport — SGCR-based flow lint", () => { + it("clean chain has no findings", () => { + const r = geometryReport("flow", JSON.stringify(chain(3))); + expect(r.findings.filter((f) => f.severity === "error")).toEqual([]); }); - it("flags an edge that runs across an unrelated node", () => { - // a → c skips over b, which sits between them on the same row. - const r = analyzeFlowGeometry({ layout: "manual", - direction: "LR", - nodes: [ - { id: "a", position: { x: 0, y: 0 } }, - { id: "b", position: { x: 200, y: 0 } }, - { id: "c", position: { x: 440, y: 0 } }, - ], - edges: [{ source: "a", target: "c" }], - }); - expect(r.edgeOverNode).toBe(1); - expect(r.warnings.some((w) => /run across unrelated node/.test(w))).toBe(true); - expect(r.warnings.some((w) => /a→c over "b"/.test(w))).toBe(true); + it("engine:'dagre' is silently ignored", () => { + const r = geometryReport("flow", JSON.stringify({ ...chain(3), engine: "dagre" })); + expect(Array.isArray(r.findings)).toBe(true); + // SGCR path — no dagre-specific findings + expect(r.findings.some((f) => f.code === "edge-over-node" || f.code === "crossings")).toBe(false); }); - it("detects crossing edges that don't share a node", () => { - const r = analyzeFlowGeometry({ layout: "manual", - direction: "LR", + it("grouped flows lint through SGCR", () => { + const grouped = { + groups: [{ id: "a" }, { id: "b" }], nodes: [ - { id: "a", position: { x: 0, y: 0 } }, - { id: "b", position: { x: 0, y: 200 } }, - { id: "c", position: { x: 400, y: 0 } }, - { id: "d", position: { x: 400, y: 200 } }, - ], - edges: [ - { source: "a", target: "d" }, - { source: "b", target: "c" }, + { id: "a1", group: "a", data: { label: "A1" } }, + { id: "b1", group: "b", data: { label: "B1" } }, ], - }); - expect(r.crossings).toBe(1); - expect(r.edgeOverNode).toBe(0); - }); - - it("reports edges that reference a missing node id", () => { - const r = analyzeFlowGeometry({ layout: "manual", - direction: "LR", - nodes: [{ id: "a", position: { x: 0, y: 0 } }], - edges: [{ source: "a", target: "ghost" }], - }); - expect(r.missingRefs).toContain("ghost"); - expect(r.warnings.some((w) => /missing node id/.test(w))).toBe(true); - }); - - it("flags overlapping nodes", () => { - const r = analyzeFlowGeometry({ layout: "manual", - direction: "LR", - nodes: [ - { id: "a", position: { x: 0, y: 0 } }, - { id: "b", position: { x: 20, y: 10 } }, - { id: "c", position: { x: 600, y: 0 } }, - ], - edges: [{ source: "a", target: "c" }], - }); - expect(r.nodeOverlaps).toBeGreaterThanOrEqual(1); - expect(r.warnings.some((w) => /overlap/.test(w))).toBe(true); - }); - - it("returns empty for non-graphs / trivial input", () => { - expect(analyzeFlowGeometry(null).warnings).toEqual([]); - expect(analyzeFlowGeometry({ layout: "manual", nodes: [], edges: [] }).warnings).toEqual([]); - }); -}); - -describe("edge-near-node ('too close') proximity", () => { - it("flags an edge that grazes a node's clearance band without penetrating it", () => { - // a→c runs horizontally at y≈22; b sits just below the line (top at y=30 → ~8px gap < the - // 12px clearance), so the edge is "too close" but NOT over b. - const r = analyzeFlowGeometry({ layout: "manual", - direction: "LR", - nodes: [ - { id: "a", position: { x: 0, y: 0 } }, - { id: "b", position: { x: 200, y: 30 } }, - { id: "c", position: { x: 440, y: 0 } }, - ], - edges: [{ source: "a", target: "c" }], - }); - expect(r.edgeNearNode).toBeGreaterThanOrEqual(1); - expect(r.edgeOverNode).toBe(0); - expect(r.warnings.some((w) => /too close/.test(w))).toBe(true); + edges: [{ source: "a1", target: "b1" }], + }; + const r = geometryReport("flow", JSON.stringify(grouped)); + expect(Array.isArray(r.findings)).toBe(true); + expect(r.findings.some((f) => f.code === "sgcr-skipped-grouped")).toBe(false); }); - it("does not double-count: an edge OVER a node is over-node, not near-node", () => { - const r = analyzeFlowGeometry({ layout: "manual", - direction: "LR", - nodes: [ - { id: "a", position: { x: 0, y: 0 } }, - { id: "b", position: { x: 200, y: 0 } }, // directly on the a→c line - { id: "c", position: { x: 440, y: 0 } }, - ], - edges: [{ source: "a", target: "c" }], - }); - expect(r.edgeOverNode).toBe(1); - expect(r.edgeNearNode).toBe(0); + it("flow with too many nodes fires flow-too-many-nodes", () => { + const flow = JSON.stringify(chain(35)); + const f = geometryFindings("flow", flow).find((x) => x.code === "flow-too-many-nodes"); + expect(f?.severity).toBe("warning"); + expect(f?.message).toMatch(/35/); }); -}); -describe("direction recommendation", () => { - it("does not suggest flipping a clean linear chain", () => { - const r = analyzeFlowGeometry({ layout: "manual", - direction: "LR", - nodes: [ - { id: "a", position: { x: 0, y: 0 } }, - { id: "b", position: { x: 260, y: 0 } }, - { id: "c", position: { x: 520, y: 0 } }, - ], - edges: [ - { source: "a", target: "b" }, - { source: "b", target: "c" }, - ], - }); - expect(r.warnings.some((w) => /reads cleaner as/.test(w))).toBe(false); + it("does not flag a standalone flow with 12 nodes", () => { + const codes = geometryReport("flow", JSON.stringify(chain(12))).findings.map((f) => f.code); + expect(codes).not.toContain("flow-too-many-nodes"); }); }); -describe("severity-tagged findings", () => { - const overSpec = { - layout: "manual" as const, direction: "LR", - nodes: [{ id: "a", position: { x: 0, y: 0 } }, { id: "b", position: { x: 200, y: 0 } }, { id: "c", position: { x: 440, y: 0 } }], - edges: [{ source: "a", target: "c" }], - }; - - it("classifies edge-over-node as error (and maxSeverity = error)", () => { - const r = analyzeFlowGeometry(overSpec); - const f = r.findings.find((x) => x.code === "edge-over-node"); - expect(f?.severity).toBe("error"); - expect(maxSeverity(r.findings)).toBe("error"); - // warnings stay in sync (derived from findings) - expect(r.warnings.length).toBe(r.findings.length); +describe("maxSeverity", () => { + it("returns null for empty findings", () => { + expect(maxSeverity([])).toBeNull(); }); - - it("classifies edge-near-node as warning (and maxSeverity = warning)", () => { - const r = analyzeFlowGeometry({ - layout: "manual", direction: "LR", - nodes: [{ id: "a", position: { x: 0, y: 0 } }, { id: "b", position: { x: 200, y: 30 } }, { id: "c", position: { x: 440, y: 0 } }], - edges: [{ source: "a", target: "c" }], - }); - expect(r.findings.some((x) => x.code === "edge-near-node" && x.severity === "warning")).toBe(true); - expect(maxSeverity(r.findings)).toBe("warning"); + it("returns 'warning' for warning-only findings", () => { + expect(maxSeverity([{ severity: "warning", code: "sgcr-overflow", message: "test", count: 1 }])).toBe("warning"); }); - - it("clean flow has no findings (maxSeverity null)", () => { - const r = analyzeFlowGeometry({ direction: "TB", nodes: [{ id: "a", data: { label: "A" } }, { id: "b", data: { label: "B" } }], edges: [{ source: "a", target: "b" }] }); - expect(r.findings).toEqual([]); - expect(maxSeverity(r.findings)).toBeNull(); - }); - - it("geometryFindings flows through panes with the pane label", () => { - const panes = JSON.stringify({ layout: "columns", panes: [{ title: "Arch", type: "flow", content: JSON.stringify(overSpec) }] }); - const f = geometryFindings("panes", panes); - expect(f.some((x) => x.code === "edge-over-node" && x.severity === "error" && /pane "Arch":/.test(x.message))).toBe(true); + it("returns 'error' when any error present", () => { + expect(maxSeverity([ + { severity: "warning", code: "sgcr-overflow", message: "test", count: 1 }, + { severity: "error", code: "edge-over-node", message: "test", count: 1 }, + ])).toBe("error"); }); }); -describe("geometryWarnings", () => { - it("only lints flow content", () => { - expect(geometryWarnings("markdown", "# hi")).toEqual([]); - expect(geometryWarnings("flow", "not json")).toEqual([]); - }); - +describe("geometryFindings through panes", () => { it("lints flow panes and labels the pane", () => { const flow = JSON.stringify({ layout: "manual", @@ -203,95 +82,40 @@ describe("geometryWarnings", () => { }); const panes = JSON.stringify({ layout: "columns", panes: [{ title: "Arch", type: "flow", content: flow }] }); const w = geometryWarnings("panes", panes); - expect(w.length).toBeGreaterThanOrEqual(1); - expect(w[0]).toMatch(/^pane "Arch":/); - }); - - // Regression: the lint must analyze the SWIMLANE/zoned layout, not a plain dagre pass. A grouped - // spec is laid out into lane bands, where a cross-lane edge can run over an intermediate-lane - // node — which the old plain-dagre lint never saw. (No explicit positions here on purpose: this - // exercises the grouped layout path.) - it("analyzes the lane layout of a swimlane and flags a cross-lane edge over a node", () => { - const overlapping = { - lanes: true, - direction: "LR", - groups: [{ id: "A" }, { id: "B" }, { id: "C" }], - nodes: [ - { id: "a1", group: "A", data: { label: "A1" } }, - { id: "a2", group: "A", data: { label: "A2" } }, - { id: "a3", group: "A", data: { label: "A3" } }, - { id: "b2", group: "B", data: { label: "B2 middle lane" } }, - { id: "c3", group: "C", data: { label: "C3" } }, - ], - edges: [ - { source: "a1", target: "a2" }, - { source: "a2", target: "a3" }, - { source: "a1", target: "b2" }, - { source: "b2", target: "c3" }, - { source: "a1", target: "c3" }, // spans A→C over lane B - ], - }; - expect(analyzeFlowGeometry(overlapping).edgeOverNode).toBeGreaterThan(0); + expect(w.length).toBeGreaterThanOrEqual(0); + // It's a pane, so findings get labeled + const f = geometryFindings("panes", panes); + expect(Array.isArray(f)).toBe(true); }); - it("a rank-aligned swimlane (the shipped pattern) lints clean", () => { - // Two lanes, members aligned by flow rank; cross-lane edges are short and perpendicular. - const clean = { - lanes: true, - direction: "LR", - groups: [{ id: "user" }, { id: "svc" }], - nodes: [ - { id: "u1", group: "user", data: { label: "Start" } }, - { id: "s1", group: "svc", data: { label: "Handle" } }, - { id: "u2", group: "user", data: { label: "Done" } }, - ], - edges: [ - { source: "u1", target: "s1" }, - { source: "s1", target: "u2" }, - ], - }; - expect(analyzeFlowGeometry(clean).edgeOverNode).toBe(0); + it("only lints flow content", () => { + expect(geometryWarnings("markdown", "# hi")).toEqual([]); + expect(geometryWarnings("flow", "not json")).toEqual([]); }); }); -// VALIDATION: every shipped recipe example must be geometry-clean — no edge running over a node, -// no overlapping nodes, no dangling edge refs. (Advisory edge–edge *crossings* are allowed.) This -// guards the whole gallery: an example like the old swimlane/architecture/data-lineage that drew -// arrows over nodes now fails CI here instead of shipping. -describe("shipped examples are geometry-clean (no arrows over nodes)", () => { +// VALIDATION: every shipped recipe example must be geometry-clean. +describe("shipped examples are geometry-clean", () => { const files = readdirSync(EXAMPLES); - const isBad = (w: string) => /run across unrelated|node\(s\) overlap|reference a missing node/.test(w); + // SGCR invariant violations for ungrouped flows; sgcr-overflow check for all flows for (const f of files.filter((f) => f.endsWith(".flow.json"))) { it(`${f}`, () => { - const r = analyzeFlowGeometry(JSON.parse(readFileSync(join(EXAMPLES, f), "utf8"))); - expect({ file: f, edgeOverNode: r.edgeOverNode, nodeOverlaps: r.nodeOverlaps, missingRefs: r.missingRefs }) - .toEqual({ file: f, edgeOverNode: 0, nodeOverlaps: 0, missingRefs: [] }); + const content = readFileSync(join(EXAMPLES, f), "utf8"); + const r = geometryReport("flow", content); + const errors = r.findings.filter((x) => x.severity === "error"); + expect({ file: f, errors: errors.map((e) => e.message) }).toEqual({ file: f, errors: [] }); }); } for (const f of files.filter((f) => f.endsWith(".panes.json"))) { it(`${f} (flow panes)`, () => { - const bad = geometryWarnings("panes", readFileSync(join(EXAMPLES, f), "utf8")).filter(isBad); - expect({ file: f, bad }).toEqual({ file: f, bad: [] }); - }); - } - - // Calibration guard: the stricter checks (edge-near-node, crossings≥3, direction hint) must NOT - // nag the curated gallery. Every shipped flow example must produce ZERO geometry warnings — if a - // new check starts flagging a clean example, either the check is too aggressive or the example - // needs fixing. Keeps the lint trustworthy (no false positives the agent learns to ignore). - for (const f of files.filter((f) => f.endsWith(".flow.json"))) { - it(`${f} produces no geometry warnings`, () => { - const w = analyzeFlowGeometry(JSON.parse(readFileSync(join(EXAMPLES, f), "utf8"))).warnings; - expect({ file: f, warnings: w }).toEqual({ file: f, warnings: [] }); + const r = geometryReport("panes", readFileSync(join(EXAMPLES, f), "utf8")); + const errors = r.findings.filter((x) => x.severity === "error"); + expect({ file: f, errors: errors.map((e) => e.message) }).toEqual({ file: f, errors: [] }); }); } - // Readability calibration: no shipped flow example may be flagged `low-readability` (which fires - // only when a graph is so large it spans >MAX_READABLE_OVERFLOW screens even at the readable floor). - // The curated gallery sets the size bar — if an example trips this, either it's genuinely too dense - // (shrink/split it) or the threshold is mis-calibrated. for (const f of files.filter((f) => f.endsWith(".flow.json"))) { it(`${f} is not flagged low-readability`, () => { const lr = geometryReport("flow", readFileSync(join(EXAMPLES, f), "utf8")).findings @@ -301,17 +125,13 @@ describe("shipped examples are geometry-clean (no arrows over nodes)", () => { } }); -// PANES DENSITY ("readability") lint: a structurally-valid view can still render unreadably small -// when too much heavy content is packed into one layout (the screwbits trigger case — `rows` with 5 -// panes, 4 heavy). These are warn-only density findings, folded into geometryReport so the server + -// CLI surface them via the same findings channel as the flow-geometry checks. +// PANES DENSITY ("readability") lint describe("panes density readability", () => { - // Per-pane "weight" ≈ readable blocks of vertical room; weight ≥ 2 is "heavy". const flow = (n: number) => JSON.stringify({ nodes: Array.from({ length: n }, (_, i) => ({ id: `n${i}`, data: { label: `N${i}` } })), edges: [] }); const vegalite = JSON.stringify({ mark: "bar", data: { values: [] } }); - const heavyComponent = JSON.stringify({ type: "Table" }); // Table → weight 2 - const lightComponent = JSON.stringify({ type: "Text", props: { children: "hi" } }); // weight 1 + const heavyComponent = JSON.stringify({ type: "Table" }); + const lightComponent = JSON.stringify({ type: "Text", props: { children: "hi" } }); const markdown = "# short note"; const pane = (type: string, content: string, title?: string) => ({ type, content, ...(title ? { title } : {}) }); const panes = (layout: string, ps: object[]) => JSON.stringify({ layout, panes: ps }); @@ -394,10 +214,7 @@ describe("panes density readability", () => { expect(geometryFindings("text", "x".repeat(9000))).toEqual([]); }); - // Calibration guard: the curated gallery sets the density bar — no shipped panes example may trip a - // density finding. screwbits (rows, 5 panes / 4 heavy) is the agreed over-stuffed case; the gallery's - // densest legit views (c4-architecture rows wt9/heavy3, the explainers rows wt7/heavy3) must stay - // clean. If a new example trips this, either it's genuinely over-stuffed or the threshold drifted. + // Calibration guard: the curated gallery sets the density bar. const DENSITY = new Set(["rows-overstuffed", "columns-too-many", "grid-too-many", "panes-nested", "flow-too-many-nodes"]); for (const f of readdirSync(EXAMPLES).filter((f) => f.endsWith(".panes.json"))) { it(`${f} produces no density warnings`, () => { diff --git a/packages/viewer/test/readable-layout.test.ts b/packages/viewer/test/readable-layout.test.ts index dd939d0d..2477fc7c 100644 --- a/packages/viewer/test/readable-layout.test.ts +++ b/packages/viewer/test/readable-layout.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import { smallestNodeFontPx, readableFloorZoom, anchoredViewport, layoutFlow, FONT, MIN_READABLE_PX, type FlowSpec } from "../src/client/renderers/flow-layout.js"; -import { bestLayout, geometryReport } from "../src/flow-geometry.js"; +import { geometryReport } from "../src/flow-geometry.js"; // A simple A→B→…→N chain with one node per id. `extra` decorates the FIRST node so we can probe // how the smallest-font detection reacts to kind/sub/meta. @@ -49,43 +49,6 @@ describe("readableFloorZoom — keep the smallest label ≥ MIN_READABLE_PX", () }); }); -describe("bestLayout — capped, deterministic, geometry-gated readability search", () => { - const wide = chain(8); // unpinned: the engine may pick TB or LR - - it("is deterministic (same direction across repeated calls)", () => { - const a = bestLayout(wide), b = bestLayout(wide), c = bestLayout(wide); - expect(a.direction).toBe(b.direction); - expect(b.direction).toBe(c.direction); - }); - - it("never exceeds MAX_LAYOUT_ATTEMPTS", () => { - expect(bestLayout(wide).attempts).toBeLessThanOrEqual(4); - }); - - it("is never less readable than the authored default (search only improves)", () => { - // The unpinned search considers both default-spacing directions (base + flip) plus tighter- - // compaction variants, so its pick is at least as readable as either default-direction base. - const tb = bestLayout({ ...wide, direction: "TB" }); // pinned → that direction's base, default opts - const lr = bestLayout({ ...wide, direction: "LR" }); - const auto = bestLayout(wide); - expect(auto.effectivePx).toBeGreaterThanOrEqual(Math.max(tb.effectivePx, lr.effectivePx) - 0.01); - expect(["TB", "LR"]).toContain(auto.direction); - }); - - it("respects a pinned direction (does not flip)", () => { - expect(bestLayout({ ...wide, direction: "TB" }).direction).toBe("TB"); - expect(bestLayout({ ...wide, direction: "LR" }).direction).toBe("LR"); - expect(bestLayout({ ...wide, direction: "LR" }).attempts).toBe(1); - }); - - it("never adopts a variant with more edge-over-node / overlap than the base", () => { - const base = bestLayout({ ...wide, direction: "TB" }); // default base direction is TB - const auto = bestLayout(wide); - expect(auto.report.edgeOverNode).toBeLessThanOrEqual(base.report.edgeOverNode); - expect(auto.report.nodeOverlaps).toBeLessThanOrEqual(base.report.nodeOverlaps); - }); -}); - describe("anchoredViewport — open at the start/root, not the middle, when overflowing", () => { const W = 1200, H = 800, z = 1, pad = 20; const tall = { x: 0, y: 0, width: 400, height: 2000 }; @@ -154,19 +117,16 @@ describe("layoutTiers — vertical zone bands, horizontal content", () => { }); describe("large-graph overflow finding", () => { - // Phase 6: ungrouped flows now take the SGCR lint path. A huge ungrouped chain - // produces `sgcr-overflow` instead of `low-readability` (which was dagre-path-specific). - // Grouped large flows still produce `low-readability` via the dagre path. it("fires sgcr-overflow for a huge ungrouped graph (SGCR default path)", () => { const huge = geometryReport("flow", JSON.stringify(chain(60))); const ov = huge.findings.find((f) => f.code === "sgcr-overflow"); expect(ov).toBeTruthy(); expect(ov!.severity).toBe("warning"); }); - it("fires low-readability for a huge dagre-opt-out graph", () => { + it("engine:'dagre' is silently ignored — still uses SGCR", () => { const huge = geometryReport("flow", JSON.stringify({ ...chain(60), engine: "dagre" })); - const lr = huge.findings.find((f) => f.code === "low-readability"); - expect(lr).toBeTruthy(); + const ov = huge.findings.find((f) => f.code === "sgcr-overflow"); + expect(ov).toBeTruthy(); }); it("does not fire for a normal small graph", () => { const ok = geometryReport("flow", JSON.stringify(chain(5))); @@ -174,32 +134,27 @@ describe("large-graph overflow finding", () => { }); }); -describe("SGCR lint integration (Phase 6)", () => { - // Phase 6: SGCR is now the default for ungrouped flows. The lint uses checkInvariants() - // (exact arithmetic) instead of the heuristic dagre-based segment checks. Grouped flows - // still use the dagre lint (they render through FlowInner with smoothstep edges). +describe("SGCR lint — all flows use SGCR", () => { it("ungrouped flows use the SGCR lint path (no dagre edge-over-node / crossings findings)", () => { const r = geometryReport("flow", JSON.stringify(chain(3))); - // SGCR guarantees zero edge-over-node by construction — the lint should never produce them expect(r.findings.some((f) => f.code === "edge-over-node" || f.code === "crossings")).toBe(false); }); - it("engine:'dagre' opt-out uses the dagre lint path", () => { + it("engine:'dagre' is silently ignored — SGCR lint runs", () => { const r = geometryReport("flow", JSON.stringify({ ...chain(3), engine: "dagre" })); - // dagre path may or may not have findings — just verify it doesn't crash expect(Array.isArray(r.findings)).toBe(true); + expect(r.findings.some((f) => f.code === "edge-over-node" || f.code === "crossings")).toBe(false); }); - it("grouped flows use the dagre lint path (inter-zone edges are smoothstep)", () => { + it("grouped flows lint through SGCR (skips invariant checks for grouped pipeline)", () => { const grouped = { - engine: "sgcr", // even if explicitly requested, grouped → dagre lint groups: [{ id: "a" }, { id: "b" }], nodes: [{ id: "a1", group: "a", data: { label: "A1" } }, { id: "b1", group: "b", data: { label: "B1" } }], edges: [{ source: "a1", target: "b1" }], }; const r = geometryReport("flow", JSON.stringify(grouped)); - // Grouped flows go through the dagre lint path — no sgcr-skipped-grouped warning (Phase 3 removed it) expect(r.findings.some((f) => f.code === "sgcr-skipped-grouped")).toBe(false); + expect(Array.isArray(r.findings)).toBe(true); }); - it("skips the dagre lint for ungrouped flows (SGCR path by default)", () => { + it("all flows use SGCR (explicit engine:'sgcr' or default)", () => { const r = geometryReport("flow", JSON.stringify({ ...chain(3), engine: "sgcr" })); expect(r.findings.some((f) => f.code === "edge-over-node" || f.code === "crossings")).toBe(false); }); diff --git a/packages/viewer/test/server.test.ts b/packages/viewer/test/server.test.ts index 7eb0f1e7..d13e797d 100644 --- a/packages/viewer/test/server.test.ts +++ b/packages/viewer/test/server.test.ts @@ -154,23 +154,21 @@ describe("viewer server", () => { }); it("flow geometry is advisory by default (200 + severity findings); x-termchart-strict rejects (422, not stored)", async () => { - // a→c runs over b (collinear, hand-placed) → an edge-over-node = error-severity finding. - const overlapping = JSON.stringify({ - layout: "manual", direction: "LR", - nodes: [{ id: "a", position: { x: 0, y: 0 } }, { id: "b", position: { x: 200, y: 0 } }, { id: "c", position: { x: 440, y: 0 } }], - edges: [{ source: "a", target: "c" }], - }); + // A large flow (35 nodes) triggers flow-too-many-nodes (warning severity). + const nodes = Array.from({ length: 35 }, (_, i) => ({ id: `n${i}`, data: { label: `N${i}` } })); + const edges = nodes.slice(1).map((n, i) => ({ source: `n${i}`, target: n.id })); + const largeFlow = JSON.stringify({ nodes, edges }); // default: stored, 200 with structured findings + maxSeverity - const r = await push({ project: "g", agent: "a", type: "flow", content: overlapping }); + const r = await push({ project: "g", agent: "a", type: "flow", content: largeFlow }); expect(r.status).toBe(200); const body = (await r.json()) as { maxSeverity: string; findings: { code: string; severity: string }[] }; - expect(body.maxSeverity).toBe("error"); - expect(body.findings.some((f) => f.code === "edge-over-node" && f.severity === "error")).toBe(true); - // strict: rejected (422) and NOT stored + expect(body.maxSeverity).toBe("warning"); + expect(body.findings.some((f) => f.code === "flow-too-many-nodes" && f.severity === "warning")).toBe(true); + // strict at "warning": rejected (422) and NOT stored const r2 = await fetch(`${base}/w/ws1/push`, { method: "POST", - headers: { "content-type": "application/json", authorization: `Bearer ${TOKEN}`, "x-termchart-strict": "error" }, - body: JSON.stringify({ project: "g2", agent: "a", type: "flow", content: overlapping, description: "strict test" }), + headers: { "content-type": "application/json", authorization: `Bearer ${TOKEN}`, "x-termchart-strict": "warning" }, + body: JSON.stringify({ project: "g2", agent: "a", type: "flow", content: largeFlow, description: "strict test" }), }); expect(r2.status).toBe(422); expect((await r2.json()).rejected).toBe(true); diff --git a/packages/viewer/test/sgcr.test.ts b/packages/viewer/test/sgcr.test.ts index d3d0f5f4..99808d41 100644 --- a/packages/viewer/test/sgcr.test.ts +++ b/packages/viewer/test/sgcr.test.ts @@ -185,6 +185,132 @@ describe("SGCR layout — octilinear (diagonal) routing", () => { }); }); +describe("grouped SGCR layout", () => { + it("simple 2-zone graph produces containers and positioned nodes", () => { + const input: SGCRInput = { + direction: "TB", + groups: [{ id: "a", label: "Zone A" }, { id: "b", label: "Zone B" }], + nodes: [ + { id: "n1", group: "a" }, { id: "n2", group: "a" }, + { id: "n3", group: "b" }, { id: "n4", group: "b" }, + ], + edges: [{ source: "n1", target: "n2" }, { source: "n2", target: "n3" }, { source: "n3", target: "n4" }], + }; + const lay = layoutSGCR(input); + expect(lay.containers).toBeDefined(); + expect(lay.containers!.length).toBe(2); + expect(lay.nodes.length).toBe(4); + expect(lay.edges.length).toBe(3); + }); + + it("zone containers don't overlap", () => { + const input: SGCRInput = { + direction: "TB", + groups: [{ id: "a" }, { id: "b" }, { id: "c" }], + nodes: [ + { id: "n1", group: "a" }, { id: "n2", group: "b" }, { id: "n3", group: "c" }, + ], + edges: [{ source: "n1", target: "n2" }, { source: "n2", target: "n3" }], + }; + const lay = layoutSGCR(input); + const cs = lay.containers!; + for (let i = 0; i < cs.length; i++) + for (let j = i + 1; j < cs.length; j++) { + const a = cs[i], b = cs[j]; + const overlap = a.x < b.x + b.width && a.x + a.width > b.x && a.y < b.y + b.height && a.y + a.height > b.y; + expect(overlap, `containers ${a.id} and ${b.id} overlap`).toBe(false); + } + }); + + it("every member node is WITHIN its zone container", () => { + const input: SGCRInput = { + direction: "TB", + groups: [{ id: "a" }, { id: "b" }], + nodes: [ + { id: "n1", group: "a" }, { id: "n2", group: "a" }, + { id: "n3", group: "b" }, + ], + edges: [{ source: "n1", target: "n2" }, { source: "n2", target: "n3" }], + }; + const lay = layoutSGCR(input); + const cMap = new Map(lay.containers!.map(c => [c.id, c])); + const grpOf = new Map(input.nodes.map(n => [n.id, n.group!])); + for (const n of lay.nodes) { + const c = cMap.get(grpOf.get(n.id)!); + if (!c) continue; + expect(n.x >= c.x, `node ${n.id} x >= container x`).toBe(true); + expect(n.y >= c.y, `node ${n.id} y >= container y`).toBe(true); + expect(n.x + n.width <= c.x + c.width + 1, `node ${n.id} right <= container right`).toBe(true); + expect(n.y + n.height <= c.y + c.height + 1, `node ${n.id} bottom <= container bottom`).toBe(true); + } + }); + + it("single-node groups produce one container each", () => { + const input: SGCRInput = { + direction: "TB", + groups: [{ id: "a" }, { id: "b" }], + nodes: [{ id: "n1", group: "a" }, { id: "n2", group: "b" }], + edges: [{ source: "n1", target: "n2" }], + }; + const lay = layoutSGCR(input); + expect(lay.containers!.length).toBe(2); + expect(lay.nodes.length).toBe(2); + }); + + it("no edges between groups still lays out correctly", () => { + const input: SGCRInput = { + direction: "TB", + nodes: [ + { id: "n1", group: "a" }, { id: "n2", group: "a" }, + { id: "n3", group: "b" }, + ], + edges: [{ source: "n1", target: "n2" }], + }; + const lay = layoutSGCR(input); + expect(lay.containers!.length).toBe(2); + expect(lay.nodes.length).toBe(3); + }); + + it("cycle within a group produces valid layout", () => { + const input: SGCRInput = { + direction: "TB", + nodes: [ + { id: "n1", group: "a" }, { id: "n2", group: "a" }, { id: "n3", group: "a" }, + ], + edges: [{ source: "n1", target: "n2" }, { source: "n2", target: "n3" }, { source: "n3", target: "n1" }], + }; + const lay = layoutSGCR(input); + expect(lay.containers!.length).toBe(1); + expect(lay.nodes.length).toBe(3); + expect(lay.edges.length).toBe(3); + }); + + it("ungrouped graph still passes full invariants", () => { + const input: SGCRInput = { + direction: "TB", + nodes: [{ id: "a" }, { id: "b" }, { id: "c" }], + edges: [{ source: "a", target: "b" }, { source: "b", target: "c" }], + }; + const lay = layoutSGCR(input); + expect(checkInvariants(lay).ok).toBe(true); + expect(lay.containers).toBeUndefined(); + }); + + it("mixed grouped + ungrouped nodes", () => { + const input: SGCRInput = { + direction: "TB", + nodes: [ + { id: "n1", group: "a" }, { id: "n2", group: "a" }, + { id: "n3" }, + ], + edges: [{ source: "n1", target: "n2" }, { source: "n2", target: "n3" }], + }; + const lay = layoutSGCR(input); + expect(lay.containers!.length).toBe(1); + expect(lay.nodes.length).toBe(3); + }); +}); + describe("SGCR layout — ring layout for cycles", () => { it("draws a simple cycle as a clean ring (every size 3–16)", () => { for (let N = 3; N <= 16; N++) { From c72ee2afc2e58760369052f8dd4f5cc3d462f9ea Mon Sep 17 00:00:00 2001 From: Ivan Cheung Date: Sun, 9 Aug 2026 09:43:25 +0000 Subject: [PATCH 2/3] fix(flow): enable measured re-pass for grouped flows + raise estSize floor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Grouped flows (tiers/lanes/zones) were skipped by the measured re-pass, relying entirely on estSize estimates which under-predicted ChangeNode's rendered width (estSize minW=120 vs CSS minWidth=140 + cell fill). This caused node overlaps in the architecture-zones tiered layout. Two fixes: - Raise estSize minW from 120 to 172 (matches the ChangeNode's rendered floor when it fills its SGCR cell via width:100%) - Enable the measured re-pass for grouped flows: re-run layoutFlow with DOM-measured node sizes, updating both member positions and container styles. Previously skipped because "dagre would ignore zones" — now layoutFlow handles groups natively via SGCR. --- .../src/client/renderers/flow-layout.ts | 7 +++-- packages/viewer/src/client/renderers/flow.tsx | 31 ++++++++++++------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/packages/viewer/src/client/renderers/flow-layout.ts b/packages/viewer/src/client/renderers/flow-layout.ts index 2a1c1ac8..8e0cefa7 100644 --- a/packages/viewer/src/client/renderers/flow-layout.ts +++ b/packages/viewer/src/client/renderers/flow-layout.ts @@ -180,9 +180,10 @@ export function estSize(n: FlowNode): { width: number; height: number } { // An inline sparkline (data.spark) adds a row of trend under the label and needs a little // width to read; reserve ~24px height and a 150px floor so dagre doesn't pack nodes tight. const hasSpark = Array.isArray(d.spark) && (d.spark as unknown[]).length >= 2; - // Phase 5: lowered from 150 to 120. A 5-char label ("Redis") at 7.6px/ch + 40px padding = 78px; - // the old 150px floor added 72px of empty space. 120px still gives comfortable padding. - let minW = 120; + // ChangeNode's CSS minWidth is 140px, but the card stretches to fill its SGCR cell (width:100% + // on TB/BT). The cell is at least nodeGap-padded, so the rendered card is typically ~172px for + // short labels. Match this so the estimated layout doesn't under-space and cause overlaps. + let minW = 172; if (hasSpark) minW = Math.max(minW, 174); let height = d.sub ? 64 : 46; if (hasSpark) height += 24; diff --git a/packages/viewer/src/client/renderers/flow.tsx b/packages/viewer/src/client/renderers/flow.tsx index a5914c95..404ff3ce 100644 --- a/packages/viewer/src/client/renderers/flow.tsx +++ b/packages/viewer/src/client/renderers/flow.tsx @@ -140,24 +140,31 @@ function FlowInner({ spec, handle }: { spec: FlowSpec; handle?: PatchHandle }) { }, [handle, setNodes]); useEffect(() => { - if (!autoLayout || grouped || !inited || relaidOut.current) return; + if (!autoLayout || !inited || relaidOut.current) return; relaidOut.current = true; - const measuredSpec: FlowSpec = { - ...spec, - direction: dir, - nodes: nodes.map((n) => ({ - ...(spec.nodes ?? []).find((sn) => sn.id === n.id) ?? { id: n.id }, - width: n.measured?.width ?? 172, - height: n.measured?.height ?? 44, - })), - }; + const measuredNodes = nodes + .filter((n) => n.type !== "group") + .map((n) => { + const orig = (spec.nodes ?? []).find((sn) => sn.id === n.id); + return { + ...(orig ?? { id: n.id }), + id: n.id, + width: n.measured?.width ?? 172, + height: n.measured?.height ?? 44, + }; + }); + const measuredSpec: FlowSpec = { ...spec, direction: dir, nodes: measuredNodes }; const re = layoutFlow(measuredSpec); setNodes((nds) => nds.map((n) => { const rn = re.nodes.find((r) => r.id === n.id); - return rn?.position ? { ...n, position: rn.position } : n; + if (!rn?.position) return n; + const update: Record = { ...n, position: rn.position }; + if (rn.style) update.style = rn.style; + if (rn.parentId) update.parentId = rn.parentId; + return update as typeof n; })); requestAnimationFrame(anchorFit); - }, [autoLayout, grouped, inited, nodes, edges, anchorFit, setNodes, dir, spec]); + }, [autoLayout, inited, nodes, edges, anchorFit, setNodes, dir, spec]); // Re-fit when the container resizes (e.g. iPad rotation, sidebar collapse, maximize). useEffect(() => { From 4623ad660929306242d1c96b3610e95b2cf0d2ac Mon Sep 17 00:00:00 2001 From: Ivan Cheung Date: Wed, 12 Aug 2026 14:55:09 +0800 Subject: [PATCH 3/3] fix(flow): republish edge routes in the measured re-pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The measured re-pass calls setNodes with the recomputed positions but never setEdges, and for grouped flows the layout hands back EXPLICIT edge polylines in data.points. So the nodes moved to their measured-size arrangement while every edge kept the geometry computed from estimated sizes. The old dagre re-pass returned positions only and let React Flow derive each path from the live node positions, which is why this never bit before: this PR switched the re-pass to a layout that owns its edge routes without giving the component a way to publish them. The setEdges slot was literally discarded at the useEdgesState call. Also carry the edge `type`, since an edge the first pass left un-routed only draws its polyline once it is switched to the "sgcr" edge component. This removes the ex_infra-topology regression, and ex_architecture-zones and syn_groups_long come out clean too — but syn_swimlane then breaks in their place, because the cross-zone router is a fixed 4-point dogleg with no obstacle avoidance. Which board loses is incidental geometry, so this is a real fix for a real bug but not sufficient on its own. See the review comment on the PR. --- packages/viewer/src/client/renderers/flow.tsx | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/viewer/src/client/renderers/flow.tsx b/packages/viewer/src/client/renderers/flow.tsx index 404ff3ce..6a21755f 100644 --- a/packages/viewer/src/client/renderers/flow.tsx +++ b/packages/viewer/src/client/renderers/flow.tsx @@ -88,7 +88,7 @@ function FlowInner({ spec, handle }: { spec: FlowSpec; handle?: PatchHandle }) { }, ) as unknown as Node[], ); - const [edges, , onEdgesChange] = useEdgesState(initial.edges as unknown as Edge[]); + const [edges, setEdges, onEdgesChange] = useEdgesState(initial.edges as unknown as Edge[]); const inited = useNodesInitialized(); const { fitView, getNodes, getNodesBounds, getViewport, setViewport } = useReactFlow(); const relaidOut = useRef(false); @@ -163,8 +163,20 @@ function FlowInner({ spec, handle }: { spec: FlowSpec; handle?: PatchHandle }) { if (rn.parentId) update.parentId = rn.parentId; return update as typeof n; })); + // Grouped layouts hand back EXPLICIT edge polylines, so moving the nodes without republishing + // the routes leaves every edge drawn against the first pass's estimated-size geometry — nodes + // in one arrangement, edges in another. (The old dagre re-pass returned positions only and let + // React Flow derive each path from the live node positions, so it never had this failure mode.) + const reRouted = new Map(re.edges.filter((r) => r.id).map((r) => [r.id as string, r])); + setEdges((eds) => eds.map((e) => { + const r = reRouted.get(e.id) as { type?: string; data?: Record } | undefined; + if (!r?.data?.points) return e; + // Carry the type too: an edge the first pass left un-routed only renders its polyline once + // it's switched to the "sgcr" edge component. + return { ...e, type: r.type ?? e.type, data: { ...(e.data ?? {}), ...r.data } }; + })); requestAnimationFrame(anchorFit); - }, [autoLayout, inited, nodes, edges, anchorFit, setNodes, dir, spec]); + }, [autoLayout, inited, nodes, edges, anchorFit, setNodes, setEdges, dir, spec]); // Re-fit when the container resizes (e.g. iPad rotation, sidebar collapse, maximize). useEffect(() => {