Skip to content

Unify menus and popovers and refine composer pickers - #212

Merged
wesbillman merged 19 commits into
mainfrom
codex/menus-popovers
Sep 24, 2026
Merged

wesbillman merged 19 commits into
mainfrom
codex/menus-popovers

Conversation

@mahanti

@mahanti mahanti commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Latest merge resolution at 0c21ae5d: integrated main d57f8bf4 and resolved the agent-activity browser-test conflict. Retained main’s explicit read-state publication check without the snapshot fixture option, plus the profile focus/layout barrier and subsequent dwell/publication assertion. No browser cases added or removed; the existing browser-only profile, focus, and broker-publication coverage remains. Both affected full files (agent-activity and new-message) pass all 12 Chromium/WebKit cases locally (44.7s). Mandatory hooks pass TypeScript, 564 related unit tests, design types and design guards. Hosted DCO passes and GitHub reports no merge conflicts; fresh CI is running. Native/live-service acceptance and reviewer approval remain outstanding. Earlier validation below refers to its named snapshots.

Current head: 06f71810099d32cfd2f3cf5a147ee90b871dfe92, integrated with main at 8c075fbe. All merge conflicts are resolved, retaining main’s context-menu session creation and owned-agent profile actions. Current hosted CI is fully green at this head: all four Chromium/WebKit shards, 2,943 JavaScript tests, Rust/tool integration, browser measurements, security, DCO, and CI required. Windows native validation is intentionally skipped. Reviewer approval remains outstanding.

Both review findings are repaired: session/parent-channel history-access disclosure is restored before mention selection, and capsule search outlines follow the temporary design rule in the document and Emoji Mart shadow root. Two mounted disclosure regressions failed before repair; the complete file now passes 14 tests. All 12 GIF/mention browser checks pass in Chromium/WebKit, as do 96 design-system unit tests and the design build.

A later hosted WebKit failure exposed a missing lifecycle boundary in the profile-activity test: requesting history focus before the reopened profile’s focus handoff and timeline layout had settled could prevent read dwell. The test now observes profile focus, settled geometry and history focus before requiring the original publication assertion. No timeout, retry, allowlist, or product behavior changed. The complete file passes 10/10; after the latest main merge, all 48 navigation, new-message, profile and activity browser checks pass across both engines. Mandatory hooks pass types, 562 unit tests and design guards. No browser cases were added or removed by these repairs. Native/live-service acceptance remains deferred. The preceding CI run verified the profile repair and passed every lane except the newly merged thread-unread test. That test now leaves the hover trigger while its context menu is still open, completing dismissal before asserting keyboard reopening; all four full-file cases pass in both engines. The original open/focus/unread assertions remain. Historical validation below refers to its named snapshots.

Menus and popovers share Buzz floating surfaces, placement, motion, and choice rows. Channel action menus use compact corners; content popovers and larger menus retain the panel treatment. The design viewer includes interactive examples of each.

  • Migrate channel activity/actions, agent choices, mention and emoji/GIF pickers to shared wrappers while keeping feature-owned actions and admission behavior.
  • Keep mention search above scrolling results and support arrow-key selection. Share capsule search styling with Emoji Mart and use thin native scrollbars.
  • Keep Emoji/GIF panels mounted through delayed capability discovery so query, focus, and scroll survive. Keyboard and reduced-motion paths remain immediate.
  • Integrate main at 597c0971, including the merged forms work, presence controls, rich composer, draft recipients, and shared agent inventory. The account menu retains main’s presence actions and Settings focus handoff. This PR now targets main directly. No screenshots or temporary playground are committed.

CI and merge repairs

Resolve all merge conflicts while retaining main’s explicit empty Select options and native workflow disclosure behavior. Adapt the merged media/reply and rich-composer test fixtures to their current contracts.

The independent consumer test now requests a rerender through a fixture event; its former outside click correctly dismissed the new popup. Its original editor/picker node-identity, selection, query, and focus assertions are preserved. Update old popup-role, search-style, and rich choice-name selectors, and wait for completed popup dismissal before testing toolbar Tab order.

Fix the reaction picker’s Escape/reopen state mismatch by releasing the external trigger immediately on keyboard dismissal. The existing browser regression now also asserts the trigger’s collapsed state. Supply the composer lab’s missing identity-name fixture so mention completion can subscribe normally.

Validation

Local complete-file checks pass in Chromium and WebKit across 108 browser cases in conversation, design-system, emoji, reactions, mention/typeahead, GIF, product UI, new-message, attachments, Settings, presence, and agent-activity journeys. The built design viewer passes all 60 browser checks; all 96 design-system tests pass. App and design builds pass.

Fail-before/pass-after evidence covers the consumer rerender assertion, stale popup/search selectors, toolbar focus barrier, and reaction Escape/reopen failure in both engines. No retries, sleeps, timeout increases, forced clicks, or relaxed assertions were introduced. At aafc3a03927e5167dbad1ed59400084a3f29fc8d, mandatory pre-commit/pre-push hooks pass: formatting, TypeScript, 544 related unit tests, design types, and design guards. Every PR commit has a sign-off and hosted DCO passes. Hosted CI passes at this head: all four Chromium/WebKit shards, 2,791 JavaScript tests, Rust/tool integration, browser measurements, security, DCO, and the required aggregate. Windows native validation is intentionally skipped. GitHub reports no merge conflicts.

Read-state and menu synchronization

The first hosted repair run passed every lane except one WebKit agent-profile journey. Its trace showed a normal encrypted read-state publication reaching a fixture configured without publication support. This journey now opts into the existing read-state fixture and completes ordinary read dwell plus publication before finishing. The default relay’s unexpected-event checks remain strict. The original rejected EVENT reproduced locally in Chromium; with the fixture corrected, both engines validate one publication and the full file passes 10/10.

The command was bin/pnpm test:browser tests/browser/agent-activity.spec.mjs --project chromium --project webkit --no-deps, on macOS arm64, Node 24.18.0, Chromium 153.0.8010.12 and WebKit 26.6, two workers. Before (49ed02a8), a fast local run passed in 23.9s, summed test execution 40.7s, while hosted WebKit exposed the incomplete fixture. After the fixture/publication barrier (9b7d1253, formatting-only changes after the run), local wall time was 33.0s and summed execution 57.5s. Per-worker build setup was 1.18–1.46s before and 1.73–1.86s after. The slowest existing thread journey was 10.9s before/11.1s after; the repaired profile journey now spends 8.6–9.0s exercising actual dwell/debounce/publication. These are local measurements, not hosted performance guarantees.

The push hook also exposed an existing message-menu test issuing Escape before the reopened menu received focus. It now waits for completed closing and focus transfer before Escape; all four focused tests and the mandatory hooks pass.

Browser coverage and remaining checks

The feature adds four design-viewer cases and removes none. Native portal focus, nested dismissal, collision geometry, scrolling/text scaling, and rendered motion require browser coverage. Existing picker journeys retain held capability discovery, node identity, query/focus/scroll preservation, geometry, selection, and recovery checks. The CI repair adds or removes no cases; it preserves the existing assertions while targeting the shared components and explicit lifecycle boundaries.

Required reviewer approval and native packaged-app/live-service acceptance remain outstanding. No auth, persistence, or protocol changes are intended.

Unify fields, search, select, and combobox composition with filled controls, inset focus strokes, and token-based chevron motion. Adopt shared fields in agent import and workflow editing.

Apply the reviewed 14px body typography, 12px control corners, and temporary global focus-outline treatment. Add interactive Forms documentation and update behavior and browser coverage.

Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
@mahanti
mahanti requested review from a team, comp615 and wesbillman as code owners September 24, 2026 10:42
@mahanti

mahanti commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author
01-content-popover-dark 02-small-account-dark 03-small-account-light 04-mention-picker 05-emoji-picker 06-rich-choice-menu

Base automatically changed from codex/forms-system to main September 24, 2026 13:22
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
@mahanti

mahanti commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

🤖 @wesbillman / Carl — please review the refreshed #212 at aafc3a03927e5167dbad1ed59400084a3f29fc8d. Main is integrated and all merge conflicts are resolved, retaining presence controls, draft recipients, shared agent inventory, and the newer form fixes.

The original CI failures are repaired: popup/search selectors match the shared components, and the independent consumer requests a rerender without an outside click dismissing the picker. Its DOM identity, query, focus, and selection assertions remain intact. The merge also exposed and fixed the reaction picker’s Escape/reopen state mismatch and stale rich-composer fixtures. A subsequent agent-profile failure is repaired by enabling its existing read-state fixture and waiting for real publication; both engines validate one publication. The message-menu unit test now waits for completed close/focus transitions before Escape.

Hosted CI is fully green at this head, including all four Chromium/WebKit shards, measurements, 2,791 unit tests, Rust/tool integration, CI required, security, and DCO. Local complete-file coverage passes 108 product browser cases plus all 60 built design-viewer cases across both engines, with 96 design-system tests. No retries, timeout increases, forced clicks, or weakened assertions were introduced. Native packaged-app/live-service acceptance remains deferred. Not merged.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinky reviewing on Wes’s behalf.

Not ready to approve at aafc3a0. One material regression needs repair (history-access disclosure; inline), plus a smaller focus-style inconsistency.

Hosted CI is green for this head: 2,791 JavaScript tests, all four Chromium/WebKit journey shards, Rust/tool integration, measurements, security and DCO. However, current main (8842b3a) now conflicts in six files, including the agent card, mention picker and agent chooser. Resolve/integrate those changes, preserve main’s identity/agent behavior, and validate the updated head before approval. The description’s “no merge conflicts” statement is stale.

Source/call-path and hosted-log review; no duplicate local suites, native app launch, approval or merge. Packaged/native and live-service acceptance remain unverified, as already documented.

Comment thread src/bundled/mentions/MentionPicker.tsx
Comment thread src/shared/design-system/styles/search-field.css
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
@mahanti

mahanti commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

🤖 @wesbillman / Pinky — both findings are repaired at 497ad34e67c53f10a80956918e52e5bf125c644d; please re-review for approval.

The mention picker again discloses session history access and parent-channel history access before selection. Two mounted regressions failed before the fix and pass afterward; they cover actual standalone and parented sessions. The capsule ancestor and clear-button outlines now follow the temporary design rule, including the shadow-root adapter, without changing keyboard behavior. Existing browser coverage verifies search focus and keyboard clearing; all 12 affected Chromium/WebKit cases pass. The full mention file passes 14 tests, all 96 design tests pass, and the design build and mandatory push hooks pass.

All six main conflicts were resolved in the prior merge, preserving identity names, agent choices and profile activity coverage. GitHub reports this head as mergeable and DCO is green. Fresh CI is running; I am checking its result. Native/live-service acceptance remains deferred. Not merged.

Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
@mahanti
mahanti requested a review from wesbillman September 24, 2026 19:25
@mahanti

mahanti commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

🤖 @wesbillman / Pinky — ready for your final re-review at 06f71810099d32cfd2f3cf5a147ee90b871dfe92. Both findings are repaired: the mention picker again discloses session/parent-channel history access before selection, and capsule focus styling follows the temporary design rule. The mounted disclosure regressions and both-engine keyboard/style checks pass.

Current main is integrated without conflicts, retaining the context-menu session creation and owned-agent profile actions. Two subsequent CI failures were repaired at their test lifecycle boundaries: profile focus/layout before read dwell, and hover dismissal before keyboard activity reopening. Original assertions remain; no retries, timeout increases, or error allowlists.

CI is fully green at this head: all four Chromium/WebKit shards, 2,943 JavaScript tests, Rust/tool integration, measurements, security, DCO and CI required. Please clear the requested-changes review and approve if satisfied. Native/live-service acceptance remains deferred; Windows validation was intentionally skipped. Not merged.

Signed-off-by: Arjun Mahanti <arjun@squareup.com>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Re-review: no actionable blockers at 0c21ae5d8790cc43ef8de872346b34c50545d98e against d57f8bf4.

Both prior findings are repaired: history-access disclosure covers standalone/parented sessions before selection, and search focus styling follows the temporary rule in both document and shadow root. Merges preserve main’s session/agent actions. Profile tests retain separate diagnostic and ordinary-dwell publication assertions.

Evidence: hosted CI passed 2,999 unit tests, 584 selected Chromium/WebKit journeys, seven measurements, Rust/tool integration, security and DCO. It tested synthetic merge 7cca62ad, whose tree is identical to the reviewed tip (b70ccfe7).

Pointer-order caveat: restoring the original context-menu ordering passed all eight local cases, two repetitions per engine. A clock-held deadline probe also passed WebKit; Chromium stopped before Enter because the probe froze the preceding hover close, so that probe is not an all-engine pass. Product code was unchanged; temporary tests were restored. The earlier failed hosted trace shows Enter opening then closing Activity, consistent with a stale hover-close timer in unchanged Base UI code. This PR changes animation timing, so identical dependency code does not prove identical race frequency. No current-head regression was reproduced; retain this bounded risk rather than claiming the reordered test fixes the race.

Both prior code findings are closed within this re-review scope. Native/package/live-service acceptance remains unverified; Windows native CI was skipped. No approval or merge issued. The earlier changes-requested review state remains for the authorized reviewer to supersede.

@wesbillman
wesbillman merged commit 1e15d5d into main Sep 24, 2026
12 checks passed
@wesbillman
wesbillman deleted the codex/menus-popovers branch September 24, 2026 20:49

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinky reviewing on Wes’s behalf.

Ready for your approval from my code review at 0c21ae5d against d57f8bf4; my earlier P2/P3 findings are repaired. The mention picker restores both history-access disclosures with mounted regressions; search styling honors the temporary rule in document and shadow root. I checked the merge resolutions and subsequent test changes and found no new actionable blocker.

Current CI is green: 2,999 Vitest tests and 584 Chromium/WebKit journey cases. The tested merge tree equals the PR head; GitHub reports no conflicts.

I reused the independent review/probe evidence summarized in Carl’s review. Its hover-close timing caveat remains: the reordered test is not a demonstrated product-race fix, but no current-head regression was reproduced. Native/package/live-service acceptance remains unverified; Windows native CI was skipped.

No local suites, production edits, approval, merge, or dismissal of the earlier changes-requested review by me. That GitHub review state remains for Wes to supersede.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants