[menu] Fix detached-trigger store sync and context-menu navigation#5026
Open
atomiks wants to merge 1 commit into
Open
[menu] Fix detached-trigger store sync and context-menu navigation#5026atomiks wants to merge 1 commit into
atomiks wants to merge 1 commit into
Conversation
atomiks
added
component: menu
commit: |
Bundle size
PerformanceTotal duration: 1,459.77 ms +5.53 ms(+0.4%) | Renders: 50 (+0) | Paint: 2,200.60 ms +16.22 ms(+0.7%)
10 tests within noise — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
atomiks
force-pushed
the
claude/menu-detached-trigger-context-menu-fixes
branch
from
75cc9f4 to
286862a
Compare
Bugs: MenuStore's parent observer no longer detaches itself on the first parent change (the synchronous observe-fire was clobbering the per-parent subscription); the hover-open mouseup listener is now cleaned up; the touch-to-close guard runs before onOpenChange/dispatchOpenChange so controlled consumers aren't force-closed; ArrowLeft no longer closes a root context menu (it's not treated as nested). Cleanup: remove the never-constructed nested-context-menu parent variant; drop the no-op render-time 'delete rootTriggerProps.id'; use the deduping warn util for the modal-on-child-menu warning (was console.warn every render) with corrected wording; use REASONS.triggerHover instead of the string literal; comment fix for stickIfOpen. Keep the duplicated trigger drag-release bounds check inline for now (shared helper is a follow-up).
atomiks
force-pushed
the
claude/menu-detached-trigger-context-menu-fixes
branch
from
286862a to
a98297b
Compare
atomiks
requested review from
aarongarciah,
colmtuite,
flaviendelangle and
michaldudak
as code owners
There was a problem hiding this comment.
Pull request overview
This PR addresses correctness issues in the Menu store lifecycle (especially around detached triggers), improves cleanup of hover-open event listeners, and adjusts keyboard navigation behavior for root context menus. It also removes an unused MenuParent variant and reduces dev-mode warning spam.
Changes:
- Fix
MenuStoreparent-state propagation and ref borrowing so parent subscriptions aren’t clobbered and root menus restore their ownallowMouseUpTriggerRef. - Ensure hover-open
mouseuplisteners are cleaned up, and move the touch-to-close guard to run before notifying controlled consumers / dispatching open changes. - Adjust list-navigation nesting logic so root context menus aren’t treated as nested (e.g. ArrowLeft no longer closes them), and remove the unused
nested-context-menuvariant.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/menu/trigger/MenuTrigger.tsx | Uses addEventListener cleanup to prevent stale mouseup listeners; aligns hover reason constant usage. |
| packages/react/src/menu/submenu-trigger/MenuSubmenuTrigger.tsx | Removes render-time mutation (delete rootTriggerProps.id). |
| packages/react/src/menu/store/MenuStore.ts | Fixes parent observation/subscription clobbering; restores root mouse-up gate ref when parent is removed. |
| packages/react/src/menu/root/MenuRoot.tsx | Deduped dev warning via warn; moves touch-close guard earlier; adjusts useListNavigation nesting for root context menus; removes unused parent variant. |
| packages/react/src/menu/positioner/MenuPositioner.tsx | Removes now-nonexistent nested-context-menu branch. |
| docs/src/app/(docs)/react/components/menu/types.md | Updates docs to remove the nested-context-menu MenuParent variant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
186
to
196
| public static useStore<Payload>( | ||
| externalStore: MenuStore<Payload> | undefined, | ||
| initialState: Partial<State<Payload>>, | ||
| ) { | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| const internalStore = useRefWithInit(() => { | ||
| return new MenuStore<Payload>(initialState); | ||
| }).current; | ||
|
|
||
| return externalStore ?? internalStore; | ||
| } |
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
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.
Summary
Correctness fixes around the menu store lifecycle, detached triggers, and context menus, plus cleanup.
Bugs
MenuStoreparent observer no longer detaches itself / leaks its subscription.ReactStore.observefires its listener synchronously before returning, so the single unsubscriber field was clobbered at construction (leaking the per-parent store subscription) and a laterparentchange tore down the observer itself. The observer's own unsubscriber and the per-parent subscription now live in separate fields, and the root-level branch restores this menu's ownallowMouseUpTriggerRefrather than keeping a borrowed parent ref.mouseuplistener is now cleaned up. A hover-open that closed again without amouseupleft a{ once: true }document listener armed to fire on an unrelated later interaction.onOpenChange/dispatchOpenChange. Previously the guard bailed after already notifying controlled consumers (and floating-ui's own state), so a controlled menu closed anyway.nested, so the cross-orientation close key closed it; native context menus don't (there's no parent list to return to). Its submenus remain regularparent.type === 'menu'menus.Cleanup
nested-context-menuMenuParentvariant.delete rootTriggerProps.id.warnutil for the modal-on-child-menu warning (wasconsole.warnon every render) with corrected wording; useREASONS.triggerHoverinstead of the string literal; fix thestickIfOpencomment.The duplicated trigger drag-release bounds check is kept inline for now (a shared helper is a planned follow-up).
Typecheck, eslint, prettier, and the Menu / Menubar / ContextMenu suites pass. Based on current
master.