From 120fa9047dc529a5b036ba4257a023aaed7cb382 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Fri, 10 Apr 2026 20:12:27 -0400 Subject: [PATCH] fix: normalize rename-only diff paths --- src/core/diffPaths.ts | 22 ++++++++++++++++++++++ src/core/loaders.test.ts | 24 ++++++++++++++++++++++++ src/core/loaders.ts | 20 +++++++++++++------- src/opentui/HunkDiffView.tsx | 11 +++++++---- src/ui/lib/files.test.ts | 20 +++++++++++++++++++- src/ui/lib/files.ts | 28 ++++++++++++++++++---------- 6 files changed, 103 insertions(+), 22 deletions(-) create mode 100644 src/core/diffPaths.ts diff --git a/src/core/diffPaths.ts b/src/core/diffPaths.ts new file mode 100644 index 00000000..1cc9bd62 --- /dev/null +++ b/src/core/diffPaths.ts @@ -0,0 +1,22 @@ +import type { FileDiffMetadata } from "@pierre/diffs"; + +/** Remove parser-added CR/LF suffixes from diff paths without touching meaningful spaces. */ +export function normalizeDiffPath(path: string | undefined) { + return path?.replace(/[\r\n]+$/u, ""); +} + +/** Sanitize parsed diff metadata path fields before the UI or loaders consume them. */ +export function normalizeDiffMetadataPaths(metadata: FileDiffMetadata): FileDiffMetadata { + const name = normalizeDiffPath(metadata.name) ?? metadata.name; + const prevName = normalizeDiffPath(metadata.prevName); + + if (name === metadata.name && prevName === metadata.prevName) { + return metadata; + } + + return { + ...metadata, + name, + prevName, + }; +} diff --git a/src/core/loaders.test.ts b/src/core/loaders.test.ts index 941453cc..6e2ddd0c 100644 --- a/src/core/loaders.test.ts +++ b/src/core/loaders.test.ts @@ -720,6 +720,30 @@ describe("loadAppBootstrap", () => { ).rejects.toThrow("`hunk stash show stash@{99}` could not resolve stash entry `stash@{99}`."); }); + test("strips parser-added line endings from rename-only paths", async () => { + const bootstrap = await loadAppBootstrap({ + kind: "patch", + text: [ + "diff --git a/pi/extensions/loop.ts b/agents/pi/extensions/notify.ts", + "similarity index 100%", + "rename from pi/extensions/loop.ts", + "rename to agents/pi/extensions/notify.ts", + ].join("\n"), + options: { mode: "auto" }, + }); + + expect(bootstrap.changeset.files).toHaveLength(1); + expect(bootstrap.changeset.files[0]).toMatchObject({ + path: "agents/pi/extensions/notify.ts", + previousPath: "pi/extensions/loop.ts", + metadata: { + name: "agents/pi/extensions/notify.ts", + prevName: "pi/extensions/loop.ts", + type: "rename-pure", + }, + }); + }); + test("treats malformed inline patch text as an empty review instead of throwing", async () => { const bootstrap = await loadAppBootstrap({ kind: "patch", diff --git a/src/core/loaders.ts b/src/core/loaders.ts index 5d381276..0eddbc90 100644 --- a/src/core/loaders.ts +++ b/src/core/loaders.ts @@ -9,6 +9,7 @@ import { createTwoFilesPatch } from "diff"; import { resolve as resolvePath } from "node:path"; import { findAgentFileContext, loadAgentContext } from "./agent"; import { createSkippedBinaryMetadata, isProbablyBinaryFile, patchLooksBinary } from "./binary"; +import { normalizeDiffMetadataPaths, normalizeDiffPath } from "./diffPaths"; import { buildGitDiffArgs, buildGitShowArgs, @@ -123,6 +124,7 @@ function findPatchChunk(metadata: FileDiffMetadata, chunks: string[], index: num return ( chunks.find((chunk) => [metadata.name, metadata.prevName] + .map(normalizeDiffPath) .filter((value): value is string => Boolean(value)) .map(stripPrefixes) .some( @@ -148,15 +150,19 @@ function buildDiffFile( agentContext: AgentContext | null, { isUntracked, previousPath, isBinary }: BuildDiffFileOptions = {}, ): DiffFile { + const normalizedMetadata = normalizeDiffMetadataPaths(metadata); + const path = normalizedMetadata.name; + const resolvedPreviousPath = normalizeDiffPath(previousPath) ?? normalizedMetadata.prevName; + return { - id: `${sourcePrefix}:${index}:${metadata.name}`, - path: metadata.name, - previousPath: previousPath ?? metadata.prevName, + id: `${sourcePrefix}:${index}:${path}`, + path, + previousPath: resolvedPreviousPath, patch, - language: getFiletypeFromFileName(metadata.name) ?? undefined, - stats: countDiffStats(metadata), - metadata, - agent: findAgentFileContext(agentContext, metadata.name, metadata.prevName), + language: getFiletypeFromFileName(path) ?? undefined, + stats: countDiffStats(normalizedMetadata), + metadata: normalizedMetadata, + agent: findAgentFileContext(agentContext, path, resolvedPreviousPath), isUntracked, isBinary: isBinary ?? patchLooksBinary(patch), }; diff --git a/src/opentui/HunkDiffView.tsx b/src/opentui/HunkDiffView.tsx index bbfea600..d59fd211 100644 --- a/src/opentui/HunkDiffView.tsx +++ b/src/opentui/HunkDiffView.tsx @@ -1,5 +1,6 @@ import { useMemo } from "react"; import { patchLooksBinary } from "../core/binary"; +import { normalizeDiffMetadataPaths, normalizeDiffPath } from "../core/diffPaths"; import type { DiffFile } from "../core/types"; import { findMaxLineNumber } from "../ui/diff/codeColumns"; import { buildSplitRows, buildStackRows } from "../ui/diff/pierre"; @@ -28,17 +29,19 @@ function countDiffStats(metadata: HunkDiffFile["metadata"]) { /** Adapt the public diff shape into Hunk's internal file model without exposing app-only fields. */ function toInternalDiffFile(diff: HunkDiffFile): DiffFile { const patch = diff.patch ?? ""; + const metadata = normalizeDiffMetadataPaths(diff.metadata); + const path = normalizeDiffPath(diff.path) ?? metadata.name; return { agent: null, id: diff.id, isBinary: patchLooksBinary(patch), language: diff.language, - metadata: diff.metadata, + metadata, patch, - path: diff.path ?? diff.metadata.name, - previousPath: diff.metadata.prevName, - stats: countDiffStats(diff.metadata), + path, + previousPath: metadata.prevName, + stats: countDiffStats(metadata), }; } diff --git a/src/ui/lib/files.test.ts b/src/ui/lib/files.test.ts index 00a62762..63adaa49 100644 --- a/src/ui/lib/files.test.ts +++ b/src/ui/lib/files.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import { createTestDiffFile, lines } from "../../../test/helpers/diff-helpers"; -import { buildSidebarEntries } from "./files"; +import { buildSidebarEntries, fileLabelParts } from "./files"; describe("files helpers", () => { test("buildSidebarEntries hides zero-value sidebar stats", () => { @@ -60,4 +60,22 @@ describe("files helpers", () => { deletionsText: null, }); }); + + test("fileLabelParts strips parser-added line endings from rename labels", () => { + const renamedAcrossDirectories = { + ...createTestDiffFile({ + id: "rename-across-dirs", + path: "agents/pi/extensions/notify.ts", + previousPath: "pi/extensions/loop.ts\n", + before: lines("export const stable = true;"), + after: lines("export const stable = true;"), + }), + stats: { additions: 0, deletions: 0 }, + }; + + expect(fileLabelParts(renamedAcrossDirectories)).toEqual({ + filename: "pi/extensions/loop.ts -> agents/pi/extensions/notify.ts", + stateLabel: null, + }); + }); }); diff --git a/src/ui/lib/files.ts b/src/ui/lib/files.ts index 86def67f..eed93142 100644 --- a/src/ui/lib/files.ts +++ b/src/ui/lib/files.ts @@ -1,5 +1,6 @@ import { basename, dirname } from "node:path/posix"; import type { FileDiffMetadata } from "@pierre/diffs"; +import { normalizeDiffPath } from "../../core/diffPaths"; import type { AgentAnnotation, DiffFile } from "../../core/types"; export interface FileListEntry { @@ -22,12 +23,15 @@ export type SidebarEntry = FileListEntry | FileGroupEntry; /** Build the filename-first label shown inside one sidebar row. */ function sidebarFileName(file: DiffFile) { - if (!file.previousPath || file.previousPath === file.path) { - return basename(file.path); + const path = normalizeDiffPath(file.path) ?? file.path; + const previousPath = normalizeDiffPath(file.previousPath); + + if (!previousPath || previousPath === path) { + return basename(path); } - const previousName = basename(file.previousPath); - const nextName = basename(file.path); + const previousName = basename(previousPath); + const nextName = basename(path); return previousName === nextName ? nextName : `${previousName} -> ${nextName}`; } @@ -91,7 +95,11 @@ export function filterReviewFiles(files: DiffFile[], query: string): DiffFile[] } return files.filter((file) => { - const haystack = [file.path, file.previousPath, file.agent?.summary] + const haystack = [ + normalizeDiffPath(file.path), + normalizeDiffPath(file.previousPath), + file.agent?.summary, + ] .filter(Boolean) .join(" ") .toLowerCase(); @@ -105,7 +113,8 @@ export function buildSidebarEntries(files: DiffFile[]): SidebarEntry[] { let activeGroup: string | null = null; files.forEach((file, index) => { - const group = dirname(file.path); + const path = normalizeDiffPath(file.path) ?? file.path; + const group = dirname(path); const nextGroup = group === "." ? null : group; if (nextGroup !== activeGroup) { @@ -148,10 +157,9 @@ export function fileLabelParts(file: DiffFile | undefined): { return { filename: "No file selected", stateLabel: null }; } - const baseLabel = - file.previousPath && file.previousPath !== file.path - ? `${file.previousPath} -> ${file.path}` - : file.path; + const path = normalizeDiffPath(file.path) ?? file.path; + const previousPath = normalizeDiffPath(file.previousPath); + const baseLabel = previousPath && previousPath !== path ? `${previousPath} -> ${path}` : path; // Determine state label for special cases let stateLabel: string | null = null;