🙋 feat: Collapse Settled Question Records by Default (#15107)

* 🙋 feat: Collapse Settled Question Records by Default

The durable `ask_user_question` record rendered as a permanently open
card. Answers are frequently long, multi-paragraph text, so a settled
Q&A buried the reply that followed it.

It now reads as one collapsed tool-call line — the same `ProgressText`
primitive `ToolCall`/`SkillCall` use — naming the question (or the
batch count, reusing the keys `ToolCallGroup` already had) and opening
on demand under the existing `autoExpandTools` preference. Only the
settled record collapses; the live pause and the interim progress card
are untouched.

The expanded panel was also hard to read. Authored text rendered
without `pre-wrap`, so a numbered or paragraphed answer collapsed into
one wall; the answer ran on from its inline label; and batch items sat
flush against their divider. Line breaks are now content, the answer
sits under its own label behind a rule, and dividers have air on both
sides.

`ProgressText`'s subtitle now truncates and absorbs the flex shrink, so
arbitrary authored text ellipsizes instead of pushing the line past the
message column — this also fixes long MCP server names on tool cards.

* 🩹 fix: Address Codex Round 1 on the Collapsed Question Record

- Settle the summary tense. A live, unanswered pause returns before the
  header, so every state reaching it is settled — an abandoned pause read
  "Asking" forever, and the collapse hid the "no answer" line that used to
  qualify it. Past tense unconditionally, matching `ToolCallGroup`.

- Move the rejection announcement out of the disclosure. `useExpandCollapse`
  marks the closed panel `inert`, so the failure explanation's `role="status"`
  could never reach the accessibility tree; it is now an sr-only status
  outside the panel, carrying both the label and the explanation.

- Count records, not repeated text, in the Bombadil observation. With
  Auto-expand tool details on, one settled record shows the question in both
  its summary line and its panel, so the old selector double-counted it and
  broke the `<= 1` singularity invariant.
This commit is contained in:
Danny Avila 2026-08-21 19:50:40 -04:00 committed by GitHub
parent 8ae94afa91
commit 21b7f78d56
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 313 additions and 131 deletions

View file

@ -1,3 +1,5 @@
import { useState, useEffect, useCallback } from 'react';
import { useRecoilValue } from 'recoil';
import { MessageCircleQuestion, TriangleAlert } from 'lucide-react';
import type { Agents } from 'librechat-data-provider';
import {
@ -6,9 +8,11 @@ import {
parseAskUserQuestionsArgs,
} from '~/utils/approval';
import AskUserQuestionProgress from './AskUserQuestionProgress';
import { useLocalize, useExpandCollapse } from '~/hooks';
import ProgressText from './ProgressText';
import EmptyText from './Parts/EmptyText';
import { useLocalize } from '~/hooks';
import Container from './Container';
import store from '~/store';
/**
* Static rendering of a COMPLETED (or abandoned) `ask_user_question` tool call
@ -16,6 +20,12 @@ import Container from './Container';
* is wrong here: it labels a no-output call "cancelled" and shows raw JSON args.
* The interactive card ({@link AskUserQuestion}) renders only while the pause is
* live; this component owns the part everywhere else (history, reload, exports).
*
* Settled, it is history so it reads as one collapsed tool-call line (status
* label plus the question itself) and opens on demand, under the same
* `autoExpandTools` preference every other tool card follows. Answers are
* frequently long, multi-paragraph text; left expanded they buried the reply
* that followed them.
*/
export default function AskUserQuestionCall({
args,
@ -24,6 +34,7 @@ export default function AskUserQuestionCall({
isSubmitting = false,
failed = false,
showCursor = false,
onExpand,
}: {
args: string | Record<string, unknown> | undefined;
output: string;
@ -31,8 +42,29 @@ export default function AskUserQuestionCall({
isSubmitting?: boolean;
failed?: boolean;
showCursor?: boolean;
onExpand?: () => void;
}) {
const localize = useLocalize();
const autoExpand = useRecoilValue(store.autoExpandTools);
const [expanded, setExpanded] = useState(autoExpand);
const { style: expandStyle, ref: expandRef } = useExpandCollapse(expanded);
useEffect(() => {
if (autoExpand) {
setExpanded(true);
}
}, [autoExpand]);
const toggleExpanded = useCallback(() => {
setExpanded((prev) => {
const next = !prev;
if (next) {
onExpand?.();
}
return next;
});
}, [onExpand]);
const question = parseAskUserQuestionArgs(args);
const batch = parseAskUserQuestionsArgs(args);
/**
@ -88,137 +120,169 @@ export default function AskUserQuestionCall({
</Container>
) : null;
if (batch != null) {
let statusLabel = localize('com_ui_asking');
const count = batch?.questions.length ?? 1;
/**
* Past tense unconditionally: a live, unanswered pause returns above, so
* every state that reaches this header is settled answered, abandoned
* (the run stopped before an answer), or rejected. An abandoned pause was
* still ASKED; it explains itself with "no answer" inside the panel, and a
* present-tense summary would strand it as permanently in-flight now that
* the panel starts closed. Matches `ToolCallGroup`, which settles its own
* question header on `!isSubmitting`.
*/
const statusLabel = (() => {
if (failed) {
statusLabel = localize('com_ui_question_failed');
} else if (answered) {
statusLabel = localize('com_ui_asked');
return localize('com_ui_question_failed');
}
return (
<>
<div className="my-2 flex w-full flex-col rounded-lg border border-border-light bg-surface-secondary">
<div className="flex items-center gap-2 px-3 py-2 text-xs font-medium text-text-secondary">
{failed ? (
<TriangleAlert className="h-4 w-4 text-text-warning" aria-hidden="true" />
return count > 1
? localize('com_ui_asked_n_questions', { 0: String(count) })
: localize('com_ui_asked');
})();
/** A batch is summarized by its count; a lone question is summarized by
* itself, so the collapsed line still says what was asked. */
const summary = count > 1 ? undefined : (batch?.questions[0]?.question ?? question?.question);
return (
<>
<div
className="relative my-1.5 flex h-5 shrink-0 items-center gap-2.5"
data-testid="ask-user-question-call"
>
<ProgressText
phase="completed"
onClick={toggleExpanded}
inProgressText={statusLabel}
finishedText={statusLabel}
subtitle={summary}
icon={
failed ? (
<TriangleAlert className="size-4 shrink-0 text-text-warning" aria-hidden="true" />
) : (
<MessageCircleQuestion className="h-4 w-4" aria-hidden="true" />
)}
{statusLabel}
</div>
<div className="px-3 pb-3">
{batch.questions.map((item, index) => {
const answer = batchAnswers?.[item.id];
return (
<div key={item.id} className={index > 0 ? 'border-t border-border-light pt-3' : ''}>
<MessageCircleQuestion
className="size-4 shrink-0 text-text-secondary"
aria-hidden="true"
/>
)
}
isExpanded={expanded}
/>
</div>
{/* A rejected call is the one state the reader did not ask for and
cannot see coming. The explanation lives in the panel, which starts
closed and is `inert` while it is so the announcement has to sit
outside the disclosure to reach the accessibility tree at all. */}
{failed && (
<span className="sr-only" role="status">
{`${statusLabel}. ${localize('com_ui_question_failed_description')}`}
</span>
)}
<div style={expandStyle}>
<div className="overflow-hidden" ref={expandRef}>
<div className="my-2 flex w-full flex-col gap-4 rounded-lg border border-border-light bg-surface-secondary p-4">
{batch != null ? (
batch.questions.map((item, index) => (
<div
key={item.id}
className={index > 0 ? 'border-t border-border-light pt-4' : undefined}
>
{item.header != null && (
<p className="text-xs font-medium text-text-secondary">{item.header}</p>
)}
<p className="text-sm font-medium text-text-primary [overflow-wrap:anywhere]">
{item.question}
</p>
{item.description != null && (
<p className="mt-1 text-sm text-text-secondary [overflow-wrap:anywhere]">
{item.description}
</p>
)}
{typeof answer === 'string' && (
<p className="mt-1 text-sm text-text-primary [overflow-wrap:anywhere]">
<span className="font-medium text-text-secondary">
{localize('com_ui_you_answered')}
</span>{' '}
{formatAnswerLabel(item, answer)}
</p>
)}
{typeof answer !== 'string' && !failed && (
<p className="mt-1 text-sm italic text-text-secondary">
{localize('com_ui_question_unanswered')}
</p>
<p className="mb-1 text-xs font-medium text-text-secondary">{item.header}</p>
)}
<QuestionBody
question={item.question}
description={item.description}
answer={batchAnswers?.[item.id]}
options={item.options}
multiSelect={item.multiSelect}
failed={failed}
/>
</div>
);
})}
))
) : (
<QuestionBody
question={question?.question ?? ''}
description={question?.description}
answer={answered ? effectiveOutput : undefined}
options={question?.options}
multiSelect={question?.multiSelect}
failed={failed}
/>
)}
{failed && (
<p className="mt-3 text-sm text-text-secondary">
<p className="text-sm leading-relaxed text-text-secondary">
{localize('com_ui_question_failed_description')}
</p>
)}
</div>
</div>
{resumingCursor}
</>
);
}
if (failed) {
return (
<>
<div
className="my-2 flex w-full flex-col gap-1.5 rounded-lg border border-border-light bg-surface-secondary p-3"
role="status"
>
<div className="flex items-center gap-2 text-xs font-medium text-text-warning">
<TriangleAlert className="h-4 w-4" aria-hidden="true" />
{localize('com_ui_question_failed')}
</div>
{question?.question != null && (
<p className="text-sm font-medium text-text-primary [overflow-wrap:anywhere]">
{question.question}
</p>
)}
<p className="text-sm text-text-secondary">
{localize('com_ui_question_failed_description')}
</p>
</div>
{resumingCursor}
</>
);
}
/**
* Prefer the picked option's label over its wire value when they differ.
* Multi-select answers are option values joined by ", " map the segments
* back to labels only when EVERY segment matches an option: values may
* legally contain ", " themselves, and a partial mapping could mis-split
* such a value into fragments that relabel as options the user never
* picked. When any segment misses, show the raw answer untouched.
*/
const answerLabel =
question == null ? effectiveOutput : formatAnswerLabel(question, effectiveOutput);
return (
<>
<div className="my-2 flex w-full flex-col gap-1.5 rounded-lg border border-border-light bg-surface-secondary p-3">
<div className="flex items-center gap-2 text-xs font-medium text-text-secondary">
<MessageCircleQuestion className="h-4 w-4" aria-hidden="true" />
{answered ? localize('com_ui_asked') : localize('com_ui_asking')}
</div>
<p className="text-sm font-medium text-text-primary [overflow-wrap:anywhere]">
{question?.question ?? (answered ? localize('com_ui_asked') : localize('com_ui_asking'))}
</p>
{question?.description != null && question.description.length > 0 && (
<p className="text-sm text-text-secondary [overflow-wrap:anywhere]">
{question.description}
</p>
)}
{answered ? (
<p className="text-sm text-text-primary [overflow-wrap:anywhere]">
<span className="font-medium text-text-secondary">
{localize('com_ui_you_answered')}
</span>{' '}
{answerLabel}
</p>
) : (
<p className="text-sm italic text-text-secondary">
{localize('com_ui_question_unanswered')}
</p>
)}
</div>
{resumingCursor}
</>
);
}
/**
* One question and its answer. Every text slot here is authored by the model
* or by the user so line breaks are content, not whitespace: `pre-wrap`
* keeps a numbered or paragraphed answer legible instead of collapsing it into
* one wall of text. The answer sits under its own label behind a rule rather
* than running on from it, so the eye can find where the reply starts.
*/
function QuestionBody({
question,
description,
answer,
options,
multiSelect,
failed,
}: {
question: string;
description?: string;
answer?: string;
options?: Agents.AskUserQuestionRequest['options'];
multiSelect?: boolean;
failed: boolean;
}) {
const localize = useLocalize();
return (
<div className="min-w-0">
{question.length > 0 && (
<p className="whitespace-pre-wrap text-sm font-medium leading-relaxed text-text-primary [overflow-wrap:anywhere]">
{question}
</p>
)}
{description != null && description.length > 0 && (
<p className="mt-1 whitespace-pre-wrap text-sm leading-relaxed text-text-secondary [overflow-wrap:anywhere]">
{description}
</p>
)}
{typeof answer === 'string' && (
<div className="mt-2.5 border-l-2 border-border-medium pl-3">
<p className="text-xs font-medium text-text-secondary">
{localize('com_ui_you_answered')}
</p>
<p className="mt-0.5 whitespace-pre-wrap text-sm leading-relaxed text-text-primary [overflow-wrap:anywhere]">
{formatAnswerLabel({ question, options, multiSelect }, answer)}
</p>
</div>
)}
{typeof answer !== 'string' && !failed && (
<p className="mt-2.5 text-sm italic text-text-secondary">
{localize('com_ui_question_unanswered')}
</p>
)}
</div>
);
}
/**
* Prefer the picked option's label over its wire value when they differ.
* Multi-select answers are option values joined by ", " map the segments
* back to labels only when EVERY segment matches an option: values may
* legally contain ", " themselves, and a partial mapping could mis-split
* such a value into fragments that relabel as options the user never
* picked. When any segment misses, show the raw answer untouched.
*/
function formatAnswerLabel(question: Agents.AskUserQuestionRequest, answer: string): string {
const exactLabel = question.options?.find((option) => option.value === answer)?.label;
if (exactLabel != null || question.multiSelect !== true || question.options == null) {

View file

@ -255,6 +255,7 @@ const Part = memo(function Part({
isSubmitting={isSubmitting}
showCursor={showCursor}
failed={'inputValidationError' in toolCall && toolCall.inputValidationError === true}
onExpand={onToolExpand}
/>
);
} else if (toolCall.name === 'skill') {

View file

@ -117,7 +117,15 @@ export default function ProgressText({
<span className={cn(showShimmer ? 'shimmer' : '', 'min-w-0 truncate font-medium')}>
{text}
</span>
{subtitle && <span className="font-normal text-text-secondary">{subtitle}</span>}
{/* The label names the card and stays whole; a subtitle can be
arbitrary authored text (a question, a server name), so it takes
essentially all of the shrink and ellipsizes instead of pushing
the line past the message column. */}
{subtitle && (
<span className="min-w-0 shrink-[100] truncate font-normal text-text-secondary">
{subtitle}
</span>
)}
{errorSuffix && <span className="font-normal text-status-error">· {errorSuffix}</span>}
{duration && (
<>

View file

@ -1,6 +1,8 @@
import React from 'react';
import { render, screen } from '@testing-library/react';
import { RecoilRoot } from 'recoil';
import { fireEvent, render, screen } from '@testing-library/react';
import AskUserQuestionCall from '../AskUserQuestionCall';
import store from '~/store';
const translations: Record<string, string> = {
com_ui_asked: 'Asked',
@ -10,10 +12,19 @@ const translations: Record<string, string> = {
"The agent couldn't show this question and may retry automatically.",
com_ui_question_unanswered: 'No answer was given',
com_ui_you_answered: 'You answered:',
com_ui_asked_n_questions: 'Asked {{0}} questions',
com_ui_asking_n_questions: 'Asking {{0}} questions',
};
jest.mock('~/hooks', () => ({
useLocalize: () => (key: string) => translations[key] ?? key,
useLocalize: () => (key: string, values?: Record<string, string>) => {
const template = translations[key] ?? key;
return values == null
? template
: template.replace(/\{\{(\w+)\}\}/g, (_match, name: string) => values[name] ?? '');
},
/** The disclosure is the behaviour under test — keep the real hook. */
useExpandCollapse: jest.requireActual('~/hooks/Messages/useExpandCollapse').default,
}));
jest.mock('~/utils/approval', () => ({
@ -46,6 +57,14 @@ jest.mock('../Container', () => ({
},
}));
/** Settled records mount collapsed, exactly like every other tool card. */
const renderCall = (ui: React.ReactElement, autoExpand = false) =>
render(
<RecoilRoot initializeState={({ set }) => set(store.autoExpandTools, autoExpand)}>
{ui}
</RecoilRoot>,
);
describe('AskUserQuestionCall', () => {
const args = JSON.stringify({
question: 'How would you like me to get the data?',
@ -53,21 +72,93 @@ describe('AskUserQuestionCall', () => {
});
test('renders the progress card while the call is live and unanswered', () => {
render(<AskUserQuestionCall args={args} output="" toolCallId="call_1" isSubmitting />);
renderCall(<AskUserQuestionCall args={args} output="" toolCallId="call_1" isSubmitting />);
expect(screen.getByTestId('ask-progress')).toBeInTheDocument();
expect(screen.queryByText('You answered:')).not.toBeInTheDocument();
});
test('mounts collapsed and opens on click, like any other tool card', () => {
renderCall(<AskUserQuestionCall args={args} output="public" />);
const header = screen.getByRole('button');
expect(header).toHaveAttribute('aria-expanded', 'false');
/** The collapsed line still says what was asked. */
expect(header).toHaveTextContent('Asked');
expect(header).toHaveTextContent('How would you like me to get the data?');
fireEvent.click(header);
expect(header).toHaveAttribute('aria-expanded', 'true');
});
test('opens at mount when auto-expand is on', () => {
renderCall(<AskUserQuestionCall args={args} output="public" />, true);
expect(screen.getByRole('button')).toHaveAttribute('aria-expanded', 'true');
});
test('summarizes a batch by its question count', () => {
renderCall(
<AskUserQuestionCall
args={JSON.stringify({
questions: [
{ id: 'environment', question: 'Where should this run?' },
{ id: 'window', question: 'Which window?' },
],
})}
output={JSON.stringify({ answers: { environment: 'staging', window: '7d' } })}
/>,
);
expect(screen.getByRole('button')).toHaveTextContent('Asked 2 questions');
});
test('keeps authored line breaks in a multi-paragraph answer', () => {
const answer = 'First point\n\nSecond point';
renderCall(<AskUserQuestionCall args={args} output={answer} />);
/** Identity normalizer: the default one collapses the very newlines
* this asserts are preserved. */
expect(screen.getByText(answer, { normalizer: (text) => text })).toHaveClass(
'whitespace-pre-wrap',
);
});
test('reads as settled when a run is stopped before the question is answered', () => {
renderCall(<AskUserQuestionCall args={args} output="" toolCallId="call_1" />);
const header = screen.getByRole('button');
/** The pause ended; only its panel says the answer never came, and that
* panel starts closed a present-tense summary would strand the record
* as permanently in flight. */
expect(header).toHaveTextContent('Asked');
expect(header).not.toHaveTextContent('Asking');
expect(screen.getByText('No answer was given')).toBeInTheDocument();
});
test('announces a rejected question from outside the collapsed panel', () => {
renderCall(<AskUserQuestionCall args={args} output="Error processing tool" failed />);
const announcement = screen.getByRole('status');
expect(announcement).toHaveTextContent("Question wasn't shown");
expect(announcement).toHaveTextContent(
"The agent couldn't show this question and may retry automatically.",
);
/** `useExpandCollapse` marks the closed panel inert, so an announcement
* inside it would never reach the accessibility tree. */
expect(announcement.closest('[inert]')).toBeNull();
});
test('renders a successful tool result as the user answer', () => {
render(<AskUserQuestionCall args={args} output="public" />);
renderCall(<AskUserQuestionCall args={args} output="public" />);
expect(screen.getByText('You answered:')).toBeInTheDocument();
expect(screen.getByText('Use public data')).toBeInTheDocument();
});
test('holds the streaming cursor under the answered card while the resume is in flight', () => {
const { container } = render(
const { container } = renderCall(
<AskUserQuestionCall
args={args}
output="public"
@ -82,10 +173,10 @@ describe('AskUserQuestionCall', () => {
});
test('shows no cursor once the record is not the streaming tail', () => {
const settled = render(<AskUserQuestionCall args={args} output="public" showCursor />);
const settled = renderCall(<AskUserQuestionCall args={args} output="public" showCursor />);
expect(settled.container.querySelector('.result-thinking')).toBeNull();
const midStream = render(
const midStream = renderCall(
<AskUserQuestionCall args={args} output="public" toolCallId="call_1" isSubmitting />,
);
expect(midStream.container.querySelector('.result-thinking')).toBeNull();
@ -96,7 +187,7 @@ describe('AskUserQuestionCall', () => {
'Error processing tool: Received tool input did not match expected schema ' +
'✖ String must contain at most 120 character(s) → at options[0].label';
render(<AskUserQuestionCall args={args} output={output} failed />);
renderCall(<AskUserQuestionCall args={args} output={output} failed />);
expect(screen.getByText("Question wasn't shown")).toBeInTheDocument();
expect(
@ -111,7 +202,7 @@ describe('AskUserQuestionCall', () => {
'Error processing tool: Received tool input did not match expected schema ' +
'✖ String must contain at most 120 character(s) → at options[0].label';
render(<AskUserQuestionCall args={args} output={output} />);
renderCall(<AskUserQuestionCall args={args} output={output} />);
expect(screen.getByText('You answered:')).toBeInTheDocument();
expect(screen.getByText(output)).toBeInTheDocument();
@ -119,7 +210,7 @@ describe('AskUserQuestionCall', () => {
});
test('renders each question and answer from one completed batch', () => {
render(
renderCall(
<AskUserQuestionCall
args={JSON.stringify({
questions: [
@ -146,7 +237,7 @@ describe('AskUserQuestionCall', () => {
});
test('renders a failed batch without implying the user declined to answer', () => {
render(
renderCall(
<AskUserQuestionCall
args={{ questions: [{ id: 'environment', question: 'Where should this run?' }] }}
output="Error processing tool"

View file

@ -22,7 +22,10 @@ const HITL_PROMPT = `E2E_ASK_USER_QUESTION:${HITL_LABEL}`;
const HITL_QUESTION = `Which environment should Bombadil use for ${HITL_LABEL}?`;
const HITL_OPTION = 'Staging';
const FINAL_REPLY = 'E2E mock reply: pong';
const COMPLETED_ANSWER = 'You answered: Staging';
const COMPLETED_ANSWER_LABEL = 'You answered:';
/** The settled Q&A record: a collapsed tool-call line naming the question,
* over a panel holding the description and the answer. */
const ASK_RECORD = '[data-testid="ask-user-question-call"]';
let reloadIssued = false;
let pausedReloadIssued = false;
@ -79,6 +82,12 @@ function target(
return null;
}
function visibleCount(state: State, selector: string): number {
return Array.from(state.document.querySelectorAll(selector)).filter(
(element) => visiblePoint(state, element) !== null,
).length;
}
function visibleTextCount(
state: State,
selector: string,
@ -110,6 +119,7 @@ function clickOrWait(targetValue: Target | null): Action[] {
const ui = extract((state: State) => {
const messageElements = Array.from(state.document.querySelectorAll('.message-render'));
const askRecordCount = visibleCount(state, ASK_RECORD);
const messageText = messageElements.map((element) => element.textContent ?? '').join('\n');
const modelTrigger = state.document.querySelector('button[aria-label="Select a model"]');
return {
@ -124,14 +134,22 @@ const ui = extract((state: State) => {
emailFocused: isFocused(state, '#email'),
passwordValue: inputValue(state, '#password'),
passwordFocused: isFocused(state, '#password'),
questionCount: visibleTextCount(state, 'p', HITL_QUESTION),
/** Once the pause settles, the record IS the question's presentation, so
* count records rather than every node repeating their text an
* expanded record (Auto-expand tool details) shows the question in both
* its summary line and its panel, and matching text would count one
* record twice. Before a record exists the live pause renders the
* question as a paragraph. */
questionCount:
askRecordCount > 0 ? askRecordCount : visibleTextCount(state, 'p', HITL_QUESTION),
answerOptionCount: visibleTextCount(state, 'button', HITL_OPTION, true),
finalReplyCount: messageElements.filter((element) =>
(element.textContent ?? '').includes(FINAL_REPLY),
).length,
completedAnswerCount: messageElements.filter((element) =>
(element.textContent ?? '').includes(COMPLETED_ANSWER),
).length,
completedAnswerCount: messageElements.filter((element) => {
const text = element.textContent ?? '';
return text.includes(COMPLETED_ANSWER_LABEL) && text.includes(HITL_OPTION);
}).length,
isSubmitting: state.document.querySelector('button[aria-label="Stop generating"]') !== null,
hasComposer: state.document.querySelector('#prompt-textarea') !== null,
loginEmail: target(state, '#email', 'Login email'),