Skip to content

feat(webview): display file diffs for tool call results #83

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
omercnet merged 1 commit into from
Jan 27, 2026
Merged

feat(webview): display file diffs for tool call results #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions media/main.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -889,3 +889,69 @@ kbd {
.thought-content {
padding: 10px 12px;
}

/* Diff Display Styles */
.diff-container {
margin: 8px 0;
border-radius: 4px;
overflow: hidden;
border: 1px solid var(--vscode-panel-border);
}

.diff-header {
font-weight: bold;
padding: 6px 10px;
background: var(--vscode-editor-lineHighlightBackground);
border-bottom: 1px solid var(--vscode-panel-border);
font-family: var(--vscode-editor-font-family);
font-size: var(--vscode-editor-font-size);
}

.diff-content {
font-family: var(--vscode-editor-font-family);
font-size: var(--vscode-editor-font-size);
margin: 0;
padding: 0;
overflow-x: auto;
}

.diff-line {
padding: 1px 10px;
white-space: pre;
line-height: 1.4;
}

.diff-add {
background-color: var(--vscode-diffEditor-insertedTextBackground);
color: var(
--vscode-diffEditor-insertedTextForeground,
var(--vscode-editor-foreground)
);
}

.diff-remove {
background-color: var(--vscode-diffEditor-removedTextBackground);
color: var(
--vscode-diffEditor-removedTextForeground,
var(--vscode-editor-foreground)
);
}

.diff-context {
color: var(--vscode-descriptionForeground);
}

.diff-empty {
padding: 10px;
color: var(--vscode-descriptionForeground);
font-style: italic;
text-align: center;
}

.diff-truncated {
padding: 6px 10px;
color: var(--vscode-descriptionForeground);
font-style: italic;
background: var(--vscode-editor-lineHighlightBackground);
border-top: 1px solid var(--vscode-panel-border);
}
77 changes: 77 additions & 0 deletions src/test/webview.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import {
ansiToHtml,
hasAnsiCodes,
getToolKindIcon,
computeLineDiff,
renderDiff,
type VsCodeApi,
type Tool,
type WebviewElements,
Expand Down Expand Up @@ -1261,4 +1263,79 @@ suite("Webview", () => {
assert.ok(html.includes('title="edit"'));
});
});

suite("computeLineDiff", () => {
test("returns empty array for empty inputs", () => {
const result = computeLineDiff("", "");
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test "returns empty array for empty inputs" passes empty strings to computeLineDiff, but the actual implementation treats empty strings as falsy and returns an empty array (line 240). However, empty strings ("") and null/undefined have different semantics - an empty string could represent an empty file, while null typically represents a non-existent file.

The current implementation conflates these cases: computeLineDiff("", "content") returns the same result as computeLineDiff(null, "content"), even though the first should show adding content to an existing empty file, while the second represents creating a new file.

Consider changing the logic to only check for null/undefined, not truthiness, to properly handle empty files.

Suggested change
const result = computeLineDiff("", "");
const result = computeLineDiff(null, null);

Copilot uses AI. Check for mistakes.
assert.strictEqual(result.length, 0);
});

test("marks all lines as add for new file", () => {
const result = computeLineDiff(null, "line1\nline2");
assert.strictEqual(result.length, 2);
assert.strictEqual(result[0].type, "add");
assert.strictEqual(result[0].line, "line1");
assert.strictEqual(result[1].type, "add");
assert.strictEqual(result[1].line, "line2");
});

test("marks all lines as remove for deleted file", () => {
const result = computeLineDiff("line1\nline2", null);
assert.strictEqual(result.length, 2);
assert.strictEqual(result[0].type, "remove");
assert.strictEqual(result[1].type, "remove");
});

test("marks old as remove and new as add for modified file", () => {
const result = computeLineDiff("old", "new");
assert.strictEqual(result.length, 2);
assert.strictEqual(result[0].type, "remove");
assert.strictEqual(result[0].line, "old");
assert.strictEqual(result[1].type, "add");
assert.strictEqual(result[1].line, "new");
});
});

suite("renderDiff", () => {
test("returns no changes message for empty diff", () => {
const result = renderDiff(undefined, "", "");
assert.ok(result.includes("diff-container"));
assert.ok(result.includes("No changes"));
});

test("renders file path header when provided", () => {
const result = renderDiff("/path/to/file.ts", null, "new content");
assert.ok(result.includes("diff-header"));
assert.ok(result.includes("/path/to/file.ts"));
});

test("renders additions with diff-add class", () => {
const result = renderDiff(undefined, null, "added line");
assert.ok(result.includes("diff-add"));
assert.ok(result.includes("+ added line"));
});

test("renders deletions with diff-remove class", () => {
const result = renderDiff(undefined, "removed line", null);
assert.ok(result.includes("diff-remove"));
assert.ok(result.includes("- removed line"));
});

test("escapes HTML in diff content", () => {
const result = renderDiff(
undefined,
null,
"<script>alert('xss')</script>"
);
assert.ok(result.includes("&lt;script&gt;"));
assert.ok(!result.includes("<script>alert"));
});

test("truncates large diffs", () => {
const manyLines = Array(600).fill("line").join("\n");
const result = renderDiff(undefined, null, manyLines);
assert.ok(result.includes("diff-truncated"));
assert.ok(result.includes("500"));
});
});
Comment on lines +1299 to +1340
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test suite is missing integration tests that verify diff HTML output works correctly when passed through getToolsHtml(). The current tests only verify:

  1. renderDiff() produces HTML with the correct classes (tests at lines 1299-1340)
  2. getToolsHtml() handles ANSI terminal output (tests at lines 1103-1161)

But there's no test that verifies:

const tools = {
  "tool-1": {
    name: "edit_file",
    output: renderDiff("/path/file.ts", "old", "new"),
    ...
  }
};
const html = getToolsHtml(tools);
// Verify the diff HTML is rendered correctly, not escaped

This missing test would have caught the critical HTML escaping bug where diff output gets double-escaped.

Copilot uses AI. Check for mistakes.
});
97 changes: 97 additions & 0 deletions src/views/webview/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,97 @@ export function hasAnsiCodes(text: string): boolean {
return /\x1b\[[0-9;]*m/.test(text);
}

export interface DiffLine {
type: "add" | "remove" | "context";
line: string;
}

/**
* Compute a simple line-by-line diff between old and new text.
* Returns an array of diff lines marked as add/remove/context.
Comment on lines +227 to +233
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DiffLine interface includes a "context" type in the union (line 227), but this type is never actually used in computeLineDiff(). The function only ever creates diff lines with type "add" or "remove".

Either remove "context" from the type definition, or implement context line detection in the diff algorithm. The presence of unused type options suggests incomplete implementation and could confuse future maintainers.

Suggested change
type: "add" | "remove" | "context";
line: string;
}
/**
* Compute a simple line-by-line diff between old and new text.
* Returns an array of diff lines marked as add/remove/context.
type: "add" | "remove";
line: string;
}
/**
* Compute a simple line-by-line diff between old and new text.
* Returns an array of diff lines marked as add/remove.

Copilot uses AI. Check for mistakes.
*/
export function computeLineDiff(
oldText: string | null | undefined,
newText: string | null | undefined
): DiffLine[] {
// Handle edge cases
if (!oldText && !newText) {
return [];
}
if (!oldText) {
// New file - all lines are additions
return newText!.split("\n").map((line) => ({ type: "add", line }));
}
if (!newText) {
Comment on lines +240 to +247
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation uses truthiness checks (!oldText, !newText) rather than explicit null/undefined checks. This causes empty strings to be treated the same as null/undefined, which is semantically incorrect:

  • Empty file ("") vs null (non-existent file) have different meanings
  • computeLineDiff("", "hello") incorrectly treats this as creating a new file, when it should show adding "hello" to an existing empty file

Change the checks to be explicit:

if (oldText === null || oldText === undefined) {
  // New file case
}
if (newText === null || newText === undefined) {
  // Deleted file case
}

This also aligns with the type definition that allows string | null | undefined and the ACP protocol specification mentioned in issue #43 which uses null for new/deleted files.

Suggested change
if (!oldText && !newText) {
return [];
}
if (!oldText) {
// New file - all lines are additions
return newText!.split("\n").map((line) => ({ type: "add", line }));
}
if (!newText) {
const oldIsAbsent = oldText === null || oldText === undefined;
const newIsAbsent = newText === null || newText === undefined;
if (oldIsAbsent && newIsAbsent) {
return [];
}
if (oldIsAbsent) {
// New file - all lines are additions
return newText!.split("\n").map((line) => ({ type: "add", line }));
}
if (newIsAbsent) {

Copilot uses AI. Check for mistakes.
// Deleted file - all lines are deletions
return oldText!.split("\n").map((line) => ({ type: "remove", line }));
}

// Simple line-by-line diff
const oldLines = oldText.split("\n");
const newLines = newText.split("\n");
const result: DiffLine[] = [];

// Simple algorithm: mark old lines as removed, new lines as added
// Future optimization: detect common lines and mark as context
for (const line of oldLines) {
result.push({ type: "remove", line });
}
for (const line of newLines) {
result.push({ type: "add", line });
}

Comment on lines +252 to +265
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current diff algorithm marks ALL old lines as removed and ALL new lines as added, which doesn't provide a useful diff visualization. For example, if only one line changes in a 100-line file, this will show 200 lines (100 deletions + 100 additions) instead of showing the single changed line with context.

While the comment at line 258 acknowledges this is a "simple algorithm" with "Future optimization" planned, this makes the diff feature almost unusable for real files. Even a basic longest common subsequence (LCS) algorithm would provide significantly better results by identifying unchanged lines to show as context.

Consider implementing at least a basic diff algorithm that identifies common lines, or document this limitation prominently in the PR description and user-facing documentation.

Suggested change
// Simple line-by-line diff
const oldLines = oldText.split("\n");
const newLines = newText.split("\n");
const result: DiffLine[] = [];
// Simple algorithm: mark old lines as removed, new lines as added
// Future optimization: detect common lines and mark as context
for (const line of oldLines) {
result.push({ type: "remove", line });
}
for (const line of newLines) {
result.push({ type: "add", line });
}
// Diff based on longest common subsequence (LCS) of lines
const oldLines = oldText.split("\n");
const newLines = newText.split("\n");
const m = oldLines.length;
const n = newLines.length;
// Build LCS length table
const lcsLengths: number[][] = Array(m + 1)
.fill(0)
.map(() => Array(n + 1).fill(0));
for (let i = 1; i <= m; i++) {
for (let j = 1; j <= n; j++) {
if (oldLines[i - 1] === newLines[j - 1]) {
lcsLengths[i][j] = lcsLengths[i - 1][j - 1] + 1;
} else {
lcsLengths[i][j] = Math.max(lcsLengths[i - 1][j], lcsLengths[i][j - 1]);
}
}
}
// Walk back through the table to build the diff
const result: DiffLine[] = [];
let i = m;
let j = n;
while (i > 0 && j > 0) {
if (oldLines[i - 1] === newLines[j - 1]) {
result.push({ type: "context", line: oldLines[i - 1] });
i--;
j--;
} else if (lcsLengths[i - 1][j] >= lcsLengths[i][j - 1]) {
result.push({ type: "remove", line: oldLines[i - 1] });
i--;
} else {
result.push({ type: "add", line: newLines[j - 1] });
j--;
}
}
while (i > 0) {
result.push({ type: "remove", line: oldLines[i - 1] });
i--;
}
while (j > 0) {
result.push({ type: "add", line: newLines[j - 1] });
j--;
}
// We built the diff from the end to the start, so reverse it
result.reverse();

Copilot uses AI. Check for mistakes.
return result;
}

export function renderDiff(
path: string | undefined,
oldText: string | null | undefined,
newText: string | null | undefined
): string {
const diffLines = computeLineDiff(oldText, newText);

if (diffLines.length === 0) {
return '<div class="diff-container"><div class="diff-empty">No changes</div></div>';
}

const truncated = diffLines.length > 500;
const linesToShow = truncated ? diffLines.slice(0, 500) : diffLines;

let html = '<div class="diff-container">';

if (path) {
html += '<div class="diff-header">' + escapeHtml(path) + "</div>";
}

html += '<pre class="diff-content">';

for (const diffLine of linesToShow) {
const prefix =
diffLine.type === "add" ? "+ " : diffLine.type === "remove" ? "- " : " ";
const className = "diff-line diff-" + diffLine.type;
html +=
'<div class="' +
className +
'">' +
escapeHtml(prefix + diffLine.line) +
"</div>";
}

html += "</pre>";

if (truncated) {
html +=
'<div class="diff-truncated">... (truncated, showing first 500 of ' +
diffLines.length +
" lines)</div>";
}
Comment on lines +280 to +310
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff rendering has its own truncation at 500 lines (line 280), but getToolsHtml() also truncates tool.output at 500 characters (line 351-352). This creates inconsistent behavior:

  1. For a diff with 600 lines, renderDiff() produces HTML for 500 lines + truncation message
  2. Then getToolsHtml() truncates this HTML string to 500 characters and appends "..."
  3. The result is broken HTML showing only a fragment of the diff container

This dual truncation needs to be reconciled. Either remove the character-based truncation for HTML output, or ensure the line-based truncation in renderDiff() produces output that stays well under the character limit.

Copilot uses AI. Check for mistakes.

html += "</div>";

return html;
}

export function getToolsHtml(
tools: Record<string, Tool>,
expandedToolId?: string | null
Expand Down Expand Up @@ -909,6 +1000,12 @@ export class WebviewController {
output = firstContent.content.text;
} else if (firstContent.type === "terminal") {
output = msg.terminalOutput || "";
} else if (firstContent.type === "diff") {
output = renderDiff(
firstContent.path,
firstContent.oldText,
firstContent.newText
);
Comment on lines +1004 to +1008
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When renderDiff() output is assigned to tool.output (line 1004-1008), it will later be processed by getToolsHtml() which wraps all output in <pre class="tool-output"> tags (around line 360). Since renderDiff() already returns a complete HTML structure with <div class="diff-container">, this creates nested containers with semantic and styling issues.

The fix for the HTML escaping issue (comment #1) should also address this wrapping problem, likely by detecting when tool.output contains pre-rendered HTML and handling it differently in getToolsHtml().

Copilot uses AI. Check for mistakes.
Comment on lines +1004 to +1008
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTML output from renderDiff() will be double-escaped when processed by getToolsHtml(). In getToolsHtml() at lines 354-357, if the output doesn't contain ANSI codes, it calls escapeHtml() on the output. Since diff HTML doesn't contain ANSI codes, it will be escaped, causing users to see raw HTML tags instead of the rendered diff.

To fix this, the code needs a way to indicate that tool.output contains pre-rendered HTML. Options include:

  1. Add a flag to the Tool interface (e.g., outputIsHtml: boolean)
  2. Check for specific markers in the output string (e.g., check if output starts with <div class="diff-container">)
  3. Store the content type alongside the output and check it in getToolsHtml()

The same pattern is used for ANSI terminal output (ansiToHtml returns HTML), but that works because hasAnsiCodes() detects the ANSI escape sequences in the original output before it's converted.

Suggested change
output = renderDiff(
firstContent.path,
firstContent.oldText,
firstContent.newText
);
const diffHtml = renderDiff(
firstContent.path,
firstContent.oldText,
firstContent.newText
);
// Convert HTML diff output to plain text to avoid double-escaping later.
output = diffHtml.replace(/<[^>]*>/g, "");

Copilot uses AI. Check for mistakes.
}
}

Expand Down
Loading