mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-27 04:07:05 +00:00
🪤 fix: Guard Prompts Popover Against Empty Result Keyboard Navigation (#13931)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
Docker Dev Images Build / build (Dockerfile, librechat-dev, node) (push) Waiting to run
Docker Dev Images Build / build (Dockerfile.multi, librechat-dev-api, api-build) (push) Waiting to run
Sync Locize Translations & Create Translation PR / Sync Translation Keys with Locize (push) Waiting to run
Sync Locize Translations & Create Translation PR / Create Translation PR on Version Published (push) Blocked by required conditions
Sync Helm Chart Tags / Ignore non-main push (push) Waiting to run
Sync Helm Chart Tags / Sync chart tags (push) Waiting to run
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
Docker Dev Images Build / build (Dockerfile, librechat-dev, node) (push) Waiting to run
Docker Dev Images Build / build (Dockerfile.multi, librechat-dev-api, api-build) (push) Waiting to run
Sync Locize Translations & Create Translation PR / Sync Translation Keys with Locize (push) Waiting to run
Sync Locize Translations & Create Translation PR / Create Translation PR on Version Published (push) Blocked by required conditions
Sync Helm Chart Tags / Ignore non-main push (push) Waiting to run
Sync Helm Chart Tags / Sync chart tags (push) Waiting to run
* 🛡️ fix: Guard Prompts and Mention popovers against empty-result navigation * 🛡️ fix: Prevent Tab default and clear stale filter on empty popover close
This commit is contained in:
parent
1662adc581
commit
ef1ee6ee16
4 changed files with 449 additions and 5 deletions
|
|
@ -1,8 +1,8 @@
|
|||
import { memo, useState, useRef, useEffect } from 'react';
|
||||
import { useRecoilValue, useSetRecoilState } from 'recoil';
|
||||
import { AutoSizer, List } from 'react-virtualized';
|
||||
import { Spinner, useCombobox } from '@librechat/client';
|
||||
import { EModelEndpoint } from 'librechat-data-provider';
|
||||
import { useRecoilValue, useSetRecoilState } from 'recoil';
|
||||
import type { RecoilState } from 'recoil';
|
||||
import type { MentionOption, ConvoGenerator } from '~/common';
|
||||
import { useGetConversation, useLocalize, TranslationKeys } from '~/hooks';
|
||||
|
|
@ -206,9 +206,7 @@ function MentionContent({
|
|||
setActiveIndex((prevIndex) => (prevIndex - 1 + matches.length) % matches.length);
|
||||
} else if (e.key === 'Enter' || e.key === 'Tab') {
|
||||
if (matches.length === 0) {
|
||||
if (e.key === 'Enter') {
|
||||
e.preventDefault();
|
||||
}
|
||||
e.preventDefault();
|
||||
setOpen(false);
|
||||
setShowPopover(false);
|
||||
textAreaRef.current?.focus();
|
||||
|
|
|
|||
|
|
@ -138,10 +138,15 @@ function PromptsCommand({
|
|||
useEffect(() => {
|
||||
if (!open) {
|
||||
setActiveIndex(0);
|
||||
setSearchValue('');
|
||||
} else {
|
||||
setVariableGroup(null);
|
||||
}
|
||||
}, [open]);
|
||||
}, [open, setSearchValue]);
|
||||
|
||||
useEffect(() => {
|
||||
setActiveIndex((prev) => Math.min(prev, Math.max(matches.length - 1, 0)));
|
||||
}, [matches.length]);
|
||||
|
||||
useEffect(() => {
|
||||
return () => {
|
||||
|
|
@ -214,10 +219,23 @@ function PromptsCommand({
|
|||
textAreaRef.current?.focus();
|
||||
}
|
||||
if (e.key === 'ArrowDown') {
|
||||
if (matches.length === 0) {
|
||||
return;
|
||||
}
|
||||
setActiveIndex((prevIndex) => (prevIndex + 1) % matches.length);
|
||||
} else if (e.key === 'ArrowUp') {
|
||||
if (matches.length === 0) {
|
||||
return;
|
||||
}
|
||||
setActiveIndex((prevIndex) => (prevIndex - 1 + matches.length) % matches.length);
|
||||
} else if (e.key === 'Enter' || e.key === 'Tab') {
|
||||
if (matches.length === 0) {
|
||||
e.preventDefault();
|
||||
setOpen(false);
|
||||
setShowPromptsPopover(false);
|
||||
textAreaRef.current?.focus();
|
||||
return;
|
||||
}
|
||||
if (e.key === 'Enter') {
|
||||
e.preventDefault();
|
||||
}
|
||||
|
|
|
|||
208
client/src/components/Chat/Input/__tests__/Mention.spec.tsx
Normal file
208
client/src/components/Chat/Input/__tests__/Mention.spec.tsx
Normal file
|
|
@ -0,0 +1,208 @@
|
|||
/**
|
||||
* Locks in the `@` mention popover keyboard contract for the empty-result
|
||||
* edge case (issue #13929 / PR #13928): when filtering yields zero matches
|
||||
* the arrow keys must be no-ops rather than computing a `% 0` -> NaN active
|
||||
* index, and Enter/Tab must close the popover and return focus to the
|
||||
* textarea. Deleting back to a non-empty list has to leave keyboard
|
||||
* navigation working from a valid index.
|
||||
*/
|
||||
import React from 'react';
|
||||
import { fireEvent, render, screen } from '@testing-library/react';
|
||||
import type { RecoilState } from 'recoil';
|
||||
import type { MentionOption } from '~/common';
|
||||
|
||||
const POPOVER_ATOM = 'show-mention-popover';
|
||||
const PLACEHOLDER = 'com_ui_mention';
|
||||
|
||||
const mockSetShowPopover = jest.fn();
|
||||
const mockShowPopover = { current: true };
|
||||
|
||||
jest.mock('recoil', () => {
|
||||
const actual = jest.requireActual('recoil');
|
||||
return {
|
||||
...actual,
|
||||
useRecoilValue: jest.fn((atom: unknown) =>
|
||||
atom === POPOVER_ATOM ? mockShowPopover.current : undefined,
|
||||
),
|
||||
useSetRecoilState: jest.fn((atom: unknown) =>
|
||||
atom === POPOVER_ATOM ? mockSetShowPopover : jest.fn(),
|
||||
),
|
||||
};
|
||||
});
|
||||
|
||||
const mockUseMentions = jest.fn();
|
||||
jest.mock('~/hooks/Input/useMentions', () => ({
|
||||
__esModule: true,
|
||||
default: () => mockUseMentions(),
|
||||
}));
|
||||
|
||||
jest.mock('~/hooks/Input/useSelectMention', () => ({
|
||||
__esModule: true,
|
||||
default: () => ({ onSelectMention: jest.fn() }),
|
||||
}));
|
||||
|
||||
jest.mock('~/hooks', () => ({
|
||||
useLocalize: () => (key: string) => key,
|
||||
useGetConversation: () => jest.fn(),
|
||||
}));
|
||||
|
||||
jest.mock('~/Providers', () => ({
|
||||
useAssistantsMapContext: () => ({}),
|
||||
}));
|
||||
|
||||
/* react-virtualized renders nothing in jsdom without a measured size; replace
|
||||
AutoSizer + List with a flat ul so every row's MentionItem button renders. */
|
||||
jest.mock('react-virtualized', () => ({
|
||||
...jest.requireActual('react-virtualized'),
|
||||
AutoSizer: ({ children }: { children: (size: { width: number }) => React.ReactNode }) =>
|
||||
children({ width: 320 }),
|
||||
List: ({
|
||||
rowCount,
|
||||
rowRenderer,
|
||||
}: {
|
||||
rowCount: number;
|
||||
rowRenderer: (args: {
|
||||
index: number;
|
||||
key: string;
|
||||
style: React.CSSProperties;
|
||||
}) => React.ReactNode;
|
||||
}) => {
|
||||
const rows: React.ReactNode[] = [];
|
||||
for (let i = 0; i < rowCount; i++) {
|
||||
rows.push(rowRenderer({ index: i, key: `row-${i}`, style: {} }));
|
||||
}
|
||||
return <ul data-testid="mention-list">{rows}</ul>;
|
||||
},
|
||||
}));
|
||||
|
||||
import Mention from '../Mention';
|
||||
|
||||
const makeTextarea = (initial = '@') => {
|
||||
const textarea = document.createElement('textarea');
|
||||
textarea.value = initial;
|
||||
document.body.appendChild(textarea);
|
||||
return { current: textarea } as React.MutableRefObject<HTMLTextAreaElement | null>;
|
||||
};
|
||||
|
||||
const makeMention = (overrides: Partial<MentionOption>): MentionOption => ({
|
||||
type: 'preset',
|
||||
label: 'Alpha',
|
||||
value: 'alpha',
|
||||
description: '',
|
||||
...overrides,
|
||||
});
|
||||
|
||||
const options: MentionOption[] = [
|
||||
makeMention({ label: 'Alpha', value: 'alpha' }),
|
||||
makeMention({ label: 'Beta', value: 'beta' }),
|
||||
];
|
||||
|
||||
const mentionsBundle = {
|
||||
options,
|
||||
presets: [],
|
||||
isLoading: false,
|
||||
modelSpecs: [],
|
||||
agentsList: [],
|
||||
modelsConfig: {},
|
||||
endpointsConfig: {},
|
||||
assistantListMap: {},
|
||||
};
|
||||
|
||||
const activeItemId = () =>
|
||||
document.querySelector('.bg-surface-active')?.closest('button')?.id ?? null;
|
||||
|
||||
const getInput = () => screen.getByPlaceholderText(PLACEHOLDER);
|
||||
|
||||
beforeAll(() => {
|
||||
Element.prototype.scrollIntoView = jest.fn();
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
document.body.innerHTML = '';
|
||||
mockShowPopover.current = true;
|
||||
mockUseMentions.mockReturnValue(mentionsBundle);
|
||||
});
|
||||
|
||||
const renderMention = () => {
|
||||
const textAreaRef = makeTextarea('@');
|
||||
const utils = render(
|
||||
<Mention
|
||||
index={0}
|
||||
popoverAtom={POPOVER_ATOM as unknown as RecoilState<boolean>}
|
||||
newConversation={jest.fn()}
|
||||
textAreaRef={textAreaRef}
|
||||
/>,
|
||||
);
|
||||
return { textAreaRef, ...utils };
|
||||
};
|
||||
|
||||
describe('Mention keyboard navigation', () => {
|
||||
it('renders nothing when the popover atom is false', () => {
|
||||
mockShowPopover.current = false;
|
||||
const { container } = renderMention();
|
||||
expect(container).toBeEmptyDOMElement();
|
||||
});
|
||||
|
||||
it('moves the active highlight with ArrowDown/ArrowUp and wraps around', () => {
|
||||
renderMention();
|
||||
const input = getInput();
|
||||
|
||||
expect(activeItemId()).toBe('mention-item-0');
|
||||
|
||||
fireEvent.keyDown(input, { key: 'ArrowDown' });
|
||||
expect(activeItemId()).toBe('mention-item-1');
|
||||
|
||||
fireEvent.keyDown(input, { key: 'ArrowDown' });
|
||||
expect(activeItemId()).toBe('mention-item-0');
|
||||
|
||||
fireEvent.keyDown(input, { key: 'ArrowUp' });
|
||||
expect(activeItemId()).toBe('mention-item-1');
|
||||
});
|
||||
|
||||
it('keeps navigation working after filtering to zero matches and back', () => {
|
||||
renderMention();
|
||||
const input = getInput();
|
||||
|
||||
fireEvent.change(input, { target: { value: 'zzz' } });
|
||||
expect(screen.queryByRole('button')).toBeNull();
|
||||
|
||||
/* Arrow keys on an empty list must be no-ops, not `% 0` -> NaN. */
|
||||
fireEvent.keyDown(input, { key: 'ArrowDown' });
|
||||
fireEvent.keyDown(input, { key: 'ArrowUp' });
|
||||
|
||||
fireEvent.change(input, { target: { value: '' } });
|
||||
expect(screen.getAllByRole('button')).toHaveLength(2);
|
||||
|
||||
/* Without the guard the active index would still be NaN here and no
|
||||
item would highlight; with it, navigation resumes from a valid index. */
|
||||
fireEvent.keyDown(input, { key: 'ArrowDown' });
|
||||
expect(activeItemId()).toBe('mention-item-1');
|
||||
});
|
||||
|
||||
it('closes the popover and refocuses the textarea when Enter is pressed with no matches', () => {
|
||||
const { textAreaRef } = renderMention();
|
||||
const input = getInput();
|
||||
|
||||
fireEvent.change(input, { target: { value: 'zzz' } });
|
||||
const notPrevented = fireEvent.keyDown(input, { key: 'Enter' });
|
||||
|
||||
expect(notPrevented).toBe(false);
|
||||
expect(mockSetShowPopover).toHaveBeenCalledWith(false);
|
||||
expect(document.activeElement).toBe(textAreaRef.current);
|
||||
});
|
||||
|
||||
it('prevents the default Tab action when closing on no matches so the refocus sticks', () => {
|
||||
const { textAreaRef } = renderMention();
|
||||
const input = getInput();
|
||||
|
||||
fireEvent.change(input, { target: { value: 'zzz' } });
|
||||
const notPrevented = fireEvent.keyDown(input, { key: 'Tab' });
|
||||
|
||||
/* Without preventDefault the browser's default Tab would move focus off
|
||||
the textarea right after we refocus it. */
|
||||
expect(notPrevented).toBe(false);
|
||||
expect(mockSetShowPopover).toHaveBeenCalledWith(false);
|
||||
expect(document.activeElement).toBe(textAreaRef.current);
|
||||
});
|
||||
});
|
||||
|
|
@ -0,0 +1,220 @@
|
|||
/**
|
||||
* Locks in the `/` prompts popover keyboard contract, specifically the
|
||||
* empty-result edge case that mirrors the `$` skills command and the `@`
|
||||
* mention popover: when filtering yields zero matches the arrow keys must
|
||||
* be no-ops (never `% 0` into a `NaN` active index) and Enter/Tab must
|
||||
* close the popover and return focus to the textarea. Deleting back to a
|
||||
* non-empty list has to leave keyboard navigation working.
|
||||
*/
|
||||
import React from 'react';
|
||||
import { fireEvent, render, screen } from '@testing-library/react';
|
||||
import type { PromptOption } from '~/common';
|
||||
|
||||
const PLACEHOLDER = 'com_ui_command_usage_placeholder';
|
||||
|
||||
const mockSetShowPromptsPopover = jest.fn();
|
||||
const mockShowPromptsPopover = { current: true };
|
||||
|
||||
jest.mock('recoil', () => {
|
||||
const actual = jest.requireActual('recoil');
|
||||
return {
|
||||
...actual,
|
||||
useRecoilValue: jest.fn((atom: unknown) =>
|
||||
atom === 'show-prompts-popover' ? mockShowPromptsPopover.current : undefined,
|
||||
),
|
||||
useSetRecoilState: jest.fn((atom: unknown) =>
|
||||
atom === 'show-prompts-popover' ? mockSetShowPromptsPopover : jest.fn(),
|
||||
),
|
||||
};
|
||||
});
|
||||
|
||||
jest.mock('~/store', () => ({
|
||||
__esModule: true,
|
||||
default: {
|
||||
showPromptsPopoverFamily: () => 'show-prompts-popover',
|
||||
},
|
||||
}));
|
||||
|
||||
const mockRecordUsage = jest.fn();
|
||||
jest.mock('~/data-provider', () => ({
|
||||
useRecordPromptUsage: () => ({ mutate: mockRecordUsage }),
|
||||
}));
|
||||
|
||||
const mockPromptGroupsContext = jest.fn();
|
||||
jest.mock('~/Providers', () => ({
|
||||
usePromptGroupsContext: () => mockPromptGroupsContext(),
|
||||
}));
|
||||
|
||||
jest.mock('~/components/Prompts', () => ({
|
||||
VariableDialog: () => null,
|
||||
}));
|
||||
|
||||
jest.mock('~/hooks', () => ({
|
||||
useLocalize: () => (key: string) => key,
|
||||
}));
|
||||
|
||||
/* react-virtualized renders nothing in jsdom without a measured size; replace
|
||||
AutoSizer + List with a flat ul so every row's MentionItem button renders. */
|
||||
jest.mock('react-virtualized', () => ({
|
||||
...jest.requireActual('react-virtualized'),
|
||||
AutoSizer: ({ children }: { children: (size: { width: number }) => React.ReactNode }) =>
|
||||
children({ width: 320 }),
|
||||
List: ({
|
||||
rowCount,
|
||||
rowRenderer,
|
||||
}: {
|
||||
rowCount: number;
|
||||
rowRenderer: (args: {
|
||||
index: number;
|
||||
key: string;
|
||||
style: React.CSSProperties;
|
||||
}) => React.ReactNode;
|
||||
}) => {
|
||||
const rows: React.ReactNode[] = [];
|
||||
for (let i = 0; i < rowCount; i++) {
|
||||
rows.push(rowRenderer({ index: i, key: `row-${i}`, style: {} }));
|
||||
}
|
||||
return <ul data-testid="prompts-list">{rows}</ul>;
|
||||
},
|
||||
}));
|
||||
|
||||
import PromptsCommand from '../PromptsCommand';
|
||||
|
||||
const makeTextarea = (initial = '/') => {
|
||||
const textarea = document.createElement('textarea');
|
||||
textarea.value = initial;
|
||||
document.body.appendChild(textarea);
|
||||
return { current: textarea } as React.MutableRefObject<HTMLTextAreaElement | null>;
|
||||
};
|
||||
|
||||
const makePrompt = (overrides: Partial<PromptOption>): PromptOption => ({
|
||||
id: '1',
|
||||
type: 'prompt',
|
||||
label: 'Alpha',
|
||||
value: 'Alpha',
|
||||
description: '',
|
||||
...overrides,
|
||||
});
|
||||
|
||||
const promptGroups: PromptOption[] = [
|
||||
makePrompt({ id: '1', label: 'Alpha', value: 'Alpha' }),
|
||||
makePrompt({ id: '2', label: 'Beta', value: 'Beta' }),
|
||||
];
|
||||
|
||||
const promptsMap = {
|
||||
'1': { _id: '1', productionPrompt: { prompt: 'Alpha body' } },
|
||||
'2': { _id: '2', productionPrompt: { prompt: 'Beta body' } },
|
||||
};
|
||||
|
||||
const activeItemId = () =>
|
||||
document.querySelector('.bg-surface-active')?.closest('button')?.id ?? null;
|
||||
|
||||
const getInput = () => screen.getByPlaceholderText(PLACEHOLDER);
|
||||
|
||||
beforeAll(() => {
|
||||
Element.prototype.scrollIntoView = jest.fn();
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
document.body.innerHTML = '';
|
||||
mockShowPromptsPopover.current = true;
|
||||
mockPromptGroupsContext.mockReturnValue({
|
||||
hasAccess: true,
|
||||
allPromptGroups: {
|
||||
data: { promptGroups, promptsMap },
|
||||
isLoading: false,
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
const renderCommand = () => {
|
||||
const textAreaRef = makeTextarea('/');
|
||||
const utils = render(
|
||||
<PromptsCommand index={0} textAreaRef={textAreaRef} submitPrompt={jest.fn()} />,
|
||||
);
|
||||
return { textAreaRef, ...utils };
|
||||
};
|
||||
|
||||
describe('PromptsCommand keyboard navigation', () => {
|
||||
it('renders nothing when the popover atom is false', () => {
|
||||
mockShowPromptsPopover.current = false;
|
||||
const { container } = renderCommand();
|
||||
expect(container).toBeEmptyDOMElement();
|
||||
});
|
||||
|
||||
it('moves the active highlight with ArrowDown/ArrowUp and wraps around', () => {
|
||||
renderCommand();
|
||||
const input = getInput();
|
||||
|
||||
expect(activeItemId()).toBe('prompt-item-0');
|
||||
|
||||
fireEvent.keyDown(input, { key: 'ArrowDown' });
|
||||
expect(activeItemId()).toBe('prompt-item-1');
|
||||
|
||||
fireEvent.keyDown(input, { key: 'ArrowDown' });
|
||||
expect(activeItemId()).toBe('prompt-item-0');
|
||||
|
||||
fireEvent.keyDown(input, { key: 'ArrowUp' });
|
||||
expect(activeItemId()).toBe('prompt-item-1');
|
||||
});
|
||||
|
||||
it('keeps navigation working after filtering to zero matches and back', () => {
|
||||
renderCommand();
|
||||
const input = getInput();
|
||||
|
||||
fireEvent.change(input, { target: { value: 'zzz' } });
|
||||
expect(screen.queryByRole('button')).toBeNull();
|
||||
|
||||
/* Arrow keys on an empty list must be no-ops, not `% 0` -> NaN. */
|
||||
fireEvent.keyDown(input, { key: 'ArrowDown' });
|
||||
fireEvent.keyDown(input, { key: 'ArrowUp' });
|
||||
|
||||
fireEvent.change(input, { target: { value: '' } });
|
||||
expect(screen.getAllByRole('button')).toHaveLength(2);
|
||||
|
||||
/* Without the guard the active index would still be NaN here and no
|
||||
item would highlight; with it, navigation resumes from a valid index. */
|
||||
fireEvent.keyDown(input, { key: 'ArrowDown' });
|
||||
expect(activeItemId()).toBe('prompt-item-1');
|
||||
});
|
||||
|
||||
it('closes the popover and refocuses the textarea when Enter is pressed with no matches', () => {
|
||||
const { textAreaRef } = renderCommand();
|
||||
const input = getInput();
|
||||
|
||||
fireEvent.change(input, { target: { value: 'zzz' } });
|
||||
const notPrevented = fireEvent.keyDown(input, { key: 'Enter' });
|
||||
|
||||
expect(notPrevented).toBe(false);
|
||||
expect(mockSetShowPromptsPopover).toHaveBeenCalledWith(false);
|
||||
expect(document.activeElement).toBe(textAreaRef.current);
|
||||
});
|
||||
|
||||
it('prevents the default Tab action when closing on no matches so the refocus sticks', () => {
|
||||
const { textAreaRef } = renderCommand();
|
||||
const input = getInput();
|
||||
|
||||
fireEvent.change(input, { target: { value: 'zzz' } });
|
||||
const notPrevented = fireEvent.keyDown(input, { key: 'Tab' });
|
||||
|
||||
/* Without preventDefault the browser's default Tab would move focus off
|
||||
the textarea right after we refocus it. */
|
||||
expect(notPrevented).toBe(false);
|
||||
expect(mockSetShowPromptsPopover).toHaveBeenCalledWith(false);
|
||||
expect(document.activeElement).toBe(textAreaRef.current);
|
||||
});
|
||||
|
||||
it('clears the stale search filter when the popover closes', () => {
|
||||
renderCommand();
|
||||
const input = getInput();
|
||||
|
||||
fireEvent.change(input, { target: { value: 'zzz' } });
|
||||
expect((input as HTMLInputElement).value).toBe('zzz');
|
||||
|
||||
/* PromptsCommand stays mounted across close (unlike Mention), so a leftover
|
||||
no-match query has to be cleared or the popover reopens still filtered. */
|
||||
fireEvent.keyDown(input, { key: 'Enter' });
|
||||
expect((getInput() as HTMLInputElement).value).toBe('');
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue