Polish dialogs with shared design-system controls and spacing

[mahanti]

Dialog forms used separate spacing and control styles, and Edit agent split settings and diagnostics across a sidebar and nested scroll areas. This change brings them onto shared dialog, field, choice, avatar, and disclosure recipes. Edit agent now has one scrolling column with Model, Environment, Runtime, and Technical details under Advanced; save, runtime, and dismissal behavior remain feature-owned.

• Align search/community row content with dialog headings, preserve narrow-screen gutters, and document the composition rule in DESIGN.md and the design viewer.
• Use shared Select/Combobox presentation and preserve custom values, discovery cancellation, disabled states, and drafts in collapsed sections.
• Set shared Dialog titles to text-label (16px/500), keep an actual 8px title/description gap independent of the close-button target, and give the body vertical padding matching the dialog sides (24px, 16px on narrow screens).
• Default shared IconButton controls to round. Animate shared Dialog entry/exit with existing motion tokens; reduced motion, keyboard navigation, Escape, and the search palette remain immediate.
• Add five interactive design-system Dialog examples from a simple message through grouped settings, alongside the gallery of all 12 app dialog uses.

Interactive gallery

Run the interactive gallery to inspect all 12 app dialog uses with local fixture data.

Validation

At b67f932, merged main (20d7447) and resolved the three design-viewer conflicts, preserving menu and floating-surface additions alongside dialog examples and motion coverage. Application/design builds, 75 design unit tests, all 44 viewer browser cases in Chromium and WebKit (38.9s, no retries), and required hooks passed (366 related unit tests, application/design types and design guards). No browser cases were added or removed by this resolution. Hosted CI is pending at this head.

At cfd10c0:

• Application/design TypeScript and builds passed, along with design guards and all 72 design-system unit tests.
• All 40 design-system browser cases passed in Chromium and WebKit with no retries (24.5s), using the built viewer and required host servers. An initial run against the development preview failed only the no-WebSocket and missing-host-server checks; the configured run passed without changing assertions.
• Browser measurements confirmed 16px/500 titles, an 8px title/description gap, and matching body/side padding at 390px, 800px, and 1234px in both engines. All five examples open/close; grouped settings can expand and save on narrow screens. Round icon controls were checked across sizes.
• Required company/repository commit and push hooks passed, including application/design TypeScript, all 362 related unit tests, and design guards.

The earlier agent polish passed all 32 affected agent browser cases in both engines before rebase; all hosted browser-journey shards and Rust checks also passed at 3702474. That head's JavaScript check failed on a 15-second timeout in the unchanged relay read-state growth/restart test. Updated hosted CI is pending.

Browser cases added/removed: 1 / 0 (the added motion case runs in both engines). It holds real CSS transitions to verify exit presence, focus restoration, interrupted entry, keyboard activation, and reduced motion—boundaries requiring a real browser. During implementation it caught interrupted Escape leaving a paused transition; explicit immediate-transition styling repaired it, and both engines pass. Existing agent journeys retain behavior assertions and cover column alignment, keyboard disclosure expansion, wrapped diagnostics, 390px/200% text fit, and reachable save actions. Component tests cover custom/unset choices, disabled controls, retained disclosure drafts, and avatar image failure/recovery. No coverage was deleted or moved.

Based on main (20d7447), preserving its global search palette. No backend, dependency, or native implementation changes are included.

Deferred: updated hosted CI, native packaged-app acceptance, and unrelated browser suites. The gallery does not exercise live relay writes, native window dragging, or video playback. Reviewer/code-owner approval is required before merge.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Verdict

No actionable code-review blockers found in this dialog-polish change. This is a review comment, not an approval or a claim that all merge gates are green.

Reviewed head 37024747113d076e69bf2fe5a95fda596833c6ef against base 298a50a9861c19d33e45b2b52bdadb81b128f15d.

Scope and evidence

Reviewed the shared Select/Combobox/Accordion migration, agent create/edit flows, custom/unset values, disabled states, disclosure draft retention, model-discovery cancellation and stale-result fences, save/revision guards, recovery Stop and dismissal paths, community/profile integration, search row composition, and gallery/test changes. Shared presentation remains separate from native writes and feature lifecycle ownership.

Reused hosted CI run 35766338587 instead of duplicating its broad suites locally. Its synthetic merge b7ef2e990175ce6cbf19913a31392cf3f3cb24de has the same Git tree as the reviewed head. All four Chromium/WebKit journey shards and browser measurements passed; the changed agent-control, editor-grid, and model-discovery cases are present and passing in both engines. The changed/new component suites also passed, as did lint/types, frontend build, Rust/tool integration, Semgrep, zizmor, and DCO.

Separate CI gate and limits

Required CI is still red. The JavaScript run has 2,123 passing tests and one 15-second timeout at src/features/relay/read-state.test.ts:366: “keeps ordinary reads publishing across growth, old-history reads and restart.” That path is outside this diff, but this review does not establish that the failure is unrelated or flaky. Resolve or attribute that failure before treating the PR as merge-ready; it is not a substantiated dialog-code finding.

No new local tests, live provider/relay writes, or packaged-native acceptance were performed for this review. Windows native validation was skipped. Committed screenshots cover light/default states, not every theme, disclosure, or assistive-technology interaction.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Verdict

No blocking findings. One non-blocking motion-state defect is noted below. This is a review comment, not an approval or packaged-app acceptance.

Reviewed head cfd10c0222ca76f6b431b90e3a75cfa0d6301ccb against base 298a50a9861c19d33e45b2b52bdadb81b128f15d, concentrating on the nine-file increment since the previous reviewed head. The contract is shared dialog typography/spacing, round icon defaults, and Base UI-owned motion/presence without changing feature-owned form, save, recovery, or dismissal behavior.

Non-blocking: reset Escape-only motion state on reopen (P3)

In Dialog.tsx:37-50, instantClose survives a subsequent controlled open. Source-traced production sequence: open Communities, dismiss with Escape, reopen with the pointer, then choose a community. CommunitySwitcher.tsx:34-56 opens and selects through direct setOpen calls, so neither operation clears the wrapper's Escape latch. The later ordinary action close consequently receives data-motion="none" and skips its normal fast exit. This affects motion consistency, not the selection or saved data.

Clear the Escape-only latch when a new open cycle begins, preserving immediate Escape dismissal and caller ownership. A regression should hold transitions through Escape → pointer reopen → action close and assert the normal exit presence. The new viewer test's action close after Escape occurs only under reduced motion, which masks this sequence. This finding is based on the complete source path, not a browser reproduction.

Evidence and limits

Integrated independent interaction and composition source reviews, then rechecked shared Dialog state/CSS, keyboard and reduced-motion paths, preventClose, search's explicit motion="none", community handoff/close ownership, shared IconButton composition, and viewer/test changes. Agent create/edit and AlertDialog use their existing Base UI compositions without the new data-motion attribute; their save/recovery lifecycles were not reimplemented by this increment. The earlier agent-control migration review remains applicable.

Hosted CI run 35773871887 completed successfully for this head/base. Logs identify synthetic merge 4e2a78c99af625347e567bc6f8dc43843aaf52d9 as this exact head merged into the pinned base. JavaScript passed all 202 files / 2,124 tests, including the previously failing read-state test. All four Chromium/WebKit app-journey shards passed (246 cases per engine), including the affected agent flows; browser measurements and Rust/tool integration also passed. Windows native validation was skipped.

The new motion regression lives in the separate design:test:browser viewer suite, not the automatic app-journey workflow. The PR description reports that viewer suite passing in both engines; I did not independently execute or verify those local runs. This review was source-only on Blox, with existing hosted CI evidence. No PR checkout, builds/tests, CI reruns, live relay/provider writes, or packaged-native validation were performed. Committed screenshots predate the latest shared styling, as disclosed in the PR.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Verdict

No new blocking findings in the documentation-only increment at 6aabc4aec74e250eca4599884988db2104dbb0df, against unchanged base 298a50a9861c19d33e45b2b52bdadb81b128f15d. This is a review comment, not approval or native-app acceptance.

Compared with the previous reviewed head, this removes 12 stale screenshots, their index README, and the gallery README’s link to that index. No production code, executable tests, dependencies, or configuration changed. The previous review’s nonblocking P3 Escape-motion latch remains applicable; this cleanup does not fix it.

Princess Donut independently checked deletion/reference integrity and preservation of the 12-scene executable gallery; no surviving references to the deleted screenshot bundle were found in the pinned head. I verified the complete delta and unchanged executable source trees, reusing the previous source review rather than repeating unchanged runtime analysis.

Evidence and limits: source-only on the pinned Blox bare object store; no checkout, build/test execution, or CI reruns. At the recorded read, current-head CI run35792238314 was still in progress: JavaScript, Rust/tool integration, measurements, both Chromium shards and one WebKit shard had passed; the remaining WebKit shard was in progress and Windows native was skipped. That is not a claim of fully green current-head CI. Live/native acceptance remains unverified.