diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7688c18..48324e7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,6 +3,11 @@ name: ci on: push: branches: [main] + # Branches must clear the same bar as main BEFORE they merge: the offline + # matrix (Linux + Windows, 3.11) and the live matrix (three Superset + # versions) are exactly the checks a feature branch is most likely to break. + pull_request: + branches: [main] workflow_dispatch: jobs: diff --git a/README.md b/README.md index 973c0b9..06b9659 100644 --- a/README.md +++ b/README.md @@ -108,12 +108,33 @@ Every capability, with the CLI verb reference: [docs/FEATURES.md](https://github then ask for a dashboard in plain words; the AI writes the spec, and the tool verifies and builds it. - **MCP server**: `chartwright-mcp` (installed with `pip install "chartwright[mcp]"`) - exposes six tools covering the whole lifecycle, usable from any MCP client. + exposes ten tools covering the whole lifecycle, usable from any MCP client. - **Open contract**: `chartwright schema` prints the spec's JSON Schema, so any LLM or tool can generate valid specs. - **Guardrails**: the AI proposes; the tool verifies, using your own Superset login. Verification reads names (datasets, columns, metrics), not rows. +## The design brain + +Correct-by-construction is table stakes; the design brain makes dashboards +*read well*. A toggleable BI/UX intelligence layer — audience-aware size and +scroll budgets, chart-choice limits, layout composition — split into a brief +the AI reads before authoring and a deterministic critic that reviews the +result: + +```bash +chartwright brief --audience executive # the guidance, before writing a spec +chartwright advise spec.json --fix # the critique, with safe auto-repairs +chartwright redesign old-dash --profile prod # audit + repair a live dashboard +``` + +Rules key on stable ids you can suppress per chart in the spec, thresholds +tune per audience or per deployment (`design.yaml`), and the brain learns +your house heights from the sizes you polish by hand (`chartwright +calibrate`). Off is really off: `--design off` restores byte-identical +behavior. The full rulebook and architecture: +[docs/DESIGN-BRAIN.md](https://github.com/debabsah/chartwright/blob/main/docs/DESIGN-BRAIN.md). + ## Drawing layouts as text The spec's `sketch` is the dashboard's shape, drawn with characters that you @@ -133,7 +154,8 @@ space under `K` deliberately empty. Every rule, drawn and explained: ## Testing and evidence -Every push runs 125 tests on Linux and Windows, plus the full pipeline (apply, +Every push and every pull request runs the full offline suite on Linux and +Windows, plus the full pipeline (apply, lifecycle soak, stale-tab adversary, fault injection) against real Superset 4.1.4, 5.0.0, and 6.1.0 containers. Chart options are checked against Superset's own source for every supported version, so a Superset change is diff --git a/chartwright/absorb.py b/chartwright/absorb.py index 94d59c6..266d0bb 100644 --- a/chartwright/absorb.py +++ b/chartwright/absorb.py @@ -19,6 +19,7 @@ from __future__ import annotations import copy +import json from dataclasses import asdict, dataclass, field from . import ids diff --git a/chartwright/cli.py b/chartwright/cli.py index 3d404ce..fb6af61 100644 --- a/chartwright/cli.py +++ b/chartwright/cli.py @@ -5,6 +5,10 @@ chartwright compile spec.json -o out.zip compile bundle with a stub resolution (golden/debug) chartwright check spec.json --profile P pre-flight referential resolution chartwright apply spec.json --profile P check -> compile -> import -> smoke + chartwright brief the design brief to read BEFORE authoring a spec + chartwright advise spec.json design review (add --profile for data-aware rules) + chartwright redesign --profile P decompile + audit + safe fixes -> redesigned spec + chartwright calibrate learn recommended heights from absorb history """ from __future__ import annotations @@ -37,6 +41,43 @@ def _die(payload: dict, code: int = 1) -> None: sys.exit(code) +def _design_blocks(advice: dict) -> str | None: + """Why `--design strict` should block, or None. A gate that cannot EVALUATE + must fail closed: advice degrades to counts of zero when the overlay is + broken (see _advice_payload), and a silent pass there would turn one typo + in an org-wide design.yaml into a disarmed gate everywhere it is used.""" + if advice.get("errors"): + return ("design advice could not be evaluated: " + + "; ".join(e.get("detail", "") for e in advice["errors"]) + + " -- fix it or pass --design off") + if advice["counts"]["error"] or advice["counts"]["warn"]: + return ("design findings block under --design strict; fix them, run " + "`chartwright advise --fix`, or record deliberate exceptions " + "in the spec's design.ignore") + return None + + +def _advice_payload(spec, resolution=None) -> dict: + """Advice riding along check/apply must never break the pipeline: a bad + overlay degrades to an error note inside the advice block, not a crash. + Under --design strict that error block BLOCKS (see _design_blocks); under + warn it is reported and the pipeline continues.""" + from .design import advise + + try: + return advise(spec, resolution=resolution).payload() + except Exception as e: # noqa: BLE001 - advice must NEVER break check/apply + from .design import DESIGN_BRAIN_VERSION + from .design.presets import DEFAULT_AUDIENCE + + return {"stage": "design", "ok": True, "design_brain": DESIGN_BRAIN_VERSION, + "audience": DEFAULT_AUDIENCE, + "counts": {"error": 0, "warn": 0, "info": 0}, "findings": [], + "fixed": [], "ignored": [], + "errors": [{"code": "overlay" if isinstance(e, ValueError) else "advice", + "detail": str(e)}]} + + def _client(profile_name: str): from .client import SupersetClient from .profiles import load_profile, ProfileError @@ -67,22 +108,58 @@ def main(argv: list[str] | None = None) -> None: def _main(argv: list[str] | None = None) -> None: - ap = argparse.ArgumentParser(prog="chartwright", description=__doc__) + ap = argparse.ArgumentParser(prog="chartwright", description=__doc__, + formatter_class=argparse.RawDescriptionHelpFormatter) sub = ap.add_subparsers(dest="cmd", required=True) - sub.add_parser("schema") + sub.add_parser("schema", help="print the JSON Schema a spec must satisfy") - v = sub.add_parser("validate") + v = sub.add_parser("validate", help="schema-validate a spec (offline, no network)") v.add_argument("spec") - comp = sub.add_parser("compile") + comp = sub.add_parser("compile", help="compile an import bundle offline (stub dataset ids)") comp.add_argument("spec") comp.add_argument("-o", "--output", default=None) + subhelp = {"check": "pre-flight referential resolution against the live instance", + "apply": "check -> compile -> import -> verify -> smoke", + "plan": "diff the spec against the live dashboard (drift detection)"} for name in ("check", "apply", "plan"): - p = sub.add_parser(name) + p = sub.add_parser(name, help=subhelp[name]) p.add_argument("spec") p.add_argument("--profile", required=True) + if name != "plan": + p.add_argument("--design", choices=["off", "warn", "strict"], default="warn", + help="design-brain advice: warn (report, default), strict (block), off") + + from .design.presets import AUDIENCE_NAMES + + adv = sub.add_parser("advise", help="design review of a spec against the design-brain rulebook") + adv.add_argument("spec") + adv.add_argument("--profile", default=None, help="enable data-aware rules (types, cardinality)") + adv.add_argument("--audience", choices=AUDIENCE_NAMES, default=None) + adv.add_argument("--fix", action="store_true", help="apply safe presentation-only fixes to the spec file") + adv.add_argument("--strict", action="store_true", help="exit 1 on warnings, not just errors") + adv.add_argument("--ignore", default=None, help="comma-separated rule ids to suppress") + adv.add_argument("--no-probe", action="store_true", help="skip cardinality queries (metadata only)") + + br = sub.add_parser("brief", help="the design brief to read BEFORE authoring a spec") + br.add_argument("--audience", choices=AUDIENCE_NAMES, default="analytical") + + rd = sub.add_parser("redesign", + help="decompile a live dashboard, audit it, apply safe fixes, write the redesigned spec") + rd.add_argument("dashboard", help="slug or numeric id of a live dashboard") + rd.add_argument("--profile", required=True) + rd.add_argument("--audience", choices=AUDIENCE_NAMES, default=None) + rd.add_argument("-o", "--output", default=None, + help="write the redesigned spec here (default: .json)") + rd.add_argument("--no-probe", action="store_true", help="skip cardinality queries (metadata only)") + + cal = sub.add_parser("calibrate", help="propose recommended heights from absorb history") + cal.add_argument("--write", action="store_true", help="record proposals in the design overlay") + cal.add_argument("--min-samples", type=int, default=5) + cal.add_argument("--since", default=None, metavar="90d", + help="only consider absorb events newer than this (decay knob)") dec = sub.add_parser("decompile") dec.add_argument("dashboard", help="slug or numeric id of a live dashboard") @@ -128,18 +205,144 @@ def _main(argv: list[str] | None = None) -> None: res = check(spec, client) payload = {"ok": res.ok, "stage": "resolve", "errors": [e.as_dict() for e in res.errors]} + gate = False + if args.design != "off": + advice = _advice_payload(spec, resolution=res if res.ok else None) + payload["advice"] = advice + blocked = _design_blocks(advice) if args.design == "strict" else None + gate = blocked is not None + if gate: + payload["ok"] = False + payload["errors"].append({"code": "design_gate", "detail": blocked}) print(json.dumps(payload, indent=2)) - sys.exit(0 if res.ok else 1) + sys.exit(0 if res.ok and not gate else 1) if args.cmd == "apply": spec = _load(args.spec) + advice = None + if args.design != "off": + # Pre-flight, offline (no probes: applies stay fast; data-aware + # advice is `chartwright advise --profile`). Strict blocks BEFORE + # anything on the instance is touched. + advice = _advice_payload(spec) + blocked = _design_blocks(advice) if args.design == "strict" else None + if blocked: + _die({"stage": "design", "ok": False, "advice": advice, + "errors": [{"code": "design_gate", "detail": blocked}]}) client = _client(args.profile) from .apply import apply as run_apply report = run_apply(spec, client, args.profile) - print(report.to_json()) + out = json.loads(report.to_json()) + if advice is not None: + out["advice"] = advice + print(json.dumps(out, indent=2)) sys.exit(0 if report.ok else 1) + if args.cmd == "advise": + spec = _load(args.spec) + resolution = prober = None + if args.profile: + client = _client(args.profile) + from .resolver import resolve + + resolution = resolve(spec, client) + if not args.no_probe: + from .design.probe import CardinalityProber + + prober = CardinalityProber(client) + ignore = tuple(s.strip() for s in (args.ignore or "").split(",") if s.strip()) + from .design import advise, advise_and_fix + + written = None + try: + if args.fix: + spec_data = json.loads(Path(args.spec).read_text(encoding="utf-8")) + new_data, report = advise_and_fix( + spec_data, audience=args.audience, ignore=ignore, + resolution=resolution, prober=prober) + if report.fixed: + # --fix rewrites the whole file (normalized JSON formatting, + # same as absorb); the payload discloses the path. + Path(args.spec).write_text( + json.dumps(new_data, indent=2, ensure_ascii=False) + "\n", encoding="utf-8") + written = str(args.spec) + else: + report = advise(spec, audience=args.audience, ignore=ignore, + resolution=resolution, prober=prober) + except ValueError as e: + _die({"stage": "design", "errors": [{"code": "overlay", "detail": str(e)}]}) + payload = report.payload() + if written: + payload["written"] = written + if resolution is not None and resolution.errors: + payload["resolution_errors"] = [e.as_dict() for e in resolution.errors] + print(json.dumps(payload, indent=2)) + sys.exit(1 if report.gate(args.strict) else 0) + + if args.cmd == "redesign": + client = _client(args.profile) + from .decompile import decompile_live + + try: + result = decompile_live(args.dashboard, client) + except ValueError as e: + _die({"stage": "redesign", "errors": [{"code": "decompile", "detail": str(e)}]}) + try: + spec = load_spec(result.spec) + except ValidationError as e: + _die({"stage": "redesign", "losses": result.losses_json(), "errors": [ + {"code": "decompiled_spec_invalid", + "detail": "the decompiled spec does not load; redesign by hand from " + "`chartwright decompile` output"}, + *json.loads(e.json()), + ]}) + from .apply import _ownership_guard + from .design.probe import CardinalityProber + from .design.redesign import redesign_spec + from .resolver import resolve + + owned = _ownership_guard(spec, client) is None + resolution = resolve(spec, client) + prober = None if args.no_probe else CardinalityProber(client) + try: + new_data, payload = redesign_spec( + result.spec, result.losses_json(), owned=owned, + audience=args.audience, resolution=resolution, prober=prober) + except ValueError as e: + _die({"stage": "design", "errors": [{"code": "overlay", "detail": str(e)}]}) + out = Path(args.output or f"{new_data['dashboard']['slug']}.json") + if args.output is None and out.exists(): + _die({"stage": "redesign", "errors": [{ + "code": "output_exists", + "detail": f"{out} already exists (likely a previous redesign); " + f"pass -o to choose where to write"}]}) + out.write_text(json.dumps(new_data, indent=2, ensure_ascii=False) + "\n", encoding="utf-8") + payload["output"] = str(out) + if resolution.errors: + payload["resolution_errors"] = [e.as_dict() for e in resolution.errors] + print(json.dumps(payload, indent=2)) + sys.exit(0 if payload["ok"] else 1) + + if args.cmd == "brief": + from .design.brief import render_brief + + try: + print(render_brief(args.audience)) + except ValueError as e: + _die({"stage": "design", "errors": [{"code": "overlay", "detail": str(e)}]}) + return + + if args.cmd == "calibrate": + from .design.calibrate import calibrate + + try: + print(json.dumps(calibrate(min_samples=args.min_samples, write=args.write, + since=args.since), indent=2)) + except ValueError as e: + _die({"stage": "calibrate", "errors": [{"code": "overlay", "detail": str(e)}]}) + return + if args.cmd == "plan": spec = _load(args.spec) client = _client(args.profile) @@ -166,10 +369,18 @@ def _main(argv: list[str] | None = None) -> None: live_position = json.loads(detail.get("position_json") or "{}") spec_data = json.loads(Path(args.spec).read_text(encoding="utf-8")) new_data, report = absorb_heights(spec, spec_data, live_position) + # Serialize BEFORE any file side effect: a reporting failure must never + # follow a silent mutation of the user's spec file. + payload = report.to_json() if report.absorbed and not args.dry_run: Path(args.spec).write_text( json.dumps(new_data, indent=2, ensure_ascii=False) + "\n", encoding="utf-8") - print(report.to_json()) + # Feed the design brain's calibration loop (chartwright calibrate): + # absorbed heights are ground truth about what humans actually want. + from .design.calibrate import record_absorb + + record_absorb(args.profile, spec, report.absorbed) + print(payload) sys.exit(0) if args.cmd == "decompile": diff --git a/chartwright/compiler.py b/chartwright/compiler.py index a1207d1..9a85a97 100644 --- a/chartwright/compiler.py +++ b/chartwright/compiler.py @@ -200,10 +200,20 @@ def metric(m: str): if chart.columns: p["query_mode"] = "raw" p["all_columns"] = chart.columns + if chart.sort_by: + # Raw mode sorts on COLUMNS: order_by_cols holds JSON-encoded + # [column, ascending] pairs (Table controlPanel, all supported + # versions). False = descending, matching the ranking intent. + p["order_by_cols"] = [json.dumps([chart.sort_by, False])] else: p["query_mode"] = "aggregate" p["groupby"] = chart.groupby or [] p["metrics"] = [metric(m) for m in (chart.metrics or [])] + if chart.sort_by: + # Aggregate mode sorts on a METRIC via timeseries_limit_metric + # ("Sort by" in the UI), declared by the Table plugin on 4.1.4, + # 5.0.0 and 6.1.0 alike (tools/contracts/params-contract.json). + p["timeseries_limit_metric"] = metric(chart.sort_by) p["row_limit"] = chart.row_limit or DEFAULT_ROW_LIMIT[t] p["server_page_length"] = 10 if chart.sort_by: diff --git a/chartwright/decompile.py b/chartwright/decompile.py index a43107e..e70163d 100644 --- a/chartwright/decompile.py +++ b/chartwright/decompile.py @@ -8,6 +8,7 @@ from __future__ import annotations import io +import json import re import zipfile from dataclasses import asdict, dataclass, field @@ -94,6 +95,21 @@ def _format_to_spec(cf: dict) -> dict | None: return rule +def _order_by_col(order_by_cols) -> tuple[str | None, bool]: + """Raw-table sort: order_by_cols holds ["column", ascending] pairs, stored + JSON-encoded by the control but seen as plain lists in some exports. + Returns (first column, ascending); (None, False) when there is no sort.""" + for entry in order_by_cols or []: + if isinstance(entry, str): + try: + entry = json.loads(entry) + except ValueError: + continue + if isinstance(entry, list) and len(entry) == 2 and isinstance(entry[0], str): + return entry[0], bool(entry[1]) + return None, False + + def _metric_to_spec(m, losses: list[Loss], chart: str) -> str | None: if isinstance(m, str): return m @@ -249,6 +265,9 @@ def keep_row_limit() -> None: if not out["columns"]: losses.append(Loss(name, "raw table with no columns; chart skipped")) return None + col, asc = _order_by_col(p.get("order_by_cols")) + if col: + out["sort_by"] = col else: ms = [m for m in (metric_one(m) for m in (p.get("metrics") or [])) if m] out["metrics"] = ms or None @@ -256,6 +275,17 @@ def keep_row_limit() -> None: if not out["metrics"] and not out["groupby"]: losses.append(Loss(name, "aggregate table with no metrics/groupby; chart skipped")) return None + sort = p.get("timeseries_limit_metric") or p.get("series_limit_metric") + if sort: + s = metric_one(sort) + if s: + out["sort_by"] = s + asc = p.get("order_desc") is False + if out.get("sort_by") and asc: + # The spec sorts descending (a ranking); an ascending live sort is + # named rather than silently flipped on the next apply. + losses.append(Loss(name, "ascending sort not preserved; the spec sorts " + "sort_by descending, so re-apply will sort descending")) keep_row_limit() elif spec_type == "pivot_table": ms = [m for m in (metric_one(m) for m in (p.get("metrics") or [])) if m] @@ -327,7 +357,9 @@ def keep_row_limit() -> None: out["groupby"] = gb keep_row_limit() - mapped_here = {"combineMetric", "conditional_formatting"} if spec_type == "pivot_table" else set() + mapped_here = {"combineMetric", "conditional_formatting"} if spec_type == "pivot_table" else ( + {"order_by_cols", "timeseries_limit_metric", "series_limit_metric"} + if spec_type == "table" else set()) unmapped = sorted(k for k in p if k not in _IGNORABLE and k not in mapped_here) if unmapped: losses.append(Loss(name, f"params not preserved: {unmapped}")) diff --git a/chartwright/design/__init__.py b/chartwright/design/__init__.py new file mode 100644 index 0000000..35dd7b6 --- /dev/null +++ b/chartwright/design/__init__.py @@ -0,0 +1,129 @@ +"""The design brain: a toggleable BI/UX intelligence layer. + +Two halves over one rulebook (docs/DESIGN-BRAIN.md): +- Tier G, the brief (`brief.py`): design guidance the LLM reads BEFORE + authoring a spec. +- Tier L, the critic (this module's `advise`): a deterministic linter over + the finished spec, with a presentation-only autofix subset. + +Off is really off: nothing here runs unless asked, and `spec_version` is +untouched. +""" + +from __future__ import annotations + +from ..resolver import Resolution +from ..spec import DashboardSpec, load_spec +from .fix import apply_fixes +from .model import (DESIGN_BRAIN_VERSION, RULES, AdviceReport, Finding, RuleContext, + canonical_rule_id, known_rule_ids) +from .presets import DEFAULT_AUDIENCE, Overlay, load_overlay, params_for + + +def advise(spec: DashboardSpec, *, audience: str | None = None, + ignore: tuple[str, ...] = (), resolution: Resolution | None = None, + prober=None, overlay: Overlay | None = None) -> AdviceReport: + """Run the rulebook. Precedence for the audience: caller > spec.design > + default. `ignore` entries are rule ids ('size.pie-geometry') or per-chart + keys ('size.pie-geometry@Sales by Region'); the spec's design.ignore and + the overlay's disable list are merged in.""" + overlay = overlay if overlay is not None else load_overlay() + design = spec.design + aud = audience or (design.audience if design else None) or DEFAULT_AUDIENCE + params = params_for(aud, overlay) + + # Ignore entries are validated up front: a typo'd rule id would otherwise + # be a suppression that silently never suppresses. + raw_suppressed = set(ignore) | set(design.ignore if design else ()) | set(overlay.disable) + suppressed: set[str] = set() + unmatched: list[str] = [] + for entry in raw_suppressed: + rule_id, _, scope = entry.partition("@") + canon = canonical_rule_id(rule_id) + if canon not in RULES: + unmatched.append(entry) + continue + suppressed.add(f"{canon}@{scope}" if scope else canon) + + ctx = RuleContext(spec, params, resolution, prober) + findings: list[Finding] = [] + ignored: list[str] = [] + polished: list[str] = [] + for r in RULES.values(): + if r.data_aware and resolution is None: + continue + for f in r.fn(ctx): + # Per-deployment severity override (single choke point). + f.severity = overlay.severity.get(f.rule, f.severity) + # Explicit intent first: an ignore entry is ALWAYS visible in + # `ignored`, even when the polish skip below would also apply. + if f.rule in suppressed or f.key in suppressed or f.scope_key in suppressed: + ignored.append(f.key if f.chart else f.scope_key) + continue + # Fractional height = absorb's signature: a human already sized + # this chart in the UI; HEIGHT opinions yield to that. Width and + # data complaints survive -- absorb cannot write widths, so a + # fractional height says nothing about them. + # The skip is REPORTED (`polished`): this is an inferred signal, + # and an inferred signal that silences a rule invisibly is + # indistinguishable from the rule having passed. + if f.height_driven and f.chart and ctx.human_polished(f.chart): + polished.append(f.key) + continue + # A height fix on a sketch-drawn chart is real but leaves the + # drawing stale; say so instead of silently diverging (WYSIWYG). + if (f.fix and "height" in (f.fix.get("set") or {}) and f.chart + and ctx.is_sketch(f.chart)): + f.detail += (" (the fix writes an explicit height that overrides the " + "sketch; to keep the drawing true, repeat the band's " + "line(s) instead)") + findings.append(f) + + order = {"error": 0, "warn": 1, "info": 2} + findings.sort(key=lambda f: (order[f.severity], f.rule, f.chart or "")) + return AdviceReport( + ok=not any(f.severity == "error" for f in findings), + audience=aud, findings=findings, ignored=sorted(set(ignored)), + unmatched_ignores=sorted(unmatched), polished=sorted(set(polished)), + ) + + +def advise_and_fix(spec_data: dict, *, audience: str | None = None, + ignore: tuple[str, ...] = (), resolution: Resolution | None = None, + prober=None, overlay: Overlay | None = None, + max_rounds: int = 5) -> tuple[dict, AdviceReport]: + """Fix loop: advise -> apply safe fixes -> re-advise until no fixable + findings remain. Convergence invariant: KPI heights are clamped into 2..6 + by exactly one rule (size.kpi-height); every OTHER height fix only raises, + and conflicting raises merge to max() -- so no two rules fight over one + chart's height and the loop terminates. max_rounds and the no-progress + check are backstops for invariant violations, not tuning knobs. + Returns (patched spec data, final report with .fixed populated).""" + data = spec_data + fixed_all: list[str] = [] + for _ in range(max_rounds): + report = advise(load_spec(data), audience=audience, ignore=ignore, + resolution=resolution, prober=prober, overlay=overlay) + fixable = [f for f in report.findings if f.fix] + if not fixable: + report.fixed = fixed_all + return data, report + new_data, applied = apply_fixes(data, fixable) + if new_data == data: # invariant violated: fixes made no progress + report.fixed = fixed_all + report.findings.append(Finding( + rule="design.fix-stalled", severity="warn", chart=None, where="fix loop", + detail=f"fixes {sorted(f.key for f in fixable)} made no progress; report a rule bug", + )) + return data, report + data = new_data + fixed_all += applied + report = advise(load_spec(data), audience=audience, ignore=ignore, + resolution=resolution, prober=prober, overlay=overlay) + report.fixed = fixed_all + return data, report + + +# Import for side effect: rule registration. Kept at the bottom so RULES is +# populated by the time advise() iterates it, without a circular import. +from . import rules # noqa: E402,F401 diff --git a/chartwright/design/brief.py b/chartwright/design/brief.py new file mode 100644 index 0000000..adbc524 --- /dev/null +++ b/chartwright/design/brief.py @@ -0,0 +1,101 @@ +"""Render the Tier G design brief: what the LLM reads BEFORE authoring a spec. + +Assembled from the packaged guideline files, the audience's parameter values, +a summary of what the critic will later enforce (so the author pre-complies +instead of iterating), and any house guidance from the overlay. Budget: the +brief lands in an LLM context window, so it stays compact by contract +(tests enforce a line ceiling). +""" + +from __future__ import annotations + +from importlib import resources + +from .model import RULES +from .presets import Overlay, load_overlay, params_for + + +def _guideline(name: str) -> str: + return (resources.files("chartwright.design") / "guidelines" / name).read_text(encoding="utf-8") + + +def render_brief(audience: str, overlay: Overlay | None = None) -> str: + overlay = overlay if overlay is not None else load_overlay() + p = params_for(audience, overlay) + intent = { + "executive": "one screen, few numbers, big; every extra chart costs attention", + "analytical": "scrolling analysis; depth over fold, structure over density", + "operational": "dense monitor view; everything visible, nothing decorative", + }[audience] + + # ASCII only: Windows pipes default to cp1252; the brief must survive any console. + lines = [ + f"# Design brief: {audience}", + "", + f"Intent: {intent}.", + "", + "## Budgets and sizes (1 height unit = 40 px; widths are twelfths of the page)", + "", + f"- Height budget per tab: {p.fold_units} units (~{p.fold_units * 40}px). Over budget -> tabs or prune.", + f"- KPI band: {p.kpi_row_min}-{p.kpi_row_max} big numbers, {p.kpi_height} units tall, first band on the page.", + f"- Axis charts (timeseries/bar/heatmap/histogram): >= {p.min_axis_height} units tall, 8 is the comfortable default.", + f"- At most {p.max_row_charts} axis charts per row; below 3/12 width a chart is unreadable.", + "- Pie/donut: >= 5/12 wide, >= 8 tall. Heatmap: >= 5/12 wide (7/12 when many columns), >= 6 tall.", + f"- Vertical bars: <= {p.vbar_max_categories} categories, then flip horizontal. Pies: <= {p.pie_max_slices} slices.", + f"- Timeseries: <= {p.series_max} grouped series. Tables: height should show >= {p.table_visible_ratio:.0%} of row_limit (~0.8 units/row).", + "- Charts sharing a row share a height; Superset sizes the row to its tallest child.", + ] + if p.recommended_heights: + rec = ", ".join(f"{t}: {h:g}" for t, h in sorted(p.recommended_heights.items())) + lines.append(f"- Calibrated house heights (from real usage): {rec}.") + + exemplar = { + "executive": [ + '"KKK MMM NNN QQQ", K/M/N/Q: four KPIs, one band, above everything', + '"LLLLLLLL SSSS", L: the one trend that answers the question', + '"LLLLLLLL SSSS", S: its single most useful breakdown', + '"LLLLLLLL SSSS", (three lines at line: 2 = 6 units < the 22-unit budget)', + ], + "analytical": [ + '"KKKK MMMM NNNN", KPI band first', + '"LLLLLLLL SSSS", L: trend (4-5 lines tall); S: breakdown stacked beside it', + '"LLLLLLLL SSSS",', + '"LLLLLLLL PPPP", P: second breakdown completes the sidebar', + '"TTTTTTTTTTTT", T: the detail table, full width, below the fold is fine', + '"TTTTTTTTTTTT",', + ], + "operational": [ + '"KKK MMM NNN QQQ", dense KPI band (up to 8 fit)', + '"LLLLLL SSSSSS", two half-width monitors per band', + '"LLLLLL SSSSSS",', + '"AAAAAA BBBBBB", everything visible, one screen, no scroll', + '"AAAAAA BBBBBB",', + ], + }[audience] + lines += ["", "## The canonical shape (a sketch to start from)", ""] + [f" {l}" for l in exemplar] + + lines += [ + "", + "## Defaults the compiler fills in (the critic flags the ones that matter)", + "", + "- Omitted height -> 8 units (KPIs 4, markdown 4). Omitted width -> the row splits evenly.", + "- Omitted row_limit -> 10,000 (pie 100, table 1,000, funnel 10): set it deliberately on bar/pie/table/pivot.", + "- Omitted time_grain -> P1D. Grains: PT1H P1D P1W P1M P3M P1Y. Points ~= range/grain; budget ~40 for bars, ~300 for lines.", + "- number_format is d3: ',.0f' thousands, '.1%' percent, '.3s' SI units, '$,.2f' money. One measure, one format.", + "- Native filter bar: select pickers (few, they query on load), a time_range WITH a default, numeric range sliders.", + ] + + lines += ["", _guideline("chart-choice.md").strip(), "", _guideline("composition.md").strip()] + + lines += ["", "## What the critic enforces (`chartwright advise`)", ""] + by_cat: dict[str, list[str]] = {} + for r in RULES.values(): + by_cat.setdefault(r.id.split(".")[0], []).append(f"`{r.id}` {r.doc}") + for cat in sorted(by_cat): + lines.append(f"- **{cat}**: " + "; ".join(sorted(by_cat[cat]))) + lines += ["", "Suppress a deliberate exception in the spec: " + '`"design": {"ignore": ["rule.id@Chart Name"]}` -- never dodge a finding by hand-tuning output.'] + + if overlay.brief_extra.strip(): + lines += ["", "## House guidance", "", overlay.brief_extra.strip()] + return "\n".join(lines) + "\n" diff --git a/chartwright/design/calibrate.py b/chartwright/design/calibrate.py new file mode 100644 index 0000000..4f61624 --- /dev/null +++ b/chartwright/design/calibrate.py @@ -0,0 +1,139 @@ +"""Phase 3: the brain learns from the humans it defers to. + +Every non-dry-run `chartwright absorb` appends the heights humans dragged +charts to (the polish the brain is told to respect) to an append-only log. +`chartwright calibrate` mines that log: when a chart type is systematically +resized away from its recommended height, it proposes -- and with --write, +records -- a new recommended height in the design overlay, which both the +brief and the size-rule autofixes then honor. + +Signal hygiene: one vote per (profile, slug, chart), latest wins, so one +dashboard absorbed ten times doesn't outvote nine dashboards absorbed once. +""" + +from __future__ import annotations + +import datetime +import json +import statistics +from pathlib import Path + +from ..spec import DEFAULT_HEIGHT, DashboardSpec +from .presets import design_dir, load_overlay, overlay_path + + +def log_path() -> Path: + return design_dir() / "absorb-log.jsonl" + + +def record_absorb(profile: str, spec: DashboardSpec, absorbed: list[dict]) -> None: + """Append one event per absorbed height. Best-effort: a failure to log + never fails an absorb.""" + if not absorbed: + return + types = {c.name: c.type for c in spec.charts} + audience = spec.design.audience if spec.design else None + try: + path = log_path() + path.parent.mkdir(parents=True, exist_ok=True) + ts = datetime.datetime.now().isoformat(timespec="seconds") + with path.open("a", encoding="utf-8") as f: + for row in absorbed: + f.write(json.dumps({ + "ts": ts, "profile": profile, "slug": spec.dashboard.slug, + "chart": row["chart"], "type": types.get(row["chart"]), + "height": row["height"], "audience": audience, + }) + "\n") + except OSError: + pass + + +def _baseline(chart_type: str, recommended: dict) -> float: + if chart_type in recommended: + return float(recommended[chart_type]) + return float(DEFAULT_HEIGHT.get(chart_type, DEFAULT_HEIGHT["default"])) + + +def _parse_since(since: str | None) -> datetime.datetime | None: + if not since: + return None + import re + + m = re.fullmatch(r"(\d+)d", since.strip()) + if not m: + raise ValueError(f"--since must look like '90d', got {since!r}") + return datetime.datetime.now() - datetime.timedelta(days=int(m.group(1))) + + +def calibrate(min_samples: int = 5, write: bool = False, since: str | None = None) -> dict: + """Group logged heights by (audience, chart type); where the median drifts + >= 1 unit from the current baseline with enough samples, propose it. + Events logged without an audience calibrate the flat (all-audience) + recommendation. --since 90d is the decay knob: older habits age out. + --write merges proposals into the overlay (flat recommended_heights, or + audiences..recommended_heights for audience-tagged proposals).""" + cutoff = _parse_since(since) + events: dict[tuple, dict] = {} + path = log_path() + if path.exists(): + for line in path.read_text(encoding="utf-8").splitlines(): + if not line.strip(): + continue + try: + e = json.loads(line) + except json.JSONDecodeError: + continue + if not e.get("type") or not isinstance(e.get("height"), (int, float)): + continue + if cutoff is not None: + try: + if datetime.datetime.fromisoformat(e.get("ts", "")) < cutoff: + continue + except ValueError: + continue + events[(e.get("profile"), e.get("slug"), e.get("chart"))] = e + + by_group: dict[tuple, list[float]] = {} # (audience|None, type) -> heights + for e in events.values(): + by_group.setdefault((e.get("audience"), e["type"]), []).append(float(e["height"])) + + overlay = load_overlay() + proposals = [] + candidates = [] # observed but not (yet) actionable: the report says why + for (aud, t), heights in sorted(by_group.items(), key=lambda kv: (kv[0][0] or "", kv[0][1])): + rec = ((overlay.audiences.get(aud) or {}).get("recommended_heights") + if aud else None) or overlay.recommended_heights + current = _baseline(t, rec) + median = round(statistics.median(heights), 1) + entry = {"type": t, "audience": aud, "samples": len(heights), + "median": median, "current": current} + if len(heights) >= min_samples and abs(median - current) >= 1: + proposals.append(entry) + else: + entry["note"] = (f"needs >= {min_samples} samples" if len(heights) < min_samples + else "within 1 unit of current; no change") + candidates.append(entry) + report = { + "stage": "calibrate", "ok": True, "events": len(events), "since": since, + "proposals": proposals, "candidates": candidates, "written": False, + "overlay": str(overlay_path()), + } + if write and proposals: + import yaml + + p = overlay_path() + data = {} + if p.exists(): + data = yaml.safe_load(p.read_text(encoding="utf-8")) or {} + for prop in proposals: + if prop["audience"]: + tgt = (data.setdefault("audiences", {}) + .setdefault(prop["audience"], {}) + .setdefault("recommended_heights", {})) + else: + tgt = data.setdefault("recommended_heights", {}) + tgt[prop["type"]] = prop["median"] + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(yaml.safe_dump(data, sort_keys=True), encoding="utf-8") + report["written"] = True + return report diff --git a/chartwright/design/fix.py b/chartwright/design/fix.py new file mode 100644 index 0000000..1ebd03a --- /dev/null +++ b/chartwright/design/fix.py @@ -0,0 +1,66 @@ +"""Apply the safe-fix subset to the raw spec JSON (same mechanics as absorb: +patch the dict, caller re-validates and writes). Conflicting numeric fixes on +one field merge to the max -- every height fix is a raise-to-minimum, so max +satisfies all of them and the fix loop converges. +""" + +from __future__ import annotations + +import copy + +from .model import Finding + + +def _md_block(data: dict, ti, ri, ii): + """The raw markdown-block dict at (tab, row, item), or None if the layout + changed under us (stale indices are skipped, never errors).""" + try: + rows = (data["layout"]["tabs"][ti]["rows"] if ti is not None + else data["layout"]["rows"]) + item = rows[ri][ii] + except (KeyError, IndexError, TypeError): + return None + return item if isinstance(item, dict) and "markdown" in item else None + + +def apply_fixes(spec_data: dict, findings: list[Finding]) -> tuple[dict, list[dict]]: + """Returns (patched deep copy, applied entries). Each entry says exactly + what changed and from what: {finding, rule, chart, set: {field: new}, + was: {field: old}} -- an autofix that can't show its diff is a mutation + the user has to trust blind. Unknown chart names / stale markdown indices + are skipped, not errors.""" + out = copy.deepcopy(spec_data) + by_name = {c.get("name"): c for c in out.get("charts", [])} + merged: dict[str, dict] = {} + chart_findings: list[Finding] = [] + applied: list[dict] = [] + for f in findings: + if not f.fix: + continue + if "md" in f.fix: # markdown blocks have no name; addressed by position + block = _md_block(out, *f.fix["md"]) + if block is not None: + was = {k: block.get(k) for k in f.fix["set"]} + block.update(f.fix["set"]) + applied.append({"finding": f.key, "rule": f.rule, "chart": None, + "md": f.fix["md"], "set": dict(f.fix["set"]), "was": was}) + continue + if f.fix.get("chart") not in by_name: + continue + tgt = merged.setdefault(f.fix["chart"], {}) + for k, v in f.fix["set"].items(): + if k in tgt and isinstance(v, (int, float)) and isinstance(tgt[k], (int, float)): + tgt[k] = max(tgt[k], v) + else: + tgt[k] = v + chart_findings.append(f) + for f in chart_findings: # report the FINAL merged value per touched field + name = f.fix["chart"] + applied.append({ + "finding": f.key, "rule": f.rule, "chart": name, + "set": {k: merged[name][k] for k in f.fix["set"]}, + "was": {k: by_name[name].get(k) for k in f.fix["set"]}, + }) + for name, sets in merged.items(): + by_name[name].update(sets) + return out, applied diff --git a/chartwright/design/guidelines/chart-choice.md b/chartwright/design/guidelines/chart-choice.md new file mode 100644 index 0000000..f305cc5 --- /dev/null +++ b/chartwright/design/guidelines/chart-choice.md @@ -0,0 +1,31 @@ +# Chart choice + +| The question | The chart | +|---|---| +| How much, right now | `big_number_total`; add `big_number_trend` when direction matters | +| How it moves over time | `timeseries_line`; `_area` for cumulative volume; `_bar` for discrete periods (<= ~30) | +| Ranking over a category | `bar` with `"orientation": "horizontal"`, `row_limit` ~10; long labels get a full line each. On tables, a `row_limit` needs a `sort_by` or it's a sample, not a ranking | +| Share of a whole | `pie`/donut only for <= 7 slices; else horizontal bar | +| Several measures per item | `table` or `pivot_table`, not grouped bars. Pivots: <= 3 total dimensions; column-dim values x metrics <= ~15 rendered columns | +| Two dimensions, one measure | `heatmap` (keep the grid under ~400 cells) | +| Distribution of one column | `histogram`, 20-30 bins; trim long tails with a WHERE filter and say so in the title | +| Staged conversion | `funnel`, 3-8 ordered stages | +| Hierarchical share | `treemap`, at most 2 levels | + +- A `row_limit` on a pie redefines the whole: the shown slices read as 100%, + so a truncated pie lies about share. Prefer a horizontal bar for top-N; a + truncated pie's title must say "top N". +- Table and pivot height is a DATA-DEPENDENT property, not a one-time layout + choice: size from expected rows (~0.75 units per row + 3 for title/header, + +1 for pivot column headers) and re-check whenever a row dimension is + expected to gain members. Rows past the fold hide behind an inner + scrollbar; smoke warns at apply time when they do. +- Sort order is per family: bars sort by their first metric (right for + rankings, wrong for ordinals like weekday/month); heatmap and pivot + categories sort alphabetically. For ordinal dimensions, chart an + order-encoded label column (labels prefixed with a sort index: '1-Mon') + if the dataset has one. +- One dominant category flattening its siblings is the data talking: note it + or filter it, don't hide it. +- Every chart with a time axis should tolerate the dashboard time filter; + add a `time_range` filter to the bar so viewers pick their window. diff --git a/chartwright/design/guidelines/composition.md b/chartwright/design/guidelines/composition.md new file mode 100644 index 0000000..4570395 --- /dev/null +++ b/chartwright/design/guidelines/composition.md @@ -0,0 +1,27 @@ +# Composition + +- Inverted pyramid, top to bottom: KPI band first (the answer), trends second + (the why), breakdowns and tables last (the detail). Readers scan in a Z: + the top-left cell is the most valuable slot on the page. +- Group by question, not by chart type: a trend and its breakdown belong side + by side; two unrelated charts sharing a row invite false comparison. +- Matched granularity per row: charts in one row should share their time + window and grain, or say in the title why not. +- One message per chart. A chart needing a paragraph to explain wants to be + two charts, or a table. +- Titles state the answer where possible ("Orders fell 12% WoW"), the + question otherwise ("Orders by week"); never just a column name. Filtered + charts name their scope in the title. +- Tabs when sections answer different questions; scrolling when one question + deepens. Never a tab with a single lonely chart. +- Whitespace is markdown's job, sparingly: a one-line section header beats an + empty band (2 units is plenty for a header). Consistent number formats per + measure across KPI cards (tables and pivots use Superset's smart default; + the spec cannot set per-column formats yet). +- Color restraint: Superset's default palette, RAG only where a threshold has + a real business meaning; never encode the same dimension with two palettes. +- RAG polarity is a convention, not a choice: red = adverse, green = good + (IBCS). Bands on one metric must be disjoint and tell one story. +- Heatmaps ship with a sequential scale normalized over the whole map: right + for magnitudes, wrong for signed deltas -- don't heatmap a metric that + crosses zero. diff --git a/chartwright/design/model.py b/chartwright/design/model.py new file mode 100644 index 0000000..1523254 --- /dev/null +++ b/chartwright/design/model.py @@ -0,0 +1,291 @@ +"""The critic's data model: findings, the rule registry, and a normalized +view of the layout (rows, tabs, and sketches all become the same bands) so +rules are written once, not per layout mode. +""" + +from __future__ import annotations + +import re +from dataclasses import asdict, dataclass, field +from typing import Callable, Iterator + +from ..resolver import ResolvedDataset, Resolution +from ..spec import DEFAULT_HEIGHT, DashboardSpec, MarkdownBlock + +DESIGN_BRAIN_VERSION = "2" + +KPI_TYPES = {"big_number_total", "big_number_trend"} +TIMESERIES_TYPES = {"timeseries_line", "timeseries_bar", "timeseries_area", "timeseries_scatter"} +AXIS_TYPES = TIMESERIES_TYPES | {"bar", "heatmap", "histogram"} +# Charts that are neither KPI nor axis-bearing; a contract test asserts the +# three sets exactly cover CHART_TYPES, so a 15th chart type fails CI until +# someone consciously classifies it (and reviews which rules apply). +STANDALONE_TYPES = {"pie", "table", "pivot_table", "funnel", "treemap"} + +# Renamed rule ids keep working in ignore/disable lists forever. +RULE_ALIASES: dict[str, str] = {} + + +@dataclass +class Finding: + rule: str + severity: str # error | warn | info + chart: str | None + where: str + detail: str + fix: dict | None = None # {"chart": name, "set": {field: value}} -- presentation only + # True for complaints ABOUT a chart's height: only these yield to a + # human-polished (fractional, absorb-written) height. Width and data + # complaints survive polish -- absorb can never write widths. + height_driven: bool = False + + @property + def key(self) -> str: + return f"{self.rule}@{self.chart}" if self.chart else self.rule + + @property + def scope_key(self) -> str: + """Positional key for band findings (no chart to name): + 'layout.kpi-band@tab-Ops-row-1'. Accepted in ignore lists alongside + the rule id and rule@Chart forms.""" + slug = re.sub(r"[^A-Za-z0-9]+", "-", self.where).strip("-") + return f"{self.rule}@{slug}" + + def as_dict(self) -> dict: + d = asdict(self) + d.pop("fix") + d.pop("height_driven") + d["fixable"] = self.fix is not None + return d + + +@dataclass +class AdviceReport: + ok: bool # False iff any error-severity finding + audience: str + findings: list[Finding] = field(default_factory=list) + fixed: list[str] = field(default_factory=list) + ignored: list[str] = field(default_factory=list) + + @property + def counts(self) -> dict: + c = {"error": 0, "warn": 0, "info": 0} + for f in self.findings: + c[f.severity] += 1 + return c + + def gate(self, strict: bool) -> bool: + """True when this report should block under the given strictness.""" + c = self.counts + return c["error"] > 0 or (strict and c["warn"] > 0) + + unmatched_ignores: list[str] = field(default_factory=list) + # Findings withheld because the chart carries a human-polished (fractional) + # height. Reported, never silent: a rule that stands down on an INFERRED + # signal has to say so, or the user reads the silence as approval. + polished: list[str] = field(default_factory=list) + + def payload(self) -> dict: + out = { + "stage": "design", + "ok": self.ok, + "design_brain": DESIGN_BRAIN_VERSION, + "audience": self.audience, + "counts": self.counts, + "findings": [f.as_dict() for f in self.findings], + "fixed": self.fixed, + "ignored": self.ignored, + } + if self.unmatched_ignores: + out["unmatched_ignores"] = self.unmatched_ignores + if self.polished: + out["polished"] = self.polished + return out + + +# -- normalized layout -------------------------------------------------------- + + +@dataclass +class BandItem: + width: int + height: float # visual height in the band (a stack sums its charts) + charts: list[str] # 1 name for a chart, n for a sketch stack, 0 for markdown + is_markdown: bool = False + + +@dataclass +class Band: + items: list[BandItem] + + @property + def height(self) -> float: + return max((i.height for i in self.items), default=0.0) + + @property + def chart_names(self) -> list[str]: + return [n for i in self.items for n in i.charts] + + +@dataclass +class Section: + title: str | None # tab title, None for a flat layout + mode: str # "rows" | "sketch" + bands: list[Band] + + +@dataclass +class Geo: + width: int + height: float + section: int + band: int + + +class RuleContext: + def __init__(self, spec: DashboardSpec, params, resolution: Resolution | None = None, + prober=None): + self.spec = spec + self.params = params + self.resolution = resolution + self.prober = prober + self.charts = {c.name: c for c in spec.charts} + self._heights: dict[str, float] = {} + self.sections: list[Section] = _normalize(spec) + self.geo: dict[str, Geo] = {} + for si, sec in enumerate(self.sections): + for bi, band in enumerate(sec.bands): + for item in band.items: + for name in item.charts: + self.geo[name] = Geo(item.width, self.height(name), si, bi) + + # -- lookups --------------------------------------------------------------- + + def height(self, name: str) -> float: + h = self._heights.get(name) + if h is None: + h = self._heights[name] = self.spec.resolved_height(name) + return h + + def width(self, name: str) -> int: + return self.geo[name].width + + def where(self, name: str) -> str: + g = self.geo[name] + sec = self.sections[g.section] + prefix = f"tab {sec.title!r} " if sec.title else "layout " + return f"{prefix}row {g.band}" + + def where_band(self, si: int, bi: int) -> str: + sec = self.sections[si] + prefix = f"tab {sec.title!r} " if sec.title else "layout " + return f"{prefix}row {bi}" + + def is_sketch(self, name: str) -> bool: + return self.sections[self.geo[name].section].mode == "sketch" + + def human_polished(self, name: str) -> bool: + """Fractional heights are absorb's signature: a human dragged this + chart to taste in the UI. Sizing rules stay silent on it.""" + h = self.charts[name].height + return isinstance(h, float) and not float(h).is_integer() + + def dataset_for(self, chart) -> ResolvedDataset | None: + if self.resolution is None: + return None + return self.resolution.datasets.get(chart.dataset.key()) + + def fix_height(self, chart, floor: float) -> dict: + """A height-raise fix honoring calibrated recommended heights. + + Targets are ceiled to whole units and clamped to the spec's height cap: + fractional heights are absorb's human-polish signature, and a tool- + written fix must never mint it (or the fix would silence the very + rules that produced it -- the echo chamber).""" + import math + + target = max(floor, self.params.recommended_heights.get(chart.type, 0)) + return {"chart": chart.name, "set": {"height": min(100, math.ceil(target))}} + + +# -- registry ----------------------------------------------------------------- + + +@dataclass +class Rule: + id: str + severity: str # the rule's DEFAULT level; individual findings may + # vary it (e.g. row-density escalates to error when + # a chart is starved), and the overlay's `severity` + # map overrides it per deployment. + doc: str # one line; the brief prints these + fixable: bool + data_aware: bool # needs a live resolution (skipped offline) + fn: Callable[[RuleContext], Iterator[Finding]] + since: str = "1" # design_brain version that introduced the rule + + +RULES: dict[str, Rule] = {} + + +def rule(id: str, severity: str, doc: str, fixable: bool = False, data_aware: bool = False, + since: str = "1"): + def deco(fn): + RULES[id] = Rule(id, severity, doc, fixable, data_aware, fn, since) + return fn + return deco + + +def known_rule_ids() -> set[str]: + return set(RULES) | set(RULE_ALIASES) + + +def canonical_rule_id(rule_id: str) -> str: + return RULE_ALIASES.get(rule_id, rule_id) + + +# -- layout normalization ----------------------------------------------------- + + +def _normalize(spec: DashboardSpec) -> list[Section]: + lay = spec.layout + if lay.rows: + return [Section(None, "rows", _bands_from_rows(spec, lay.rows))] + if lay.sketch: + return [Section(None, "sketch", _bands_from_sketch(spec, lay))] + sections = [] + for tab in lay.tabs or []: + if tab.rows: + sections.append(Section(tab.title, "rows", _bands_from_rows(spec, tab.rows))) + else: + sections.append(Section(tab.title, "sketch", _bands_from_sketch(spec, tab))) + return sections + + +def _bands_from_rows(spec: DashboardSpec, rows) -> list[Band]: + bands = [] + for row in rows: + items = [] + for item in row: + w = spec.resolved_item_width(item) + if isinstance(item, MarkdownBlock): + items.append(BandItem(w, item.height or DEFAULT_HEIGHT["markdown"], [], True)) + else: + items.append(BandItem(w, spec.resolved_height(item), [item])) + bands.append(Band(items)) + return bands + + +def _bands_from_sketch(spec: DashboardSpec, holder) -> list[Band]: + bands = [] + for srow in holder.parsed_sketch(): + items = [] + for child in srow.children: + if hasattr(child, "children"): # SketchColumn: a stack of charts + names = [sc.name for sc in child.children] + total = sum(spec.resolved_height(n) for n in names) + items.append(BandItem(child.width, total, names)) + else: + items.append(BandItem(child.width, spec.resolved_height(child.name), [child.name])) + bands.append(Band(items)) + return bands diff --git a/chartwright/design/presets.py b/chartwright/design/presets.py new file mode 100644 index 0000000..959c037 --- /dev/null +++ b/chartwright/design/presets.py @@ -0,0 +1,181 @@ +"""Audience presets + the house-style overlay. + +Rules never test the audience name, only parameters: adding an audience is +adding a row here. The optional overlay file (~/.config/chartwright/design.yaml, +or $CHARTWRIGHT_DESIGN_DIR/design.yaml) lets an org tune parameters, disable +rules, append house guidance to the brief, and carry heights learned by +`chartwright calibrate` -- without forking the rulebook. +""" + +from __future__ import annotations + +import os +from dataclasses import dataclass, field, fields, replace +from pathlib import Path + +DEFAULT_AUDIENCE = "analytical" + + +@dataclass(frozen=True) +class Params: + audience: str + fold_units: int # height budget per tab (1 unit = 40 px) + max_row_charts: int # axis charts per band + kpi_row_min: int + kpi_row_max: int + kpi_height: int + min_axis_height: int + # Min visible rows / row_limit. 0.5 everywhere: below half, the MAJORITY of + # the rows you deliberately asked for sit behind the grid's inner + # scrollbar -- the exact defect size.grid-fit and apply-time smoke exist to + # catch, so the offline rule must not bless it. Still a knob (deployments + # override per audience in design.yaml), just no longer a lenient default. + table_visible_ratio: float + vbar_max_categories: int + pie_max_slices: int + series_max: int # lines per timeseries + max_filter_selects: int = 6 # select pickers in the native filter bar + # chart type -> height (spec units); calibrate/overlay feed this, + # size-rule autofixes target it. + recommended_heights: dict[str, float] = field(default_factory=dict) + + +# max_row_charts stays <= floor(12 / 3): the 3/12 minimum readable width is a +# hard floor (size.min-width errors below it), so a preset must never invite a +# row the rulebook's own error branch condemns. +AUDIENCES: dict[str, Params] = { + "executive": Params("executive", fold_units=22, max_row_charts=3, kpi_row_min=2, + kpi_row_max=5, kpi_height=5, min_axis_height=8, + table_visible_ratio=0.5, vbar_max_categories=6, + pie_max_slices=5, series_max=5, max_filter_selects=5), + "analytical": Params("analytical", fold_units=66, max_row_charts=4, kpi_row_min=2, + kpi_row_max=6, kpi_height=4, min_axis_height=6, + table_visible_ratio=0.5, vbar_max_categories=8, + pie_max_slices=7, series_max=10, max_filter_selects=6), + "operational": Params("operational", fold_units=22, max_row_charts=4, kpi_row_min=2, + kpi_row_max=8, kpi_height=3, min_axis_height=5, + table_visible_ratio=0.5, vbar_max_categories=8, + pie_max_slices=7, series_max=8, max_filter_selects=7), +} +AUDIENCE_NAMES = tuple(AUDIENCES) + + +@dataclass +class Overlay: + """Parsed design.yaml. Empty overlay == no file == today's behavior.""" + + params: dict = field(default_factory=dict) # applies to every audience + audiences: dict = field(default_factory=dict) # audience -> param dict + disable: list[str] = field(default_factory=list) # rule ids to suppress + brief_extra: str = "" # appended to the brief + recommended_heights: dict = field(default_factory=dict) # chart type -> units + severity: dict = field(default_factory=dict) # rule id -> error|warn|info + + +def design_dir() -> Path: + custom = os.environ.get("CHARTWRIGHT_DESIGN_DIR") + return Path(custom) if custom else Path.home() / ".config" / "chartwright" + + +def overlay_path() -> Path: + return design_dir() / "design.yaml" + + +def _check_heights(rec, where: str) -> dict: + """recommended_heights must be {chart type: 1..100}; the fix loop targets + these values, so a bad entry would corrupt specs and blame the user.""" + if not isinstance(rec, dict): + raise ValueError(f"design overlay: {where} must be a mapping of chart type -> height") + for t, h in rec.items(): + if not isinstance(h, (int, float)) or isinstance(h, bool) or not 1 <= h <= 100: + raise ValueError( + f"design overlay: {where}[{t!r}] must be a number in 1..100, got {h!r}") + return rec + + +def _check_param_block(block, where: str) -> dict: + if not isinstance(block, dict): + raise ValueError(f"design overlay: {where} must be a mapping") + valid = {f.name for f in fields(Params)} - {"audience"} + bad = sorted(set(block) - valid) + if bad: + raise ValueError(f"design overlay: {where}: unknown parameters {bad} (known: {sorted(valid)})") + for k, v in block.items(): + if k == "recommended_heights": + _check_heights(v, f"{where}.recommended_heights") + elif not isinstance(v, (int, float)) or isinstance(v, bool): + raise ValueError(f"design overlay: {where}.{k} must be a number, got {v!r}") + return block + + +def load_overlay(path: Path | None = None) -> Overlay: + """Parse and VALIDATE design.yaml. The overlay is an org-wide, hand-edited + trust boundary: every value is checked here, once, with a typed error -- + never a TypeError three modules later.""" + p = path or overlay_path() + if not p.exists(): + return Overlay() + import yaml + + try: + data = yaml.safe_load(p.read_text(encoding="utf-8")) or {} + except yaml.YAMLError as e: + raise ValueError(f"design overlay {p} is not valid YAML: {e}") from e + if not isinstance(data, dict): + raise ValueError(f"design overlay {p} must be a YAML mapping") + known = {f.name for f in fields(Overlay)} + unknown = sorted(set(data) - known) + if unknown: + raise ValueError(f"design overlay {p}: unknown keys {unknown} (known: {sorted(known)})") + + ov = Overlay(**{k: v for k, v in data.items() if k in known}) + if not isinstance(ov.disable, list) or not all(isinstance(x, str) for x in ov.disable): + raise ValueError(f"design overlay {p}: disable must be a list of rule-id strings") + if not isinstance(ov.brief_extra, str): + raise ValueError(f"design overlay {p}: brief_extra must be a string") + _check_param_block(ov.params, "params") + _check_heights(ov.recommended_heights, "recommended_heights") + if not isinstance(ov.audiences, dict): + raise ValueError(f"design overlay {p}: audiences must be a mapping") + bad_aud = sorted(set(ov.audiences) - set(AUDIENCES)) + if bad_aud: + raise ValueError( + f"design overlay {p}: unknown audiences {bad_aud} (known: {sorted(AUDIENCES)})") + for aud, block in ov.audiences.items(): + _check_param_block(block, f"audiences.{aud}") + if not isinstance(ov.severity, dict): + raise ValueError(f"design overlay {p}: severity must be a mapping of rule id -> level") + from .model import known_rule_ids # function-level: no import cycle + + known = known_rule_ids() + for rule_id, level in ov.severity.items(): + if rule_id not in known: + raise ValueError(f"design overlay {p}: severity for unknown rule {rule_id!r}") + if level not in ("error", "warn", "info"): + raise ValueError( + f"design overlay {p}: severity[{rule_id!r}] must be error|warn|info, got {level!r}") + return ov + + +def params_for(audience: str, overlay: Overlay | None = None) -> Params: + if audience not in AUDIENCES: + raise ValueError(f"unknown audience {audience!r}; one of {sorted(AUDIENCES)}") + p = AUDIENCES[audience] + overlay = overlay or Overlay() + per_audience = _check_param_block(overlay.audiences.get(audience) or {}, f"audiences.{audience}") + updates: dict = {} + updates.update(_check_param_block(overlay.params, "params")) + updates.update(per_audience) + # recommended_heights MERGES per key across all four layers (preset, + # overlay top-level, overlay params, overlay per-audience) -- a wholesale + # replace would silently drop org-wide calibration on any audience that + # tunes a single type. + rec = { + **p.recommended_heights, + **_check_heights(overlay.recommended_heights, "recommended_heights"), + **(overlay.params.get("recommended_heights") or {}), + **(per_audience.get("recommended_heights") or {}), + } + if rec: + updates["recommended_heights"] = rec + return replace(p, **updates) if updates else p diff --git a/chartwright/design/probe.py b/chartwright/design/probe.py new file mode 100644 index 0000000..452dcdb --- /dev/null +++ b/chartwright/design/probe.py @@ -0,0 +1,61 @@ +"""Bounded cardinality probes for data-aware rules. + +One grouped COUNT per (dataset, column), capped: rules only ever need +"more than N distinct values", never the true count, so the query carries +row_limit = cap + 1 and the answer is min(cardinality, cap + 1). Probes are +cached per run and any HTTP/parse failure degrades to None (the rule skips; +advice never fails a pipeline over a probe). +""" + +from __future__ import annotations + +from ..client import SupersetAPIError, SupersetClient +from ..resolver import ResolvedDataset + +_COUNT_METRIC = {"expressionType": "SQL", "sqlExpression": "COUNT(*)", "label": "count", + "optionName": "metric_sdc_probe"} + + +class CardinalityProber: + def __init__(self, client: SupersetClient): + self.client = client + self._cache: dict[tuple[int, str, int], int | None] = {} + + def count_up_to(self, ds: ResolvedDataset, column: str, cap: int) -> int | None: + """Distinct values in `column`, saturating at cap + 1; None on failure.""" + key = (ds.id, column, cap) + if key in self._cache: + return self._cache[key] + # A smaller answer under a bigger cap is exact; reuse it. + for (i, c, k), v in self._cache.items(): + if i == ds.id and c == column and v is not None and (v <= k or k >= cap): + self._cache[key] = min(v, cap + 1) + return self._cache[key] + ctx = { + "datasource": {"id": ds.id, "type": "table"}, + "queries": [{ + "columns": [column], + "metrics": [_COUNT_METRIC], + "filters": [], + "orderby": [], + "row_limit": cap + 1, + "time_range": "No filter", + }], + "result_format": "json", + "result_type": "full", + } + try: + r = self.client.chart_data(ctx) + if r.status_code != 200: + self._cache[key] = None + return None + result = r.json()["result"] + n = sum(len(q.get("data") or []) for q in result) + except (SupersetAPIError, KeyError, ValueError): + n = None + self._cache[key] = n + return n + + def more_than(self, ds: ResolvedDataset, column: str, n: int) -> bool | None: + c = self.count_up_to(ds, column, n) + return None if c is None else c > n diff --git a/chartwright/design/redesign.py b/chartwright/design/redesign.py new file mode 100644 index 0000000..1ba0c8c --- /dev/null +++ b/chartwright/design/redesign.py @@ -0,0 +1,53 @@ +"""One-shot redesign: decompile -> audit -> safe fixes -> redesigned spec. + +The core is client-free: it takes an already-decompiled spec and applies the +brain. Ownership decides where the result can land: a tool-born dashboard +redesigns in place (same slug, chart ids stable); a UI-born one gets a +`-redesign` slug so apply builds it side by side and the original is never +touched (the ownership guard would refuse the original slug anyway). + +Structural findings (re-composition, chart-type swaps) stay findings: the +spec author acts on them between redesign and apply. Advice, not authority. +""" + +from __future__ import annotations + +import copy + +from . import advise_and_fix + + +def redesign_spec(spec_data: dict, losses: list, *, owned: bool, + audience: str | None = None, ignore: tuple[str, ...] = (), + resolution=None, prober=None, overlay=None) -> tuple[dict, dict]: + """Returns (redesigned spec data, report payload). Raises ValueError only + for a broken design overlay; everything else is reported in the payload.""" + data = spec_data + slug_changed = False + if not owned: + data = copy.deepcopy(spec_data) + data["dashboard"]["slug"] += "-redesign" + data["dashboard"]["title"] += " (redesigned)" + slug_changed = True + + new_data, report = advise_and_fix( + data, audience=audience, ignore=ignore, + resolution=resolution, prober=prober, overlay=overlay) + + remaining = [f for f in report.findings if f.severity != "info"] + payload = { + "stage": "redesign", + "ok": report.ok, + "slug": new_data["dashboard"]["slug"], + "slug_changed": slug_changed, + "losses": losses, + "advice": report.payload(), + "next": ( + ("the original is not tool-owned; this spec applies SIDE BY SIDE under " + f"slug {new_data['dashboard']['slug']!r}. " if slug_changed else "") + + (f"{len(report.fixed)} geometry fix(es) applied. " if report.fixed else "") + + (f"{len(remaining)} structural finding(s) remain: edit the spec for them, then apply." + if remaining else "no structural findings: review the spec, then apply.") + ), + } + return new_data, payload diff --git a/chartwright/design/rules.py b/chartwright/design/rules.py new file mode 100644 index 0000000..f580fd8 --- /dev/null +++ b/chartwright/design/rules.py @@ -0,0 +1,1042 @@ +"""The rulebook: every Tier L (lintable) design rule. + +Grounded in BI practice (Few, Tufte, IBCS) and Superset rendering facts the +skill learned the hard way (axis charts flatten below ~6 units; vertical bars +drop labels past ~8 categories; pies crowd under 5/12 width). Each rule yields +Findings; thresholds come from the audience Params, never hard-coded branches +on audience names. Autofixes are presentation-only by principle: geometry and +orientation, never row limits, filters, or chart types. + +Rule ids are a stable public surface (spec `design.ignore` keys on them). +""" + +from __future__ import annotations + +import json +import math +import re +from datetime import date + +from ..spec import ( + DEFAULT_TIME_GRAIN, + GRID_HEADER_UNITS, + grid_rows_visible, + grid_units_for_rows, +) +from .model import AXIS_TYPES, KPI_TYPES, TIMESERIES_TYPES, Finding, RuleContext, rule + +# -- size: minimum readable geometry ------------------------------------------ + + +@rule("size.min-width", "warn", "below 3/12 width a chart is unreadable; KPIs need 2/12", since="2") +def min_width(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type in ("pie", "heatmap"): + continue # they own stricter geometry rules + w = ctx.width(c.name) + if c.type in KPI_TYPES: + if w < 2: + yield Finding( + "size.min-width", "warn", c.name, ctx.where(c.name), + f"big number at {w}/12: the value gets cropped; give it >= 2/12", + ) + elif w < 3: + yield Finding( + "size.min-width", "error", c.name, ctx.where(c.name), + f"{c.type} at {w}/12 is unreadable at any height; 3/12 is the hard floor", + ) + elif w < 4: + yield Finding( + "size.min-width", "warn", c.name, ctx.where(c.name), + f"{c.type} at {w}/12 is cramped; 4/12 or wider reads comfortably", + ) + + +@rule("size.axis-min-height", "warn", "axis charts below the audience minimum height flatten and drop labels", fixable=True) +def axis_min_height(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type not in AXIS_TYPES or c.type == "heatmap": + continue # heatmap-geometry owns the heatmap's stricter floor + if c.type == "bar" and c.orientation == "horizontal" and c.row_limit is not None: + continue # hbar-window's per-bar formula binds instead + h = ctx.height(c.name) + if h < ctx.params.min_axis_height: + yield Finding( + "size.axis-min-height", "warn", c.name, ctx.where(c.name), + f"{c.type} at {h:g} units renders flattened with axis labels dropped; " + f"needs >= {ctx.params.min_axis_height}", + fix=ctx.fix_height(c, ctx.params.min_axis_height), + height_driven=True, + ) + + +@rule("size.kpi-height", "warn", "big numbers read best at 2-6 units", fixable=True) +def kpi_height(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type not in KPI_TYPES: + continue + h = ctx.height(c.name) + if not 2 <= h <= 6: + target = min(6, max(2, math.ceil(max( + ctx.params.kpi_height, ctx.params.recommended_heights.get(c.type, 0))))) + yield Finding( + "size.kpi-height", "warn", c.name, ctx.where(c.name), + f"big number at {h:g} units ({'starved' if h < 2 else 'wastes hero space'}); " + f"2-6 reads best", + fix={"chart": c.name, "set": {"height": target}}, + height_driven=True, + ) + + +@rule("size.pie-geometry", "warn", "pies need >= 5/12 width and 8 height or the ring shrinks and the legend crowds", fixable=True) +def pie_geometry(ctx: RuleContext): + # Width and height are SEPARATE findings: a human-polished (fractional) + # height silences only the height complaint; absorb can't write widths. + for c in ctx.spec.charts: + if c.type != "pie": + continue + w, h = ctx.width(c.name), ctx.height(c.name) + if w < 5: + hint = ("widen its sketch run to >= 5 of 12 cells" if ctx.is_sketch(c.name) + else "widen it to >= 5 of 12") + yield Finding( + "size.pie-geometry", "warn", c.name, ctx.where(c.name), + f"pie squeezed: width {w}/12 ({hint}); ring shrinks and legend crowds", + ) + if h < 8: + yield Finding( + "size.pie-geometry", "warn", c.name, ctx.where(c.name), + f"pie squeezed: height {h:g} < 8; ring shrinks and legend crowds", + fix=ctx.fix_height(c, 8), height_driven=True, + ) + + +@rule("size.heatmap-geometry", "warn", "heatmaps need >= 5/12 width (7/12 with many columns) and 6 height", fixable=True) +def heatmap_geometry(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type != "heatmap": + continue + w, h = ctx.width(c.name), ctx.height(c.name) + min_w = 5 + if ctx.prober is not None and (ds := ctx.dataset_for(c)): + if ctx.prober.more_than(ds, c.x_column, 12): + min_w = 7 + if w < min_w: + yield Finding( + "size.heatmap-geometry", "warn", c.name, ctx.where(c.name), + f"heatmap cramped: width {w}/12 < {min_w}" + + (" (x has > 12 columns)" if min_w == 7 else ""), + ) + if h < 6: + yield Finding( + "size.heatmap-geometry", "warn", c.name, ctx.where(c.name), + f"heatmap cramped: height {h:g} below the readable floor of 6 (8 recommended)", + fix=ctx.fix_height(c, 8), height_driven=True, + ) + + +@rule("size.table-window", "warn", "a table's height should show a meaningful share of its row_limit") +def table_window(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type != "table" or c.row_limit is None: + continue + h = ctx.height(c.name) + # Same grid model as size.grid-fit and apply-time smoke: offline this + # can only reason about row_limit (the ceiling), where grid-fit probes + # the real count -- but both now measure a row the same way, so they + # can no longer give one chart contradictory verdicts. + visible = grid_rows_visible(h) + want = ctx.params.table_visible_ratio * c.row_limit + if visible < want: + yield Finding( + "size.table-window", "warn", c.name, ctx.where(c.name), + f"table shows ~{visible:.0f} of {c.row_limit} rows at {h:g} units " + f"(a scroll dungeon); raise height to " + f"~{math.ceil(grid_units_for_rows(want))} or lower row_limit", + fix=ctx.fix_height(c, math.ceil(grid_units_for_rows(want))) + if grid_units_for_rows(want) <= 20 else None, + height_driven=True, + ) + + +@rule("size.hbar-window", "warn", "horizontal bars need ~0.5 units of height per bar", fixable=True) +def hbar_window(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type != "bar" or c.orientation != "horizontal" or c.row_limit is None: + continue + h = ctx.height(c.name) + needed = c.row_limit * 0.5 + 2 + if h >= needed: + continue + fix = ctx.fix_height(c, math.ceil(needed)) if needed <= 20 else None + yield Finding( + "size.hbar-window", "warn", c.name, ctx.where(c.name), + f"{c.row_limit} bars in {h:g} units squeezes each bar; needs ~{math.ceil(needed)}" + + ("" if fix else f"; that exceeds a sane height, lower row_limit instead"), + fix=fix, height_driven=True, + ) + + +@rule("size.pivot-window", "warn", "a pivot's height should show a meaningful share of its row_limit", since="2") +def pivot_window(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type != "pivot_table" or c.row_limit is None: + continue + h = ctx.height(c.name) + header = GRID_HEADER_UNITS + (1 if c.columns else 0) # nested column headers cost one more + visible = grid_rows_visible(h, header) + want = ctx.params.table_visible_ratio * c.row_limit + if visible < want: + yield Finding( + "size.pivot-window", "warn", c.name, ctx.where(c.name), + f"pivot shows ~{visible:.0f} of {c.row_limit} rows at {h:g} units " + f"({header:g} header units); raise height to " + f"~{math.ceil(grid_units_for_rows(want, header))} or lower row_limit", + fix=ctx.fix_height(c, math.ceil(grid_units_for_rows(want, header))) + if grid_units_for_rows(want, header) <= 20 else None, + height_driven=True, + ) + + +@rule("size.grid-fit", "warn", "table/pivot heights must fit their data-driven row counts (they grow after authoring)", + fixable=True, data_aware=True, since="2") +def grid_fit(ctx: RuleContext): + """Pre-apply half of issue #1 (the apply-time half lives in smoke): probe + the actual dimension cardinality and check the configured height fits. + Honest scope: single row-dimension pivots and single-groupby aggregate + tables -- multi-dim leaf counts aren't knowable from per-column probes.""" + if ctx.prober is None: + return + for c in ctx.spec.charts: + if c.type == "pivot_table" and len(c.rows) == 1: + dim, extra_header = c.rows[0], (1 if c.columns else 0) + elif c.type == "table" and (c.groupby or []) and len(c.groupby) == 1 and not c.columns: + dim, extra_header = c.groupby[0], 0 + else: + continue + ds = ctx.dataset_for(c) + if ds is None: + continue + cap = c.row_limit or 60 + n = ctx.prober.count_up_to(ds, dim, min(cap, 60)) + if n is None: + continue + n = min(n, cap) + # Shared grid model (chartwright/spec.py), same numbers smoke uses. + needed = math.ceil(grid_units_for_rows(n, GRID_HEADER_UNITS + extra_header)) + h = ctx.height(c.name) + if needed <= h: + continue + yield Finding( + "size.grid-fit", "warn", c.name, ctx.where(c.name), + f"{dim!r} yields ~{n} rendered rows needing ~{needed} units; height {h:g} " + f"hides the tail behind an inner scrollbar -- and row counts grow with the " + f"data, so this only gets worse", + fix=ctx.fix_height(c, needed) if needed <= 20 else None, + height_driven=True, + ) + + +@rule("size.row-harmony", "warn", "charts sharing a row should share a height (Superset sizes the row to its tallest child)", fixable=True) +def row_harmony(ctx: RuleContext): + for si, sec in enumerate(ctx.sections): + if sec.mode != "rows": + continue # sketch rows draw their raggedness deliberately (dots) + for bi, band in enumerate(sec.bands): + names = [n for n in band.chart_names if ctx.charts[n].type not in KPI_TYPES] + if len(names) < 2: + continue + heights = {n: ctx.height(n) for n in names} + top = max(heights.values()) + # ceil: a fractional band max is a human-polished neighbor, and a + # tool fix must never mint the fractional human-polish signature. + target = min(100, math.ceil(top)) + for n, h in heights.items(): + if h < top: + yield Finding( + "size.row-harmony", "warn", n, ctx.where_band(si, bi), + f"{h:g} units beside a {top:g}-unit neighbor leaves a ragged hole; " + f"equalize to {target:g}", + fix={"chart": n, "set": {"height": target}}, + height_driven=True, + ) + + +# -- layout: composition ------------------------------------------------------- + + +@rule("layout.kpi-first", "warn", "summary KPIs belong above detail charts (inverted pyramid)") +def kpi_first(ctx: RuleContext): + for si, sec in enumerate(ctx.sections): + first_detail = None + for bi, band in enumerate(sec.bands): + if any(ctx.charts[n].type not in KPI_TYPES for n in band.chart_names): + first_detail = bi + break + if first_detail is None: + continue + late = [n for bi, band in enumerate(sec.bands) if bi > first_detail + for n in band.chart_names if ctx.charts[n].type in KPI_TYPES] + if late: + yield Finding( + "layout.kpi-first", "warn", None, ctx.where_band(si, first_detail), + f"big numbers {late} sit below detail charts; lead with the summary band", + ) + + +@rule("layout.kpi-band", "warn", "KPIs get their own band, in readable numbers") +def kpi_band(ctx: RuleContext): + # Reasoning is per horizontal SLOT (a BandItem), not per flattened chart: + # a vertical stack of KPIs beside a hero chart is the canonical sidebar + # pattern the sketch grammar exists to express, not a mixed band. + p = ctx.params + for si, sec in enumerate(ctx.sections): + for bi, band in enumerate(sec.bands): + kpis = [n for n in band.chart_names if ctx.charts[n].type in KPI_TYPES] + others = [n for n in band.chart_names if ctx.charts[n].type not in KPI_TYPES] + if kpis and others: + # Offenders are bare full-height KPI slots beside detail slots; + # KPI-only stacks (sidebars) are exempt. + bare = [i.charts[0] for i in band.items + if len(i.charts) == 1 and ctx.charts[i.charts[0]].type in KPI_TYPES] + mixed_stacks = [i for i in band.items if len(i.charts) > 1 + and any(ctx.charts[n].type in KPI_TYPES for n in i.charts) + and any(ctx.charts[n].type not in KPI_TYPES for n in i.charts)] + if bare: + yield Finding( + "layout.kpi-band", "warn", None, ctx.where_band(si, bi), + f"big numbers {bare} sit full-height beside detail charts; give KPIs " + f"their own band, or stack them in a column beside the tall chart", + ) + elif mixed_stacks: + yield Finding( + "layout.kpi-band", "warn", None, ctx.where_band(si, bi), + "a stack mixes big numbers with detail charts; keep stacks homogeneous", + ) + elif kpis: + has_md = any(i.is_markdown for i in band.items) + if len(kpis) > p.kpi_row_max: + yield Finding( + "layout.kpi-band", "warn", None, ctx.where_band(si, bi), + f"{len(kpis)} KPIs in one band reads as noise; keep <= {p.kpi_row_max}", + ) + elif len(kpis) < p.kpi_row_min and not has_md: + yield Finding( + "layout.kpi-band", "warn", None, ctx.where_band(si, bi), + f"a band of {len(kpis)} KPI looks unfinished; aim for " + f"{p.kpi_row_min}-{p.kpi_row_max} or pair it with a markdown note", + ) + + +@rule("layout.row-density", "warn", "too many axis charts side by side starves each of width") +def row_density(ctx: RuleContext): + # Horizontal SLOTS, not flattened charts: a stack of three charts occupies + # one slot's width, so it counts once (the user already split vertically). + for si, sec in enumerate(ctx.sections): + for bi, band in enumerate(sec.bands): + slots = [i for i in band.items + if any(ctx.charts[n].type in AXIS_TYPES for n in i.charts)] + if len(slots) <= ctx.params.max_row_charts: + continue + sev = "error" if any(i.width < 3 for i in slots) else "warn" + yield Finding( + "layout.row-density", sev, None, ctx.where_band(si, bi), + f"{len(slots)} side-by-side slots with axis charts " + f"(max {ctx.params.max_row_charts}); " + + ("some land under 3/12 wide, unreadable" if sev == "error" + else "split across rows"), + ) + + +@rule("layout.row-fill", "warn", "a row should fill the 12-column grid") +def row_fill(ctx: RuleContext): + for si, sec in enumerate(ctx.sections): + if sec.mode != "rows": + continue # sketches mark holes deliberately with '.' + for bi, band in enumerate(sec.bands): + total = sum(i.width for i in band.items) + missing = 12 - total + if missing <= 0: + continue + yield Finding( + "layout.row-fill", "warn" if missing >= 3 else "info", None, + ctx.where_band(si, bi), + f"row widths sum to {total}/12, leaving a {missing}-column hole at the right; " + f"widen a chart or add one", + ) + + +@rule("layout.fold-budget", "warn", "the dashboard should fit its audience's scroll budget") +def fold_budget(ctx: RuleContext): + for si, sec in enumerate(ctx.sections): + total = 0.0 + for bi, band in enumerate(sec.bands): + total += band.height + if total > ctx.params.fold_units: + where = f"tab {sec.title!r}" if sec.title else "the dashboard" + yield Finding( + "layout.fold-budget", "warn", None, ctx.where_band(si, bi), + f"{where} runs {total:g}+ units against a {ctx.params.audience} budget of " + f"{ctx.params.fold_units} (~{ctx.params.fold_units * 40}px); the budget runs " + f"out at row {bi}; move detail into tabs or prune", + ) + break + + +@rule("layout.tab-balance", "info", "tabs should carry comparable weight") +def tab_balance(ctx: RuleContext): + if len(ctx.sections) < 2: + return + counts = {s.title: sum(len(b.chart_names) for b in s.bands) for s in ctx.sections} + lo, hi = min(counts.values()), max(counts.values()) + if hi >= 5 and hi > 4 * max(1, lo): + thin = [t for t, n in counts.items() if n == lo] + yield Finding( + "layout.tab-balance", "info", None, "tabs", + f"tab weight skews {hi}:{lo} ({counts}); rebalance or inline the thin tab(s) {thin}", + ) + + +@rule("layout.orphan-chart", "info", "a lone narrow chart in its own row looks unfinished") +def orphan_chart(ctx: RuleContext): + for si, sec in enumerate(ctx.sections): + if sec.mode != "rows": + continue + for bi, band in enumerate(sec.bands): + if len(band.items) == 1 and band.items[0].charts and band.items[0].width < 8: + yield Finding( + "layout.orphan-chart", "info", band.items[0].charts[0], + ctx.where_band(si, bi), + f"alone in its row at {band.items[0].width}/12; widen it to 12 or pair it", + ) + + +@rule("layout.section-headers", "info", "large flat dashboards need markdown signposts") +def section_headers(ctx: RuleContext): + if len(ctx.sections) != 1 or ctx.sections[0].mode != "rows" or ctx.sections[0].title: + return + n = len(ctx.spec.charts) + has_md = any(i.is_markdown for b in ctx.sections[0].bands for i in b.items) + if n > 8 and not has_md: + yield Finding( + "layout.section-headers", "info", None, "layout", + f"{n} charts with no markdown section headers; readers need signposts " + f"(or split into tabs)", + ) + + +# -- chart: encoding choice ---------------------------------------------------- + + +@rule("chart.vbar-categories", "warn", "vertical bars drop labels past ~8 categories; rank with horizontal bars", fixable=True) +def vbar_categories(ctx: RuleContext): + p = ctx.params + for c in ctx.spec.charts: + if c.type != "bar" or c.orientation != "vertical": + continue + rl = c.row_limit + if rl is not None and rl <= p.vbar_max_categories: + continue + if ctx.prober is not None and (ds := ctx.dataset_for(c)): + if ctx.prober.more_than(ds, c.x_column, p.vbar_max_categories) is False: + continue # the data itself stays under the label limit + fix = ({"chart": c.name, "set": {"orientation": "horizontal"}} + if rl is not None and rl <= 15 else None) + yield Finding( + "chart.vbar-categories", "warn", c.name, ctx.where(c.name), + (f"vertical bar with row_limit {rl}" if rl is not None + else "vertical bar with no row_limit (defaults to 10,000)") + + f": Superset drops category labels past ~{p.vbar_max_categories}; " + "flip to horizontal and cap around 10", + fix=fix, + ) + + +@rule("chart.pie-slices", "warn", "pies stop working past ~7 slices") +def pie_slices(ctx: RuleContext): + p = ctx.params + for c in ctx.spec.charts: + if c.type != "pie": + continue + rl = c.row_limit + if rl is not None and rl <= p.pie_max_slices: + continue + if ctx.prober is not None and (ds := ctx.dataset_for(c)): + if ctx.prober.more_than(ds, c.groupby, p.pie_max_slices) is False: + continue + yield Finding( + "chart.pie-slices", "warn", c.name, ctx.where(c.name), + (f"pie with row_limit {rl}" if rl is not None + else "pie with no row_limit (defaults to 100)") + + f": more than {p.pie_max_slices} slices is unreadable. Prefer a horizontal " + f"bar (rankings don't claim to be a whole); if it must stay a pie, know that " + f"a row_limit redefines the whole -- the shown slices read as 100% -- so the " + f"title must disclose the truncation (e.g. 'top {p.pie_max_slices} ...')", + ) + + +@rule("chart.series-limit", "warn", "a timeseries with too many grouped series turns to spaghetti", data_aware=True) +def series_limit(ctx: RuleContext): + if ctx.prober is None: + return + for c in ctx.spec.charts: + if c.type not in TIMESERIES_TYPES or not c.groupby: + continue + ds = ctx.dataset_for(c) + if ds is None: + continue + if ctx.prober.more_than(ds, c.groupby, ctx.params.series_max): + yield Finding( + "chart.series-limit", "warn", c.name, ctx.where(c.name), + f"groupby {c.groupby!r} has more than {ctx.params.series_max} values: " + f"a line per value is spaghetti; filter to the top few or use a coarser dimension", + ) + + +@rule("chart.metrics-per-bar", "warn", "many metrics per category read better as a table") +def metrics_per_bar(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type == "bar" and len(c.metrics) >= 4: + yield Finding( + "chart.metrics-per-bar", "warn", c.name, ctx.where(c.name), + f"{len(c.metrics)} metrics per category makes grouped bars unreadable; " + f"a table or pivot answers this better", + ) + + +@rule("chart.temporal-type", "error", "a time axis must point at a temporal column", data_aware=True) +def temporal_type(ctx: RuleContext): + for c in ctx.spec.charts: + col = getattr(c, "time_column", None) + if not col: + continue + ds = ctx.dataset_for(c) + if ds is None: + continue + if ds.is_temporal(col) is False: + yield Finding( + "chart.temporal-type", "error", c.name, ctx.where(c.name), + f"time_column {col!r} is not a temporal column on {ds.table!r}; " + f"the chart will render broken or empty" + + (f" (dataset main time column: {ds.main_dttm_col!r})" if ds.main_dttm_col else ""), + ) + + +@rule("chart.histogram-bins", "info", "histograms read best at 10-50 bins") +def histogram_bins(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type == "histogram" and not 10 <= c.bins <= 50: + yield Finding( + "chart.histogram-bins", "info", c.name, ctx.where(c.name), + f"{c.bins} bins ({'too coarse' if c.bins < 10 else 'too noisy'}); 20-30 suits most distributions", + ) + + +@rule("chart.treemap-depth", "warn", "treemaps past two grouping levels become unreadable nesting") +def treemap_depth(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type == "treemap" and len(c.groupby) > 2: + yield Finding( + "chart.treemap-depth", "warn", c.name, ctx.where(c.name), + f"{len(c.groupby)} nesting levels; keep to 2 or break the hierarchy into charts", + ) + + +@rule("chart.funnel-stages", "warn", "funnels need 3-8 ordered stages", data_aware=True) +def funnel_stages(ctx: RuleContext): + if ctx.prober is None: + return + for c in ctx.spec.charts: + if c.type != "funnel": + continue + ds = ctx.dataset_for(c) + if ds is None: + continue + n = ctx.prober.count_up_to(ds, c.groupby, 8) + if n is None: + continue + if n > 8 and (c.row_limit is None or c.row_limit > 8): + yield Finding( + "chart.funnel-stages", "warn", c.name, ctx.where(c.name), + f"groupby {c.groupby!r} has more than 8 values; a funnel wants 3-8 ordered stages", + ) + elif n < 3: + yield Finding( + "chart.funnel-stages", "warn", c.name, ctx.where(c.name), + f"groupby {c.groupby!r} has only {n} value(s); a funnel wants 3-8 ordered stages", + ) + + +@rule("chart.heatmap-grid", "warn", "a heatmap past ~400 cells is unreadable at any size", data_aware=True) +def heatmap_grid(ctx: RuleContext): + if ctx.prober is None: + return + for c in ctx.spec.charts: + if c.type != "heatmap": + continue + ds = ctx.dataset_for(c) + if ds is None: + continue + cx = ctx.prober.count_up_to(ds, c.x_column, 30) + cy = ctx.prober.count_up_to(ds, c.y_column, 30) + if not cx or not cy: + continue + # A saturated side (count == cap+1) hides the true product: a 6,000x10 + # grid reads as 31x10 = 310 and would pass. Re-probe the saturated + # side at the cap the OTHER side implies before deciding. + if cx * cy <= 400 and (cx > 30 or cy > 30): + if cx > 30: + cx = ctx.prober.count_up_to(ds, c.x_column, math.ceil(400 / cy)) or cx + if cy > 30: + cy = ctx.prober.count_up_to(ds, c.y_column, math.ceil(400 / cx)) or cy + if cx * cy > 400: + at_least = "at least " if cx > 30 or cy > 30 else "~" + yield Finding( + "chart.heatmap-grid", "warn", c.name, ctx.where(c.name), + f"{at_least}{cx}x{cy} = {cx * cy}+ cells; filter or coarsen one axis " + f"to stay under ~400", + ) + + +@rule("chart.pivot-dims", "warn", "a pivot past three total dimensions is unreadable nesting", since="2") +def pivot_dims(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type == "pivot_table" and len(c.rows) + len(c.columns) > 3: + yield Finding( + "chart.pivot-dims", "warn", c.name, ctx.where(c.name), + f"{len(c.rows)} row + {len(c.columns)} column dimensions; keep the total " + f"to 3 or split the question across charts", + ) + + +@rule("chart.pivot-columns", "warn", "column-dim values x metrics = rendered columns; past ~15 the pivot scrolls sideways", data_aware=True, since="2") +def pivot_columns(ctx: RuleContext): + if ctx.prober is None: + return + for c in ctx.spec.charts: + if c.type != "pivot_table" or not c.columns: + continue + ds = ctx.dataset_for(c) + if ds is None: + continue + rendered = len(c.metrics) + for col in c.columns: + n = ctx.prober.count_up_to(ds, col, 16) + if n is None: + rendered = None + break + rendered *= n + if rendered is not None and rendered > 15: + yield Finding( + "chart.pivot-columns", "warn", c.name, ctx.where(c.name), + f"~{rendered}+ rendered columns ({len(c.metrics)} metric(s) x column-dim " + f"values); the pivot scrolls sideways -- filter the column dimension or " + f"move it to rows", + ) + + +@rule("chart.format-bands", "warn", "conditional-formatting bands must tell one coherent story per metric", since="2") +def format_bands(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type != "pivot_table" or not c.conditional_formatting: + continue + by_metric: dict[str, list] = {} + for r in c.conditional_formatting: + lo, hi = ((r.target_left, r.target_right) if r.operator == "between" + else (float("-inf"), r.target) if r.operator == "<" + else (r.target, float("inf"))) + by_metric.setdefault(r.metric, []).append((lo, hi, r.color)) + for metric, bands in by_metric.items(): + for i in range(len(bands)): + for j in range(i + 1, len(bands)): + (a0, a1, ca), (b0, b1, cb) = bands[i], bands[j] + if a0 < b1 and b0 < a1 and ca != cb: + yield Finding( + "chart.format-bands", "warn", c.name, ctx.where(c.name), + f"metric {metric!r}: {ca} and {cb} bands overlap " + f"(cell color depends on rule order, not the value); " + f"make the ranges disjoint", + ) + if len(bands) == 1: + yield Finding( + "chart.format-bands", "info", c.name, ctx.where(c.name), + f"metric {metric!r} has a single {bands[0][2]} band: one color is " + f"decoration, not a signal; band the full green/amber/red story " + f"or drop it", + ) + + +_ORDINAL_RE = re.compile(r"(^|_)(day|weekday|dow|month|quarter|hour)(_|$|name)", re.I) + + +@rule("chart.ordinal-order", "info", "ordinal dimensions (weekday, month) sort alphabetically unless order-encoded", since="2") +def ordinal_order(ctx: RuleContext): + def dims(c): + if c.type == "bar": + return [c.x_column] + if c.type == "heatmap": + return [c.x_column, c.y_column] + if c.type == "pivot_table": + return [*c.rows, *c.columns] + return [] + + for c in ctx.spec.charts: + hits = [d for d in dims(c) if d and _ORDINAL_RE.search(d)] + if hits: + yield Finding( + "chart.ordinal-order", "info", c.name, ctx.where(c.name), + f"{hits} look ordinal but Superset sorts categories alphabetically " + f"(Apr, Aug, Dec...); chart an order-encoded label column " + f"(e.g. '1-Mon') if the dataset has one", + ) + + +@rule("chart.treemap-vs-bar", "info", "a one-level treemap of few categories is a worse bar chart", data_aware=True, since="2") +def treemap_vs_bar(ctx: RuleContext): + if ctx.prober is None: + return + for c in ctx.spec.charts: + if c.type != "treemap" or len(c.groupby) != 1: + continue + ds = ctx.dataset_for(c) + if ds is None: + continue + if ctx.prober.more_than(ds, c.groupby[0], 10) is False: + yield Finding( + "chart.treemap-vs-bar", "info", c.name, ctx.where(c.name), + f"one grouping level with <= 10 values: a horizontal bar shows the " + f"same data with readable labels and comparable lengths", + ) + + +@rule("chart.dupe", "info", "two charts answering the identical question is redundancy") +def chart_dupe(ctx: RuleContext): + seen: dict[str, str] = {} + for c in ctx.spec.charts: + fp = json.dumps(c.model_dump(exclude={"name", "width", "height"}), + sort_keys=True, default=str) + if fp in seen: + yield Finding( + "chart.dupe", "info", c.name, ctx.where(c.name), + f"identical to {seen[fp]!r} (same dataset, type, metrics, dimensions, filters)", + ) + else: + seen[fp] = c.name + + +# -- data: query intent --------------------------------------------------------- + + +@rule("data.row-limit-intent", "info", "row limits doing design work should be deliberate, not defaults") +def row_limit_intent(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type == "table" and c.row_limit is None: + yield Finding( + "data.row-limit-intent", "info", c.name, ctx.where(c.name), + "table riding the default row_limit (1,000); set it deliberately", + ) + elif c.type == "bar" and c.orientation == "horizontal" and c.row_limit is None: + yield Finding( + "data.row-limit-intent", "info", c.name, ctx.where(c.name), + "horizontal bar with no row_limit (defaults to 10,000); a ranking wants ~10", + ) + elif c.type == "pivot_table" and c.row_limit is None: + yield Finding( + "data.row-limit-intent", "info", c.name, ctx.where(c.name), + "pivot riding the default row_limit (10,000); set it deliberately", + ) + + +@rule("data.top-n-sort", "warn", "a limit without an order is a sample, not a ranking", since="2") +def top_n_sort(ctx: RuleContext): + for c in ctx.spec.charts: + if (c.type == "table" and (c.metrics or c.groupby) and c.sort_by is None + and c.row_limit is not None and c.row_limit <= 100): + yield Finding( + "data.top-n-sort", "warn", c.name, ctx.where(c.name), + f"row_limit {c.row_limit} with no sort_by shows {c.row_limit} ARBITRARY " + f"rows, not a top {c.row_limit}; set sort_by to the ranking metric", + ) + + +_RANGE_DAYS = {"day": 1, "week": 7, "month": 30.4, "quarter": 91, "year": 365} +_GRAIN_DAYS = {"PT1S": 1 / 86400, "PT1M": 1 / 1440, "PT1H": 1 / 24, + "P1D": 1, "P1W": 7, "P1M": 30.4, "P3M": 91, "P1Y": 365} +# Points a chart type can render before it stops informing: bars must read as +# discrete periods; lines/areas add nothing past a few hundred points at +# dashboard width. +_POINT_BUDGET = {"timeseries_bar": 40, "timeseries_line": 300, + "timeseries_area": 300, "timeseries_scatter": 300} + + +def _grain_days(grain: str) -> float | None: + if grain in _GRAIN_DAYS: + return _GRAIN_DAYS[grain] + if "P1W" in grain: # Superset's week-anchor spellings (1969-12-28T.../P1W) + return 7 + return None + + +def _span_days(time_range: str) -> float | None: + tr = time_range.strip().lower() + m = re.fullmatch(r"last\s+(\d+)\s+(day|week|month|quarter|year)s?", tr) + if m: + return int(m.group(1)) * _RANGE_DAYS[m.group(2)] + m = re.fullmatch(r"last\s+(day|week|month|quarter|year)", tr) + if m: + return _RANGE_DAYS[m.group(1)] + if " : " in time_range: + try: + a, b = (date.fromisoformat(part.strip().split(" ")[0].split("T")[0]) + for part in time_range.split(" : ", 1)) + return abs((b - a).days) + except ValueError: + return None + return None + + +@rule("data.grain-vs-range", "warn", "the time grain should yield a sane number of points for the range") +def grain_vs_range(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type not in TIMESERIES_TYPES or not c.time_range: + continue + span = _span_days(c.time_range) + # An omitted grain compiles to the P1D default -- the most common + # LLM-authored shape is exactly the one that needs this rule. + eff = c.time_grain or DEFAULT_TIME_GRAIN + grain = _grain_days(eff) + if span is None or grain is None: + continue + label = eff if c.time_grain else f"{eff} (the default when omitted)" + points = span / grain + budget = _POINT_BUDGET.get(c.type, 300) + if span == 0: + yield Finding( + "data.grain-vs-range", "warn", c.name, ctx.where(c.name), + f"{c.time_range!r} spans 0 days; extend the time_range", + ) + elif points < 2: + yield Finding( + "data.grain-vs-range", "warn", c.name, ctx.where(c.name), + f"{c.time_range!r} at grain {label} yields ~{points:.1f} point(s); " + f"a line needs a finer grain or a longer range", + ) + elif points > budget: + hint = ("bars stop reading as discrete periods; switch to a line or coarsen " + "the grain" if c.type == "timeseries_bar" else "coarsen the grain") + yield Finding( + "data.grain-vs-range", "warn", c.name, ctx.where(c.name), + f"{c.time_range!r} at grain {label} yields ~{points:,.0f} points " + f"(budget ~{budget} for {c.type}); {hint}", + ) + + +@rule("chart.trend-grain", "info", "trend tiles at a fine grain over full history draw thousands of points in a small card", since="2") +def trend_grain(ctx: RuleContext): + fine = (None, "PT1S", "PT1M", "PT1H", "P1D") + windowed = any(f.type == "time_range" and f.default for f in ctx.spec.filters) + if windowed: + return + for c in ctx.spec.charts: + if c.type == "big_number_trend" and c.time_grain in fine: + yield Finding( + "chart.trend-grain", "info", c.name, ctx.where(c.name), + f"sparkline at grain {c.time_grain or 'P1D (default)'} with no defaulted " + f"dashboard time window draws full history daily; coarsen to P1W/P1M or " + f"give the time_range filter a default", + ) + + +# -- narrative & filters: polish ------------------------------------------------- + +_MINOR_WORDS = {"a", "an", "the", "of", "by", "vs", "and", "or", "in", "on", + "per", "for", "to", "with", "at", "as"} + + +def _case_class(name: str) -> str | None: + words = re.findall(r"[A-Za-z][A-Za-z']*", name) + # All-uppercase words are acronyms (AOV, SLA, YoY has mixed...): they say + # nothing about the author's casing style, so they don't vote. + significant = [w for w in words[1:] + if w.lower() not in _MINOR_WORDS and len(w) > 1 and not w.isupper()] + if not significant: + return None + if all(w[0].isupper() for w in significant): + return "title" + if all(w[0].islower() for w in significant): + return "sentence" + return None + + +@rule("narrative.title-style", "info", "chart titles should share one casing style") +def title_style(ctx: RuleContext): + classes: dict[str, list[str]] = {"title": [], "sentence": []} + for c in ctx.spec.charts: + cls = _case_class(c.name) + if cls: + classes[cls].append(c.name) + if classes["title"] and classes["sentence"]: + minority = min(classes.values(), key=len) + style = "Title Case" if minority is classes["title"] else "sentence case" + yield Finding( + "narrative.title-style", "info", None, "charts", + f"mixed title casing; {minority} use {style} while the rest do not. " + f"NOTE: renaming a chart changes its identity (uuid); align future names, " + f"do not bulk-rename a live dashboard", + ) + + +@rule("narrative.big-number-format", "info", "hero numbers deserve a number format") +def big_number_format(ctx: RuleContext): + for c in ctx.spec.charts: + if c.type in KPI_TYPES and c.number_format is None: + yield Finding( + "narrative.big-number-format", "info", c.name, ctx.where(c.name), + "no number_format: raw float precision on a hero number; ',.0f' or '.3s' read better", + ) + + +@rule("narrative.filtered-title", "info", "a filtered chart's title should say what it shows") +def filtered_title(ctx: RuleContext): + for c in ctx.spec.charts: + fragments = [] + for f in c.filters: + if f.op not in ("==", "IN", "LIKE"): + continue + values = f.value if isinstance(f.value, list) else [f.value] + # Strings only: booleans and numbers ("True", "42") are predicates, + # not names a human would echo in a title. + fragments += [v.strip("%") for v in values if isinstance(v, str) + and len(v.strip("%")) >= 3 and not v.replace(".", "").isdigit()] + if fragments and not any(frag.lower() in c.name.lower() for frag in fragments): + yield Finding( + "narrative.filtered-title", "info", c.name, ctx.where(c.name), + f"filtered to {fragments} but the title doesn't say so; " + f"name the scope so the chart says what it shows", + ) + + +@rule("filters.time-picker", "info", "time-based dashboards want a time range picker in the filter bar") +def time_picker(ctx: RuleContext): + temporal = any(c.type in TIMESERIES_TYPES | {"big_number_trend"} for c in ctx.spec.charts) + has_picker = any(f.type == "time_range" for f in ctx.spec.filters) + if temporal and not has_picker: + yield Finding( + "filters.time-picker", "info", None, "filters", + "timeseries charts but no time_range filter in the native bar; " + "viewers will want to change the window", + ) + + +@rule("filters.count", "warn", "past ~6 select pickers a filter bar stops being navigable (and each costs a query on load)", since="2") +def filters_count(ctx: RuleContext): + selects = [f.name for f in ctx.spec.filters if f.type == "select"] + if len(selects) > ctx.params.max_filter_selects: + yield Finding( + "filters.count", "warn", None, "filters", + f"{len(selects)} select filters (max {ctx.params.max_filter_selects} for this " + f"audience); each is a distinct-values query on every load -- keep the few " + f"viewers actually change, move the rest to per-chart WHERE filters", + ) + + +@rule("filters.duplicate-column", "info", "two filters on the same column fight each other", since="2") +def filters_duplicate(ctx: RuleContext): + seen: dict[tuple, str] = {} + for f in ctx.spec.filters: + if f.type not in ("select", "range"): + continue + key = (f.dataset.key(), f.column) + if key in seen: + yield Finding( + "filters.duplicate-column", "info", None, "filters", + f"filters {seen[key]!r} and {f.name!r} both target " + f"{f.column!r}; viewers get two controls with one meaning", + ) + else: + seen[key] = f.name + + +@rule("filters.select-cardinality", "warn", "a select over hundreds of distinct values is an unusable picker", data_aware=True, since="2") +def filters_select_cardinality(ctx: RuleContext): + if ctx.prober is None: + return + for f in ctx.spec.filters: + if f.type != "select": + continue + ds = (ctx.resolution.datasets.get(f.dataset.key()) + if ctx.resolution is not None else None) + if ds is None: + continue + if ctx.prober.more_than(ds, f.column, 500): + yield Finding( + "filters.select-cardinality", "warn", None, "filters", + f"select {f.name!r} on {f.column!r} has more than 500 distinct values: " + f"an unusable picker and a heavy load-time query; use a numeric range " + f"filter or a coarser column", + ) + + +@rule("filters.time-default", "info", "an undefaulted time picker loads the dashboard over ALL history", since="2") +def filters_time_default(ctx: RuleContext): + for f in ctx.spec.filters: + if f.type == "time_range" and not f.default: + yield Finding( + "filters.time-default", "info", None, "filters", + f"time_range filter {f.name!r} has no default: first load scans and " + f"draws full history; set a default window (e.g. 'Last quarter')", + ) + + +@rule("filters.range-default", "info", "a range slider with no default bounds spans the whole domain", since="2") +def filters_range_default(ctx: RuleContext): + for f in ctx.spec.filters: + if f.type == "range" and f.le is None and f.ge is None: + yield Finding( + "filters.range-default", "info", None, "filters", + f"range filter {f.name!r} sets neither ge nor le; give it a default " + f"bound so the slider starts somewhere meaningful", + ) + + +@rule("narrative.format-consistency", "info", "one measure, one number format", since="2") +def format_consistency(ctx: RuleContext): + by_metric: dict[str, dict] = {} + for c in ctx.spec.charts: + if c.type in KPI_TYPES: + by_metric.setdefault(c.metric, {})[c.name] = c.number_format + for metric, charts in by_metric.items(): + if len(charts) > 1 and len(set(charts.values())) > 1: + yield Finding( + "narrative.format-consistency", "info", None, "charts", + f"metric {metric!r} renders with different number formats across " + f"{sorted(charts)}: {sorted(set(str(v) for v in charts.values()))}; " + f"pick one", + ) + + +@rule("layout.markdown-height", "info", "a one-line markdown header doesn't need a chart-sized block", fixable=True, since="2") +def markdown_height(ctx: RuleContext): + def rows_of(container): + return container.get("rows") or [] + + # Operates on the raw layout indices so the fix can address the block + # (markdown has no name to key on). + lay = ctx.spec.layout + sources = ([(None, lay.rows)] if lay.rows + else [(ti, t.rows) for ti, t in enumerate(lay.tabs or []) if t.rows]) + for ti, rows in sources: + for ri, row in enumerate(rows or []): + for ii, item in enumerate(row): + if isinstance(item, str): + continue + lines = [l for l in item.markdown.splitlines() if l.strip()] + h = item.height or 4 + if len(lines) <= 1 and h >= 3: + where = (f"tab {(lay.tabs[ti].title if ti is not None else '')!r} row {ri}" + if ti is not None else f"layout row {ri}") + yield Finding( + "layout.markdown-height", "info", None, where, + f"one-line markdown block at {h} units; 2 is plenty for a header", + fix={"md": [ti, ri, ii], "set": {"height": 2}}, + ) diff --git a/chartwright/mcp_server.py b/chartwright/mcp_server.py index db4e647..cf3dd2c 100644 --- a/chartwright/mcp_server.py +++ b/chartwright/mcp_server.py @@ -53,17 +53,44 @@ def validate_spec(spec_json: str) -> str: return json.dumps(err or {"ok": True, "stage": "schema"}) +def _advice(spec, resolution=None, audience: str | None = None) -> dict: + """Advice riding along MCP responses degrades, never raises (mirrors the CLI).""" + from .design import advise + + try: + return advise(spec, audience=audience, resolution=resolution).payload() + except Exception as e: # noqa: BLE001 + return {"stage": "design", "ok": True, "design_brain": "1", + "counts": {"error": 0, "warn": 0, "info": 0}, "findings": [], + "fixed": [], "ignored": [], + "errors": [{"code": "overlay" if isinstance(e, ValueError) else "advice", + "detail": str(e)}]} + + +def _bad_audience(audience: str) -> str | None: + from .design.presets import AUDIENCE_NAMES + + if audience and audience not in AUDIENCE_NAMES: + return json.dumps({"ok": False, "stage": "design", "errors": [{ + "code": "audience", + "detail": f"unknown audience {audience!r}; one of {sorted(AUDIENCE_NAMES)}"}]}) + return None + + @mcp.tool() def check_spec(spec_json: str, profile: str) -> str: """Pre-flight referential resolution against the live Superset instance: - every dataset triple, column, and metric must exist. Returns typed errors.""" + every dataset triple, column, and metric must exist. Returns typed errors + plus a design-brain advice block.""" spec, err = _parse_spec(spec_json) if err: return json.dumps(err) from .apply import check res = check(spec, _client(profile)) - return json.dumps({"ok": res.ok, "stage": "resolve", "errors": [e.as_dict() for e in res.errors]}) + return json.dumps({"ok": res.ok, "stage": "resolve", + "errors": [e.as_dict() for e in res.errors], + "advice": _advice(spec, resolution=res if res.ok else None)}) @mcp.tool() @@ -91,6 +118,73 @@ def plan_dashboard(spec_json: str, profile: str) -> str: return run_plan(spec, _client(profile)).to_json() +@mcp.tool() +def design_brief(audience: str = "analytical") -> str: + """The design brief to read BEFORE authoring a spec: audience budgets, + chart choice, composition, and what the critic enforces. Audiences: + executive | analytical | operational.""" + bad = _bad_audience(audience) + if bad: + return bad + from .design.brief import render_brief + + try: + return render_brief(audience) + except ValueError as e: + return json.dumps({"ok": False, "stage": "design", "errors": [{"code": "overlay", "detail": str(e)}]}) + + +@mcp.tool() +def advise_spec(spec_json: str, audience: str = "", profile: str = "") -> str: + """Design review of a spec against the design-brain rulebook (offline; + pass a profile for data-aware rules: column types and cardinality). + Audiences: executive | analytical | operational.""" + bad = _bad_audience(audience) + if bad: + return bad + spec, err = _parse_spec(spec_json) + if err: + return json.dumps(err) + from .design import advise + + resolution = prober = None + if profile: + from .design.probe import CardinalityProber + from .resolver import resolve + + client = _client(profile) + resolution = resolve(spec, client) + prober = CardinalityProber(client) + try: + report = advise(spec, audience=audience or None, resolution=resolution, prober=prober) + except ValueError as e: + return json.dumps({"ok": False, "stage": "design", "errors": [{"code": "overlay", "detail": str(e)}]}) + payload = report.payload() + if resolution is not None and resolution.errors: + payload["resolution_errors"] = [e.as_dict() for e in resolution.errors] + return json.dumps(payload) + + +@mcp.tool() +def fix_spec(spec_json: str, audience: str = "") -> str: + """Apply the design brain's safe, presentation-only fixes (heights, bar + orientation) to a spec. Returns {spec, advice}: the patched spec JSON and + the advice report with .fixed listing what changed. Offline.""" + bad = _bad_audience(audience) + if bad: + return bad + spec, err = _parse_spec(spec_json) + if err: + return json.dumps(err) + from .design import advise_and_fix + + try: + new_data, report = advise_and_fix(json.loads(spec_json), audience=audience or None) + except ValueError as e: + return json.dumps({"ok": False, "stage": "design", "errors": [{"code": "overlay", "detail": str(e)}]}) + return json.dumps({"ok": True, "stage": "design", "spec": new_data, "advice": report.payload()}) + + @mcp.tool() def decompile_dashboard(dashboard: str, profile: str) -> str: """Turn a live dashboard (slug or numeric id) into a spec + a named @@ -104,6 +198,49 @@ def decompile_dashboard(dashboard: str, profile: str) -> str: return json.dumps({"ok": True, "spec": result.spec, "losses": result.losses_json()}) +@mcp.tool() +def redesign_dashboard(dashboard: str, profile: str, audience: str = "") -> str: + """One-shot redesign of a live dashboard (slug or numeric id): decompile, + design-audit with data-aware rules, apply safe geometry fixes. Returns the + redesigned spec, the decompile losses, and the remaining findings. A + dashboard the tool does not own gets a '-redesign' slug (applies side by + side; the original is untouched).""" + bad = _bad_audience(audience) + if bad: + return bad + from pydantic import ValidationError + + from .apply import _ownership_guard + from .decompile import decompile_live + from .design.probe import CardinalityProber + from .design.redesign import redesign_spec + from .resolver import resolve + from .spec import load_spec as _load_spec + + client = _client(profile) + try: + result = decompile_live(dashboard, client) + except ValueError as e: + return json.dumps({"ok": False, "stage": "redesign", + "errors": [{"code": "decompile", "detail": str(e)}]}) + try: + spec = _load_spec(result.spec) + except ValidationError: + return json.dumps({"ok": False, "stage": "redesign", "losses": result.losses_json(), + "errors": [{"code": "decompiled_spec_invalid", + "detail": "decompiled spec does not load; use decompile_dashboard"}]}) + owned = _ownership_guard(spec, client) is None + try: + new_data, payload = redesign_spec( + result.spec, result.losses_json(), owned=owned, audience=audience or None, + resolution=resolve(spec, client), prober=CardinalityProber(client)) + except ValueError as e: + return json.dumps({"ok": False, "stage": "design", + "errors": [{"code": "overlay", "detail": str(e)}]}) + payload["spec"] = new_data + return json.dumps(payload) + + def main() -> None: mcp.run() diff --git a/chartwright/resolver.py b/chartwright/resolver.py index 2b96907..6119eb9 100644 --- a/chartwright/resolver.py +++ b/chartwright/resolver.py @@ -23,6 +23,18 @@ class ResolvedDataset: columns: list[str] metrics: list[str] main_dttm_col: str | None = None + # Superset GenericDataType per column (0 numeric, 1 string, 2 temporal, + # 3 boolean); absent when the instance doesn't report it. Consumed by the + # design brain's type-aware rules; resolution itself only needs names. + column_types: dict[str, int] = field(default_factory=dict) + temporal_columns: list[str] = field(default_factory=list) + + def is_temporal(self, column: str) -> bool | None: + """True/False when the instance reported a type, None when unknown.""" + if column in self.temporal_columns: + return True + tg = self.column_types.get(column) + return None if tg is None else tg == 2 @dataclass @@ -106,15 +118,20 @@ def _resolve_dataset(ref: DatasetRef, client: SupersetClient, res: Resolution) - return None m = matches[0] detail = client.dataset_detail(m["id"]) + cols = detail.get("columns", []) return ResolvedDataset( id=m["id"], uuid=str(m["uuid"]), table=m["table_name"], schema=m.get("schema") or None, database_name=(m.get("database") or {}).get("database_name"), - columns=[c["column_name"] for c in detail.get("columns", [])], + columns=[c["column_name"] for c in cols], metrics=[x["metric_name"] for x in detail.get("metrics", [])], main_dttm_col=detail.get("main_dttm_col") or None, + column_types={c["column_name"]: c["type_generic"] for c in cols + if c.get("type_generic") is not None}, + temporal_columns=[c["column_name"] for c in cols + if c.get("is_dttm") or c.get("type_generic") == 2], ) @@ -172,6 +189,14 @@ def _check_chart_fields(chart, ds: ResolvedDataset, res: Resolution) -> None: _check_metric(m, chart.name, ds, res) for g in chart.groupby or []: _check_column(g, chart.name, ds, res, "groupby") + if chart.sort_by: + # Resolved like everything else it references: raw mode sorts by a + # column, aggregate mode by a metric. Unchecked, a typo here used to + # pass check and then silently not sort. + if chart.columns: + _check_column(chart.sort_by, chart.name, ds, res, "sort_by") + else: + _check_metric(chart.sort_by, chart.name, ds, res) elif t == "pivot_table": for m in chart.metrics: _check_metric(m, chart.name, ds, res) diff --git a/chartwright/sketch.py b/chartwright/sketch.py index df46120..87779f0 100644 --- a/chartwright/sketch.py +++ b/chartwright/sketch.py @@ -19,6 +19,7 @@ from __future__ import annotations from dataclasses import dataclass, field +from functools import lru_cache @dataclass @@ -63,6 +64,15 @@ def _widths_to_twelfths(cell_counts: list[int], total_cells: int) -> list[int]: return floors +@lru_cache(maxsize=64) +def parse_sketch_cached(lines: tuple[str, ...], legend_items: tuple[tuple[str, str], ...], + line_units: int) -> list[SketchRow]: + """Memoized parse: geometry lookups hit parsed_sketch per chart per rule, + which measured ~cubic on sketch dashboards before caching. Callers treat + the returned rows as read-only (they are shared across calls).""" + return parse_sketch(list(lines), dict(legend_items), line_units) + + def parse_sketch(lines: list[str], legend: dict[str, str], line_units: int) -> list[SketchRow]: if not lines: raise SketchError("sketch is empty") diff --git a/chartwright/smoke.py b/chartwright/smoke.py index 463f82e..aa0230b 100644 --- a/chartwright/smoke.py +++ b/chartwright/smoke.py @@ -13,12 +13,48 @@ from __future__ import annotations +import math from dataclasses import dataclass from .client import SupersetClient from .compiler import _metric_payload from .resolver import Resolution -from .spec import DashboardSpec +from .spec import GRID_HEADER_UNITS, DashboardSpec, grid_units_for_rows + + +def _fit_warning(chart, spec: DashboardSpec, result: list) -> str | None: + """Data-driven height fit for table/pivot_table (issue #1). The spec's + height is a fixed layout property while rendered rows grow with the data: + rows past the fold hide behind the chart's INNER scrollbar, the dashboard + looks complete, and nothing else would ever complain. Smoke already holds + the query result, so the comparison costs nothing extra.""" + if chart.type not in ("table", "pivot_table"): + return None + data: list[dict] = [] + for q in result: + data.extend(q.get("data") or []) + if not data: + return None + if chart.type == "pivot_table": + if not chart.rows: + return None # columns-only pivot: a single metric band, height-safe + leaf = len({tuple(r.get(d) for d in chart.rows) for r in data}) + header_units = GRID_HEADER_UNITS + (1 if chart.columns else 0) + what = f"pivot renders ~{leaf} leaf rows" + else: + leaf = len(data) # already capped by the query's row_limit + header_units = GRID_HEADER_UNITS + what = f"table renders ~{leaf} rows" + height = spec.resolved_height(chart.name) + # Shared grid model (chartwright/spec.py): the design critic's + # size.grid-fit and size.table-window read the same numbers, so pre-apply + # advice and this post-apply warning can never contradict each other. + needed = grid_units_for_rows(leaf, header_units) + if needed <= height: + return None + return (f"{what} (~{needed * 40:.0f}px) but height={height:g} ({height * 40:.0f}px): " + f"rows will hide behind an inner scrollbar; raise height to " + f"~{math.ceil(needed)} units or cap row_limit") @dataclass @@ -117,6 +153,9 @@ def smoke_chart(chart, spec: DashboardSpec, resolution: Resolution, client: Supe return SmokeResult(chart.name, False, False, f"unparseable chart/data response: {e}") if rows == 0: return SmokeResult(chart.name, True, True, "query succeeded but returned 0 rows") + fit = _fit_warning(chart, spec, result) + if fit: + return SmokeResult(chart.name, True, True, f"{rows} rows; {fit}") return SmokeResult(chart.name, True, False, f"{rows} rows") diff --git a/chartwright/spec.py b/chartwright/spec.py index 936709a..edd105c 100644 --- a/chartwright/spec.py +++ b/chartwright/spec.py @@ -4,7 +4,7 @@ feedback channel an LLM caller gets; keep messages precise and actionable. Surface: 14 chart types, per-chart WHERE filters, a dashboard-level native -filter bar (select + time_range), markdown blocks, and tabs. +filter bar (select, time_range, numeric range), markdown blocks, and tabs. """ from __future__ import annotations @@ -24,6 +24,25 @@ } DEFAULT_TIME_GRAIN = "P1D" +# How a rendered table/pivot grid consumes spec height. ONE model, used by the +# design critic (size.table-window, size.pivot-window, size.grid-fit) and by +# apply-time smoke, so offline advice, data-aware advice, and the apply warning +# can never disagree about the same chart. ~30px per grid row over a 40px unit; +# 3 units of card title + column header (one more when a pivot nests column +# dimensions). Calibration knob, same status as ROW_UNITS_PER_SPEC_UNIT. +GRID_UNITS_PER_ROW = 0.75 +GRID_HEADER_UNITS = 3 + + +def grid_units_for_rows(rows: float, header_units: float = GRID_HEADER_UNITS) -> float: + """Spec height units needed to render `rows` grid rows without an inner scrollbar.""" + return rows * GRID_UNITS_PER_ROW + header_units + + +def grid_rows_visible(height: float, header_units: float = GRID_HEADER_UNITS) -> float: + """Inverse: grid rows visible at `height` before the inner scrollbar starts.""" + return max(0.0, (height - header_units) / GRID_UNITS_PER_ROW) + ADHOC_AGGREGATES = ("SUM", "AVG", "COUNT", "COUNT_DISTINCT", "MIN", "MAX") _ADHOC_RE = re.compile(r"^(SUM|AVG|COUNT|COUNT_DISTINCT|MIN|MAX)\((.+?)\)(?:\s+AS\s+(.+))?$") @@ -178,7 +197,11 @@ class TableChart(_ChartBase): metrics: list[str] | None = None groupby: list[str] | None = None row_limit: int | None = Field(default=None, ge=1) - sort_by: str | None = Field(default=None, description="Metric or column to sort by (aggregate mode: metric)") + sort_by: str | None = Field( + default=None, + description="Sort key, DESCENDING (a ranking). Aggregate mode: one of the " + "chart's metrics, written the same way. Raw mode: a column name.", + ) @model_validator(mode="after") def _mode(self) -> "TableChart": @@ -391,9 +414,10 @@ class _SketchHolder(BaseModel): ) def parsed_sketch(self): - from .sketch import parse_sketch + from .sketch import parse_sketch_cached - return parse_sketch(self.sketch or [], self.legend or {}, self.line) + return parse_sketch_cached( + tuple(self.sketch or ()), tuple(sorted((self.legend or {}).items())), self.line) class Tab(_SketchHolder): @@ -418,6 +442,22 @@ class DashboardMeta(BaseModel): slug: str = Field(min_length=1, pattern=r"^[a-z0-9][a-z0-9-]*$", description="uuid seed input; lowercase kebab-case") +class DesignConfig(BaseModel): + """Per-dashboard design-brain settings (see docs/DESIGN-BRAIN.md). + + Additive and optional: a spec without this block behaves exactly as + before. `ignore` entries are rule ids ('size.pie-geometry'), optionally + scoped to one chart ('size.pie-geometry@Sales by Region'); suppressed + findings are still reported as ignored, so silence stays visible.""" + + model_config = ConfigDict(extra="forbid") + + audience: Literal["executive", "analytical", "operational"] | None = Field( + default=None, description="Design preset; CLI --audience overrides") + ignore: list[str] = Field( + default_factory=list, description="Design rule ids to suppress") + + class Layout(_SketchHolder): """Flat rows, tabs, or an ASCII sketch; exactly one.""" @@ -433,6 +473,13 @@ def _exactly_one(self) -> "Layout": raise ValueError("layout: provide exactly one of rows / tabs / sketch") if self.sketch and not self.legend: raise ValueError("layout: a sketch needs a legend") + seen: set[str] = set() + for t in self.tabs or []: + if t.title in seen: + raise ValueError( + f"duplicate tab title {t.title!r}: tab titles must be unique " + "(they name tabs in diffs, advice, and the UI)") + seen.add(t.title) return self def all_rows(self) -> list[list[RowItem]]: @@ -454,6 +501,7 @@ class DashboardSpec(BaseModel): charts: list[Chart] = Field(min_length=1) filters: list[DashboardFilter] = Field(default_factory=list, description="Native filter bar") layout: Layout + design: DesignConfig | None = Field(default=None, description="Design-brain settings (optional)") @field_validator("charts") @classmethod @@ -577,7 +625,16 @@ def resolved_item_width(self, item: RowItem) -> int: own = self._item_width(item) if own is not None: return own - return max(1, (GRID_WIDTH - explicit_total) // len(implicit)) + # Largest-remainder: implicit widths must SUM to the full remainder or + # every 5-implicit row renders with a phantom 2-column hole at the + # right (floor division used to drop it). Earlier items get the +1. + base, rem = divmod(GRID_WIDTH - explicit_total, len(implicit)) + # Position by IDENTITY for blocks, value only for chart names (unique by + # validator). Two equal markdown blocks both matched the first one's + # index under `==`, so both took the +1 and the row summed past 12. + pos = next(i for i, x in enumerate(implicit) + if x is item or (isinstance(item, str) and x == item)) + return max(1, base + (1 if pos < rem else 0)) def resolved_height(self, name: str) -> float: chart = next(c for c in self.charts if c.name == name) diff --git a/docs/DESIGN-BRAIN-V2.md b/docs/DESIGN-BRAIN-V2.md new file mode 100644 index 0000000..2e2f717 --- /dev/null +++ b/docs/DESIGN-BRAIN-V2.md @@ -0,0 +1,436 @@ +# Design Brain v2: Review Findings and Roadmap + +> **Status: EXECUTED, with one item re-opened.** 36 of the 37 roadmap items +> below landed in the four `design brain v2 batch A-D` commits (design_brain +> version "2"); §15 of [DESIGN-BRAIN.md](DESIGN-BRAIN.md) records the few +> places execution deliberately deviated from a proposal. This page remains +> the review record. +> +> **Item 20 (stale hard-coded counts) only half-landed** — 'six MCP tools' +> was corrected, '125 tests' was not, and this page asserted completion while +> incomplete. Closed afterwards by moving the counts into a single row in +> [VERIFICATION.md](VERIFICATION.md) that `tests/test_docs.py` now enforces, +> so the class of drift cannot recur. Recorded rather than quietly amended: +> a status line that overstates is the same defect as a rule that passes +> silently, which is what theme 1 below is about. +> +> Originally produced 2026-07-16 by a +> multi-lens agent review of the shipped design brain (docs/DESIGN-BRAIN.md). +> Method: seven independent review lenses (core machinery, per-rule logic, +> adversarial spec fuzzing, BI/UX domain expertise, tool ergonomics, +> architecture/learning-loop, doc truth), each empowered to execute the code +> against scratch specs. Every bug, edge-case, and doc-drift claim was then +> independently re-verified by an adversarial agent instructed to refute it. +> **80 findings survived verification, 0 were refuted** +> (17 bugs, 15 edge cases, 20 gaps, 14 doc drifts, +> 10 improvements, 4 missed opportunities), deduplicated and +> ranked into the 37 items below. Raw findings: work-specs/design-brain-v2-review-raw.json (local, untracked). + +## Themes + +1. Inferred signals lie: provenance must be explicit — the fractional-height 'human polish' heuristic, ok:true under strict gates, and silently inert ignore entries all make the brain's own output or the user's explicit intent invisible; state should be tracked, not guessed from side effects. +2. Config is a trust boundary: design.yaml, design.ignore, and MCP arguments are user/org-edited inputs that currently crash, no-op, or corrupt specs silently — validate values (not just key names) once at load with typed errors. +3. Sketch normalization loses structure: flattening vertical stacks into flat chart lists makes count-based rules (row-density, kpi-band) condemn the exact layouts the sketch grammar exists to express; rules must reason over horizontal slots and items. +4. Preach it, lint it: the brief and guidelines state thresholds (3/12 width, ~30 bars, format consistency, pie integrity) the rulebook never enforces — and docs claim tests, aliases, and behaviors that don't exist; every promise needs either machinery or a correction. +5. Blind spots in the chart/filter taxonomy: pivot_table, range filters, sort order, and color have zero rules, and nothing guards exhaustiveness when a 15th chart type arrives — coverage gaps should fail CI, not fall through silently. +6. Close the learning loop honestly: absorb -> calibrate -> fix must not forge human signatures, must report what it observed, and needs audience granularity and decay so learned heights improve dashboards instead of blinding or misleading the rulebook. + +## Roadmap + +Ordered by the review's prioritization: confirmed bugs and false-positive +risks first, then the design-knowledge gaps that most improve real +dashboards, then architecture and polish. Effort: S < 1h single-file, +M multi-file, L new subsystem. + +### 1. Fix `chartwright absorb` NameError (missing `import json`) that crashes after mutating the spec file + +`bug` · impact **high** · effort **S** + +Confirmed: every real absorb run rewrites the spec and appends the calibration log, THEN crashes in AbsorbReport.to_json with code 'unexpected'. The tool mutates user files while reporting failure, and the entire phase-3 learning loop looks broken. One-line fix, zero risk. + +**Proposal:** Add `import json` to absorb.py. Add one test asserting AbsorbReport.to_json() round-trips (covers the CLI-only path tests currently miss). Reorder the absorb CLI branch to print the report before file side effects so a reporting failure can never follow a silent mutation. + +### 2. Kill the echo chamber: tool-written fractional heights forge the 'human polished' signature and permanently blind size.* rules + +`bug` · impact **high** · effort **M** + +Confirmed by four independent lenses (core/adversarial/toolux/arch — merged). advise --fix and calibrate-fed fix_height write fractional heights; human_polished() then suppresses every size.* finding on charts no human touched, including width complaints the fix never addressed and violations under a stricter audience. Report says ok:true over hidden defects. The learning loop's output disables the rulebook it calibrates. Also swallows width-only findings on genuinely absorbed charts (absorb can never write widths) and drops polished+ignored findings from the ignored list. + +**Proposal:** Three coordinated changes: (a) math.ceil autofix height targets in fix_height/row-harmony so tool output never mints the fractional signature (one-line core fix); (b) narrow the suppression to height-driven complaints — split or dimension-tag the pie/heatmap geometry findings so width clauses survive on polished charts; (c) move the suppressed-key check before the polish skip in advise() so explicit design.ignore intent always appears in `ignored`. Add regression tests for the vbar-flip/row-harmony masking scenario and the width-only case. + +### 3. Validate design.yaml overlay values (types, ranges, audience keys) and clamp fix targets to spec bounds + +`bug` · impact **high** · effort **M** + +Confirmed across three lenses (merged). The overlay is an org-wide user-edited trust boundary; today a string value crashes check/apply even at --design warn as 'unexpected' TypeError (the code's own message admits it's wrong), scalar `disable` silently becomes a set of characters, recommended_heights > 100 makes the fix loop corrupt the spec then blame the user's overlay, audience-key typos are silently ignored, and audience-level recommended_heights wholesale replaces params-level entries instead of merging. + +**Proposal:** One validation pass in load_overlay/params_for raising ValueError (the CLI's existing typed channel): disable must be list[str]; params/audiences dicts with numeric values; audiences keyed by AUDIENCE_NAMES; recommended_heights dict[str, number] range-checked 1..100 (also when calibrate writes it). Clamp fix_height targets to the spec's height cap. Fix the recommended_heights per-key merge across all four layers. Belt-and-braces: _advice_payload catches Exception, honoring its 'never break the pipeline' contract. ~20 lines in presets.py + small model.py/cli.py touches, kills eight confirmed symptoms. + +### 4. data.grain-vs-range must apply the compiler's P1D default when time_grain is omitted + +`bug` · impact **high** · effort **S** + +Confirmed: the most common LLM-authored shape (optional time_grain omitted) compiles identically to an explicit P1D chart but gets zero advice — 'last 5 years' silently renders ~1,825 daily points. The rule is disabled precisely on the specs that need it most. + +**Proposal:** In grain_vs_range, evaluate `_GRAIN_DAYS.get(c.time_grain or DEFAULT_TIME_GRAIN)` and drop time_grain from the skip condition (keep skipping only on missing/unparseable time_range). One rule change + two test rows. + +### 5. Add a standalone minimum-width rule and make presets arithmetically self-consistent + +`bug` · impact **high** · effort **M** + +Confirmed: the brief preaches 'below 3/12 is unreadable' but width is only checked when a row already exceeds max_row_charts — a 2/12 timeseries in a 2-chart row passes clean, and operational's max_row_charts=5 (15/12 needed) invites exactly the layout the rulebook's own error branch condemns. Merged with the coverage gap: tables/pivots/funnels/treemaps have NO width rule at all (four 3/12 tables: zero findings), and 1/12 KPIs pass under operational. + +**Proposal:** New size.min-width rule over all non-markdown charts: axis/table/pivot < 3/12 error, < 4/12 warn (pie/heatmap keep their dedicated rules); KPI floor at 2/12. Cap or derive max_row_charts at floor(12/3)=4 and drop operational's to 4 with a doc note. Tests for the exactly-at-max row and the table-row case. + +### 6. chart.pie-slices: stop recommending row_limit truncation that manufactures a lying part-to-whole + +`bug` · impact **high** · effort **S** + +Confirmed: Superset computes pie percentages over returned rows only and the spec has no 'other' bucket, so the rule's 'set row_limit 7' remedy makes the top-7 read as 100% of the whole — violating the one integrity constraint pies have. The critic is actively teaching LLM authors to misstate shares. + +**Proposal:** Reword the finding to prefer horizontal bar; if the pie stays, require the title to disclose truncation. Add one guideline line: 'a row_limit on a pie redefines the whole; a truncated pie lies about share'. Rule text + guideline + test-string updates. + +### 7. Count horizontal slots, not flattened charts: fix row-density and kpi-band false positives on sketch stacks + +`false-positive` · impact **high** · effort **S** + +Confirmed twice each (merged four findings). Band.chart_names flattens sketch columns, so layout.row-density warns (and can falsely ERROR, blocking --design strict) on charts the user already split vertically, and layout.kpi-band flags the canonical KPI-sidebar-beside-hero-chart pattern the sketch grammar exists to express. These punish the tool's own best layouts and push users to ignore-list legitimate rules. + +**Proposal:** In row_density and kpi_band, iterate band.items: an item counts once, at item.width, and a pure-KPI stack is its own virtual band (only flag KPI/detail mixing within one item or in rows mode). Update DESIGN-BRAIN's kpi-band row to document the mixing branch. One file (rules.py) + sketch-stack tests, which currently don't exist. + +### 8. Update the stale MCP tool-surface pin, install the [mcp] extra in CI, and close MCP parity gaps + +`bug` · impact **medium** · effort **M** + +Confirmed: test_mcp_server.py pins 6 tools while 9 exist, and importorskip silently skips the whole file in dev — the 'pins the tool surface' guarantee is dead and the 3 design tools have zero coverage. Merged with parity gaps: advise_spec drops resolution_errors (agent can't tell data-aware rules were skipped), check_spec/build carry no advice block (brain default-OFF over MCP, contradicting decision-log #1), findings say fixable:true with no way to obtain the fix, and bad audience returns code 'overlay'. + +**Proposal:** Pin the 9-tool list and add the mcp extra to dev/CI. Attach resolution_errors in advise_spec; add the advice block to check_spec; add a fix_spec/advise_and_fix MCP tool returning patched spec_json; validate audience against AUDIENCE_NAMES with code 'audience'. Offline FastMCP tests for design_brief/advise_spec. + +### 9. Preset sanity: executive kpi_row_min to 2, operational fold_units to 22 + +`false-positive` · impact **medium** · effort **S** + +Confirmed: a classic two-KPI executive hero band fires 'looks unfinished' (false positive on a professional layout), and operational's fold budget (26) exceeds the ~22-unit viewport its own intent line ('everything visible' wall monitors) requires — the preset contradicts itself. + +**Proposal:** presets.py value changes: executive kpi_row_min=2; operational fold_units=22 (or rewrite the intent line). The kpi width floor lands with the size.min-width rule (rank 5). Update the two pinned tests. + +### 10. calibrate: report the per-type candidate entries it already builds but throws away + +`bug` · impact **medium** · effort **S** + +Confirmed dead code: 'needs >= N samples' / 'within 1 unit' notes are constructed then never appended, so users see events:4, proposals:[] with no explanation of what was observed or how far they are from a proposal — the learning loop looks inert exactly when it's warming up. + +**Proposal:** Append non-qualifying entries under a `candidates` key in the calibrate report. ~3 lines in calibrate.py + one test. + +### 11. chart.heatmap-grid: fix the probe-cap blind spot that passes 6,000-cell grids + +`bug` · impact **medium** · effort **S** + +Confirmed: count_up_to saturates at 31, so any grid whose smaller axis has <= 12 values (day-of-week is 7 — the most common heatmap shape) can never exceed the 400-cell threshold no matter how huge the other axis is. Merged with the double-probe ordering nit (small-cap probe runs first, wasting a live query). + +**Proposal:** When a side saturates, warn with 'at least NxM cells' phrasing (or re-probe at ceil(400/other)). Probe once at the max cap any rule requests per (dataset, column) — or have more_than probe at a fixed generous cap. Note the unbounded server-side GROUP BY cost in the probe docstring. + +### 12. narrative rule noise pack: acronym Title-Case misclassification and boolean/negative filter-title fragments + +`false-positive` · impact **medium** · effort **S** + +Both confirmed. BI names are dense with acronyms (MRR/ARR/KPI), so title-style flags perfectly consistent dashboards — and its own NOTE admits renaming is dangerous, making the bogus advice costly. Boolean flag filters demand 'True' appear in titles; negative numbers slip the digit guard. Cheap, common, credibility-eroding noise. + +**Proposal:** Two one-line guards in rules.py: skip fully-uppercase words in _case_class; skip non-string values (isinstance check, bool before int) in filtered_title's fragment loop. Tests for each. + +### 13. Deduplicate size findings: axis-min-height must skip heatmaps (and hbars with row_limit) + +`false-positive` · impact **medium** · effort **S** + +Confirmed: one short heatmap yields two warns with contradictory targets (6 vs 8); convergence rests on the max-merge crutch. Inflated counts and conflicting guidance in a report an LLM acts on verbatim. + +**Proposal:** Skip heatmap in axis_min_height (it owns a stricter rule) and horizontal bars with row_limit set (hbar-window's formula binds). Align heatmap-geometry's '< 6' message with its fix target of 8. rules.py + tests. + +### 14. Validate tab-title uniqueness in the spec + +`bug` · impact **medium** · effort **S** + +Confirmed three times (merged): duplicate titles collapse layout.tab-balance's title-keyed counts into fabricated skews (blaming the fattest tab family as 'thin', or masking a real 5:1 skew), and make every 'tab X row N' where-string ambiguous. The spec already validates chart and filter name uniqueness — tabs were just missed. + +**Proposal:** Add a uniqueness validator for tab titles in Layout mirroring _unique_names (root fix: repairs tab-balance counts AND where() ambiguity for all callers). One spec.py validator + test. + +### 15. Make the fix-loop convergence invariant explicit and guarded + +`bug` · impact **low** · effort **S** + +Confirmed (merged three findings): fix.py's max() merge and both docstrings claim 'heights only ever rise', but size.kpi-height lowers. Safe today only by accident of rule membership — the first future rule touching KPI heights ping-pongs or silently discards the lowering. + +**Proposal:** Fix the two docstrings to state the real invariant (KPI heights clamped 2-6 by exactly one rule; all other height fixes only rise) and add one contract test asserting no two fixable rules target the same chart's height in opposing directions. Optionally have advise_and_fix detect a no-progress round and stop with the conflicting keys in the report. + +### 16. pivot_table rule pack: end the rulebook's worst blind spot + +`gap` · impact **high** · effort **M** + +Not one of the 31 rules covers pivot_table despite its 10,000-row default and COLUMNS metrics layout — the tool can emit its worst scroll dungeon (3x2 dims, 5 metrics, 160px tall, 10k rows) with ok:true. Highest-value knowledge gap by rendered-damage-per-rule. + +**Proposal:** Four rules: size.pivot-window (height vs row_limit, reuse table-window's 0.8 units/row math), chart.pivot-columns (data-aware: column-cardinality x metrics > ~15 rendered columns), chart.pivot-dims (rows+columns > 3, mirrors treemap-depth), extend data.row-limit-intent to pivot_table. One pivot limits line in chart-choice.md. rules.py + guideline + tests. + +### 17. Filter-bar rule pack: count, duplicates, cardinality, undefaulted time windows, range filters + +`gap` · impact **medium** · effort **M** + +filters.* has exactly one rule; a 7-select bar with duplicate columns and an undefaulted all-history time picker (slow, meaningless for executives) passes clean. Filter overload is a top documented dashboard failure. Merged with the confirmed range-filter invisibility (third filter type absent from docstring, rules, brief). + +**Proposal:** Add filters.count (selects > ~6, threshold per preset), filters.duplicate-column, filters.select-cardinality (existing probe machinery, > ~500 distinct), filters.time-default (info; warn for executive), and a bare-range-filter info. Fix the spec module docstring ('select + time_range') and mention range filters once in the brief. + +### 18. grain-vs-range v2: per-type point budgets, KPI-trend coverage, complete grain table + +`gap` · impact **medium** · effort **M** + +Merged four findings: timeseries_bar's ~30-period cap is stated in the guideline but the linter allows 1000 (730 daily bars pass); big_number_trend sparklines full-scan all history with no grain guidance anywhere; _GRAIN_DAYS misses PT1M/PT1S and Superset week-anchor grains ('last day' at PT1M = 1,440 points, the exact pathology, silently skipped); zero-span ranges get 'use a finer grain' advice no grain can satisfy. + +**Proposal:** Per-type point budgets in grain_vs_range (bar ~40, line/area ~300); new info rule for big_number_trend with absent/fine grain and no defaulted time window; add PT1M/PT1S and week-anchor spellings to _GRAIN_DAYS (or a tiny ISO-duration regex); when span==0 say 'extend the time_range' and drop the grain suggestion. All in rules.py + one chart-choice.md line each. + +### 19. data.top-n-sort: a limit without an order is a sample, not a ranking + +`gap` · impact **medium** · effort **S** + +The classic silent ranking bug: 'Top Ten Stores' with row_limit 10 and no sort_by shows 10 arbitrary rows and no rule notices. Bars are compiler-protected; tables are not, and sort order is otherwise absent from the rulebook. + +**Proposal:** New warn rule: aggregate-mode table with row_limit <= ~100 and no sort_by. One line in chart-choice.md's ranking row. rules.py + test. + +### 20. Fix the factually wrong category-sort guidance and add chart.ordinal-order + +`gap` · impact **medium** · effort **M** + +Confirmed: chart-choice.md says 'category axes sort alphabetically' but the compiler metric-sorts bars/pies/funnels (the prescribed order-encoded-label workaround is inert for bars — x_axis_sort is hardcoded to the metric). The real failure — weekday/month/funnel-stage scrambled by metric or alphabetically ('Apr, Aug, Dec') — is unmentioned and unlinted. + +**Proposal:** Rewrite the guideline per chart family (metric-sorted: good for rankings, wrong for ordinals; heatmap/pivot: alphabetical). Add info rule chart.ordinal-order with a cheap column-name heuristic (day|weekday|month|quarter|dow). Guideline + rules.py + tests. + +### 21. Brief upgrade: canonical exemplar sketch, defaults table, format/grain vocabulary + +`gap` · impact **medium** · effort **S** + +The brief uses 56 of its 150-line budget and never states the defaults the critic later judges (height 8, row_limit 10,000), the d3 tokens, or the grain vocabulary it demands — and prior art (Show Me, Draco, LIDA) is unanimous that exemplars beat rule lists for generative compliance. Cheapest lever on first-shot spec quality. + +**Proposal:** Spend ~25 lines in brief.py: a 10-line canonical ASCII sketch per audience intent, a 4-line defaults table ('omit height -> 8; omit row_limit -> 10,000 — the critic flags both'), one line each for d3 formats and ISO grains. Update the line-budget test. + +### 22. narrative.format-consistency rule and an honest guideline about what the spec can express + +`gap` · impact **medium** · effort **M** + +Tier G demands 'consistent number formats per measure' but no rule checks it, and tables/pivots hardcode SMART_NUMBER so the guideline is inexpressible on any mixed dashboard — the brain instructs the LLM to do the impossible. + +**Proposal:** New info rule: identical metric strings across KPI charts with unequal number_format. Soften the guideline to what the spec can express today. File a spec v-next note for per-metric d3 format on table/pivot/timeseries (do not build it yet). + +### 23. First color knowledge: chart.format-bands and RAG/heatmap-scale guideline lines + +`gap` · impact **medium** · effort **M** + +'Color' appears nowhere in rules.py, yet FormatRule is the one surface the spec controls color on: overlapping RAG bands, green-for-high inversions, and lone decorative bands all pass. Heatmaps pin a sequential scheme that is wrong for signed deltas and no guideline mentions it. + +**Proposal:** Add chart.format-bands (warn: overlapping/contradictory ranges per metric; info: single lone band). Guideline lines: RAG polarity (red = adverse, IBCS) and 'sequential scale suits magnitudes; avoid heatmapping signed deltas'. rules.py + composition.md + tests. + +### 24. Small chart-choice rules: treemap-vs-bar and one-line markdown header height + +`gap` · impact **low** · effort **S** + +Two cheap, self-contained wins: a single-level low-cardinality treemap is strictly dominated by a horizontal bar (Cleveland-McGill) and never flagged; unsized one-line markdown headers burn 4 units (160px), undercutting the whitespace guideline, and are autofixable under existing safety policy. + +**Proposal:** Info rule chart.treemap-vs-bar (len(groupby)==1 and cardinality <= ~10). Fixable info rule layout.markdown-height (<= 1 non-empty line, height >= 3 -> set 2). One guideline line each. + +### 25. Validate ignore/disable entries and give band findings scoped keys + +`architecture` · impact **medium** · effort **M** + +Merged three findings: typo'd or stale design.ignore/--ignore/overlay-disable entries are silently inert (the user believes a rule is off while it fires — contradicting the module's own 'silence stays visible' philosophy), and band-level findings key on the bare rule id, so accepting one deliberate band silences the rule dashboard-wide. + +**Proposal:** In advise(): split each suppression key on '@', require the rule-id half to exist in RULES (alias-aware once rank 27 lands), surface unmatched entries as `unmatched_ignores` in the payload. Give band findings scoped keys from their stable where-strings ('layout.kpi-band@tab-Ops-row-2') accepted alongside current forms. design/__init__.py + model.py + docs + tests. + +### 26. Chart-type exhaustiveness contract test for the design taxonomy + +`architecture` · impact **medium** · effort **S** + +A 15th chart type falls through the design brain silently: compiler/resolver/decompile fail loudly if missed, but the hand-maintained KPI/TIMESERIES/AXIS sets and rule string literals just have no opinion and nobody is told. The repo already uses this exact pattern (test_params_contract). + +**Proposal:** Declare the standalone set (pie, table, pivot_table, funnel, treemap) in design/model.py and one contract test: KPI_TYPES | AXIS_TYPES | STANDALONE == set(CHART_TYPES). New types then fail CI until consciously classified. + +### 27. Make DESIGN-BRAIN's promises true: golden dogfood test, single version constant, since/alias mechanism + +`architecture` · impact **medium** · effort **M** + +Confirmed (merged two findings): the doc claims hypothesis property tests and an nyc_taxi golden test that do not exist — the sharpest truth violation found in a doc that declares itself 'the reference'. Rule-id aliases are promised for renames but no mechanism exists (a rename would orphan every ignore/disable list), `since` markers don't exist, and the '1' version string is triplicated with the constant unused. + +**Proposal:** Add the ~6-line golden test (example advises clean at analytical); add an 'advise never raises' loop over the existing seeded mutator or delete the property-test claim; add `since` to Rule (default '1') and an ALIASES dict consulted in the suppression check; make payload() and cli.py import DESIGN_BRAIN_VERSION. Rewrite §12 to match. + +### 28. Per-deployment severity overrides, and make Rule.severity honest metadata + +`architecture` · impact **medium** · effort **M** + +Orgs on --design strict have exactly one knob (full suppression) when they disagree with a level. Worse, the registry's severity already disagrees with what two rules emit (row-density warn->error, row-fill warn->info), so any future override or doc tooling keyed on Rule.severity mis-maps today, and no test relates emitted to registered severities. + +**Proposal:** Add `severity: {rule_id: level}` to the Overlay, applied at the single collection choke point in advise() (one dict lookup). Document Rule.severity as the default level; add a test whitelisting the two known escalation/demotion variances. Rides on rank 3's overlay validation. + +### 29. Calibration v2: log audience per absorb event, key recommendations audience->type, add --since decay + +`architecture` · impact **medium** · effort **M** + +Heights dragged on dense operational monitors and spacious executive scorecards pour into one immortal global median; fix_height's max(floor, rec) then inflates operational autofixes with analytical-learned heights, and the brief prints it as house truth for all audiences. Votes from deleted dashboards live forever. + +**Proposal:** Log spec.design.audience in each absorb event (backward compatible). Key overlay recommended_heights as audience->type with flat-type fallback. Add `calibrate --since 90d` as the cheap decay knob. Skip weighting schemes until the median demonstrably misleads. + +### 30. Cache sketch parsing and geometry lookups: advise is measured ~cubic on sketch dashboards + +`architecture` · impact **medium** · effort **S** + +Confirmed with profiles: resolved_height does an O(N) scan and re-parses EVERY sketch holder from raw ASCII per call; 62 sketch charts = 316-506ms per advise, times 6 fix-loop rounds, times redesign on decompiled dashboards — multi-second runs spent re-parsing the same ASCII art (parse_sketch called 188x per advise). + +**Proposal:** Two small caches, no behavior change: RuleContext.height() returns self.geo[name].height (already computed at construction); memoize parse_sketch per _SketchHolder. Add the benchmark script's worst case as a smoke test bound. + +### 31. advise --fix reports what changed (old/new values) and discloses the file rewrite + +`improvement` · impact **medium** · effort **M** + +The fix report is keys-only; the LLM must re-read the file to learn what was set, and the full-file canonical re-serialization turns a 3-value change into a whole-file git diff with no marker that the file was rewritten. Directly serves the skill's 'apply or consciously ignore' loop and the MCP fix_spec tool (rank 8). + +**Proposal:** Report fixed entries as objects {rule, chart, set:{field:new}, was:{field:old}}, add the written path to the payload, and document that --fix normalizes formatting (shared with absorb). fix.py + cli.py + tests. + +### 32. check --design strict gate emits the same design_gate error entry as apply + +`improvement` · impact **medium** · effort **S** + +Same gate, two error surfaces: check flips ok:false with empty errors and stage 'resolve' (agent must infer), while apply dies with an explicit design_gate code and remediation text. Related confirmed doc-drift: §10 claims payload ok reflects --strict but AdviceReport.ok is errors-only. + +**Proposal:** Append the design_gate error entry to check's payload['errors']; one-line §10 doc fix ('ok is false iff an error exists; the strict gate rides the exit code'). cli.py + DESIGN-BRAIN.md + test. + +### 33. Distribute the implicit-width remainder so row-fill stops blaming users for the tool's arithmetic + +`improvement` · impact **medium** · effort **S** + +Merged two findings: rows-mode floor division leaves real rendered holes (7 widthless charts -> 7/12) that layout.row-fill then tells the author to hand-fix, though they expressed no width opinion. Sketch mode already solves this with largest-remainder scaling in the same repo. + +**Proposal:** Reuse the largest-remainder approach in resolved_item_width so all-implicit rows sum to exactly 12 (compiler and rules share the function, so the render fixes itself and the FP disappears). Align the row-fill §7 severity cell ('warn/info') while touching it. spec.py + doc line + tests. + +### 34. Decide the sketch-autofix WYSIWYG policy: don't silently break the drawing + +`improvement` · impact **low** · effort **M** + +Confirmed: a height autofix on a sketch chart overrides drawn geometry, leaves the sketch text lying to the next reader, and row-harmony deliberately skips sketch sections so the tool-introduced raggedness is never reported — final report clean. + +**Proposal:** In sketch sections, withhold height autofixes and advise editing the sketch instead ('repeat the line to make this band 6 units'); or at minimum re-run row-harmony on bands the fix loop touched so the raggedness is reported. Pairs with the provenance work in rank 2. + +### 35. Docs discoverability pass: README design-brain section, FEATURES verb table, design.yaml reference, VERIFICATION coverage + +`doc-drift` · impact **medium** · effort **M** + +Merged four confirmed findings: the 0.2.0 headline feature is invisible on the landing page (zero README mentions, no DESIGN-BRAIN link), the canonical CLI verb table under-reports by a third (missing exactly the four design verbs), the advertised design.yaml overlay format is documented nowhere outside source, and VERIFICATION.md's suite breakdown omits all four design test modules — a reader concludes the feature is untested. + +**Proposal:** One PR: README 'design critic' bullet + advise quick-start line + DESIGN-BRAIN link; four rows in the FEATURES verb table; a ~15-line 'House style: design.yaml' section (five keys, worked example, $CHARTWRIGHT_DESIGN_DIR) linked from FEATURES; one VERIFICATION bullet for the design test modules. + +### 36. Doc precision sweep: stale counts and eight confirmed §7/§10/legend/docstring drifts + +`doc-drift` · impact **medium** · effort **S** + +All confirmed, all trivial: 125/16/six counts stale across three docs (actual 161/20/9), 'two new MCP tools' (three shipped), rule count 27 vs 31, hbar-window 'fold budget' vs hard-coded 20, row-limit-intent 'bar/pie/table' misattribution, row-fill severity split, the ⚡ legend over-claim, and the spec docstring's 'select + time_range'. Wrong docs poison design.ignore targeting and strict-gating expectations. + +**Proposal:** One editing pass; de-number prose everywhere except VERIFICATION.md ('the full offline suite') so the next feature doesn't re-rot the counts. Split the ⚡ legend into id-level (profile-only) vs description-level (offline, sharpens with probe). + +### 37. CLI/payload polish: degraded-payload audience key, redesign overwrite guard, --help formatting, 1-indexed locations + +`polish` · impact **low** · effort **S** + +Four small confirmed papercuts: the degraded advice payload drops the 'audience' key exactly when the overlay breaks (agents keying on it crash), redesign's default output silently clobbers CWD files, --help renders the docstring as a run-on wall with six verbs undescribed, and 0-indexed 'row 0' locations invite off-by-one edits by agents. + +**Proposal:** Add audience to the fallback dict; refuse the default redesign path when it exists ('pass -o to overwrite'); RawDescriptionHelpFormatter + help= strings for the six original verbs; 1-index row references and cite sketch line ranges for sketch bands (the one behavior-visible change — update tests). + +## Verification ledger + +| lens | findings kept | +|---|---| +| adversarial | 12 | +| arch | 10 | +| biux | 15 | +| core | 8 | +| docs | 12 | +| rules | 11 | +| toolux | 12 | + +## Appendix: confirmed findings index + +| # | finding | category | impact | lens | +|---|---|---|---|---| +| 1 | pivot_table is a complete rulebook blind spot | gap | high | biux | +| 2 | Top-N without an order: capped tables are arbitrary samples and no rule notices | gap | medium | biux | +| 3 | big_number_trend has no grain/range guidance anywhere | gap | medium | biux | +| 4 | timeseries_bar's ~30-period cap is stated in the guideline but the linter allows 1000 | gap | medium | biux | +| 5 | Filter-bar composition has one rule; select-count, duplicates, cardinality, and undefaulted time windows are unchecked | gap | medium | biux | +| 6 | Number-format consistency per measure: promised in Tier G, unlintable and partly inexpressible | gap | medium | biux | +| 7 | No color knowledge at all: conditional-formatting bands unchecked, heatmap scale semantics unaddressed | gap | medium | biux | +| 8 | Single-level treemap vs bar: the Cleveland-McGill call the rulebook never makes | missed-opportunity | low | biux | +| 9 | One-line markdown headers default to 4 units, undercutting the whitespace guideline | missed-opportunity | low | biux | +| 10 | The brief teaches rules but shows no exemplar and hides the defaults the rules judge | improvement | medium | biux | +| 11 | design.yaml overlay format is advertised but documented nowhere | gap | medium | docs | +| 12 | VERIFICATION.md's offline-suite breakdown has zero coverage of the design brain | gap | medium | docs | +| 13 | README.md is silent on the design brain and doesn't link DESIGN-BRAIN.md | missed-opportunity | medium | docs | +| 14 | Minimum readable width is preached but never enforced; operational preset permits layouts the rulebook itself calls unreadable | bug | high | biux | +| 15 | chart.pie-slices advice manufactures a lying part-to-whole | bug | high | biux | +| 16 | Category sort-order guidance is factually wrong for bar/pie/funnel and silent on ordinal scrambling | doc-drift | medium | biux | +| 17 | Preset value sanity: exec kpi_row_min=3 false-positives a 2-KPI hero band; operational fold=26 contradicts its own intent | edge-case | medium | biux | +| 18 | Doc drift: spec docstring and design doc still say the filter bar is 'select + time_range'; range filters are invisible to the design brain | doc-drift | low | biux | +| 19 | DESIGN-BRAIN §12 claims two tests that do not exist (hypothesis property tests, nyc_taxi golden) | doc-drift | high | docs | +| 20 | Stale hard-coded counts: '125 tests', '16 modules', 'six MCP tools' across three docs | doc-drift | medium | docs | +| 21 | FEATURES.md 'CLI Verbs' table is missing all four design-brain verbs | doc-drift | medium | docs | +| 22 | DESIGN-BRAIN §10: 'ok is false at gate level (warn under --strict)' — payload ok never reflects --strict | doc-drift | medium | docs | +| 23 | Rule count is 31, not 27 (DESIGN-BRAIN §15.4 and the working notes both say 27) | doc-drift | low | docs | +| 24 | size.hbar-window: doc says fix caps at the fold budget; code uses a hard-coded 20 | doc-drift | low | docs | +| 25 | data.row-limit-intent: doc says 'bar/pie/table'; code covers table + horizontal bar only | doc-drift | low | docs | +| 26 | layout.row-fill emits info for small holes; §7 table lists it as plain warn | doc-drift | low | docs | +| 27 | §7 legend over-claims: '⚡ marks data-aware rules (only run with --profile)' but two ⚡-annotated rules run offline | doc-drift | low | docs | +| 28 | Overlay values are never shape/type-validated: strings crash as 'unexpected' TypeError, scalar disable silently no-ops, out-of-range numbers blow up the fix loop | bug | high | core | +| 29 | Fractional-height 'human polished' suppression swallows width-only size findings, though absorb can never write widths | bug | medium | core | +| 30 | layout.row-density counts vertically STACKED sketch charts as side-by-side row occupants -> false-positive warn | edge-case | medium | core | +| 31 | Duplicate tab titles collapse layout.tab-balance counts (dict keyed by title) and nothing validates tab-title uniqueness | bug | low | core | +| 32 | Polish-suppression runs before ignore bookkeeping, so a finding that is both polished and ignored vanishes from the ignored list | edge-case | low | core | +| 33 | params_for: audience-level recommended_heights wholesale replaces params-level entries instead of merging | gap | low | core | +| 34 | Unknown rule ids in design.ignore / --ignore / overlay disable are accepted silently, so typos no-op invisibly | gap | low | core | +| 35 | Fix-loop convergence rests on an unstated invariant: the one LOWERING fix (size.kpi-height) never collides with the raising fixes | improvement | low | core | +| 36 | data.grain-vs-range is silently disabled when time_grain is omitted, though the compiler defaults it to P1D | bug | high | rules | +| 37 | chart.heatmap-grid probe cap (30) is too low for the 400-cell threshold: huge grids with one small axis pass silently | edge-case | medium | rules | +| 38 | layout.kpi-band mixing warn fires on the canonical sketch pattern the tool itself supports: a stacked KPI column beside a tall chart | bug | medium | rules | +| 39 | size.axis-min-height and size.heatmap-geometry double-report the same defect with conflicting targets | bug | low | rules | +| 40 | fix-loop convergence docstrings are false: size.kpi-height lowers heights | doc-drift | low | rules | +| 41 | narrative.filtered-title turns boolean filters into title demands ('True' should appear in the title) | edge-case | low | rules | +| 42 | layout.tab-balance collapses duplicate tab titles and then reports a fabricated skew | edge-case | low | rules | +| 43 | rows-mode implicit widths floor-divide (sketch mode uses largest-remainder), so layout.row-fill blames the user for a hole the resolver created | improvement | medium | rules | +| 44 | No width rule covers tables/pivots/funnels/treemaps: a row of four 3/12-wide tables passes with zero findings | gap | medium | rules | +| 45 | _GRAIN_DAYS misses sub-hour grains and Superset week-variant grains, silently skipping grain-vs-range | gap | low | rules | +| 46 | Band-level findings cannot be ignored per-band: suppressing one accepted layout.kpi-band silences all of them | gap | low | rules | +| 47 | Autofix-written fractional heights inherit absorb's 'human polished' signature and silence later size rules | bug | high | adversarial | +| 48 | Malformed design.yaml overlay values crash advise with a raw TypeError ('unexpected' CLI error) | bug | high | adversarial | +| 49 | fix_height doesn't clamp to the spec's height cap: overlay/calibrate heights > 100 make the fix loop corrupt the spec, then blame the user | bug | medium | adversarial | +| 50 | layout.row-density counts vertically stacked sketch-column charts as side-by-side row occupants (can escalate to a false error) | bug | medium | adversarial | +| 51 | layout.kpi-band flags the canonical KPI-sidebar stack (KPIs stacked in a column beside a hero chart) | edge-case | medium | adversarial | +| 52 | narrative.title-style misclassifies acronyms as Title Case, flagging uniformly-cased dashboards | bug | low | adversarial | +| 53 | narrative.filtered-title stringifies boolean filter values: "filtered to ['True'] but the title doesn't say so" | edge-case | low | adversarial | +| 54 | Autofix on sketch layouts silently breaks the drawing's WYSIWYG contract with no follow-up finding | edge-case | low | adversarial | +| 55 | Duplicate tab titles are valid and make design-brain 'where' strings ambiguous (and collapse tab-balance weights) | edge-case | low | adversarial | +| 56 | design.ignore entries that match nothing are silently inert — a typo quietly re-enables the rule | gap | medium | adversarial | +| 57 | Implicit-width floor division under-fills rows, then layout.row-fill blames the author for the tool's own arithmetic | improvement | medium | adversarial | +| 58 | data.grain-vs-range suggests 'a finer grain' for zero-length ranges where no grain can help | improvement | low | adversarial | +| 59 | MCP tool-surface test is stale (missing the 3 design-brain tools) and silently skipped in dev | bug | medium | toolux | +| 60 | calibrate builds per-type 'note' entries (needs >= N samples / within 1 unit) then throws them away | bug | medium | toolux | +| 61 | Tool-written fractional heights (calibrated autofixes) masquerade as human polish and permanently silence size rules | edge-case | medium | toolux | +| 62 | README has zero mention of the design brain — the headline 0.2.0 feature is invisible on the landing page | doc-drift | medium | toolux | +| 63 | Degraded advice payload (broken overlay inside check/apply) drops the 'audience' key and reports ok:true | edge-case | low | toolux | +| 64 | redesign/decompile default output lands in CWD and silently overwrites | edge-case | low | toolux | +| 65 | MCP parity gaps that matter: advise_spec drops resolution_errors, check/build carry no advice, 'fixable' is a dead-end signal | gap | medium | toolux | +| 66 | check --design strict gate flips ok:false with empty errors and stage 'resolve' — no design_gate marker | gap | medium | toolux | +| 67 | advise --fix rewrites the whole file in canonical formatting and reports keys without values | improvement | medium | toolux | +| 68 | Finding locations are 0-indexed ('layout row 0') and reference normalized bands, not sketch lines | improvement | low | toolux | +| 69 | Bad audience over MCP is reported as code 'overlay' | improvement | low | toolux | +| 70 | chartwright --help renders the module docstring as an unreadable run-on wall; core verbs lack help lines | improvement | low | toolux | +| 71 | `chartwright absorb` crashes with NameError after mutating the spec file (missing `import json` in absorb.py) | bug | high | arch | +| 72 | Echo chamber: calibrated fractional heights written by --fix masquerade as human polish and permanently blind all size.* rules on the chart | bug | high | arch | +| 73 | Versioning story promised in the doc is not implemented: no `since` markers, no rule-id aliases, and the design_brain version string is triplicated | doc-drift | medium | arch | +| 74 | Sketch-mode advise is superlinear (measured ~cubic): parsed_sketch() and chart lookup re-run per height query | edge-case | medium | arch | +| 75 | Fix-merge convergence rests on an accidental invariant: fix.py's max() assumes all height fixes raise, but size.kpi-height lowers | edge-case | low | arch | +| 76 | Overlay values are never type-validated; one bad value in design.yaml crashes check/apply even at --design warn | gap | high | arch | +| 77 | No per-deployment severity override, and Rule.severity is already unreliable metadata that any future override would mis-key on | gap | medium | arch | +| 78 | Calibration learns one global height per chart type: no audience granularity, no decay, votes live forever | missed-opportunity | medium | arch | +| 79 | A 15th chart type falls through the design brain silently: taxonomy sets and rule literals have no exhaustiveness guard | gap | medium | arch | +| 80 | Heatmap cardinality probe fires twice per column because rule registration order runs the small-cap probe first | improvement | low | arch | diff --git a/docs/DESIGN-BRAIN.md b/docs/DESIGN-BRAIN.md new file mode 100644 index 0000000..ffdc704 --- /dev/null +++ b/docs/DESIGN-BRAIN.md @@ -0,0 +1,530 @@ +# The Design Brain + +> **Status: SHIPPED — design brain 2.** This page is both the design and the +> reference for the implementation in `chartwright/design/`. The decision log +> at the bottom records every judgment call made without a review gate; §15 +> records where the implementation deliberately deviates from the design +> text. A verified multi-lens review of the first implementation produced the +> ranked roadmap in [DESIGN-BRAIN-V2.md](DESIGN-BRAIN-V2.md); all 37 items +> are executed, and §7's rule table is generated from the live registry so it +> cannot rot. Rendering-quality verification against a live Superset (UI +> eyeballing of advised-vs-unadvised dashboards) is still pending — the +> thresholds come from the skill's field notes and BI literature, not yet +> from side-by-side screenshots. + +## 1. Problem + +Chartwright guarantees a dashboard **imports correctly**. It does not +guarantee the dashboard **reads well**. Nothing stops a spec from shipping a +pie chart squeezed into 2 of 12 columns, a vertical bar with 40 category +labels (Superset silently drops most of them), a KPI buried under three rows +of tables, or a 4,000-pixel scroll for an executive audience. Today the only +design knowledge in the system is a prose section in `skill/SKILL.md` — +frozen inside one prompt, invisible to the CLI, not versioned as a surface, +not toggleable, and not testable. + +The design brain is that missing layer: a codified, continuously-improvable +body of BI/UX design knowledge (Few, Tufte, IBCS, and Superset-specific +rendering facts) that the toolchain can consult, enforce, and explain — and +that the user can switch off. + +## 2. Principles + +1. **Same trust model as the rest of the tool.** The LLM stays the untrusted + parser at the edge. Design judgment that requires intelligence is + delivered *to* the LLM as versioned knowledge; design judgment that can be + mechanically checked is enforced *after* the LLM in tested code. Nothing + about the brain loosens the bright line (the spec remains the only + LLM-authored artifact). +2. **Advice, not authority.** The brain never silently changes what data a + chart shows. Autofixes are presentation-only (geometry, orientation). + Anything that would change the data shown (row limits, filters, chart + type) is a finding with a suggested edit, never an automatic one. +3. **Off is really off.** `--design off` (or omitting `advise`) yields + byte-identical behavior to today. The feature is additive; `spec_version` + stays `"1"`. +4. **Human polish outranks the brain.** Heights written back by + `chartwright absorb` are fractional by construction; sizing rules treat a + fractional height as a human decision and stay silent on that chart. +5. **Improvable without architecture.** Adding or refining a guideline is a + markdown edit; adding an enforceable rule is one small function, one + guideline card, and one test. Rule ids are a stable public surface. + +## 3. Architecture + +One knowledge base, two delivery mechanisms, split by who can apply the rule: + +``` + chartwright/design/guidelines/*.md (rule cards: the knowledge) + │ + ┌───────────────┴────────────────┐ + │ Tier G (generative) │ Tier L (lintable) + │ needs intelligence to apply │ mechanically checkable + │ │ + ▼ ▼ + `chartwright brief` `chartwright advise` + a design brief the LLM a deterministic critic over the + reads BEFORE authoring finished spec, with autofix + (skill step 1.5) (skill step 4.5; also in apply/check) +``` + +- **Tier G — the brief.** Composition, grouping, narrative flow, when to use + tabs, title-as-insight, color restraint: judgments only an intelligence can + make. Delivered as a compact markdown brief the skill loads before writing + a spec. Continuously improvable by editing guideline files; no code change. +- **Tier L — the critic.** Predicates over the spec (optionally enriched with + live metadata): minimum readable geometry per chart type, category limits, + layout composition, fold budgets. Implemented as a rule registry in Python, + each rule tested, each finding actionable, safe subset auto-fixable. + +Both tiers share the rule cards, so the brief can tell the LLM what the +critic will later enforce (the LLM pre-complies instead of iterating). + +### Module layout + +``` +chartwright/design/ + __init__.py advise() / advise_and_fix() entry points + model.py Finding, AdviceReport, RuleContext, @rule registry, taxonomy + rules.py all Tier L rule implementations (split when it outgrows one file) + presets.py audience parameter tables + design.yaml overlay + fix.py apply_fixes(spec_data, findings) -> (new_data, applied) + brief.py render_brief(audience) -> markdown + probe.py bounded cardinality probes (data-aware rules) + calibrate.py the absorb-log learning loop + redesign.py decompile -> audit -> fix, in one shot + guidelines/ Tier G knowledge (packaged data, shipped in the wheel) +``` + +## 4. CLI surface + +``` +chartwright advise [--audience A] [--profile P] [--fix] [--strict] + [--ignore rule1,rule2] [--no-probe] +chartwright brief [--audience A] +chartwright redesign --profile P [-o spec.json] [--audience A] [--no-probe] +chartwright calibrate [--write] [--min-samples N] [--since 90d] +``` + +- `advise` (offline by default): evaluates the spec, prints an + `AdviceReport` JSON (shape in §10). Exit 0 unless a finding of severity + `error` exists, or `--strict` and any `warn` exists. +- `advise --profile P`: adds **data-aware** rules — column type checks and + bounded cardinality probes against the live instance (§8). `--no-probe` + keeps it to metadata already fetched by resolution (no queries). +- `advise --fix`: applies the safe-fix subset in place (same file-rewrite + mechanics as `absorb`; formatting normalizes), re-validates, and reports + each change as `{rule, chart, set: {field: new}, was: {field: old}}` plus + the `written` path. Idempotent: a second `--fix` run is a no-op. +- `brief`: prints the Tier G design brief for the audience — the document the + skill reads before authoring. Compact by contract (a test caps the line + count), because it lands in an LLM context window. +- `check`/`apply` gain `--design off|warn|strict` (default `warn`): + - `warn`: advice rides along in the payload under `"advice"`, never blocks. + - `strict`: `error`/`warn` findings block (a `design_gate` entry lands in + `errors` and the exit code is 1); apply blocks BEFORE anything on the + instance is touched. + - `off`: byte-identical to the pre-brain behavior, advice machinery never + runs. +- `calibrate`: the learning loop (§13 phase 3) — mines absorb history into + per-audience recommended heights; `--since` is the decay knob. +- MCP server: `design_brief`, `advise_spec`, `fix_spec`, `redesign_dashboard` + mirror the CLI verbs; `check_spec` carries the advice block. + +## 5. Spec surface + +One additive optional block (models stay `extra="forbid"`): + +```json +"design": { + "audience": "executive", + "ignore": ["size.pie-geometry", "layout.fold-budget@Ops Detail"] +} +``` + +- `audience`: this dashboard's preset; CLI `--audience` overrides it, the + built-in default (`analytical`) applies when both are absent. +- `ignore`: rule ids to suppress, dashboard-wide (`rule.id`), per chart + (`rule.id@Chart Name`), or per band for findings that name no chart + (`rule.id@tab-Ops-row-1` — the finding's `where`, slugified). Suppressions + are reported in every AdviceReport (`"ignored"`), so silence is always + visible; entries whose rule id doesn't exist come back as + `unmatched_ignores` instead of silently suppressing nothing. + +Precedence everywhere: CLI flag > spec `design` block > built-in default. + +## 6. Audiences + +A professional designer designs for a reader. Rules read their thresholds +from an audience preset, so one rulebook serves three very different +dashboards. Heights are in spec units (1 unit = 40 px; a laptop viewport +minus Superset chrome is ≈ 22 units). + +| Parameter | `executive` | `analytical` (default) | `operational` | +|---|---|---|---| +| Intent | one screen, few numbers, big | scrolling analysis, depth | dense wall/monitor view | +| `fold_units` (height budget, per tab) | 22 | 66 | 22 | +| `max_row_charts` (axis slots per row) | 3 | 4 | 4 | +| `kpi_per_row` (min–max) | 2–5 | 2–6 | 2–8 | +| `kpi_height` | 5 | 4 | 3 | +| `min_axis_height` | 8 | 6 | 5 | +| `max_filter_selects` | 5 | 6 | 7 | +| `table_visible_ratio` (min visible/row_limit) | 0.5 | 0.5 | 0.5 | +| `vbar_max_categories` | 6 | 8 | 8 | +| `pie_max_slices` | 5 | 7 | 7 | +| `series_max` (lines per timeseries) | 5 | 10 | 8 | + +Presets are data (`presets.py`), not branches: rules never test the audience +name, only parameters. Adding an audience is adding a row. + +### House style: design.yaml + +`~/.config/chartwright/design.yaml` (or `$CHARTWRIGHT_DESIGN_DIR/design.yaml`) +overlays the presets for a whole deployment — no fork of the rulebook. Five +keys, all optional, all validated at load with typed errors: + +```yaml +params: # every audience + min_axis_height: 7 +audiences: # one audience + executive: {fold_units: 20, recommended_heights: {table: 12}} +disable: [narrative.title-style] # rule ids (aliases accepted) +severity: {filters.time-default: warn} # per-deployment level overrides +recommended_heights: {table: 11} # calibrate --write maintains these +brief_extra: | + House style: fiscal weeks start Sunday; money is always '$,.0f'. +``` + +`recommended_heights` merges per key across preset -> overlay -> per-audience +layers; the brief prints the merged values and height autofixes target them. + +## 7. The rulebook + +Stable ids (`category.slug`) are the public API — `ignore`/`disable` lists +and severity overrides key on them, and renames keep working through the +alias table. Severity is the rule's DEFAULT level: **error** = unreadable +for any audience; **warn** = below professional quality; **info** = polish +nudge (a few rules vary it per finding, and the overlay can override it per +deployment). "fix" marks the safe-autofix subset (presentation-only, §9). +"data" marks rules that only run with a live resolution (`--profile`); +several offline rules additionally sharpen or stand down when probes are +available (noted in their text). "v2" marks rules added by the +post-review batch (docs/DESIGN-BRAIN-V2.md). + +The table below is GENERATED from the registry — do not hand-edit it; +rerun the snippet in the comment and splice. + + + +| id | sev | fix | data | since | rule | +|---|---|---|---|---|---| +| `chart.dupe` | info | — | — | 1 | two charts answering the identical question is redundancy | +| `chart.format-bands` | warn | — | — | 2 | conditional-formatting bands must tell one coherent story per metric | +| `chart.funnel-stages` | warn | — | ⚡ | 1 | funnels need 3-8 ordered stages | +| `chart.heatmap-grid` | warn | — | ⚡ | 1 | a heatmap past ~400 cells is unreadable at any size | +| `chart.histogram-bins` | info | — | — | 1 | histograms read best at 10-50 bins | +| `chart.metrics-per-bar` | warn | — | — | 1 | many metrics per category read better as a table | +| `chart.ordinal-order` | info | — | — | 2 | ordinal dimensions (weekday, month) sort alphabetically unless order-encoded | +| `chart.pie-slices` | warn | — | — | 1 | pies stop working past ~7 slices | +| `chart.pivot-columns` | warn | — | ⚡ | 2 | column-dim values x metrics = rendered columns; past ~15 the pivot scrolls sideways | +| `chart.pivot-dims` | warn | — | — | 2 | a pivot past three total dimensions is unreadable nesting | +| `chart.series-limit` | warn | — | ⚡ | 1 | a timeseries with too many grouped series turns to spaghetti | +| `chart.temporal-type` | error | — | ⚡ | 1 | a time axis must point at a temporal column | +| `chart.treemap-depth` | warn | — | — | 1 | treemaps past two grouping levels become unreadable nesting | +| `chart.treemap-vs-bar` | info | — | ⚡ | 2 | a one-level treemap of few categories is a worse bar chart | +| `chart.trend-grain` | info | — | — | 2 | trend tiles at a fine grain over full history draw thousands of points in a small card | +| `chart.vbar-categories` | warn | ✔ | — | 1 | vertical bars drop labels past ~8 categories; rank with horizontal bars | +| `data.grain-vs-range` | warn | — | — | 1 | the time grain should yield a sane number of points for the range | +| `data.row-limit-intent` | info | — | — | 1 | row limits doing design work should be deliberate, not defaults | +| `data.top-n-sort` | warn | — | — | 2 | a limit without an order is a sample, not a ranking | +| `filters.count` | warn | — | — | 2 | past ~6 select pickers a filter bar stops being navigable (and each costs a query on load) | +| `filters.duplicate-column` | info | — | — | 2 | two filters on the same column fight each other | +| `filters.range-default` | info | — | — | 2 | a range slider with no default bounds spans the whole domain | +| `filters.select-cardinality` | warn | — | ⚡ | 2 | a select over hundreds of distinct values is an unusable picker | +| `filters.time-default` | info | — | — | 2 | an undefaulted time picker loads the dashboard over ALL history | +| `filters.time-picker` | info | — | — | 1 | time-based dashboards want a time range picker in the filter bar | +| `layout.fold-budget` | warn | — | — | 1 | the dashboard should fit its audience's scroll budget | +| `layout.kpi-band` | warn | — | — | 1 | KPIs get their own band, in readable numbers | +| `layout.kpi-first` | warn | — | — | 1 | summary KPIs belong above detail charts (inverted pyramid) | +| `layout.markdown-height` | info | ✔ | — | 2 | a one-line markdown header doesn't need a chart-sized block | +| `layout.orphan-chart` | info | — | — | 1 | a lone narrow chart in its own row looks unfinished | +| `layout.row-density` | warn | — | — | 1 | too many axis charts side by side starves each of width | +| `layout.row-fill` | warn | — | — | 1 | a row should fill the 12-column grid | +| `layout.section-headers` | info | — | — | 1 | large flat dashboards need markdown signposts | +| `layout.tab-balance` | info | — | — | 1 | tabs should carry comparable weight | +| `narrative.big-number-format` | info | — | — | 1 | hero numbers deserve a number format | +| `narrative.filtered-title` | info | — | — | 1 | a filtered chart's title should say what it shows | +| `narrative.format-consistency` | info | — | — | 2 | one measure, one number format | +| `narrative.title-style` | info | — | — | 1 | chart titles should share one casing style | +| `size.axis-min-height` | warn | ✔ | — | 1 | axis charts below the audience minimum height flatten and drop labels | +| `size.grid-fit` | warn | ✔ | ⚡ | 2 | table/pivot heights must fit their data-driven row counts (they grow after authoring) | +| `size.hbar-window` | warn | ✔ | — | 1 | horizontal bars need ~0.5 units of height per bar | +| `size.heatmap-geometry` | warn | ✔ | — | 1 | heatmaps need >= 5/12 width (7/12 with many columns) and 6 height | +| `size.kpi-height` | warn | ✔ | — | 1 | big numbers read best at 2-6 units | +| `size.min-width` | warn | — | — | 2 | below 3/12 width a chart is unreadable; KPIs need 2/12 | +| `size.pie-geometry` | warn | ✔ | — | 1 | pies need >= 5/12 width and 8 height or the ring shrinks and the legend crowds | +| `size.pivot-window` | warn | ✔ | — | 2 | a pivot's height should show a meaningful share of its row_limit | +| `size.row-harmony` | warn | ✔ | — | 1 | charts sharing a row should share a height (Superset sizes the row to its tallest child) | +| `size.table-window` | warn | ✔ | — | 1 | a table's height should show a meaningful share of its row_limit | + +Anything fuzzier than this (reading order beyond KPI-first, grouping +related metrics, matched granularity across a row, insight-stating titles) +is Tier G: it goes in the brief, not the linter. + +## 8. Data-aware mode + +Resolution already fetches full column metadata (`dataset_detail`) and +discards everything but names. Phase 2 extends `ResolvedDataset` with +`column_types` (Superset's `type_generic`) and the `is_dttm` set — zero extra +API calls — which powers `chart.temporal-type`. + +Cardinality (`chart.pie-slices` ⚡, `chart.series-limit`, `chart.funnel-stages`, +`chart.heatmap-grid`) uses one bounded probe per distinct (dataset, column): +a `COUNT` grouped query through `/api/v1/chart/data` (the smoke module's +existing machinery) with `row_limit = threshold + 1` — the rule only needs +"more than N", never the true count. Probes are cached per run, skipped +entirely under `--no-probe`, and never run for `advise` without `--profile`. + +## 9. Autofix semantics + +- **Safe set only:** heights, widths (rows mode), bar orientation. All + presentation; a fixed spec queries identically to the unfixed one. +- Mechanics mirror `absorb`: findings carry a patch + (`{"chart": "Top Products", "set": {"height": 8}}`), `fix.py` applies them + to the raw spec JSON, the result is re-validated before writing, and the + report lists every applied fix as `rule@chart`. +- **Idempotent by test:** fixed specs re-advise with zero fixable findings. +- Sketch layouts: heights are fixable (an explicit `chart.height` overrides + sketch height by existing precedence in `spec.resolved_height`); widths are + not (the drawing is authoritative) — width findings under a sketch include + a suggested redrawn sketch line for the LLM or human to adopt. +- Fractional heights (absorb's signature) suppress sizing rules on that + chart: human polish wins (§2.4). + +## 10. Output contract + +`advise` speaks the same JSON dialect as every other stage: + +```json +{ + "stage": "design", + "ok": true, + "design_brain": "2", + "audience": "analytical", + "counts": {"error": 0, "warn": 2, "info": 1}, + "findings": [ + { + "rule": "size.pie-geometry", + "severity": "warn", + "chart": "Sales by Region", + "where": "layout row 2", + "detail": "pie squeezed: height 4 < 8; ring shrinks and legend crowds", + "fixable": true + } + ], + "fixed": [ + {"finding": "size.pie-geometry@Sales by Region", "rule": "size.pie-geometry", + "chart": "Sales by Region", "set": {"height": 8}, "was": {"height": 4}} + ], + "ignored": ["layout.fold-budget"], + "unmatched_ignores": ["size.pie-geometri"], + "polished": ["size.axis-min-height@Weekly Orders"] +} +``` + +- `ok` is false iff a finding of severity `error` exists. The `--strict` gate + (and `--design strict` on check/apply) rides the exit code and appends a + `design_gate` entry to `errors`; it does not redefine `ok`'s meaning. +- `fixed` entries disclose the full diff (`set` new values, `was` old); + `advise --fix` additionally reports the `written` file path. +- `unmatched_ignores` lists ignore/disable entries whose rule id doesn't + exist — a typo'd suppression is surfaced, never a silent no-op. +- `polished` lists sizing findings withheld because the chart carries a + human-polished (fractional) height — §2.4's deference, made visible. + `ignored` is the user's *explicit* intent; `polished` is the brain's own + *inference*, and an inference that silences a rule invisibly reads exactly + like the rule having passed. Present only when non-empty. +- Under `apply --design warn`, this object is embedded in the apply report + as `"advice"` and never affects `apply`'s own `ok`. Under + `--design strict` the gate fails CLOSED: if advice could not be evaluated + at all (a broken `design.yaml`), that blocks too, rather than reporting + counts of zero and passing. + +## 11. Interactions with the existing system + +- **`absorb`:** fractional heights silence sizing rules (§9). The two flows + compose: brain roughs in professional geometry, human drags to taste, + absorb records it, brain respects it forever after. +- **`decompile`:** running `advise` on a decompiled spec is a **design audit + of any legacy UI-built dashboard** — an emergent feature worth documenting: + `chartwright decompile old-dash -o spec.json && chartwright advise spec.json`. +- **`redesign`:** the one-shot form of the above: decompile → data-aware + audit → safe geometry fixes → redesigned spec + losses + remaining + structural findings. Ownership decides where it lands: tool-born + dashboards redesign in place; UI-born ones come back under a `-redesign` + slug (title suffixed too) so apply builds the redesign **side by side** + and the original is never overwritten. Structural findings stay findings — + the spec author (usually the skill-driven LLM) acts on them before apply. +- **`smoke` (issue #1):** table/pivot heights are fixed layout properties + while rendered rows are data-driven — data that grows after authoring hides + new rows behind the chart's inner scrollbar with everything looking green. + Smoke now compares the rows its query already fetched against the + configured height and warns on every apply (`~9 leaf rows (~430px) but + height=8 (320px)`); the data-aware `size.grid-fit` rule catches the same + class pre-apply for single-dimension grids. All of it — smoke, + `size.grid-fit`, `size.table-window`, `size.pivot-window` — reads ONE grid + model (`grid_units_for_rows` / `grid_rows_visible` in `chartwright/spec.py`), + so the offline critic, the data-aware critic, and the apply-time warning + cannot give one chart three different verdicts. +- **Chart identity:** no rule may ever autofix a chart `name` — names seed + uuid5 identity; a rename is a delete+create on the live instance. +- **`plan`/golden tests:** advise is pure spec-side analysis; compiled bytes + are untouched, golden tests unaffected. +- **Skill (`skill/SKILL.md`):** the static "Design rules" section is replaced + by two procedure steps: + - *Step 1.5* — brain on (default): run `CW brief --audience ` + and follow it while authoring. Infer audience from the request + ("executive scorecard" / "ops monitor" / default analytical). Brain off + (user said "no design opinions" / "exactly as I specify"): skip the + brief, pass `--design off`. + - *Step 4.5* — after `check` passes: `CW advise --profile

`; + apply or consciously `ignore` findings (with the user, in the spec's + `design.ignore`) — at most 2 design iterations, then surface remaining + findings verbatim. + - Anti-evasion row: *"Advice finding seems wrong → record it in + `design.ignore` and tell the user, or report a rule bug; never hand-tune + output to dodge the critic."* + +## 12. Testing + +What actually runs (tests/test_design*.py, test_calibrate.py, test_redesign.py): + +- **Per rule, table-driven:** a clean spec stays silent; a violating spec + fires exactly the expected finding; the autofixed spec re-advises clean. +- **Fix-loop invariants:** convergence and idempotence; autofixes never mint + fractional (human-signature) heights; the KPI clamp cannot oscillate. +- **Seeded fuzz:** advise never raises across a grid of height/width/layout + mutations of a mixed-type spec. +- **Golden dogfood:** `examples/nyc_taxi_operations.json` advises clean at + `analytical` (its two deliberate exceptions recorded in `design.ignore`). +- **Contract tests:** the chart-type taxonomy exactly covers `CHART_TYPES` + (a 15th type fails CI until classified); one sketch parse per holder + (the geometry-cache bound); brief line budget per audience. +- **Trust-boundary tests:** every design.yaml value class rejects with a + typed error; typo'd ignores surface as `unmatched_ignores`. + +## 13. Phases + +| Phase | Scope | +|---|---| +| **1** (next minor) | design/ package, offline rulebook (all non-⚡ rules), presets, `advise`/`brief` CLI, `--fix`, spec `design` block, `apply`/`check --design`, skill rewrite, MCP tools, tests | +| **2** | data-aware rules (resolver type enrichment, bounded cardinality probes, `--no-probe`), house-style overlay: `~/.config/chartwright/design.yaml` (org-level parameter overrides, disabled rules, extra Tier G guidance appended to the brief) | +| **3** | calibration loop: mine absorb history and backups for systematic human corrections (e.g. tables consistently dragged from 8 to 11 units) and propose preset updates — the brain learns from every human polish it was told to respect | + +## 14. Decision log + +Judgment calls made without a review gate, per instruction; each is +reversible and none is load-bearing enough to block on: + +1. **Default `--design warn` on apply/check** (brain visible by default, + never blocking). Rationale: an invisible-by-default brain never gets used; + a blocking-by-default one breaks existing pipelines. +2. **Audience presets named `executive`/`analytical`/`operational`**, with + `analytical` default; "audience" chosen over "profile" (collides with + connection profiles) and "preset". +3. **Autofix = presentation only.** Anything data-affecting (row_limit, + chart type, filters) is suggestion-only, however confident the rule. +4. **`chart.temporal-type` lives in advise (error), not `check`.** It is + type-correctness, but moving it into check changes a shipped gate's + behavior on specs that currently pass; advise-as-error gets the same + protection without a silent contract change. +5. **Rules are Python functions in a registry, not a YAML/DSL engine.** + Stable ids + parameter tables give the evolvability a DSL would; the DSL + is speculative machinery (YAGNI). +6. **Fractional height = human polish → sizing rules skip the chart.** + Zero-config way to honor absorb without a new spec field. +7. **No design score in v1.** A 0–100 number invites gaming and argues with + itself; counts by severity carry the same information honestly. +8. **Guidelines ship as package data** so `brief` works from a pip install, + not only a repo checkout. +9. **`spec_version` stays `"1"`;** the `design` block is additive and + optional. No migration. +10. **Names never autofixed** (uuid identity churn), even for the + title-style rule. +11. **Chart-name casing, cross-dataset filter applicability, and + reading-order-beyond-KPI-first stay Tier G** (or info-severity): + heuristics too fuzzy to enforce mechanically without false positives. +12. **Learning/calibration deferred to phase 3**; the architecture point is + that rules-as-data + stable ids make it a parameter update, not a + rewrite. + +## 15. Implementation deviations (recorded, not silent) + +Where the shipped code deliberately departs from the design text above: + +1. **Width autofixes are report-only everywhere**, not just under sketches. + A width change can overflow a row's 12-column sum and invalidate the + spec; the finding carries the suggested width (or redrawn sketch run) + instead. Heights remain fully autofixable. +2. **`apply --design` advises offline** (no resolution, no probes) so + applies stay fast; `check --design` attaches type-aware advice for free + (it already resolved); full data-aware advice is `advise --profile`. +3. **`size.axis-min-height` fixes to the audience minimum** (8/6/5 by + audience), not a blanket 8 — a dense operational board shouldn't be + inflated to analytical proportions. +4. **Rule cards are the registry docstrings** (one line per rule, printed in + the brief) plus two Tier G guideline files; 27 separate markdown cards + would have been boilerplate. +5. **Phase 3 concretely:** `absorb` appends to + `~/.config/chartwright/absorb-log.jsonl` (one vote per profile/slug/chart, + latest wins) and `chartwright calibrate [--write]` proposes/records + `recommended_heights` in the overlay, which the brief prints and height + autofixes target. `$CHARTWRIGHT_DESIGN_DIR` relocates both files. +6. **`chart.dupe` proved itself in testing**: it flagged the test suite's own + lazily-copied KPIs. Working as intended. + +Recorded during the v2 roadmap burn-down: + +7. **Row references stay 0-indexed** everywhere (`layout row 0`), matching + the spec validator's long-shipped messages; the review's 1-indexing + suggestion was declined for consistency. +8. **`filters.time-default` ships as info for every audience**; deployments + that want it blocking for executives raise it via the overlay's + `severity` map rather than a boolean param. +9. **Sketch WYSIWYG resolved as disclosure, not withholding**: height fixes + still apply to sketch-drawn charts (explicit heights legitimately override + the drawing — absorb's precedent), and the finding says the drawing goes + stale and how to redraw it. +10. **A per-metric d3 format on table/pivot/timeseries is a spec v-next + candidate** (the compiler pins SMART_NUMBER today); the guideline was + softened to what the spec can express rather than promising the + inexpressible. + +Recorded during the post-merge review burn-down: + +11. **One grid model, three consumers.** `size.table-window` (0.8 units/row, + 1 header unit), `size.grid-fit` (0.75 + 3) and smoke (30px/row + 3×40px) + each modelled a rendered grid row differently, so a 20-row table at + height 8 passed offline while the data-aware rule and the apply warning + both said it hid ten rows. The constants now live once in + `chartwright/spec.py`; the rules and smoke import them. +12. **`table_visible_ratio` is 0.5 for every audience**, up from 0.25 on + `analytical`/`operational`. Below half, the MAJORITY of the rows the + author deliberately asked for sit behind the inner scrollbar — the exact + defect §11's issue-#1 work exists to catch, so the offline rule must not + bless it. It remains a per-deployment knob; it is no longer a lenient + default. `size.table-window` and `size.pivot-window` also became + autofixable (a height raise, guarded to sane heights) now that they + compute a real target rather than a ratio verdict. +13. **The polish skip is disclosed, not silent** (`polished` in §10). §2.4's + deference to a human-dragged height is an INFERRED signal; v2 theme 1 + named it alongside `unmatched_ignores` and the strict-gate `ok`, and only + the other two landed. Known limits of the inference, unchanged: a drag to + an exact 40px boundary absorbs as an integer and is not recognized, and a + hand-written fractional height silences sizing rules without the author + intending it. Explicit provenance (a spec field written by `absorb`) is + the real fix and stays a v-next candidate; reporting it is the floor. +14. **`--design strict` fails closed.** Advice still degrades to an error + note instead of crashing check/apply, but under `strict` an unevaluable + overlay blocks: previously one typo in an org-wide `design.yaml` + silently disarmed the gate everywhere it was used. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 1aa4bf2..06db8d5 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -30,6 +30,16 @@ that way. Everything below works from that one file. - **Precise Sizing**: Set exact widths and heights per chart, or drag a chart taller in the UI and `chartwright absorb` writes the new height back into the spec; widths are a one-line edit in the layout. - **Rows, Tabs, and Notes**: Even or custom row splits, titled tabs, and markdown blocks for headers and notes. +## The Design Brain +- **A Visual Designer On Call**: An optional intelligence layer that knows BI/UX practice (Few, Tufte, IBCS) and Superset's rendering quirks, on by default and off with one flag ([full reference](DESIGN-BRAIN.md)). +- **The Brief**: `chartwright brief` prints design guidance tuned to an audience preset (`executive` / `analytical` / `operational`) — budgets, chart choice, composition — for the AI (or you) to read before writing a spec. +- **The Critic**: `chartwright advise` reviews a finished spec: readable minimum sizes, layout composition (KPIs first, fold budgets, row density), chart-choice limits, narrative polish. `--fix` applies the safe geometry subset; `--profile` adds data-aware checks (a time axis on a non-temporal column, a pie hiding 40 slices). +- **Deliberate Exceptions, Visible**: Suppress any rule per dashboard or per chart in the spec's `design` block; suppressions are reported, never silent. +- **House Style**: A `design.yaml` overlay tunes thresholds, disables rules, and appends org guidance to the brief — no fork of the rulebook. +- **It Learns From You**: Heights you polish in the UI flow back via `absorb`; `chartwright calibrate` mines them and updates the recommended heights the brief and autofixes use. +- **Design Audits of Legacy Dashboards**: `decompile` + `advise` grades any UI-built dashboard against the rulebook. +- **One-Shot Redesign**: `chartwright redesign ` decompiles a live dashboard, audits it, applies the safe geometry fixes, and writes the redesigned spec — side by side under a new slug when the original isn't tool-built, in place when it is. + ## Dashboards as Code - **Drift Detection**: `chartwright plan` diffs the spec against the live dashboard: charts, filters, scopes, title, layout. - **Adopt Existing Dashboards**: Turn any dashboard built in the UI into a spec with `chartwright decompile`; anything it cannot carry over is listed, so you know exactly what stayed in the UI. @@ -82,7 +92,7 @@ that way. Everything below works from that one file. | `chartwright restore` | Bring back a backed-up dashboard, completely | ## Testing and Evidence -- **125 tests** on Linux and Windows on every push. +- **The full offline suite** on Linux and Windows on every push, and on every pull request ([exact counts](VERIFICATION.md)). - **Live CI against real Superset 4.1.4, 5.0.0, and 6.1.0** on every push: full apply, lifecycle soak, stale-tab adversary, fault injection. - **500-cycle soak** passed on the oldest and newest supported versions. - **Chart options verified against Superset's own source code** for every supported version, so a Superset change is caught in our tests before it reaches your dashboards. diff --git a/docs/VERIFICATION.md b/docs/VERIFICATION.md index 2598a0f..c073c28 100644 --- a/docs/VERIFICATION.md +++ b/docs/VERIFICATION.md @@ -34,7 +34,7 @@ injection. | Layer | Proves | Where it runs | |---|---|---| -| Offline suite (125 tests, 16 modules) | Contract, determinism, round-trips, credentials | every push, Linux + Windows | +| Offline suite (236 tests, 26 modules) | Contract, determinism, round-trips, credentials | every push and every PR, Linux + Windows | | Chart-option contract | Every emitted chart option is declared by each version's plugin source | every push | | Live guarantee check | 15-chart apply, per-chart data check, ids stable across re-apply | every push, all 3 versions | | Lifecycle soak | 500 randomized edit cycles with invariants held | 500 cycles on 6.1.0 and 4.1.4 before release; 25 cycles per version on every push | @@ -42,7 +42,12 @@ injection. | Fault injection | A typed failure at every stage boundary; complete restore | every push, all 3 versions | | Real-instance use | Production 4.1.x on Windows, real datasets, real users | ongoing | -## 1. Offline suite: 125 tests +## 1. Offline suite + +The counts in the table above are the only hard numbers in the docs, and +`tests/test_docs.py` fails when they drift from what pytest actually +collects — the same "generated, not hand-maintained" rule the rule table in +[DESIGN-BRAIN.md](DESIGN-BRAIN.md) §7 follows. - **Spec contract** (`test_spec.py`, `test_spec_v2.py`): validation semantics. A layout is rows or tabs, never both; row widths must fit the @@ -71,10 +76,17 @@ injection. in both shell-string and argument-list form, home-directory expansion, TLS options, and a named error when no credential source exists. - **Drift and plan normalization** (`test_dashdiff.py`), **MCP parity** - (`test_mcp_server.py`: the six MCP tools mirror the CLI one to one), + (`test_mcp_server.py`: the MCP tools mirror the CLI one to one), **backup layout** (`test_backup_layout.py`: backups are separated per profile and the location override is honored), plus dedicated suites for pivot formatting, range filters, layout sketches, and absorb. +- **The design brain** (`test_design*.py`, `test_calibrate.py`, + `test_redesign.py`): every rule table-driven against violating and clean + specs; fix-loop convergence, idempotence, and the no-fractional-heights + invariant; a seeded advise-never-raises fuzz; the chart-type taxonomy + contract; design.yaml trust-boundary validation; the golden dogfood + (the shipped example advises clean); calibration grouping, decay, and + overlay round-trips. ## 2. Chart options, checked against plugin source @@ -207,7 +219,7 @@ layer exists. ```bash # offline (any machine, no Superset needed) -python -m pytest tests/ -q # 125 tests +python -m pytest tests/ -q # the offline suite python tools/params_drift.py --all # chart options vs plugin source, 3 versions # live (any sandbox; sandbox/up.sh --tag boots one) diff --git a/examples/nyc_taxi_operations.json b/examples/nyc_taxi_operations.json index b0061aa..bab0abe 100644 --- a/examples/nyc_taxi_operations.json +++ b/examples/nyc_taxi_operations.json @@ -8,7 +8,11 @@ { "name": "Total Trips", "type": "big_number_total", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, "metric": "COUNT(*)", "subtitle": "completed trips", "number_format": ",.0f" @@ -16,7 +20,11 @@ { "name": "Total Revenue", "type": "big_number_total", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, "metric": "SUM(total_amount)", "subtitle": "all charges, USD", "number_format": "$,.0f" @@ -24,7 +32,11 @@ { "name": "Average Fare", "type": "big_number_total", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, "metric": "AVG(fare_amount)", "subtitle": "per trip, USD", "number_format": "$,.2f" @@ -32,7 +44,11 @@ { "name": "Average Trip Distance", "type": "big_number_total", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, "metric": "AVG(trip_distance)", "subtitle": "miles", "number_format": ",.2f" @@ -40,23 +56,39 @@ { "name": "Daily Trips", "type": "timeseries_line", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, - "metrics": ["COUNT(*) AS Trips"], + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, + "metrics": [ + "COUNT(*) AS Trips" + ], "time_column": "pickup_time", "time_grain": "P1D" }, { "name": "Daily Revenue", "type": "timeseries_area", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, - "metrics": ["SUM(total_amount) AS Revenue"], + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, + "metrics": [ + "SUM(total_amount) AS Revenue" + ], "time_column": "pickup_time", "time_grain": "P1D" }, { "name": "Demand by Hour and Weekday", "type": "heatmap", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, "x_column": "pickup_hour", "y_column": "pickup_weekday", "metric": "COUNT(*)" @@ -64,35 +96,61 @@ { "name": "How Riders Pay", "type": "pie", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, "metric": "COUNT(*)", "groupby": "payment_method", - "donut": true + "donut": true, + "row_limit": 7 }, { "name": "Trips by Pickup Borough", "type": "bar", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, "x_column": "pickup_borough", - "metrics": ["COUNT(*) AS Trips"] + "metrics": [ + "COUNT(*) AS Trips" + ], + "row_limit": 8 }, { "name": "Top 10 Pickup Zones", "type": "bar", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, "x_column": "pickup_zone", - "metrics": ["COUNT(*) AS Trips"], + "metrics": [ + "COUNT(*) AS Trips" + ], "orientation": "horizontal", "row_limit": 10 }, { "name": "Trip Distance Distribution (trips up to 20 mi)", "type": "histogram", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, "column": "trip_distance", "bins": 25, "filters": [ - {"column": "trip_distance", "op": "<=", "value": 20} + { + "column": "trip_distance", + "op": "<=", + "value": 20 + } ] } ], @@ -104,13 +162,21 @@ { "type": "select", "name": "Pickup Borough", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, "column": "pickup_borough" }, { "type": "select", "name": "Payment Method", - "dataset": {"database": "examples", "schema": "main", "table": "nyc_yellow_taxi"}, + "dataset": { + "database": "examples", + "schema": "main", + "table": "nyc_yellow_taxi" + }, "column": "payment_method" } ], @@ -150,5 +216,11 @@ "J": "Top 10 Pickup Zones", "K": "Trip Distance Distribution (trips up to 20 mi)" } + }, + "design": { + "ignore": [ + "filters.time-default", + "chart.ordinal-order@Demand by Hour and Weekday" + ] } } diff --git a/pyproject.toml b/pyproject.toml index e4200c9..1f82f92 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "chartwright" -version = "0.1.0" +version = "0.3.0" description = "Build, verify, and maintain Apache Superset dashboards from small spec files." license = "Apache-2.0" readme = "README.md" @@ -40,4 +40,7 @@ requires = ["setuptools>=68"] build-backend = "setuptools.build_meta" [tool.setuptools] -packages = ["chartwright"] +packages = ["chartwright", "chartwright.design"] + +[tool.setuptools.package-data] +"chartwright.design" = ["guidelines/*.md"] diff --git a/skill/SKILL.md b/skill/SKILL.md index 1297ec7..8847179 100644 --- a/skill/SKILL.md +++ b/skill/SKILL.md @@ -40,51 +40,60 @@ approximate it with a different mechanism. pivot_table, heatmap, histogram, funnel, treemap), per-chart `filters` (WHERE), dashboard-level `filters` (select + time_range native filter bar, time_range with an optional `default`), layout as `rows`, `tabs`, or an - ASCII `sketch` with a `legend`, markdown blocks in rows. -2. Write the spec to the specs dir (absolute path). Slug lowercase-kebab; + ASCII `sketch` with a `legend`, markdown blocks in rows, an optional + `design` block (audience + rule suppressions). +2. Design brain, ON by default: run `CW brief --audience ` and follow it + while authoring. Infer the audience from the request: executive + scorecard/leadership review -> `executive`; ops monitor/wall display -> + `operational`; anything else -> `analytical`. Record the audience in the + spec's `design.audience`. Brain OFF (user said "no design opinions" / + "exactly as I specify"): skip the brief, skip step 6, and pass + `--design off` to check and apply. +3. Write the spec to the specs dir (absolute path). Slug lowercase-kebab; chart names unique. -3. `CW validate `: fix schema errors (max 3 attempts). -4. `CW check --profile `: fix referential errors - (max 3 attempts total across 3+4; errors name the exact dataset/column/ - metric at fault). -5. `CW apply --profile `: on success give the user +4. `CW validate `: fix schema errors (max 3 attempts). +5. `CW check --profile `: fix referential errors + (max 3 attempts total across 4+5; errors name the exact dataset/column/ + metric at fault). The payload carries an `advice` block (design findings); + act on it in step 6. +6. `CW advise --profile `: the design critic, with + data-aware rules (column types, cardinality). Apply what it suggests: + `CW advise --fix` applies the safe geometry subset in + place; the rest you edit in the spec. A finding that is a deliberate + exception goes in the spec's `design.ignore` as `"rule.id@Chart Name"`, + and you tell the user. At most 2 design iterations, then surface the + remaining findings verbatim. +7. `CW apply --profile `: on success give the user the dashboard_url and any smoke warnings verbatim. Apply backs up the previous state under `~/.config/chartwright/backups///` (restore with `CW restore --profile `). -6. Modify tool-born dashboards by editing their spec and re-running 4-5. +8. Modify tool-born dashboards by editing their spec and re-running 5-7. Modify UI-born dashboards via `CW decompile --profile -o `; - show the user the lossiness report before editing. -7. If drift is possible (someone edited in the UI), run + show the user the lossiness report before editing. `CW advise` on a + decompiled spec is a design audit of a legacy dashboard. To redesign one + in one shot, `CW redesign --profile -o + `: decompile + audit + safe geometry fixes; act on the + remaining structural findings by editing the spec (step 6 rules apply), + show the user the losses and the `next` line, then apply. A dashboard the + tool does not own comes back under a `-redesign` slug: apply builds it + side by side, never over the original. +9. If drift is possible (someone edited in the UI), run `CW plan --profile ` first and surface what apply would change. -## Design rules (the dashboard must read well as generated) +## Sketch heights (while drawing) -Sketch heights: each sketch line adds `line` height units (default `line: 2`, -one unit = 40 px). Draw: +Each sketch line adds `line` height units (default `line: 2`, one unit = +40 px). KPI rows: 2 sketch lines. Axis charts (timeseries, bar, heatmap, +histogram): 4-5 lines; fewer renders flattened with labels dropped. +Pie/donut: 4+ lines and >= 5 of 12 width. The brief carries the full sizing +table; `CW advise` checks the result. -- Big-number (KPI) rows: 2 sketch lines. -- Every chart with axes (timeseries, bar, heatmap, histogram): 4 to 5 sketch - lines. Fewer than 4 renders flattened, with axis labels dropped. -- Pie/donut: 4+ lines and at least 5 of 12 row width, or the ring shrinks - and the legend crowds it. -- A heatmap with many columns: 7 of 12 width or more, 5 lines. - -Chart choice: - -- A ranking over a dimension with many or long values: `bar` with - `"orientation": "horizontal"` and a `row_limit` near 10; vertical bars fit - at most ~8 category labels before Superset starts dropping them. When the - user wants several measures per item, use a `table` instead. -- A histogram over a long-tailed column: trim the tail with a chart-level - WHERE filter and name the trim in the chart title so the chart says what - it shows; 20 to 30 bins. -- Category axes sort alphabetically. When the dataset carries an - order-encoded label column (labels prefixed with their sort index), chart - that column instead of the natural-name column. -- One dominant category flattens its siblings in a vertical bar; that is the - data talking, not a defect; note it or filter it, don't hide it. +Heights the user polished by hand in the UI come back via +`CW absorb --profile ` as fractional units; the +design brain respects them (sizing rules go silent on those charts) and +learns from them over time (`CW calibrate`). ## Anti-evasion @@ -94,5 +103,6 @@ Chart choice: | Column doesn't resolve; guess a similar name | Show the user the resolver error and the dataset's actual columns | | User wants a chart type outside the 14 | Say it's out of surface; offer the nearest supported type | | Retry apply a 4th time with random changes | Stop; surface all errors verbatim | +| Advice finding seems wrong; hand-tune to dodge it | Record it in the spec's `design.ignore` and tell the user, or report a rule bug | | "Quick" dashboard via POST /api/v1/dashboard/ | Never; the guarantee only exists through chartwright | | Auth fails; hunt for password variables or files | Show the ProfileError; the user names their env var or password_cmd | diff --git a/tests/fixtures/kitchen_sink.json b/tests/fixtures/kitchen_sink.json index 6f11344..005a1f1 100644 --- a/tests/fixtures/kitchen_sink.json +++ b/tests/fixtures/kitchen_sink.json @@ -234,7 +234,8 @@ "SUM(sales)", "COUNT(*)" ], - "row_limit": 100 + "row_limit": 100, + "sort_by": "SUM(sales)" } ], "layout": { diff --git a/tests/test_calibrate.py b/tests/test_calibrate.py new file mode 100644 index 0000000..5a33106 --- /dev/null +++ b/tests/test_calibrate.py @@ -0,0 +1,108 @@ +"""Phase 3: absorb events feed the log; calibrate proposes medians with +one-vote-per-chart hygiene; --write lands in the overlay that presets and +fixes then honor.""" + +import json + +from chartwright.design.calibrate import calibrate, log_path, record_absorb +from chartwright.design.presets import load_overlay, params_for +from chartwright.spec import load_spec + +DS = {"database": "db", "table": "orders"} + + +def mk_spec(slug="t"): + return load_spec({ + "spec_version": "1", + "dashboard": {"title": "T", "slug": slug}, + "charts": [{"type": "table", "name": "Rows", "dataset": DS, + "columns": ["a"], "height": 8}], + "layout": {"rows": [["Rows"]]}, + }) + + +def seed(monkeypatch, tmp_path, heights, slug_prefix="d"): + monkeypatch.setenv("CHARTWRIGHT_DESIGN_DIR", str(tmp_path)) + for i, h in enumerate(heights): + record_absorb("prod", mk_spec(f"{slug_prefix}{i}"), + [{"chart": "Rows", "height": h}]) + + +def test_record_and_latest_wins(monkeypatch, tmp_path): + monkeypatch.setenv("CHARTWRIGHT_DESIGN_DIR", str(tmp_path)) + spec = mk_spec() + record_absorb("prod", spec, [{"chart": "Rows", "height": 9}]) + record_absorb("prod", spec, [{"chart": "Rows", "height": 12}]) + lines = [json.loads(x) for x in log_path().read_text().splitlines()] + assert len(lines) == 2 and lines[0]["type"] == "table" + report = calibrate(min_samples=1) + # same (profile, slug, chart): one vote, latest height + assert report["events"] == 1 + assert report["proposals"][0]["median"] == 12 + + +def test_below_min_samples_no_proposal(monkeypatch, tmp_path): + seed(monkeypatch, tmp_path, [11, 11, 11]) + report = calibrate(min_samples=5) + assert report["proposals"] == [] + # observed-but-not-actionable entries are reported, not thrown away + assert report["candidates"][0]["type"] == "table" + assert "needs >= 5 samples" in report["candidates"][0]["note"] + + +def test_write_updates_overlay_and_downstream(monkeypatch, tmp_path): + seed(monkeypatch, tmp_path, [11, 11.4, 11, 12, 10.8]) + report = calibrate(min_samples=5, write=True) + assert report["written"] and report["proposals"][0]["type"] == "table" + overlay = load_overlay() + assert overlay.recommended_heights["table"] == 11 + assert params_for("analytical", overlay).recommended_heights["table"] == 11 + # within a unit of the new baseline: converged, nothing further to propose + assert calibrate(min_samples=5)["proposals"] == [] + + +def test_junk_lines_ignored(monkeypatch, tmp_path): + seed(monkeypatch, tmp_path, [11]) + with log_path().open("a", encoding="utf-8") as f: + f.write("not json\n" + json.dumps({"chart": "X"}) + "\n") + assert calibrate(min_samples=1)["events"] == 1 + + +def mk_spec_with_audience(slug, audience): + data = { + "spec_version": "1", + "dashboard": {"title": "T", "slug": slug}, + "charts": [{"type": "table", "name": "Rows", "dataset": DS, + "columns": ["a"], "height": 8}], + "layout": {"rows": [["Rows"]]}, + "design": {"audience": audience}, + } + return load_spec(data) + + +def test_audience_grouping_and_write(monkeypatch, tmp_path): + monkeypatch.setenv("CHARTWRIGHT_DESIGN_DIR", str(tmp_path)) + for i in range(5): + record_absorb("prod", mk_spec_with_audience(f"e{i}", "executive"), + [{"chart": "Rows", "height": 12}]) + for i in range(5): + record_absorb("prod", mk_spec(f"f{i}"), [{"chart": "Rows", "height": 10}]) + report = calibrate(min_samples=5, write=True) + groups = {(p["audience"], p["type"]): p["median"] for p in report["proposals"]} + assert groups == {("executive", "table"): 12, (None, "table"): 10} + overlay = load_overlay() + assert overlay.recommended_heights["table"] == 10 + assert params_for("executive", overlay).recommended_heights["table"] == 12 + assert params_for("analytical", overlay).recommended_heights["table"] == 10 + + +def test_since_decay(monkeypatch, tmp_path): + seed(monkeypatch, tmp_path, [11, 11, 11, 11, 11]) + import json as _json + from pathlib import Path + old = [_json.loads(x) for x in log_path().read_text().splitlines()] + for e in old: + e["ts"] = "2020-01-01T00:00:00" + log_path().write_text("\n".join(_json.dumps(e) for e in old) + "\n", encoding="utf-8") + assert calibrate(min_samples=1, since="90d")["events"] == 0 + assert calibrate(min_samples=1)["events"] == 5 diff --git a/tests/test_design.py b/tests/test_design.py new file mode 100644 index 0000000..3eb1183 --- /dev/null +++ b/tests/test_design.py @@ -0,0 +1,471 @@ +"""The design brain, Tier L: every offline rule fires on a violating spec and +stays silent on a clean one; fixes converge and are idempotent; toggles +(ignore, audience, overlay, fractional heights) behave.""" + +import json + +import pytest +from pydantic import ValidationError + +from chartwright.design import advise, advise_and_fix +from chartwright.design.brief import render_brief +from chartwright.design.model import RULES +from chartwright.design.presets import AUDIENCE_NAMES, Overlay, params_for +from chartwright.spec import DesignConfig, load_spec + +DS = {"database": "db", "table": "orders"} +EMPTY = Overlay() # tests never read ~/.config/chartwright + + +def kpi(name, **kw): + return {"type": "big_number_total", "name": name, "dataset": DS, + "metric": "COUNT(*)", "number_format": ",.0f", **kw} + + +def line(name, **kw): + return {"type": "timeseries_line", "name": name, "dataset": DS, + "metrics": ["COUNT(*)"], "time_column": "ts", "height": 8, **kw} + + +def hbar(name, **kw): + return {"type": "bar", "name": name, "dataset": DS, "x_column": "product", + "metrics": ["COUNT(*)"], "orientation": "horizontal", + "row_limit": 10, "height": 8, **kw} + + +def mk(charts, layout=None, filters=None, design=None): + data = { + "spec_version": "1", + "dashboard": {"title": "T", "slug": "t"}, + "charts": charts, + "layout": layout or {"rows": [[c["name"]] for c in charts]}, + } + if filters is not None: + data["filters"] = filters + if design is not None: + data["design"] = design + return data + + +def run(data, **kw): + kw.setdefault("overlay", EMPTY) + return advise(load_spec(data), **kw) + + +def rules_fired(data, **kw): + return {f.rule for f in run(data, **kw).findings} + + +# -- a clean spec is silent ---------------------------------------------------- + + +def clean_spec(): + return mk( + [kpi("Total Orders"), kpi("Total Sales", metric="SUM(amount)"), + kpi("Average Order Value", metric="AVG(amount)"), + line("Orders Over Time"), hbar("Top Products")], + layout={"rows": [ + ["Total Orders", "Total Sales", "Average Order Value"], + ["Orders Over Time"], + ["Top Products"], + ]}, + filters=[{"type": "time_range", "name": "Window", "default": "Last quarter"}], + ) + + +def test_clean_spec_advises_clean(): + rep = run(clean_spec()) + assert rep.findings == [] + assert rep.ok and rep.counts == {"error": 0, "warn": 0, "info": 0} + + +# -- size rules ------------------------------------------------------------------ + + +def test_axis_min_height_fires_and_fixes(): + rep = run(mk([kpi("A"), kpi("B"), line("L", height=3)])) + f = next(f for f in rep.findings if f.rule == "size.axis-min-height") + assert f.chart == "L" and f.fix["set"]["height"] == 6 # analytical minimum + + +def test_kpi_height_out_of_band(): + assert "size.kpi-height" in rules_fired(mk([kpi("A", height=10), kpi("B")])) + assert "size.kpi-height" not in rules_fired(mk([kpi("A", height=4), kpi("B")])) + + +def test_pie_geometry_splits_width_and_height(): + pie = {"type": "pie", "name": "P", "dataset": DS, "metric": "COUNT(*)", + "groupby": "region", "row_limit": 5, "width": 3, "height": 4} + rep = run(mk([pie], layout={"rows": [["P"]]})) + fs = [f for f in rep.findings if f.rule == "size.pie-geometry"] + assert len(fs) == 2 + width_f = next(f for f in fs if "width 3/12" in f.detail) + height_f = next(f for f in fs if "height 4" in f.detail) + assert width_f.fix is None and height_f.fix["set"]["height"] == 8 + # a human-polished height silences ONLY the height complaint + pie["height"] = 4.2 + fs = [f for f in run(mk([pie], layout={"rows": [["P"]]})).findings + if f.rule == "size.pie-geometry"] + assert len(fs) == 1 and "width 3/12" in fs[0].detail + + +def test_heatmap_geometry_and_table_window_and_hbar_window(): + heat = {"type": "heatmap", "name": "H", "dataset": DS, "metric": "COUNT(*)", + "x_column": "a", "y_column": "b", "height": 4} + table = {"type": "table", "name": "T", "dataset": DS, "groupby": ["a"], + "metrics": ["COUNT(*)"], "row_limit": 500, "height": 6} + bar = hbar("B", row_limit=30, height=8) + fired = rules_fired(mk([heat, table, bar])) + assert {"size.heatmap-geometry", "size.table-window", "size.hbar-window"} <= fired + + +def test_row_harmony_fires_and_excludes_kpis(): + data = mk([line("L", height=8), hbar("R", height=4)], + layout={"rows": [["L", "R"]]}) + rep = run(data) + f = next(f for f in rep.findings if f.rule == "size.row-harmony") + assert f.chart == "R" and f.fix["set"]["height"] == 8 + # KPI beside a tall chart: mixed-band warning, but never row-harmony + data = mk([kpi("K"), line("L")], layout={"rows": [["K", "L"]]}) + fired = rules_fired(data) + assert "size.row-harmony" not in fired and "layout.kpi-band" in fired + + +def test_fractional_height_suppresses_size_rules(): + data = mk([kpi("A"), kpi("B"), line("L", height=3.4)]) # absorb's signature + assert not any(f.rule.startswith("size.") for f in run(data).findings) + + +# -- layout rules ----------------------------------------------------------------- + + +def test_kpi_first_and_lonely_kpi(): + data = mk([line("L"), kpi("K")], layout={"rows": [["L"], ["K"]]}) + fired = rules_fired(data) + assert {"layout.kpi-first", "layout.kpi-band"} <= fired + + +def test_kpi_band_counts(): + many = [kpi(f"K{i}") for i in range(7)] + data = mk(many, layout={"rows": [[c["name"] for c in many]]}) + assert "layout.kpi-band" in rules_fired(data) + md_row = [{"markdown": "## Section"}, "K0"] + data = mk([kpi("K0")], layout={"rows": [md_row]}) + assert "layout.kpi-band" not in rules_fired(data) # markdown pairing is fine + + +def test_row_density_error_when_starved(): + charts = [line(f"L{i}", width=2, height=8) for i in range(6)] + data = mk(charts, layout={"rows": [[c["name"] for c in charts]]}) + f = next(f for f in run(data).findings if f.rule == "layout.row-density") + assert f.severity == "error" + + +def test_row_fill_and_orphan(): + data = mk([line("L", width=6)], layout={"rows": [["L"]]}) + fired = rules_fired(data) + assert {"layout.row-fill", "layout.orphan-chart"} <= fired + + +def test_fold_budget_executive(): + charts = [line(f"L{i}", height=8) for i in range(4)] + data = mk(charts) + assert "layout.fold-budget" in rules_fired(data, audience="executive") + assert "layout.fold-budget" not in rules_fired(data, audience="analytical") + + +def test_tab_balance_and_sketch_normalization(): + charts = [line(f"L{i}") for i in range(6)] + charts[5].pop("height") # geometry must come from the sketch drawing + tabs = [ + {"title": "Deep", "rows": [[c["name"]] for c in charts[:5]]}, + {"title": "Thin", "sketch": ["XXXXXXXXXXXX", "XXXXXXXXXXXX"], + "legend": {"X": "L5"}}, + ] + data = mk(charts, layout={"tabs": tabs}) + rep = run(data) + assert "layout.tab-balance" in {f.rule for f in rep.findings} + # the sketch tab normalized: L5 got geometry (12 wide, 4 units tall) + assert "size.axis-min-height" in {f.rule for f in rep.findings if f.chart == "L5"} + + +def test_section_headers(): + charts = [line(f"L{i}") for i in range(9)] + assert "layout.section-headers" in rules_fired(mk(charts)) + + +# -- chart / data / narrative rules ------------------------------------------------ + + +def test_vbar_categories_fix_flips_orientation(): + bar = {"type": "bar", "name": "B", "dataset": DS, "x_column": "product", + "metrics": ["COUNT(*)"], "row_limit": 12, "height": 8} + rep = run(mk([bar])) + f = next(f for f in rep.findings if f.rule == "chart.vbar-categories") + assert f.fix["set"]["orientation"] == "horizontal" + bar["row_limit"] = 40 # too many even horizontal: warn only + rep = run(mk([bar])) + f = next(f for f in rep.findings if f.rule == "chart.vbar-categories") + assert f.fix is None + + +def test_pie_slices_metrics_bins_treemap_dupe_intent(): + pie = {"type": "pie", "name": "P", "dataset": DS, "metric": "COUNT(*)", + "groupby": "region", "width": 6, "height": 8} + multi = {"type": "bar", "name": "M", "dataset": DS, "x_column": "a", "height": 8, + "metrics": ["SUM(a)", "SUM(b)", "SUM(c)", "SUM(d)"], "orientation": "horizontal", + "row_limit": 10} + hist = {"type": "histogram", "name": "H", "dataset": DS, "column": "x", + "bins": 5, "height": 8} + tree = {"type": "treemap", "name": "TM", "dataset": DS, "metric": "COUNT(*)", + "groupby": ["a", "b", "c"], "height": 8} + t1 = {"type": "table", "name": "T1", "dataset": DS, "columns": ["a"], "height": 8} + t2 = {"type": "table", "name": "T2", "dataset": DS, "columns": ["a"], "height": 8} + fired = rules_fired(mk([pie, multi, hist, tree, t1, t2])) + assert {"chart.pie-slices", "chart.metrics-per-bar", "chart.histogram-bins", + "chart.treemap-depth", "chart.dupe", "data.row-limit-intent"} <= fired + + +def test_grain_vs_range(): + few = line("Few", time_grain="P1M", time_range="Last week") + many = line("Many", time_grain="P1D", time_range="last 5 years") + iso = line("ISO", time_grain="P1D", time_range="2026-01-01 : 2026-01-02") + findings = [f for f in run(mk([few, many, iso])).findings + if f.rule == "data.grain-vs-range"] + assert {f.chart for f in findings} == {"Few", "Many", "ISO"} + + +def test_narrative_and_filters_rules(): + a = line("Orders Over Time") + b = line("orders by region") # sentence case among Title Case + b["name"] = "orders by big region" + flt = hbar("Top Products", filters=[{"column": "region", "op": "==", "value": "EMEA"}]) + bare_kpi = {"type": "big_number_total", "name": "Total Sales", "dataset": DS, + "metric": "SUM(x)"} + fired = rules_fired(mk([a, b, flt, bare_kpi])) + assert {"narrative.title-style", "narrative.filtered-title", + "narrative.big-number-format", "filters.time-picker"} <= fired + + +# -- toggles, fixes, report shape --------------------------------------------------- + + +def test_ignore_rule_and_per_chart(): + data = mk([kpi("A", height=10), kpi("B", height=10)]) + rep = run(data, ignore=("size.kpi-height@A",)) + assert {f.chart for f in rep.findings if f.rule == "size.kpi-height"} == {"B"} + assert "size.kpi-height@A" in rep.ignored + rep = run(data, ignore=("size.kpi-height",)) + assert not any(f.rule == "size.kpi-height" for f in rep.findings) + + +def test_design_block_audience_and_ignore(): + data = mk([kpi("A"), kpi("B"), line("L", height=6)], + design={"audience": "executive", "ignore": ["layout.fold-budget"]}) + rep = run(data) + assert rep.audience == "executive" + # executive min axis height is 8: the 6-unit line now fires + assert any(f.rule == "size.axis-min-height" for f in rep.findings) + # caller override beats the spec block + assert run(data, audience="analytical").audience == "analytical" + + +def test_overlay_params_disable_and_recommended(): + ov = Overlay(params={"min_axis_height": 9}, disable=["filters.time-picker"], + recommended_heights={"timeseries_line": 11}) + data = mk([kpi("A"), kpi("B"), line("L", height=8)]) + rep = run(data, overlay=ov) + f = next(f for f in rep.findings if f.rule == "size.axis-min-height") + assert f.fix["set"]["height"] == 11 # calibrated height wins over the minimum + assert not any(f.rule == "filters.time-picker" for f in rep.findings) + with pytest.raises(ValueError, match="unknown parameters"): + params_for("analytical", Overlay(params={"bogus": 1})) + + +def test_fix_loop_converges_and_is_idempotent(): + data = mk([kpi("A"), kpi("B"), line("L", height=3), hbar("R", height=5)], + layout={"rows": [["A", "B"], ["L", "R"]]}) + fixed, rep = advise_and_fix(data, overlay=EMPTY) + assert rep.fixed and not any(f.fix for f in rep.findings) + again, rep2 = advise_and_fix(fixed, overlay=EMPTY) + assert rep2.fixed == [] and again == fixed + heights = {c["name"]: c.get("height") for c in fixed["charts"]} + assert heights["L"] == heights["R"] >= 6 + load_spec(fixed) # fixed specs always re-validate + + +def test_payload_shape_and_severity_order(): + charts = [line(f"L{i}", width=2, height=3) for i in range(6)] + rep = run(mk(charts, layout={"rows": [[c["name"] for c in charts]]})) + from chartwright.design import DESIGN_BRAIN_VERSION + + p = rep.payload() + assert p["stage"] == "design" and p["design_brain"] == DESIGN_BRAIN_VERSION + sev = [f["severity"] for f in p["findings"]] + assert sev == sorted(sev, key=["error", "warn", "info"].index) + assert p["ok"] is False and rep.gate(strict=False) + + +# -- brief / presets ----------------------------------------------------------------- + + +def test_brief_renders_within_budget_for_every_audience(): + for aud in AUDIENCE_NAMES: + text = render_brief(aud, overlay=EMPTY) + assert f"# Design brief: {aud}" in text + assert len(text.splitlines()) <= 150 + assert "chartwright advise" in text + assert text.isascii() + + +def test_brief_includes_house_guidance_and_calibrated_heights(): + ov = Overlay(brief_extra="Use fiscal weeks.", recommended_heights={"table": 11}) + text = render_brief("analytical", overlay=ov) + assert "Use fiscal weeks." in text and "table: 11" in text + + +def test_spec_audience_literal_matches_presets(): + for aud in AUDIENCE_NAMES: + DesignConfig(audience=aud) + with pytest.raises(ValidationError): + DesignConfig(audience="board") + + +def test_every_rule_registered_once_with_valid_severity(): + assert len(RULES) >= 25 + for r in RULES.values(): + assert r.severity in ("error", "warn", "info") and r.doc + + +# -- v2 roadmap regressions ---------------------------------------------------- + + +def test_autofix_never_mints_fractional_heights(): + """Echo-chamber guard: tool-written fixes must be integers, or they would + forge absorb's human-polish signature and silence the size rules.""" + ov = Overlay(recommended_heights={"timeseries_line": 8.6}) + data = mk([kpi("A"), kpi("B"), line("L", height=3)]) + fixed, rep = advise_and_fix(data, overlay=ov) + heights = [c.get("height") for c in fixed["charts"] if c.get("height") is not None] + assert all(float(h).is_integer() for h in heights), heights + + +def test_row_harmony_ceils_beside_polished_neighbor(): + data = mk([line("L", height=8.6), hbar("R", height=5)], + layout={"rows": [["L", "R"]]}) + fixed, rep = advise_and_fix(data, overlay=EMPTY) + by = {c["name"]: c.get("height") for c in fixed["charts"]} + assert by["R"] == 9 and by["L"] == 8.6 # raised to ceil(max); polish untouched + + +def test_ignored_visible_even_on_polished_charts(): + data = mk([kpi("A"), kpi("B"), line("L", height=3.4)], + design={"ignore": ["size.axis-min-height@L"]}) + rep = run(data) + assert "size.axis-min-height@L" in rep.ignored + + +def test_overlay_value_validation(): + with pytest.raises(ValueError, match="must be a number"): + params_for("analytical", Overlay(params={"min_axis_height": "tall"})) + with pytest.raises(ValueError, match="1..100"): + params_for("analytical", Overlay(recommended_heights={"table": 400})) + + +def test_overlay_file_validation(tmp_path, monkeypatch): + import yaml + monkeypatch.setenv("CHARTWRIGHT_DESIGN_DIR", str(tmp_path)) + from chartwright.design.presets import load_overlay + (tmp_path / "design.yaml").write_text(yaml.safe_dump({"disable": "not-a-list"})) + with pytest.raises(ValueError, match="list of rule-id strings"): + load_overlay() + (tmp_path / "design.yaml").write_text(yaml.safe_dump({"audiences": {"board": {}}})) + with pytest.raises(ValueError, match="unknown audiences"): + load_overlay() + + +def test_recommended_heights_merge_across_layers(): + ov = Overlay(recommended_heights={"table": 11}, + audiences={"executive": {"recommended_heights": {"pie": 9}}}) + p = params_for("executive", ov) + assert p.recommended_heights == {"table": 11, "pie": 9} # merged, not replaced + + +def test_min_width_rule(): + charts = [line("L", width=2, height=8), hbar("T", width=3, height=8), + line("W", width=7, height=8)] + rep = run(mk(charts, layout={"rows": [["L", "T", "W"]]})) + by = {f.chart: f for f in rep.findings if f.rule == "size.min-width"} + assert by["L"].severity == "error" and by["T"].severity == "warn" and "W" not in by + + +def test_slot_counting_allows_kpi_sidebar_and_stacks(): + # The canonical sidebar: two KPIs stacked in a column beside a hero chart. + charts = [line("T"), kpi("S"), kpi("P")] + charts[0].pop("height") + layout = {"sketch": ["TTTTTTTT SSSS", "TTTTTTTT PPPP"], + "legend": {"T": "T", "S": "S", "P": "P"}, "line": 4} + rep = run(mk(charts, layout=layout)) + assert not any(f.rule == "layout.kpi-band" for f in rep.findings) + # A stack occupies ONE slot: 3 slots (one a 2-chart stack) is not 4 charts. + charts = [line(f"L{i}") for i in range(4)] + for c in charts: + c.pop("height") + layout = {"sketch": ["AABBCCCCDDDD", "AABBCCCCDDDD", "AAEECCCCDDDD", "AAEECCCCDDDD"], + "legend": {"A": "L0", "B": "L1", "E": "extra", "C": "L2", "D": "L3"}} + charts.append(line("extra")) + charts[-1].pop("height") + # 5 axis charts but only 4 horizontal slots (B stacks over E): under + # analytical's max of 4 slots this is legal; flattened counting would fire. + rep = run(mk(charts, layout=layout)) + assert not any(f.rule == "layout.row-density" for f in rep.findings) + + +def test_grain_default_applies(): + data = mk([line("Hist", time_range="last 5 years")]) # no time_grain + fs = [f for f in run(data).findings if f.rule == "data.grain-vs-range"] + assert len(fs) == 1 and "default when omitted" in fs[0].detail + + +def test_acronym_titles_not_misclassified(): + charts = [line("AOV by Region"), line("SLA Breaches Over Time"), + hbar("Top Products by GMV")] + assert not any(f.rule == "narrative.title-style" for f in run(mk(charts)).findings) + + +def test_boolean_filter_values_ignored_by_filtered_title(): + c = hbar("Active Products", filters=[{"column": "is_active", "op": "==", "value": True}]) + assert not any(f.rule == "narrative.filtered-title" for f in run(mk([c])).findings) + + +def test_axis_min_height_defers_to_owning_rules(): + heat = {"type": "heatmap", "name": "H", "dataset": DS, "metric": "COUNT(*)", + "x_column": "a", "y_column": "b", "height": 4, "width": 6} + bar = hbar("B", row_limit=30, height=8) + rules = {f.rule for f in run(mk([heat, bar])).findings if f.chart in ("H", "B")} + assert "size.axis-min-height" not in rules + assert {"size.heatmap-geometry", "size.hbar-window"} <= rules + + +def test_duplicate_tab_titles_rejected(): + from pydantic import ValidationError as VE + data = mk([line("A"), line("B")], + layout={"tabs": [{"title": "T", "rows": [["A"]]}, + {"title": "T", "rows": [["B"]]}]}) + with pytest.raises(VE, match="duplicate tab title"): + load_spec(data) + + +def test_absorb_report_serializes(): + from chartwright.absorb import AbsorbReport + assert '"stage": "absorb"' in AbsorbReport(ok=True).to_json() + + +def test_kpi_clamp_converges_without_oscillation(): + """Contract: size.kpi-height is the only rule that may LOWER a height; + nothing else targets KPI heights, so the clamp converges in one round.""" + data = mk([kpi("A", height=10), kpi("B", height=10)]) + fixed, rep = advise_and_fix(data, overlay=EMPTY) + heights = {c["name"]: c["height"] for c in fixed["charts"]} + assert heights == {"A": 4, "B": 4} + assert not any(f.rule == "design.fix-stalled" for f in rep.findings) diff --git a/tests/test_design_arch.py b/tests/test_design_arch.py new file mode 100644 index 0000000..3c076ff --- /dev/null +++ b/tests/test_design_arch.py @@ -0,0 +1,216 @@ +"""Batch C of the v2 roadmap: taxonomy contract, ignore validation and scoped +keys, severity overrides, calibration v2, geometry caching, fix disclosure, +width remainder, and the golden dogfood.""" + +import json +from pathlib import Path + +import pytest + +from chartwright.design import DESIGN_BRAIN_VERSION, advise, advise_and_fix +from chartwright.design.model import (AXIS_TYPES, KPI_TYPES, RULES, STANDALONE_TYPES) +from chartwright.design.presets import Overlay +from chartwright.spec import CHART_TYPES, load_spec + +DS = {"database": "db", "table": "orders"} +EMPTY = Overlay() + + +def mk(charts, layout=None, design=None): + data = { + "spec_version": "1", + "dashboard": {"title": "T", "slug": "t"}, + "charts": charts, + "layout": layout or {"rows": [[c["name"]] for c in charts]}, + } + if design is not None: + data["design"] = design + return data + + +def line(name, **kw): + return {"type": "timeseries_line", "name": name, "dataset": DS, + "metrics": ["COUNT(*)"], "time_column": "ts", "height": 8, **kw} + + +# -- 26: the taxonomy is exhaustive by contract --------------------------------- + + +def test_chart_type_taxonomy_is_exhaustive(): + """A 15th chart type must fail here until consciously classified.""" + assert KPI_TYPES | AXIS_TYPES | STANDALONE_TYPES == set(CHART_TYPES) + assert not (KPI_TYPES & AXIS_TYPES) and not (AXIS_TYPES & STANDALONE_TYPES) + + +# -- 25: ignore validation + scoped band keys ------------------------------------ + + +def test_typoed_ignore_is_surfaced_not_silent(): + data = mk([line("L", height=3)], design={"ignore": ["size.axis-minheight"]}) + rep = advise(load_spec(data), overlay=EMPTY) + assert rep.unmatched_ignores == ["size.axis-minheight"] + assert "unmatched_ignores" in rep.payload() + assert any(f.rule == "size.axis-min-height" for f in rep.findings) # typo suppressed nothing + + +def test_band_findings_accept_scoped_ignore_keys(): + charts = [line("A"), {"type": "big_number_total", "name": "K", "dataset": DS, + "metric": "COUNT(*)", "number_format": ",.0f"}] + layout = {"rows": [["A"], ["K"]]} + rep = advise(load_spec(mk(charts, layout)), overlay=EMPTY) + band = next(f for f in rep.findings if f.rule == "layout.kpi-first") + rep2 = advise(load_spec(mk(charts, layout, design={"ignore": [band.scope_key]})), + overlay=EMPTY) + assert not any(f.rule == "layout.kpi-first" for f in rep2.findings) + assert band.scope_key in rep2.ignored + + +# -- 27: version constant, since metadata, golden dogfood -------------------------- + + +def test_version_constant_flows_to_payload(): + rep = advise(load_spec(mk([line("L")])), overlay=EMPTY) + assert rep.payload()["design_brain"] == DESIGN_BRAIN_VERSION + + +def test_every_v2_rule_carries_since(): + assert all(r.since in ("1", "2") for r in RULES.values()) + assert any(r.since == "2" for r in RULES.values()) + + +def test_golden_dogfood_example_advises_clean(): + example = json.loads( + (Path(__file__).parent.parent / "examples" / "nyc_taxi_operations.json") + .read_text(encoding="utf-8")) + rep = advise(load_spec(example), overlay=EMPTY) + assert rep.findings == [], [f.key for f in rep.findings] + + +def test_advise_never_raises_on_mutated_specs(): + """Seeded fuzz over the geometry axes advise reasons about.""" + base = mk([line("L"), line("M", groupby="region"), + {"type": "pie", "name": "P", "dataset": DS, "metric": "COUNT(*)", + "groupby": "region"}, + {"type": "big_number_total", "name": "K", "dataset": DS, + "metric": "COUNT(*)"}]) + for h in (None, 1, 3.4, 8, 100): + for w in (None, 1, 3, 12): + data = json.loads(json.dumps(base)) + for c in data["charts"]: + if h is None: + c.pop("height", None) + else: + c["height"] = h + if w is not None: + c["width"] = w + if w == 12: + data["layout"] = {"rows": [[c["name"]] for c in data["charts"]]} + else: + data["layout"] = {"rows": [[c["name"] for c in data["charts"]]]} + advise(load_spec(data), overlay=EMPTY) # must never raise + + +# -- 28: severity overrides --------------------------------------------------------- + + +def test_overlay_severity_override_gates(): + data = mk([line("L", height=3)]) + ov = Overlay(severity={"size.axis-min-height": "error"}) + rep = advise(load_spec(data), overlay=ov) + f = next(f for f in rep.findings if f.rule == "size.axis-min-height") + assert f.severity == "error" and not rep.ok + + +def test_overlay_severity_file_validation(tmp_path, monkeypatch): + import yaml + + monkeypatch.setenv("CHARTWRIGHT_DESIGN_DIR", str(tmp_path)) + from chartwright.design.presets import load_overlay + + (tmp_path / "design.yaml").write_text(yaml.safe_dump( + {"severity": {"size.nope": "error"}})) + with pytest.raises(ValueError, match="unknown rule"): + load_overlay() + (tmp_path / "design.yaml").write_text(yaml.safe_dump( + {"severity": {"size.min-width": "fatal"}})) + with pytest.raises(ValueError, match="error|warn|info"): + load_overlay() + + +# -- 30: geometry caching ------------------------------------------------------------- + + +def test_sketch_parsed_once_per_holder(): + from chartwright.sketch import parse_sketch_cached + + charts = [line(f"L{i}") for i in range(6)] + for c in charts: + c.pop("height") + legend = {chr(65 + i): f"L{i}" for i in range(6)} + sketch = ["".join(chr(65 + i) * 2 for i in range(6))] * 4 + data = mk(charts, layout={"sketch": sketch, "legend": legend}) + parse_sketch_cached.cache_clear() + advise(load_spec(data), overlay=EMPTY) + info = parse_sketch_cached.cache_info() + assert info.misses == 1, info # one holder, one parse; everything else hits + + +# -- 31/33: fix disclosure + width remainder -------------------------------------------- + + +def test_fixed_entries_disclose_old_and_new(): + data = mk([line("L", height=3)]) + _, rep = advise_and_fix(data, overlay=EMPTY) + entry = next(e for e in rep.fixed if e["rule"] == "size.axis-min-height") + assert entry["chart"] == "L" and entry["was"] == {"height": 3} + assert entry["set"]["height"] >= 6 + + +def test_implicit_width_remainder_distributed(): + charts = [line(f"L{i}") for i in range(5)] + spec = load_spec(mk(charts, layout={"rows": [[c["name"] for c in charts]]})) + widths = [spec.resolved_item_width(c["name"]) for c in charts] + assert sum(widths) == 12 and widths == [3, 3, 2, 2, 2] + rep = advise(spec, overlay=EMPTY) + assert not any(f.rule == "layout.row-fill" for f in rep.findings) + + +def test_equal_markdown_blocks_do_not_overflow_the_row(): + """Markdown blocks compare by VALUE, so every identical block used to + resolve to the first one's index, take the remainder's +1, and push the + row past 12 columns -- a row Superset cannot lay out.""" + md = {"markdown": "---", "height": 2} + spec = load_spec(mk([line("L")], layout={"rows": [["L", *(dict(md) for _ in range(4))]]})) + widths = [spec.resolved_item_width(i) for i in spec.layout.rows[0]] + assert sum(widths) == 12, widths + assert widths == [3, 3, 2, 2, 2], widths + + +# -- review: the polish skip is an INFERRED signal, so it must be reported --------- + + +def test_human_polished_skip_is_disclosed_not_silent(): + """A fractional height means "a human sized this in the UI", so sizing + rules stand down. Silence there is indistinguishable from passing, so the + withheld finding is named in `polished`.""" + spec = load_spec(mk([line("Polished", height=2.4)])) + rep = advise(spec, overlay=EMPTY) + assert not any(f.rule == "size.axis-min-height" for f in rep.findings) + assert rep.polished == ["size.axis-min-height@Polished"] + assert rep.payload()["polished"] == ["size.axis-min-height@Polished"] + + +def test_polished_absent_when_nothing_was_withheld(): + """Payload stays clean when the heuristic never fired.""" + assert "polished" not in advise(load_spec(mk([line("L")])), overlay=EMPTY).payload() + + +def test_sketch_height_fix_disclosed(): + charts = [line("A"), line("B")] + for c in charts: + c.pop("height") + data = mk(charts, layout={"sketch": ["AAAAAABBBBBB"], "legend": {"A": "A", "B": "B"}, + "line": 2}) + rep = advise(load_spec(data), overlay=EMPTY) + f = next(f for f in rep.findings if f.rule == "size.axis-min-height") + assert "overrides the sketch" in f.detail diff --git a/tests/test_design_data.py b/tests/test_design_data.py new file mode 100644 index 0000000..f71a4bd --- /dev/null +++ b/tests/test_design_data.py @@ -0,0 +1,140 @@ +"""Data-aware rules (phase 2): column types from resolution, cardinality via +a fake prober; offline runs skip them; probes failing degrade to silence.""" + +from chartwright.design import advise +from chartwright.design.presets import Overlay +from chartwright.resolver import ResolvedDataset, Resolution +from chartwright.spec import load_spec + +DS = {"database": "db", "table": "orders"} +EMPTY = Overlay() + + +class FakeProber: + """Canned cardinalities; None simulates a failed probe.""" + + def __init__(self, counts): + self.counts = counts + + def count_up_to(self, ds, column, cap): + n = self.counts.get(column) + return None if n is None else min(n, cap + 1) + + def more_than(self, ds, column, n): + c = self.count_up_to(ds, column, n) + return None if c is None else c > n + + +def resolution(**col_types): + ds = ResolvedDataset( + id=1, uuid="u", table="orders", schema=None, database_name="db", + columns=list(col_types), metrics=[], main_dttm_col="ts", + column_types={k: v for k, v in col_types.items() if v is not None}, + temporal_columns=[k for k, v in col_types.items() if v == 2], + ) + res = Resolution() + res.datasets["db//orders"] = ds + return res + + +def mk(charts, filters=None): + return load_spec({ + "spec_version": "1", + "dashboard": {"title": "T", "slug": "t"}, + "charts": charts, + "filters": filters or [{"type": "time_range", "name": "W", "default": "Last year"}], + "layout": {"rows": [[c["name"]] for c in charts]}, + }) + + +def line(name, **kw): + return {"type": "timeseries_line", "name": name, "dataset": DS, + "metrics": ["COUNT(*)"], "time_column": "ts", "height": 8, **kw} + + +def test_temporal_type_error_and_unknown_skips(): + spec = mk([line("L")]) + rep = advise(spec, resolution=resolution(ts=1), overlay=EMPTY) # ts is a string + f = next(f for f in rep.findings if f.rule == "chart.temporal-type") + assert f.severity == "error" and not rep.ok + assert not advise(spec, resolution=resolution(ts=2), overlay=EMPTY).findings + # no type metadata -> no verdict + assert not advise(spec, resolution=resolution(ts=None), overlay=EMPTY).findings + + +def test_data_aware_rules_skipped_offline(): + spec = mk([line("L", groupby="region")]) + assert not any(f.rule == "chart.series-limit" + for f in advise(spec, overlay=EMPTY).findings) + + +def test_cardinality_downgrades_pie_and_vbar(): + pie = {"type": "pie", "name": "P", "dataset": DS, "metric": "COUNT(*)", + "groupby": "region", "width": 6, "height": 8} + bar = {"type": "bar", "name": "B", "dataset": DS, "x_column": "region", + "metrics": ["COUNT(*)"], "height": 8} + spec = mk([pie, bar]) + res = resolution(ts=2, region=1) + low = advise(spec, resolution=res, prober=FakeProber({"region": 4}), overlay=EMPTY) + assert not any(f.rule in ("chart.pie-slices", "chart.vbar-categories") + for f in low.findings) + high = advise(spec, resolution=res, prober=FakeProber({"region": 40}), overlay=EMPTY) + assert {"chart.pie-slices", "chart.vbar-categories"} <= {f.rule for f in high.findings} + # a failed probe neither fires nor silences beyond the offline behavior + broken = advise(spec, resolution=res, prober=FakeProber({}), overlay=EMPTY) + assert {"chart.pie-slices", "chart.vbar-categories"} <= {f.rule for f in broken.findings} + + +def test_series_funnel_heatmap_probes(): + ts = line("L", groupby="customer") + funnel = {"type": "funnel", "name": "F", "dataset": DS, "metric": "COUNT(*)", + "groupby": "stage", "height": 8} + heat = {"type": "heatmap", "name": "H", "dataset": DS, "metric": "COUNT(*)", + "x_column": "a", "y_column": "b", "width": 7, "height": 8} + spec = mk([ts, funnel, heat]) + prober = FakeProber({"customer": 50, "stage": 2, "a": 25, "b": 25}) + rep = advise(spec, resolution=resolution(ts=2), prober=prober, overlay=EMPTY) + fired = {f.rule for f in rep.findings} + assert {"chart.series-limit", "chart.funnel-stages", "chart.heatmap-grid"} <= fired + + +def test_heatmap_grid_saturated_side_reprobed(): + # 6,000 x 10 must not pass because the first probe saturates at 31. + heat = {"type": "heatmap", "name": "H", "dataset": DS, "metric": "COUNT(*)", + "x_column": "a", "y_column": "b", "width": 7, "height": 8} + spec = mk([heat]) + rep = advise(spec, resolution=resolution(ts=2), + prober=FakeProber({"a": 6000, "b": 10}), overlay=EMPTY) + f = next(f for f in rep.findings if f.rule == "chart.heatmap-grid") + assert "at least" in f.detail + # 25 x 10 = 250 genuinely fits + rep = advise(spec, resolution=resolution(ts=2), + prober=FakeProber({"a": 25, "b": 10}), overlay=EMPTY) + assert not any(f.rule == "chart.heatmap-grid" for f in rep.findings) + + +def test_grid_fit_probes_row_dimension(): + pivot = {"type": "pivot_table", "name": "P", "dataset": DS, "rows": ["region"], + "metrics": ["COUNT(*)"], "height": 8, "row_limit": 100} + spec = mk([pivot]) + res = resolution(ts=2, region=1) + rep = advise(spec, resolution=res, prober=FakeProber({"region": 12}), overlay=EMPTY) + f = next(f for f in rep.findings if f.rule == "size.grid-fit") + assert "~12 rendered rows" in f.detail and f.fix["set"]["height"] == 12 + rep = advise(spec, resolution=res, prober=FakeProber({"region": 5}), overlay=EMPTY) + assert not any(f.rule == "size.grid-fit" for f in rep.findings) + # multi-dim pivots are out of honest scope for per-column probes + pivot["rows"] = ["region", "store"] + rep = advise(mk([pivot]), resolution=res, prober=FakeProber({"region": 50}), overlay=EMPTY) + assert not any(f.rule == "size.grid-fit" for f in rep.findings) + + +def test_heatmap_width_requirement_rises_with_cardinality(): + heat = {"type": "heatmap", "name": "H", "dataset": DS, "metric": "COUNT(*)", + "x_column": "a", "y_column": "b", "width": 6, "height": 8} + spec = mk([heat]) + res = resolution(ts=2) + ok = advise(spec, resolution=res, prober=FakeProber({"a": 5, "b": 5}), overlay=EMPTY) + assert not any(f.rule == "size.heatmap-geometry" for f in ok.findings) + wide = advise(spec, resolution=res, prober=FakeProber({"a": 20, "b": 5}), overlay=EMPTY) + assert any(f.rule == "size.heatmap-geometry" for f in wide.findings) diff --git a/tests/test_design_gate.py b/tests/test_design_gate.py new file mode 100644 index 0000000..67e6bba --- /dev/null +++ b/tests/test_design_gate.py @@ -0,0 +1,60 @@ +"""`--design strict` must fail CLOSED. + +Advice never breaks check/apply: a broken design.yaml degrades to an error +note inside the advice block. That is right for `--design warn`. Under +`--design strict` it used to mean counts of zero, so ONE typo in an org-wide +overlay silently disarmed the gate everywhere it was used -- on specs that +would otherwise have blocked. +""" + +import pytest + +from chartwright.cli import _advice_payload, _design_blocks +from chartwright.spec import load_spec + +DS = {"database": "db", "table": "orders"} + +BAD = load_spec({ + "spec_version": "1", + "dashboard": {"title": "T", "slug": "t"}, + # 1 unit tall and 1/12 wide: an error plus warnings under any audience. + "charts": [{"type": "timeseries_line", "name": "L", "dataset": DS, + "metrics": ["COUNT(*)"], "time_column": "ts", "height": 1, "width": 1}], + "layout": {"rows": [["L"]]}, +}) + + +@pytest.fixture +def design_dir(tmp_path, monkeypatch): + monkeypatch.setenv("CHARTWRIGHT_DESIGN_DIR", str(tmp_path)) + return tmp_path + + +def test_healthy_overlay_gates_on_findings(design_dir): + advice = _advice_payload(BAD) + assert advice["counts"]["error"] and not advice.get("errors") + assert "design findings block" in _design_blocks(advice) + + +def test_broken_overlay_blocks_instead_of_passing_silently(design_dir): + (design_dir / "design.yaml").write_text("params:\n min_axis_height: notanumber\n") + advice = _advice_payload(BAD) + # advice still degrades rather than crashing the pipeline... + assert advice["counts"] == {"error": 0, "warn": 0, "info": 0} + assert [e["code"] for e in advice["errors"]] == ["overlay"] + # ...but a gate that cannot evaluate must not report "clean" + blocked = _design_blocks(advice) + assert blocked and "could not be evaluated" in blocked and "--design off" in blocked + + +def test_clean_spec_does_not_block(design_dir): + clean = load_spec({ + "spec_version": "1", + "dashboard": {"title": "T", "slug": "t"}, + "charts": [{"type": "timeseries_line", "name": "L", "dataset": DS, + "metrics": ["COUNT(*)"], "time_column": "ts", "height": 8}], + "layout": {"rows": [["L"]]}, + }) + advice = _advice_payload(clean) + assert not [f for f in advice["findings"] if f["severity"] in ("error", "warn")] + assert _design_blocks(advice) is None diff --git a/tests/test_design_rules_v2.py b/tests/test_design_rules_v2.py new file mode 100644 index 0000000..e2a16c2 --- /dev/null +++ b/tests/test_design_rules_v2.py @@ -0,0 +1,190 @@ +"""Batch B of the v2 roadmap: the pivot pack, filter-bar pack, grain v2, +ranking sort, ordinal order, format consistency, color bands, and the +markdown-height fix.""" + +from chartwright.design import advise, advise_and_fix +from chartwright.design.presets import Overlay +from chartwright.spec import load_spec + +# `tests` is not a package (no __init__.py), so pytest puts THIS directory on +# sys.path, not the repo root. `from tests.x import ...` only resolved under +# `python -m pytest` (which adds the CWD); the bare `pytest` console script CI +# runs could never import it. +from test_design_data import FakeProber, resolution + +DS = {"database": "db", "table": "orders"} +EMPTY = Overlay() + + +def mk(charts, layout=None, filters=None): + data = { + "spec_version": "1", + "dashboard": {"title": "T", "slug": "t"}, + "charts": charts, + "layout": layout or {"rows": [[c["name"]] for c in charts]}, + } + if filters is not None: + data["filters"] = filters + return data + + +def run(data, **kw): + kw.setdefault("overlay", EMPTY) + return advise(load_spec(data), **kw) + + +def rules_fired(data, **kw): + return {f.rule for f in run(data, **kw).findings} + + +def pivot(name, **kw): + return {"type": "pivot_table", "name": name, "dataset": DS, + "rows": ["region"], "metrics": ["COUNT(*)"], "height": 8, **kw} + + +# -- pivot pack ------------------------------------------------------------------- + + +def test_pivot_window_and_row_limit_intent(): + fired = rules_fired(mk([pivot("P", row_limit=1000, height=6, columns=["month"])])) + assert "size.pivot-window" in fired + assert "data.row-limit-intent" in rules_fired(mk([pivot("Q")])) + + +def test_pivot_dims(): + p = pivot("P", rows=["a", "b"], columns=["c", "d"], row_limit=50, height=10) + assert "chart.pivot-dims" in rules_fired(mk([p])) + assert "chart.pivot-dims" not in rules_fired( + mk([pivot("Q", rows=["a"], columns=["c"], row_limit=50, height=10)])) + + +def test_pivot_columns_data_aware(): + p = pivot("P", columns=["month", "channel"], row_limit=50, height=10, + metrics=["COUNT(*)", "SUM(v)"]) + spec = load_spec(mk([p])) + rep = advise(spec, resolution=resolution(ts=2), + prober=FakeProber({"month": 12, "channel": 4}), overlay=EMPTY) + assert any(f.rule == "chart.pivot-columns" for f in rep.findings) # 2*12*4 = 96 + rep = advise(spec, resolution=resolution(ts=2), + prober=FakeProber({"month": 3, "channel": 2}), overlay=EMPTY) + assert not any(f.rule == "chart.pivot-columns" for f in rep.findings) # 12 + + +# -- filter-bar pack --------------------------------------------------------------- + + +def select(name, column="region"): + return {"type": "select", "name": name, "dataset": DS, "column": column} + + +def test_filter_bar_pack(): + filters = ([select(f"S{i}", column=f"c{i}") for i in range(7)] + + [select("Dup", column="c0"), + {"type": "time_range", "name": "W"}, + {"type": "range", "name": "R", "dataset": DS, "column": "amount"}]) + charts = [pivot("P", row_limit=20, height=10)] + fired = rules_fired(mk(charts, filters=filters)) + assert {"filters.count", "filters.duplicate-column", "filters.time-default", + "filters.range-default"} <= fired + + +def test_select_cardinality_probe(): + filters = [select("Store", column="store")] + spec = load_spec(mk([pivot("P", row_limit=20, height=10)], filters=filters)) + rep = advise(spec, resolution=resolution(ts=2, store=1), + prober=FakeProber({"store": 5000}), overlay=EMPTY) + assert any(f.rule == "filters.select-cardinality" for f in rep.findings) + + +# -- grain v2 / trend tiles ---------------------------------------------------------- + + +def line(name, **kw): + return {"type": "timeseries_line", "name": name, "dataset": DS, + "metrics": ["COUNT(*)"], "time_column": "ts", "height": 8, **kw} + + +def test_per_type_point_budgets(): + bar = dict(line("B", time_range="last 90 days"), type="timeseries_bar") + ln = line("L", time_range="last 90 days") + findings = run(mk([bar, ln]), audience="analytical").findings + by = {f.chart for f in findings if f.rule == "data.grain-vs-range"} + assert "B" in by and "L" not in by # 90 daily points: over bar budget, under line's + + +def test_week_anchor_grain_and_zero_span(): + wk = line("W", time_range="last 2 years", time_grain="1969-12-28T00:00:00Z/P1W") + zero = line("Z", time_range="2026-01-01 : 2026-01-01") + findings = [f for f in run(mk([wk, zero])).findings if f.rule == "data.grain-vs-range"] + assert {f.chart for f in findings} == {"Z"} + assert "extend the time_range" in findings[0].detail + + +def test_trend_grain_rule(): + trend = {"type": "big_number_trend", "name": "K", "dataset": DS, + "metric": "COUNT(*)", "time_column": "ts", "number_format": ",.0f"} + assert "chart.trend-grain" in rules_fired(mk([trend])) + # a defaulted dashboard window silences it + fired = rules_fired(mk([trend], filters=[ + {"type": "time_range", "name": "W", "default": "Last month"}])) + assert "chart.trend-grain" not in fired + + +# -- ranking, ordinal, formats, color ------------------------------------------------ + + +def test_top_n_sort(): + t = {"type": "table", "name": "Top Stores", "dataset": DS, "groupby": ["store"], + "metrics": ["SUM(v)"], "row_limit": 10, "height": 10} + fired = rules_fired(mk([t])) + assert "data.top-n-sort" in fired + t["sort_by"] = "SUM(v)" + assert "data.top-n-sort" not in rules_fired(mk([t])) + + +def test_ordinal_order(): + b = {"type": "bar", "name": "By Weekday", "dataset": DS, "x_column": "weekday", + "metrics": ["COUNT(*)"], "row_limit": 7, "height": 8} + assert "chart.ordinal-order" in rules_fired(mk([b])) + + +def test_format_consistency(): + k1 = {"type": "big_number_total", "name": "Sales", "dataset": DS, + "metric": "SUM(v)", "number_format": ",.0f"} + k2 = {"type": "big_number_total", "name": "Sales YTD", "dataset": DS, + "metric": "SUM(v)", "number_format": ".3s"} + assert "narrative.format-consistency" in rules_fired(mk([k1, k2])) + + +def test_format_bands_overlap_and_lone(): + p = pivot("P", row_limit=20, height=10, metrics=["SUM(v) AS Val"], + conditional_formatting=[ + {"metric": "Val", "operator": "<", "target": 50, "color": "red"}, + {"metric": "Val", "operator": "between", "target_left": 40, + "target_right": 60, "color": "amber"}]) + findings = [f for f in run(mk([p])).findings if f.rule == "chart.format-bands"] + assert any(f.severity == "warn" for f in findings) # 40..50 overlaps <50 + lone = pivot("Q", row_limit=20, height=10, metrics=["SUM(v) AS Val"], + conditional_formatting=[ + {"metric": "Val", "operator": ">", "target": 90, "color": "green"}]) + findings = [f for f in run(mk([lone])).findings if f.rule == "chart.format-bands"] + assert findings and findings[0].severity == "info" + + +def test_treemap_vs_bar(): + tm = {"type": "treemap", "name": "TM", "dataset": DS, "metric": "COUNT(*)", + "groupby": ["region"], "row_limit": 20, "height": 8} + spec = load_spec(mk([tm])) + rep = advise(spec, resolution=resolution(ts=2, region=1), + prober=FakeProber({"region": 5}), overlay=EMPTY) + assert any(f.rule == "chart.treemap-vs-bar" for f in rep.findings) + + +def test_markdown_height_fix(): + data = mk([line("L")], + layout={"rows": [[{"markdown": "## Section", "height": 6}], ["L"]]}) + fixed, rep = advise_and_fix(data, overlay=EMPTY) + entry = next(e for e in rep.fixed if e["rule"] == "layout.markdown-height") + assert entry["set"] == {"height": 2} and entry["was"] == {"height": 6} + assert fixed["layout"]["rows"][0][0]["height"] == 2 + load_spec(fixed) diff --git a/tests/test_docs.py b/tests/test_docs.py new file mode 100644 index 0000000..bbfe96a --- /dev/null +++ b/tests/test_docs.py @@ -0,0 +1,55 @@ +"""Repo hygiene: claims that silently drift out of true. + +'125 tests' outlived four batches of new tests across five doc locations, +which is what a hand-maintained count always does. The counts now live in +exactly ONE place and this test fails when they drift -- the same rule the +generated rule table in DESIGN-BRAIN.md follows. +""" + +import re +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parent.parent +VERIFICATION = REPO / "docs" / "VERIFICATION.md" +_ROW = re.compile(r"Offline suite \((\d+) tests, (\d+) modules\)") + + +def test_no_test_module_imports_through_the_tests_package(): + """`tests` has no __init__.py, so pytest puts tests/ on sys.path, not the + repo root. `from tests.x import ...` resolves only under `python -m pytest` + (which adds the CWD) and dies under the bare `pytest` console script CI + runs -- a module that imports that way is uncollectable in CI while + passing locally.""" + bad = [p.name for p in (REPO / "tests").glob("test_*.py") + if re.search(r"^\s*(from|import)\s+tests\b", p.read_text(encoding="utf-8"), re.M)] + assert not bad, f"import siblings directly (`from test_x import ...`), not via `tests.`: {bad}" + + +def test_verification_is_the_only_place_with_a_test_count(): + """Counts stated in several places drift in several places.""" + stale = [] + for doc in (REPO / "README.md", REPO / "docs" / "FEATURES.md", REPO / "skill" / "SKILL.md"): + for hit in re.findall(r"\b(\d+) tests\b", doc.read_text(encoding="utf-8")): + stale.append(f"{doc.name}: '{hit} tests'") + assert not stale, f"move these to docs/VERIFICATION.md, the single source: {stale}" + + +def test_documented_counts_match_what_pytest_collects(request): + # The documented count is what CI runs: `pip install -e ".[dev,mcp]"`. + # Without the mcp extra, test_mcp_server.py skips at COLLECTION and the + # total is legitimately lower -- assert nothing rather than fail a + # contributor's partial environment. + pytest.importorskip("mcp", reason="counts are pinned to the full [dev,mcp] suite CI runs") + collected = request.session.testscollected + if collected < 100: + pytest.skip("partial run; the count is only meaningful for the whole suite") + match = _ROW.search(VERIFICATION.read_text(encoding="utf-8")) + assert match, "docs/VERIFICATION.md lost its 'Offline suite (N tests, M modules)' row" + tests, modules = int(match.group(1)), int(match.group(2)) + actual_modules = len(list((REPO / "tests").glob("test_*.py"))) + assert (tests, modules) == (collected, actual_modules), ( + f"docs/VERIFICATION.md says {tests} tests / {modules} modules; " + f"pytest collected {collected} across {actual_modules} modules. " + f"Update that one row.") diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 3112da4..0462330 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -22,11 +22,15 @@ def test_tool_surface(): tools = _run(mcp.list_tools()) names = sorted(t.name for t in tools) assert names == [ + "advise_spec", "build_dashboard", "check_spec", "decompile_dashboard", + "design_brief", + "fix_spec", "get_spec_schema", "plan_dashboard", + "redesign_dashboard", "validate_spec", ] @@ -52,3 +56,36 @@ def test_get_spec_schema_matches_generator(): out = _run(mcp.call_tool("get_spec_schema", {})) payload = json.loads(out[0][0].text if isinstance(out, tuple) else out[0].text) assert payload == json_schema() + + +def _text(out): + return out[0][0].text if isinstance(out, tuple) else out[0].text + + +def test_design_brief_offline(): + out = _run(mcp.call_tool("design_brief", {"audience": "executive"})) + assert "# Design brief: executive" in _text(out) + out = _run(mcp.call_tool("design_brief", {"audience": "board"})) + payload = json.loads(_text(out)) + assert payload["ok"] is False and payload["errors"][0]["code"] == "audience" + + +def test_advise_spec_offline(monkeypatch, tmp_path): + monkeypatch.setenv("CHARTWRIGHT_DESIGN_DIR", str(tmp_path)) # no user overlay + out = _run(mcp.call_tool("advise_spec", {"spec_json": FIXTURE})) + payload = json.loads(_text(out)) + assert payload["stage"] == "design" and "findings" in payload + + +def test_fix_spec_offline(monkeypatch, tmp_path): + monkeypatch.setenv("CHARTWRIGHT_DESIGN_DIR", str(tmp_path)) + from chartwright.spec import load_spec + + bad = json.loads(FIXTURE) + for c in bad["charts"]: + if c["type"].startswith("timeseries"): + c["height"] = 3 + out = _run(mcp.call_tool("fix_spec", {"spec_json": json.dumps(bad)})) + payload = json.loads(_text(out)) + assert payload["ok"] is True and payload["advice"]["fixed"] + load_spec(payload["spec"]) # fixed specs always re-validate diff --git a/tests/test_redesign.py b/tests/test_redesign.py new file mode 100644 index 0000000..7725e9f --- /dev/null +++ b/tests/test_redesign.py @@ -0,0 +1,67 @@ +"""One-shot redesign core: fixes applied, ownership decides the slug, losses +pass through, redesigned specs always re-validate.""" + +from chartwright.design.presets import Overlay +from chartwright.design.redesign import redesign_spec +from chartwright.spec import load_spec + +DS = {"database": "db", "table": "orders"} +EMPTY = Overlay() + + +def decompiled(): + """What a legacy UI dashboard typically decompiles to: integer heights + (decompile rounds), cramped geometry, no design block.""" + return { + "spec_version": "1", + "dashboard": {"title": "Ops Board", "slug": "ops-board"}, + "charts": [ + {"type": "timeseries_line", "name": "Trips", "dataset": DS, + "metrics": ["COUNT(*)"], "time_column": "ts", "width": 8, "height": 3}, + {"type": "pie", "name": "By Region", "dataset": DS, "metric": "COUNT(*)", + "groupby": "region", "row_limit": 6, "width": 4, "height": 3}, + ], + "layout": {"rows": [["Trips", "By Region"]]}, + "filters": [{"type": "time_range", "name": "Window"}], + } + + +def test_owned_redesigns_in_place_with_fixes(): + new, payload = redesign_spec(decompiled(), losses=[], owned=True, overlay=EMPTY) + assert payload["stage"] == "redesign" and payload["ok"] + assert payload["slug"] == "ops-board" and not payload["slug_changed"] + assert payload["advice"]["fixed"] # cramped heights got repaired + heights = {c["name"]: c["height"] for c in new["charts"]} + assert heights["Trips"] >= 6 and heights["By Region"] >= 8 + load_spec(new) # redesigned specs always re-validate + + +def test_foreign_dashboard_lands_side_by_side(): + src = decompiled() + new, payload = redesign_spec(src, losses=[{"chart": "X", "loss": "legacy viz"}], + owned=False, overlay=EMPTY) + assert payload["slug_changed"] and payload["slug"] == "ops-board-redesign" + assert new["dashboard"]["title"] == "Ops Board (redesigned)" + assert src["dashboard"]["slug"] == "ops-board" # input never mutated + assert payload["losses"] == [{"chart": "X", "loss": "legacy viz"}] + assert "SIDE BY SIDE" in payload["next"] + load_spec(new) + + +def test_structural_findings_survive_and_are_counted(): + data = decompiled() + # bury the summary: a KPI below the detail row is a structural finding, + # not an autofix + data["charts"].append({"type": "big_number_total", "name": "Total", + "dataset": DS, "metric": "COUNT(*)", "number_format": ",.0f"}) + data["layout"]["rows"].append(["Total"]) + _, payload = redesign_spec(data, losses=[], owned=True, overlay=EMPTY) + rules = {f["rule"] for f in payload["advice"]["findings"]} + assert "layout.kpi-first" in rules + assert "structural finding(s) remain" in payload["next"] + + +def test_audience_flows_through(): + _, payload = redesign_spec(decompiled(), losses=[], owned=True, + audience="executive", overlay=EMPTY) + assert payload["advice"]["audience"] == "executive" diff --git a/tests/test_smoke_fit.py b/tests/test_smoke_fit.py new file mode 100644 index 0000000..960f234 --- /dev/null +++ b/tests/test_smoke_fit.py @@ -0,0 +1,78 @@ +"""Issue #1: table/pivot heights are fixed while rendered rows grow with the +data; smoke must warn when rows would hide behind the inner scrollbar.""" + +from chartwright.smoke import _fit_warning +from chartwright.spec import load_spec + +DS = {"database": "db", "table": "orders"} + + +def spec_with(chart): + return load_spec({ + "spec_version": "1", + "dashboard": {"title": "T", "slug": "t"}, + "charts": [chart], + "layout": {"rows": [[chart["name"]]]}, + }) + + +def result_rows(rows): + return [{"data": rows}] + + +def test_pivot_overflow_warns_with_leaf_rows(): + # The issue's exact scenario: height 8 sized for 5 leaf rows; data grew to 9. + chart = {"type": "pivot_table", "name": "P", "dataset": DS, "rows": ["region"], + "columns": ["month"], "metrics": ["COUNT(*)"], "height": 8, "row_limit": 100} + spec = spec_with(chart) + # 9 regions x 2 months = 18 result rows but 9 leaf rows + data = [{"region": f"r{i}", "month": m, "count": 1} + for i in range(9) for m in ("Jan", "Feb")] + warning = _fit_warning(spec.charts[0], spec, result_rows(data)) + assert warning and "~9 leaf rows" in warning and "inner scrollbar" in warning + # 5 leaf rows fit exactly the way the author sized it + data = [{"region": f"r{i}", "month": "Jan", "count": 1} for i in range(5)] + assert _fit_warning(spec.charts[0], spec, result_rows(data)) is None + + +def test_table_overflow_and_fit(): + chart = {"type": "table", "name": "T", "dataset": DS, "columns": ["a"], + "height": 6, "row_limit": 50} + spec = spec_with(chart) + assert _fit_warning(spec.charts[0], spec, result_rows([{"a": i} for i in range(20)])) + assert _fit_warning(spec.charts[0], spec, result_rows([{"a": 1}, {"a": 2}])) is None + + +def test_smoke_and_the_design_critic_share_one_grid_model(): + """Three components once modelled 'how tall must a grid be' three ways + (0.8 units/row + 1 header here, 0.75 + 3 there, 30px/row in smoke), so one + table could be silent offline, warned data-aware, and warned again at + apply. They now read the same numbers from chartwright.spec.""" + from chartwright.design import advise + from chartwright.design.presets import Overlay + from chartwright.spec import grid_rows_visible, grid_units_for_rows + + rows, height = 20, 8 + chart = {"type": "table", "name": "T", "dataset": DS, "columns": ["a"], + "height": height, "row_limit": rows} + spec = spec_with(chart) + + # smoke (post-apply, real rows) and the offline critic agree it is too short + assert _fit_warning(spec.charts[0], spec, result_rows([{"a": i} for i in range(rows)])) + findings = advise(spec, overlay=Overlay()).findings + assert any(f.rule == "size.table-window" for f in findings), [f.rule for f in findings] + + # and both derive that from the same two functions + assert grid_units_for_rows(rows) > height + assert grid_rows_visible(height) < rows + + +def test_non_grid_charts_and_empty_results_skipped(): + chart = {"type": "timeseries_line", "name": "L", "dataset": DS, + "metrics": ["COUNT(*)"], "time_column": "ts", "height": 2} + spec = spec_with(chart) + assert _fit_warning(spec.charts[0], spec, result_rows([{"ts": 1}] * 99)) is None + pivot = {"type": "pivot_table", "name": "P", "dataset": DS, "rows": ["r"], + "metrics": ["COUNT(*)"], "height": 4} + spec = spec_with(pivot) + assert _fit_warning(spec.charts[0], spec, result_rows([])) is None diff --git a/tests/test_table_sort.py b/tests/test_table_sort.py new file mode 100644 index 0000000..c37abe4 --- /dev/null +++ b/tests/test_table_sort.py @@ -0,0 +1,140 @@ +"""Table `sort_by` must actually sort. + +It used to compile to `order_desc: true` and nothing else -- a direction with +no column to apply it to -- so a "top 10" table returned 10 arbitrary rows. +Three symptoms, one field: the compiler dropped it, the resolver never checked +it (a typo passed `check`), and `plan` reported the chart changed forever +because decompile could never produce a field the compiler never wrote. +""" + +import json + +from chartwright.compiler import _chart_params, compile_bundle +from chartwright.dashdiff import _normalize +from chartwright.decompile import decompile_bundle +from chartwright.resolver import Resolution, ResolvedDataset, _check_chart_fields +from chartwright.spec import load_spec +from chartwright.testing import stub_resolution + +DS = {"database": "db", "table": "orders"} + + +def mk(chart): + return load_spec({ + "spec_version": "1", + "dashboard": {"title": "T", "slug": "t"}, + "charts": [chart], + "layout": {"rows": [[chart["name"]]]}, + }) + + +def params_for(chart): + spec = mk(chart) + return _chart_params(spec.charts[0], spec, stub_resolution(spec)) + + +def aggregate(**over): + return {"name": "Top Regions", "type": "table", "dataset": DS, + "metrics": ["SUM(v)"], "groupby": ["region"], "row_limit": 10, **over} + + +def raw(**over): + return {"name": "Recent", "type": "table", "dataset": DS, + "columns": ["region", "v"], "row_limit": 10, **over} + + +def _lookup(spec): + ds = stub_resolution(spec).for_chart(spec.charts[0].dataset) + return lambda u: {"database": DS["database"], "schema": None, + "table": DS["table"]} if u == ds.uuid else None + + +# -- compile ------------------------------------------------------------------ + + +def test_aggregate_sort_emits_the_sort_metric(): + p = params_for(aggregate(sort_by="SUM(v)")) + sort = p["timeseries_limit_metric"] + assert sort["aggregate"] == "SUM" and sort["column"]["column_name"] == "v" + assert p["order_desc"] is True + + +def test_raw_sort_emits_order_by_cols(): + p = params_for(raw(sort_by="v")) + assert p["order_by_cols"] == [json.dumps(["v", False])] # False = descending + assert p["order_desc"] is True + + +def test_no_sort_by_emits_no_sort_keys(): + """Off is off: specs without sort_by compile exactly as before.""" + p = params_for(aggregate()) + assert "timeseries_limit_metric" not in p and "order_by_cols" not in p + assert "order_desc" not in p + + +# -- resolve ------------------------------------------------------------------ + + +def _errors(chart, columns=("region", "v")): + ds = ResolvedDataset(1, "u", "orders", None, "db", list(columns), [], None) + res = Resolution() + _check_chart_fields(mk(chart).charts[0], ds, res) + return [e.code for e in res.errors] + + +def test_typo_in_aggregate_sort_by_is_a_resolution_error(): + assert _errors(aggregate(sort_by="SUM(nope)")) == ["column_not_found"] + assert _errors(aggregate(sort_by="not_a_metric")) == ["metric_not_found"] + + +def test_typo_in_raw_sort_by_is_a_resolution_error(): + assert _errors(raw(sort_by="nope")) == ["column_not_found"] + + +def test_valid_sort_by_resolves_clean(): + assert _errors(aggregate(sort_by="SUM(v)")) == [] + assert _errors(raw(sort_by="region")) == [] + + +# -- round trip / plan --------------------------------------------------------- + + +def test_sort_by_survives_compile_decompile(): + for chart in (aggregate(sort_by="SUM(v)"), raw(sort_by="v")): + spec = mk(chart) + result = decompile_bundle(compile_bundle(spec, stub_resolution(spec)), _lookup(spec)) + assert result.losses == [], [x.as_dict() for x in result.losses] + assert result.spec["charts"][0]["sort_by"] == chart["sort_by"] + + +def test_plan_does_not_drift_on_a_sorted_table(): + """The user-visible symptom: `plan` never returned clean, so the CI drift + gate this tool advertises could not be used on any table with a sort.""" + spec = mk(aggregate(sort_by="SUM(v)")) + live = load_spec(decompile_bundle( + compile_bundle(spec, stub_resolution(spec)), _lookup(spec)).spec) + assert _normalize(live)["charts"] == _normalize(spec)["charts"] + + +def test_ascending_live_sort_is_reported_as_a_loss(): + """The spec sorts descending; an ascending live sort is NAMED, not + silently flipped on the next apply.""" + spec = mk(aggregate(sort_by="SUM(v)")) + bundle = compile_bundle(spec, stub_resolution(spec)) + import io + import zipfile + + import yaml + + zf = zipfile.ZipFile(io.BytesIO(bundle)) + out = io.BytesIO() + with zipfile.ZipFile(out, "w") as w: + for n in zf.namelist(): + blob = zf.read(n) + if "/charts/" in n: + cy = yaml.safe_load(blob) + cy["params"]["order_desc"] = False + blob = yaml.safe_dump(cy).encode() + w.writestr(n, blob) + result = decompile_bundle(out.getvalue(), _lookup(spec)) + assert any("ascending sort not preserved" in x.what for x in result.losses)