Skip to content

[Feature]: Layout and accessibility checks in done and preview, and end-to-end coverage of the design loop #493

Open
Open
[Feature]: Layout and accessibility checks in done and preview, and end-to-end coverage of the design loop#493

Description

@wudilyy999
wudilyy999
opened on Oct 14, 2026

Problem / pain point

  • done is the agent's last check before it stops, and it only sees syntax and runtime errors. The static lint in packages/core/src/tools/done.ts covers unclosed tags, duplicate ids, missing alt, broken hash links and JSX structure; makeRuntimeVerifier (apps/desktop/src/main/done-verify.ts) loads the artifact in system Chrome at 1280×800 and collects console, page and request errors. A design that loads cleanly but scrolls sideways, clips its text, sets grey text on a grey background or has tiny tap targets gets "ok — no syntactic or runtime issues detected", followed by an instruction to stop calling tools.
  • preview does not show these either: it reports nodes, width, height, loadMs, the console and asset errors, and a screenshot (vision models) or a depth-4 tag outline (text-only models). A vision model has to notice a layout defect in the picture; a text-only model has no way to.
  • Claude Design's help center says Claude can "review your design for accessibility, contrast ratios, information hierarchy, and general usability" and "checks its own output against your design system, and makes corrections before you see them" (Get started with Claude Design). In Open CoDesign the first is left to the model's eyes and the second does not exist.
  • The repo has no end-to-end coverage. CLAUDE.md and AGENTS.md name Playwright for E2E, CLAUDE.md lists pnpm test:e2e, website/roadmap.md lists "Add broader Playwright coverage for New Design → ask → scaffold → write → preview → tweaks → done" under v0.2.x, and .github/dependabot.yml groups Playwright packages, but there is no Playwright dependency, no config and no test:e2e script. The one browser-level test, apps/desktop/src/main/preview-interactions.browser.test.ts, drives system Chrome with puppeteer-core and skips itself when Chrome is missing. agent.native-lifecycle.test.ts runs the real pi Agent with a scripted stream for lifecycle cases, but no test runs the tool chain (editor, preview, done) as one flow.

I searched open and closed issues and PRs for Playwright, contrast, accessibility, overflow, layout checks and e2e. Nothing open covers this. The closed #392 bundled accessibility and code-quality verifiers with 15 skills and a multi-agent orchestrator and was not merged.

Proposed solution

Two parts. Part 1 needs no new dependency and no decision beyond this issue. Part 2 asks the maintainers a few questions before any code.

Part 1: deterministic layout and accessibility checks in preview and done

  1. One probe function that runs inside the page Chrome already has open: plain DOM, getComputedStyle and getBoundingClientRect, no library. preview-runtime.ts already runs a page.evaluate for metrics, and done-verify.ts has a settle window after load; both call the same probe from a shared module, so the two reports cannot disagree. It returns at most MAX_LAYOUT_ISSUES entries (20), each with the check name, a selector and the measured numbers, and it visits at most MAX_PROBE_ELEMENTS elements (5,000). If it reaches that cap, throws or times out, the report says so as a warning; a failed probe is never dropped silently.
  2. Checks, each with named thresholds:
    • horizontal overflow: the document is wider than the viewport, at 1280 px and again at a 390 px phone width (the verifier resizes the viewport, no reload);
    • clipped text: an element with overflow: hidden whose content is larger than its box;
    • contrast: WCAG 2.x ratio of the text colour over the effective background, 4.5:1 and 3:1 for large text. The background comes from walking ancestors and compositing alpha; colours are resolved through a 1×1 canvas so oklch() or color-mix() arrive as sRGB. A background image or gradient is reported as "not measured" instead of guessed;
    • tap targets: links, buttons and inputs under 24×24 CSS px (WCAG 2.2 SC 2.5.8);
    • controls with no accessible name: no text, aria-label, aria-labelledby, <label> or title.
  3. Findings are warnings, not errors. done still decides ok / has_errors from errors alone, so a false positive cannot start a repair loop or use up the three repair rounds. They appear in the existing non-blocking warnings block of the done result (its heading changes from "design metadata warnings" to "warnings"), and preview gets a layoutIssues field next to metrics. DoneRuntimeVerifier returns { errors, warnings } instead of DoneError[]. It has one producer (makeRuntimeVerifier) and two consumers: makeDoneTool, and the standalone done:verify:v1 IPC, which keeps returning errors and gains warnings.
  4. Later, once feat(core): map design tokens to DESIGN.md fields #490 lands: a drift check that compares the colours and font families the page uses with the tokens in DESIGN.md, reusing the colour parser from that PR. It is the closest equivalent of "checks its own output against your design system".

Part 2: regression coverage for the design loop

  • 2a. Vitest tests with the real pi Agent and a scripted model stream, in the style of agent.native-lifecycle.test.ts: scaffold, write through the editor tool, preview, done returns has_errors, fix, done returns ok; and a layout warning that does not block done. No new dependency, part of pnpm test.
  • 2b. An Electron smoke test: start the built app against a local fake model endpoint, create a design, see the preview render. This needs answers to three questions before I write it:
    1. Driver: Playwright, which the docs and roadmap name (Apache-2.0, dev-only, never shipped), or the puppeteer-core already in the tree attached to Electron's remote-debugging port. If you prefer the second, I will correct the wording in CLAUDE.md, AGENTS.md and the roadmap to match.
    2. Model: a local HTTP server that speaks the OpenAI-compatible stream, selected through the normal custom-provider setting, so the app needs no test-only code path. Is that acceptable?
    3. CI: check runs on ubuntu only to control Actions cost and already installs the Electron binary. Should the smoke test join that job (Linux needs a virtual display), run in its own workflow, or stay a local pnpm test:e2e?

Plan. About 3,000 lines in total, about half of it tests, in PRs of 350 to 700 lines. Each PR stands alone and keeps pnpm lint && pnpm typecheck && pnpm test green.

  1. Probe, overflow and clipped-text checks, the { errors, warnings } contract, and the preview and done wiring (about 550).
  2. Contrast (about 450).
  3. Tap targets and accessible names (about 350).
  4. 2a agent-loop tests (about 600).
  5. Drift check, after feat(core): map design tokens to DESIGN.md fields #490 (about 400).
  6. 2b Electron smoke test, after your answers (about 700).

Alternatives considered

Scope

Generation capability

Hard-constraints check

  • I have read the hard constraints in CLAUDE.md and this proposal does not conflict with any of them. Part 1 adds no dependency and runs only inside the Chrome page that preview and done already open; the probe sits in modules those tools already load. Part 2b's driver is a dev-only tool and is a question for you.

Activity

  1. github-actions commented on Oct 14, 2026

    @github-actions
    github-actionsbot
    on Oct 14, 2026 – with GitHub Actions
    Contributor

    Thanks for the thorough proposal — separating the no-new-dependency work in Part 1 from the questions in Part 2b makes it easy to evaluate, and the body points at the right modules.

    What I can confirm from the public tree

    • The docs/scripts mismatch you describe is real. CLAUDE.md states "Tests: Vitest (unit) + Playwright (E2E)" and lists pnpm test:e2e, but the root package.json scripts include test, test:scripts, lint, lint:fix, typecheck, format, build, dev, docs:*, smoke, changeset, version-packages, release — there is no test:e2e. pnpm-lock.yaml lists puppeteer-core under the apps/desktop and packages/exporters importers, which is consistent with preview-interactions.browser.test.ts driving system Chrome and skipping when Chrome is absent. The documented Playwright story is ahead of the actual dependency tree.
    • Part 1 as described fits hard constraints 4 and 6: a probe built on plain DOM, getComputedStyle and getBoundingClientRect, running inside the Chrome page the tools already open, adds no shipped/bundled dependency and no app-start cost.

    On the Part 1 design

    • Keeping layout findings as warnings while done decides ok / has_errors from errors alone is the right call — a heuristic false positive should not consume a repair round. Please make sure the heading change from "design metadata warnings" to "warnings" does not change how the model reads that block.
    • The { errors, warnings } change touches two consumers (makeDoneTool and the done:verify:v1 IPC). CLAUDE.md asks that IPC payloads carry a schemaVersion; the standalone IPC should gain warnings additively rather than reshaping the existing errors.
    • Caps (MAX_LAYOUT_ISSUES, MAX_PROBE_ELEMENTS) plus a surfaced warning on cap hit, timeout or thrown probe is the right failure mode — and keeping "not measured" for background images/gradients instead of guessing a ratio is the correct default.
    • Deferring the DESIGN.md drift check until feat(core): map design tokens to DESIGN.md fields #490 lands is reasonable; that is the only piece that depends on new colour parsing.
    • Your PR plan matches repo practice, though note the root exposes pnpm typecheck (Turborepo), so pnpm -r typecheck is not required. New features are expected to ship at least one Vitest test, so the thresholds and cap behaviour should be unit-testable without Chrome.

    Part 2b questions (maintainer calls)

    1. Driver — both paths exist in the tree today: the docs name Playwright, the code uses puppeteer-core. A dev-only, never-shipped Playwright dependency is allowed under constraint 4 (and Apache-2.0 is permissive regardless), but it would be a second browser-automation dependency alongside puppeteer-core. Whichever is chosen, the wording in CLAUDE.md, AGENTS.md and the roadmap should be corrected to match.
    2. Model — a local HTTP server speaking the OpenAI-compatible stream, selected through the normal custom-provider setting with no test-only code path, is the approach that fits the "reuse pi primitives" direction; a maintainer should confirm the provider registration path needs no special-casing.
    3. CI — this is a maintainer decision, but the tradeoff is Ubuntu-only check needing a virtual display for Electron (e.g. xvfb), a separate workflow, or a local-only pnpm test:e2e.

    Thanks also for linking #392 as prior art.

    Need more info:

    • Which platform would you develop the Electron smoke test on locally (macOS / Linux / Windows)? That determines whether display setup is only a CI concern for you.

    Open-CoDesign Bot

  2. wudilyy999 commented on Oct 15, 2026

    @wudilyy999
    wudilyy999
    on Oct 15, 2026
    ContributorAuthor

    Thanks for the review. Answers in order.

    Platform. macOS. I would develop and run the Electron smoke test locally on macOS, where no display setup is needed. The Linux virtual display would only be a CI concern, and I would try that job on a fork before proposing it.

    IPC. done:verify:v1 keeps its errors field as it is and gains warnings as an additional field, so the channel name and existing callers stay the same. The current payload has no schemaVersion; I will add schemaVersion: 1 next to warnings in the same PR, again as an additional field.

    Heading. Each warning entry already carries a source. The block heading becomes "Non-blocking warnings" and the entries keep the source: message form, so layout and DESIGN.md warnings read the same way: advice that does not change ok or has_errors.

    Commands. You are right about pnpm typecheck; I have corrected the plan in the description, and the PR descriptions will use pnpm lint && pnpm typecheck && pnpm test.

    Testing without Chrome. The in-page function only collects raw samples (resolved sRGB colours, rectangles, overflow flags, element counts). The decisions live in pure functions on the Node side: the WCAG ratio, alpha compositing, the thresholds, MAX_LAYOUT_ISSUES and the cap, timeout and thrown-probe warnings. Those get plain Vitest tests. A Chrome-backed test follows preview-interactions.browser.test.ts and skips itself when Chrome is missing.

  3. github-actions commented on Oct 15, 2026

    @github-actions
    github-actionsbot
    on Oct 15, 2026 – with GitHub Actions
    Contributor

    Thanks for the detailed follow-up — the answers line up with what's in the public tree, so here's a point-by-point closure on the open questions and what's still a maintainer call.

    Platform. macOS for local development is a fine choice. Nothing in the repo requires display setup on macOS, and keeping the Linux virtual-display work as a fork/CI experiment is the right way to de-risk it before proposing a job.

    IPC. Keeping the errors field as-is and adding warnings plus schemaVersion: 1 as additional fields is the correct shape. CLAUDE.md requires a schemaVersion on IPC payloads ("Schema-version everything that lives on disk. Config files, SQLite tables, IPC payloads, exported bundle formats — all carry a schemaVersion field"), so this closes the gap the earlier review flagged. Adding it alongside the existing done:verify:v1 channel name is slightly redundant but harmless, and it keeps existing callers working as long as unknown fields are ignored.

    Heading. "Non-blocking warnings" with source: message entries keeps layout and DESIGN.md warnings reading the same way and, importantly, leaves ok / has_errors driven by errors only. That preserves the repair-round semantics we discussed.

    Commands. Confirmed against the root package.json: it exposes lint, typecheck, test (and test:scripts), and there is still no test:e2e script. pnpm lint && pnpm typecheck && pnpm test is the right trio; note that root test runs pnpm test:scripts && turbo run test.

    Testing without Chrome. Splitting the in-page sampler (raw sRGB colours, rects, overflow flags, counts) from the pure decision functions on the Node side (WCAG ratio, alpha compositing, thresholds, MAX_LAYOUT_ISSUES, cap/timeout/thrown-probe warnings) is exactly the testable boundary this repo wants, and a Chrome-backed test that skips when Chrome is absent matches the pattern already used by the existing browser test. Good call.

    Verified from the public tree:

    • package.json — lint, typecheck, test, test:scripts exist; no test:e2e.
    • pnpm-lock.yaml — puppeteer-core appears under the apps/desktop and packages/exporters importers, consistent with the existing Chrome-driven test.
    • CLAUDE.md — schema-version requirement covers IPC payloads; "New features require at least one Vitest test."
    • AGENTS.md — constraint 7 ("Reuse pi primitives first") and constraint 5 (permissive shipped deps, workflow-only tools may differ) are the ones Part 2 touches.

    Still maintainer calls (unchanged):

    1. Electron smoke-test driver — Playwright (named in docs/roadmap, permissive, dev-only) vs the puppeteer-core already in the tree. Whichever wins, the docs/roadmap wording should be corrected to match.
    2. CI placement for the smoke test — join the Ubuntu-only check job (needs a virtual display), a separate workflow, or local-only pnpm test:e2e.
    3. A maintainer should still confirm the provider-registration path for the local fake OpenAI-compatible endpoint needs no test-only special-casing.

    None of those block Part 1 or Part 2a, so those PRs can land first while the driver/CI questions are decided.

    Open-CoDesign Bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions