feat(react): D53 Menu — the overlay tier (HAN-44) - #56
Merged
handamade merged 23 commits intoJul 31, 2026
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
handamade
enabled auto-merge (squash)
July 31, 2026 16:09
…lan (D53) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Container only per Task 3 of the D53 Menu cycle: MenuItem/MenuSeparator land in Task 4, keyboard nav in Task 5, CSS anchor placement in Task 6. Same posture as Dialog (D50) — controlled-only, platform owns the top layer and Esc, zero dependencies. Fixes a double-dismissal risk in the brief's reference implementation: calling onClose directly from the Esc/item-select handlers, combined with hidePopover() also dispatching `toggle`, let a single dismissal reach the consumer twice once the controlled `open` prop round-tripped back through the sync effect. Esc and item-select now only attribute a reason and call hidePopover() themselves; the `toggle` that raises is the sole place onClose is invoked, so the later effect observes the popover already closed and no-ops instead of re-dispatching. Also exercises the `toggle` contract Task 2's polyfill introduced (untested until now): opening's toggle is ignored, a directly-invoked hidePopover() (simulating a platform-driven close) reports "outside" exactly once.
…(D53)
Four review findings on the Task 3 Menu shell. Two design questions were
ruled on by the spec owner; both rulings override the task brief.
Ruling 1 — Menu is controlled-only in the same sense Dialog (D50) is: it
never changes its own visibility, it reports a reason and waits for the
consumer to flip `open`. Esc now calls preventDefault() (suppressing the
platform's native popover dismissal) and reports onClose("esc") without
hiding; MenuContext.close reports onClose("item-select") without hiding.
Light dismiss stays the one asymmetry, and it is platform-forced: the
browser hides the popover and the hide-side beforetoggle is not
cancelable, so the resulting toggle is the first Menu hears of it. That
asymmetry is now documented rather than implied.
With Esc and item-select no longer hiding, `toggle` can only mean an
outside dismissal, so reasonRef became dead state and is removed. A
consumer-driven close (`open` true -> false) still raises a toggle from
the sync effect's own hidePopover(); the effect sets a suppression flag
immediately before that call and the toggle handler consumes-and-clears
it, so a programmatic close no longer invents an onClose("outside"). The
flag is checked before the open/closed test so a close-then-reopen inside
one frame cannot leave it set to swallow the next real dismissal.
Ruling 2 — aria-haspopup/aria-expanded move off the wrapper div, which
has no role and so never associates them with the focusable control, onto
the trigger element itself via cloneElement. The wrapper stays: Task 6
needs it as the anchor box for CSS anchor positioning. `trigger` is
narrowed to ReactElement, following Tooltip's precedent, and guarded with
isValidElement at runtime — a non-element trigger renders untouched with
one console.warn instead of throwing out of cloneElement.
Tests: 9 -> 14 in Menu.test.tsx. New coverage for MenuContext.close
(previously untested, and the whole point of Task 3 for Task 4), the
programmatic-close path, the flag lifecycle across a reopen, the wrapper
staying ARIA-free, and the non-element trigger fallback. Each fix was
mutation-tested to confirm its tests fail without it.
Monorepo suite 460 -> 465, lint and tsc clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…(D53) - The header no longer says Esc comes from the platform; Menu intercepts it with preventDefault(), which is what makes the D50 contract hold. - The cloneElement fallback comment claimed it guards Fragments; isValidElement returns true for a Fragment, so React's own dev warning covers that case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ocus (D53)
The keyboard hook returned focus to the trigger on every open true->false
transition, unguarded. On a light dismiss that is wrong: the browser hides
the popover and the user's click has already moved focus somewhere
deliberate, so the cleanup yanked focus off (say) a search input they had
just clicked, and their keystrokes went nowhere.
Guard the cleanup on focus still being inside the menu:
if (popoverRef.current?.contains(document.activeElement)) focusTrigger();
and delete the explicit focusTrigger() from the Esc branch. Esc only
*reports* the dismissal (D50) — the menu stays open and fully interactive
until the consumer flips `open`, and when it does, the cleanup runs and
restores focus. The explicit call was redundant on that path and actively
harmful when a consumer legitimately declines to close: focus landed
outside a still-open menu, arrow keys stopped reaching the handler, and a
second Escape went to the platform's close watcher — resurfacing as
onClose("outside"), the exact reason-misreporting this hook exists to
prevent.
The guard reads the DOM rather than a reason flag, so it stays correct for
every close route. It does not depend on React's effect ordering: Menu
declares its popover sync effect before calling this hook, so the cleanup
runs before hidePopover() and sees focus inside; were that reversed, the
platform's own hide-popover algorithm refocuses previouslyFocusedElement
(the trigger) and the guard simply declines. Either way focus lands on the
trigger.
Also in the hook: ArrowUp from the no-selection state (current === -1)
went to items[len - 2] via the modulo; `current <= 0` folds the two cases
that both mean "the last item".
Tests: three typeahead tests that did not discriminate are replaced —
typeahead now types "de" (Delete) where "d" alone already reached the
expected item, skip-disabled asserts the enabled item it should land on
instead of merely not the disabled one, and the 500ms reset window is
covered for the first time (fake timers, no real sleep). Adds ArrowUp over
a disabled item, ArrowUp from no selection, the declining-consumer Esc
case, and the light-dismiss case. Each was mutation-checked by reverting
its fix in isolation and confirming the test fails.
Task 3's 14 Menu.test.tsx contract tests are untouched and still pass;
suppressNextCloseRef is not affected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds useMenuPlacement.ts: above the anchor floor (CSS.supports check) it only declares anchor-name/position-anchor and CSS.supports-gated JS fallback sets inset from getBoundingClientRect() below it, staying current on scroll/resize. Wires the hook into Menu.tsx and appends the @supports (anchor-name: --x) CSS rules to menu.module.css. Also polyfills CSS.supports in vitest.setup.ts: jsdom's CSS global has no supports method, which the new hook calls unconditionally on every Menu render, so all Menu/MenuItem/useMenuKeyboard suites crashed without it. Defaults to true (matches Psi's above-floor default) and is overridden per test in useMenuPlacement.test.tsx to exercise both branches.
… (D53) Two review findings on Task 6's below-the-anchor-floor fallback: - RTL: the fallback wrote physical left/right regardless of document direction, while the CSS above the floor uses logical position-area spans that flip automatically. getBoundingClientRect() always returns physical coordinates, so the fix can't simply swap left/right for inset-inline-start/-end (that would double-flip against an already direction-resolved value). Instead, useMenuPlacement now reads getComputedStyle(trigger).direction and XORs it against the placement's "-start"/"-end" suffix to choose which physical edge to pin, keeping the two placement paths visually consistent across writing directions. Covered by 9 new tests (4 placements x ltr/rtl, plus one confirming direction resolves through ancestor inheritance rather than only an own dir attribute) and verified by a mutation check: reverting the direction logic breaks exactly the 5 RTL-facing assertions while the 4 LTR cases still pass. - Gap: the base .menu rule zeroed margin, so the fallback path sat flush against its trigger while the @supports(anchor-name) path kept its 4px breathing room. Unified both paths onto the same var(--psi-space-4) token on the base rule (the @supports block, left untouched, now just re-asserts the same value) rather than hardcoding a pixel offset into the JS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Registers Menu/MenuItem/MenuSeparator with the manifest, MCP server and generated docs — without this they exist but are invisible to agents. - packages/react/src/index.ts: export Menu, MenuContext, MenuItem, MenuSeparator, and their prop/Placement types - packages/react/src/Menu/slots.json: items slot accepts MenuItem/ MenuSeparator, cardinality 1..* - packages/react/src/contracts.json: Menu joins the interactive contract - packages/react/src/a11y-meta.ts: Menu/MenuItem/MenuSeparator entries corrected against the shipped D50 controlled-close contract (Esc and item-select only report onClose(reason); the popover stays open until the consumer flips `open`) rather than the brief's stale "Esc closes and returns focus" claim - packages/react/src/a11y.axe.test.tsx: axe cases for an open menu with a separator and a disabled item - packages/react/scripts/emit-manifest.ts: register the three new components; resolve MenuItem/MenuSeparator against their shared Menu/ folder instead of an own-name folder that doesn't exist; let MenuSeparator opt out of the "no props extracted" guard since it has zero props by design - packages/react/scripts/slots.test.ts: extend the real-src fixture list so contracts.json's now-larger interactive set doesn't trip its own unknown-component check - packages/mcp/src/store.ts: trim the search-overview item cap so three more (verbose) components don't push the ~6000-char response budget over, at the expense of a couple of (terse) topics - README.md, packages/react/README.md, packages/react/llms.txt, packages/mcp/README.md: component count 15 -> 18 - packages/react/docs/Menu.md, MenuItem.md, MenuSeparator.md: generated by emit-docs.ts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ordering (D53)
Two review fixes from Task 7, plus minors.
Menu's slot contract could not express Menu, and would have broken Task 8's
`row-actions` pattern two ways. scripts/patterns.ts treats every non-`body`
slot as a ReactNode *prop* — it skips `body` in the prop loop and takes an
element's JSX children from `body` alone — so the shipped `items` slot would
have emitted `<Menu items={<>...</>} />`: a prop Menu does not have, and no
children. And with no `trigger` slot declared, a compose tree could not express
Menu's required `trigger` prop at all (patterns.ts throws `unknown slot` for
any undeclared name except `body`).
Renamed the children slot to `body` and added a `trigger` slot bound to the
`interactive` contract, cardinality 1..1 to match the required prop. Order is
1/2, not the reviewer's 0/1: every authored slots.json is 1-based (Dialog,
Field, Panel, Toolbar). The `interactive` contract now naming `Menu` is a
harmless self-reference for validation — compose trees are finite JSON, so
`walk` cannot loop — but it is one notch over-permissive, since Menu cannot
honor being a trigger (it spreads props onto its popover div, not a focusable
node). Kept the contract anyway rather than an allowlist that goes stale on the
next interactive component; see the task report for the reasoning.
Proved end to end: a Task-8-shaped compose tree filling `trigger` and `body`
now validates and renders `<Menu placement="..." trigger={<Button .../>}>` with
MenuItem/MenuSeparator as children. Under the old contract the same tree
validated *silently* and emitted the invalid `items={...}` form. Locked in by a
new render-preset.test.ts case that reads the real src/Menu/slots.json, so a
rename back fails the test.
The MCP empty-query overview was capped at 25 items to fit the documented
6000-char response budget, but item count is the wrong axis. Briefs were
ordered components -> patterns -> topics, so topics were always the evicted
tail: at 25 items, eight topics were dropped including `getting-started`, which
carries the five-required-CSS-imports rule a consumer agent most needs on its
first call — while ~400 chars of budget went unspent. Registering Menu is what
first pushed it over, so this is a D53 regression, not a pre-existing wart.
Fixed on the right axis: order topics -> patterns -> components and fill to the
serialized-length budget instead of an item count. Topics are terse and
slow-growing; components are verbose, fast-growing, and the axis a keyword
query recovers best. The overview now carries all 12 topics and all 3 patterns
plus 15 components at 5866 chars, and stays byte-for-byte identical at 25, 40
and 80 components — only the component tail truncates, so it cannot silently
ratchet.
Minors: rename the public `Placement` export to `MenuPlacement` (Tooltip
already has a private, differently-valued `Placement`); export
`MenuContextValue`; document focus-on-open in a11y-meta; correct the typeahead
key range from "A-Z / 0-9" to any single printable key without modifiers; and
replace MenuSeparator's "decorative" with "non-interactive", since a separator
role is exposed to assistive tech.
pnpm build, pnpm test (59 files, 503 tests, +2), pnpm lint, and the docs-drift
check all green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fourth pattern (patterns.json 3 -> 4), gaps: []. Composes Menu's real trigger/body slot contract (not the stale items-slot shape from the task-8 brief): a Button trigger with visible text as its accessible name, sidestepping the known IconButton aria-label manifest gap from Task 7. Body holds edit/duplicate/separator/danger-delete MenuItems. Menu's own aria-label mirrors the trigger's label for the popover's accessible name, matching the precedent already used in Menu.test.tsx. delete-label is a concrete "Delete" rather than destructive-confirm's "<verb the object>" placeholder convention — that bracket text renders as literal JSX-breaking text when it lands in a body-slot position (verified by transpiling the rendered preset), so it's avoided here. Updates emit-patterns.test.ts and seed-patterns.test.ts pattern-count assertions (3 -> 4) and adds row-actions-specific preset assertions, following the same update filter-toolbar's landing made to these tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… (D53) The pattern advertised "kebab menu" and "overflow menu" but composes a text-labelled Button trigger, because IconButton cannot carry an accessible name through the manifest (docgen filters host props) and so is not composable in a pattern. An agent searching for a kebab menu would have got a recipe that is not one — the same docs-honesty failure HAN-41 was filed about. Match phrases now describe the delivered affordance, and the intent records why the icon-only variant is unavailable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nch (D53) Four stories, every one open:true so VR captures something. FallbackPlacement stubs CSS.supports so VR exercises the sub-anchor-floor JS placement branch instead of trusting whatever the CI browser supports. Baselines are collected separately, from the CI VR artifact — not generated locally (Linux-rendered).
All three packages move together (lockstep): psi-mcp bakes its search index at build time from react/dist/manifest.json and tokens/dist/*, so three new components change its published artifact even though it ships no new code.
check-docs-drift runs as its own CI step, not inside pnpm test, so Task 8's fourth pattern passed every local gate while leaving three agent-facing files claiming three. Exactly the drift the check exists to catch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tion (D53) The FallbackPlacement story did not simulate a sub-anchor-floor browser. Stubbing CSS.supports only convinces our own JS the feature is missing; the browser still evaluates @supports (anchor-name: --x) itself, and every browser Playwright runs supports it. So both positioning systems applied at once and fought — the captured baseline showed the menu offset ~38px right and ~66px down from where either path alone would put it, a state no real browser can be in. The decorator now also neutralises position-area and position-try-fallbacks, leaving the JS branch's inline inset as the only thing placing the popover. Commits the six baselines whose stories were already correct (Default, Placements, WithDisabledItem across light and ember). FallbackPlacement's two are deliberately absent — they will come from the next CI run against the corrected story. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous override had no effect — [data-psi-menu] is specificity (0,1,0) and the CSS module's .menu[data-placement="…"] is (0,2,0), so the anchor rules still applied and the captured baseline was unchanged. !important is what makes the JS branch's inline inset the sole positioner. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Restores Menu.stories.tsx to its Task 9 state. Two attempted CSS overrides were added on the theory that stubbing CSS.supports leaves the browser's own @supports (anchor-name: --x) block active, so both positioning systems would apply at once. Three CI runs produced a byte-identical fallback baseline (md5 ba165309a29bdf052464a04e88def956) across no override, a plain override, and an !important override — which disproves that theory. The overrides changed nothing and their comments claimed a mechanism they do not have. The six verified baselines (Default, Placements, WithDisabledItem across light and ember) stay committed. FallbackPlacement's two remain uncommitted: the JS branch renders the popover ~38px right and ~66px below the trigger rather than flush beneath it, and that is unexplained. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… browser (D53) Verified in a browser rather than guessed. Stubbing CSS.supports only tells our JS the feature is missing — the browser still evaluates @supports (anchor-name: --x) and keeps applying position-area, which overrides the JS branch's inline top/left. Measured: inline style read 'top: 64px; left: 32px' (correct — the trigger's bottom-left) while the element's actual rect was (4, 4), the viewport corner. Two details made the earlier attempts no-ops: the override needs !important to beat the CSS module's .menu[data-placement="..."] at (0,2,0), and the reset value must be 'none' — position-area's initial value is none, so 'normal' is invalid and dropped silently even with !important. With 'position-area: none !important' the popover lands at (36, 68) = trigger bottom-left + the 4px margin, exactly what the fallback computes. The JS fallback math was correct throughout; only the story was wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Captured against the corrected story. Both are byte-identical to their Default counterparts, which is the right result: for bottom-start with no collision, the JS fallback places the popover exactly where CSS anchor positioning does. That equivalence is now pinned by VR — a regression in either path shows up as a diff. All 8 Menu baselines committed; VR should be green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
handamade
force-pushed
the
dkurkin/han-44-08-menu-component-overlay-tier-d53
branch
from
July 31, 2026 16:24
e3024e9 to
ff602b1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements D53 from the 9-task plan. Closes HAN-44.
Menu+MenuItem+MenuSeparatoron the native Popover API. Zero new runtime dependencies —psi-reactstill has nodependenciesfield at all.The acceptance test passes
row-actionstakes the pattern index to 4, allgaps: []— the same machine-checkable exit conditionfilter-toolbargave Toolbar in the 0.7 cycle.Two spec-owner rulings changed the design mid-flight
1. Restored Dialog's D50 controlled contract. The first implementation had Menu closing itself on Esc and item-select. Review established that only light dismiss is platform-forced (the hide-side
beforetoggleis not cancelable); Esc is already intercepted by Menu's ownpreventDefault(), and item-select is entirely Psi's path. So Esc and item-select now only report — the popover stays open until the consumer flipsopen, and a consumer may legitimately decline. A consumer-driven close fires noonCloseat all.2. ARIA moved onto the trigger element via
cloneElement, rather than a role-less wrapper where assistive tech would never associate it with the focusable control.Defects review caught that would have shipped
items;patterns.tstreats every non-bodyslot as a prop, so a pattern would have emitted invalid<Menu items={…} />. It also had notriggerslot, thoughtriggeris required. Both fixed, guarded by a test reading the realslots.json.popover.contains(document.activeElement).left/rightwhile the CSS above the anchor floor uses logicalspan-inline-*. Every browser running the JS branch is inside Psi's documented floor.getting-started. Now fills by serialized length in kind order, so only the component tail truncates — byte-identical at 25, 40 and 80 components.Browser floor is unchanged
Chrome/Edge 119+, Safari 18+, Firefox 128+. The Popover API is under it. CSS anchor positioning is Baseline 2026 (Chrome 125+ / Firefox 132+ / Safari 18.2+) and
@position-tryneeds Firefox 147+ / Safari 26+ — both above the floor, which is why aCSS.supports-gated JS branch exists. No collision flip below the floor; that is the documented degradation.Gates
pnpm build(WCAG AA contrast ×4 themes + D46 scopes + D48 validator) ·pnpm test503 across 59 files ·pnpm lintclean · docs-drift green at 18 components / 4 patterns.VR will fail on this push by design — four new Menu stories have no committed baselines yet. Baselines follow from the CI artifact.
Follow-ups filed, not fixed here
Angle-bracket content placeholders (
<verb the object>) render as invalid JSX in text-child positions — affectsdestructive-confirm,filter-toolbarandsettings-form-row, pre-existing. AndIconButton'saria-labelis not a manifest prop, so an icon-only kebab trigger is not composable in a pattern;row-actionsuses a text Button and says so.🤖 Generated with Claude Code