From f9876eaaf0d5a947deaa42065cbeef3faccb7d39 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 17 Aug 2026 12:16:19 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=9C=20style:=20Step=20Through=20Batche?= =?UTF-8?q?d=20Questions=20One=20at=20a=20Time=20(#14935)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A batched `ask_user_question` interrupt rendered every question stacked in one scrolling form, which reads as a wall on mobile and desktop alike. Show one question per step instead, with clickable progress dots, Back/Next, and Submit only on the last step. The batch contract is untouched: one interrupt, one answer map, Submit still gated on every question having an answer, Skip still declines the whole batch from any step. Single-question batches render exactly as before. --- .../Messages/Content/AskUserQuestions.tsx | 306 +++++++++++++----- .../__tests__/AskUserQuestions.test.tsx | 203 ++++++++++-- client/src/hooks/Input/useAskQuestionsForm.ts | 23 +- client/src/locales/en/translation.json | 6 + 4 files changed, 431 insertions(+), 107 deletions(-) diff --git a/client/src/components/Chat/Messages/Content/AskUserQuestions.tsx b/client/src/components/Chat/Messages/Content/AskUserQuestions.tsx index 9f09c4e58a..85ed86a268 100644 --- a/client/src/components/Chat/Messages/Content/AskUserQuestions.tsx +++ b/client/src/components/Chat/Messages/Content/AskUserQuestions.tsx @@ -1,3 +1,4 @@ +import { useCallback, useEffect, useId, useMemo, useRef } from 'react'; import { Button, TextareaAutosize } from '@librechat/client'; import { Check, ChevronUp, TriangleAlert } from 'lucide-react'; import type { Agents } from 'librechat-data-provider'; @@ -6,6 +7,12 @@ import { splitOtherOption } from '~/utils/approval'; import { useLocalize } from '~/hooks'; import { cn } from '~/utils'; +/** + * One bounded batch of `ask_user_question` items, presented a single question at + * a time. The batch arrives as one interrupt and submits as one answer map — the + * stepper is purely presentational, so Submit still waits until every question + * has an answer and Skip still declines the whole batch from any step. + */ export default function AskUserQuestions({ actionId, questions, @@ -18,12 +25,81 @@ export default function AskUserQuestions({ onExpand?: () => void; }) { const localize = useLocalize(); + const promptId = useId(); const form = useAskQuestionsForm(actionId, questions); + const { goToStep, selectOption } = form; + + const scrollRef = useRef(null); + const stepRef = useRef(null); + /** Set only when a choice click is about to unmount the button that owns + * focus, which would otherwise drop focus to mid-batch. */ + const refocusRef = useRef(false); + + const total = questions.length; + const stepped = total > 1; + const activeIndex = form.step; + const isLastStep = activeIndex === total - 1; + /** Narrower than `locked`: an expired or errored batch is unanswerable but + * still worth paging through, so only an in-flight submit freezes the steps. */ + const navLocked = form.status === 'submitting'; + + const firstUnanswered = useMemo(() => { + for (let index = 0; index < questions.length; index++) { + if (!Object.hasOwn(form.answers, questions[index].id)) { + return index; + } + } + return -1; + }, [questions, form.answers]); + + const handleSelectOption = useCallback( + (question: Agents.AskUserQuestionBatchItem, value: string) => { + selectOption(question, value); + if (question.multiSelect === true || activeIndex >= total - 1) { + return; + } + refocusRef.current = true; + goToStep(activeIndex + 1); + }, + [activeIndex, goToStep, selectOption, total], + ); + + useEffect(() => { + if (scrollRef.current != null) { + scrollRef.current.scrollTop = 0; + } + if (!refocusRef.current) { + return; + } + refocusRef.current = false; + stepRef.current?.focus(); + }, [activeIndex]); if (form.status === 'submitted') { return null; } + const question = questions[activeIndex]; + if (question == null) { + return null; + } + + const { choices, otherLabel } = splitOtherOption(question.options); + const selected = Object.hasOwn(form.state.selected, question.id) + ? form.state.selected[question.id] + : []; + const text = Object.hasOwn(form.state.text, question.id) ? form.state.text[question.id] : ''; + const legend = question.header ?? (stepped ? null : localize('com_ui_question_number', { 0: 1 })); + /** Only worth surfacing when the gap is somewhere the user cannot see: the + * last step's own blank textarea already explains a disabled Submit. */ + const remaining = total - Object.keys(form.answers).length; + const showRemaining = + stepped && + isLastStep && + !form.locked && + firstUnanswered >= 0 && + firstUnanswered !== activeIndex; + return (
{onExpand != null && ( @@ -38,70 +114,108 @@ export default function AskUserQuestions({
)} -
- {questions.map((question, questionIndex) => { - const { choices, otherLabel } = splitOtherOption(question.options); - const selected = Object.hasOwn(form.state.selected, question.id) - ? form.state.selected[question.id] - : []; - const text = Object.hasOwn(form.state.text, question.id) - ? form.state.text[question.id] - : ''; - return ( -
0 && 'border-t border-border-light')} - > - - {question.header ?? localize('com_ui_question_number', { 0: questionIndex + 1 })} - -

- {question.question} -

- {question.description != null && question.description.length > 0 && ( -

- {question.description} -

- )} - {choices.length > 0 && ( -
- {choices.map((option) => { - const isSelected = selected.includes(option.value); - return ( - - ); - })} -
- )} - form.setText(question, event.target.value)} - minRows={1} - maxRows={6} - placeholder={otherLabel ?? localize('com_ui_your_answer')} - className="mt-2 w-full resize-none rounded-md border border-border-light bg-surface-primary p-2 text-sm text-text-primary" - aria-label={`${question.question} ${localize('com_ui_your_answer')}`} - /> -
- ); - })} + {stepped && ( +
+

+ {localize('com_ui_question_step', { 0: activeIndex + 1, 1: total })} +

+
+ {questions.map((item, index) => { + const isAnswered = Object.hasOwn(form.answers, item.id); + const isActive = index === activeIndex; + return ( + + ); + })} +
+
+ )} +
+ {/* The height floor sits on the step itself, not the scroll container: + the container needs `min-h-0` to shrink inside the flex column, and + tailwind-merge would drop it for a second `min-h-*`. */} +
+ {legend != null && ( + {legend} + )} +

+ {question.question} +

+ {question.description != null && question.description.length > 0 && ( +

+ {question.description} +

+ )} + {choices.length > 0 && ( +
+ {choices.map((option) => { + const isSelected = selected.includes(option.value); + return ( + + ); + })} +
+ )} + form.setText(question, event.target.value)} + minRows={1} + maxRows={6} + placeholder={otherLabel ?? localize('com_ui_your_answer')} + className="mt-2 w-full resize-none rounded-md border border-border-light bg-surface-primary p-2 text-sm text-text-primary" + aria-label={`${question.question} ${localize('com_ui_your_answer')}`} + /> +
{(form.status === 'error' || form.status === 'expired') && (
@@ -111,7 +225,24 @@ export default function AskUserQuestions({ : localize('com_ui_ask_answer_error')}
)} -
+ {showRemaining && ( + + )} +
- +
+ {stepped && ( + + )} + {isLastStep ? ( + + ) : ( + + )} +
); diff --git a/client/src/components/Chat/Messages/Content/__tests__/AskUserQuestions.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/AskUserQuestions.test.tsx index 3c19cb2c4f..aa17d8b05b 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/AskUserQuestions.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/AskUserQuestions.test.tsx @@ -1,12 +1,15 @@ import React from 'react'; import { RecoilRoot } from 'recoil'; import { fireEvent, render, screen } from '@testing-library/react'; +import type { Agents } from 'librechat-data-provider'; +import { ASK_USER_DECLINED_ANSWER } from '~/utils/approval'; import AskUserQuestions from '../AskUserQuestions'; const mockSubmitAskAnswer = jest.fn(); +let mockStatus = 'idle'; jest.mock('~/components/Chat/Messages/Content/ApprovalContext', () => ({ - useAskSubmitStatus: () => ({ getAskStatus: () => 'idle' }), + useAskSubmitStatus: () => ({ getAskStatus: () => mockStatus }), useResumeSubmit: () => ({ submitAskAnswer: mockSubmitAskAnswer }), })); @@ -14,7 +17,15 @@ jest.mock('~/hooks', () => ({ useLocalize: () => (key: string, values?: Record) => { const labels: Record = { com_ui_question_number: `Question ${values?.[0] ?? ''}`, + com_ui_question_step: `Question ${values?.[0] ?? ''} of ${values?.[1] ?? ''}`, + com_ui_question_step_answered: `Go to question ${values?.[0] ?? ''}, answered`, + com_ui_question_step_unanswered: `Go to question ${values?.[0] ?? ''}, not answered`, + com_ui_question_navigation: 'Question navigation', + com_ui_questions_remaining_one: `${values?.[0] ?? ''} question still needs an answer`, + com_ui_questions_remaining: `${values?.[0] ?? ''} questions still need an answer`, com_ui_your_answer: 'Your answer', + com_ui_back: 'Back', + com_ui_next: 'Next', com_ui_skip: 'Skip', com_ui_submit: 'Submit', com_ui_submitting: 'Submitting', @@ -23,7 +34,7 @@ jest.mock('~/hooks', () => ({ }, })); -const questions = [ +const questions: Agents.AskUserQuestionBatchItem[] = [ { id: 'environment', header: 'Environment', @@ -36,23 +47,78 @@ const questions = [ { id: 'window', question: 'Which time window?' }, ]; +const renderBatch = (actionId: string, batch: Agents.AskUserQuestionBatchItem[] = questions) => + render( + + + , + ); + describe('AskUserQuestions', () => { - beforeEach(() => mockSubmitAskAnswer.mockClear()); + beforeEach(() => { + mockStatus = 'idle'; + mockSubmitAskAnswer.mockClear(); + }); + + test('shows one question at a time and walks the batch with Next/Back', () => { + renderBatch('ask-steps'); + + expect(screen.getByText('Where should this run?')).toBeInTheDocument(); + expect(screen.queryByText('Which time window?')).not.toBeInTheDocument(); + expect(screen.getByText('Question 1 of 2')).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: 'Submit' })).not.toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'Next' })); + + expect(screen.getByText('Which time window?')).toBeInTheDocument(); + expect(screen.queryByText('Where should this run?')).not.toBeInTheDocument(); + expect(screen.getByText('Question 2 of 2')).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: 'Next' })).not.toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'Back' })); + + expect(screen.getByText('Where should this run?')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Back' })).toBeDisabled(); + }); + + test('advances automatically when a single-select choice is picked', () => { + renderBatch('ask-advance'); + + fireEvent.click(screen.getByRole('button', { name: 'Staging' })); + + expect(screen.getByText('Which time window?')).toBeInTheDocument(); + expect(screen.getByText('Question 2 of 2')).toBeInTheDocument(); + }); + + test('does not auto-advance a multi-select question', () => { + renderBatch('ask-multi', [ + { + id: 'regions', + question: 'Which regions?', + multiSelect: true, + options: [ + { label: 'us-east', value: 'us-east' }, + { label: 'eu-west', value: 'eu-west' }, + ], + }, + { id: 'window', question: 'Which time window?' }, + ]); + + fireEvent.click(screen.getByRole('checkbox', { name: 'us-east' })); + + expect(screen.getByText('Which regions?')).toBeInTheDocument(); + expect(screen.getByText('Question 1 of 2')).toBeInTheDocument(); + }); test('submits one answer map after every question is complete', () => { - render( - - - , - ); - - const submit = screen.getByRole('button', { name: 'Submit' }); - expect(submit).toBeDisabled(); + renderBatch('ask-batch'); fireEvent.click(screen.getByRole('button', { name: 'Staging' })); fireEvent.change(screen.getByRole('textbox', { name: /Which time window/ }), { target: { value: 'Last seven days' }, }); + + const submit = screen.getByRole('button', { name: 'Submit' }); expect(submit).toBeEnabled(); fireEvent.click(submit); @@ -66,12 +132,45 @@ describe('AskUserQuestions', () => { ); }); - test('retains partial answers across surface remounts', () => { - const view = render( - - - , - ); + test('keeps Submit gated on the last step until every question is answered', () => { + renderBatch('ask-gated'); + + fireEvent.click(screen.getByRole('button', { name: 'Next' })); + expect(screen.getByRole('button', { name: 'Submit' })).toBeDisabled(); + expect( + screen.getByRole('button', { name: '2 questions still need an answer' }), + ).toBeInTheDocument(); + + fireEvent.change(screen.getByRole('textbox', { name: /Which time window/ }), { + target: { value: 'Today' }, + }); + expect(screen.getByRole('button', { name: 'Submit' })).toBeDisabled(); + + fireEvent.click(screen.getByRole('button', { name: '1 question still needs an answer' })); + expect(screen.getByText('Where should this run?')).toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'Staging' })); + expect(screen.getByRole('button', { name: 'Submit' })).toBeEnabled(); + }); + + test('jumps to any question from the step dots and marks answered ones', () => { + renderBatch('ask-dots'); + + fireEvent.click(screen.getByRole('button', { name: 'Go to question 2, not answered' })); + expect(screen.getByText('Which time window?')).toBeInTheDocument(); + + fireEvent.change(screen.getByRole('textbox', { name: /Which time window/ }), { + target: { value: 'Today' }, + }); + expect(screen.getByRole('button', { name: 'Go to question 2, answered' })).toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'Go to question 1, not answered' })); + expect(screen.getByText('Where should this run?')).toBeInTheDocument(); + }); + + test('retains partial answers and the current step across surface remounts', () => { + const view = renderBatch('ask-remount'); + fireEvent.click(screen.getByRole('button', { name: 'Next' })); fireEvent.change(screen.getByRole('textbox', { name: /Which time window/ }), { target: { value: 'Today' }, }); @@ -82,24 +181,37 @@ describe('AskUserQuestions', () => { , ); + expect(screen.getByText('Question 2 of 2')).toBeInTheDocument(); expect(screen.getByRole('textbox', { name: /Which time window/ })).toHaveValue('Today'); }); - test('supports question ids inherited by ordinary objects', () => { - render( - - - , + test('leaves a single-question batch free of stepper chrome', () => { + renderBatch('ask-single', [ + { id: 'confirmation', question: 'Continue?', options: [{ label: 'Yes', value: 'yes' }] }, + ]); + + expect(screen.queryByRole('button', { name: 'Next' })).not.toBeInTheDocument(); + expect(screen.queryByRole('button', { name: 'Back' })).not.toBeInTheDocument(); + expect(screen.queryByRole('group', { name: 'Question navigation' })).not.toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'Yes' })); + fireEvent.click(screen.getByRole('button', { name: 'Submit' })); + + expect(mockSubmitAskAnswer).toHaveBeenCalledWith( + 'ask-single', + { confirmation: 'yes' }, + expect.objectContaining({ onSuccess: expect.any(Function) }), ); + }); + + test('supports question ids inherited by ordinary objects', () => { + renderBatch('ask-prototype-id', [ + { + id: 'constructor', + question: 'Continue?', + options: [{ label: 'Yes', value: 'yes' }], + }, + ]); fireEvent.click(screen.getByRole('button', { name: 'Yes' })); fireEvent.click(screen.getByRole('button', { name: 'Submit' })); @@ -110,4 +222,33 @@ describe('AskUserQuestions', () => { expect.objectContaining({ onSuccess: expect.any(Function) }), ); }); + + test('keeps an expired batch readable but freezes steps mid-submit', () => { + mockStatus = 'expired'; + const view = renderBatch('ask-expired'); + + fireEvent.click(screen.getByRole('button', { name: 'Next' })); + expect(screen.getByText('Which time window?')).toBeInTheDocument(); + + mockStatus = 'submitting'; + view.rerender( + + + , + ); + expect(screen.getByRole('button', { name: 'Back' })).toBeDisabled(); + }); + + test('skips the whole batch from any step', () => { + renderBatch('ask-skip'); + + fireEvent.click(screen.getByRole('button', { name: 'Next' })); + fireEvent.click(screen.getByRole('button', { name: 'Skip' })); + + expect(mockSubmitAskAnswer).toHaveBeenCalledWith( + 'ask-skip', + { environment: ASK_USER_DECLINED_ANSWER, window: ASK_USER_DECLINED_ANSWER }, + expect.objectContaining({ onSuccess: expect.any(Function) }), + ); + }); }); diff --git a/client/src/hooks/Input/useAskQuestionsForm.ts b/client/src/hooks/Input/useAskQuestionsForm.ts index b0c0d897f5..f4f4b21d8c 100644 --- a/client/src/hooks/Input/useAskQuestionsForm.ts +++ b/client/src/hooks/Input/useAskQuestionsForm.ts @@ -8,13 +8,17 @@ import { import { ASK_USER_DECLINED_ANSWER } from '~/utils/approval'; interface AskQuestionsFormState { + /** Index of the question currently on screen. Lives beside the answers so the + * composer popover and the chat card resume on the same step when one + * surface hands over to the other. */ + step: number; text: Record; selected: Record; } const askQuestionsFormState = atomFamily({ key: 'askQuestionsFormState', - default: { text: {}, selected: {} }, + default: { step: 0, text: {}, selected: {} }, }); function ownValue(record: Record, key: string): T | undefined { @@ -35,6 +39,7 @@ export default function useAskQuestionsForm( const setText = useCallback( (question: Agents.AskUserQuestionBatchItem, value: string) => { setState((previous) => ({ + ...previous, text: { ...previous.text, [question.id]: value }, selected: question.multiSelect === true || value.length === 0 @@ -56,6 +61,7 @@ export default function useAskQuestionsForm( : [...current, value]; } return { + ...previous, text: question.multiSelect === true ? previous.text : { ...previous.text, [question.id]: '' }, selected: { ...previous.selected, [question.id]: selected }, @@ -65,6 +71,16 @@ export default function useAskQuestionsForm( [setState], ); + const goToStep = useCallback( + (index: number) => { + const bounded = Math.min(Math.max(index, 0), Math.max(questions.length - 1, 0)); + setState((previous) => + previous.step === bounded ? previous : { ...previous, step: bounded }, + ); + }, + [questions.length, setState], + ); + const answers = useMemo(() => { const resolved = Object.create(null) as Record; for (const question of questions) { @@ -81,7 +97,7 @@ export default function useAskQuestionsForm( } } return resolved; - }, [questions, state]); + }, [questions, state.text, state.selected]); const canSubmit = !locked && questions.every((question) => (ownValue(answers, question.id)?.length ?? 0) > 0); @@ -107,11 +123,14 @@ export default function useAskQuestionsForm( return { state, + step: Math.min(state.step, Math.max(questions.length - 1, 0)), status, locked, + answers, canSubmit, setText, selectOption, + goToStep, submit, skip, }; diff --git a/client/src/locales/en/translation.json b/client/src/locales/en/translation.json index dfd4132ec6..ec08f6bf40 100644 --- a/client/src/locales/en/translation.json +++ b/client/src/locales/en/translation.json @@ -1741,6 +1741,12 @@ "com_ui_question_failed": "Question wasn't shown", "com_ui_question_failed_description": "The agent couldn't show this question and may retry automatically.", "com_ui_question_number": "Question {{0}}", + "com_ui_question_step": "Question {{0}} of {{1}}", + "com_ui_question_navigation": "Question navigation", + "com_ui_question_step_answered": "Go to question {{0}}, answered", + "com_ui_question_step_unanswered": "Go to question {{0}}, not answered", + "com_ui_questions_remaining_one": "{{0}} question still needs an answer", + "com_ui_questions_remaining": "{{0}} questions still need an answer", "com_ui_answer_questions_above": "Answer the questions above", "com_ui_asking_questions_one": "Asking {{0}} question", "com_ui_asking_questions": "Asking {{0}} questions",