mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
🩹 fix: Keep Unfilled Activity Labels Invisible and Unmask Endpoint Settings
Follow-up review round. Publishing the reservation on every batch made two latent rendering paths reachable on every run, and both are fixed here. Empty labels no longer change grouping. The previous pass still formed a tool-group for a textless label, which wrapped even a single tool call and pulled THINK parts inside it — and since a reservation is published the moment each batch ends, that applied during every generation and permanently after a blank or failed fill. An empty label now flushes the legacy way instead: it still delimits its batch, but the block re-splits exactly as it renders with the feature off. Parallel lanes no longer show a blank line. Lanes render raw parts, so an unfilled label had nothing to draw; empty ones are dropped. Making labels act as collapsible headers inside lanes is still a separate gap. Edited responses no longer offset on resume. The sync replaces initialResponse.content with the server's aggregatedContent, which already contains the kept prefix AND everything generated since — so its length is not the prefix length, and indices reconciled from that snapshot are already absolute. Offsetting again pushed the label past its slot onto a later part. The shift now applies only to a fresh edited submission. Activity settings resolve field by field. Selecting one config object whole meant any endpoints.all block — even one carrying nothing but headers — shadowed the named or custom endpoint and silently disabled activity labels everywhere. Global still wins per field. Adds groupToolCalls coverage for the invisible-while-empty contract, which is the part most likely to regress: it is normal state on every run, not an edge case.
This commit is contained in:
parent
0d7d250520
commit
a4f194ed7e
5 changed files with 130 additions and 21 deletions
|
|
@ -1,5 +1,7 @@
|
|||
import { memo, useMemo } from 'react';
|
||||
import { ContentTypes } from 'librechat-data-provider';
|
||||
import type { TMessageContentParts, SearchResultData, TAttachment } from 'librechat-data-provider';
|
||||
import { getActivityLabelPart, getActivityLabelText } from '~/utils/activityLabels';
|
||||
import MemoryArtifacts from './MemoryArtifacts';
|
||||
import Sources from '~/components/Web/Sources';
|
||||
import { SearchContext } from '~/Providers';
|
||||
|
|
@ -159,7 +161,17 @@ export const ParallelColumns = memo(function ParallelColumns({
|
|||
}: ParallelColumnsProps) {
|
||||
return (
|
||||
<div className={cn('flex w-full flex-col gap-3 md:flex-row', 'sibling-content-group')}>
|
||||
{columns.map(({ agentId, parts: columnParts }, colIdx) => {
|
||||
{columns.map(({ agentId, parts: allColumnParts }, colIdx) => {
|
||||
/** Lanes render raw parts, so an activity label cannot become a
|
||||
* collapsible header here (tracked separately). An UNFILLED one has
|
||||
* nothing to render at all, and every batch now publishes its
|
||||
* reservation immediately — so drop empty labels rather than emit a
|
||||
* blank line into the column while generation is pending. */
|
||||
const columnParts = allColumnParts.filter(
|
||||
({ part }) =>
|
||||
part?.type !== ContentTypes.ACTIVITY_LABEL ||
|
||||
getActivityLabelText(getActivityLabelPart(part)).length > 0,
|
||||
);
|
||||
// Show loading cursor if column has no content parts yet (empty array from placeholder)
|
||||
const showLoadingCursor = isSubmitting && columnParts.length === 0;
|
||||
|
||||
|
|
|
|||
|
|
@ -810,9 +810,16 @@ export default function useResumableSSE(
|
|||
* the run starts, and the server indexes only the NEW content — so
|
||||
* run steps offset by that prefix (`useStepHandler`). The label index
|
||||
* is claimed in the same server-side space and needs the identical
|
||||
* shift, or it lands inside the prefix and overwrites kept content. */
|
||||
* shift, or it lands inside the prefix and overwrites kept content.
|
||||
*
|
||||
* NOT on a resume: the sync replaces `initialResponse.content` with
|
||||
* the server's `aggregatedContent`, which already contains the prefix
|
||||
* AND everything generated since. Its length is not the prefix
|
||||
* length, and the indices reconciled from that snapshot are already
|
||||
* absolute — offsetting again would push the label past its slot and
|
||||
* overwrite a later part. */
|
||||
const initialContent =
|
||||
currentSubmission.editedContent != null
|
||||
!isResume && currentSubmission.editedContent != null
|
||||
? ((currentSubmission.initialResponse as TMessage | undefined)?.content ?? [])
|
||||
: [];
|
||||
const offsetEvent =
|
||||
|
|
|
|||
79
client/src/utils/__tests__/groupToolCalls.test.ts
Normal file
79
client/src/utils/__tests__/groupToolCalls.test.ts
Normal file
|
|
@ -0,0 +1,79 @@
|
|||
import { ContentTypes } from 'librechat-data-provider';
|
||||
import type { TMessageContentParts } from 'librechat-data-provider';
|
||||
import type { PartWithIndex } from '~/components/Chat/Messages/Content/ParallelContent';
|
||||
import { groupSequentialToolCalls } from '../groupToolCalls';
|
||||
|
||||
const toolCall = (id: string): TMessageContentParts =>
|
||||
({
|
||||
type: ContentTypes.TOOL_CALL,
|
||||
[ContentTypes.TOOL_CALL]: { id, name: 'web_search', args: '{}', output: 'ok' },
|
||||
}) as unknown as TMessageContentParts;
|
||||
|
||||
const think = (text: string): TMessageContentParts =>
|
||||
({ type: ContentTypes.THINK, [ContentTypes.THINK]: text }) as unknown as TMessageContentParts;
|
||||
|
||||
const label = (text: string): TMessageContentParts =>
|
||||
({
|
||||
type: ContentTypes.ACTIVITY_LABEL,
|
||||
[ContentTypes.ACTIVITY_LABEL]: text,
|
||||
pending: text.length === 0,
|
||||
}) as unknown as TMessageContentParts;
|
||||
|
||||
const withIndex = (parts: TMessageContentParts[]): PartWithIndex[] =>
|
||||
parts.map((part, idx) => ({ part, idx }));
|
||||
|
||||
describe('groupSequentialToolCalls with activity labels', () => {
|
||||
/**
|
||||
* Every batch publishes its reservation the moment the batch ends, so an
|
||||
* empty label is the NORMAL state while generation is in flight. It must
|
||||
* render exactly as the feature-off path does: a lone tool call stays a
|
||||
* single (no group wrapper) and THINK parts stay standalone.
|
||||
*/
|
||||
it('leaves a single tool call and its reasoning untouched while the label is empty', () => {
|
||||
const grouped = groupSequentialToolCalls(
|
||||
withIndex([think('deciding what to search'), toolCall('t1'), label('')]),
|
||||
);
|
||||
|
||||
expect(grouped).toHaveLength(2);
|
||||
expect(grouped[0]).toMatchObject({ type: 'single' });
|
||||
expect(grouped[1]).toMatchObject({ type: 'single' });
|
||||
/** No labelPart anywhere, and crucially no 'tool-group' wrapper. */
|
||||
expect(grouped.some((entry) => entry.type === 'tool-group')).toBe(false);
|
||||
});
|
||||
|
||||
/** A blank/failed fill is permanent, and must stay equally invisible. */
|
||||
it('keeps legacy splitting for two tool calls when the label never fills', () => {
|
||||
const grouped = groupSequentialToolCalls(
|
||||
withIndex([toolCall('t1'), toolCall('t2'), label('')]),
|
||||
);
|
||||
|
||||
/** Two adjacent tool calls group even without the feature. */
|
||||
expect(grouped).toHaveLength(1);
|
||||
expect(grouped[0]).toMatchObject({ type: 'tool-group' });
|
||||
expect((grouped[0] as { labelPart?: PartWithIndex }).labelPart).toBeUndefined();
|
||||
});
|
||||
|
||||
/** Once real text lands the block becomes one labeled group, THINK included. */
|
||||
it('absorbs reasoning into a labeled group once the label has text', () => {
|
||||
const grouped = groupSequentialToolCalls(
|
||||
withIndex([
|
||||
think('deciding what to search'),
|
||||
toolCall('t1'),
|
||||
label('Found the failing spec'),
|
||||
]),
|
||||
);
|
||||
|
||||
expect(grouped).toHaveLength(1);
|
||||
expect(grouped[0].type).toBe('tool-group');
|
||||
const group = grouped[0] as { parts: PartWithIndex[]; labelPart?: PartWithIndex };
|
||||
expect(group.parts).toHaveLength(2);
|
||||
expect(group.labelPart?.part).toMatchObject({
|
||||
[ContentTypes.ACTIVITY_LABEL]: 'Found the failing spec',
|
||||
});
|
||||
});
|
||||
|
||||
/** An empty orphan label has nothing to render and no block to delimit. */
|
||||
it('drops an empty orphan label entirely', () => {
|
||||
expect(groupSequentialToolCalls(withIndex([label('')]))).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
|
@ -70,19 +70,21 @@ export function groupSequentialToolCalls(parts: PartWithIndex[]): GroupedPart[]
|
|||
continue;
|
||||
}
|
||||
if (item.part.type === ContentTypes.ACTIVITY_LABEL) {
|
||||
/** A reserved-but-unfilled slot (and a failed/blank fill) still
|
||||
* DELIMITS its batch, so grouping does not re-shuffle when the text
|
||||
* lands — but it is not attached as a labelPart, leaving the group to
|
||||
* render its generic verb exactly as it would without the feature. */
|
||||
const hasText = getActivityLabelText(getActivityLabelPart(item.part)).length > 0;
|
||||
if (!hasText) {
|
||||
/** A reserved-but-unfilled slot (and a failed/blank fill) must be
|
||||
* INVISIBLE. Every batch now publishes its reservation immediately,
|
||||
* so forming a group here would wrap even a single tool call and pull
|
||||
* THINK parts inside it while generation is still pending. Flushing
|
||||
* the legacy way instead delimits the batch AND re-splits it exactly
|
||||
* as it renders with the feature off. */
|
||||
flushWithoutLabel();
|
||||
continue;
|
||||
}
|
||||
if (currentBlock.length > 0) {
|
||||
result.push({
|
||||
type: 'tool-group',
|
||||
parts: [...currentBlock],
|
||||
...(hasText && { labelPart: item }),
|
||||
});
|
||||
result.push({ type: 'tool-group', parts: [...currentBlock], labelPart: item });
|
||||
currentBlock = [];
|
||||
} else if (hasText) {
|
||||
} else {
|
||||
/** Orphan label (block parts hidden/filtered): renders standalone. */
|
||||
result.push({ type: 'single', part: item });
|
||||
}
|
||||
|
|
|
|||
|
|
@ -99,15 +99,24 @@ export function resolveActivityConfig(
|
|||
const endpoints = appConfig?.endpoints as
|
||||
| (Record<string, TEndpoint | undefined> & { all?: TEndpoint })
|
||||
| undefined;
|
||||
const config: Partial<TEndpoint> | undefined =
|
||||
endpoints?.all ?? endpoints?.[endpoint] ?? customEndpointConfig;
|
||||
/**
|
||||
* Resolved FIELD BY FIELD rather than by picking one config object whole.
|
||||
* Selecting wholesale means any `endpoints.all` block — even one carrying
|
||||
* nothing but `headers` — shadows the named/custom endpoint entirely and
|
||||
* silently disables activity labels everywhere. Global still wins per
|
||||
* field, so a real `all.activityLabel` keeps overriding the endpoint.
|
||||
*/
|
||||
const all = endpoints?.all as Partial<TEndpoint> | undefined;
|
||||
const named = (endpoints?.[endpoint] ?? customEndpointConfig) as Partial<TEndpoint> | undefined;
|
||||
const pick = <K extends keyof TEndpoint>(key: K): TEndpoint[K] | undefined =>
|
||||
all?.[key] ?? named?.[key];
|
||||
return {
|
||||
enabled: config?.activityLabel === true,
|
||||
model: config?.activityModel,
|
||||
endpoint: config?.activityEndpoint,
|
||||
prompt: config?.activityPrompt,
|
||||
maxPerRun: config?.activityMaxPerRun,
|
||||
charLimit: config?.activityCharLimit,
|
||||
enabled: pick('activityLabel') === true,
|
||||
model: pick('activityModel'),
|
||||
endpoint: pick('activityEndpoint'),
|
||||
prompt: pick('activityPrompt'),
|
||||
maxPerRun: pick('activityMaxPerRun'),
|
||||
charLimit: pick('activityCharLimit'),
|
||||
/** `titleModel` is the documented fallback below, not a field here. */
|
||||
};
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue