Skip to content

Commit 266650e

Browse files
hannesrudolphhannesrudolph
committed
fix(webview-ui): Address PR review findings for diff view improvements
- Fix stripCData() to properly handle HTML-encoded CDATA markers - Fix convertNewFileToUnifiedDiff() to emit valid hunks for empty files - Improve DiffView patch selection to match by filePath - Add performance optimization to disable highlighting for large diffs (>1000 lines) - Improve theme detection using proper VS Code theme classes - Extract duplicate diff stats logic into shared utility module
1 parent 55973b5 commit 266650e

File tree

6 files changed

+125
-147
lines changed

6 files changed

+125
-147
lines changed

‎webview-ui/src/components/chat/BatchDiffApproval.tsx‎

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import React, { memo, useState } from "react"
22
import CodeAccordian from "../common/CodeAccordian"
3+
import { computeUnifiedDiffStats } from "../../utils/diffStats"
34

45
interface FileDiff {
56
path: string
@@ -17,27 +18,6 @@ interface BatchDiffApprovalProps {
1718
ts: number
1819
}
1920

20-
/** Compute +/− from a unified diff (ignores headers/hunk lines) */
21-
function computeUnifiedStats(diff?: string): { added: number; removed: number } | null {
22-
if (!diff) return null
23-
let added = 0
24-
let removed = 0
25-
let saw = false
26-
for (const line of diff.split("\n")) {
27-
if (line.startsWith("+++ ") || line.startsWith("--- ") || line.startsWith("@@")) continue
28-
if (line.startsWith("+")) {
29-
added++
30-
saw = true
31-
} else if (line.startsWith("-")) {
32-
removed++
33-
saw = true
34-
}
35-
}
36-
return saw && (added > 0 || removed > 0) ? { added, removed } : null
37-
}
38-
39-
/* keep placeholder (legacy) – replaced by computeUnifiedStats after normalization */
40-
4121
export const BatchDiffApproval = memo(({ files = [], ts }: BatchDiffApprovalProps) => {
4222
const [expandedFiles, setExpandedFiles] = useState<Record<string, boolean>>({})
4323

@@ -58,7 +38,7 @@ export const BatchDiffApproval = memo(({ files = [], ts }: BatchDiffApprovalProp
5838
{files.map((file) => {
5939
// Use backend-provided unified diff only. No client-side fallback for apply_diff batches.
6040
const unified = file.content || ""
61-
const stats = computeUnifiedStats(unified)
41+
const stats = computeUnifiedDiffStats(unified)
6242

6343
return (
6444
<div key={`${file.path}-${ts}`}>

‎webview-ui/src/components/chat/ChatRow.tsx‎

Lines changed: 1 addition & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import Thumbnails from "../common/Thumbnails"
2525
import ImageBlock from "../common/ImageBlock"
2626
import ErrorRow from "./ErrorRow"
2727
import { extractUnifiedDiff } from "../../utils/diffUtils"
28+
import { computeDiffStats } from "../../utils/diffStats"
2829

2930
import McpResourceRow from "../mcp/McpResourceRow"
3031

@@ -117,66 +118,6 @@ const ChatRow = memo(
117118

118119
export default ChatRow
119120

120-
function computeDiffStats(diff?: string): { added: number; removed: number } | null {
121-
if (!diff) return null
122-
123-
// Strategy 1: Unified diff (+/- lines)
124-
let added = 0
125-
let removed = 0
126-
let sawPlusMinus = false
127-
for (const line of diff.split("\n")) {
128-
if (line.startsWith("+++ ") || line.startsWith("--- ") || line.startsWith("@@")) continue
129-
if (line.startsWith("+")) {
130-
added++
131-
sawPlusMinus = true
132-
} else if (line.startsWith("-")) {
133-
removed++
134-
sawPlusMinus = true
135-
}
136-
}
137-
if (sawPlusMinus) {
138-
if (added === 0 && removed === 0) return null
139-
return { added, removed }
140-
}
141-
142-
// Strategy 2: Roo multi-search-replace blocks
143-
// Count lines in SEARCH vs REPLACE sections across all blocks
144-
// Matches optional metadata lines and optional '-------' line
145-
const blockRegex =
146-
/<<<<<<?\s*SEARCH[\s\S]*?(?:^:start_line:.*\n)?(?:^:end_line:.*\n)?(?:^-------\s*\n)?([\s\S]*?)^(?:=======\s*\n)([\s\S]*?)^(?:>>>>>>> REPLACE)/gim
147-
148-
let hasBlocks = false
149-
added = 0
150-
removed = 0
151-
152-
const asLines = (s: string) => {
153-
// Normalize Windows newlines and trim trailing newline so counts reflect real lines
154-
const norm = s.replace(/\r\n/g, "\n")
155-
if (norm === "") return 0
156-
// Split, drop potential trailing empty caused by final newline
157-
const parts = norm.split("\n")
158-
return parts[parts.length - 1] === "" ? parts.length - 1 : parts.length
159-
}
160-
161-
let match: RegExpExecArray | null
162-
while ((match = blockRegex.exec(diff)) !== null) {
163-
hasBlocks = true
164-
const searchContent = match[1] ?? ""
165-
const replaceContent = match[2] ?? ""
166-
const searchCount = asLines(searchContent)
167-
const replaceCount = asLines(replaceContent)
168-
if (replaceCount > searchCount) added += replaceCount - searchCount
169-
else if (searchCount > replaceCount) removed += searchCount - replaceCount
170-
}
171-
172-
if (hasBlocks) {
173-
if (added === 0 && removed === 0) return null
174-
return { added, removed }
175-
}
176-
177-
return null
178-
}
179-
180121
export const ChatRowContent = ({
181122
message,
182123
lastModifiedMessage,

‎webview-ui/src/components/common/CodeAccordian.tsx‎

Lines changed: 2 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { ToolUseBlock, ToolUseBlockHeader } from "./ToolUseBlock"
88
import CodeBlock from "./CodeBlock"
99
import { PathTooltip } from "../ui/PathTooltip"
1010
import DiffView from "./DiffView"
11+
import { computeDiffStats } from "../../utils/diffStats"
1112

1213
interface CodeAccordianProps {
1314
path?: string
@@ -24,61 +25,6 @@ interface CodeAccordianProps {
2425
diffStats?: { added: number; removed: number }
2526
}
2627

27-
// Fallback computation of + / - counts from code (supports both unified diff and Roo's multi-search-replace blocks)
28-
function computeDiffStatsFromCode(diff?: string): { added: number; removed: number } | null {
29-
if (!diff) return null
30-
31-
// Strategy 1: unified diff markers
32-
let added = 0
33-
let removed = 0
34-
let sawPlusMinus = false
35-
for (const line of diff.split("\n")) {
36-
if (line.startsWith("+++ ") || line.startsWith("--- ") || line.startsWith("@@")) continue
37-
if (line.startsWith("+")) {
38-
added++
39-
sawPlusMinus = true
40-
} else if (line.startsWith("-")) {
41-
removed++
42-
sawPlusMinus = true
43-
}
44-
}
45-
if (sawPlusMinus) {
46-
if (added === 0 && removed === 0) return null
47-
return { added, removed }
48-
}
49-
50-
// Strategy 2: Roo multi-search-replace blocks
51-
const blockRegex =
52-
/<<<<<<?\s*SEARCH[\s\S]*?(?:^:start_line:.*\n)?(?:^:end_line:.*\n)?(?:^-------\s*\n)?([\s\S]*?)^(?:=======\s*\n)([\s\S]*?)^(?:>>>>>>> REPLACE)/gim
53-
54-
const asLines = (s: string) => {
55-
const norm = (s || "").replace(/\r\n/g, "\n")
56-
if (!norm) return 0
57-
const parts = norm.split("\n")
58-
return parts[parts.length - 1] === "" ? parts.length - 1 : parts.length
59-
}
60-
61-
let hasBlocks = false
62-
added = 0
63-
removed = 0
64-
65-
let match: RegExpExecArray | null
66-
while ((match = blockRegex.exec(diff)) !== null) {
67-
hasBlocks = true
68-
const searchCount = asLines(match[1] ?? "")
69-
const replaceCount = asLines(match[2] ?? "")
70-
if (replaceCount > searchCount) added += replaceCount - searchCount
71-
else if (searchCount > replaceCount) removed += searchCount - replaceCount
72-
}
73-
74-
if (hasBlocks) {
75-
if (added === 0 && removed === 0) return null
76-
return { added, removed }
77-
}
78-
79-
return null
80-
}
81-
8228
const CodeAccordian = ({
8329
path,
8430
code = "",
@@ -100,7 +46,7 @@ const CodeAccordian = ({
10046
const derivedStats = useMemo(() => {
10147
if (diffStats && (diffStats.added > 0 || diffStats.removed > 0)) return diffStats
10248
if ((language || inferredLanguage) && (language || inferredLanguage) === "diff") {
103-
return computeDiffStatsFromCode(source || code || "")
49+
return computeDiffStats(source || code || "")
10450
}
10551
return null
10652
}, [diffStats, language, inferredLanguage, source, code])

‎webview-ui/src/components/common/DiffView.tsx‎

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,11 @@ const DiffView = memo(({ source, filePath }: DiffViewProps) => {
2626
// Determine language from file path and prepare highlighter
2727
const normalizedLang = useMemo(() => normalizeLanguage(getLanguageFromPath(filePath || "") || "txt"), [filePath])
2828
const [highlighter, setHighlighter] = useState<any>(null)
29-
const isLightTheme = useMemo(
30-
() => typeof document !== "undefined" && document.body.className.toLowerCase().includes("light"),
31-
[],
32-
)
29+
const isLightTheme = useMemo(() => {
30+
if (typeof document === "undefined") return false
31+
const cls = document.body.className
32+
return /\bvscode-light\b|\bvscode-high-contrast-light\b/i.test(cls)
33+
}, [])
3334

3435
useEffect(() => {
3536
let mounted = true
@@ -45,8 +46,14 @@ const DiffView = memo(({ source, filePath }: DiffViewProps) => {
4546
}
4647
}, [normalizedLang])
4748

49+
// Disable syntax highlighting for large diffs (performance optimization)
50+
const shouldHighlight = useMemo(() => {
51+
const lineCount = source.split("\n").length
52+
return lineCount <= 1000 // Only highlight diffs with <= 1000 lines
53+
}, [source])
54+
4855
const renderHighlighted = (code: string): React.ReactNode => {
49-
if (!highlighter) return code
56+
if (!highlighter || !shouldHighlight) return code
5057
try {
5158
const hast: any = highlighter.codeToHast(code, {
5259
lang: normalizedLang,
@@ -87,7 +94,15 @@ const DiffView = memo(({ source, filePath }: DiffViewProps) => {
8794
if (!patches || patches.length === 0) return []
8895

8996
const lines: DiffLine[] = []
90-
const patch = patches[0]
97+
const patch = filePath
98+
? (patches.find((p) =>
99+
[p.newFileName, p.oldFileName].some(
100+
(n) => typeof n === "string" && (n === filePath || n?.endsWith("/" + filePath)),
101+
),
102+
) ?? patches[0])
103+
: patches[0]
104+
105+
if (!patch) return []
91106

92107
let prevHunk: any = null
93108
for (const hunk of patch.hunks) {
@@ -151,7 +166,7 @@ const DiffView = memo(({ source, filePath }: DiffViewProps) => {
151166
console.error("[DiffView] Failed to parse diff:", error)
152167
return []
153168
}
154-
}, [source])
169+
}, [source, filePath])
155170

156171
return (
157172
<div

‎webview-ui/src/utils/diffStats.ts‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
/**
2+
* Shared utility for computing diff statistics from various diff formats
3+
*/
4+
5+
/**
6+
* Compute +/− counts from a unified diff (ignores headers/hunk lines)
7+
*/
8+
export function computeUnifiedDiffStats(diff?: string): { added: number; removed: number } | null {
9+
if (!diff) return null
10+
11+
let added = 0
12+
let removed = 0
13+
let sawPlusMinus = false
14+
15+
for (const line of diff.split("\n")) {
16+
// Skip unified diff headers and hunk markers
17+
if (line.startsWith("+++ ") || line.startsWith("--- ") || line.startsWith("@@")) continue
18+
19+
if (line.startsWith("+")) {
20+
added++
21+
sawPlusMinus = true
22+
} else if (line.startsWith("-")) {
23+
removed++
24+
sawPlusMinus = true
25+
}
26+
}
27+
28+
if (sawPlusMinus && (added > 0 || removed > 0)) {
29+
return { added, removed }
30+
}
31+
32+
return null
33+
}
34+
35+
/**
36+
* Compute +/− counts from Roo's multi-search-replace block format
37+
*/
38+
export function computeSearchReplaceDiffStats(diff?: string): { added: number; removed: number } | null {
39+
if (!diff) return null
40+
41+
// Matches optional metadata lines and optional '-------' line
42+
const blockRegex =
43+
/<<<<<<?\s*SEARCH[\s\S]*?(?:^:start_line:.*\n)?(?:^:end_line:.*\n)?(?:^-------\s*\n)?([\s\S]*?)^(?:=======\s*\n)([\s\S]*?)^(?:>>>>>>> REPLACE)/gim
44+
45+
const asLines = (s: string) => {
46+
// Normalize Windows newlines and trim trailing newline so counts reflect real lines
47+
const norm = (s || "").replace(/\r\n/g, "\n")
48+
if (!norm) return 0
49+
const parts = norm.split("\n")
50+
return parts[parts.length - 1] === "" ? parts.length - 1 : parts.length
51+
}
52+
53+
let hasBlocks = false
54+
let added = 0
55+
let removed = 0
56+
57+
let match: RegExpExecArray | null
58+
while ((match = blockRegex.exec(diff)) !== null) {
59+
hasBlocks = true
60+
const searchContent = match[1] ?? ""
61+
const replaceContent = match[2] ?? ""
62+
const searchCount = asLines(searchContent)
63+
const replaceCount = asLines(replaceContent)
64+
65+
if (replaceCount > searchCount) {
66+
added += replaceCount - searchCount
67+
} else if (searchCount > replaceCount) {
68+
removed += searchCount - replaceCount
69+
}
70+
}
71+
72+
if (hasBlocks && (added > 0 || removed > 0)) {
73+
return { added, removed }
74+
}
75+
76+
return null
77+
}
78+
79+
/**
80+
* Compute diff stats from any supported diff format (unified or search-replace)
81+
* Tries unified diff format first, then falls back to search-replace format
82+
*/
83+
export function computeDiffStats(diff?: string): { added: number; removed: number } | null {
84+
if (!diff) return null
85+
86+
// Try unified diff format first
87+
const unifiedStats = computeUnifiedDiffStats(diff)
88+
if (unifiedStats) return unifiedStats
89+
90+
// Fall back to search-replace format
91+
return computeSearchReplaceDiffStats(diff)
92+
}

‎webview-ui/src/utils/diffUtils.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,10 @@ function isUnifiedDiff(s: string): boolean {
4444
function stripCData(s: string): string {
4545
return (
4646
s
47-
// Remove HTML-encoded and raw CDATA open/close (case-insensitive covers both)
47+
// First, normalize HTML-encoded CDATA markers to raw
48+
.replace(/&lt;!\[CDATA\[/gi, "<![CDATA[")
49+
.replace(/\]\]&gt;/gi, "]]>")
50+
// Then strip raw markers
4851
.replace(/<!\[CDATA\[/gi, "")
4952
.replace(/\]\]>/gi, "")
5053
)
@@ -61,9 +64,10 @@ function convertNewFileToUnifiedDiff(content: string, filePath?: string): string
6164
// Drop trailing empty item produced by a final newline so we count only real content lines
6265
const contentLines = parts[parts.length - 1] === "" ? parts.slice(0, -1) : parts
6366

67+
const count = contentLines.length
6468
let diff = `--- /dev/null\n`
6569
diff += `+++ ${fileName}\n`
66-
diff += `@@ -0,0 +1,${contentLines.length} @@\n`
70+
diff += `@@ -0,0 +${count ? 1 : 0},${count} @@\n`
6771

6872
for (const line of contentLines) {
6973
diff += `+${line}\n`

0 commit comments

Comments
 (0)