mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
💬 refactor: Raise ask_user_question Option Label Cap to 280 Chars (#14491)
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
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
* 💬 fix: Raise ask_user_question Option Label Cap to 280 Chars Raise OPTION_LABEL_MAX from 120 to 280 and make every ask_user_question surface wrap long, model-generated strings instead of overflowing. * 🪟 fix: Bound ask_user_question Popover to the Viewport The popover is absolutely positioned, so content taller than the viewport is unreachable by page scroll. Cap the panel at 60vh with the option list as the only flexible scroll region, and scroll the keyboard-selected row into view since selection paints a highlight without moving focus.
This commit is contained in:
parent
9c95bf445f
commit
1fce7e1f3c
6 changed files with 92 additions and 49 deletions
|
|
@ -1,4 +1,4 @@
|
|||
import { memo } from 'react';
|
||||
import { memo, useEffect, useRef } from 'react';
|
||||
import { useWatch } from 'react-hook-form';
|
||||
import { Button } from '@librechat/client';
|
||||
import { Check, ChevronDown, CornerDownLeft, TriangleAlert, X } from 'lucide-react';
|
||||
|
|
@ -71,6 +71,29 @@ function AskUserQuestionPopoverPanel({
|
|||
handlePopoverKeyDown,
|
||||
} = ask;
|
||||
|
||||
/** Keyboard selection only paints a highlight (no focus move), so the
|
||||
* scrollable option list has to follow `selected` itself or arrow/digit
|
||||
* navigation can land on a row that is scrolled out of view. Manual
|
||||
* scrollTop math rather than scrollIntoView: it cannot disturb the page. */
|
||||
const listRef = useRef<HTMLDivElement>(null);
|
||||
const optionRefs = useRef<(HTMLButtonElement | null)[]>([]);
|
||||
useEffect(() => {
|
||||
if (typeof selected !== 'number') {
|
||||
return;
|
||||
}
|
||||
const list = listRef.current;
|
||||
const row = optionRefs.current[selected];
|
||||
if (list == null || row == null) {
|
||||
return;
|
||||
}
|
||||
const rowBottom = row.offsetTop + row.offsetHeight;
|
||||
if (row.offsetTop < list.scrollTop) {
|
||||
list.scrollTop = row.offsetTop;
|
||||
} else if (rowBottom > list.scrollTop + list.clientHeight) {
|
||||
list.scrollTop = rowBottom - list.clientHeight;
|
||||
}
|
||||
}, [selected]);
|
||||
|
||||
if (!liveAsk) {
|
||||
return null;
|
||||
}
|
||||
|
|
@ -81,16 +104,22 @@ function AskUserQuestionPopoverPanel({
|
|||
<div className="absolute bottom-28 z-10 w-full space-y-2">
|
||||
{/* Digit shortcuts (1..N) work when focus is inside the popover too, not
|
||||
only from the composer — keydown bubbles here from the focused row/
|
||||
control. */}
|
||||
control. Height is viewport-bounded with the option list as the only
|
||||
scroll region: the panel is absolutely positioned, so anything that
|
||||
overflows it is unreachable by page scroll. */}
|
||||
<div
|
||||
className="popover border-token-border-light rounded-2xl border bg-white p-2 shadow-lg dark:bg-gray-700"
|
||||
className="popover border-token-border-light flex max-h-[60vh] flex-col rounded-2xl border bg-white p-2 shadow-lg dark:bg-gray-700"
|
||||
onKeyDown={handlePopoverKeyDown}
|
||||
>
|
||||
<div className="flex items-start justify-between gap-2 p-2">
|
||||
<div>
|
||||
<p className="text-sm font-medium text-text-primary">{liveAsk.question.question}</p>
|
||||
<div className="flex shrink-0 items-start justify-between gap-2 p-2">
|
||||
<div className="max-h-[24vh] min-w-0 overflow-y-auto">
|
||||
<p className="text-sm font-medium text-text-primary [overflow-wrap:anywhere]">
|
||||
{liveAsk.question.question}
|
||||
</p>
|
||||
{liveAsk.question.description != null && liveAsk.question.description.length > 0 && (
|
||||
<p className="mt-0.5 text-xs text-text-secondary">{liveAsk.question.description}</p>
|
||||
<p className="mt-0.5 text-xs text-text-secondary [overflow-wrap:anywhere]">
|
||||
{liveAsk.question.description}
|
||||
</p>
|
||||
)}
|
||||
</div>
|
||||
<div className="flex items-center">
|
||||
|
|
@ -112,46 +141,51 @@ function AskUserQuestionPopoverPanel({
|
|||
</button>
|
||||
</div>
|
||||
</div>
|
||||
{options.map((option, index) => {
|
||||
const isChecked = multiSelect && checked.includes(index);
|
||||
return (
|
||||
<button
|
||||
key={option.value}
|
||||
type="button"
|
||||
role={multiSelect ? 'checkbox' : undefined}
|
||||
aria-checked={multiSelect ? isChecked : undefined}
|
||||
disabled={locked}
|
||||
className={cn(
|
||||
'flex w-full items-center gap-3 rounded-lg p-2 text-left text-sm text-text-primary',
|
||||
selected === index ? 'bg-surface-active' : 'hover:bg-surface-hover',
|
||||
locked ? 'cursor-not-allowed opacity-60' : '',
|
||||
)}
|
||||
onClick={() => (multiSelect ? toggleChecked(index) : submitOption(index))}
|
||||
>
|
||||
<span
|
||||
<div ref={listRef} className="relative min-h-0 flex-1 overflow-y-auto">
|
||||
{options.map((option, index) => {
|
||||
const isChecked = multiSelect && checked.includes(index);
|
||||
return (
|
||||
<button
|
||||
key={option.value}
|
||||
ref={(el) => {
|
||||
optionRefs.current[index] = el;
|
||||
}}
|
||||
type="button"
|
||||
role={multiSelect ? 'checkbox' : undefined}
|
||||
aria-checked={multiSelect ? isChecked : undefined}
|
||||
disabled={locked}
|
||||
className={cn(
|
||||
'flex h-5 w-5 items-center justify-center rounded text-xs',
|
||||
isChecked
|
||||
? 'bg-surface-submit text-white'
|
||||
: 'bg-surface-tertiary text-text-secondary',
|
||||
'flex w-full items-center gap-3 rounded-lg p-2 text-left text-sm text-text-primary',
|
||||
selected === index ? 'bg-surface-active' : 'hover:bg-surface-hover',
|
||||
locked ? 'cursor-not-allowed opacity-60' : '',
|
||||
)}
|
||||
onClick={() => (multiSelect ? toggleChecked(index) : submitOption(index))}
|
||||
>
|
||||
{isChecked ? <Check className="h-3.5 w-3.5" aria-hidden="true" /> : index + 1}
|
||||
</span>
|
||||
<span className="flex-1">{option.label}</span>
|
||||
</button>
|
||||
);
|
||||
})}
|
||||
<span
|
||||
className={cn(
|
||||
'flex h-5 w-5 shrink-0 items-center justify-center rounded text-xs',
|
||||
isChecked
|
||||
? 'bg-surface-submit text-white'
|
||||
: 'bg-surface-tertiary text-text-secondary',
|
||||
)}
|
||||
>
|
||||
{isChecked ? <Check className="h-3.5 w-3.5" aria-hidden="true" /> : index + 1}
|
||||
</span>
|
||||
<span className="min-w-0 flex-1 [overflow-wrap:anywhere]">{option.label}</span>
|
||||
</button>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
{/** A failed submission keeps the question answerable (controls stay
|
||||
* enabled), but the chat card that would show the error is hidden
|
||||
* while the popover is up — so surface it here for retry guidance. */}
|
||||
{errored && (
|
||||
<div className="flex items-center gap-1.5 px-2 pt-1 text-xs text-text-warning">
|
||||
<div className="flex shrink-0 items-center gap-1.5 px-2 pt-1 text-xs text-text-warning">
|
||||
<TriangleAlert className="h-4 w-4 shrink-0" aria-hidden="true" />
|
||||
{localize('com_ui_ask_answer_error')}
|
||||
</div>
|
||||
)}
|
||||
<div className="flex items-center justify-between gap-2 p-2">
|
||||
<div className="flex shrink-0 items-center justify-between gap-2 p-2">
|
||||
<button
|
||||
type="button"
|
||||
className="text-xs italic text-text-secondary hover:text-text-primary hover:underline"
|
||||
|
|
|
|||
|
|
@ -110,7 +110,9 @@ export default function AskUserQuestion({
|
|||
return (
|
||||
<div className="my-2 flex w-full flex-col gap-2 rounded-lg border border-border-light bg-surface-secondary p-3">
|
||||
<div className="flex items-start justify-between gap-2">
|
||||
<p className="text-sm font-medium text-text-primary">{question.question}</p>
|
||||
<p className="min-w-0 text-sm font-medium text-text-primary [overflow-wrap:anywhere]">
|
||||
{question.question}
|
||||
</p>
|
||||
{collapsed && isLivePause && (
|
||||
<button
|
||||
type="button"
|
||||
|
|
@ -123,7 +125,9 @@ export default function AskUserQuestion({
|
|||
)}
|
||||
</div>
|
||||
{question.description != null && question.description.length > 0 && (
|
||||
<p className="text-sm text-text-secondary">{question.description}</p>
|
||||
<p className="text-sm text-text-secondary [overflow-wrap:anywhere]">
|
||||
{question.description}
|
||||
</p>
|
||||
)}
|
||||
|
||||
{choices.length > 0 && (
|
||||
|
|
@ -136,6 +140,7 @@ export default function AskUserQuestion({
|
|||
role={multiSelect ? 'checkbox' : undefined}
|
||||
aria-checked={multiSelect ? checkedIndices.includes(index) : undefined}
|
||||
disabled={locked}
|
||||
className="h-auto min-h-9 max-w-full whitespace-normal py-1.5 text-left [overflow-wrap:anywhere]"
|
||||
onClick={() => (multiSelect ? toggleIndex(index) : submitSingle(index))}
|
||||
>
|
||||
{option.label}
|
||||
|
|
|
|||
|
|
@ -56,7 +56,9 @@ export default function AskUserQuestionCall({
|
|||
{localize('com_ui_question_failed')}
|
||||
</div>
|
||||
{question?.question != null && (
|
||||
<p className="text-sm font-medium text-text-primary">{question.question}</p>
|
||||
<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')}
|
||||
|
|
@ -91,14 +93,16 @@ export default function AskUserQuestionCall({
|
|||
<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">
|
||||
<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">{question.description}</p>
|
||||
<p className="text-sm text-text-secondary [overflow-wrap:anywhere]">
|
||||
{question.description}
|
||||
</p>
|
||||
)}
|
||||
{answered ? (
|
||||
<p className="text-sm text-text-primary">
|
||||
<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>
|
||||
|
|
|
|||
|
|
@ -80,11 +80,11 @@ describe('ask_user_question tool contract', () => {
|
|||
type: 'tool_call',
|
||||
args: {
|
||||
question: 'How should I get the data?',
|
||||
options: [{ label: 'x'.repeat(161), value: 'public-data' }],
|
||||
options: [{ label: 'x'.repeat(281), value: 'public-data' }],
|
||||
},
|
||||
}),
|
||||
).rejects.toThrow(
|
||||
'Option labels must be 120 characters or fewer. Shorten the label and retry.',
|
||||
'Option labels must be 280 characters or fewer. Shorten the label and retry.',
|
||||
);
|
||||
expect(validationErrors).toEqual(
|
||||
new Map([['tool-1', { fieldPath: 'options[0].label', isLengthLimit: true }]]),
|
||||
|
|
@ -149,7 +149,7 @@ describe('ask_user_question tool contract', () => {
|
|||
).toBe(true);
|
||||
expect(
|
||||
AskUserQuestionToolDefinition.schema.properties.options.items.properties.label.maxLength,
|
||||
).toBe(120);
|
||||
).toBe(280);
|
||||
expect(
|
||||
askUserQuestionToolSchema.safeParse({
|
||||
question: 'pick',
|
||||
|
|
@ -163,10 +163,10 @@ describe('ask_user_question tool contract', () => {
|
|||
expect(instance.description).toBe(AskUserQuestionToolDefinition.description);
|
||||
expect(instance.description).toContain('exactly ONE question per turn');
|
||||
expect(instance.description).toContain('NEVER call this tool in parallel');
|
||||
expect(instance.description).toContain('option label within 120 characters');
|
||||
expect(instance.description).toContain('option label within 280 characters');
|
||||
expect(
|
||||
AskUserQuestionToolDefinition.schema.properties.options.items.properties.label.description,
|
||||
).toContain('Maximum 120 characters');
|
||||
).toContain('Maximum 280 characters');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -20,7 +20,7 @@ export const ASK_USER_QUESTION_TOOL_NAME = 'ask_user_question';
|
|||
*/
|
||||
const QUESTION_MAX = 2000;
|
||||
const DESCRIPTION_MAX = 4000;
|
||||
const OPTION_LABEL_MAX = 120;
|
||||
const OPTION_LABEL_MAX = 280;
|
||||
const OPTION_VALUE_MAX = 500;
|
||||
const OPTIONS_MAX = 12;
|
||||
|
||||
|
|
|
|||
|
|
@ -9,7 +9,7 @@ describe('getToolInputValidationDetails', () => {
|
|||
const validationError = parseToolInputValidationError(
|
||||
new Error(
|
||||
'Received tool input did not match expected schema\n' +
|
||||
'✖ Option labels must be 120 characters or fewer. Shorten the label and retry.\n' +
|
||||
'✖ Option labels must be 280 characters or fewer. Shorten the label and retry.\n' +
|
||||
' → at options[0].label',
|
||||
),
|
||||
);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue