Skip to content

Commit f51ea67

Browse files
berry-13dev-elie
andauthored
⌨️ feat: Keyboard Submit, Focus and Escape for Ask User Cards (#17231)
* ⌨️ feat: Keyboard submit for ask_user_question answer box The interactive ask_user_question card's answer textarea had no key handling, so the answer could only be submitted by clicking. Route its keydown through resolveComposerKeyDown so it follows the composer's Enter-to-send preference, a rebound submit chord, and IME composition. The run is paused rather than generating, so it resolves as idle and Ctrl/Cmd+Enter submits instead of reaching the during-run actions. * ⌨️ feat: Keyboard focus and confirm for batched ask_user_question - Focus the first question's controls when a batch arrives, only when focus is idle (body, or the empty composer that sent the turn), and focus into the next step on advance instead of the fieldset: the picked or first option, else the answer field. - Enter on the already-picked single-select option confirms the step; Enter on an unpicked option stays its click, so a pick never submits by itself (WCAG 3.2.2). Multi-select rows only toggle. - Ctrl/Cmd+Enter confirm from the answer field like Enter; Shift+Enter never does. * 🐛 fix: Keep Held Enter and Remounts From Picking Batched Answers Review findings on the batched ask_user_question card, and what this commit does about each. 1. Held or double-tapped Enter could pick answers and submit the batch. After Enter confirmed a step, focus moved to the next step's first option. A keyboard Enter on a focused button is its click, so the key repeat of a held Enter (or a quick double-tap) picked that option and advanced again. On the last step the next repeat landed on the now-pressed option, confirmed it, and submitted the whole batch with answers the user never chose. Fix: after a step change, focus goes to the step's picked option, else its answer field. An unpicked option is only a focus target when a batch first arrives. Repeats now reach a text field, which can page on but never picks an option or submits on the user's behalf. 2. Focus-on-arrival ran on every mount, not only on arrival. Its effect has empty deps, so moving the batch between the composer popover and the chat card, or remounting a message that still holds an unanswered batch (reopening a conversation, a Share view), pulled focus onto an option of that batch. The next Enter or Space would then answer it. Fix: `AskUserQuestions` takes a `focusOnArrival` prop, set only by the surface showing the live pause (the popover, or the chat card while it is live and visible), so other mounts never take focus. Focus is placed at most once per batch, tracked by action id, so a move between surfaces is not treated as a new arrival. Tests: a held Enter from an answer field picks nothing and never submits; a surface that isn't the live pause leaves focus alone; a remount of the same batch doesn't take focus again. All three fail without this change. The Chat/Messages/Content, Chat/Input, Chat/ask and hooks/Input suites pass, apart from TokenUsage/index.spec.tsx, which fails the same way without this change. tsc, ESLint and Prettier are clean on the changed files. * feat: Escape to Skip Ask Cards and Move the Popover to Chat Escape in the chat card skips the question or batch, but only before anything is entered, so a stray Escape cannot discard an answer and resume the run. Escape with focus inside the composer popover moves the question to the chat, as it already did from the composer, and returns focus to the composer once the popover closes. Also drops the em dashes from the batched card's comments. * fix: Read Enter-to-Send From the Chat Surface Host in the Ask Card * test: Cover Ask Batch Keyboard Control in E2E Scenarios * fix: Keep Held and Modified Enter From Submitting Ask Answers Ignore auto-repeated Enter before confirming a picked option, so a held Enter cannot both pick and submit the last question. In the answer field, Shift, Alt and repeated Enter now prevent the enclosing chat form's implicit submit instead of returning early. Escape that cancels an IME composition no longer moves the popover to the chat, and the arrival-focus flag lives in the batch's Jotai form state. * fix: Honor a Submit Shortcut Rebound Off Enter in Ask Answer Fields The window shortcut handler yields submitMessage in every editing field except the composer, so a chord such as Ctrl+J did nothing in the ask answer fields. Both the single-question answer box and the batched answer field now act on it, through a shared isReboundSubmitChord. --------- Co-authored-by: elie605 <ebtien@gmail.com>
1 parent ce9a9d8 commit f51ea67

10 files changed

Lines changed: 1140 additions & 34 deletions

File tree

‎client/src/components/Chat/Input/AskUserQuestionPopover.tsx‎

Lines changed: 46 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
1-
import { memo, useEffect, useRef } from 'react';
1+
import { memo, useCallback, useEffect, useRef } from 'react';
22
import { useWatch } from 'react-hook-form';
33
import { Button, TooltipAnchor } from '@librechat/client';
44
import { ChevronDown, CornerDownLeft, TriangleAlert } from 'lucide-react';
55
import AskUserQuestions from '~/components/Chat/Messages/Content/AskUserQuestions';
66
import useAskAnswerMode from '~/hooks/Input/useAskAnswerMode';
77
import AskOptions from '~/components/Chat/ask/options';
88
import { useChatFormContext } from '~/Providers';
9+
import { isCancelKey } from '~/utils/shortcuts';
910
import { useComposerOverlay } from './overlay';
1011
import { useLocalize } from '~/hooks';
1112

@@ -32,20 +33,47 @@ function AskUserQuestionPopoverContent({
3233
textAreaRef?: React.RefObject<HTMLTextAreaElement>;
3334
}) {
3435
const ask = useAskAnswerMode(conversationId);
35-
useComposerOverlay(conversationId, ask.popoverVisible);
36+
const { collapse, popoverVisible } = ask;
37+
useComposerOverlay(conversationId, popoverVisible);
3638

37-
if (!ask.popoverVisible || !ask.liveAsk) {
39+
/**
40+
* Escape from inside the popover does what it does from the composer: moves
41+
* the question to the chat. Focus then returns to the composer, which a
42+
* batch keeps disabled until the popover is gone, hence after the commit.
43+
*/
44+
const refocusComposer = useRef(false);
45+
const escapeToChat = useCallback(() => {
46+
refocusComposer.current = true;
47+
collapse();
48+
}, [collapse]);
49+
useEffect(() => {
50+
if (popoverVisible || !refocusComposer.current) {
51+
return;
52+
}
53+
refocusComposer.current = false;
54+
textAreaRef?.current?.focus();
55+
}, [popoverVisible, textAreaRef]);
56+
57+
if (!popoverVisible || !ask.liveAsk) {
3858
return null;
3959
}
4060

4161
if (ask.liveAsk.questions != null && ask.liveAsk.questions.length > 0) {
42-
return <AskUserQuestionsPopoverPanel ask={ask} />;
62+
return <AskUserQuestionsPopoverPanel ask={ask} onEscape={escapeToChat} />;
4363
}
4464

45-
return <AskUserQuestionPopoverPanel ask={ask} textAreaRef={textAreaRef} />;
65+
return (
66+
<AskUserQuestionPopoverPanel ask={ask} textAreaRef={textAreaRef} onEscape={escapeToChat} />
67+
);
4668
}
4769

48-
function AskUserQuestionsPopoverPanel({ ask }: { ask: ReturnType<typeof useAskAnswerMode> }) {
70+
function AskUserQuestionsPopoverPanel({
71+
ask,
72+
onEscape,
73+
}: {
74+
ask: ReturnType<typeof useAskAnswerMode>;
75+
onEscape: () => void;
76+
}) {
4977
const { liveAsk, collapse } = ask;
5078
const questions = liveAsk?.questions;
5179
if (liveAsk == null || questions == null || questions.length === 0) {
@@ -62,6 +90,8 @@ function AskUserQuestionsPopoverPanel({ ask }: { ask: ReturnType<typeof useAskAn
6290
actionId={liveAsk.actionId}
6391
questions={questions}
6492
className="shrink-0"
93+
focusOnArrival
94+
onEscape={onEscape}
6595
headerAction={<MoveToChatButton onClick={collapse} />}
6696
/>
6797
</div>
@@ -101,9 +131,11 @@ function MoveToChatButton({ onClick }: { onClick: () => void }) {
101131
function AskUserQuestionPopoverPanel({
102132
ask,
103133
textAreaRef,
134+
onEscape,
104135
}: {
105136
ask: ReturnType<typeof useAskAnswerMode>;
106137
textAreaRef?: React.RefObject<HTMLTextAreaElement>;
138+
onEscape: () => void;
107139
}) {
108140
const localize = useLocalize();
109141
const { control } = useChatFormContext();
@@ -165,7 +197,14 @@ function AskUserQuestionPopoverPanel({
165197
overflows it is unreachable by page scroll. */}
166198
<div
167199
className="popover border-border-light bg-surface-secondary rounded-theme-popover flex max-h-[60vh] flex-col border p-2 shadow-lg [view-transition-name:ask-question]"
168-
onKeyDown={handlePopoverKeyDown}
200+
onKeyDown={(e) => {
201+
if (isCancelKey(e.nativeEvent) && !e.nativeEvent.isComposing) {
202+
e.preventDefault();
203+
onEscape();
204+
return;
205+
}
206+
handlePopoverKeyDown(e);
207+
}}
169208
>
170209
<div className="text-text-secondary flex shrink-0 items-start justify-between gap-2 p-1 pl-2">
171210
<div className="max-h-[24vh] min-w-0 overflow-y-auto pt-1">
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
import React, { createRef } from 'react';
2+
import { RecoilRoot } from 'recoil';
3+
import { useForm, FormProvider } from 'react-hook-form';
4+
import { act, fireEvent, render, screen } from '@testing-library/react';
5+
import type { Agents } from 'librechat-data-provider';
6+
import AskUserQuestionPopover from '../AskUserQuestionPopover';
7+
8+
const mockCollapse = jest.fn();
9+
const mockHandlePopoverKeyDown = jest.fn();
10+
/** The popover is memoized, so visibility changes reach it the way the real
11+
* hook's state does: through a store it subscribes to. */
12+
const mockVisibility = {
13+
visible: true,
14+
listeners: new Set<() => void>(),
15+
set(visible: boolean) {
16+
this.visible = visible;
17+
this.listeners.forEach((listener) => listener());
18+
},
19+
subscribe(listener: () => void) {
20+
mockVisibility.listeners.add(listener);
21+
return () => mockVisibility.listeners.delete(listener);
22+
},
23+
};
24+
let mockQuestions: Agents.AskUserQuestionBatchItem[] | undefined;
25+
26+
jest.mock('~/hooks', () => ({
27+
useLocalize: () => (key: string) => key,
28+
}));
29+
30+
jest.mock('~/components/Chat/Messages/Content/ApprovalContext', () => ({
31+
useAskSubmitStatus: () => ({ getAskStatus: () => 'idle' }),
32+
useResumeSubmit: () => ({ submitAskAnswer: jest.fn() }),
33+
}));
34+
35+
jest.mock('../overlay', () => ({
36+
useComposerOverlay: jest.fn(),
37+
}));
38+
39+
jest.mock('~/Providers', () => ({
40+
useChatFormContext: () => jest.requireActual('react-hook-form').useFormContext(),
41+
}));
42+
43+
jest.mock('~/hooks/Input/useAskAnswerMode', () => ({
44+
__esModule: true,
45+
default: () => ({
46+
popoverVisible: jest
47+
.requireActual('react')
48+
.useSyncExternalStore(mockVisibility.subscribe, () => mockVisibility.visible),
49+
liveAsk: {
50+
actionId: 'ask-1',
51+
question: { question: 'Which environment?', options: [{ label: 'Staging', value: 's' }] },
52+
questions: mockQuestions,
53+
},
54+
options: [{ label: 'Staging', value: 's' }],
55+
selected: null,
56+
checked: [],
57+
multiSelect: false,
58+
locked: false,
59+
errored: false,
60+
toggleChecked: jest.fn(),
61+
canSubmit: false,
62+
submit: jest.fn(),
63+
submitOption: jest.fn(),
64+
skip: jest.fn(),
65+
collapse: mockCollapse,
66+
handlePopoverKeyDown: mockHandlePopoverKeyDown,
67+
}),
68+
}));
69+
70+
function Host({ textAreaRef }: { textAreaRef: React.RefObject<HTMLTextAreaElement> }) {
71+
const methods = useForm({ defaultValues: { text: '' } });
72+
return (
73+
<RecoilRoot>
74+
<FormProvider {...methods}>
75+
<textarea ref={textAreaRef} aria-label="Composer" />
76+
<AskUserQuestionPopover conversationId="conversation-1" textAreaRef={textAreaRef} />
77+
</FormProvider>
78+
</RecoilRoot>
79+
);
80+
}
81+
82+
describe('AskUserQuestionPopover Escape', () => {
83+
beforeEach(() => {
84+
jest.clearAllMocks();
85+
mockVisibility.visible = true;
86+
mockQuestions = undefined;
87+
});
88+
89+
/** Escape on `target`, then the popover closes; returns the composer. */
90+
const escapeThenClose = (target: () => HTMLElement) => {
91+
const textAreaRef = createRef<HTMLTextAreaElement>();
92+
render(<Host textAreaRef={textAreaRef} />);
93+
fireEvent.keyDown(target(), { key: 'Escape' });
94+
act(() => mockVisibility.set(false));
95+
return textAreaRef.current;
96+
};
97+
98+
test('moves a single question to the chat and returns focus to the composer', () => {
99+
const composer = escapeThenClose(() => screen.getByRole('button', { name: /Staging/ }));
100+
expect(mockCollapse).toHaveBeenCalledTimes(1);
101+
expect(mockHandlePopoverKeyDown).not.toHaveBeenCalled();
102+
expect(composer).toHaveFocus();
103+
});
104+
105+
test('moves a batch to the chat and returns focus to the composer', () => {
106+
mockQuestions = [
107+
{ id: 'window', question: 'Which time window?' },
108+
{ id: 'region', question: 'Which region?' },
109+
];
110+
const composer = escapeThenClose(() =>
111+
screen.getByRole('textbox', { name: /Which time window/ }),
112+
);
113+
expect(mockCollapse).toHaveBeenCalledTimes(1);
114+
expect(composer).toHaveFocus();
115+
});
116+
117+
test('leaves an Escape that cancels an IME composition alone', () => {
118+
render(<Host textAreaRef={createRef<HTMLTextAreaElement>()} />);
119+
fireEvent.keyDown(screen.getByRole('button', { name: /Staging/ }), {
120+
key: 'Escape',
121+
isComposing: true,
122+
});
123+
expect(mockCollapse).not.toHaveBeenCalled();
124+
});
125+
126+
test('passes other keys through to the popover shortcuts', () => {
127+
render(<Host textAreaRef={createRef<HTMLTextAreaElement>()} />);
128+
fireEvent.keyDown(screen.getByRole('button', { name: /Staging/ }), { key: '1' });
129+
expect(mockHandlePopoverKeyDown).toHaveBeenCalledTimes(1);
130+
expect(mockCollapse).not.toHaveBeenCalled();
131+
});
132+
});

‎client/src/components/Chat/Messages/Content/AskUserQuestion.tsx‎

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,25 @@ import { useContext, useMemo, useState } from 'react';
22
import { Button, TextareaAutosize, TooltipAnchor } from '@librechat/client';
33
import { ChevronDown, MessageCircleQuestion, TriangleAlert } from 'lucide-react';
44
import type { Agents } from 'librechat-data-provider';
5+
import type { KeyboardEvent } from 'react';
6+
import type { ComposerBindings } from '~/hooks/Input/useComposerBindings';
7+
import { isCancelKey, isReboundSubmitChord, resolveComposerKeyDown } from '~/utils/shortcuts';
58
import { useApprovalContext, useAskSubmitStatus, useResumeSubmit } from './ApprovalContext';
69
import { splitOtherOption, ASK_USER_DECLINED_ANSWER } from '~/utils/approval';
10+
import { useOptionalChatSurface } from '~/components/Chat/Subagents/surface';
11+
import { cn, forceResize, insertTextAtCursor } from '~/utils';
712
import useAskAnswerMode from '~/hooks/Input/useAskAnswerMode';
813
import AskOptions from '~/components/Chat/ask/options';
914
import { ChatContext } from '~/Providers/ChatContext';
1015
import AskUserQuestions from './AskUserQuestions';
1116
import { useLocalize } from '~/hooks';
12-
import { cn } from '~/utils';
17+
18+
/** Stock preferences for a card rendered outside a chat surface. */
19+
const NO_HOST_BINDINGS: ComposerBindings = {
20+
shortcutsEnabled: true,
21+
submitOverride: undefined,
22+
yieldedChords: new Set(),
23+
};
1324

1425
/**
1526
* Renders an `ask_user_question` pause: the prompt, optional description, any
@@ -81,6 +92,7 @@ function AskUserQuestionsCard({
8192
<AskUserQuestions
8293
actionId={actionId}
8394
questions={questions}
95+
focusOnArrival={live}
8496
headerAction={
8597
onExpand != null && (
8698
<TooltipAnchor
@@ -122,6 +134,10 @@ function AskUserQuestionSingle({
122134
const { submitAskAnswer } = useResumeSubmit();
123135
const [answer, setAnswer] = useState(() => getAskAnswerDraft(actionId));
124136
const [localChecked, setLocalChecked] = useState<number[]>([]);
137+
const surface = useOptionalChatSurface();
138+
const enterToSend = surface?.enterToSend ?? false;
139+
const { shortcutsEnabled, submitOverride, yieldedChords } =
140+
surface?.composerBindings ?? NO_HOST_BINDINGS;
125141
/**
126142
* The composer popover is the primary answer surface — while it's VISIBLE
127143
* for this pause, rendering the card too duplicates the question. The card
@@ -216,6 +232,59 @@ function AskUserQuestionSingle({
216232
submitAskAnswer(actionId, values.join(', '));
217233
};
218234

235+
/**
236+
* The answer box follows the composer's Enter table, so the user's
237+
* Enter-to-send preference and any rebound submit chord apply here too. The
238+
* run is paused on this question rather than generating, so it resolves as
239+
* idle: Ctrl/Cmd+Enter must submit the answer, not reach the during-run
240+
* steer/queue actions.
241+
*/
242+
const handleAnswerKeyDown = (e: KeyboardEvent<HTMLTextAreaElement>) => {
243+
const isComposing = e.nativeEvent.isComposing || e.key === 'Process' || e.keyCode === 229;
244+
if (!isComposing && isReboundSubmitChord(e.nativeEvent, { shortcutsEnabled, submitOverride })) {
245+
e.preventDefault();
246+
if (canSubmit && !locked) {
247+
submitCombined();
248+
}
249+
return;
250+
}
251+
const action = resolveComposerKeyDown(e.nativeEvent, {
252+
isComposing,
253+
isSubmitting: false,
254+
allowSubmitWhileGenerating: false,
255+
hasDuringRunModifier: false,
256+
shortcutsEnabled,
257+
enterToSend,
258+
submitOverride,
259+
yieldedChords,
260+
});
261+
if (action === 'none') {
262+
return;
263+
}
264+
e.preventDefault();
265+
if (action === 'newline') {
266+
insertTextAtCursor(e.currentTarget, '\n');
267+
forceResize(e.currentTarget);
268+
return;
269+
}
270+
if (action === 'submit' && canSubmit && !locked) {
271+
submitCombined();
272+
}
273+
};
274+
275+
/** Escape skips, but only before anything is entered, so a stray Escape
276+
* cannot discard an answer and resume the run. */
277+
const handleCardKeyDown = (e: KeyboardEvent<HTMLDivElement>) => {
278+
if (!isCancelKey(e.nativeEvent) || e.nativeEvent.isComposing) {
279+
return;
280+
}
281+
if (locked || trimmed.length > 0 || (multiSelect && checkedIndices.length > 0)) {
282+
return;
283+
}
284+
e.preventDefault();
285+
handleSkip();
286+
};
287+
219288
/**
220289
* The live card shares its view-transition-name with the popover panel, so
221290
* collapse/expand morphs one surface into the other. The placeholder copy
@@ -233,6 +302,7 @@ function AskUserQuestionSingle({
233302
isLivePause && !showPlaceholder && '[view-transition-name:ask-question]',
234303
)}
235304
aria-hidden={showPlaceholder || undefined}
305+
onKeyDown={handleCardKeyDown}
236306
>
237307
<div className="text-text-secondary flex items-start justify-between gap-2">
238308
<div className="min-w-0 pt-1">
@@ -284,6 +354,7 @@ function AskUserQuestionSingle({
284354
setAnswerValue(e.target.value);
285355
setAskAnswerDraft(actionId, e.target.value);
286356
}}
357+
onKeyDown={handleAnswerKeyDown}
287358
minRows={2}
288359
maxRows={12}
289360
placeholder={otherLabel ?? localize('com_ui_your_answer')}

0 commit comments

Comments
 (0)