diff --git a/docs/superpowers/specs/2026-06-08-flow-readability-lint-design.md b/docs/superpowers/specs/2026-06-08-flow-readability-lint-design.md new file mode 100644 index 0000000..e65a777 --- /dev/null +++ b/docs/superpowers/specs/2026-06-08-flow-readability-lint-design.md @@ -0,0 +1,205 @@ +# Push-time readability lint ("flow validation") + +**Date:** 2026-06-08 +**Status:** Implemented 2026-06-15 — **adapted** from the design below (see "Implementation note"). + +## Implementation note (2026-06-15) + +Between approval and implementation the codebase grew a severity-tagged **geometry-findings** +system (`packages/viewer/src/flow-geometry.ts` → `geometryReport`; `Finding{severity,code,message}`) +that already provides *everything this spec proposed as new plumbing*: the server returns +`200 + {warnings, findings}` on a non-clean push (`204` when clean, `422` under +`x-termchart-strict`), and the CLI already prints findings to stderr. `validateContent` also moved +to `@ivanmkc/termchart-core`. + +So the design's **goal** (a static, push-time, warn-only density lint covering every push path) +was kept, but its **architecture** was adapted to that reality — which also honors this spec's own +stated principle ("single source of truth … no duplicated lint logic across packages"): + +- **No** new `packages/viewer/src/readability.ts` module — the density checks are folded into + `geometryReport` and emitted as `Finding`s. +- **No** `server.ts` change — it already calls `geometryReport` and returns the findings. +- **No** `push.ts` change — the CLI already surfaces findings (`printGeometry`). +- New finding `code`s: `rows-overstuffed`, `columns-too-many`, `grid-too-many`, `panes-nested`, + `flow-too-many-nodes` (all `warning` severity). + +**Thresholds recalibrated against the real corpus** (the spec called them "starting points, not +load-bearing constants"). The `rows` rule of "> 4 weight OR ≥ 3 heavy" flagged four *curated* +gallery examples (c4-architecture: weight 9 / 3 heavy; the three explainers: weight 7 / 3 heavy), +so it was tightened to **≥ 4 heavy OR weight > 10**, which cleanly separates the over-stuffed +trigger case (screwbits: weight 10 / 4 heavy → warns) from the densest legit gallery views. The +other thresholds (columns > 3, grid > 8, nested panes, standalone flow > 30 nodes) were already +clear of the gallery and kept as specified. A "panes density" calibration guard test asserts no +shipped example trips a finding. + +Files actually touched: `packages/viewer/src/flow-geometry.ts`, +`packages/viewer/test/flow-geometry.test.ts` (the original "Files touched" list below is the +pre-adaptation plan). + +--- + +## Problem + +A pushed view can render unreadably small. The trigger case is +`explainer/screwbits` (the [demo workspace](https://termchart-viewer-yhjtx5dusq-uc.a.run.app/w/demo/#explainer%2Fscrewbits)): +a `rows` panes layout with **5 panes, ~4 of them "heavy"** (markdown → flow → +component → vegalite → component). Stacked vertically, each pane gets a sliver of +the viewport, so the flow (10 nodes, TB) and the charts render cramped and tiny. + +Nothing today catches this. The two existing checks are orthogonal: + +- `packages/viewer/src/validate.ts` — **structural only** at push time: does the + JSON parse, does it have the right shape (`{nodes,edges}`, `{layout,panes}`, + component tree). It does not look at how much content is crammed into one view. +- `packages/cli/src/lint.ts` — Mermaid **terminal-subset** lint (unsupported + constructs, node-count cliff for the ASCII renderer). Unrelated to the rich + viewer. + +The diagram-recipes skill *advertises* a "geometry lint" that returns readability +warnings on flow push — but no such code exists. This spec fills that gap. + +## Goal + +Add a static, push-time **readability lint** that warns (does not block) when a +view is over-stuffed and will render unreadably dense, covering **every** push +path. Warn-only so thresholds can be calibrated against the real corpus before any +tightening. + +## Non-goals (YAGNI) + +- **No** screenshot/vision check (true pixel readability) — considered and + rejected as too expensive for a per-push gate. +- **No** rejecting pushes — warn-only. +- **No** auto-fixing the renderer (pagination / min-scale). +- **No** in-viewer warning banner — possible later add, out of scope here. + +## Architecture + +A new pure module **`packages/viewer/src/readability.ts`**, mirroring the shape and +conventions of `validate.ts`. It never renders; it reads the parsed spec. + +``` +server push handler (packages/viewer/src/server.ts): + validateContent(type, content) → 400 on structural error (unchanged) + readabilityLint(type, content) → string[] warnings (NEW) + store.set + hub.broadcast → push always succeeds (unchanged) + respond: + 204 if no warnings (unchanged shape) + 200 + { warnings: string[] } if any warnings (NEW) +``` + +Running it **server-side**, right after `validateContent` passes, is what makes it +universal: it covers the CLI, the ADK listener agent, and raw `POST /push` alike — +not just the CLI. The server is the single source of truth; the CLI only prints +what the server returns (no duplicated lint logic across packages). + +### Module interface + +```ts +// packages/viewer/src/readability.ts + +/** A non-fatal readability concern: the view is renderable but will be cramped. */ +export interface ReadabilityWarning { + /** Stable code for testing / future filtering, e.g. "rows-overstuffed". */ + code: string; + /** Human-readable, actionable message. */ + message: string; +} + +/** + * Inspect a parsed push payload for layouts that will render unreadably dense. + * Pure: no rendering, no I/O. `content` is the raw JSON string (already known to + * parse + be structurally valid, since validateContent runs first). + * Returns [] when the view is comfortably sized. + */ +export function readabilityLint(type: string, content: string): ReadabilityWarning[]; +``` + +The server flattens warnings to `string[]` (`message`) for the response body; the +`code` exists for unit tests and possible future client-side filtering. + +## Heuristics — the "density budget" + +Estimate each pane's comfortable **weight** (≈ readable blocks of vertical room), +then flag layouts whose total exceeds what a viewport shows at a readable size. + +### Per-pane weight + +| Pane type | Weight | +|----------------------|--------| +| `flow` | 2, **+1 per ~8 nodes beyond 6** (10 nodes ≈ 2–3) | +| `vegalite` | 2 (axes + legend need room) | +| `component` | 1, or **2** if the tree is large (contains `Table`/`SimpleGrid`, or > ~40 nodes) | +| `markdown` / `text` | `ceil(chars / 1200)`, capped at 2 | +| `mermaid` | 1 | +| nested `panes` | 2 (cumulative shrink) | + +A pane with weight ≥ 2 is "heavy". + +### Layout rules (starting thresholds) + +These are warn-only and **explicitly tunable** against the corpus; they are +starting points, not load-bearing constants. + +- **`rows`**: warn if total weight **> 4** OR **≥ 3 heavy** panes. + → screwbits ≈ weight 8–10 with ~4 heavy → warns. +- **`columns`**: warn if **> 3** panes (each column too narrow to read). +- **`grid`**: warn if **> 8** tiles (each tile drops below a readable size). +- **nested panes**: warn whenever a pane's content is itself a `panes` payload. +- **standalone `flow`** (top-level push, not in panes): warn if node count is very + high (**> 30**) — suggest splitting into multiple scopes. + +### Warning messages (actionable) + +Each warning names the offender and gives the fix. Examples: + +- `rows-overstuffed`: + > `panes 'rows' has 5 panes incl. 3 heavy (flow, vegalite, component); stacked they'll render cramped. Split across scopes, use a grid, or fold detail into fewer panes.` +- `columns-too-many`: + > `panes 'columns' has 4 panes; columns get too narrow past 3. Drop one or switch to rows/grid.` +- `grid-too-many`: + > `panes 'grid' has 10 tiles; each drops below a readable size past ~8. Split across scopes.` +- `panes-nested`: + > `panes[2] nests another panes layout; nested panes compound the shrink. Flatten into one layout or a separate scope.` +- `flow-too-many-nodes`: + > `flow has 38 nodes; past ~30 it renders too small to read. Split into linked scopes or collapse detail.` + +## Surfacing warnings (CLI) + +`packages/cli/src/push.ts` currently treats any 2xx as success and ignores the +body. Change: on a **200** response, parse `{ warnings }`, print each to **stderr** +(prefixed, e.g. `readability: `), and still **return 0**. A **204** +(clean, or an old server that predates this change) prints nothing. Fully +backward-compatible. + +## Testing + +- **Unit** (`packages/viewer/test/readability.test.ts`, mirroring + `validate.test.ts`): + - clean single-pane view → no warnings + - screwbits-shaped `rows` (5 panes, ~4 heavy) → `rows-overstuffed` + - `columns` with 4 panes → `columns-too-many` + - `grid` with 10 tiles → `grid-too-many` + - nested panes → `panes-nested` + - standalone flow with 35 nodes → `flow-too-many-nodes` + - light report (one markdown + one small component in rows) → no warnings + - non-structured types (`mermaid`/`markdown`/`text` at top level) → no warnings +- **Server**: push that triggers a warning returns `200 + {warnings}` AND still + stores + broadcasts (push not blocked). +- **CLI** (`packages/cli/test/push.test.ts`): a `200 + {warnings}` response prints + each warning to stderr and exits 0; a `204` prints nothing and exits 0. + +## Rollout note + +The lint runs in the viewer server, so the **deployed Cloud Run viewer must be +redeployed** for cloud pushes to surface warnings; a local dev viewer picks it up +immediately. The CLI change is independent and backward-compatible (against an old +server it simply never sees a 200-with-warnings). + +## Files touched + +- `packages/viewer/src/readability.ts` — new pure lint module. +- `packages/viewer/src/server.ts` — call lint after validate; respond 200+warnings. +- `packages/cli/src/push.ts` — print warnings from a 200 response to stderr. +- `packages/viewer/test/readability.test.ts` — new unit tests. +- `packages/cli/test/push.test.ts` — CLI stderr/exit-code test. diff --git a/packages/viewer/src/flow-geometry.ts b/packages/viewer/src/flow-geometry.ts index 1fc51b0..4d61f41 100644 --- a/packages/viewer/src/flow-geometry.ts +++ b/packages/viewer/src/flow-geometry.ts @@ -54,7 +54,10 @@ export type Severity = "error" | "warning"; * 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"; + code: + | "missing-ref" | "edge-over-node" | "node-overlap" | "edge-near-node" | "crossings" | "direction" | "low-readability" + // 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"; message: string; count: number; } @@ -453,6 +456,102 @@ export function bestLayout(spec: unknown): ChosenLayout { return { ...chosen, attempts }; } +// ── 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; + +function flowNodeCount(content: string): number { + try { + const o = JSON.parse(content) as { nodes?: unknown }; + return Array.isArray(o.nodes) ? o.nodes.length : 0; + } catch { + return 0; + } +} + +/** 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 + } + case "vegalite": + return 2; // axes + legend need room + 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 + default: + return 1; // mermaid, ansi, … + } +} + +/** 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"; + const cells: { type: string; weight: number }[] = []; + let nestedIdx = -1; + panes.forEach((p, i) => { + if (p && typeof p === "object") { + const pane = p as { type?: unknown; content?: unknown }; + if (typeof pane.type === "string" && typeof pane.content === "string") { + cells.push({ type: pane.type, weight: paneWeight(pane.type, pane.content) }); + if (pane.type === "panes" && nestedIdx < 0) nestedIdx = i; + } + } + }); + const n = cells.length; + 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, + message: `panes 'rows' has ${n} panes incl. ${heavy.length} heavy (${heavy.map((c) => c.type).join(", ")}); stacked they'll render cramped. Split across scopes, use a grid, or fold detail into fewer panes.`, + }); + else if (layout === "columns" && n > 3) + out.push({ + severity: "warning", code: "columns-too-many", count: n, + message: `panes 'columns' has ${n} panes; columns get too narrow past 3. Drop one or switch to rows/grid.`, + }); + else if (layout === "grid" && n > 8) + out.push({ + severity: "warning", code: "grid-too-many", count: n, + message: `panes 'grid' has ${n} tiles; each drops below a readable size past ~8. Split across scopes.`, + }); + + return out; +} + /** * 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 @@ -486,12 +585,27 @@ export function geometryReport(type: string, content: string, depth = 0): { warn 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 }; } 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; + // Layout-level density (over-stuffed rows / too many columns or tiles / nested panes). + for (const f of panesDensityFindings(panes, (v as { layout?: unknown }).layout)) { + findings.push(f); + warnings.push(f.message); + } panes.forEach((p, i) => { if (p && typeof p === "object") { const pane = p as { type?: unknown; content?: unknown; title?: unknown }; diff --git a/packages/viewer/test/flow-geometry.test.ts b/packages/viewer/test/flow-geometry.test.ts index 13a6a5c..c372b01 100644 --- a/packages/viewer/test/flow-geometry.test.ts +++ b/packages/viewer/test/flow-geometry.test.ts @@ -300,3 +300,110 @@ 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. +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 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 }); + const codes = (type: string, content: string) => geometryReport(type, content).findings.map((f) => f.code); + + it("flags screwbits-shaped rows (5 panes, 4 heavy) as rows-overstuffed", () => { + const view = panes("rows", [ + pane("markdown", markdown), + pane("flow", flow(10)), + pane("component", heavyComponent), + pane("vegalite", vegalite), + pane("component", heavyComponent), + ]); + const f = geometryFindings("panes", view).find((x) => x.code === "rows-overstuffed"); + expect(f?.severity).toBe("warning"); + expect(f?.message).toMatch(/rows/); + }); + + it("does not flag a light rows view (markdown + one small component)", () => { + const view = panes("rows", [pane("markdown", markdown), pane("component", lightComponent)]); + expect(codes("panes", view)).not.toContain("rows-overstuffed"); + }); + + it("flags columns with 4 panes as columns-too-many", () => { + const view = panes("columns", [pane("markdown", markdown), pane("flow", flow(3)), pane("vegalite", vegalite), pane("markdown", markdown)]); + const f = geometryFindings("panes", view).find((x) => x.code === "columns-too-many"); + expect(f?.severity).toBe("warning"); + }); + + it("does not flag columns with 3 panes", () => { + const view = panes("columns", [pane("markdown", markdown), pane("flow", flow(3)), pane("vegalite", vegalite)]); + expect(codes("panes", view)).not.toContain("columns-too-many"); + }); + + it("flags a grid with 10 tiles as grid-too-many", () => { + const view = panes("grid", Array.from({ length: 10 }, () => pane("component", lightComponent))); + const f = geometryFindings("panes", view).find((x) => x.code === "grid-too-many"); + expect(f?.severity).toBe("warning"); + }); + + it("does not flag a grid with 8 tiles", () => { + const view = panes("grid", Array.from({ length: 8 }, () => pane("component", lightComponent))); + expect(codes("panes", view)).not.toContain("grid-too-many"); + }); + + it("flags nested panes as panes-nested", () => { + const inner = panes("rows", [pane("markdown", markdown), pane("component", lightComponent)]); + const view = panes("columns", [pane("markdown", markdown), pane("panes", inner, "nested")]); + const f = geometryFindings("panes", view).find((x) => x.code === "panes-nested"); + expect(f?.severity).toBe("warning"); + }); + + it("flags a standalone flow with 35 nodes as flow-too-many-nodes", () => { + const f = geometryFindings("flow", flow(35)).find((x) => x.code === "flow-too-many-nodes"); + expect(f?.severity).toBe("warning"); + expect(f?.message).toMatch(/35/); + }); + + it("does not flag a standalone flow with 12 nodes", () => { + expect(codes("flow", flow(12))).not.toContain("flow-too-many-nodes"); + }); + + it("does not apply flow-too-many-nodes to a flow nested inside panes (the panes density rule owns that)", () => { + const view = panes("rows", [pane("flow", flow(35))]); + expect(codes("panes", view)).not.toContain("flow-too-many-nodes"); + }); + + it("all density findings are warn-only (never error)", () => { + const view = panes("rows", [ + pane("flow", flow(10)), pane("component", heavyComponent), pane("vegalite", vegalite), pane("component", heavyComponent), + ]); + const density = geometryFindings("panes", view).filter((f) => + ["rows-overstuffed", "columns-too-many", "grid-too-many", "panes-nested", "flow-too-many-nodes"].includes(f.code)); + expect(density.length).toBeGreaterThan(0); + expect(density.every((f) => f.severity === "warning")).toBe(true); + }); + + it("ignores freeform top-level types", () => { + expect(geometryFindings("markdown", "# a very ".repeat(500))).toEqual([]); + 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. + 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`, () => { + const density = geometryReport("panes", readFileSync(join(EXAMPLES, f), "utf8")).findings + .filter((x) => DENSITY.has(x.code)).map((x) => x.message); + expect({ file: f, density }).toEqual({ file: f, density: [] }); + }); + } +});