fix(chat): bound oversized tool diff previews (#3293)
Closes #3292. Keep #3286 open for the separate context-panel DiffView path. Thanks for fixing oversized tool-card previews.
This commit is contained in:
@@ -86,7 +86,7 @@ Use this doc when you ask an agent to change tool/header/description behavior.
|
||||
- Every other tool, including search/fetch, OpenCode built-ins, custom tools, plugins, and MCP tools, is **expandable** and renders through `ToolPart`.
|
||||
- The managed `openchamber` plugin tool uses the expandable path and hides its broad protocol input. The plugin supplies the selected action's human description as the native tool title; the UI renders that metadata without owning an action map. The full versioned result envelope renders through the same neutral JSON summary/tree/raw views as other tools, without a tool-specific output card.
|
||||
- `ToolPart` defers expanded content after a user toggle, preventing large tool input/output payloads from mounting during the initial chat render.
|
||||
- The rich tool diff preview lives in `ToolPartDiffPreview.tsx` and is lazy-loaded from `ToolPart`. It is the only tool-card piece that imports the `@pierre/diffs` + Shiki rendering stack, keeping that stack out of the eager chat startup graph. While its chunk loads (first rendered diff only) the plain-text patch from `PlainDiffFallback.tsx` renders as the Suspense fallback, mirroring the preview's error fallback. `ToolPart` itself must not statically import `@pierre/diffs` runtime modules or `@/lib/shiki/appThemeRegistry`.
|
||||
- The rich tool diff preview lives in `ToolPartDiffPreview.tsx` and is lazy-loaded from `ToolPart`. It is the only tool-card piece that imports the `@pierre/diffs` + Shiki rendering stack, keeping that stack out of the eager chat startup graph. While its chunk loads (first rendered diff only) the plain-text patch from `PlainDiffFallback.tsx` renders as the Suspense fallback, mirroring the preview's error fallback. Patches over 256 KiB or 2,000 lines skip rich parsing and use a bounded plain-text preview; navigation keeps the original patch. `ToolPart` itself must not statically import `@pierre/diffs` runtime modules or `@/lib/shiki/appThemeRegistry`.
|
||||
- The `@pierre/diffs` stack is knowingly unprotected against the JS/TS `template-call` backtracking that OOM'd the renderer in openchamber/openchamber#2587. Our own markdown Shiki worker sanitizes every grammar it loads (`@/lib/shiki/sanitizeTemplateCallGrammar`), but the diff worker pool runs `preferredHighlighter: 'shiki-wasm'` (`DiffWorkerProvider.tsx`) and resolves its languages by id through `@pierre/diffs`' own registry — `langs` accepts `SupportedLanguages` strings only, so there is no seam to hand it a pre-sanitized `LanguageRegistration`. A pathological template literal inside a rendered diff can therefore still hang that pool's Oniguruma engine. The available levers are upstream (a `langs` overload accepting grammar objects) or switching that pool to the JS regex engine; neither is done.
|
||||
- Running bash output falls back to `state.metadata.output` until canonical `state.output` arrives. Its output viewport grows with the content up to `46vh`, then scrolls and follows new output until the user scrolls up; following resumes when the user returns to the bottom. Live output appends or replaces rewritten snapshots as plain text without worker highlighting; finalized output normalizes ANSI terminal controls with a bounded synthetic-cell budget, bypasses the throttle, and receives the normal one-time highlighted rendering.
|
||||
- Thinking/Justification duration is hidden in `sorted` mode (handled in `ReasoningPart.tsx` + `JustificationBlock.tsx`).
|
||||
|
||||
@@ -1,19 +1,25 @@
|
||||
import React from 'react';
|
||||
|
||||
import { getToolDiffPreviewText } from './toolDiffPreview';
|
||||
|
||||
/**
|
||||
* Plain-text patch rendering used when the rich `@pierre/diffs` preview is
|
||||
* unavailable: non-diff render modes, preview errors, and while the lazily
|
||||
* loaded diff preview chunk is still downloading. Lives in its own module so
|
||||
* `ToolPart` can render it without importing the @pierre/diffs stack.
|
||||
*/
|
||||
export const PlainDiffFallback: React.FC<{ diff: string }> = ({ diff }) => (
|
||||
<pre
|
||||
className="m-0 overflow-auto whitespace-pre-wrap break-words rounded-lg p-2 typography-code"
|
||||
style={{
|
||||
backgroundColor: 'var(--syntax-base-background)',
|
||||
color: 'var(--syntax-base-foreground)',
|
||||
}}
|
||||
>
|
||||
{diff}
|
||||
</pre>
|
||||
);
|
||||
export const PlainDiffFallback: React.FC<{ diff: string }> = ({ diff }) => {
|
||||
const preview = React.useMemo(() => getToolDiffPreviewText(diff), [diff]);
|
||||
|
||||
return (
|
||||
<pre
|
||||
className="m-0 overflow-auto whitespace-pre-wrap break-words rounded-lg p-2 typography-code"
|
||||
style={{
|
||||
backgroundColor: 'var(--syntax-base-background)',
|
||||
color: 'var(--syntax-base-foreground)',
|
||||
}}
|
||||
>
|
||||
{preview}
|
||||
</pre>
|
||||
);
|
||||
};
|
||||
|
||||
@@ -22,6 +22,7 @@ import { FileTypeIcon } from '@/components/icons/FileTypeIcon';
|
||||
import { copyTextToClipboard } from '@/lib/clipboard';
|
||||
import type { ToolPopupContent } from '../types';
|
||||
import { PlainDiffFallback } from './PlainDiffFallback';
|
||||
import { isToolDiffPreviewOversized } from './toolDiffPreview';
|
||||
import { lazyWithChunkRecovery } from '@/lib/chunkLoadRecovery';
|
||||
|
||||
import {
|
||||
@@ -1200,11 +1201,15 @@ const renderAnimatedPathWithIcon = (path: string, animate = true, grow = true, s
|
||||
// Suspense fallback, mirroring the preview's own error fallback.
|
||||
const LazyToolPartDiffPreview = lazyWithChunkRecovery(() => import('./ToolPartDiffPreview'));
|
||||
|
||||
const DiffPreview: React.FC<{ diff: string; diffViewMode: DiffViewMode }> = ({ diff, diffViewMode }) => (
|
||||
<React.Suspense fallback={<PlainDiffFallback diff={diff} />}>
|
||||
<LazyToolPartDiffPreview diff={diff} diffViewMode={diffViewMode} />
|
||||
</React.Suspense>
|
||||
);
|
||||
const DiffPreview: React.FC<{ diff: string; diffViewMode: DiffViewMode }> = ({ diff, diffViewMode }) => {
|
||||
if (isToolDiffPreviewOversized(diff)) return <PlainDiffFallback diff={diff} />;
|
||||
|
||||
return (
|
||||
<React.Suspense fallback={<PlainDiffFallback diff={diff} />}>
|
||||
<LazyToolPartDiffPreview diff={diff} diffViewMode={diffViewMode} />
|
||||
</React.Suspense>
|
||||
);
|
||||
};
|
||||
|
||||
interface ToolExpandedContentProps {
|
||||
part: ToolPartType;
|
||||
|
||||
@@ -0,0 +1,29 @@
|
||||
export const TOOL_DIFF_PREVIEW_MAX_CHARS = 256 * 1024;
|
||||
export const TOOL_DIFF_PREVIEW_MAX_LINES = 2_000;
|
||||
|
||||
const getPreviewEnd = (diff: string): number | null => {
|
||||
const scanEnd = Math.min(diff.length, TOOL_DIFF_PREVIEW_MAX_CHARS);
|
||||
let lineCount = 1;
|
||||
|
||||
for (let index = 0; index < scanEnd; index += 1) {
|
||||
const character = diff.charCodeAt(index);
|
||||
if (character !== 10 && character !== 13) continue;
|
||||
|
||||
const separatorLength = character === 13 && diff.charCodeAt(index + 1) === 10 ? 2 : 1;
|
||||
if (index + separatorLength < diff.length) {
|
||||
lineCount += 1;
|
||||
if (lineCount > TOOL_DIFF_PREVIEW_MAX_LINES) return index;
|
||||
}
|
||||
if (separatorLength === 2) index += 1;
|
||||
}
|
||||
|
||||
return scanEnd < diff.length ? scanEnd : null;
|
||||
};
|
||||
|
||||
export const isToolDiffPreviewOversized = (diff: string): boolean => getPreviewEnd(diff) !== null;
|
||||
|
||||
export const getToolDiffPreviewText = (diff: string): string => {
|
||||
const previewEnd = getPreviewEnd(diff);
|
||||
if (previewEnd === null) return diff;
|
||||
return `${diff.slice(0, previewEnd).trimEnd()}\n…`;
|
||||
};
|
||||
@@ -10,6 +10,12 @@ import {
|
||||
getRenderablePatchInfo,
|
||||
resolveToolQuickOpenTarget,
|
||||
} from './toolDiffUtils';
|
||||
import {
|
||||
getToolDiffPreviewText,
|
||||
isToolDiffPreviewOversized,
|
||||
TOOL_DIFF_PREVIEW_MAX_CHARS,
|
||||
TOOL_DIFF_PREVIEW_MAX_LINES,
|
||||
} from './toolDiffPreview';
|
||||
|
||||
const identity = (path: string) => path;
|
||||
|
||||
@@ -182,6 +188,61 @@ describe('toolDiffUtils', () => {
|
||||
expect(entries[0]?.renderMode).toBe('text');
|
||||
expect(entries[0]?.patch).toContain('@@');
|
||||
});
|
||||
|
||||
test('keeps oversized metadata patches out of the rich diff renderer', () => {
|
||||
const patch = [
|
||||
'--- a/src/generated.ts',
|
||||
'+++ b/src/generated.ts',
|
||||
`@@ -1,${TOOL_DIFF_PREVIEW_MAX_LINES + 1} +0,0 @@`,
|
||||
...Array.from(
|
||||
{ length: TOOL_DIFF_PREVIEW_MAX_LINES + 1 },
|
||||
(_, index) => `-${String(index).padStart(40, '0')}`,
|
||||
),
|
||||
].join('\n');
|
||||
const metadata = {
|
||||
files: [{ relativePath: 'src/generated.ts', patch }],
|
||||
};
|
||||
const entries = getDiffPatchEntries(metadata, undefined, identity);
|
||||
|
||||
expect(entries).toHaveLength(1);
|
||||
expect(entries[0]?.renderMode).toBe('text');
|
||||
expect(entries[0]?.patch).toBe(patch);
|
||||
expect(resolveToolQuickOpenTarget('apply_patch', undefined, metadata)?.patch).toBe(patch);
|
||||
|
||||
const preview = getToolDiffPreviewText(entries[0]?.patch ?? '');
|
||||
expect(preview.endsWith('\n…')).toBe(true);
|
||||
expect(preview.split('\n')).toHaveLength(TOOL_DIFF_PREVIEW_MAX_LINES + 1);
|
||||
expect(preview.length).toBeLessThan(patch.length);
|
||||
});
|
||||
|
||||
test('bounds oversized single-line diff previews by character count', () => {
|
||||
const patch = `+${'x'.repeat(TOOL_DIFF_PREVIEW_MAX_CHARS + 1_000)}`;
|
||||
const preview = getToolDiffPreviewText(patch);
|
||||
|
||||
expect(preview.endsWith('\n…')).toBe(true);
|
||||
expect(preview.length).toBe(TOOL_DIFF_PREVIEW_MAX_CHARS + 2);
|
||||
});
|
||||
|
||||
test('preserves diff previews at the character and line limits', () => {
|
||||
const characterLimit = 'x'.repeat(TOOL_DIFF_PREVIEW_MAX_CHARS);
|
||||
const lineLimit = Array.from({ length: TOOL_DIFF_PREVIEW_MAX_LINES }, () => '+line').join('\n');
|
||||
const terminatedLineLimit = `${lineLimit}\n`;
|
||||
|
||||
expect(getToolDiffPreviewText(characterLimit)).toBe(characterLimit);
|
||||
expect(getToolDiffPreviewText(lineLimit)).toBe(lineLimit);
|
||||
expect(getToolDiffPreviewText(terminatedLineLimit)).toBe(terminatedLineLimit);
|
||||
});
|
||||
|
||||
test('counts bare carriage returns as line separators', () => {
|
||||
const patch = Array.from(
|
||||
{ length: TOOL_DIFF_PREVIEW_MAX_LINES + 1 },
|
||||
(_, index) => `+${index}`,
|
||||
).join('\r');
|
||||
|
||||
expect(isToolDiffPreviewOversized(patch)).toBe(true);
|
||||
expect(getToolDiffPreviewText(patch).split('\r')).toHaveLength(TOOL_DIFF_PREVIEW_MAX_LINES);
|
||||
});
|
||||
|
||||
test('resolves the quick-open target from the same entry the expanded card renders', () => {
|
||||
const patch = [
|
||||
'--- a/src/file.ts',
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
import { parsePatchFiles } from '@pierre/diffs';
|
||||
|
||||
import { isToolDiffPreviewOversized } from './toolDiffPreview';
|
||||
|
||||
export type DiffPatchEntry = {
|
||||
id: string;
|
||||
title: string;
|
||||
@@ -431,6 +433,15 @@ const getPatchEntriesFromText = (
|
||||
idPrefix: string,
|
||||
resolveTitle: (path: string) => string,
|
||||
): DiffPatchEntry[] => {
|
||||
if (isToolDiffPreviewOversized(patch)) {
|
||||
return [{
|
||||
id: `${idPrefix}-0`,
|
||||
title: resolveTitle(fallbackTitle),
|
||||
patch,
|
||||
renderMode: 'text',
|
||||
}];
|
||||
}
|
||||
|
||||
const normalized = normalizeLooseUnifiedPatch(patch);
|
||||
if (!normalized) {
|
||||
return [];
|
||||
|
||||
Reference in New Issue
Block a user