From 5b31bb720d6deb6f386173161eb55efd09e4273d Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 16 Mar 2026 23:50:18 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=A7=20fix:=20Proper=20MCP=20Menu=20Dis?= =?UTF-8?q?missal=20(#12256)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .../src/components/Chat/Input/MCPSelect.tsx | 21 +-- .../src/components/Chat/Input/MCPSubMenu.tsx | 1 - .../Chat/Input/__tests__/MCPSelect.spec.tsx | 142 ++++++++++++++++ .../Chat/Input/__tests__/MCPSubMenu.spec.tsx | 156 ++++++++++++++++++ .../components/Nav/Favorites/FavoriteItem.tsx | 4 +- 5 files changed, 304 insertions(+), 20 deletions(-) create mode 100644 client/src/components/Chat/Input/__tests__/MCPSelect.spec.tsx create mode 100644 client/src/components/Chat/Input/__tests__/MCPSubMenu.spec.tsx diff --git a/client/src/components/Chat/Input/MCPSelect.tsx b/client/src/components/Chat/Input/MCPSelect.tsx index a5356f5094..13a86c856a 100644 --- a/client/src/components/Chat/Input/MCPSelect.tsx +++ b/client/src/components/Chat/Input/MCPSelect.tsx @@ -1,4 +1,4 @@ -import React, { memo, useMemo, useCallback, useRef } from 'react'; +import React, { memo, useMemo } from 'react'; import * as Ariakit from '@ariakit/react'; import { ChevronDown } from 'lucide-react'; import { PermissionTypes, Permissions } from 'librechat-data-provider'; @@ -27,24 +27,9 @@ function MCPSelectContent() { const menuStore = Ariakit.useMenuStore({ focusLoop: true }); const isOpen = menuStore.useState('open'); - const focusedElementRef = useRef(null); const selectedCount = mcpValues?.length ?? 0; - // Wrap toggleServerSelection to preserve focus after state update - const handleToggle = useCallback( - (serverName: string) => { - // Save currently focused element - focusedElementRef.current = document.activeElement as HTMLElement; - toggleServerSelection(serverName); - // Restore focus after React re-renders - requestAnimationFrame(() => { - focusedElementRef.current?.focus(); - }); - }, - [toggleServerSelection], - ); - const selectedServers = useMemo(() => { if (!mcpValues || mcpValues.length === 0) { return []; @@ -103,6 +88,8 @@ function MCPSelectContent() { ))} diff --git a/client/src/components/Chat/Input/MCPSubMenu.tsx b/client/src/components/Chat/Input/MCPSubMenu.tsx index b0b8fad1bb..f8e617cba3 100644 --- a/client/src/components/Chat/Input/MCPSubMenu.tsx +++ b/client/src/components/Chat/Input/MCPSubMenu.tsx @@ -35,7 +35,6 @@ const MCPSubMenu = React.forwardRef( placement: 'right', }); - // Don't render if no MCP servers are configured if (!selectableServers || selectableServers.length === 0) { return null; } diff --git a/client/src/components/Chat/Input/__tests__/MCPSelect.spec.tsx b/client/src/components/Chat/Input/__tests__/MCPSelect.spec.tsx new file mode 100644 index 0000000000..7662ee5e6e --- /dev/null +++ b/client/src/components/Chat/Input/__tests__/MCPSelect.spec.tsx @@ -0,0 +1,142 @@ +import React from 'react'; +import userEvent from '@testing-library/user-event'; +import { render, screen, within } from '@testing-library/react'; +import MCPSelect from '../MCPSelect'; + +const mockToggleServerSelection = jest.fn(); + +const defaultMcpServerManager = { + localize: (key: string) => key, + isPinned: true, + mcpValues: [] as string[], + placeholderText: 'MCP Servers', + selectableServers: [ + { serverName: 'server-a', config: { title: 'Server A' } }, + { serverName: 'server-b', config: { title: 'Server B' } }, + ], + connectionStatus: {}, + isInitializing: () => false, + getConfigDialogProps: () => null, + toggleServerSelection: mockToggleServerSelection, + getServerStatusIconProps: () => null, +}; + +let mockCanUseMcp = true; +let mockMcpServerManager = { ...defaultMcpServerManager }; + +jest.mock('~/Providers', () => ({ + useBadgeRowContext: () => ({ + conversationId: 'test-conv', + storageContextKey: undefined, + mcpServerManager: mockMcpServerManager, + }), +})); + +jest.mock('~/hooks', () => ({ + useLocalize: () => (key: string) => key, + useHasAccess: () => mockCanUseMcp, +})); + +jest.mock('@librechat/client', () => { + // eslint-disable-next-line @typescript-eslint/no-require-imports + const R = require('react'); + return { + TooltipAnchor: ({ + children, + render, + }: { + children: React.ReactNode; + render: React.ReactElement; + }) => R.cloneElement(render, {}, ...(Array.isArray(children) ? children : [children])), + MCPIcon: ({ className }: { className?: string }) => R.createElement('span', { className }), + Spinner: ({ className }: { className?: string }) => R.createElement('span', { className }), + }; +}); + +jest.mock('~/components/MCP/MCPConfigDialog', () => ({ + __esModule: true, + default: () => null, +})); + +describe('MCPSelect', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockCanUseMcp = true; + mockMcpServerManager = { ...defaultMcpServerManager }; + }); + + it('renders the menu button', () => { + render(); + expect(screen.getByRole('button', { name: /MCP Servers/i })).toBeInTheDocument(); + }); + + it('opens menu on button click and shows server items', async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole('button', { name: /MCP Servers/i })); + + const menu = screen.getByRole('menu', { name: /com_ui_mcp_servers/i }); + expect(menu).toBeVisible(); + expect(within(menu).getByRole('menuitemcheckbox', { name: /Server A/i })).toBeInTheDocument(); + expect(within(menu).getByRole('menuitemcheckbox', { name: /Server B/i })).toBeInTheDocument(); + }); + + it('closes menu on Escape', async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole('button', { name: /MCP Servers/i })); + expect(screen.getByRole('menu', { name: /com_ui_mcp_servers/i })).toBeVisible(); + + await user.keyboard('{Escape}'); + expect(screen.getByRole('button', { name: /MCP Servers/i })).toHaveAttribute( + 'aria-expanded', + 'false', + ); + }); + + it('keeps menu open after toggling a server item', async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole('button', { name: /MCP Servers/i })); + await user.click(screen.getByRole('menuitemcheckbox', { name: /Server A/i })); + + expect(mockToggleServerSelection).toHaveBeenCalledWith('server-a'); + expect(screen.getByRole('menu', { name: /com_ui_mcp_servers/i })).toBeVisible(); + }); + + it('arrow-key navigation wraps from last item to first', async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole('button', { name: /MCP Servers/i })); + const items = screen.getAllByRole('menuitemcheckbox'); + expect(items).toHaveLength(2); + + await user.keyboard('{ArrowDown}'); + await user.keyboard('{ArrowDown}'); + await user.keyboard('{ArrowDown}'); + expect(items[0]).toHaveFocus(); + }); + + it('renders nothing when user lacks MCP access', () => { + mockCanUseMcp = false; + const { container } = render(); + expect(container.firstChild).toBeNull(); + expect(screen.queryByRole('button')).not.toBeInTheDocument(); + }); + + it('renders nothing when selectableServers is empty', () => { + mockMcpServerManager = { ...defaultMcpServerManager, selectableServers: [] }; + const { container } = render(); + expect(container.firstChild).toBeNull(); + }); + + it('renders nothing when not pinned and no servers selected', () => { + mockMcpServerManager = { ...defaultMcpServerManager, isPinned: false, mcpValues: [] }; + const { container } = render(); + expect(container.firstChild).toBeNull(); + }); +}); diff --git a/client/src/components/Chat/Input/__tests__/MCPSubMenu.spec.tsx b/client/src/components/Chat/Input/__tests__/MCPSubMenu.spec.tsx new file mode 100644 index 0000000000..be8fb5d9c2 --- /dev/null +++ b/client/src/components/Chat/Input/__tests__/MCPSubMenu.spec.tsx @@ -0,0 +1,156 @@ +import React from 'react'; +import * as Ariakit from '@ariakit/react'; +import userEvent from '@testing-library/user-event'; +import { render, screen, within } from '@testing-library/react'; +import MCPSubMenu from '../MCPSubMenu'; + +const mockToggleServerSelection = jest.fn(); +const mockSetIsPinned = jest.fn(); + +const defaultMcpServerManager = { + isPinned: true, + mcpValues: [] as string[], + setIsPinned: mockSetIsPinned, + placeholderText: 'MCP Servers', + selectableServers: [ + { serverName: 'server-a', config: { title: 'Server A' } }, + { serverName: 'server-b', config: { title: 'Server B', description: 'Second server' } }, + ], + connectionStatus: {}, + isInitializing: () => false, + getConfigDialogProps: () => null, + toggleServerSelection: mockToggleServerSelection, + getServerStatusIconProps: () => null, +}; + +let mockMcpServerManager = { ...defaultMcpServerManager }; + +jest.mock('~/Providers', () => ({ + useBadgeRowContext: () => ({ + storageContextKey: undefined, + mcpServerManager: mockMcpServerManager, + }), +})); + +jest.mock('~/hooks', () => ({ + useLocalize: () => (key: string) => key, + useHasAccess: () => true, +})); + +jest.mock('@librechat/client', () => { + // eslint-disable-next-line @typescript-eslint/no-require-imports + const R = require('react'); + return { + MCPIcon: ({ className }: { className?: string }) => R.createElement('span', { className }), + PinIcon: ({ unpin }: { unpin?: boolean }) => + R.createElement('span', { 'data-testid': unpin ? 'unpin-icon' : 'pin-icon' }), + Spinner: ({ className }: { className?: string }) => R.createElement('span', { className }), + }; +}); + +jest.mock('~/components/MCP/MCPConfigDialog', () => ({ + __esModule: true, + default: () => null, +})); + +function ParentMenu({ children }: { children: React.ReactNode }) { + return ( + + {/* eslint-disable-next-line i18next/no-literal-string */} + Parent + {children} + + ); +} + +function renderSubMenu(props: React.ComponentProps = {}) { + return render( + + + , + ); +} + +describe('MCPSubMenu', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockMcpServerManager = { ...defaultMcpServerManager }; + }); + + it('renders nothing when selectableServers is empty', () => { + mockMcpServerManager = { ...defaultMcpServerManager, selectableServers: [] }; + renderSubMenu(); + expect(screen.queryByText('MCP Servers')).not.toBeInTheDocument(); + }); + + it('renders the submenu trigger with default placeholder', () => { + renderSubMenu(); + expect(screen.getByText('MCP Servers')).toBeInTheDocument(); + }); + + it('renders custom placeholder when provided', () => { + renderSubMenu({ placeholder: 'Custom Label' }); + expect(screen.getByText('Custom Label')).toBeInTheDocument(); + expect(screen.queryByText('MCP Servers')).not.toBeInTheDocument(); + }); + + it('opens submenu and shows real server items', async () => { + const user = userEvent.setup(); + renderSubMenu(); + + await user.click(screen.getByText('MCP Servers')); + + const menu = screen.getByRole('menu', { name: /com_ui_mcp_servers/i }); + expect(menu).toBeVisible(); + expect(within(menu).getByRole('menuitemcheckbox', { name: /Server A/i })).toBeInTheDocument(); + expect(within(menu).getByRole('menuitemcheckbox', { name: /Server B/i })).toBeInTheDocument(); + }); + + it('keeps menu open after toggling a server item', async () => { + const user = userEvent.setup(); + renderSubMenu(); + + await user.click(screen.getByText('MCP Servers')); + await user.click(screen.getByRole('menuitemcheckbox', { name: /Server A/i })); + + expect(mockToggleServerSelection).toHaveBeenCalledWith('server-a'); + expect(screen.getByRole('menu', { name: /com_ui_mcp_servers/i })).toBeVisible(); + }); + + it('calls setIsPinned with toggled value when pin button is clicked', async () => { + const user = userEvent.setup(); + mockMcpServerManager = { ...defaultMcpServerManager, isPinned: false }; + renderSubMenu(); + + await user.click(screen.getByRole('button', { name: /com_ui_pin/i })); + + expect(mockSetIsPinned).toHaveBeenCalledWith(true); + }); + + it('arrow-key navigation wraps from last item to first', async () => { + const user = userEvent.setup(); + renderSubMenu(); + + await user.click(screen.getByText('MCP Servers')); + const items = screen.getAllByRole('menuitemcheckbox'); + expect(items).toHaveLength(2); + + await user.click(items[1]); + expect(items[1]).toHaveFocus(); + + await user.keyboard('{ArrowDown}'); + expect(items[0]).toHaveFocus(); + }); + + it('pin button shows unpin label when pinned', () => { + mockMcpServerManager = { ...defaultMcpServerManager, isPinned: true }; + renderSubMenu(); + expect(screen.getByRole('button', { name: /com_ui_unpin/i })).toBeInTheDocument(); + }); + + it('pin button shows pin label when not pinned', () => { + mockMcpServerManager = { ...defaultMcpServerManager, isPinned: false }; + renderSubMenu(); + expect(screen.getByRole('button', { name: /com_ui_pin/i })).toBeInTheDocument(); + }); +}); diff --git a/client/src/components/Nav/Favorites/FavoriteItem.tsx b/client/src/components/Nav/Favorites/FavoriteItem.tsx index 173be27d00..248008869d 100644 --- a/client/src/components/Nav/Favorites/FavoriteItem.tsx +++ b/client/src/components/Nav/Favorites/FavoriteItem.tsx @@ -126,8 +126,8 @@ export default function FavoriteItem({ className={cn( 'absolute right-2 flex items-center', isPopoverActive - ? 'opacity-100' - : 'opacity-0 group-focus-within:opacity-100 group-hover:opacity-100', + ? 'pointer-events-auto opacity-100' + : 'pointer-events-none opacity-0 group-focus-within:pointer-events-auto group-focus-within:opacity-100 group-hover:pointer-events-auto group-hover:opacity-100', )} onClick={(e) => e.stopPropagation()} >