CERBERUS/LibreChat
1
0
Fork 0
mirror of https://github.com/danny-avila/LibreChat.git synced 2026-10-24 11:12:49 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 11

fix(client): address third round of Codex review findings

Browse source 
Reuse the shared CopyButton for the handoff instruction copy control, which
recreated hover, focus-ring, opacity, rounding and copied-state appearance
locally. The icon crossfade and tooltip now come from the shared primitive and
only the reveal-on-hover behavior stays local.

Keep unlabeled commentary out of collapsing tool groups. An unlabeled group
auto-collapses once complete, so folding commentary in hid the assistant's
prose behind a generic action header. The flush now splits the block at
commentary and groups each surrounding run on its own, restoring the shape the
previous fallback had while keeping reasoning folded in.

Drop shopping results without a link and places with nothing to name, for the
same reason the image results are filtered: every provider field is optional,
and an unusable entry still made the array nonempty, so the card rendered an
anchor with no destination that looked actionable but could not be opened.
This commit is contained in:
Marco Beretta 2026-08-28 20:19:33 +02:00
parent b6abd50416
commit 6e2435c9b8
No known key found for this signature in database
GPG key ID: D918033D8E74CC11
5 changed files with 111 additions and 60 deletions
Download patch file Download diff file Expand all files Collapse all files

66
client/src/components/Chat/Messages/Content/AgentHandoff.tsx
View file

@ -1,9 +1,8 @@
import React, { useEffect, useId, useMemo, useRef, useState, useCallback } from 'react';
import { ChevronDown } from 'lucide-react';
import { EModelEndpoint, Constants } from 'librechat-data-provider';
import { Clipboard, CheckMark, TooltipAnchor } from '@librechat/client';
import type { TMessage } from 'librechat-data-provider';
import type { MouseEvent } from 'react';
import CopyButton from '~/components/Messages/Content/CopyButton';
import { unescapeJsonString } from './Parts/parseJsonField';
import MessageIcon from '~/components/Share/MessageIcon';
import { toolPanelSpacingClassName } from './disclosure';
@ -124,26 +123,18 @@ const AgentHandoff: React.FC<AgentHandoffProps> = ({ name, args: _args = '' }) =
const hasInfo = fields.length > 0;
const agentName = targetAgent?.name || localize('com_ui_agent');
const handleCopy = useCallback(
(e: MouseEvent<HTMLButtonElement>) => {
e.stopPropagation();
navigator.clipboard.writeText(copyText).then(
() => {
clearTimeout(copiedTimerRef.current);
setIsCopied(true);
copiedTimerRef.current = setTimeout(() => setIsCopied(false), 2000);
},
() => {
setIsCopied(false);
},
);
},
[copyText],
);
const copyLabel = isCopied
? localize('com_ui_copied_to_clipboard')
: localize('com_ui_copy_to_clipboard');
const handleCopy = useCallback(() => {
navigator.clipboard.writeText(copyText).then(
() => {
clearTimeout(copiedTimerRef.current);
setIsCopied(true);
copiedTimerRef.current = setTimeout(() => setIsCopied(false), 2000);
},
() => {
setIsCopied(false);
},
);
}, [copyText]);
return (
<div className="my-2">
@ -201,27 +192,16 @@ const AgentHandoff: React.FC<AgentHandoffProps> = ({ name, args: _args = '' }) =
>
{localize('com_ui_handoff_instructions')}
</span>
<TooltipAnchor
description={copyLabel}
render={
<button
type="button"
onClick={handleCopy}
aria-label={copyLabel}
className={cn(
'flex shrink-0 items-center justify-center rounded-lg p-1 text-text-tertiary transition-opacity duration-150',
'opacity-60 group-focus-within/handoff:opacity-100 group-hover/handoff:opacity-100',
'hover:bg-surface-hover hover:text-text-primary',
'focus-visible:opacity-100 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-border-heavy',
)}
>
{isCopied ? (
<CheckMark className="h-3.5 w-3.5" aria-hidden="true" />
) : (
<Clipboard size="14" aria-hidden="true" />
)}
</button>
}
<CopyButton
isCopied={isCopied}
iconOnly
onClick={handleCopy}
label={localize('com_ui_copy_to_clipboard')}
copiedLabel={localize('com_ui_copied_to_clipboard')}
/** Only the reveal-on-hover behavior is local; the icon
* crossfade, tooltip, hover and focus ring come from the
* shared primitive. */
className="shrink-0 opacity-60 focus-visible:opacity-100 group-focus-within/handoff:opacity-100 group-hover/handoff:opacity-100"
/>
</div>
{fields.length === 1 ? (

3
client/src/components/Chat/Messages/Content/__tests__/AgentHandoff.test.tsx
View file

@ -37,6 +37,9 @@ jest.mock('~/components/Share/MessageIcon', () => ({
jest.mock('lucide-react', () => ({
ChevronDown: () => <span data-testid="chevron-down" />,
/** The shared CopyButton/ActionButton composition renders both. */
Copy: () => <span data-testid="copy-icon" />,
Check: () => <span data-testid="check-icon" />,
}));
jest.mock('~/utils', () => ({

23
client/src/components/Chat/Messages/Content/verticals.tsx
View file

@ -50,17 +50,28 @@ export function collectSearchVerticals(attachments?: TAttachment[]): SearchVerti
if (!data) {
continue;
}
/** Both image URLs are optional in the provider contract and `ImageStrip`
* needs one for the `<img>` src. Keeping an entry with neither still
* makes `images.length` nonzero, so the card renders a broken tile
* instead of omitting the unusable result. */
/** Every field of these provider records is optional, and each strip
* renders one as a link. An unusable entry still makes the array
* nonempty, so the card shows a broken tile or an anchor with no
* destination that looks actionable but cannot be opened. Drop them
* here rather than teaching three strips to render half a result:
* an image needs a src, a product needs somewhere to go, and a place
* needs something to name. */
for (const image of data.images ?? []) {
if (image.thumbnailUrl || image.imageUrl) {
images.push(image);
}
}
shopping.push(...(data.shopping ?? []));
places.push(...(data.places ?? []));
for (const item of data.shopping ?? []) {
if (item.link) {
shopping.push(item);
}
}
for (const place of data.places ?? []) {
if (place.name || place.address) {
places.push(place);
}
}
}
return { images, shopping, places };
}

32
client/src/utils/__tests__/groupToolCalls.test.ts
View file

@ -270,6 +270,38 @@ describe('groupSequentialToolCalls reasoning transparency', () => {
expect(result.map((g) => g.type)).toEqual(['tool-group', 'single', 'tool-group']);
});
/** An unlabeled group auto-collapses once complete, so commentary folded
* into it would hide the assistant's prose behind a generic action
* header. Reasoning still folds in; only commentary splits the run. */
it('keeps unlabeled commentary out of the collapsing tool group', () => {
const result = groupSequentialToolCalls(
withIndex([
toolCall('a'),
toolCall('b'),
text('here is what I found so far', 'commentary'),
toolCall('c'),
toolCall('d'),
label(''),
]),
);
expect(result.map((g) => g.type)).toEqual(['tool-group', 'single', 'tool-group']);
});
it('still folds reasoning into a run that commentary split', () => {
const result = groupSequentialToolCalls(
withIndex([
think('deciding'),
toolCall('a'),
text('a note mid-run', 'commentary'),
think('reconsidering'),
toolCall('b'),
label(''),
]),
);
expect(result.map((g) => g.type)).toEqual(['tool-group', 'single', 'tool-group']);
});
it('does not group transfer/handoff tool calls', () => {
const result = groupSequentialToolCalls(withIndex([transferPart(), transferPart()]));
expect(result.map((g) => g.type)).toEqual(['single', 'single']);

47
client/src/utils/groupToolCalls.ts
View file

@ -62,6 +62,14 @@ function isReasoningPart(part: TMessageContentParts): boolean {
return part.type === ContentTypes.THINK;
}
/** Assistant prose emitted mid-run. Grouping treats it as block-joining so a
* labeled batch can still claim the tools around it, but an UNLABELED group
* must never swallow it: that group auto-collapses once complete, which would
* hide the prose behind a generic action header. */
function isCommentaryPart(part: TMessageContentParts): boolean {
return part.type === ContentTypes.TEXT && (part as { phase?: string }).phase === 'commentary';
}
function countToolCalls(parts: PartWithIndex[]): number {
let count = 0;
for (const { part } of parts) {
@ -117,25 +125,42 @@ export function groupSequentialToolCalls(parts: PartWithIndex[]): GroupedPart[]
* before it belongs to earlier batches whose labels stayed empty. */
let claimStart = 0;
/** Splits the block at commentary so an unlabeled group only ever holds
* reasoning and tool calls; each surrounding run still has to earn the
* grouped chrome on its own. */
const flushWithoutLabel = () => {
const toolCallCount = countToolCalls(currentBlock);
const hasReasoning = currentBlock.some((p) => isReasoningPart(p.part));
if (toolCallCount >= 2 || (toolCallCount >= 1 && hasReasoning)) {
result.push({ type: 'tool-group', parts: [...currentBlock] });
} else {
for (const p of currentBlock) {
result.push({ type: 'single', part: p });
let run: PartWithIndex[] = [];
const emitRun = () => {
const toolCallCount = countToolCalls(run);
const hasReasoning = run.some((p) => isReasoningPart(p.part));
if (toolCallCount >= 2 || (toolCallCount >= 1 && hasReasoning)) {
result.push({ type: 'tool-group', parts: run });
} else {
for (const p of run) {
result.push({ type: 'single', part: p });
}
}
run = [];
};
for (const p of currentBlock) {
if (isCommentaryPart(p.part)) {
emitRun();
result.push({ type: 'single', part: p });
continue;
}
run.push(p);
}
emitRun();
currentBlock = [];
claimStart = 0;
};
for (const item of parts) {
const isCommentary =
item.part.type === ContentTypes.TEXT &&
(item.part as { phase?: string }).phase === 'commentary';
if (isGroupableToolCall(item.part) || item.part.type === ContentTypes.THINK || isCommentary) {
if (
isGroupableToolCall(item.part) ||
item.part.type === ContentTypes.THINK ||
isCommentaryPart(item.part)
) {
currentBlock.push(item);
continue;
}