From 8abb379cdce59ac47ebb7767cd27198609f65ff1 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Sun, 23 Aug 2026 21:48:02 +0200 Subject: [PATCH] fix(client): address Codex review findings --- .github/workflows/codegraph-select.yml | 2 +- .../Chat/Messages/Content/AskUserQuestion.tsx | 6 +- .../components/Chat/Messages/Content/Part.tsx | 2 + .../Chat/Messages/Content/Parts/DiffView.tsx | 10 +- .../Messages/Content/Parts/MemoryCall.tsx | 92 +++++++++++++------ .../Chat/Messages/Content/Parts/Reasoning.tsx | 8 +- .../Content/Parts/__tests__/DiffView.test.ts | 42 +++++++++ .../Parts/__tests__/MemoryCall.test.tsx | 87 +++++++++++++----- .../Parts/__tests__/SubagentCall.test.tsx | 1 - .../Chat/Messages/Content/ToolCallGroup.tsx | 8 +- .../AskUserQuestionCollapse.test.tsx | 31 ++++--- .../Messages/Content/__tests__/Error.spec.tsx | 2 +- .../src/hooks/Input/useAskAnswerMode.spec.ts | 39 ++++++-- client/src/hooks/Input/useAskAnswerMode.ts | 24 ++--- packages/api/src/agents/run.ts | 2 +- 15 files changed, 250 insertions(+), 106 deletions(-) create mode 100644 client/src/components/Chat/Messages/Content/Parts/__tests__/DiffView.test.ts diff --git a/.github/workflows/codegraph-select.yml b/.github/workflows/codegraph-select.yml index e74a86e270..d76d673a8a 100644 --- a/.github/workflows/codegraph-select.yml +++ b/.github/workflows/codegraph-select.yml @@ -59,7 +59,7 @@ jobs: RESP=$(curl -sS --fail-with-body -m 45 -H "Authorization: Bearer $TOKEN" \ -H 'content-type: application/json' --data-binary @body.json "$URL/v1/select"); RC=$? if [ "$RC" -ne 0 ] || [ -z "$RESP" ] || ! echo "$RESP" | jq -e .selected >/dev/null 2>&1; then - note "_codegraph unavailable (curl exit $RC: ${RESP:0:120}); skipped — full CI runs as always_" + note "_codegraph unavailable (curl exit $RC: ${RESP:0:120}); skipped, full CI runs as always_" exit 0 fi diff --git a/client/src/components/Chat/Messages/Content/AskUserQuestion.tsx b/client/src/components/Chat/Messages/Content/AskUserQuestion.tsx index ce9132c65e..6087302a00 100644 --- a/client/src/components/Chat/Messages/Content/AskUserQuestion.tsx +++ b/client/src/components/Chat/Messages/Content/AskUserQuestion.tsx @@ -2,8 +2,8 @@ import { useContext, useMemo, useState } from 'react'; import { Button, TextareaAutosize, TooltipAnchor } from '@librechat/client'; import { ChevronUp, MessageCircleQuestion, TriangleAlert } from 'lucide-react'; import type { Agents } from 'librechat-data-provider'; -import { splitOtherOption, ASK_USER_DECLINED_ANSWER } from '~/utils/approval'; import { useApprovalContext, useAskSubmitStatus, useResumeSubmit } from './ApprovalContext'; +import { splitOtherOption, ASK_USER_DECLINED_ANSWER } from '~/utils/approval'; import useAskAnswerMode from '~/hooks/Input/useAskAnswerMode'; import AskOptions from '~/components/Chat/ask/options'; import { ChatContext } from '~/Providers/ChatContext'; @@ -127,7 +127,7 @@ function AskUserQuestionSingle({ }; /** `answerMode.skip()` is gated on answer mode being ACTIVE, which a - * question moved to the chat is not — and that is precisely when this card + * question moved to the chat is not. That is precisely when this card * is the only surface left. Decline through the answer path instead, * which is gated on the live pause rather than on answer mode. */ const handleSkip = () => { @@ -161,7 +161,7 @@ function AskUserQuestionSingle({ * The live card shares its view-transition-name with the popover panel, so * collapse/expand morphs one surface into the other. The placeholder copy * is `visibility: hidden` (out of the tab order and the a11y tree) and - * carries NO transition name — duplicate names would void the morph — but + * carries NO transition name because duplicate names would void the morph, but * it still occupies the card's exact footprint, so the thread reserves the * space while the question lives in the composer and nothing reflows when * it moves back. diff --git a/client/src/components/Chat/Messages/Content/Part.tsx b/client/src/components/Chat/Messages/Content/Part.tsx index 4e21fc07fc..c36ab4def7 100644 --- a/client/src/components/Chat/Messages/Content/Part.tsx +++ b/client/src/components/Chat/Messages/Content/Part.tsx @@ -307,6 +307,8 @@ const Part = memo(function Part({ output={toolCall.output ?? ''} initialProgress={toolCall.progress ?? 0.1} isSubmitting={isSubmitting} + runStepStatus={toolCall.runStepStatus} + runStepDurationMs={toolCall.runStepDurationMs} attachments={attachments} hideAttachments={hideAttachments} onExpand={onToolExpand} diff --git a/client/src/components/Chat/Messages/Content/Parts/DiffView.tsx b/client/src/components/Chat/Messages/Content/Parts/DiffView.tsx index 255084cb58..9b2c5337e8 100644 --- a/client/src/components/Chat/Messages/Content/Parts/DiffView.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/DiffView.tsx @@ -19,7 +19,7 @@ export interface ParsedDiff { /** * Parses unified-diff text into typed lines with old/new line numbers when the * hunk headers carry them. Also understands the argless `@@` separators and - * `--- old_text` / `+++ new_text` markers the streaming args preview emits — + * `--- old_text` / `+++ new_text` markers the streaming args preview emits. * file headers and `\ No newline` markers are dropped (the window header * already names the file). */ @@ -30,13 +30,18 @@ export function parseUnifiedDiff(diff: string): ParsedDiff { let hasLineNumbers = false; let oldLine: number | undefined; let newLine: number | undefined; + let inHunk = false; for (const raw of diff.replace(/\n$/, '').split('\n')) { - if (raw.startsWith('--- ') || raw.startsWith('+++ ') || raw.startsWith('\\')) { + if (!inHunk && (raw.startsWith('--- ') || raw.startsWith('+++ '))) { + continue; + } + if (raw.startsWith('\\')) { continue; } const hunk = HUNK_HEADER.exec(raw); if (hunk) { + inHunk = true; oldLine = Number(hunk[1]); newLine = Number(hunk[2]); hasLineNumbers = true; @@ -44,6 +49,7 @@ export function parseUnifiedDiff(diff: string): ParsedDiff { continue; } if (raw === '@@') { + inHunk = true; oldLine = undefined; newLine = undefined; lines.push({ type: 'hunk', text: '' }); diff --git a/client/src/components/Chat/Messages/Content/Parts/MemoryCall.tsx b/client/src/components/Chat/Messages/Content/Parts/MemoryCall.tsx index 7d7e8cc307..35acd69e17 100644 --- a/client/src/components/Chat/Messages/Content/Parts/MemoryCall.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/MemoryCall.tsx @@ -1,6 +1,7 @@ import { useMemo } from 'react'; import { Brain } from 'lucide-react'; -import type { TAttachment } from 'librechat-data-provider'; +import { Tools } from 'librechat-data-provider'; +import type { PartMetadata, TAttachment } from 'librechat-data-provider'; import ProgressText from '~/components/Chat/Messages/Content/ProgressText'; import { toolPanelSpacingClassName } from '../disclosure'; import useToolCallState from './useToolCallState'; @@ -11,9 +12,21 @@ import { cn } from '~/utils'; type MemoryToolName = 'set_memory' | 'delete_memory'; +export function isMemoryFailureOutput(toolName: MemoryToolName, output: string): boolean { + const trimmed = output.trim(); + if (!trimmed) { + return false; + } + return toolName === 'set_memory' + ? !/^(?:Memory set for key|Memory saved\b)/i.test(trimmed) + : !/^Memory deleted(?: for key|\.)/i.test(trimmed); +} + export default function MemoryCall({ toolName, isSubmitting, + runStepStatus, + runStepDurationMs, initialProgress = 0.1, args, output = '', @@ -24,6 +37,8 @@ export default function MemoryCall({ toolName: MemoryToolName; initialProgress: number; isSubmitting: boolean; + runStepStatus?: PartMetadata['runStepStatus']; + runStepDurationMs?: PartMetadata['runStepDurationMs']; args?: string | Record; output?: string; attachments?: TAttachment[]; @@ -35,64 +50,83 @@ export default function MemoryCall({ const memoryKey = useMemo(() => parseJsonField(args, 'key'), [args]); const memoryValue = useMemo(() => parseJsonField(args, 'value'), [args]); const hasPanel = !!memoryKey || !!memoryValue; + const memoryFailed = useMemo( + () => + isMemoryFailureOutput(toolName, output) || + (attachments?.some((attachment) => attachment?.[Tools.memory]?.type === 'error') ?? false), + [attachments, toolName, output], + ); - const { showCode, toggleCode, expandStyle, expandRef, progress, cancelled, hasError } = - useToolCallState(initialProgress, isSubmitting, output, hasPanel, onExpand); + const { showCode, toggleCode, expandStyle, expandRef, phase } = useToolCallState({ + initialProgress, + isSubmitting, + output, + hasInput: hasPanel || memoryFailed || runStepStatus === 'failed', + onExpand, + runStepStatus, + extraError: memoryFailed, + }); + let finishedText = localize(isSave ? 'com_ui_memory_saved' : 'com_ui_memory_removed'); + if (phase === 'cancelled') { + finishedText = localize('com_ui_cancelled'); + } else if (phase === 'failed') { + finishedText = localize('com_ui_memory'); + } return ( <>
- {(hasPanel || hasError) && ( + {(hasPanel || phase === 'failed') && (
- {memoryKey && ( -
- {memoryKey} -
- )} - {isSave && memoryValue && ( -
{memoryValue}
- )} - {!isSave && ( -
- {localize('com_ui_memory_deleted')} -
- )} - {hasError && ( - 
+              {phase === 'failed' ? (
+                
                   {output}
                 

+              ) : (
+                <>
+                  {memoryKey && (
+                    

+                      {memoryKey}
+                    

+                  )}
+                  {isSave && memoryValue && (
+                    

+                      {memoryValue}
+                    

+                  )}
+                  {!isSave && (
+                    

+                      {localize('com_ui_memory_deleted')}
+                    

+                  )}
+                
               )}
)} diff --git a/client/src/components/Chat/Messages/Content/Parts/Reasoning.tsx b/client/src/components/Chat/Messages/Content/Parts/Reasoning.tsx index cdef4899cf..535ce48229 100644 --- a/client/src/components/Chat/Messages/Content/Parts/Reasoning.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/Reasoning.tsx @@ -5,9 +5,9 @@ import { ContentTypes } from 'librechat-data-provider'; import type { MouseEvent, FocusEvent } from 'react'; import { ThinkingContent, ThinkingButton, FloatingThinkingBar, useInViewport } from './Thinking'; import { disclosureChevronClassName } from '~/components/Chat/Messages/Content/disclosure'; -import CopyButton from '~/components/Messages/Content/CopyButton'; import { useLocalize, useExpandCollapse, useLazyCollapseBody } from '~/hooks'; import useSmoothStreaming from '~/hooks/Messages/useSmoothStreaming'; +import CopyButton from '~/components/Messages/Content/CopyButton'; import { showThinkingAtom } from '~/store/showThinking'; import { fontSizeAtom } from '~/store/fontSize'; import { useMessageContext } from '~/Providers'; @@ -21,7 +21,7 @@ const stripThinkTags = (reasoning: string): string => const PEEK_SENTENCES = 4; -/** Tail of streaming reasoning — the last few sentences — for the collapsed +/** Tail of streaming reasoning, specifically the last few sentences, for the collapsed * live peek. Bounds work on long reasoning by scanning only the trailing * slice before splitting on sentence boundaries. */ const lastSentences = (text: string): string => { @@ -41,7 +41,7 @@ const PEEK_FADE = /** * Collapsed live preview of streaming reasoning. Mirrors the expanded thought - * panel — same rounded outline and text treatment — but with a border instead + * panel. It uses the same rounded outline and text treatment, but with a border instead * of a surface fill, showing the trailing few sentences in a short, * bottom-pinned window whose top and bottom edges fade out, so the newest * thought stays in view while older lines scroll up and dissolve (the "thinking @@ -256,7 +256,7 @@ type ReasoningCompactProps = { * Compact reasoning row for use INSIDE a ToolCallGroup. Keeps the tool-row * header rhythm (icon + label + chevron) so an interleaved thought reads as a * sibling of the surrounding tool calls, while retaining the standalone - * {@link Reasoning} affordances — a hover-revealed copy button on the header and + * {@link Reasoning} affordances: a hover-revealed copy button on the header and * a floating collapse + copy bar inside the rounded content panel. */ export const ReasoningCompact = memo( diff --git a/client/src/components/Chat/Messages/Content/Parts/__tests__/DiffView.test.ts b/client/src/components/Chat/Messages/Content/Parts/__tests__/DiffView.test.ts new file mode 100644 index 0000000000..54dceac2cb --- /dev/null +++ b/client/src/components/Chat/Messages/Content/Parts/__tests__/DiffView.test.ts @@ -0,0 +1,42 @@ +import { parseUnifiedDiff } from '../DiffView'; + +describe('parseUnifiedDiff', () => { + it('drops file headers but keeps changed lines that begin with header markers', () => { + const parsed = parseUnifiedDiff( + [ + '--- a/example.txt', + '+++ b/example.txt', + '@@ -4,2 +4,2 @@', + '--- deleted text', + '+++ added text', + ' unchanged', + ].join('\n'), + ); + + expect(parsed).toEqual({ + additions: 1, + deletions: 1, + hasLineNumbers: true, + lines: [ + { type: 'hunk', text: '@@ -4,2 +4,2 @@' }, + { type: 'del', text: '-- deleted text', oldLine: 4 }, + { type: 'add', text: '++ added text', newLine: 4 }, + { type: 'context', text: 'unchanged', oldLine: 5, newLine: 5 }, + ], + }); + }); + + it('keeps the same changed lines after an argless streaming hunk marker', () => { + const parsed = parseUnifiedDiff( + ['--- old_text', '+++ new_text', '@@', '--- deleted text', '+++ added text'].join('\n'), + ); + + expect(parsed.lines).toEqual([ + { type: 'hunk', text: '' }, + { type: 'del', text: '-- deleted text', oldLine: undefined }, + { type: 'add', text: '++ added text', newLine: undefined }, + ]); + expect(parsed.additions).toBe(1); + expect(parsed.deletions).toBe(1); + }); +}); diff --git a/client/src/components/Chat/Messages/Content/Parts/__tests__/MemoryCall.test.tsx b/client/src/components/Chat/Messages/Content/Parts/__tests__/MemoryCall.test.tsx index 241fdf395b..fc125e671a 100644 --- a/client/src/components/Chat/Messages/Content/Parts/__tests__/MemoryCall.test.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/__tests__/MemoryCall.test.tsx @@ -12,6 +12,7 @@ jest.mock('~/hooks', () => ({ com_ui_memory_deleting: 'Deleting memory', com_ui_memory_removed: 'Deleted memory', com_ui_memory_deleted: 'Memory deleted', + com_ui_memory: 'Memory', com_ui_cancelled: 'Cancelled', com_ui_tool_failed: 'failed', }; @@ -26,24 +27,22 @@ jest.mock('~/utils', () => ({ jest.mock('~/components/Chat/Messages/Content/ProgressText', () => ({ __esModule: true, default: ({ - progress, + phase, inProgressText, finishedText, subtitle, - errorSuffix, hasInput, }: { - progress: number; + phase: 'running' | 'completed' | 'cancelled' | 'failed'; inProgressText: string; finishedText: string; subtitle?: string; - errorSuffix?: string; hasInput?: boolean; }) => (
- {progress < 1 ? inProgressText : finishedText} + {phase === 'running' ? inProgressText : finishedText} {subtitle ? ` ${subtitle}` : ''} - {errorSuffix ? ` ${errorSuffix}` : ''} + {phase === 'failed' ? ' failed' : ''}
), })); @@ -54,17 +53,30 @@ jest.mock('../Attachment', () => ({ jest.mock('../useToolCallState', () => ({ __esModule: true, - default: (initialProgress: number, _isSubmitting: boolean, output: string) => ({ - showCode: true, - toggleCode: jest.fn(), - expandStyle: {}, - expandRef: { current: null }, - progress: initialProgress, - cancelled: false, - hasError: output.startsWith('Error'), - hasOutput: output.length > 0, - hasContent: true, - }), + default: ({ + initialProgress, + runStepStatus, + extraError, + }: { + initialProgress: number; + runStepStatus?: 'completed' | 'cancelled' | 'failed'; + extraError?: boolean; + }) => { + let phase: 'running' | 'completed' | 'cancelled' | 'failed' = + initialProgress < 1 ? 'running' : 'completed'; + if (runStepStatus === 'cancelled') { + phase = 'cancelled'; + } else if (runStepStatus === 'failed' || extraError) { + phase = 'failed'; + } + return { + showCode: true, + toggleCode: jest.fn(), + expandStyle: {}, + expandRef: { current: null }, + phase, + }; + }, })); describe('MemoryCall', () => { @@ -75,14 +87,14 @@ describe('MemoryCall', () => { initialProgress={1} isSubmitting={false} args={{ key: 'preferences', value: 'Prefers dark mode.' }} - output="Memory saved." + output={'Memory set for key "preferences" (4 tokens)'} />, ); expect(screen.getByTestId('progress-text')).toHaveTextContent('Saved memory preferences'); expect(screen.getByText('preferences')).toHaveClass('font-bold', 'uppercase'); expect(screen.getByText('Prefers dark mode.')).toBeInTheDocument(); - expect(screen.queryByText('Memory saved.')).not.toBeInTheDocument(); + expect(screen.queryByText(/Memory set for key/)).not.toBeInTheDocument(); }); it('labels an in-flight save without a parsed key', () => { @@ -107,7 +119,7 @@ describe('MemoryCall', () => { initialProgress={1} isSubmitting={false} args={{ key: 'outdated_note' }} - output="Memory deleted." + output={'Memory deleted for key "outdated_note"'} />, ); @@ -116,18 +128,43 @@ describe('MemoryCall', () => { expect(screen.getByText('Memory deleted')).toBeInTheDocument(); }); - it('surfaces the error output inside the panel', () => { + it.each([ + ['set_memory', 'Invalid key "invalid". Must be one of: preferences'], + ['set_memory', 'Memory storage would exceed limit. Cannot save this memory.'], + ['set_memory', 'Failed to set memory for key "preferences"'], + ['set_memory', '{"type":"content_filter","message":"Blocked"}'], + ['delete_memory', 'Failed to delete memory for key "preferences"'], + ] as const)('surfaces %s failure output instead of optimistic content', (toolName, output) => { + render( + , + ); + + expect(screen.getByTestId('progress-text')).toHaveTextContent('Memory preferences failed'); + expect(screen.getByText(output)).toBeInTheDocument(); + expect(screen.queryByText('requested value')).not.toBeInTheDocument(); + expect(screen.queryByText('Memory deleted')).not.toBeInTheDocument(); + }); + + it('honors an explicit failed run step even when the output resembles success', () => { render( , ); - expect(screen.getByTestId('progress-text')).toHaveTextContent('failed'); - expect(screen.getByText('Error: memory storage full')).toBeInTheDocument(); + expect(screen.getByTestId('progress-text')).toHaveTextContent('Memory preferences failed'); + expect(screen.getByText(/Memory set for key/)).toBeInTheDocument(); + expect(screen.queryByText('requested value')).not.toBeInTheDocument(); }); }); diff --git a/client/src/components/Chat/Messages/Content/Parts/__tests__/SubagentCall.test.tsx b/client/src/components/Chat/Messages/Content/Parts/__tests__/SubagentCall.test.tsx index c74aa3194f..8b15688475 100644 --- a/client/src/components/Chat/Messages/Content/Parts/__tests__/SubagentCall.test.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/__tests__/SubagentCall.test.tsx @@ -442,5 +442,4 @@ describe('SubagentCall', () => { expect(rendered.getSelection()?.durable).toBeUndefined(); expect(rendered.getSelection()?.legacyOutput).toBe(output); }); - }); diff --git a/client/src/components/Chat/Messages/Content/ToolCallGroup.tsx b/client/src/components/Chat/Messages/Content/ToolCallGroup.tsx index 880f379282..0f78825c15 100644 --- a/client/src/components/Chat/Messages/Content/ToolCallGroup.tsx +++ b/client/src/components/Chat/Messages/Content/ToolCallGroup.tsx @@ -153,7 +153,7 @@ export default function ToolCallGroup({ const retainedForPendingApprovalRef = useRef(false); /** `parts` may include interleaved reasoning ("Thoughts") parts that render - * inside the body but are not actions — count and summarize only the real + * inside the body but are not actions. Count and summarize only the real * tool calls so the header and stacked icons stay accurate. */ const toolMetadata = useMemo( () => parts.map((p) => getToolMeta(p.part)).filter((m): m is ToolMeta => m != null), @@ -233,7 +233,7 @@ export default function ToolCallGroup({ }, [toolMetadata, localize, mcpServerNames]); /** Reasoning interleaved with the tool calls renders inside the body but is - * hidden while collapsed — note it in the header's accessible label so screen + * hidden while collapsed. Note it in the header's accessible label so screen * readers know the group also contains thoughts. */ const hasReasoning = useMemo( () => parts.some((p) => p.part.type === ContentTypes.THINK), @@ -255,7 +255,7 @@ export default function ToolCallGroup({ /** `ask_user_question` calls form their own category, mirroring subagents: * a homogeneous group reads "Asking/Asked N questions" (never "Used N - * tools — ask_user_question") with a question glyph. */ + * tools: ask_user_question") with a question glyph. */ const allAskQuestions = activitySummary.askQuestionCount > 0 && activitySummary.askQuestionCount === count; /** Past tense once the turn is settled — matches the Asking/Asked record @@ -264,7 +264,7 @@ export default function ToolCallGroup({ const askQuestionsDone = allAskQuestions && (allCompleted || !isSubmitting); /** For a single-tool group, lead with the tool's own (capitalized) label - * instead of the generic "Used 1 tool — name", which reads awkwardly. */ + * instead of the generic "Used 1 tool: name", which reads awkwardly. */ const singleToolLabel = useMemo(() => { const raw = getToolDisplayLabel(toolMetadata[0]?.name ?? '', localize, mcpServerNames); return raw ? raw.charAt(0).toUpperCase() + raw.slice(1) : ''; diff --git a/client/src/components/Chat/Messages/Content/__tests__/AskUserQuestionCollapse.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/AskUserQuestionCollapse.test.tsx index eedb7287b3..2f3d9e7dfe 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/AskUserQuestionCollapse.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/AskUserQuestionCollapse.test.tsx @@ -95,23 +95,24 @@ describe('collapsing a live ask_user_question', () => { fireEvent.click(screen.getByTestId('collapse-from-popover')); expect(screen.getByTestId('popover-visible').textContent).toBe('false'); - /** The pause is still live — the card has it now... */ - expect(screen.getByTestId('active').textContent).toBe('true'); + /** The pause is still live, but the card owns it outside answer mode. */ + expect(screen.getByTestId('active').textContent).toBe('false'); expect(screen.getByText('North star or full funnel?')).toBeInTheDocument(); - /** ...and the composer is a composer again. */ + /** The composer is a normal composer again. */ expect(screen.getByTestId('composer-locked').textContent).toBe('false'); expect(screen.getByTestId('composer-answers').textContent).toBe('false'); }); - it('gives the collapsed card the popover’s dismiss', () => { + it('moves the collapsed card back to the popover', () => { renderPause(); fireEvent.click(screen.getByTestId('collapse-from-popover')); - expect(screen.getByLabelText('Expand')).toBeInTheDocument(); - fireEvent.click(screen.getByLabelText('Close')); + fireEvent.click(screen.getByLabelText('Expand')); - /** Dismiss exits answer mode entirely, exactly as the popover’s × did. */ - expect(screen.getByTestId('active').textContent).toBe('false'); + expect(screen.getByTestId('active').textContent).toBe('true'); + expect(screen.getByTestId('popover-visible').textContent).toBe('true'); + expect(screen.getByTestId('composer-locked').textContent).toBe('true'); + expect(screen.queryByLabelText('Expand')).not.toBeInTheDocument(); }); }); @@ -120,22 +121,22 @@ describe('collapsing a live ask_user_question', () => { mockLiveAsk = { actionId: 'act-1', question: mockSingle, messageId: 'message-1' }; }); - it('keeps the composer as the answer box, and still offers a dismiss', () => { + it('releases the composer until the card moves back to the popover', () => { renderPause(); expect(screen.getByTestId('composer-answers').textContent).toBe('true'); expect(screen.getByTestId('composer-locked').textContent).toBe('false'); fireEvent.click(screen.getByTestId('collapse-from-popover')); - /** A single question is answered IN the composer, so it stays the answer - * box past collapse — the card is only the display handing over. */ - expect(screen.getByTestId('composer-answers').textContent).toBe('true'); + /** The card now owns the answer, so the normal composer is available. */ + expect(screen.getByTestId('composer-answers').textContent).toBe('false'); expect(screen.getByText('Which environment?')).toBeInTheDocument(); - fireEvent.click(screen.getByLabelText('Close')); + fireEvent.click(screen.getByLabelText('Answer from the message box')); - expect(screen.getByTestId('active').textContent).toBe('false'); - expect(screen.getByTestId('composer-answers').textContent).toBe('false'); + expect(screen.getByTestId('active').textContent).toBe('true'); + expect(screen.getByTestId('composer-answers').textContent).toBe('true'); + expect(screen.getByTestId('popover-visible').textContent).toBe('true'); }); }); }); diff --git a/client/src/components/Messages/Content/__tests__/Error.spec.tsx b/client/src/components/Messages/Content/__tests__/Error.spec.tsx index cbef66151c..143d2ca769 100644 --- a/client/src/components/Messages/Content/__tests__/Error.spec.tsx +++ b/client/src/components/Messages/Content/__tests__/Error.spec.tsx @@ -58,7 +58,7 @@ describe('Error — typed provider errors', () => { }); }); -describe('Error — agent context budget errors', () => { +describe('Error: agent context budget errors', () => { beforeAll(() => { /** CodeBlock observes its code bar; jsdom ships no IntersectionObserver. */ (global as { IntersectionObserver?: unknown }).IntersectionObserver = class { diff --git a/client/src/hooks/Input/useAskAnswerMode.spec.ts b/client/src/hooks/Input/useAskAnswerMode.spec.ts index 5fcc286e2b..7f0df2014f 100644 --- a/client/src/hooks/Input/useAskAnswerMode.spec.ts +++ b/client/src/hooks/Input/useAskAnswerMode.spec.ts @@ -61,6 +61,14 @@ const liveAsk = { question: { question: 'Pick one', options: [], multiSelect: false }, } as unknown as ReturnType; +const batchAsk = { + ...liveAsk, + questions: [ + { id: 'environment', question: 'Which environment?' }, + { id: 'window', question: 'Which window?' }, + ], +} as typeof liveAsk; + describe('useAskAnswerMode', () => { beforeEach(() => { jest.clearAllMocks(); @@ -95,15 +103,7 @@ describe('useAskAnswerMode', () => { }); it('locks the composer for a batch, and hands it back the moment it collapses', () => { - mockUseGetMessages.mockReturnValue({ - data: { - ...liveAsk, - questions: [ - { id: 'environment', question: 'Which environment?' }, - { id: 'window', question: 'Which window?' }, - ], - }, - }); + mockUseGetMessages.mockReturnValue({ data: batchAsk }); const { result } = renderHook(() => useAskAnswerMode('conversation-1')); @@ -120,6 +120,27 @@ describe('useAskAnswerMode', () => { expect(mockSubmitAskAnswer).not.toHaveBeenCalled(); }); + it('does not move the normal composer draft into a collapsed batch answer', () => { + mockUseGetMessages.mockReturnValue({ data: batchAsk }); + const { result } = renderHook(() => useAskAnswerMode('conversation-1')); + + act(() => result.current.collapse()); + + expect(mockSetAnswerDraft).not.toHaveBeenCalled(); + expect(mockResetComposer).not.toHaveBeenCalled(); + }); + + it('does not overwrite normal composer text when expanding a batch', () => { + mockCollapsedIds = ['a1']; + mockAnswerDraft = { actionId: 'a1', text: 'stale batch handoff' }; + mockUseGetMessages.mockReturnValue({ data: batchAsk }); + const { result } = renderHook(() => useAskAnswerMode('conversation-1')); + + act(() => result.current.expand()); + + expect(mockSetComposerText).not.toHaveBeenCalled(); + }); + it('disables the query and forces liveAsk null for a new (unsaved) conversation', () => { mockUseGetMessages.mockReturnValue({ data: liveAsk }); diff --git a/client/src/hooks/Input/useAskAnswerMode.ts b/client/src/hooks/Input/useAskAnswerMode.ts index a7dc59d6e3..230703c431 100644 --- a/client/src/hooks/Input/useAskAnswerMode.ts +++ b/client/src/hooks/Input/useAskAnswerMode.ts @@ -18,7 +18,7 @@ import store from '~/store'; * Action ids the user moved into the chat: the popover is hidden AND the * composer is released (answer mode off), so the chat card is the question's * only surface until the card's chevron moves it back. One state for one - * user-visible concept — a hidden popover whose composer stayed armed was + * user-visible concept. A hidden popover whose composer stayed armed was * indistinguishable from one whose composer did not. */ const collapsedAskActionsAtom = atom({ @@ -136,7 +136,7 @@ export default function useAskAnswerMode(conversationId?: string | null) { const collapsed = answerable && collapsedIds.includes(liveAsk.actionId); /** * Answer mode: the popover is up AND the composer is the free-form answer - * box. The two are deliberately the same condition — the composer's answer + * box. The two are deliberately the same condition because the composer's answer * role is only discoverable while the popover explains it. */ const active = answerable && !collapsed; @@ -157,7 +157,7 @@ export default function useAskAnswerMode(conversationId?: string | null) { const answerText = answerDraft.actionId === liveAsk?.actionId ? answerDraft.text : ''; const setAnswerText = useCallback( (text: string) => { - if (liveAsk) { + if (liveAsk && !batchMode) { setAnswerDraft({ actionId: liveAsk.actionId, text }); /** While the card owns the answer, `useAutoSave` is tracking the * conversation draft instead. Keep the dormant ask draft current so @@ -167,7 +167,7 @@ export default function useAskAnswerMode(conversationId?: string | null) { } } }, - [liveAsk, saveDrafts, setAnswerDraft], + [batchMode, liveAsk, saveDrafts, setAnswerDraft], ); /** Selection state is per-question: a new pause must never inherit a stale @@ -184,31 +184,33 @@ export default function useAskAnswerMode(conversationId?: string | null) { * which morphTransition's synchronous flush requires. */ const collapse = useCallback(() => { if (liveAsk) { - const composerAnswer = formContext?.getValues('text') ?? answerText; + const composerAnswer = !batchMode ? (formContext?.getValues('text') ?? answerText) : ''; morphTransition(() => { - setAnswerDraft({ actionId: liveAsk.actionId, text: composerAnswer }); - if (!saveDrafts) { - formContext?.reset(); + if (!batchMode) { + setAnswerDraft({ actionId: liveAsk.actionId, text: composerAnswer }); + if (!saveDrafts) { + formContext?.reset(); + } } setCollapsedIds((prev) => prev.includes(liveAsk.actionId) ? prev : [...prev, liveAsk.actionId], ); }); } - }, [liveAsk, formContext, answerText, saveDrafts, setAnswerDraft, setCollapsedIds]); + }, [liveAsk, batchMode, formContext, answerText, saveDrafts, setAnswerDraft, setCollapsedIds]); const expand = useCallback(() => { if (liveAsk) { morphTransition(() => { /** Autosave restores the ask-specific draft after the key switch. If * drafts are disabled, perform that handoff directly. */ - if (!saveDrafts) { + if (!batchMode && !saveDrafts) { formContext?.setValue('text', answerText); } setCollapsedIds((prev) => prev.filter((id) => id !== liveAsk.actionId)); }); } - }, [liveAsk, saveDrafts, formContext, answerText, setCollapsedIds]); + }, [liveAsk, batchMode, saveDrafts, formContext, answerText, setCollapsedIds]); /** Pure check toggle: the keyboard highlight is steered only by the * composer's digit/arrow shortcuts, so a mouse toggle never leaves a diff --git a/packages/api/src/agents/run.ts b/packages/api/src/agents/run.ts index 5982312a44..87f96c7879 100644 --- a/packages/api/src/agents/run.ts +++ b/packages/api/src/agents/run.ts @@ -779,7 +779,7 @@ function shapeSummarizationConfig( * Below this context budget a summarization cycle cannot make progress: the * summary allocation rounds down to a handful of tokens, the rewritten history * still overflows, and the graph re-triggers summarization on every step until - * the recursion limit aborts the run — dozens of wasted LLM calls surfaced to + * the recursion limit aborts the run. Dozens of wasted LLM calls surfaced to * the user as an opaque LangGraph error. Falling back to plain pruning instead * either fits the request or fails fast with the actionable `empty_messages` * token-budget breakdown. Matches the floor `initializeAgent` applies when the