mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-10-22 18:13:27 +00:00
fix(client): address eighth round of Codex review findings
Compose the shared Button for the compact reasoning disclosure, keeping only the row geometry local so the focus ring and disabled handling come from the shared recipe. Lazy-mount the compact reasoning body with useLazyCollapseBody, as the standalone panel already does. Collapsed is the default whenever thoughts are hidden, and a streaming THINK part re-renders on every delta, so the full text was being mounted and laid out behind an invisible panel. Classify grouped memory failures with isMemoryFailureOutput. Those tools report failure in prose that generic isError parsing does not recognize, so a persisted call with no terminal status showed a failed card inside a group reporting success with no failure count. Announce the diff statistics to assistive technology. The +/- glyphs carry their meaning through color and sign, so aria-hidden left screen-reader users without the change summary; the counts are now also exposed as localized screen-reader text.
This commit is contained in:
Marco Beretta
parent
ad6818803b
commit
13877d6978
5 changed files with 74 additions and 24 deletions
|
|
@ -30,10 +30,21 @@ export default function CodeWindowHeader({ language, code, diffStats }: CodeWind
|
|||
<LangIcon lang={language} className="size-3.5 shrink-0" />
|
||||
{language}
|
||||
{diffStats && (
|
||||
<span className="flex items-center gap-1 font-normal tabular-nums" aria-hidden="true">
|
||||
<span className="text-status-success">+{diffStats.additions}</span>
|
||||
<span className="text-status-error">-{diffStats.deletions}</span>
|
||||
</span>
|
||||
<>
|
||||
<span className="flex items-center gap-1 font-normal tabular-nums" aria-hidden="true">
|
||||
<span className="text-status-success">+{diffStats.additions}</span>
|
||||
<span className="text-status-error">-{diffStats.deletions}</span>
|
||||
</span>
|
||||
{/* The glyphs above carry the meaning visually through color and
|
||||
sign, which neither reaches a screen reader, so the counts are
|
||||
announced as words instead of being dropped. */}
|
||||
<span className="sr-only">
|
||||
{localize('com_ui_diff_stats', {
|
||||
0: String(diffStats.additions),
|
||||
1: String(diffStats.deletions),
|
||||
})}
|
||||
</span>
|
||||
</>
|
||||
)}
|
||||
</span>
|
||||
<CopyButton isCopied={isCopied} onClick={handleCopy} label={localize('com_ui_copy_code')} />
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@ import copy from 'copy-to-clipboard';
|
|||
import { useAtomValue } from 'jotai';
|
||||
import { Lightbulb, ChevronDown } from 'lucide-react';
|
||||
import { ContentTypes } from 'librechat-data-provider';
|
||||
import { disclosureChevronVariants } from '@librechat/client';
|
||||
import { Button, disclosureChevronVariants } from '@librechat/client';
|
||||
import type { MouseEvent, FocusEvent } from 'react';
|
||||
import { ThinkingContent, ThinkingButton, ThinkingLabel, FloatingThinkingBar, useInViewport } from './Thinking';
|
||||
import { useLocalize, useExpandCollapse, useLazyCollapseBody } from '~/hooks';
|
||||
|
|
@ -292,13 +292,22 @@ export const ReasoningCompact = memo(
|
|||
const containerRef = useRef<HTMLDivElement>(null);
|
||||
const { ref: headerRef, inViewport: headerInViewport } = useInViewport();
|
||||
const { style: expandStyle, ref: expandRef } = useExpandCollapse(isExpanded);
|
||||
/** Collapsed is the default whenever thoughts are hidden, and a streaming
|
||||
* THINK part re-renders on every delta. Keeping the full text mounted
|
||||
* behind the invisible panel made long reasoning streams progressively
|
||||
* more expensive on top of the visible peek. */
|
||||
const { shouldRenderBody, mountBody, handleTransitionEnd } = useLazyCollapseBody(isExpanded);
|
||||
|
||||
const reasoningText = useMemo(() => stripThinkTags(reasoning), [reasoning]);
|
||||
|
||||
const handleToggle = useCallback((e: MouseEvent<HTMLButtonElement>) => {
|
||||
e.preventDefault();
|
||||
setIsExpanded((prev) => !prev);
|
||||
}, []);
|
||||
const handleToggle = useCallback(
|
||||
(e: MouseEvent<HTMLButtonElement>) => {
|
||||
e.preventDefault();
|
||||
mountBody();
|
||||
setIsExpanded((prev) => !prev);
|
||||
},
|
||||
[mountBody],
|
||||
);
|
||||
|
||||
const handleCopy = useCallback(() => {
|
||||
if (!copy(reasoningText, { format: 'text/plain' })) {
|
||||
|
|
@ -334,12 +343,12 @@ export const ReasoningCompact = memo(
|
|||
onBlur={handleBlur}
|
||||
>
|
||||
<div ref={headerRef} className="relative flex h-5 shrink-0 items-center gap-1.5">
|
||||
<button
|
||||
type="button"
|
||||
<Button
|
||||
variant="ghost"
|
||||
onClick={handleToggle}
|
||||
aria-expanded={isExpanded}
|
||||
aria-controls={contentId}
|
||||
className="group/disclosure inline-flex min-w-0 flex-1 items-center gap-2 text-text-secondary focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-border-heavy"
|
||||
className="group/disclosure h-auto min-w-0 flex-1 justify-start gap-2 rounded-none p-0 font-normal text-text-secondary hover:bg-transparent"
|
||||
>
|
||||
<Lightbulb className="size-4 shrink-0 text-text-secondary" aria-hidden="true" />
|
||||
<span className="tool-status-text font-medium">{label}</span>
|
||||
|
|
@ -350,7 +359,7 @@ export const ReasoningCompact = memo(
|
|||
)}
|
||||
aria-hidden="true"
|
||||
/>
|
||||
</button>
|
||||
</Button>
|
||||
{isExpanded && (
|
||||
<CopyButton
|
||||
isCopied={isCopied}
|
||||
|
|
@ -373,18 +382,23 @@ export const ReasoningCompact = memo(
|
|||
aria-label={label}
|
||||
aria-hidden={!isExpanded || undefined}
|
||||
style={expandStyle}
|
||||
onTransitionEnd={handleTransitionEnd}
|
||||
>
|
||||
<div className="overflow-hidden" ref={expandRef}>
|
||||
<div className="relative my-2 rounded-2xl border border-border-light bg-surface-secondary p-4 pb-9 text-text-secondary">
|
||||
<p className={cn('whitespace-pre-wrap leading-[26px]', fontSize)}>{reasoningText}</p>
|
||||
<FloatingThinkingBar
|
||||
isVisible={isBarVisible && isExpanded && !headerInViewport}
|
||||
isExpanded={isExpanded}
|
||||
onClick={handleToggle}
|
||||
content={reasoningText}
|
||||
contentId={contentId}
|
||||
/>
|
||||
</div>
|
||||
{shouldRenderBody && (
|
||||
<div className="relative my-2 rounded-2xl border border-border-light bg-surface-secondary p-4 pb-9 text-text-secondary">
|
||||
<p className={cn('whitespace-pre-wrap leading-[26px]', fontSize)}>
|
||||
{reasoningText}
|
||||
</p>
|
||||
<FloatingThinkingBar
|
||||
isVisible={isBarVisible && isExpanded && !headerInViewport}
|
||||
isExpanded={isExpanded}
|
||||
onClick={handleToggle}
|
||||
content={reasoningText}
|
||||
contentId={contentId}
|
||||
/>
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
|
|
|
|||
|
|
@ -8,6 +8,11 @@ jest.mock('~/hooks', () => ({
|
|||
style: { display: 'grid', gridTemplateRows: isExpanded ? '1fr' : '0fr' },
|
||||
ref: { current: null },
|
||||
}),
|
||||
useLazyCollapseBody: (isExpanded: boolean) => ({
|
||||
shouldRenderBody: isExpanded,
|
||||
mountBody: jest.fn(),
|
||||
handleTransitionEnd: jest.fn(),
|
||||
}),
|
||||
}));
|
||||
|
||||
jest.mock('../Thinking', () => ({
|
||||
|
|
@ -59,4 +64,14 @@ describe('ReasoningCompact', () => {
|
|||
expect(row).toHaveClass('mb-1', 'mt-0');
|
||||
expect(row).not.toHaveClass('mt-1');
|
||||
});
|
||||
|
||||
it('leaves the full reasoning text unmounted while collapsed', () => {
|
||||
/** Collapsed is the default whenever thoughts are hidden, and a streaming
|
||||
* THINK part re-renders on every delta, so keeping the whole text mounted
|
||||
* behind the invisible panel cost layout work for nothing. */
|
||||
render(<ReasoningCompact reasoning="A long stream of reasoning" label="Thoughts" />);
|
||||
|
||||
expect(screen.getByRole('button', { name: 'Thoughts' })).toBeInTheDocument();
|
||||
expect(screen.queryByText('A long stream of reasoning')).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -23,6 +23,7 @@ import { ToolAuthWarning, ToolAuthWarningContext } from './auth';
|
|||
import { useMCPIconMap, useMCPServerNames } from '~/hooks/MCP';
|
||||
import { resolveToolCallPhase } from '~/utils/toolCallPhase';
|
||||
import { AttachmentGroup, ReasoningCompact } from './Parts';
|
||||
import { isMemoryFailureOutput } from './Parts/MemoryCall';
|
||||
import { isError, StackedToolIcons } from './ToolOutput';
|
||||
import { isBashProgrammaticToolCall } from './routing';
|
||||
import { ASK_USER_QUESTION } from '~/utils/approval';
|
||||
|
|
@ -115,10 +116,18 @@ function getToolMeta(part: TMessageContentParts): ToolMeta | null {
|
|||
const completed = !!tc.output || tc.progress === 1;
|
||||
const name = tc.name ?? '';
|
||||
const iconName = isBashProgrammaticToolCall(name, tc.args) ? Tools.bash_tool : name;
|
||||
/** Memory tools report failure in prose ("Invalid key ...") that generic
|
||||
* `isError` parsing does not recognize, so `MemoryCall` classifies it with
|
||||
* its own predicate. Reuse that here or a persisted call with no terminal
|
||||
* status shows a failed card inside a group claiming success. */
|
||||
const failedOutput =
|
||||
name === 'set_memory' || name === 'delete_memory'
|
||||
? isMemoryFailureOutput(name, tc.output ?? '')
|
||||
: hasFailedOutput(tc.output);
|
||||
return {
|
||||
name,
|
||||
iconName,
|
||||
...resolveOutcome(runStepStatus, completed, hasFailedOutput(tc.output)),
|
||||
...resolveOutcome(runStepStatus, completed, failedOutput),
|
||||
};
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1197,6 +1197,7 @@
|
|||
"com_ui_description_placeholder": "Optional: Enter a description to display for the prompt",
|
||||
"com_ui_detailed": "Detailed",
|
||||
"com_ui_details": "Details",
|
||||
"com_ui_diff_stats": "{{0}} additions, {{1}} deletions",
|
||||
"com_ui_disabled": "Disabled",
|
||||
"com_ui_disabling": "Disabling...",
|
||||
"com_ui_dismiss": "Dismiss",
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue