From b9a8ba054d04fdf0d5a1f228108a64c14fa4c3ef Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Fri, 10 Apr 2026 16:55:04 +0700 Subject: [PATCH] fix(app): keep diff rows aligned for blank lines --- packages/app/src/components/git-diff-pane.tsx | 71 +++++++++--------- packages/app/src/utils/diff-layout.test.ts | 75 ++++++++++++++++++- packages/app/src/utils/diff-layout.ts | 39 ++++++++++ packages/app/src/utils/diff-rendering.test.ts | 23 ++++++ packages/app/src/utils/diff-rendering.ts | 16 ++++ 5 files changed, 187 insertions(+), 37 deletions(-) create mode 100644 packages/app/src/utils/diff-rendering.test.ts create mode 100644 packages/app/src/utils/diff-rendering.ts diff --git a/packages/app/src/components/git-diff-pane.tsx b/packages/app/src/components/git-diff-pane.tsx index 41e3dcf23..2d0c3d766 100644 --- a/packages/app/src/components/git-diff-pane.tsx +++ b/packages/app/src/components/git-diff-pane.tsx @@ -57,7 +57,12 @@ import { import { WORKSPACE_SECONDARY_HEADER_HEIGHT } from "@/constants/layout"; import { Fonts } from "@/constants/theme"; import { shouldAnchorHeaderBeforeCollapse } from "@/utils/git-diff-scroll"; -import { buildSplitDiffRows, type SplitDiffDisplayLine, type SplitDiffRow } from "@/utils/diff-layout"; +import { + buildSplitDiffRows, + buildUnifiedDiffLines, + type SplitDiffDisplayLine, + type SplitDiffRow, +} from "@/utils/diff-layout"; import { DropdownMenu, DropdownMenuContent, @@ -75,6 +80,11 @@ import { openExternalUrl } from "@/utils/open-external-url"; import { GitActionsSplitButton } from "@/components/git-actions-split-button"; import { usePanelStore } from "@/stores/panel-store"; import { buildWorkspaceExplorerStateKey } from "@/hooks/use-file-explorer-actions"; +import { + formatDiffContentText, + formatDiffGutterText, + hasVisibleDiffTokens, +} from "@/utils/diff-rendering"; export type { GitActionId, GitAction, GitActions } from "@/components/git-actions-policy"; @@ -165,7 +175,7 @@ function DiffGutterCell({ type === "remove" && styles.removeLineNumberText, ]} > - {lineNumber != null ? String(lineNumber) : ""} + {formatDiffGutterText(lineNumber)} ); @@ -178,10 +188,12 @@ function DiffTextLine({ line: DiffLine; wrapLines: boolean; }) { + const visibleTokens = hasVisibleDiffTokens(line.tokens) ? line.tokens : null; + return ( - {line.tokens && line.type !== "header" ? ( - + {line.type !== "header" && visibleTokens ? ( + ) : ( - {line.content || " "} + {formatDiffContentText(line.content)} )} @@ -207,10 +219,12 @@ function SplitTextLine({ line: SplitDiffDisplayLine | null; wrapLines: boolean; }) { + const visibleTokens = line && hasVisibleDiffTokens(line.tokens) ? line.tokens : null; + return ( - {line?.tokens ? ( - + {visibleTokens ? ( + ) : ( - {line?.content ?? ""} + {formatDiffContentText(line?.content)} )} @@ -240,6 +254,8 @@ function DiffLineView({ gutterWidth: number; wrapLines: boolean; }) { + const visibleTokens = hasVisibleDiffTokens(line.tokens) ? line.tokens : null; + return ( - {lineNumber != null ? String(lineNumber) : ""} + {formatDiffGutterText(lineNumber)} - {line.tokens && line.type !== "header" ? ( - + {line.type !== "header" && visibleTokens ? ( + ) : ( - {line.content || " "} + {formatDiffContentText(line.content)} )} @@ -287,6 +303,8 @@ function SplitDiffLine({ gutterWidth: number; wrapLines: boolean; }) { + const visibleTokens = line && hasVisibleDiffTokens(line.tokens) ? line.tokens : null; + return ( - {line?.lineNumber != null ? String(line.lineNumber) : ""} + {formatDiffGutterText(line?.lineNumber ?? null)} - {line?.tokens ? ( - + {visibleTokens ? ( + ) : ( - {line?.content ?? ""} + {formatDiffContentText(line?.content)} )} @@ -535,26 +553,7 @@ function DiffFileBody({ ); } - const computedLines: { line: DiffLine; lineNumber: number | null; key: string }[] = []; - for (const [hunkIndex, hunk] of file.hunks.entries()) { - let oldLineNo = hunk.oldStart; - let newLineNo = hunk.newStart; - for (const [lineIndex, line] of hunk.lines.entries()) { - let lineNumber: number | null = null; - if (line.type === "remove") { - lineNumber = oldLineNo; - oldLineNo++; - } else if (line.type === "add") { - lineNumber = newLineNo; - newLineNo++; - } else if (line.type === "context") { - lineNumber = newLineNo; - oldLineNo++; - newLineNo++; - } - computedLines.push({ line, lineNumber, key: `${hunkIndex}-${lineIndex}` }); - } - } + const computedLines = buildUnifiedDiffLines(file); if (wrapLines) { return ( diff --git a/packages/app/src/utils/diff-layout.test.ts b/packages/app/src/utils/diff-layout.test.ts index c5e756d81..a6f00f0cc 100644 --- a/packages/app/src/utils/diff-layout.test.ts +++ b/packages/app/src/utils/diff-layout.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { buildSplitDiffRows } from "./diff-layout"; +import { buildSplitDiffRows, buildUnifiedDiffLines } from "./diff-layout"; import type { ParsedDiffFile } from "@/hooks/use-checkout-diff-query"; function makeFile(lines: ParsedDiffFile["hunks"][number]["lines"]): ParsedDiffFile { @@ -79,3 +79,76 @@ describe("buildSplitDiffRows", () => { }); }); }); + +describe("buildUnifiedDiffLines", () => { + it("computes line numbers per line type within a hunk", () => { + const lines = buildUnifiedDiffLines( + makeFile([ + { type: "header", content: "@@ -10,3 +10,4 @@" }, + { type: "context", content: "before" }, + { type: "add", content: "inserted" }, + { type: "remove", content: "removed" }, + { type: "context", content: "after" }, + ]), + ); + + expect( + lines.map(({ line, lineNumber }) => ({ + type: line.type, + lineNumber, + content: line.content, + })), + ).toEqual([ + { type: "header", lineNumber: null, content: "@@ -10,3 +10,4 @@" }, + { type: "context", lineNumber: 10, content: "before" }, + { type: "add", lineNumber: 11, content: "inserted" }, + { type: "remove", lineNumber: 11, content: "removed" }, + { type: "context", lineNumber: 12, content: "after" }, + ]); + }); + + it("restarts numbering at each hunk boundary", () => { + const file: ParsedDiffFile = { + path: "example.ts", + isNew: false, + isDeleted: false, + additions: 1, + deletions: 0, + status: "ok", + hunks: [ + { + oldStart: 75, + oldCount: 2, + newStart: 75, + newCount: 3, + lines: [ + { type: "header", content: "@@ -75,2 +75,3 @@" }, + { type: "context", content: "first" }, + { type: "add", content: "inserted" }, + { type: "context", content: "second" }, + ], + }, + { + oldStart: 165, + oldCount: 2, + newStart: 166, + newCount: 2, + lines: [ + { type: "header", content: "@@ -165,2 +166,2 @@" }, + { type: "context", content: "third" }, + { type: "context", content: "fourth" }, + ], + }, + ], + }; + + const lines = buildUnifiedDiffLines(file); + + expect(lines[0]?.lineNumber).toBeNull(); + expect(lines[1]?.lineNumber).toBe(75); + expect(lines[3]?.lineNumber).toBe(77); + expect(lines[4]?.lineNumber).toBeNull(); + expect(lines[5]?.lineNumber).toBe(166); + expect(lines[6]?.lineNumber).toBe(167); + }); +}); diff --git a/packages/app/src/utils/diff-layout.ts b/packages/app/src/utils/diff-layout.ts index b5746eab3..e6d9a2473 100644 --- a/packages/app/src/utils/diff-layout.ts +++ b/packages/app/src/utils/diff-layout.ts @@ -7,6 +7,12 @@ export interface SplitDiffDisplayLine { lineNumber: number | null; } +export interface UnifiedDiffDisplayLine { + key: string; + line: DiffLine; + lineNumber: number | null; +} + export type SplitDiffRow = | { kind: "header"; @@ -61,6 +67,39 @@ function toDisplayLine(input: { }; } +export function buildUnifiedDiffLines(file: ParsedDiffFile): UnifiedDiffDisplayLine[] { + const lines: UnifiedDiffDisplayLine[] = []; + + for (const [hunkIndex, hunk] of file.hunks.entries()) { + let oldLineNo = hunk.oldStart; + let newLineNo = hunk.newStart; + + for (const [lineIndex, line] of hunk.lines.entries()) { + let lineNumber: number | null = null; + + if (line.type === "remove") { + lineNumber = oldLineNo; + oldLineNo += 1; + } else if (line.type === "add") { + lineNumber = newLineNo; + newLineNo += 1; + } else if (line.type === "context") { + lineNumber = newLineNo; + oldLineNo += 1; + newLineNo += 1; + } + + lines.push({ + key: `${hunkIndex}-${lineIndex}`, + line, + lineNumber, + }); + } + } + + return lines; +} + export function buildSplitDiffRows(file: ParsedDiffFile): SplitDiffRow[] { const rows: SplitDiffRow[] = []; diff --git a/packages/app/src/utils/diff-rendering.test.ts b/packages/app/src/utils/diff-rendering.test.ts new file mode 100644 index 000000000..c63f18e5f --- /dev/null +++ b/packages/app/src/utils/diff-rendering.test.ts @@ -0,0 +1,23 @@ +import { describe, expect, it } from "vitest"; + +import { formatDiffContentText, formatDiffGutterText, hasVisibleDiffTokens } from "./diff-rendering"; + +describe("diff-rendering", () => { + it("keeps header gutters tall even when they do not show a line number", () => { + expect(formatDiffGutterText(null)).toBe(" "); + expect(formatDiffGutterText(82)).toBe("82"); + }); + + it("keeps empty split cells tall even when they have no visible content", () => { + expect(formatDiffContentText(undefined)).toBe(" "); + expect(formatDiffContentText("")).toBe(" "); + expect(formatDiffContentText("const value = 1;")).toBe("const value = 1;"); + }); + + it("treats empty highlighted token rows as blank lines instead of visible content", () => { + expect(hasVisibleDiffTokens(undefined)).toBe(false); + expect(hasVisibleDiffTokens([])).toBe(false); + expect(hasVisibleDiffTokens([{ text: "" }])).toBe(false); + expect(hasVisibleDiffTokens([{ text: "const value = 1;" }])).toBe(true); + }); +}); diff --git a/packages/app/src/utils/diff-rendering.ts b/packages/app/src/utils/diff-rendering.ts new file mode 100644 index 000000000..ec18716c7 --- /dev/null +++ b/packages/app/src/utils/diff-rendering.ts @@ -0,0 +1,16 @@ +interface HighlightLikeToken { + text: string; +} + +// Preserve row height when a gutter or diff cell is intentionally blank. +export function formatDiffGutterText(lineNumber: number | null): string { + return lineNumber == null ? " " : String(lineNumber); +} + +export function formatDiffContentText(content: string | null | undefined): string { + return content && content.length > 0 ? content : " "; +} + +export function hasVisibleDiffTokens(tokens: HighlightLikeToken[] | null | undefined): boolean { + return Boolean(tokens?.some((token) => token.text.length > 0)); +}