fix(react): Menu dismissal correctness and anchor box (D58) - #62
Merged
Conversation
Switching between two menus sharing one openId leaves both closed: when
the platform light-dismisses A, its popover is already closed by the time
A's open prop flips, so the sync effect arms no suppression and the
queued toggle reports onClose("outside") for a menu the consumer already
closed — clearing openId and killing B. Breaks the shipped row-actions
pattern.
Also: the trigger wrapper stretches as a grid/flex-column item so the
menu anchors to the cell; Placements cannot render four auto popovers at
once; FallbackPlacement never restores CSS.supports.
D57 is reserved for the theme console, so this takes D58.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six tasks: reproduce-then-fix the stale dismissal with a Playwright interaction spec, the trigger width fix, the Placements split, the FallbackPlacement cleanup, the polyfill comment plus changeset, then verification with the CI baseline-refresh loop spelled out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nu (D58)
Switching between two menus sharing one openId left both closed. When the
platform light-dismisses A because B's trigger was clicked, A's popover is
already closed by the time A's open prop flips, so the sync effect arms no
suppression and A's queued toggle reported onClose("outside") — clearing
openId and killing B. Breaks the shipped row-actions pattern.
Guards the report on a latest-value openRef. Adds a screenshot-free
Playwright interaction spec: this class is unreachable from jsdom, whose
polyfill has neither light dismiss nor mutual dismissal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… (D58) Review pass on Task 1. Four findings: 1. The guard mirrored `open` into a ref written by a passive effect, which React schedules after commit — leaving a window where committed props say `open: false` while the ref still reads `true`. React looks the onToggle handler up from committed props at dispatch time, so reading `open` from the closure can never be staler than the ref. Drop the ref and its effect; read `open` directly and add it to the useCallback deps. 2. The Playwright regression assertion (`last-reason` is "none") passed on its first poll against the initial value, so post-fix it could pass before the stale toggle arrived at all. Outlast the stale window first, then re-assert that B is still open — B closing milliseconds after opening is the actual user-visible symptom and nothing pinned it. 3. Add the jsdom counterpart: the ordering is fabricable with a bare `toggle` after the consumer's state has caught up, and `pnpm test` is the inner loop. Verified red against an unguarded handleToggle. 4. Both doc comments claimed light dismiss fires "on pointerdown"; per the spec's algorithm pointerdown only records the target. Say what was actually observed instead — the platform dismisses before the consumer's click handler runs — and regenerate the tracked Menu.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The spec mandated a useRef mirroring `open`, written in a passive effect. React resolves onToggle from props stamped during the mutation phase of commit, so a passive effect can lag them — leaving a window where the guard reads a stale `true`. Reading `open` from the closure has no such window and is eight lines shorter. The rejected-alternatives entry is reversed in place rather than deleted: the original rationale was backwards and is easy to re-derive wrongly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The wrapper is the anchor box for both placement paths, so as a grid or flex-column item it stretched and the menu anchored to the cell edge instead of the trigger. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
popover=auto mutually dismisses, so the combined Placements story could only ever render one of its four menus. Splitting keeps all four placements under VR, which D53 requires. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…(D58) It overwrote CSS.supports without restoring it and injected [data-psi-menu] CSS document-wide, so every later Menu in the session -- including the autodocs page -- took the JS fallback branch while the browser still applied position-area. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixes three findings on the FallbackPlacement story's decorator: the "child effects flush before parent" comment was backwards (useMenuPlacement uses a passive effect, so a decorator layout effect would run first regardless) — replaced with the real reason for installing during render. Also makes the CSS.supports restore identity-checked instead of unconditional, and swaps the psi-fallback-probe class (which squats the shipped psi- namespace) for a data attribute. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The plan justified FallbackPlacement's render-phase CSS.supports stub by claiming child effects flush before parent effects. That is wrong — useMenuPlacement uses a passive effect, and layout effects always flush before passive ones, so a decorator useLayoutEffect would have won. The render-phase install still ships, for a stronger reason. Also records two carries D58 does not close: the stub is still realm-wide while the story is mounted, and pnpm build never typechecks story files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
vitest.setup.ts claimed light dismiss was exercised in Playwright VR. VR only screenshots -- it was exercised nowhere, which is why D53 shipped the bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Enforce lockstep versioning (fixed changeset group across psi-tokens/psi-react/psi-mcp), retract the "pointerdown" claim from the D58 spec and plan in favor of the observation-backed "before the consumer's click handler runs" phrasing, note that the interaction spec's "none" assertion is stricter than the shipped contract, correct the plan's VR baseline-refresh step (orphaned Placements baselines must be deleted by hand; expected delta is 2 deleted/10 added/0 modified), and add two comment clauses recording test/decorator scope that would otherwise be easy to over-read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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 23:08
Placements replaced by four per-placement stories, SwitchingBetweenMenus added. Baselines are the CI (Linux) render, per apps/storybook/vr/README.md. Delta is exactly 10 added / 2 deleted / ZERO modified — no existing baseline moved, confirming the .trigger width fix was a rendered no-op in every story that already existed. CI reported 10 failed, 181 passed, all ten failures "snapshot doesn't exist". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Switching between two menus sharing one
openId— the shape of the shippedrow-actionspattern — left both closed. The user clicks B and nothing opens.When the platform light-dismisses A because B's trigger was clicked, A's popover is
already closed by the time A's
openprop flips, so the sync effect arms nosuppression and A's queued
togglereportsonClose("outside")— clearingopenIdand killing B.
suppressNextCloseRefcannot cover this: it is armed only where Menudrives the close, and here the platform did.
onClosenow reports only for a menu still open by its ownopenprop,read from the closure (
useCallbackdeps[onClose, open]). No API change.flex-column item, so the menu anchored to the cell edge. Both placement paths read
that wrapper, so one line fixes both. Measured: 428px → 109.23px, matching its button.
Placementscould only ever render one of its four menus(
popover="auto"mutually dismisses) — split into four.FallbackPlacementoverwrote
CSS.supportswithout restoring it and injected document-wide CSS,poisoning every later Menu in the session including autodocs.
unreachable from jsdom, whose polyfill has neither light dismiss nor mutual
dismissal — the gap that let D53 ship this.
vitest.setup.ts's comment claimedPlaywright VR covered it; VR only screenshots, so it was covered nowhere.
.changeset/config.jsongainsfixedfor the three packages. Withoutit
changeset versionwould have shipped react 0.8.1 with tokens/mcp stranded at0.8.0 — the first divergence ever, against CLAUDE.md's lockstep rule.
VR baselines will churn — that is the acceptance signal
Expected: 2 deleted (
components-menu--placements--*), 10 added (fourplacement stories +
SwitchingBetweenMenus, light + ember), and zero modified.Storybook's global decorator already rendered
.triggershrink-to-fit in existingstories (
menu--defaultwrapper 73.59px = button 73.59px), so the width fix shouldmove no existing pixels. A modified Menu baseline, or any diff outside Menu
stories, is a tripwire to investigate.
Spec:
docs/superpowers/specs/2026-07-31-menu-dismissal-correctness-design.mdPlan:
docs/superpowers/plans/2026-07-31-menu-dismissal-correctness.md🤖 Generated with Claude Code