Repository navigation
feat(desktop): check layout and accessibility in done and preview - #495
Conversation
DoneRuntimeVerifier now returns { errors, warnings } instead of a bare
error list. Warnings never change the done status, so a false positive
cannot start a repair round; they are listed under "Non-blocking
warnings" with the existing metadata warnings. When a layout warning is
present the stop instruction changes: the agent may fix one it agrees
with and check it with preview, but is told not to call done again.
The preview result gains an optional layoutIssues list, capped at 20
and never part of ok. done:verify:v1 keeps its errors field and gains
warnings and a schemaVersion.
The desktop verifier reports no warnings yet.
After a page loads, preview and done now measure it at its current width and again at a 390 px phone width, then put the viewport back. They look for a page that scrolls sideways, naming the outermost elements that cross the edge, and for text that its box clips. A page that hides horizontal overflow, content inside a scroller, fixed elements, ellipsis and line clamps, collapsed panels and running animations are not reported. The page-side function only collects measurements; the decisions are made on plain data, so the thresholds and the budgets (20 findings, 5,000 elements, 3 elements and 5 clipped texts per width) are tested without a browser. preview returns the findings as layoutIssues and done lists them as warnings. A failed measurement is reported as a warning, a cancelled run still stops.
The layout probe is split into the shared samples and limits, the page function, and the analysis, which works on plain data. Three checks join overflow and clipped text: - contrast: the text colour over the backgrounds behind it, WCAG 2.x AA (4.5:1, and 3:1 for large text), grouped by colour pair and listed worst first. Text on an image or gradient, or partly transparent, is counted and reported, not guessed. Colours are read through a canvas so oklch() and color-mix() come out as sRGB. - tap targets: a link, button or input under 24 by 24 px, at phone width, unless it is inline in text or has no other target within that square (WCAG 2.2 SC 2.5.8). - names: a control with no text, aria-label, aria-labelledby, label, title, image alt or SVG title. Contrast and names run at the current width, tap targets at phone width. When there are more findings than the budget, one is taken from each check in turn; findings past a per-check limit are counted and the result says so. LayoutIssue gains the three checks, and its measured and limit fields become optional, since a missing name has no number.
A fixture for each rule and each exemption, in system Chrome: grey on white just under 4.5:1 and the 3:1 rule for large text, backgrounds from ancestors and translucent layers, oklch() and color-mix(), pairs reported once with a count, and text on a gradient or partly transparent counted instead of guessed. Small targets pass alone and fail beside another target, inline links are exempt, disabled and visually hidden controls are skipped. Names come from text, aria-label, aria-labelledby, label, title, image alt, SVG title and input defaults; a placeholder is not a name. Alpha in rgb() and rgba() is read exactly: a canvas rounds it to 8 bits and moved a ratio by 0.03.
A small target passes alone, so a test for an exemption has to put a target beside it; otherwise the check would pass without the exemption. Disabled, hidden, transparent, aria-hidden and visually hidden controls and a link inside a sentence now each sit next to a neighbour, and removing the exemption fails the test. The colour reader only handles the comma form of rgb() and rgba() that Chrome writes for computed colours; every other space goes through the canvas.
…ted model Eight scenarios on the real pi Agent, with only the model stream and the browser executors replaced: write, preview, hear a runtime error from done, fix it and finish; a layout warning that does not fail done and changes what the agent is told; fixing the warning and checking it with preview, not done; preview listing layout issues while staying ok; warnings beside runtime errors; five done calls with warnings that never spend a repair round; three error rounds that still end the run with the workspace kept; and a layout measurement that did not run.
The harness document test replaces puppeteer and answered the layout function with an early, partial shape. The analysis now reads every field, so it failed and the verifier reported "Layout checks did not run". It answers with the shared fixture instead.
There was a problem hiding this comment.
Review mode: initial
Findings
-
[Minor] Text clipped on the left or top is reported as a bottom clip with self-contradictory numbers —
collectLayoutSamplesflags a clip whentext.left < box.left - toleranceortext.top < box.top - tolerance(apps/desktop/src/main/layout-probe.ts, thechecks.clippedblock), but theClippedTextsample only carriestextRight/boxRight/textBottom/boxBottom(apps/desktop/src/main/layout-samples.ts), andclippedIssuespicks the vertical branch whenevertext.textRight <= text.boxRight(apps/desktop/src/main/layout-analysis.ts,clippedIssues()). A left/top-only overflow (negativetext-indent, RTL, a negative inline margin) therefore emits "…the text reaches N from the top but the box ends at M" where N < M, so the warning contradicts itself and points the agent at the wrong side. Impact is limited because findings are advisory and never changeok/done, but the mismatch is real and untested (noleft/topcase exists inlayout-analysis.test.tsor the browser tests).
Suggested fix: record the overflowing side (ortextLeft/textTop) inClippedTextand describe that side, or drop thetext.left/text.topterms from the probe if left/top clipping is intentionally out of scope. -
[Nit]
previewcan exceed its declared warning budget —PreviewResult.warningsis declaredType.Array(Type.String(), { maxItems: 5 })(packages/core/src/tools/preview.ts:163) andtrimPreviewResultslicesconsoleErrors,assetErrorsandlayoutIssuesbut notwarnings.runPreviewsetsresult.warningsfrominspection.notes(up to 5: element/control truncation, unmeasured contrast, dropped colour groups, omitted findings, over-budget) and the recovered-shutdown branch now appends one more instead of replacing (apps/desktop/src/main/preview-runtime.ts,report.warnings = [...(report.warnings ?? []), message]). A page that both truncates the probe and fails graceful shutdown therefore yields 6 warnings. Nothing currently runtime-validatesmaxItems, so this is contract drift rather than a hard failure.
Suggested fix: slicewarningsintrimPreviewResultthe same way as the other arrays.
Questions
- None blocking.
Summary
- Review mode: initial
- The change is internally consistent with the stated contract: layout findings are advisory,
donederivesstatusfromerrorsalone, andpreview.okis untouched (verified inpackages/core/src/tools/done.tsandpackages/core/src/tools/preview.tsplus the newagent.design-loop.test.ts/layout-wiring.*.test.ts). Thedone:verify:v1change is additive (schemaVersion: 1+warnings) and the preload type is updated in step (apps/desktop/src/main/ipc/generate.ts,apps/desktop/src/preload/index.ts). - Constraints hold: no new dependencies, no direct provider SDK imports, probe is plain DOM/
getComputedStyle/canvas, measurement separated from decision (layout-probe.tsvslayout-analysis.ts), and cancellation re-throws rather than being swallowed (signal?.throwIfAborted()indone-verify.ts; the preview path is covered bylayout-wiring.failure.browser.test.ts). A changeset is present. - Linked-issue scope: the PR uses
Refs #493and explicitly describes this as part of the plan, so the completion criteria are not claimed and no issue-closure validation is needed. - Residual risk:
hasLayoutAdvicealso fires for the truncation/partial-measurement notes indone, so a non-defect note (e.g. ">5,000 elements") switches the agent from theSTOPinstruction to the fix-and-preview wording. That looks deliberate (anylayout-sourced warning enters advice mode) but is worth confirming, since the note is not an actionable defect.
Testing
- Not run (automation). Coverage is strong: pure colour-maths/selection units (
layout-analysis.test.ts,layout-probe.test.ts), system-Chrome fixtures per rule and exemption (layout-probe.browser.test.ts,layout-probe.contrast.browser.test.ts,layout-probe.accessibility.browser.test.ts), end-to-end wiring (layout-wiring.browser.test.ts,layout-wiring.failure.browser.test.ts), and a production-agent design loop (agent.design-loop.test.ts). The one gap is a left/top-only clip case (see the Minor finding).
Open-CoDesign Bot
PreviewResult.warnings declares at most five entries, but trimPreviewResult cut only the console errors, asset errors and layout issues. The layout notes can already fill the list, and a late browser shutdown adds one more, so a result could carry six. It is cut like the other arrays now. Refs OpenCoworkAI#493
The probe flagged text that sticks out of a clipping box on the left or top, but a clipped-text sample held only the right and bottom edges, and the analysis chose the bottom message whenever the text did not reach past the right edge. Text pushed off the left or top by a negative text-indent, a negative margin or a flex alignment was reported as "reaches N from the top but the box ends at M" with N below M, which names the wrong side. A sample now records the side that sticks out the most, with the text's edge and the box's edge on that side, and the message describes that side. The right edge no longer wins by default either: a 1 px overhang on the right used to choose the right-edge message even when the bottom was cut by 30 px. Refs OpenCoworkAI#493
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
- No new findings. Both items from the previous review are resolved in the commits since
b098de8:- [Minor → resolved] Left/top clipping is no longer misreported.
ClippedTextnow carriesside: ClippedSideplustextEdge/boxEdge(apps/desktop/src/main/layout-samples.ts), the probe picks the worst of the four sides withbeyond = text.right - box.right | text.bottom - box.bottom | box.left - text.left | box.top - text.top(apps/desktop/src/main/layout-probe.ts, thechecks.clippedblock), andclippedEdges()switches on that side so the message and themeasured/limitsign agree (apps/desktop/src/main/layout-analysis.ts). Unit coverage now includesleft/top(layout-analysis.test.ts) and the browser test drives all four sides plus the 2 px/3 px tolerance boundary (layout-probe.browser.test.ts). The reported numbers are now self-consistent (Math.sign(measured - limit)is asserted per side), so the contradictory "reaches N from the top …" string can no longer be produced. - [Nit → resolved]
previewwarning budget is now enforced.MAX_PREVIEW_WARNINGS = 5is the single source for both thePreviewResult.warningsmaxItemsand the new slice intrimPreviewResult(), andpreview.test.tsasserts the cap.
- [Minor → resolved] Left/top clipping is no longer misreported.
Questions
- None blocking.
Summary
- Review mode: follow-up after new commits
- No material issues. The change stays advisory-by-construction:
donederivesstatusfromerrorsalone (packages/core/src/tools/done.ts) andpreview.okis untouched, so a heuristic finding cannot cost a repair round; thedone:verify:v1change remains additive (schemaVersion: 1+warnings) with the preload type updated in step (apps/desktop/src/main/ipc/generate.ts,apps/desktop/src/preload/index.ts). - Constraints hold: no new dependencies, no direct provider SDK imports, probe is plain DOM/
getComputedStyle/canvas, measurement (layout-probe.ts) is separated from decisions (layout-analysis.ts), and cancellation still re-throws rather than being swallowed. The changeset is present and the changes remain covered by the linked issue as partial work (Refs #493). - Residual observations (not merge blockers):
- The recovered-shutdown warning is now appended rather than replacing the list, but
trimPreviewResult()keeps the first 5 warnings, so on a page that already fills the budget with measurement notes the shutdown message is dropped from the tool result. The browser was terminated regardless and the path still emitspreview.cleanup.recoveredviagetLogger('preview'), so this reads as acceptable cleanup degradation, not a silent user-visible failure. - Carried over from the initial review and unchanged by these commits:
hasLayoutAdvicekeys offsource === LAYOUT_WARNING_SOURCE, so a non-defect note (e.g. the ">5,000 elements" truncation note) also switchesdonefrom theSTOPwording to the fix-and-previewwording. This looks deliberate (any layout-sourced entry enters advice mode); worth a maintainer confirmation only.
- The recovered-shutdown warning is now appended rather than replacing the list, but
Testing
- Not run (automation). Coverage for the touched paths is strong and proportionate: colour maths/selection units (
layout-analysis.test.ts,layout-probe.test.ts), system-Chrome fixtures per rule and exemption including the new per-side clip matrix (layout-probe.browser.test.ts,layout-probe.contrast.browser.test.ts,layout-probe.accessibility.browser.test.ts), end-to-end wiring and failure/cancellation (layout-wiring.browser.test.ts,layout-wiring.failure.browser.test.ts), the production-agent design loop (agent.design-loop.test.ts), and the newMAX_PREVIEW_WARNINGScap assertion inpreview.test.ts.
Open-CoDesign Bot
main gained OpenCoworkAI#496, which added a done test next to the runtime warnings tests in done.test.ts. Both blocks are kept. The new test passed a verifier that returned an array, so it returns { errors, warnings } like the others.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
- No new findings. The commits since
ea14719are a merge ofmain's DESIGN.md compatibility work (.changeset/design-md-generated-output-compatibility.md,packages/shared/src/design-md.ts,apps/desktop/resources/templates/skills/design-system-baton.md) plus the matching test-shape updates; the probe/analysis code (layout-probe.ts,layout-analysis.ts,layout-samples.ts,done-verify.ts,preview-runtime.ts) is unchanged from the last reviewed state, and both previously reported items stay resolved.
Summary
- Review mode: follow-up after new commits
- The merge was checked for interaction with this PR's contract and is consistent: DESIGN.md extension findings keep
source: 'DESIGN.md', layout findings useLAYOUT_WARNING_SOURCE(packages/core/src/tools/done.ts), sohasLayoutAdvicestill keys only offsource === 'layout'. A DESIGN.md warning alone therefore keeps theSTOPwording (covered by the PR'skeeps the stop instruction when no layout warning is present), and a page with both lists them together under the single "Non-blocking warnings" heading whilestatusis still derived fromerrorsalone. - The
done:verify:v1wire change stays additive (schemaVersion: 1pluswarnings) with the preload type in step (apps/desktop/src/main/ipc/generate.ts,apps/desktop/src/preload/index.ts), andpreview.okremains untouched bylayoutIssues. - Project constraints hold: no new dependencies, no direct provider SDK imports, probe is plain DOM/
getComputedStyle/canvas, decisions stay separated from measurement, and cancellation re-throws (signal?.throwIfAborted()indone-verify.ts). - Residual observations (not merge blockers, unchanged from the previous review):
previewkeeps only the first 5 warnings aftertrimPreviewResult(), so on a page already at the note budget the recovered-shutdown message can be dropped from the tool result; the browser is still terminated andpreview.cleanup.recoveredis still logged.- A layout note (e.g. the ">5,000 elements" truncation note or "contrast was not checked") also flips
doneinto the fix-and-previewwording even though it names no defect. Looks deliberate; worth a maintainer confirmation only. - The layout catch in
preview-runtime.tsdoes not callopts.signal?.throwIfAborted()the waydone-verify.tsdoes, so cancellation during measurement relies on the later signal checks inrunPreviewto keep the run cancelled.layout-wiring.failure.browser.test.tsassertsok: false/reason: 'Preview cancelled', so this reads as intentional; just worth keeping in mind if the post-measurement flow changes.
Testing
- Not run (automation). Coverage remains proportionate: unit tests for the colour maths, selection and warning mapping (
layout-analysis.test.ts,layout-probe.test.ts), system-Chrome fixtures per rule and exemption (layout-probe.browser.test.ts,layout-probe.contrast.browser.test.ts,layout-probe.accessibility.browser.test.ts), end-to-end wiring and failure/cancellation (layout-wiring.browser.test.ts,layout-wiring.failure.browser.test.ts), and the production-agent loop (agent.design-loop.test.ts). The merged DESIGN.md tests exercise the warning path alongside it.
Open-CoDesign Bot
Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
There was a problem hiding this comment.
Findings
- No new findings in the commits since
76118ead. The only functional change since the last review is inapps/desktop/src/main/layout-probe.ts:471-502:runInspectionnow bounds each step with a sharedLAYOUT_BUDGET_MSdeadline (withinBudget) and restores the preview viewport under its ownLAYOUT_RESTORE_BUDGET_MS(apps/desktop/src/main/layout-samples.ts:14), instead of wrapping the whole inspection in oneboundedPreviewwith an unboundedfinallyrestore. That ordering is correct: thefinallyawait finishes before the caller resumes, and on timeout or cancellation the inspection still rejects, sodone-verify.ts/preview-runtime.tsreport the failure as a warning and the run stays cancelled. The newapps/desktop/src/main/layout-probe.lifecycle.test.tsexercises the four paths that matter (phone-measurement timeout, already-cancelled call, desktop overrun, cancellation mid-phone-measurement). - The rest of the compare diff is a merge of
main:@types/node22.20.5, postcss 8.5.30 /nanoidlockfile churn, and the SHA-pinnedanchore/sbom-actionandossf/scorecard-actionbumps in.github/workflows/. Those are not author changes and are already on the base branch, so they do not need review here.
Summary
- Review mode: follow-up after new commits
- The bounded-restore change is consistent with the PR's stated contract: findings remain advice (
okand thedonestatus still derive fromerrorsonly), a failed measurement still surfaces asLayout checks did not run: …rather than a clean page or a failed run, and no new wire, budget, or dependency changes were introduced. - Previously reported behaviour stays intact:
done:verify:v1is still additive (schemaVersion: 1+warnings),preview.layoutIssuesdoes not changepreview.ok, andLAYOUT_WARNING_SOURCEstill driveshasLayoutAdviceinpackages/core/src/tools/done.tsindependently of DESIGN.md warnings. - Residual observations (non-blocking, unchanged unless noted):
trimPreviewResultkeeps only the firstMAX_PREVIEW_WARNINGS(5) warnings (packages/core/src/tools/preview.ts), so the recovered-shutdown message appended inapps/desktop/src/main/preview-runtime.tscan still be dropped when the note budget is already full; the browser is still terminated andpreview.cleanup.recoveredis still logged.- A layout note (the
>5,000 elementstruncation note, “contrast was not checked”, or the per-check-limit note) also flipsdoneinto the fix-and-previewwording even though it names no defect. Reads as deliberate. - The
catcharoundinspectLayoutinapps/desktop/src/main/preview-runtime.tsstill does not callopts.signal?.throwIfAborted(); cancellation relies on the later signal checks inrunPreview, which the failure browser test asserts. - New, low risk: if
page.setViewport(original)itself exceedsLAYOUT_RESTORE_BUDGET_MS, that rejection propagates as the inspection failure and a vision preview still proceeds to the screenshot after thecatch. Only reachable with a wedged renderer, and no test covers a restore timeout (the lifecycle tests cover measurement timeouts and cancellation only). - The PR body still describes the budget as “5 seconds for both measurements”; the effective worst case is now 5 s of measurement plus up to 1 s of restore. Worth a one-line update in the description, not a code change.
- Apart from those notes, the PR looks ready to merge.
Testing
- Not run (automation). Coverage is proportionate: unit tests for the colour maths, issue selection, warning mapping and lifecycle (
layout-analysis.test.ts,layout-probe.test.ts,layout-probe.lifecycle.test.ts), system-Chrome fixtures per rule and exemption (layout-probe.browser.test.ts,layout-probe.contrast.browser.test.ts,layout-probe.accessibility.browser.test.ts), end-to-end wiring and failure/cancellation (layout-wiring.browser.test.ts,layout-wiring.failure.browser.test.ts), and the production-agent loop (agent.design-loop.test.ts).
Open-CoDesign Bot
Summary
Refs #493 (the first part of the plan, PRs 1 to 4, in one PR).
doneonly saw syntax and runtime errors, so a page that loaded cleanly but scrolled sideways, hid part of its text, set grey text on white, or had an icon button with no name got "ok" and an instruction to stop.doneandpreviewnow measure the open page and report what they find as advice. Findings never changeokor thedonestatus, so a false positive cannot cost a repair round.#493 planned this as four PRs. I put them in one so that the contract, the checks and the tests of the whole loop land together, and kept the commits in the planned order. If you would rather review them separately, they split at these commits:
c2c8019(the contract; behavior unchanged),993fdab(probe, overflow and clipped text),31bcfd7tocf9a565(contrast, tap targets, names),aeffd6fandb098de8(the scripted-agent tests and the fake browser they use).What is measured
At the page's own width (1280 px for
done, theviewportoption forpreview) and at 390 px, after which the viewport is put back. No library: plain DOM,getComputedStyle,getBoundingClientRectand a 1 × 1 canvas to read colours in spaces other than sRGB.position: fixedtext-overflow: ellipsis, a line clamp, a box with no height or width, a running CSS animationaria-hiddenand visually hidden controlsaria-label,aria-labelledby,<label>,title, imagealtor SVGtitle(input defaults count; a placeholder does not)aria-hiddenand hidden controlsReporting
previewreturns the findings aslayoutIssues(at most 20, omitted when empty;okis unchanged) and counts them in its summary line.donelists them under "Non-blocking warnings". When one is present, the stop instruction changes: the agent may fix it and check it withpreview, and is told not to calldoneagain, so advice cannot start a loop.DoneRuntimeVerifierreturns{ errors, warnings }.done:verify:v1keepserrorsand gainswarningsand aschemaVersion.previewkeeps at most 5 warnings; 20 findings in all, taken from each check in turn when there are more; 5,000 elements and 400 controls per measurement; 5 seconds for both measurements, plus a separate 1-second budget to restore the original viewport before returning after success, failure, timeout or cancellation. Past a limit, or on a page with a background image, the result says so (inwarningsforpreview, as a layout warning fordone). Findings past a per-check limit are counted and reported.layoutwarning too, sodoneuses the advice wording for it. That wording is conditional ("if you agree one is a real defect") and a note names no defect, so the agent writes its summary.Not in this PR
The DESIGN.md drift check (it needs the colour parser in #490) and the Electron smoke test (the questions in #493 about the driver, the model and CI).
Size
+3,354 / −92 over 30 files: 1,121 lines of code in 12 files and 2,227 lines of tests. The plan in #493 estimated about 1,950 lines across the four PRs. The difference is mostly browser tests: a fixture for each rule and each exemption in system Chrome, and, for each exemption, a page that fails without it.
Type of change
Linked issue
Refs #493
Checklist
pnpm lint && pnpm typecheck && pnpm testpasses locally (see Checks)pnpm changeset) if user-visibleNo page in
website/describesdoneorpreviewchecks, so there is nothing to update.Checks
pnpm lint,pnpm typecheck(10 packages) andpnpm testpass on this branch at6c799fd: core 608, desktop 2,802 (190 files), shared 350, exporters 198, and the other packages. The first rebase onto main needed one change: thedonerepair-limit test that feat(core): stop a run that keeps repeating the same tool call #489 added returned the old verifier shape, so it now returns{ errors, warnings }(in the first commit,c2c8019). Merging main later brought fix: tolerate compatible DESIGN.md metadata during verification #496, which added adonetest beside mine indone.test.ts; I kept both and changed its verifier to return{ errors, warnings }. The PPTX tests inpackages/exportersunpack under/tmp, a symlink on macOS, wherezip-librefuses to write; turbo's strict env mode dropsTMPDIR, so my Mac falls back to/tmp. WithTURBO_ENV_MODE=loose TMPDIR=/private/tmpall of them pass. I did not touch that package.preview-interactions.browser.test.ts.agent.design-loop.test.tsruns the production agent with only the model stream and the browser executors replaced.6c799fdis Sun-sunshine06's commit on this branch: each measurement step is bounded by the shared deadline, and the viewport is restored before the inspection ends, withlayout-probe.lifecycle.test.ts(four paths). I did not mutation-check that change.previewresult and summary, the IPC fields) and each change made a test fail, except one: when the browser shuts down late, the recovered-shutdown warning is now added to the list instead of replacing it, and no test covers that (the path had none before). Several first runs survived because my fixtures never made the page scroll sideways, or put no target next to a small one; they now do. After the first review I changed the clipped-side code and the warning cap in 12 more places (the sides recorded, the choice of the side that sticks out most, both sides of the 2 px tolerance, the edges, each message, the cap); each fails a test, and the one that first survived, the tolerance at exactly 2 px, now has one.Principles
donestill decidesok/has_errorsfrom errors alone, andpreview.okis unchanged. The wire changes are additive (warningsandschemaVersionondone:verify:v1,layoutIssues), and the heading "Non-blocking design metadata warnings" is now "Non-blocking warnings".layout-probe.tscollects,layout-analysis.tsdecides on plain data); every threshold and budget is a named constant passed to the page function.doneandpreview, so the two reports cannot disagree.