Polish composer controls, pickers, and attachment feedback - #218
Conversation
Consolidate popover visuals and Base UI behavior, align composer and picker spacing, unify capsule controls and search, and use shared native scrollbar styling. Include component documentation, regression coverage, and five supplied visual references. This is a local checkpoint; review and broader validation remain deferred. Signed-off-by: Codex <codex@openai.com>
Signed-off-by: Codex <codex@openai.com>
Signed-off-by: Codex <codex@openai.com>
Signed-off-by: Codex <codex@openai.com>
Signed-off-by: Codex <codex@openai.com>
Signed-off-by: Codex <codex@openai.com>
Signed-off-by: Codex <codex@openai.com>
|
🤖 @wesbillman — could you or Carl review this composer polish PR? The conflicts are resolved; local validation passed with 927 tests and 54 browser checks. |
Signed-off-by: Codex <codex@openai.com>
Signed-off-by: Codex <codex@openai.com>
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Codex <codex@openai.com>
Signed-off-by: Codex <codex@openai.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Source review clear of merge-blocking findings at 3eb3fcda5345d5c52c809cf37adac6fe3b774417 against 2983b2f4b62673733419c6c52ed0e4dfbcc153d1. One non-blocking, narrow toast-lifecycle note is inline. Picker/completion focus ownership, attachment cancellation and final-item removal cleanup were traced across their shared callers; no broader correctness regression was established.
Required CI remains red; this is not merge-ready certification. Run 36082414877 tested synthetic merge 5d38253e1dc7b6893fd1aad95ae019a769878d1c of these exact pins. All four Chromium/WebKit journey shards and all 3,533 Vitest tests passed. Browser measurements failed the 200ms warm-channel-switch ceiling (221.6/211.1/207.3ms), with two later measurement cases not run. The available evidence does not isolate a PR-caused slowdown; resolve that required gate without relaxing the budget before merge.
Source-only Blox review plus existing hosted CI; no PR code executed or CI rerun by this review. Hosted evidence does not establish the full design-viewer matrix or smoke/audio/reduced-motion browser acceptance. Windows native validation was skipped. This is a COMMENT, not approval.
| if (editingDisabled || !files.length) return; | ||
| if (editing.target) { | ||
| setError("Finish editing before attaching new files."); | ||
| setAttachmentError("Finish editing before attaching new files."); |
There was a problem hiding this comment.
[P3] Retire the edit-only attachment instruction when editing ends
Non-blocking: start editing a message, paste a file, then cancel/close the edit. attachFiles() sets “Finish editing before attaching new files,” but the edit-restore callback still clears only error, leaving this zero-timeout toast visible after the restriction has gone. The composer remains mounted for the same channel/thread, so the instruction persists until dismissal or another successful attachment. Consider clearing this edit-specific notice on edit restoration, with a regression test for paste → cancel. This does not require clearing unrelated upload/selection notices on every successful send.
Signed-off-by: Codex <codex@openai.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No new merge-blocking findings at 5db8ac282d09d4493b5b9ea776c51ad008f0eeb8 against source-review base 2983b2f4b62673733419c6c52ed0e4dfbcc153d1. This re-review covers the three-file delta from the prior review: lazy media preparation avoids empty-composer work while preserving immediate attachment cancellation, final-removal feedback, reuse and unmount cleanup. The earlier non-blocking toast note remains unchanged.
Hosted CI 36122619477 is green, including all four Chromium/WebKit functional shards and the previously failing Chromium warm-switch ceiling. Observed warm switches were 105.6–114.3ms against 200ms. Attribution caveat: CI tested synthetic merge 4fbe90da106f2f754c7a63425117813f76ccba9e, combining this head with newer main df7b7e7f45739f3e06e12d81623385701acdc51d, not the pinned source-review base. These samples are not an isolated before/after measurement of this patch.
Source-only Blox review; no PR code/tests executed or CI rerun here. Windows native validation and the WebKit warm-switch measurement were skipped; native and full design-viewer acceptance remain unverified. The new component test does not cover StrictMode or injected audio failures; those paths were source-traced without a concrete defect. This is a COMMENT, not approval.





Composer controls and pickers now use consistent shared surfaces and sizing, with compact attachments that leave more room for drafting.
Validation
Conflict-resolution snapshot: af4456f, integrating main at 9891975. The later profile-only main commit 254baae also merges cleanly.
Review fixes and test coverage
Retained main’s consolidated popover, search-field, sizing, and hover contracts while preserving composer anchoring and attachment feedback. Combined distinct tests from both branches. The combined browser journey exposed focus theft when switching directly from mentions to emoji; using Base UI’s default focus restoration fixed it in both engines without weakening the assertion. Removed duplicate icon exports, popover registry/specimen entries, and option-row CSS introduced by automatic merging.
Previously reconciled audio cleanup stubs and a stale region selector with the new dialog semantics. Reused byte-identical existing assets instead of shipping duplicates. Updated neutral chip expectations and added sizing assertions without removing coverage.
No browser cases were removed or moved. Existing journeys exercise geometry, native editing/focus, and clipboard-to-toast wiring; the added composer viewer case checks actual responsive layout and focus. Lifecycle/failure checks use colocated component tests. The stale chip expectation failed in both engines before correction; the broader push gate exposed the audio-stub and role failures before their fixes.
Remaining checks
Required hosted CI, DCO and human/code-owner review must complete before merge. Native acceptance and the entire browser/viewer matrix were not run locally. The pre-existing product playground name-service fixture error remains outside this change; previous passing playground checks do not validate its inline completion.
Screenshots
Seven current screenshots of actual components with fixture data are saved locally and available for manual attachment. They are not uploaded as part of this PR.
Current CI validation
Head 5db8ac2 fixes a measured channel-switch regression caused by preparing attachment-removal images and audio whenever an empty composer mounted. Media now prepares once when attachments first appear and remains owned by the mounted list through final removal. Playback and timers still clean up on unmount. The lifecycle regression fails against the previous eager implementation and passes with the fix.
The branch also integrates main 2983b2f and retains its maintained Settings scroll synchronization unchanged. The original WebKit failure is resolved: all four hosted functional shards passed at 3eb3fcd, as did JavaScript and Rust. That head exposed the performance failure fixed above.
Hosted CI passed at 5db8ac2: JavaScript, Rust/tool integration, browser measurements, all four Chromium/WebKit functional shards, and CI required. DCO, Semgrep, and zizmor also passed. The hosted channel-switch samples are 105.6–114.3 ms against the unchanged 200 ms ceiling. The full hosted browser run covers the image-conversion fixture that timed out locally. Windows native validation was not run by this automatic Linux workflow. Human/code-owner approval and native acceptance remain outstanding.