🔧 fix: Proper MCP Menu Dismissal#12256
Merged
Merged
Conversation
dustinhealy
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Ariakit menu/focus behavior for the MCP server selector UI to improve dismissal/focus handling, and adds a dedicated RTL/Jest test suite for the MCPSelect component.
Changes:
- Simplifies
MCPSelect/MCPSubMenumenu-store setup by removingfocusLoopand custom focus restoration logic. - Updates
MCPSelect’s menu to usemodalbehavior and wires menu item toggling directly totoggleServerSelection. - Adds
MCPSelect.spec.tsxcovering basic open/close and selection behaviors.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| client/src/components/Chat/Input/MCPSelect.tsx | Refactors menu store usage, adds modal menu behavior, simplifies toggle handler wiring. |
| client/src/components/Chat/Input/MCPSubMenu.tsx | Simplifies menu store options (removes focusLoop). |
| client/src/components/Chat/Input/tests/MCPSelect.spec.tsx | Adds initial component-level tests for MCPSelect menu behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
danny-avila
force-pushed
the
fix/mcp-selector-ux
branch
2 times, most recently
from
1343165 to
72ad387
Compare
Use Ariakit's `modal={true}` instead of a manual `requestAnimationFrame`
focus-restore wrapper, which eliminates the `useRef`/`useCallback`
overhead and lets Ariakit manage focus trapping natively. Also removes
the unused `focusLoop` option from both MCP menu stores and a narrating
comment in MCPSubMenu.
Cover button rendering, menu open/close via click and Escape, and server toggle keeping the menu open. Renders real MCPServerMenuItem and StackedMCPIcons components instead of re-implementing their logic in mocks.
Matches the pattern used by MCPSubMenu, BookmarkMenu, and other Ariakit menus in the codebase. Ensures the menu fully detaches from the DOM and accessibility tree when closed.
Ariakit's CompositeStore (which MenuStore extends) defaults focusLoop
to false. The previous commit incorrectly removed the explicit
focusLoop: true, which silently disabled Arrow-key wraparound
(mandatory per WAI-ARIA Menu pattern). modal={true} only traps Tab
focus — it does not enable Arrow-key looping.
- Add aria-modal regression guard so removing modal={true} fails a test
- Add guard branch tests: no MCP access, empty servers, unpinned+empty
- Fix TooltipAnchor mock to correctly spread array children
- Fix import ordering per project conventions
- Move component import to top with other imports
- Replace unasserted jest.fn() mocks with plain values
- Use mutable module-scoped vars for per-test mock overrides
danny-avila
force-pushed
the
fix/mcp-selector-ux
branch
from
72ad387 to
fe896d0
Compare
danny-avila
changed the title
Updated the opacity and pointer events logic in the FavoriteItem component to improve user interaction. The changes ensure that the component correctly manages pointer events based on the popover state, enhancing accessibility and usability.
Cover guard branch (empty servers), submenu open/close with real Ariakit components and real MCPServerMenuItem, toggle persistence, pin/unpin button behavior and aria-label states. Only context providers and cross-package UI are mocked.
ArrowDown from the last item must wrap to the first — this fails without focusLoop: true on the menu store, directly guarding the keyboard accessibility regression that was silently introduced.
jcbartle
pushed a commit
to jcbartle/LibreChat
that referenced
this pull request
May 11, 2026
* fix: replace manual focus hack with modal menu in MCP selector
Use Ariakit's `modal={true}` instead of a manual `requestAnimationFrame`
focus-restore wrapper, which eliminates the `useRef`/`useCallback`
overhead and lets Ariakit manage focus trapping natively. Also removes
the unused `focusLoop` option from both MCP menu stores and a narrating
comment in MCPSubMenu.
* test: add MCPSelect menu interaction tests
Cover button rendering, menu open/close via click and Escape, and
server toggle keeping the menu open. Renders real MCPServerMenuItem
and StackedMCPIcons components instead of re-implementing their
logic in mocks.
* fix: add unmountOnHide to MCP menu for consistency
Matches the pattern used by MCPSubMenu, BookmarkMenu, and other
Ariakit menus in the codebase. Ensures the menu fully detaches
from the DOM and accessibility tree when closed.
* fix: restore focusLoop on MCP menu stores
Ariakit's CompositeStore (which MenuStore extends) defaults focusLoop
to false. The previous commit incorrectly removed the explicit
focusLoop: true, which silently disabled Arrow-key wraparound
(mandatory per WAI-ARIA Menu pattern). modal={true} only traps Tab
focus — it does not enable Arrow-key looping.
* test: improve MCPSelect test coverage and mock hygiene
- Add aria-modal regression guard so removing modal={true} fails a test
- Add guard branch tests: no MCP access, empty servers, unpinned+empty
- Fix TooltipAnchor mock to correctly spread array children
- Fix import ordering per project conventions
- Move component import to top with other imports
- Replace unasserted jest.fn() mocks with plain values
- Use mutable module-scoped vars for per-test mock overrides
* fix: enhance pointer event handling in FavoriteItem component
Updated the opacity and pointer events logic in the FavoriteItem component to improve user interaction. The changes ensure that the component correctly manages pointer events based on the popover state, enhancing accessibility and usability.
* test: add MCPSubMenu menu interaction tests
Cover guard branch (empty servers), submenu open/close with real
Ariakit components and real MCPServerMenuItem, toggle persistence,
pin/unpin button behavior and aria-label states. Only context
providers and cross-package UI are mocked.
* test: add focusLoop regression guard for both MCP menus
ArrowDown from the last item must wrap to the first — this fails
without focusLoop: true on the menu store, directly guarding the
keyboard accessibility regression that was silently introduced.
---------
Co-authored-by: Danny Avila <danny@librechat.ai>
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
This pull request refactors the focus and menu behavior in the MCP server selection components and adds comprehensive tests for the MCPSelect component in order to resolve an issue with the dropdown for the component not being dismissed on click away.
Refactoring menu and focus logic:
focusLoopoption fromAriakit.useMenuStorein bothMCPSelectandMCPSubMenu, simplifying menu state management and relying on default Ariakit behavior. [1] [2] [3]onTogglehandler to directly usetoggleServerSelection, improving accessibility and interaction consistency. [1] [2]Testing improvements:
MCPSelectcomponent inMCPSelect.spec.tsx, covering menu rendering, item display, menu closing on Escape, and menu persistence after toggling server items.Change Type
Testing
Before
Screen.Recording.2026-03-15.at.6.58.06.PM.mov
After
Screen.Recording.2026-03-15.at.6.58.28.PM.mov
Checklist
Please delete any irrelevant options.