Make popovers dismiss consistently and badge the phone Decisions count #303
- MA
## Integration and validation
Use one shared dismissal hook for editor popovers and navigation menus, preserving IME handling and Escape-only focus restoration. Keep the current rich-text composer and compact navigation placement; picker boundaries use DOM nodes scoped to their own composer. Counts retain their motion flags.
Reviewed current language-menu positioning/presence and workspace geometry/motion producers before renewing their exact pins and removing only the obsolete count exception. Added a small package entry point so navigation does not load the full editor; the existing initial bundle budget passes unchanged.
Checks and types, 141 focused tests and all 50 relevant browser cases pass locally (48 in the combined run plus both corrected dismissal cases in a four-case mention rerun). New tests use the existing rich-text helper. Final main rebase and required CI will run before merge.
The picker screenshot in the original proposal shows the historical textarea; the current rich-text appearance was retained.
<details> <summary>Original proposal and screenshots</summary>
## Why
Popovers dismissed inconsistently. The "changes" chip closed only on mousedown and ignored Escape. The code language menu closed on Escape only when focus was inside its panel, and ignored focus moving away. The phone tab bar showed the Decisions count as a bare number while desktop uses the petrol count badge.
## What changed
- New `usePopoverDismissal` in `packages/editor/src/popover-dismissal.ts`, matching the semantics of #279's `useMenuDismissal` (which `packages/editor` cannot import): Escape (capture phase, stops propagation), outside pointerdown, and focus leaving. A pure `isOutside` helper is unit tested. - Changes chip and language menu use it. Escape returns focus to the trigger. - Composer pickers: the @ and # pickers already closed on Escape from the textarea. They now also close on an outside press or focus leaving, using the same hook. Escape keeps the draft and the composer's focus. - Phone tab bar: Decisions count uses the shared `Count` badge, as in the desktop view switcher.
## Screenshots
Before (phone tab bar):

After:

@ picker open (after, unchanged look; now also closes on outside press):

## Testing
- `bun test packages/editor apps/web`: 919 pass - `bun run types`: pass - `bun run ci`: dprint/oxlint/tokens pass; the design-contract check fails, see below - New e2e: language menu (Escape from trigger, focus leaving) in `e2e/code.e2e.ts`; @ picker (Escape and outside press keep the draft) in `e2e/chat-mentions.e2e.ts`. Not run locally; relying on CI.
## Needs a design-contract review
`bun run ci` reports reviewed dynamic owners changed in `apps/web/src/workspace.tsx` (lines 152, 162, 180, 189, 330, 517) and `packages/editor/src/widgets/language-menu.tsx` (lines 176, 181), plus a stale exception for `motionContract("feedback").className` in `workspace.tsx` (exceptions in `scripts/design-contract/exceptions/dynamic-web.json` and `dynamic-editor.json`). The changes are safe: line shifts, one removed feedback class use replaced by the shared `Count`, and no new style or class logic.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Follow-up: after #279 lands, migrate its apps/web useMenuDismissal to the packages/editor usePopoverDismissal hook (this one is the superset: capture phase, IME guard, reason argument).
</details>
- MA
**Review:** Changes needed (small)
Verified at a97c12f in a worktree build (language menu, phone tab bar). CI: only `format, lint, types, tests` fails, and only on the design-contract review hashes (`workspace.tsx` x6 + stale `motionContract("feedback")` exception, `language-menu.tsx` x2), which is acceptable. container and e2e pass, including the new mention-picker and language-menu tests. I could not drive the chat pickers or the changes chip live (my server has no Planner), so those rest on the passing e2e and code reading.
Observed working: language menu closes on Escape from the panel (focus returns to trigger), on outside click, on Shift+Tab and Tab (focus leaves, menu closes); Arrow and Enter selection still work. Decisions badge uses the shared `Count` and fits at 390 and 320 (20px pill, no clipping or wrap). The Chat unread badge was already `Count` on main, so the two tabs now match.
Findings 1. `packages/editor/src/popover-dismissal.ts:30`: `event.isComposing || event.isComposing` is a duplicated condition. Presumably the second should be `event.keyCode === 229`. Without it, Safari's Escape that ends a composition (isComposing already false, keyCode 229) closes the picker and drops the IME commit. The old composer path in `chat.tsx` (`mentionKeyAction` / `referencePickerKeyAction`) handled this, and the capture-phase hook now pre-empts it. 2. `apps/web/src/chat/chat.tsx` (textarea `onKeyDown`, the `mentionAction === "dismiss"` and reference `action === "dismiss"` branches): now unreachable while a picker is open, because the hook stops propagation in capture. Either remove those branches (and the Escape cases in `mention-picker`/`reference-picker` key helpers) or leave a note. Two Escape paths with different IME rules is confusing. 3. Divergence from #279's `useMenuDismissal` (apps/web/src/menu-dismissal.ts), which will be confusing once both land: - #279 binds `pointerdown` and `focusin` in the bubble phase; this binds them in capture. Capture is more robust (it survives `stopPropagation` in the page), so prefer it in both. - #279 has no `isComposing` and no `defaultPrevented` guard; this has both. The `defaultPrevented` guard means an inner handler that already consumed Escape wins in #279's world but not here (capture runs first, so it is inert in practice). - #279 calls `close(restoreFocus: boolean)` and takes refs; this passes a `"escape" | "outside"` reason and takes a `regions()` callback. Same idea, two APIs. - Suggest landing one hook (this one is the superset, in `packages/editor` so apps/web can import it), then have #279's menus call it. At minimum align on the capture phase and the IME guard, and the reason argument. 4. Minor: outside `pointerdown` means a touch that starts a scroll outside the picker or chip also dismisses it. Consistent with #279 and the existing language menu, so fine, just noting.
No double-handling found: the capture Escape + `stopPropagation` prevents the editor Escape and the textarea handler while open, and nothing runs when closed (effect is gated on `open`). The selection toolbar and sheet Escape are not reached while a popover is open, which is the intended nesting.
- MA
Addressed: IME guard now checks keyCode 229; removed the unreachable textarea dismiss branches (one Escape path); added the #279 migration note. Note: main's phone Chat count used the bare motion-feedback span, not Count, and e2e (responsive-activity) asserts its motion; both phone counts now use Count with the motion prop to keep that behavior. The earlier e2e failure was those assertions.
lavaman131referenced b687d5900a4elast month on GitHub
MaggieAppletonhead-ref-force-pushed 23f35910f17clast month on GitHub
MaggieAppletonhead-ref-force-pushed eaa53d15c9f9last month on GitHub
- MA
CI is still failing in `Format, lint, and enforce design tokens`. From the recorded run metadata plus the current diff, the likely root cause is stale design-contract review metadata around the new popover dismissal work: `scripts/design-contract/exceptions/dynamic-editor.json` for `packages/editor/src/widgets/language-menu.tsx`, `scripts/design-contract/exceptions/dynamic-web.json` for `apps/web/src/workspace.tsx`, and possibly removal of the now-stale `motionContract("feedback").className` exception if the `Count` migration eliminated that dynamic owner.
I did not push a speculative fix because this runner does not have Bun 1.4.2 available, and the normal Bun installer is blocked here, so I could not run the required reproduction/verification commands (`bun install --frozen-lockfile`, `bun run fix`, `bun run ci`).
Next action for a human: on a machine with Bun 1.4.2, run those commands on `eaa53d15c9f9c6400bc005756b4b289f3e7c8602`, renew or remove the flagged exception entries based on the actual design-contract output, and rerun CI.
`babysit-head:eaa53d15c9f9c6400bc005756b4b289f3e7c8602`
> [!WARNING] > <details> > <summary>Firewall blocked 1 domain</summary> > > The following domain was blocked by the firewall during workflow execution: > > - `releaseassets.githubusercontent.com` > > To allow these domains, add them to the `network.allowed` list in your workflow frontmatter: > > ```yaml > network: > allowed: > - defaults > - "releaseassets.githubusercontent.com" > ``` > > See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information. > > </details>
> Generated by [PR CI fixer](https://github.com/githubnext/chopin/actions/runs/37618976002) · gpt54 · 78.9 AIC · ⌖ 6.27 AIC · ⊞ 15.1K · [◷](https://github.com/search?q=repo%3Agithubnext%2Fchopin+%22gh-aw-workflow-call-id%3A+githubnext%2Fchopin%2Fpr-ci-fix%22&type=issues)
<!-- gh-aw-agentic-workflow: PR CI fixer, engine: codex, model: gpt-5.4, id: 37618976002, workflow_id: pr-ci-fix, run: https://github.com/githubnext/chopin/actions/runs/37618976002 --> <!-- gh-aw-workflow-call-id: githubnext/chopin/pr-ci-fix -->
MaggieAppletonhead-ref-force-pushed c9c564dfb4c0last month on GitHub
MaggieAppletonhead-ref-force-pushed e09fe46cfe67last month on GitHub
MaggieAppletonhead-ref-force-pushed 4959ed5054calast month on GitHub
- MA
CI babysitter rechecked the latest failed `ci` run for `4959ed5054cafbc8eedd4c7ba7f4f3221c6b7038`. The head still matches, `container` and `e2e` are green, and the first meaningful failure is still the `Format, lint, and enforce design tokens` step in the `format, lint, types, tests` job. From the current diff, that points at stale design-contract review metadata around the edited dynamic owners in `apps/web/src/workspace.tsx` and `packages/editor/src/widgets/language-menu.tsx`, plus the corresponding entries in `scripts/design-contract/exceptions/dynamic-web.json` and `scripts/design-contract/exceptions/dynamic-editor.json`.
I did not queue a commit from this runner because `bun` and `mise` are not installed here, so I could not run the required local reproduction and verification commands (`bun install --frozen-lockfile`, `bun run fix`, `bun run ci`). Next action for a human: on a machine with Bun 1.4.2 available, rerun those commands on this head, review the design-contract output for those files, renew or remove the exact exception entries that the current output flags, and rerun CI.
`babysit-head:4959ed5054cafbc8eedd4c7ba7f4f3221c6b7038`
> Generated by [PR CI fixer](https://github.com/githubnext/chopin/actions/runs/37627234570) · gpt54 · 42 AIC · ⌖ 4.92 AIC · ⊞ 15.1K · [◷](https://github.com/search?q=repo%3Agithubnext%2Fchopin+%22gh-aw-workflow-call-id%3A+githubnext%2Fchopin%2Fpr-ci-fix%22&type=issues)
<!-- gh-aw-agentic-workflow: PR CI fixer, engine: codex, model: gpt-5.4, id: 37627234570, workflow_id: pr-ci-fix, run: https://github.com/githubnext/chopin/actions/runs/37627234570 --> <!-- gh-aw-workflow-call-id: githubnext/chopin/pr-ci-fix -->
MaggieAppletonhead-ref-force-pushed e1ee6b8f9a7alast month on GitHub
MaggieAppletonreferenced this in Polish shell menus and Add project focus #337last month on GitHub
MaggieAppletonhead-ref-force-pushed d9b7d36de971last month on GitHub
MaggieAppletonhead-ref-force-pushed b8e445497bf5last month on GitHub
MaggieAppletonmerged 009aafc43988last month on GitHub
MaggieAppletonhead-ref-deletedlast month on GitHub