🔧 fix: Proper MCP Menu Dismissal (#12256)

* 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>

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<HTMLElement | null>(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() {
         <Ariakit.Menu
           portal={true}
           gutter={8}
+          modal={true}
+          unmountOnHide={true}
           aria-label={localize('com_ui_mcp_servers')}
           className={cn(
             'z-50 flex min-w-[260px] max-w-[320px] flex-col rounded-xl',
@@ -121,7 +108,7 @@ function MCPSelectContent() {
                 connectionStatus={connectionStatus}
                 isInitializing={isInitializing}
                 statusIconProps={getServerStatusIconProps(server.serverName)}
-                onToggle={handleToggle}
+                onToggle={toggleServerSelection}
               />
             ))}
           </div>

client/src/components/Chat/Input/MCPSubMenu.tsx

@@ -35,7 +35,6 @@ const MCPSubMenu = React.forwardRef<HTMLDivElement, MCPSubMenuProps>(
       placement: 'right',
     });
 
-    // Don't render if no MCP servers are configured
     if (!selectableServers || selectableServers.length === 0) {
       return null;
     }

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(<MCPSelect />);
+    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(<MCPSelect />);
+
+    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(<MCPSelect />);
+
+    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(<MCPSelect />);
+
+    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(<MCPSelect />);
+
+    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(<MCPSelect />);
+    expect(container.firstChild).toBeNull();
+    expect(screen.queryByRole('button')).not.toBeInTheDocument();
+  });
+
+  it('renders nothing when selectableServers is empty', () => {
+    mockMcpServerManager = { ...defaultMcpServerManager, selectableServers: [] };
+    const { container } = render(<MCPSelect />);
+    expect(container.firstChild).toBeNull();
+  });
+
+  it('renders nothing when not pinned and no servers selected', () => {
+    mockMcpServerManager = { ...defaultMcpServerManager, isPinned: false, mcpValues: [] };
+    const { container } = render(<MCPSelect />);
+    expect(container.firstChild).toBeNull();
+  });
+});

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 (
+    <Ariakit.MenuProvider>
+      {/* eslint-disable-next-line i18next/no-literal-string */}
+      <Ariakit.MenuButton>Parent</Ariakit.MenuButton>
+      <Ariakit.Menu open={true}>{children}</Ariakit.Menu>
+    </Ariakit.MenuProvider>
+  );
+}
+
+function renderSubMenu(props: React.ComponentProps<typeof MCPSubMenu> = {}) {
+  return render(
+    <ParentMenu>
+      <MCPSubMenu {...props} />
+    </ParentMenu>,
+  );
+}
+
+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();
+  });
+});

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()}
       >