From 6e2435c9b8e3252eb7da7381304bae3861852c5b Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Fri, 28 Aug 2026 20:19:33 +0200 Subject: [PATCH] fix(client): address third round of Codex review findings 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. --- .../Chat/Messages/Content/AgentHandoff.tsx | 66 +++++++------------ .../Content/__tests__/AgentHandoff.test.tsx | 3 + .../Chat/Messages/Content/verticals.tsx | 23 +++++-- .../utils/__tests__/groupToolCalls.test.ts | 32 +++++++++ client/src/utils/groupToolCalls.ts | 47 +++++++++---- 5 files changed, 111 insertions(+), 60 deletions(-) diff --git a/client/src/components/Chat/Messages/Content/AgentHandoff.tsx b/client/src/components/Chat/Messages/Content/AgentHandoff.tsx index 6a35ac7492..a52c2222b1 100644 --- a/client/src/components/Chat/Messages/Content/AgentHandoff.tsx +++ b/client/src/components/Chat/Messages/Content/AgentHandoff.tsx @@ -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 = ({ name, args: _args = '' }) = const hasInfo = fields.length > 0; const agentName = targetAgent?.name || localize('com_ui_agent'); - const handleCopy = useCallback( - (e: MouseEvent) => { - 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 (
@@ -201,27 +192,16 @@ const AgentHandoff: React.FC = ({ name, args: _args = '' }) = > {localize('com_ui_handoff_instructions')} - - {isCopied ? ( -
{fields.length === 1 ? ( diff --git a/client/src/components/Chat/Messages/Content/__tests__/AgentHandoff.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/AgentHandoff.test.tsx index 22c8fde76d..499a20a8ae 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/AgentHandoff.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/AgentHandoff.test.tsx @@ -37,6 +37,9 @@ jest.mock('~/components/Share/MessageIcon', () => ({ jest.mock('lucide-react', () => ({ ChevronDown: () => , + /** The shared CopyButton/ActionButton composition renders both. */ + Copy: () => , + Check: () => , })); jest.mock('~/utils', () => ({ diff --git a/client/src/components/Chat/Messages/Content/verticals.tsx b/client/src/components/Chat/Messages/Content/verticals.tsx index 1845a901ba..d00f0cf385 100644 --- a/client/src/components/Chat/Messages/Content/verticals.tsx +++ b/client/src/components/Chat/Messages/Content/verticals.tsx @@ -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 `` 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 }; } diff --git a/client/src/utils/__tests__/groupToolCalls.test.ts b/client/src/utils/__tests__/groupToolCalls.test.ts index 12a8d4ce1e..40012b8a7a 100644 --- a/client/src/utils/__tests__/groupToolCalls.test.ts +++ b/client/src/utils/__tests__/groupToolCalls.test.ts @@ -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']); diff --git a/client/src/utils/groupToolCalls.ts b/client/src/utils/groupToolCalls.ts index 50468d7e4a..ec5d85fb37 100644 --- a/client/src/utils/groupToolCalls.ts +++ b/client/src/utils/groupToolCalls.ts @@ -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; }