Skip to content

fix(flow): tiered layout — wrap rows + per-band width (grouped density) #299

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ivanmkc merged 2 commits into from
Aug 8, 2026
+138 −101
Merged

fix(flow): tiered layout — wrap rows + per-band width (grouped density) #299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcb4dc6
feat(flow): lint uses SGCR checkInvariants for ungrouped flows (Phase 6)
ivanmkc-google Aug 8, 2026
070eded
fix(flow): tiered layout — wrap rows + per-band width (grouped densit…
ivanmkc-google Aug 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions packages/viewer/src/client/renderers/flow-layout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,14 +524,33 @@ function layoutTiers(spec: FlowSpec, nodes: FlowNode[], edges: FlowEdge[]): { no
const { rel: pos } = layoutCluster(ms, edges, sizeOf, "LR");
const idx = new Map(ms.map((m, i) => [m.id, i] as const));
const sorted = ms.slice().sort((a, b) => (pos[a.id].x - pos[b.id].x) || (idx.get(a.id)! - idx.get(b.id)!));
// Wrap members into multiple rows when the single-row width would exceed a readable
// threshold (~900px content). This prevents 9-node bands from producing an infinitely-wide
// row that overflows the viewport. Each row gets its own y offset.
const MAX_ROW_W = 900;
const place: Record<string, { x: number; y: number }> = {};
let cx = 0, h = 0;
for (const m of sorted) { const sz = sizeOf.get(m.id)!; place[m.id] = { x: cx, y: 0 }; cx += sz.width + ROW_GAP; h = Math.max(h, sz.height); }
laid.set(key, { place, w: Math.max(0, cx - ROW_GAP), h });
let cx = 0, rowY = 0, rowH = 0, bandW = 0;
for (const m of sorted) {
const sz = sizeOf.get(m.id)!;
if (cx > 0 && cx + sz.width > MAX_ROW_W) {
// wrap to next row
bandW = Math.max(bandW, cx - ROW_GAP);
rowY += rowH + ROW_GAP;
cx = 0; rowH = 0;
}
place[m.id] = { x: cx, y: rowY };
cx += sz.width + ROW_GAP;
rowH = Math.max(rowH, sz.height);
}
bandW = Math.max(bandW, cx > 0 ? cx - ROW_GAP : 0);
const h = rowY + rowH;
laid.set(key, { place, w: bandW, h });
}

// Stack bands vertically; all bands share the widest row's width so they line up.
const sharedW = Math.max(0, ...laneKeys.map((k) => laid.get(k)!.w)) + GROUP_PAD * 2;
// Stack bands vertically. Each band uses its own content width (not the widest).
// This prevents a 1-node band from being stretched to the width of a 9-node band.
// Cross-band edges still route correctly because they connect to specific nodes,
// not to the band container edges.
const containers: FlowNode[] = [];
const childMembers: FlowNode[] = [];
let cursor = 0;
Expand All @@ -542,7 +561,7 @@ function layoutTiers(spec: FlowSpec, nodes: FlowNode[], edges: FlowEdge[]): { no
containers.push({
id: tkey(key), type: "group", position: { x: 0, y: cursor },
data: { label: m.label, color: m.color },
style: { width: sharedW, height: bandH },
style: { width: it.w + GROUP_PAD * 2, height: bandH },
selectable: false, draggable: false, zIndex: 0,
} as FlowNode);
for (const mn of members.get(key)!) {
Expand Down
144 changes: 78 additions & 66 deletions packages/viewer/src/flow-geometry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
type FlowSpec, type DagreOpts,
} 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 { paneContentDensityFindings, panesGridPaneWidthAt, standaloneDensityFindings } from "./element-density.js";

Expand Down Expand Up @@ -572,68 +573,75 @@ function panesDensityFindings(panes: unknown[], layoutRaw: unknown): Finding[] {
return out;
}

// `engine:"sgcr"` only engages on ungrouped graphs (flow.tsx mount silently picks dagre otherwise),
// and only after we've checked the SGCR-computed canvas isn't so much bigger than the viewport that
// the layout spills off-screen. Both are warnings, not errors: the renderer still produces a useful
// view, but the agent should know.
const SGCR_VIEWPORT_OVERFLOW = 1.6; // ratio of SGCR canvas vs available viewport before we warn
const SGCR_MAX_LINT_NODES = 200; // skip the lint layout for pathological inputs
function sgcrReport(v: unknown): { warnings: string[]; findings: Finding[] } {
const findings: Finding[] = [];
const s = (v && typeof v === "object" ? v : {}) as {
nodes?: unknown[]; edges?: unknown[]; direction?: unknown;
};
// 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 grouped = nodes.some((n) => n && typeof n === "object" &&
(typeof (n as { group?: unknown }).group === "string" ||
typeof ((n as { data?: { group?: unknown } }).data)?.group === "string"));
if (grouped) {
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<string, unknown> : 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: "sgcr-skipped-grouped", count: 1,
message: `\`engine:"sgcr"\` was requested but this graph is grouped (zones/lanes/tiers) — the SGCR engine is ungrouped-only today, so the viewer silently uses the dagre layout instead. Drop the grouping to engage SGCR, or remove \`engine:"sgcr"\` to suppress this warning.`,
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.`,
});
} else if (Array.isArray(s.edges) && s.edges.length && nodes.length && nodes.length <= SGCR_MAX_LINT_NODES) {
// Compute the SGCR layout the renderer will use, then check it fits the reference viewport.
// SGCR proves geometry relative to ITS OWN computed canvas, which can be far bigger than the
// screen — large graphs spill off the edges even though check.ts is happy (issue #195).
try {
const input: SGCRInput = {
nodes: nodes.filter((n) => n && typeof n === "object" && typeof (n as { id?: unknown }).id !== "undefined")
.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<string, unknown> : undefined,
};
}),
edges: (s.edges as unknown[]).filter((e) => e && typeof e === "object" &&
typeof (e as { source?: unknown }).source !== "undefined" &&
typeof (e as { target?: unknown }).target !== "undefined")
.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,
};
const opts: SGCROptions = {};
const lay = layoutSGCR(input, opts);
const avail = { w: REF_VIEWPORT.w * (1 - FIT_PADDING * 2), h: REF_VIEWPORT.h * (1 - FIT_PADDING * 2) };
const wOver = lay.width / avail.w;
const 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 (the renderer falls back); skip silently.
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 };
}
Expand All @@ -654,14 +662,18 @@ export function geometryReport(type: string, content: string, depth = 0): { warn
return { warnings: [], findings: [] };
}
if (type === "flow") {
// engine:"sgcr" lays out with the provable Slotted Grid engine (edge-over-node / node overlap /
// arrowhead stacking are impossible by construction) and its geometry differs from dagre — the
// dagre-based lint below would emit findings that don't reflect what actually renders (e.g. an
// "edge over node" / "crossings" warning for a graph the engine draws cleanly). Skip the dagre
// lint but emit SGCR-specific findings: grouped specs silently fall back to dagre at render time
// (sgcr-skipped-grouped), and oversized SGCR canvases spill the viewport (sgcr-overflow).
if (v && typeof v === "object" && (v as { engine?: unknown }).engine === "sgcr")
return sgcrReport(v);
// 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
Expand Down
64 changes: 35 additions & 29 deletions packages/viewer/test/readable-layout.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,47 +153,53 @@ describe("layoutTiers — vertical zone bands, horizontal content", () => {
});
});

describe("low-readability finding", () => {
it("fires for a graph too large to read even at the floor", () => {
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", () => {
const huge = geometryReport("flow", JSON.stringify({ ...chain(60), engine: "dagre" }));
const lr = huge.findings.find((f) => f.code === "low-readability");
expect(lr).toBeTruthy();
expect(lr!.severity).toBe("warning");
});
it("does not fire for a normal small graph", () => {
const ok = geometryReport("flow", JSON.stringify(chain(5)));
expect(ok.findings.some((f) => f.code === "low-readability")).toBe(false);
expect(ok.findings.some((f) => f.code === "low-readability" || f.code === "sgcr-overflow")).toBe(false);
});
});

describe("sgcr-skipped-grouped finding", () => {
// A grouped spec with engine:"sgcr" — viewer silently falls back to dagre (the SGCR engine is
// ungrouped-only today). Without this lint, the agent had no way to know SGCR didn't engage.
const grouped: FlowSpec & { engine: string } = {
engine: "sgcr",
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" }],
};
it("fires for engine:'sgcr' + grouped graph", () => {
const r = geometryReport("flow", JSON.stringify(grouped));
const f = r.findings.find((x) => x.code === "sgcr-skipped-grouped");
expect(f).toBeTruthy();
expect(f!.severity).toBe("warning");
});
it("does not fire for an ungrouped sgcr graph", () => {
const ungrouped = { ...chain(3), engine: "sgcr" };
const r = geometryReport("flow", JSON.stringify(ungrouped));
expect(r.findings.some((f) => f.code === "sgcr-skipped-grouped")).toBe(false);
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).
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("does not fire when engine is not sgcr", () => {
const r = geometryReport("flow", JSON.stringify({ ...grouped, engine: "dagre" }));
it("engine:'dagre' opt-out uses the dagre lint path", () => {
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);
});
it("grouped flows use the dagre lint path (inter-zone edges are smoothstep)", () => {
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);
});
it("skips the dagre lint for engine:'sgcr' (no edge-over-node / crossings from the dagre path)", () => {
it("skips the dagre lint for ungrouped flows (SGCR path by 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);
});
Expand Down
Loading