mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-10 22:41:14 +00:00
feat: show aggregated +/− line counts in FileChangesPanel header (#11618)
* feat: show aggregated +/− line counts in FileChangesPanel header * feat(FileChangesPanel): show merged diff relative to final file state * fix(webview): restrict readFileContent to paths inside the workspace * fix: add workspace-boundary validation to readFileContent to prevent path traversal * fix(tests): mock isPathOutsideWorkspace in readFileContent spec * fix(tests): mock isPathOutsideWorkspace in readFileContent spec * fix: use path.resolve/path.sep in readFileContent test mock for cross-platform compatibility The isPathOutsideWorkspace mock used hardcoded Unix-style path comparisons (/mock/workspace with forward slashes), which fails on Windows where path.resolve() produces paths with drive letters (C:\mock\workspace\...). Replace manual string normalization with path.resolve() and path.sep so the mock behaves correctly on both Windows and Unix. --------- Co-authored-by: Roo Code <roomote@roocode.com>
This commit is contained in:
parent
0d5b932d2e
commit
5db2062d0c
8 changed files with 358 additions and 7 deletions
|
|
@ -103,7 +103,10 @@ export interface ExtensionMessage {
|
|||
| "branchWorktreeIncludeResult"
|
||||
| "folderSelected"
|
||||
| "skills"
|
||||
| "fileContent"
|
||||
text?: string
|
||||
/** For fileContent: { path, content, error? } */
|
||||
fileContent?: { path: string; content: string | null; error?: string }
|
||||
payload?: any // eslint-disable-line @typescript-eslint/no-explicit-any
|
||||
checkpointWarning?: {
|
||||
type: "WAIT_TIMEOUT" | "INIT_TIMEOUT"
|
||||
|
|
@ -442,6 +445,7 @@ export interface WebviewMessage {
|
|||
| "openImage"
|
||||
| "saveImage"
|
||||
| "openFile"
|
||||
| "readFileContent"
|
||||
| "openMention"
|
||||
| "cancelTask"
|
||||
| "cancelAutoApproval"
|
||||
|
|
@ -783,6 +787,8 @@ export interface ClineSayTool {
|
|||
matchCount?: number
|
||||
diff?: string
|
||||
content?: string
|
||||
// Original file content before first edit (for merged diff display in FileChangesPanel)
|
||||
originalContent?: string
|
||||
// Unified diff statistics computed by the extension
|
||||
diffStats?: { added: number; removed: number }
|
||||
regex?: string
|
||||
|
|
|
|||
|
|
@ -150,6 +150,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
|
|||
...sharedMessageProps,
|
||||
diff: diffContent,
|
||||
content: unifiedPatch,
|
||||
originalContent,
|
||||
diffStats,
|
||||
isProtected: isWriteProtected,
|
||||
} satisfies ClineSayTool)
|
||||
|
|
@ -194,6 +195,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
|
|||
...sharedMessageProps,
|
||||
diff: diffContent,
|
||||
content: unifiedPatch,
|
||||
originalContent,
|
||||
diffStats,
|
||||
isProtected: isWriteProtected,
|
||||
} satisfies ClineSayTool)
|
||||
|
|
|
|||
|
|
@ -341,6 +341,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
|
|||
tool: "appliedDiff",
|
||||
path: getReadablePath(task.cwd, relPath),
|
||||
diff: sanitizedDiff,
|
||||
originalContent,
|
||||
isOutsideWorkspace,
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,210 @@
|
|||
// npx vitest core/webview/__tests__/webviewMessageHandler.readFileContent.spec.ts
|
||||
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest"
|
||||
|
||||
vi.mock("../../../api/providers/fetchers/modelCache")
|
||||
|
||||
vi.mock("vscode", () => ({
|
||||
window: {
|
||||
showInformationMessage: vi.fn(),
|
||||
showErrorMessage: vi.fn(),
|
||||
showTextDocument: vi.fn(),
|
||||
},
|
||||
workspace: {
|
||||
workspaceFolders: [{ uri: { fsPath: "/mock/workspace" } }],
|
||||
openTextDocument: vi.fn().mockResolvedValue({}),
|
||||
},
|
||||
}))
|
||||
|
||||
vi.mock("../../../i18n", () => ({
|
||||
t: vi.fn((key: string) => key),
|
||||
}))
|
||||
|
||||
vi.mock("fs/promises", () => {
|
||||
const readFile = vi.fn().mockResolvedValue("file content here")
|
||||
return {
|
||||
default: {
|
||||
rm: vi.fn(),
|
||||
mkdir: vi.fn(),
|
||||
readFile,
|
||||
writeFile: vi.fn(),
|
||||
},
|
||||
rm: vi.fn(),
|
||||
mkdir: vi.fn(),
|
||||
readFile,
|
||||
writeFile: vi.fn(),
|
||||
}
|
||||
})
|
||||
|
||||
vi.mock("../../../utils/fs")
|
||||
vi.mock("../../../utils/path")
|
||||
vi.mock("../../../utils/globalContext")
|
||||
|
||||
vi.mock("../../../utils/pathUtils", () => ({
|
||||
isPathOutsideWorkspace: vi.fn((filePath: string) => {
|
||||
const nodePath = require("path")
|
||||
const normalized = nodePath.resolve(filePath)
|
||||
const workspaceRoot = nodePath.resolve("/mock/workspace")
|
||||
// Path is inside workspace if it equals or is under workspace root
|
||||
if (normalized === workspaceRoot) return false
|
||||
if (normalized.startsWith(workspaceRoot + nodePath.sep)) return false
|
||||
return true
|
||||
}),
|
||||
}))
|
||||
|
||||
vi.mock("../../mentions/resolveImageMentions", () => ({
|
||||
resolveImageMentions: vi.fn(async ({ text, images }: { text: string; images?: string[] }) => ({
|
||||
text,
|
||||
images: [...(images ?? [])],
|
||||
})),
|
||||
}))
|
||||
|
||||
import { webviewMessageHandler } from "../webviewMessageHandler"
|
||||
import type { ClineProvider } from "../ClineProvider"
|
||||
import * as fs from "fs/promises"
|
||||
|
||||
const MOCK_CWD = "/mock/workspace/project"
|
||||
|
||||
const mockProvider = {
|
||||
getState: vi.fn(),
|
||||
postMessageToWebview: vi.fn(),
|
||||
customModesManager: {
|
||||
getCustomModes: vi.fn(),
|
||||
deleteCustomMode: vi.fn(),
|
||||
},
|
||||
context: {
|
||||
extensionPath: "/mock/extension/path",
|
||||
globalStorageUri: { fsPath: "/mock/global/storage" },
|
||||
},
|
||||
contextProxy: {
|
||||
context: {
|
||||
extensionPath: "/mock/extension/path",
|
||||
globalStorageUri: { fsPath: "/mock/global/storage" },
|
||||
},
|
||||
setValue: vi.fn(),
|
||||
getValue: vi.fn(),
|
||||
},
|
||||
log: vi.fn(),
|
||||
postStateToWebview: vi.fn(),
|
||||
getCurrentTask: vi.fn().mockReturnValue({ cwd: MOCK_CWD }),
|
||||
getTaskWithId: vi.fn(),
|
||||
createTaskWithHistoryItem: vi.fn(),
|
||||
cwd: MOCK_CWD,
|
||||
} as unknown as ClineProvider
|
||||
|
||||
describe("webviewMessageHandler - readFileContent path traversal prevention", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
vi.mocked(fs.readFile).mockResolvedValue("file content here")
|
||||
vi.mocked(mockProvider.getCurrentTask).mockReturnValue({ cwd: MOCK_CWD } as any)
|
||||
})
|
||||
|
||||
it("allows reading a file within the workspace using a relative path", async () => {
|
||||
await webviewMessageHandler(mockProvider, {
|
||||
type: "readFileContent",
|
||||
text: "src/index.ts",
|
||||
})
|
||||
|
||||
expect(fs.readFile).toHaveBeenCalled()
|
||||
expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
type: "fileContent",
|
||||
fileContent: expect.objectContaining({
|
||||
path: "src/index.ts",
|
||||
content: "file content here",
|
||||
}),
|
||||
}),
|
||||
)
|
||||
})
|
||||
|
||||
it("blocks path traversal with ../", async () => {
|
||||
await webviewMessageHandler(mockProvider, {
|
||||
type: "readFileContent",
|
||||
text: "../../../etc/passwd",
|
||||
})
|
||||
|
||||
expect(fs.readFile).not.toHaveBeenCalled()
|
||||
expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
type: "fileContent",
|
||||
fileContent: expect.objectContaining({
|
||||
path: "../../../etc/passwd",
|
||||
content: null,
|
||||
error: "Path is outside workspace",
|
||||
}),
|
||||
}),
|
||||
)
|
||||
})
|
||||
|
||||
it("blocks absolute paths outside the workspace", async () => {
|
||||
await webviewMessageHandler(mockProvider, {
|
||||
type: "readFileContent",
|
||||
text: "/etc/shadow",
|
||||
})
|
||||
|
||||
expect(fs.readFile).not.toHaveBeenCalled()
|
||||
expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
type: "fileContent",
|
||||
fileContent: expect.objectContaining({
|
||||
path: "/etc/shadow",
|
||||
content: null,
|
||||
error: "Path is outside workspace",
|
||||
}),
|
||||
}),
|
||||
)
|
||||
})
|
||||
|
||||
it("blocks traversal disguised in the middle of a path", async () => {
|
||||
await webviewMessageHandler(mockProvider, {
|
||||
type: "readFileContent",
|
||||
text: "src/../../../../etc/passwd",
|
||||
})
|
||||
|
||||
expect(fs.readFile).not.toHaveBeenCalled()
|
||||
expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
type: "fileContent",
|
||||
fileContent: expect.objectContaining({
|
||||
content: null,
|
||||
error: "Path is outside workspace",
|
||||
}),
|
||||
}),
|
||||
)
|
||||
})
|
||||
|
||||
it("returns error when no path is provided", async () => {
|
||||
await webviewMessageHandler(mockProvider, {
|
||||
type: "readFileContent",
|
||||
text: "",
|
||||
})
|
||||
|
||||
expect(fs.readFile).not.toHaveBeenCalled()
|
||||
expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
type: "fileContent",
|
||||
fileContent: expect.objectContaining({
|
||||
content: null,
|
||||
error: "No path provided",
|
||||
}),
|
||||
}),
|
||||
)
|
||||
})
|
||||
|
||||
it("allows reading a file using an absolute path within the workspace", async () => {
|
||||
await webviewMessageHandler(mockProvider, {
|
||||
type: "readFileContent",
|
||||
text: `${MOCK_CWD}/src/index.ts`,
|
||||
})
|
||||
|
||||
expect(fs.readFile).toHaveBeenCalled()
|
||||
expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
type: "fileContent",
|
||||
fileContent: expect.objectContaining({
|
||||
content: "file content here",
|
||||
}),
|
||||
}),
|
||||
)
|
||||
})
|
||||
})
|
||||
|
|
@ -63,6 +63,7 @@ import { openMention } from "../mentions"
|
|||
import { resolveImageMentions } from "../mentions/resolveImageMentions"
|
||||
import { RooIgnoreController } from "../ignore/RooIgnoreController"
|
||||
import { getWorkspacePath } from "../../utils/path"
|
||||
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
|
||||
import { Mode, defaultModeSlug } from "../../shared/modes"
|
||||
import { getModels, flushModels } from "../../api/providers/fetchers/modelCache"
|
||||
import { GetModelsOptions } from "../../shared/api"
|
||||
|
|
@ -1142,6 +1143,44 @@ export const webviewMessageHandler = async (
|
|||
}
|
||||
openFile(filePath, message.values as { create?: boolean; content?: string; line?: number })
|
||||
break
|
||||
case "readFileContent": {
|
||||
const relPath = message.text || ""
|
||||
if (!relPath) {
|
||||
provider.postMessageToWebview({
|
||||
type: "fileContent",
|
||||
fileContent: { path: relPath, content: null, error: "No path provided" },
|
||||
})
|
||||
break
|
||||
}
|
||||
try {
|
||||
const cwd = getCurrentCwd()
|
||||
if (!cwd) {
|
||||
provider.postMessageToWebview({
|
||||
type: "fileContent",
|
||||
fileContent: { path: relPath, content: null, error: "No workspace path available" },
|
||||
})
|
||||
break
|
||||
}
|
||||
const absPath = path.resolve(cwd, relPath)
|
||||
// Workspace-boundary validation: prevent path traversal attacks
|
||||
if (isPathOutsideWorkspace(absPath)) {
|
||||
provider.postMessageToWebview({
|
||||
type: "fileContent",
|
||||
fileContent: { path: relPath, content: null, error: "Path is outside workspace" },
|
||||
})
|
||||
break
|
||||
}
|
||||
const content = await fs.readFile(absPath, "utf-8")
|
||||
provider.postMessageToWebview({ type: "fileContent", fileContent: { path: relPath, content } })
|
||||
} catch (err) {
|
||||
const errorMsg = err instanceof Error ? err.message : String(err)
|
||||
provider.postMessageToWebview({
|
||||
type: "fileContent",
|
||||
fileContent: { path: relPath, content: null, error: errorMsg },
|
||||
})
|
||||
}
|
||||
break
|
||||
}
|
||||
case "openMention":
|
||||
openMention(getCurrentCwd(), message.text)
|
||||
break
|
||||
|
|
|
|||
|
|
@ -44,7 +44,11 @@ vi.mock("@src/components/common/CodeAccordian", () => ({
|
|||
),
|
||||
}))
|
||||
|
||||
function createFileEditMessage(path: string, diff: string): ClineMessage {
|
||||
function createFileEditMessage(
|
||||
path: string,
|
||||
diff: string,
|
||||
diffStats?: { added: number; removed: number },
|
||||
): ClineMessage {
|
||||
return {
|
||||
type: "ask",
|
||||
ask: "tool",
|
||||
|
|
@ -55,6 +59,7 @@ function createFileEditMessage(path: string, diff: string): ClineMessage {
|
|||
tool: "appliedDiff",
|
||||
path,
|
||||
diff,
|
||||
...(diffStats && { diffStats }),
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
|
@ -172,4 +177,23 @@ describe("FileChangesPanel", () => {
|
|||
fireEvent.click(accordianToggle)
|
||||
expect(accordianToggle).toHaveTextContent("expanded")
|
||||
})
|
||||
|
||||
it("hides aggregate stats when no diffStats are present", () => {
|
||||
const messages = [createFileEditMessage("src/a.ts", "diff a"), createFileEditMessage("src/b.ts", "diff b")]
|
||||
renderPanel(messages)
|
||||
|
||||
expect(screen.queryByTestId("total-added")).not.toBeInTheDocument()
|
||||
expect(screen.queryByTestId("total-removed")).not.toBeInTheDocument()
|
||||
})
|
||||
|
||||
it("shows aggregated + and - totals in the header when diffStats are present", () => {
|
||||
const messages = [
|
||||
createFileEditMessage("src/a.ts", "diff a", { added: 3, removed: 1 }),
|
||||
createFileEditMessage("src/b.ts", "diff b", { added: 2, removed: 5 }),
|
||||
]
|
||||
renderPanel(messages)
|
||||
|
||||
expect(screen.getByTestId("total-added")).toHaveTextContent("+5")
|
||||
expect(screen.getByTestId("total-removed")).toHaveTextContent("-6")
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1,8 +1,9 @@
|
|||
import { memo, useEffect, useMemo, useState, useCallback } from "react"
|
||||
import { memo, useEffect, useMemo, useState, useCallback, useRef } from "react"
|
||||
import { useTranslation } from "react-i18next"
|
||||
import { ChevronDown, ChevronRight, FileDiff } from "lucide-react"
|
||||
import { createTwoFilesPatch } from "diff"
|
||||
|
||||
import type { ClineMessage } from "@roo-code/types"
|
||||
import type { ClineMessage, ExtensionMessage } from "@roo-code/types"
|
||||
|
||||
import { Collapsible, CollapsibleContent, CollapsibleTrigger } from "@/components/ui"
|
||||
import { cn } from "@/lib/utils"
|
||||
|
|
@ -20,10 +21,14 @@ const FileChangesPanel = memo(({ clineMessages, className }: FileChangesPanelPro
|
|||
const { t } = useTranslation()
|
||||
const [panelExpanded, setPanelExpanded] = useState(false)
|
||||
const [expandedPaths, setExpandedPaths] = useState<Set<string>>(new Set())
|
||||
const [finalContentByPath, setFinalContentByPath] = useState<Record<string, string | null>>({})
|
||||
const pendingPathsRef = useRef<Set<string>>(new Set())
|
||||
|
||||
// Reset expanded file rows when switching to a different task (clineMessages identity change)
|
||||
// Reset expanded file rows and final content cache when switching to a different task
|
||||
useEffect(() => {
|
||||
setExpandedPaths(new Set())
|
||||
setFinalContentByPath({})
|
||||
pendingPathsRef.current = new Set()
|
||||
}, [clineMessages])
|
||||
|
||||
const fileChanges = useMemo(() => fileChangesFromMessages(clineMessages), [clineMessages])
|
||||
|
|
@ -40,6 +45,17 @@ const FileChangesPanel = memo(({ clineMessages, className }: FileChangesPanelPro
|
|||
return map
|
||||
}, [fileChanges])
|
||||
|
||||
// Aggregate total lines added/removed across all files for the panel header
|
||||
const totalStats = useMemo(() => {
|
||||
return fileChanges.reduce(
|
||||
(acc, e) => ({
|
||||
added: acc.added + (e.diffStats?.added ?? 0),
|
||||
removed: acc.removed + (e.diffStats?.removed ?? 0),
|
||||
}),
|
||||
{ added: 0, removed: 0 },
|
||||
)
|
||||
}, [fileChanges])
|
||||
|
||||
const togglePath = useCallback((path: string) => {
|
||||
setExpandedPaths((prev) => {
|
||||
const next = new Set(prev)
|
||||
|
|
@ -49,6 +65,38 @@ const FileChangesPanel = memo(({ clineMessages, className }: FileChangesPanelPro
|
|||
})
|
||||
}, [])
|
||||
|
||||
// Request final file content when a row is expanded and we have originalContent
|
||||
useEffect(() => {
|
||||
for (const path of expandedPaths) {
|
||||
const entries = byPath.get(path)
|
||||
if (!entries?.length) continue
|
||||
const originalContent = entries[0].originalContent
|
||||
const lookupPath = path.startsWith("./") ? path.slice(2) : path
|
||||
if (
|
||||
originalContent !== undefined &&
|
||||
!(lookupPath in finalContentByPath) &&
|
||||
!pendingPathsRef.current.has(lookupPath)
|
||||
) {
|
||||
pendingPathsRef.current.add(lookupPath)
|
||||
vscode.postMessage({ type: "readFileContent", text: lookupPath })
|
||||
}
|
||||
}
|
||||
}, [expandedPaths, byPath, finalContentByPath])
|
||||
|
||||
// Listen for fileContent responses
|
||||
useEffect(() => {
|
||||
const handler = (event: MessageEvent) => {
|
||||
const message: ExtensionMessage = event.data
|
||||
if (message.type === "fileContent" && message.fileContent?.path != null) {
|
||||
const fc = message.fileContent
|
||||
pendingPathsRef.current.delete(fc.path)
|
||||
setFinalContentByPath((prev) => ({ ...prev, [fc.path]: fc.content ?? null }))
|
||||
}
|
||||
}
|
||||
window.addEventListener("message", handler)
|
||||
return () => window.removeEventListener("message", handler)
|
||||
}, [])
|
||||
|
||||
if (fileChanges.length === 0) return null
|
||||
|
||||
const fileCount = byPath.size
|
||||
|
|
@ -69,12 +117,30 @@ const FileChangesPanel = memo(({ clineMessages, className }: FileChangesPanelPro
|
|||
<span className="text-sm font-medium">
|
||||
{t("chat:fileChangesInConversation.header", { count: fileCount })}
|
||||
</span>
|
||||
{totalStats.added > 0 || totalStats.removed > 0 ? (
|
||||
<div
|
||||
className="flex items-center gap-2 ml-auto shrink-0"
|
||||
aria-label={`${totalStats.added} lines added, ${totalStats.removed} lines removed`}>
|
||||
<span className="text-xs font-medium text-vscode-charts-green" data-testid="total-added">
|
||||
+{totalStats.added}
|
||||
</span>
|
||||
<span className="text-xs font-medium text-vscode-charts-red" data-testid="total-removed">
|
||||
-{totalStats.removed}
|
||||
</span>
|
||||
</div>
|
||||
) : null}
|
||||
</CollapsibleTrigger>
|
||||
<CollapsibleContent>
|
||||
<div className="flex flex-col gap-1 pb-2 pl-6">
|
||||
{Array.from(byPath.entries()).map(([path, entries]) => {
|
||||
// If multiple edits to same file, concatenate diffs with a separator
|
||||
const combinedDiff = entries.map((e) => e.diff).join("\n\n")
|
||||
const originalContent = entries[0].originalContent
|
||||
const lookupPath = path.startsWith("./") ? path.slice(2) : path
|
||||
const finalContent = finalContentByPath[lookupPath]
|
||||
const hasMergedDiff =
|
||||
originalContent !== undefined && finalContent != null && finalContent !== ""
|
||||
const displayDiff = hasMergedDiff
|
||||
? createTwoFilesPatch(path, path, originalContent, finalContent)
|
||||
: entries.map((e) => e.diff).join("\n\n")
|
||||
const combinedStats = entries.reduce(
|
||||
(acc, e) => ({
|
||||
added: acc.added + (e.diffStats?.added ?? 0),
|
||||
|
|
@ -87,7 +153,7 @@ const FileChangesPanel = memo(({ clineMessages, className }: FileChangesPanelPro
|
|||
<div key={path} className="rounded border border-vscode-panel-border overflow-hidden">
|
||||
<CodeAccordian
|
||||
path={path}
|
||||
code={combinedDiff}
|
||||
code={displayDiff}
|
||||
language="diff"
|
||||
isExpanded={isExpanded}
|
||||
onToggleExpand={() => togglePath(path)}
|
||||
|
|
|
|||
|
|
@ -8,6 +8,8 @@ export interface FileChangeEntry {
|
|||
path: string
|
||||
diff: string
|
||||
diffStats?: { added: number; removed: number }
|
||||
/** Original file content before first edit (for merged diff display) */
|
||||
originalContent?: string
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -56,6 +58,7 @@ export function fileChangesFromMessages(messages: ClineMessage[] | undefined): F
|
|||
path: tool.path,
|
||||
diff,
|
||||
diffStats: tool.diffStats,
|
||||
originalContent: tool.originalContent,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue