From 7f8a485a77f3ba0da303bc0546bd9d2c1e67b7f6 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Sun, 16 Aug 2026 09:38:12 +0200 Subject: [PATCH] Keep large diff reviews responsive (#3422) * perf(app): bound large diff row mounting Keep the existing vertical and horizontal scroll owners so virtualization does not change gesture or scrolling behavior. Reserve exact offscreen row heights while mounting only the visible window, and retain the full renderer when wrapping or editing reviews requires dynamic geometry. * perf(app): keep large diff reviews responsive Opening an inline editor changed the whole review action object, reconciled large syntax-token trees, and disabled row virtualization. Keep the expensive token subtree stable and preserve deterministic row geometry while the editor is open.\n\nBound server-side highlighting for pathological single lines and remove the unused client highlighter so the actual diff pipeline owns the limit. * test(app): check mounted diff typography Row virtualization can unmount a fixed diff row after scrolling. Assert the configured font size against a currently mounted row so the test continues to verify rendered typography without depending on window position. * test(app): stabilize Mermaid streaming coverage --- packages/app/e2e/browser/changes-pane.spec.ts | 8 +- .../app/e2e/browser/diff-performance.spec.ts | 184 +++++++ .../app/e2e/browser/mermaid-streaming.spec.ts | 2 + packages/app/e2e/support/helpers/diagram.ts | 4 +- packages/app/src/git/diff-pane.tsx | 269 +++++++-- .../app/src/git/diff-virtualization.test.ts | 54 ++ packages/app/src/git/diff-virtualization.ts | 143 +++++ packages/app/src/review/surface.test.tsx | 21 +- packages/app/src/review/surface.tsx | 23 +- .../app/src/utils/diff-highlighter.test.ts | 520 ------------------ packages/app/src/utils/diff-highlighter.ts | 313 ----------- packages/app/src/utils/diff-rendering.test.ts | 20 + packages/app/src/utils/diff-rendering.ts | 22 + .../src/server/utils/diff-highlighter.test.ts | 56 ++ .../src/server/utils/diff-highlighter.ts | 46 +- 15 files changed, 755 insertions(+), 930 deletions(-) create mode 100644 packages/app/e2e/browser/diff-performance.spec.ts create mode 100644 packages/app/src/git/diff-virtualization.test.ts create mode 100644 packages/app/src/git/diff-virtualization.ts delete mode 100644 packages/app/src/utils/diff-highlighter.test.ts delete mode 100644 packages/app/src/utils/diff-highlighter.ts diff --git a/packages/app/e2e/browser/changes-pane.spec.ts b/packages/app/e2e/browser/changes-pane.spec.ts index 87f41af01..03ad5e802 100644 --- a/packages/app/e2e/browser/changes-pane.spec.ts +++ b/packages/app/e2e/browser/changes-pane.spec.ts @@ -669,11 +669,13 @@ async function expectFlatFileList(page: Page): Promise { } async function expectDiffCodeFontSize(page: Page, fontSize: number): Promise { + const visibleCodeText = page + .getByTestId(/^diff-code-text-/) + .filter({ visible: true }) + .first(); await expect .poll(async () => { - return page - .getByTestId("diff-code-text-1") - .evaluate((text) => Number.parseFloat(getComputedStyle(text).fontSize)); + return visibleCodeText.evaluate((text) => Number.parseFloat(getComputedStyle(text).fontSize)); }) .toBe(fontSize); } diff --git a/packages/app/e2e/browser/diff-performance.spec.ts b/packages/app/e2e/browser/diff-performance.spec.ts new file mode 100644 index 000000000..46f776236 --- /dev/null +++ b/packages/app/e2e/browser/diff-performance.spec.ts @@ -0,0 +1,184 @@ +import { writeFile } from "node:fs/promises"; +import path from "node:path"; +import type { CDPSession } from "@playwright/test"; +import { buildHostWorkspaceRoute } from "../../src/utils/host-routes"; +import { test, expect } from "../support/fixtures"; +import { getServerId } from "../support/helpers/server-id"; +import { connectSeedClient } from "../support/helpers/seed-client"; +import { createTempGitRepo } from "../support/helpers/workspace"; +import { waitForWorkspaceTabsVisible } from "../support/helpers/workspace-tabs"; + +const RUN_DIFF_PERF = process.env.PASEO_DIFF_PERF_E2E === "1"; +const diffPerfDescribe = RUN_DIFF_PERF ? test.describe : test.describe.skip; +const LINE_COUNT = 2_400; +const CPU_SLOWDOWN = Number(process.env.PASEO_DIFF_PERF_CPU_SLOWDOWN ?? 6); +const MAX_MOUNTED_CODE_ROWS = 220; +const MAX_EXPANSION_MS = 6_000; +const CHANGES_PREFERENCES_KEY = "@paseo:changes-preferences"; + +diffPerfDescribe("Diff rendering performance", () => { + test("large unwrapped diffs keep mounting bounded under CPU slowdown", async ({ + page, + }, testInfo) => { + test.setTimeout(90_000); + const repo = await createTempGitRepo("diff-perf-", { + files: [{ path: "src/large.ts", content: "export const baseline = 0;\n" }], + }); + const client = await connectSeedClient(); + let cdp: CDPSession | null = null; + + try { + const generatedLines = Array.from( + { length: LINE_COUNT }, + (_, index) => `export const generated_${index} = "${index}-${"wide".repeat(16)}";`, + ).join("\n"); + await writeFile( + path.join(repo.path, "src/large.ts"), + `export const baseline = 1;\n${generatedLines}\n`, + ); + const created = await client.createWorkspace({ + source: { kind: "directory", path: repo.path }, + }); + if (!created.workspace) { + throw new Error(created.error ?? "Failed to create diff performance workspace"); + } + + await page.addInitScript( + ({ preferencesKey }) => { + localStorage.setItem( + preferencesKey, + JSON.stringify({ + layout: "unified", + viewMode: "flat", + wrapLines: false, + hideWhitespace: false, + }), + ); + }, + { preferencesKey: CHANGES_PREFERENCES_KEY }, + ); + await page.setViewportSize({ width: 1400, height: 900 }); + await page.goto(buildHostWorkspaceRoute(getServerId(), created.workspace.id)); + await waitForWorkspaceTabsVisible(page); + await page.getByRole("button", { name: "Open explorer" }).click(); + const fileHeader = page.getByTestId("diff-file-0"); + await expect(fileHeader).toContainText("large.ts", { timeout: 30_000 }); + + cdp = await page.context().newCDPSession(page); + await cdp.send("Emulation.setCPUThrottlingRate", { rate: CPU_SLOWDOWN }); + const expansionStart = await page.evaluate(() => performance.now()); + await fileHeader.click(); + await expect(page.getByTestId("diff-file-0-body")).toBeVisible({ timeout: 30_000 }); + await expect(page.getByTestId("diff-code-row-0")).toBeVisible({ timeout: 30_000 }); + const expansionMs = (await page.evaluate(() => performance.now())) - expansionStart; + + const scroll = page.getByTestId("git-diff-scroll"); + const initial = await readDiffMountReport(page); + expect(initial.mountedCodeRows).toBeLessThanOrEqual(MAX_MOUNTED_CODE_ROWS); + expect(initial.mountedGutterRows).toBe(initial.mountedCodeRows); + + await page.getByTestId("diff-gutter-row-1").hover(); + await page.getByTestId("diff-gutter-action-1").click(); + await expect(page.getByTestId("inline-review-editor")).toBeVisible(); + const withEditor = await readDiffMountReport(page); + expect(withEditor.mountedCodeRows).toBeLessThanOrEqual(MAX_MOUNTED_CODE_ROWS); + expect(withEditor.mountedGutterRows).toBe(withEditor.mountedCodeRows); + expect(await readDiffRowAlignment(page, 2)).toBeCloseTo(0, 0); + await expect(page.getByTestId("inline-review-editor-input")).toBeFocused(); + await page.getByTestId("inline-review-editor-cancel").click(); + await expect(page.getByTestId("inline-review-editor")).toHaveCount(0); + + await scroll.evaluate((element) => { + element.scrollTop = element.scrollHeight - element.clientHeight; + element.dispatchEvent(new Event("scroll", { bubbles: false })); + }); + await expect + .poll(async () => (await readDiffMountReport(page)).highestMountedRow) + .toBeGreaterThan(LINE_COUNT - 100); + const afterScroll = await readDiffMountReport(page); + expect(afterScroll.mountedCodeRows).toBeLessThanOrEqual(MAX_MOUNTED_CODE_ROWS); + expect(afterScroll.mountedGutterRows).toBe(afterScroll.mountedCodeRows); + + const report = { + cpuSlowdown: CPU_SLOWDOWN, + sourceLines: LINE_COUNT, + expansionMs: Math.round(expansionMs), + initial, + withEditor, + afterScroll, + }; + await testInfo.attach("diff-performance", { + body: JSON.stringify(report, null, 2), + contentType: "application/json", + }); + console.log(`[perf] Diff rendering: ${JSON.stringify(report)}`); + expect(expansionMs).toBeLessThan(MAX_EXPANSION_MS); + + await cdp.send("Emulation.setCPUThrottlingRate", { rate: 1 }); + await scroll.evaluate((element) => { + element.scrollTop = 0; + element.dispatchEvent(new Event("scroll", { bubbles: false })); + }); + await page.getByTestId("changes-toggle-layout").click(); + const oldLine = page.getByText("export const baseline = 0;", { exact: true }); + const newLine = page.getByText("export const baseline = 1;", { exact: true }); + await expect(oldLine).toBeVisible(); + await expect(newLine).toBeVisible(); + const [bodyBox, oldBox, newBox] = await Promise.all([ + page.getByTestId("diff-file-0-body").boundingBox(), + oldLine.boundingBox(), + newLine.boundingBox(), + ]); + expect(bodyBox).not.toBeNull(); + expect(oldBox).not.toBeNull(); + expect(newBox).not.toBeNull(); + expect(oldBox!.x).toBeLessThan(newBox!.x); + expect(oldBox!.x).toBeGreaterThanOrEqual(bodyBox!.x); + expect(newBox!.x).toBeGreaterThan(bodyBox!.x + bodyBox!.width / 2); + expect(newBox!.x).toBeLessThan(bodyBox!.x + bodyBox!.width); + } finally { + await cdp?.send("Emulation.setCPUThrottlingRate", { rate: 1 }).catch(() => undefined); + await cdp?.detach().catch(() => undefined); + await client.close().catch(() => undefined); + await repo.cleanup().catch(() => undefined); + } + }); +}); + +async function readDiffRowAlignment( + page: import("@playwright/test").Page, + rowIndex: number, +): Promise { + const gutterTop = await page + .getByTestId(`diff-gutter-row-${rowIndex}`) + .evaluate((row) => row.getBoundingClientRect().top); + const codeTop = await page + .getByTestId(`diff-code-row-${rowIndex}`) + .evaluate((row) => row.getBoundingClientRect().top); + return gutterTop - codeTop; +} + +async function readDiffMountReport(page: import("@playwright/test").Page): Promise<{ + mountedCodeRows: number; + mountedGutterRows: number; + highestMountedRow: number; + bodyDomElements: number; +}> { + return page.getByTestId("diff-file-0-body").evaluate((body) => { + const codeRows = Array.from( + body.querySelectorAll('[data-testid^="diff-code-row-"]'), + ); + const gutterRows = body.querySelectorAll('[data-testid^="diff-gutter-row-"]'); + return { + mountedCodeRows: codeRows.length, + mountedGutterRows: gutterRows.length, + highestMountedRow: Math.max( + -1, + ...codeRows.map((row) => + Number((row.getAttribute("data-testid") ?? "").slice("diff-code-row-".length)), + ), + ), + bodyDomElements: body.querySelectorAll("*").length, + }; + }); +} diff --git a/packages/app/e2e/browser/mermaid-streaming.spec.ts b/packages/app/e2e/browser/mermaid-streaming.spec.ts index a9f87862c..184242689 100644 --- a/packages/app/e2e/browser/mermaid-streaming.spec.ts +++ b/packages/app/e2e/browser/mermaid-streaming.spec.ts @@ -34,6 +34,8 @@ const STREAMED_MERMAID = [ test("keeps a Mermaid diagram rendered while its message streams, completes, and reloads", async ({ page, }) => { + test.setTimeout(90_000); + const agent = await seedMockAgentWorkspace({ repoPrefix: "mermaid-streaming-", title: "Streaming Mermaid", diff --git a/packages/app/e2e/support/helpers/diagram.ts b/packages/app/e2e/support/helpers/diagram.ts index f3d2742c1..6cf501c0f 100644 --- a/packages/app/e2e/support/helpers/diagram.ts +++ b/packages/app/e2e/support/helpers/diagram.ts @@ -30,7 +30,7 @@ export async function expectDiagramRemainsRenderedWhileStreaming(page: Page): Pr const samples = 80; for (let sample = 0; sample < samples; sample += 1) { await expect(diagram).toBeVisible({ timeout: 100 }); - await expect(svg).toBeVisible({ timeout: 100 }); + await expect(svg).toBeVisible({ timeout: 500 }); await page.waitForTimeout(25); } } @@ -44,5 +44,5 @@ export async function expectCompletedDiagram(page: Page, labels: readonly string } export async function reloadConversation(page: Page): Promise { - await page.reload(); + await page.reload({ waitUntil: "domcontentloaded" }); } diff --git a/packages/app/src/git/diff-pane.tsx b/packages/app/src/git/diff-pane.tsx index 5c70f61da..02c07a5f6 100644 --- a/packages/app/src/git/diff-pane.tsx +++ b/packages/app/src/git/diff-pane.tsx @@ -4,6 +4,7 @@ import { useEffect, useMemo, useRef, + useSyncExternalStore, memo, type ReactElement, type ReactNode, @@ -43,6 +44,12 @@ import { } from "lucide-react-native"; import { type ParsedDiffFile, type DiffLine, type HighlightToken } from "@/git/use-diff-query"; import { buildDiffFlatItems, sumHeightsBefore, type DiffFlatItem } from "@/git/diff-flat-items"; +import { + createDiffRowWindow, + createDiffViewport, + type DiffRowWindow, + type DiffViewport, +} from "@/git/diff-virtualization"; import { buildDiffTree, collectDirPaths, compressSingleChildChains } from "@/git/diff-tree"; import { DiffFolderRow } from "@/git/diff-folder-row"; import { @@ -104,6 +111,7 @@ import { collectAllTabs, useWorkspaceLayoutStore } from "@/stores/workspace-layo import { buildWorkspaceTabPersistenceKey } from "@/workspace-tabs/model"; import { buildWorkspaceExplorerStateKey } from "@/hooks/use-file-explorer-actions"; import { + compactHighlightTokens, formatDiffContentText, formatDiffGutterText, hasVisibleDiffTokens, @@ -180,7 +188,7 @@ function HighlightedToken({ token }: { token: HighlightToken }) { return {token.text}; } -function HighlightedText({ +const HighlightedText = memo(function HighlightedText({ tokens, textMetricsStyle, wrapLines = false, @@ -196,10 +204,14 @@ function HighlightedText({ [textMetricsStyle, wrapLines], ); - const keyedTokens = useMemo( - () => tokens.map((token, index) => ({ key: `${index}-${token.text}`, token })), - [tokens], - ); + const keyedTokens = useMemo(() => { + let characterOffset = 0; + return compactHighlightTokens(tokens).map((token) => { + const keyedToken = { key: `${characterOffset}-${token.style ?? "base"}`, token }; + characterOffset += token.text.length; + return keyedToken; + }); + }, [tokens]); return ( @@ -208,7 +220,7 @@ function HighlightedText({ ))} ); -} +}); interface DiffFileSectionProps { file: ParsedDiffFile; @@ -804,8 +816,53 @@ function InlineReviewRow({ ); } +const DIFF_ROW_OVERSCAN = 900; +const FULL_DIFF_VIEWPORT = { top: 0, height: Number.MAX_SAFE_INTEGER }; +const subscribeToNothing = () => () => {}; +const getFullDiffViewport = () => FULL_DIFF_VIEWPORT; + +function useDiffRowWindow({ + viewport, + bodyOffset, + rowHeights, + enabled, +}: { + viewport?: DiffViewport; + bodyOffset: number; + rowHeights: readonly number[]; + enabled: boolean; +}): DiffRowWindow { + const viewportSnapshot = useSyncExternalStore( + viewport?.subscribe ?? subscribeToNothing, + viewport?.getSnapshot ?? getFullDiffViewport, + viewport?.getSnapshot ?? getFullDiffViewport, + ); + return useMemo(() => { + if (!enabled) { + return createDiffRowWindow(rowHeights, { + viewportTop: 0, + viewportHeight: Number.MAX_SAFE_INTEGER, + overscan: 0, + }); + } + return createDiffRowWindow(rowHeights, { + viewportTop: viewportSnapshot.top - bodyOffset - BORDER_WIDTH[1], + viewportHeight: viewportSnapshot.height, + overscan: DIFF_ROW_OVERSCAN, + }); + }, [bodyOffset, enabled, rowHeights, viewportSnapshot.height, viewportSnapshot.top]); +} + +function DiffRowSpacer({ height }: { height: number }) { + if (height <= 0) { + return null; + } + return ; +} + function SplitDiffColumn({ rows, + rowWindow, side, gutterWidth, wrapLines, @@ -814,6 +871,7 @@ function SplitDiffColumn({ showDivider = false, }: { rows: SplitDiffRow[]; + rowWindow: DiffRowWindow; side: "left" | "right"; gutterWidth: number; wrapLines: boolean; @@ -844,12 +902,20 @@ function SplitDiffColumn({ [textMetricsStyle], ); - const keyedRows = useMemo(() => rows.map((row, i) => ({ key: `row-${i}`, row })), [rows]); + const keyedRows = useMemo( + () => + rows.slice(rowWindow.startIndex, rowWindow.endIndex).map((row, index) => ({ + key: `row-${rowWindow.startIndex + index}`, + row, + })), + [rowWindow.endIndex, rowWindow.startIndex, rows], + ); if (wrapLines) { return ( + {keyedRows.map(({ key, row }) => { if (row.kind === "header") { return ( @@ -882,6 +948,7 @@ function SplitDiffColumn({ ); })} + ); @@ -890,6 +957,7 @@ function SplitDiffColumn({ return ( + {keyedRows.map(({ key, row }) => { if (row.kind === "header") { return ( @@ -931,6 +999,7 @@ function SplitDiffColumn({ ); })} + + {keyedRows.map(({ key, row }) => { if (row.kind === "header") { return ( @@ -974,6 +1044,7 @@ function SplitDiffColumn({ ); })} + @@ -1255,6 +1326,8 @@ const DiffFileHeader = memo(function DiffFileHeader({ export function DiffFileBody({ file, + viewport, + bodyOffset = 0, layout, wrapLines, codeFontSize, @@ -1264,6 +1337,8 @@ export function DiffFileBody({ testID, }: { file: ParsedDiffFile; + viewport?: DiffViewport; + bodyOffset?: number; layout: "unified" | "split"; wrapLines: boolean; codeFontSize: number; @@ -1276,6 +1351,50 @@ export function DiffFileBody({ const [bodyWidth, setBodyWidth] = useState(0); const [hoveredReviewTargetKey, setHoveredReviewTargetKey] = useState(null); const { t } = useTranslation(); + const canRenderRows = file.status !== "too_large" && file.status !== "binary"; + const splitRows = useMemo( + () => (canRenderRows && layout === "split" ? getCachedSplitDiffRows(file) : []), + [canRenderRows, file, layout], + ); + const unifiedLines = useMemo( + () => (canRenderRows && layout === "unified" ? getCachedUnifiedDiffLines(file) : []), + [canRenderRows, file, layout], + ); + const rowHeights = useMemo(() => { + const lineHeight = getNumericLineHeight(textMetricsStyle) ?? Math.round(codeFontSize * 1.5); + if (layout === "split") { + return splitRows.map((row) => { + if (row.kind === "header") { + return lineHeight; + } + return ( + lineHeight + + (getSplitInlineReviewThreadState({ + left: row.left?.reviewTarget, + right: row.right?.reviewTarget, + reviewActions, + })?.height ?? 0) + ); + }); + } + return unifiedLines.map( + ({ reviewTarget }) => + lineHeight + (getInlineReviewThreadState({ reviewTarget, reviewActions })?.height ?? 0), + ); + }, [codeFontSize, layout, reviewActions, splitRows, textMetricsStyle, unifiedLines]); + const rowWindow = useDiffRowWindow({ + viewport, + bodyOffset, + rowHeights, + enabled: Boolean(viewport) && canRenderRows && !wrapLines, + }); + const visibleUnifiedLines = useMemo( + () => + unifiedLines + .slice(rowWindow.startIndex, rowWindow.endIndex) + .map((entry, index) => ({ entry, index: rowWindow.startIndex + index })), + [rowWindow.endIndex, rowWindow.startIndex, unifiedLines], + ); const handleLayout = useCallback( (event: LayoutChangeEvent) => { @@ -1324,11 +1443,11 @@ export function DiffFileBody({ const gutterWidth = lineNumberGutterWidth(maxLineNo, codeFontSize); if (layout === "split") { - const rows = buildSplitDiffRows(file); return ( - {computedLines.map(({ line, lineNumber, key, reviewTarget }, index) => ( - - - - - ))} + + {visibleUnifiedLines.map( + ({ entry: { line, lineNumber, key, reviewTarget }, index }) => ( + + + + + ), + )} + ); @@ -1382,28 +1504,33 @@ export function DiffFileBody({ return ( - {computedLines.map(({ line, lineNumber, key, reviewTarget }, index) => ( - - - - - ))} + + {visibleUnifiedLines.map( + ({ entry: { line, lineNumber, key, reviewTarget }, index }) => ( + + + + + ), + )} + - {computedLines.map(({ line, key, reviewTarget }, index) => ( + + {visibleUnifiedLines.map(({ entry: { line, key, reviewTarget }, index }) => ( ))} + @@ -1828,7 +1957,9 @@ const EMPTY_PATH_LIST: string[] = []; interface DiffFileMetrics { contentLength: number; + splitRows?: SplitDiffRow[]; splitLineCount?: number; + unifiedLines?: ReturnType; unifiedLineCount: number; } @@ -1855,11 +1986,23 @@ function getDiffFileMetrics(file: ParsedDiffFile): DiffFileMetrics { function getSplitDiffLineCount(file: ParsedDiffFile): number { const metrics = getDiffFileMetrics(file); if (metrics.splitLineCount === undefined) { - metrics.splitLineCount = buildSplitDiffRows(file).length; + metrics.splitLineCount = getCachedSplitDiffRows(file).length; } return metrics.splitLineCount; } +function getCachedSplitDiffRows(file: ParsedDiffFile): SplitDiffRow[] { + const metrics = getDiffFileMetrics(file); + metrics.splitRows ??= buildSplitDiffRows(file); + return metrics.splitRows; +} + +function getCachedUnifiedDiffLines(file: ParsedDiffFile): ReturnType { + const metrics = getDiffFileMetrics(file); + metrics.unifiedLines ??= buildUnifiedDiffLines(file); + return metrics.unifiedLines; +} + function computeEmptyMessage( hideWhitespace: boolean, diffMode: "uncommitted" | "base", @@ -2062,6 +2205,8 @@ export function SharedDiffView({ files, displayPreferences, mode }: SharedDiffVi [allFolderPathSet, collapsedFolders], ); const diffListRef = useRef>(null); + const diffViewport = useMemo(() => createDiffViewport(), []); + useEffect(() => () => diffViewport.dispose(), [diffViewport]); const scrollbar = useOverlayFlatListScrollbar(diffListRef, { enabled: !isCompact }); const { onLayout: updateScrollbarLayout, onScroll: updateScrollbarOffset } = scrollbar; const consumedFocusRequestRef = useRef(null); @@ -2216,9 +2361,13 @@ export function SharedDiffView({ files, displayPreferences, mode }: SharedDiffVi const handleDiffListScroll = useCallback( (event: NativeSyntheticEvent) => { diffListScrollOffsetRef.current = event.nativeEvent.contentOffset.y; + diffViewport.update({ + top: diffListScrollOffsetRef.current, + height: diffListViewportHeightRef.current, + }); updateScrollbarOffset(event); }, - [updateScrollbarOffset], + [diffViewport, updateScrollbarOffset], ); const handleDiffListLayout = useCallback( @@ -2228,9 +2377,10 @@ export function SharedDiffView({ files, displayPreferences, mode }: SharedDiffVi return; } diffListViewportHeightRef.current = height; + diffViewport.update({ top: diffListScrollOffsetRef.current, height }); updateScrollbarLayout(event); }, - [updateScrollbarLayout], + [diffViewport, updateScrollbarLayout], ); const computeItemOffset = useCallback( @@ -2387,7 +2537,7 @@ export function SharedDiffView({ files, displayPreferences, mode }: SharedDiffVi ); const renderFlatItem = useCallback( - ({ item }: { item: DiffFlatItem }) => { + ({ item, index }: { item: DiffFlatItem; index: number }) => { if (item.type === "folder") { return ( { + it("renders only the viewport and overscan while preserving the full height", () => { + const rows = Array.from({ length: 1_200 }, () => 18); + const window = createDiffRowWindow(rows, { + viewportTop: 0, + viewportHeight: 900, + overscan: 900, + }); + + expect(window.startIndex).toBe(0); + expect(window.endIndex).toBe(100); + expect(window.beforeHeight).toBe(0); + expect(window.afterHeight).toBe(19_800); + expect(window.beforeHeight + window.renderedHeight + window.afterHeight).toBe(21_600); + }); + + it("finds a deep variable-height window without replaying earlier rows", () => { + const rows = [18, 18, 90, 18, 150, 18, 18]; + const window = createDiffRowWindow(rows, { + viewportTop: 120, + viewportHeight: 60, + overscan: 0, + }); + + expect(window).toEqual({ + startIndex: 2, + endIndex: 5, + beforeHeight: 36, + renderedHeight: 258, + afterHeight: 36, + totalHeight: 330, + }); + }); + + it("returns an empty row window when an overscanned body is still offscreen", () => { + const window = createDiffRowWindow([18, 18, 18], { + viewportTop: -2_000, + viewportHeight: 800, + overscan: 600, + }); + + expect(window).toEqual({ + startIndex: 0, + endIndex: 0, + beforeHeight: 0, + renderedHeight: 0, + afterHeight: 54, + totalHeight: 54, + }); + }); +}); diff --git a/packages/app/src/git/diff-virtualization.ts b/packages/app/src/git/diff-virtualization.ts new file mode 100644 index 000000000..aa474c51f --- /dev/null +++ b/packages/app/src/git/diff-virtualization.ts @@ -0,0 +1,143 @@ +export interface DiffViewportWindow { + viewportTop: number; + viewportHeight: number; + overscan: number; +} + +export interface DiffRowWindow { + startIndex: number; + endIndex: number; + beforeHeight: number; + renderedHeight: number; + afterHeight: number; + totalHeight: number; +} + +export interface DiffViewportSnapshot { + top: number; + height: number; +} + +export interface DiffViewport { + getSnapshot: () => DiffViewportSnapshot; + subscribe: (listener: () => void) => () => void; + update: (snapshot: DiffViewportSnapshot) => void; + dispose: () => void; +} + +/** Coalesce native/web scroll events so diff bodies select rows at most once per frame. */ +export function createDiffViewport(): DiffViewport { + let snapshot: DiffViewportSnapshot = { top: 0, height: 0 }; + let pending = snapshot; + let frame: number | null = null; + const listeners = new Set<() => void>(); + + const flush = () => { + frame = null; + if (pending.top === snapshot.top && pending.height === snapshot.height) { + return; + } + snapshot = pending; + for (const listener of listeners) { + listener(); + } + }; + + return { + getSnapshot: () => snapshot, + subscribe(listener) { + listeners.add(listener); + return () => listeners.delete(listener); + }, + update(nextSnapshot) { + pending = nextSnapshot; + frame ??= requestAnimationFrame(flush); + }, + dispose() { + if (frame !== null) { + cancelAnimationFrame(frame); + } + frame = null; + listeners.clear(); + }, + }; +} + +/** + * Select the contiguous diff rows that intersect a viewport while reserving + * exact spacer heights for everything outside it. + */ +export function createDiffRowWindow( + rowHeights: readonly number[], + viewport: DiffViewportWindow, +): DiffRowWindow { + const offsets = Array.from({ length: rowHeights.length + 1 }, () => 0); + offsets[0] = 0; + for (let index = 0; index < rowHeights.length; index += 1) { + offsets[index + 1] = offsets[index] + rowHeights[index]; + } + + const totalHeight = offsets[rowHeights.length]; + const visibleTop = viewport.viewportTop - viewport.overscan; + const visibleBottom = viewport.viewportTop + viewport.viewportHeight + viewport.overscan; + + if (visibleBottom <= 0) { + return emptyWindow(0, 0, totalHeight); + } + if (visibleTop >= totalHeight) { + return emptyWindow(rowHeights.length, totalHeight, 0); + } + + const startIndex = firstIndexWhoseEndExceeds(offsets, Math.max(0, visibleTop)); + const endIndex = firstIndexWhoseStartReaches(offsets, Math.min(totalHeight, visibleBottom)); + const beforeHeight = offsets[startIndex]; + const renderedHeight = offsets[endIndex] - beforeHeight; + + return { + startIndex, + endIndex, + beforeHeight, + renderedHeight, + afterHeight: totalHeight - offsets[endIndex], + totalHeight, + }; +} + +function emptyWindow(index: number, beforeHeight: number, afterHeight: number): DiffRowWindow { + return { + startIndex: index, + endIndex: index, + beforeHeight, + renderedHeight: 0, + afterHeight, + totalHeight: beforeHeight + afterHeight, + }; +} + +function firstIndexWhoseEndExceeds(offsets: readonly number[], target: number): number { + let low = 1; + let high = offsets.length; + while (low < high) { + const middle = Math.floor((low + high) / 2); + if ((offsets[middle] ?? Number.POSITIVE_INFINITY) > target) { + high = middle; + } else { + low = middle + 1; + } + } + return Math.min(offsets.length - 1, low - 1); +} + +function firstIndexWhoseStartReaches(offsets: readonly number[], target: number): number { + let low = 0; + let high = offsets.length - 1; + while (low < high) { + const middle = Math.floor((low + high) / 2); + if (offsets[middle] >= target) { + high = middle; + } else { + low = middle + 1; + } + } + return low; +} diff --git a/packages/app/src/review/surface.test.tsx b/packages/app/src/review/surface.test.tsx index 86a8fb7eb..8ae0c4d97 100644 --- a/packages/app/src/review/surface.test.tsx +++ b/packages/app/src/review/surface.test.tsx @@ -270,7 +270,16 @@ describe("git diff inline review helpers", () => { expect(rowState?.left).toBeNull(); expect(rowState?.right?.comments).toEqual([rightComment]); - expect(rowState?.height).toBe(210); + expect(rowState?.height).toBe(226); + }); + + it("includes thread padding in the inline editor height", () => { + const reviewTarget = target(); + const actions = buildReviewActions({ + editor: { target: reviewTarget, commentId: null, body: "" }, + }); + + expect(getInlineReviewThreadState({ reviewTarget, reviewActions: actions })?.height).toBe(148); }); it("pins no-wrap review threads to the visible diff viewport", () => { @@ -412,13 +421,13 @@ describe("InlineReviewEditor", () => { expect(onSave).toHaveBeenCalledWith("ready"); }); - it("shows shared shortcut hints while focused on a fine-pointer screen", () => { + it("does not show shortcut hints in the action buttons", () => { window.matchMedia = vi.fn().mockReturnValue({ matches: true, addEventListener: vi.fn(), removeEventListener: vi.fn(), }); - const { getByTestId, getByText, queryByText } = render( + const { getByTestId, queryByText } = render( { ); const input = getByTestId("editor-input"); - expect(getByText("Esc")).toBeTruthy(); - expect(getByText(/(?:⌘⏎|Ctrl\+⏎)/)).toBeTruthy(); - - fireEvent.blur(input); + fireEvent.focus(input); expect(queryByText("Esc")).toBeNull(); + expect(queryByText(/(?:⌘⏎|Ctrl\+⏎)/)).toBeNull(); }); }); diff --git a/packages/app/src/review/surface.tsx b/packages/app/src/review/surface.tsx index fd79290b5..9e9c6ca6f 100644 --- a/packages/app/src/review/surface.tsx +++ b/packages/app/src/review/surface.tsx @@ -14,12 +14,9 @@ import { } from "react-native"; import { StyleSheet, withUnistyles } from "react-native-unistyles"; import { Button } from "@/components/ui/button"; -import { Shortcut } from "@/components/ui/shortcut"; import { isWeb } from "@/constants/platform"; -import { useHasFinePointer } from "@/hooks/use-fine-pointer"; import { inlineUnistylesStyle } from "@/styles/unistyles-inline-style"; import type { Theme } from "@/styles/theme"; -import type { ShortcutKey } from "@/utils/format-shortcut"; import { useWorkspaceFocusRestoration } from "@/workspace/focus"; import { useReviewDraftComments, useReviewDraftStore, type ReviewDraftComment } from "./store"; import { buildReviewableDiffTargetKey, type ReviewableDiffTarget } from "@/utils/diff-layout"; @@ -50,9 +47,8 @@ function getWebTextInputElement(input: TextInput | null): HTMLElement | null { export const INLINE_REVIEW_COMMENT_HEIGHT = 72; export const INLINE_REVIEW_EDITOR_HEIGHT = 132; const INLINE_REVIEW_GAP = 6; +const INLINE_REVIEW_VERTICAL_PADDING = 8; export const SMALL_ACTION_HIT_SLOP = 8; -const REVIEW_CANCEL_SHORTCUT_KEYS: ShortcutKey[] = ["Esc"]; -const REVIEW_SAVE_SHORTCUT_KEYS: ShortcutKey[] = ["mod", "Enter"]; const foregroundMutedIconColorMapping = (theme: Theme) => ({ color: theme.colors.foregroundMuted }); const destructiveIconColorMapping = (theme: Theme) => ({ color: theme.colors.destructive }); const accentForegroundIconColorMapping = (theme: Theme) => ({ @@ -222,7 +218,8 @@ export function getInlineReviewThreadState(input: { const height = visibleCommentCount * INLINE_REVIEW_COMMENT_HEIGHT + editorCount * INLINE_REVIEW_EDITOR_HEIGHT + - Math.max(0, visibleBlockCount - 1) * INLINE_REVIEW_GAP; + Math.max(0, visibleBlockCount - 1) * INLINE_REVIEW_GAP + + INLINE_REVIEW_VERTICAL_PADDING * 2; return { comments, hasEditor, editingCommentId, height }; } @@ -507,12 +504,10 @@ export function InlineReviewEditor({ const { t } = useTranslation(); const inputRef = useRef(null); const focus = useWorkspaceFocusRestoration(); - const canShowKeyboardHints = useHasFinePointer(); const [body, setBody] = useState(initialBody); const [isFocused, setIsFocused] = useState(false); const trimmedBody = body.trim(); const canSave = trimmedBody.length > 0; - const showKeyboardHints = isFocused && canShowKeyboardHints; useEffect(() => { inputRef.current?.focus(); @@ -527,14 +522,6 @@ export function InlineReviewEditor({ focus.restore(); }, [focus]); const handleSave = useCallback(() => onSave(trimmedBody), [onSave, trimmedBody]); - const cancelShortcut = useMemo( - () => (showKeyboardHints ? : null), - [showKeyboardHints], - ); - const saveShortcut = useMemo( - () => (showKeyboardHints ? : null), - [showKeyboardHints], - ); useEffect(() => { const element = getWebTextInputElement(inputRef.current); @@ -598,7 +585,6 @@ export function InlineReviewEditor({ onPress={onCancel} variant="ghost" size="xs" - trailing={cancelShortcut} > {t("review.comment.cancel")} @@ -610,7 +596,6 @@ export function InlineReviewEditor({ onPress={handleSave} variant="default" size="xs" - trailing={saveShortcut} > {t("review.comment.save")} @@ -658,7 +643,7 @@ const styles = StyleSheet.create((theme) => ({ flex: 1, minWidth: 0, gap: INLINE_REVIEW_GAP, - paddingVertical: theme.spacing[2], + paddingVertical: INLINE_REVIEW_VERTICAL_PADDING, paddingHorizontal: theme.spacing[3], }, commentBlock: { diff --git a/packages/app/src/utils/diff-highlighter.test.ts b/packages/app/src/utils/diff-highlighter.test.ts deleted file mode 100644 index 76a3a63ba..000000000 --- a/packages/app/src/utils/diff-highlighter.test.ts +++ /dev/null @@ -1,520 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { - parseDiff, - reconstructNewFile, - reconstructOldFile, - highlightDiffFile, - parseAndHighlightDiff, - type ParsedDiffFile, -} from "./diff-highlighter"; - -const SIMPLE_DIFF = `diff --git a/example.ts b/example.ts -index 1234567..abcdefg 100644 ---- a/example.ts -+++ b/example.ts -@@ -1,5 +1,5 @@ - const foo = 1; --const bar = 2; -+const bar = 3; - const baz = foo + bar; - - export { foo, bar, baz }; -`; - -const MULTI_HUNK_DIFF = `diff --git a/example.ts b/example.ts -index 1234567..abcdefg 100644 ---- a/example.ts -+++ b/example.ts -@@ -1,3 +1,3 @@ - const foo = 1; --const bar = 2; -+const bar = 3; - const baz = foo + bar; -@@ -10,3 +10,4 @@ - function greet(name: string) { - return "Hello, " + name; - } -+export { greet }; -`; - -const NEW_FILE_DIFF = `diff --git a/newfile.ts b/newfile.ts -new file mode 100644 -index 0000000..1234567 ---- /dev/null -+++ b/newfile.ts -@@ -0,0 +1,3 @@ -+const x = 1; -+const y = 2; -+export { x, y }; -`; - -const DELETED_FILE_DIFF = `diff --git a/oldfile.ts b/oldfile.ts -deleted file mode 100644 -index 1234567..0000000 ---- a/oldfile.ts -+++ /dev/null -@@ -1,2 +0,0 @@ --const legacy = true; --export { legacy }; -`; - -const RUST_DIFF = `diff --git a/lib.rs b/lib.rs -index 1234567..abcdefg 100644 ---- a/lib.rs -+++ b/lib.rs -@@ -1,4 +1,4 @@ - fn main() { -- let version = 1; -+ let version = 2; - println!("Hello, world!"); - } -`; - -const C_DIFF = `diff --git a/example.c b/example.c -index 1234567..abcdefg 100644 ---- a/example.c -+++ b/example.c -@@ -1,4 +1,4 @@ - int main(void) { -- int version = 1; -+ int version = 2; - return version; - } -`; - -const JAVA_DIFF = `diff --git a/Main.java b/Main.java -index 1234567..abcdefg 100644 ---- a/Main.java -+++ b/Main.java -@@ -1,5 +1,5 @@ - public class Main { - public static void main(String[] args) { -- int version = 1; -+ int version = 2; - } - } -`; - -const OBJECTIVE_C_DIFF = `diff --git a/AppDelegate.m b/AppDelegate.m -index 1234567..abcdefg 100644 ---- a/AppDelegate.m -+++ b/AppDelegate.m -@@ -1,4 +1,4 @@ - int main(void) { -- int version = 1; -+ int version = 2; - return version; - } -`; - -const GO_DIFF = `diff --git a/main.go b/main.go -index 1234567..abcdefg 100644 ---- a/main.go -+++ b/main.go -@@ -1,7 +1,7 @@ - package main - - import "fmt" - - func main() { -- version := 1 -+ version := 2 - fmt.Println(version) - } -`; - -const PHP_DIFF = `diff --git a/index.php b/index.php -index 1234567..abcdefg 100644 ---- a/index.php -+++ b/index.php -@@ -1,4 +1,4 @@ - -`; - -const YAML_DIFF = `diff --git a/config.yaml b/config.yaml -index 1234567..abcdefg 100644 ---- a/config.yaml -+++ b/config.yaml -@@ -1,3 +1,3 @@ - app: paseo --count: 1 -+count: 2 - enabled: true -`; - -const XML_DIFF = `diff --git a/config.xml b/config.xml -index 1234567..abcdefg 100644 ---- a/config.xml -+++ b/config.xml -@@ -1,3 +1,3 @@ - -- 1 -+ 2 - -`; - -describe("parseDiff", () => { - it("parses a simple diff with one hunk", () => { - const files = parseDiff(SIMPLE_DIFF); - - expect(files).toHaveLength(1); - expect(files[0].path).toBe("example.ts"); - expect(files[0].isNew).toBe(false); - expect(files[0].isDeleted).toBe(false); - expect(files[0].additions).toBe(1); - expect(files[0].deletions).toBe(1); - expect(files[0].hunks).toHaveLength(1); - - const hunk = files[0].hunks[0]; - expect(hunk.oldStart).toBe(1); - expect(hunk.oldCount).toBe(5); - expect(hunk.newStart).toBe(1); - expect(hunk.newCount).toBe(5); - - // Header + 5 content lines (1 context, 1 remove, 1 add, 1 context, 1 blank context, 1 context) - expect(hunk.lines[0].type).toBe("header"); - expect(hunk.lines[1].type).toBe("context"); - expect(hunk.lines[1].content).toBe("const foo = 1;"); - expect(hunk.lines[2].type).toBe("remove"); - expect(hunk.lines[2].content).toBe("const bar = 2;"); - expect(hunk.lines[3].type).toBe("add"); - expect(hunk.lines[3].content).toBe("const bar = 3;"); - }); - - it("parses no-prefix git diff headers", () => { - const files = parseDiff( - SIMPLE_DIFF.replace("a/example.ts b/example.ts", "example.ts example.ts") - .replace("--- a/example.ts", "--- example.ts") - .replace("+++ b/example.ts", "+++ example.ts"), - ); - - expect(files).toHaveLength(1); - expect(files[0].path).toBe("example.ts"); - expect(files[0].additions).toBe(1); - expect(files[0].deletions).toBe(1); - expect(files[0].hunks).toHaveLength(1); - }); - - it("parses paths with spaces", () => { - const files = parseDiff(SIMPLE_DIFF.replaceAll("example.ts", "file with space.ts")); - - expect(files).toHaveLength(1); - expect(files[0].path).toBe("file with space.ts"); - expect(files[0].hunks).toHaveLength(1); - }); - - it("parses no-prefix paths with spaces", () => { - const files = parseDiff( - SIMPLE_DIFF.replaceAll("example.ts", "file with space.ts") - .replace( - "a/file with space.ts b/file with space.ts", - "file with space.ts file with space.ts", - ) - .replace("--- a/file with space.ts", "--- file with space.ts") - .replace("+++ b/file with space.ts", "+++ file with space.ts"), - ); - - expect(files).toHaveLength(1); - expect(files[0].path).toBe("file with space.ts"); - expect(files[0].hunks).toHaveLength(1); - }); - - it("preserves no-prefix paths that start with a or b", () => { - const files = parseDiff( - `${SIMPLE_DIFF.replace( - "diff --git a/example.ts b/example.ts", - "diff --git a/example.ts a/example.ts", - ).replace("--- a/example.ts\n+++ b/example.ts", "--- a/example.ts\n+++ a/example.ts")} -${SIMPLE_DIFF.replaceAll("example.ts", "other.ts") - .replace("diff --git a/other.ts b/other.ts", "diff --git b/other.ts b/other.ts") - .replace("--- a/other.ts\n+++ b/other.ts", "--- b/other.ts\n+++ b/other.ts")}`, - ); - - expect(files.map((file) => file.path)).toEqual(["a/example.ts", "b/other.ts"]); - }); - - it("parses a diff with multiple hunks", () => { - const files = parseDiff(MULTI_HUNK_DIFF); - - expect(files).toHaveLength(1); - expect(files[0].hunks).toHaveLength(2); - - expect(files[0].hunks[0].oldStart).toBe(1); - expect(files[0].hunks[0].newStart).toBe(1); - - expect(files[0].hunks[1].oldStart).toBe(10); - expect(files[0].hunks[1].newStart).toBe(10); - }); - - it("parses a new file diff", () => { - const files = parseDiff(NEW_FILE_DIFF); - - expect(files).toHaveLength(1); - expect(files[0].path).toBe("newfile.ts"); - expect(files[0].isNew).toBe(true); - expect(files[0].isDeleted).toBe(false); - expect(files[0].additions).toBe(3); - expect(files[0].deletions).toBe(0); - }); - - it("parses a deleted file diff", () => { - const files = parseDiff(DELETED_FILE_DIFF); - - expect(files).toHaveLength(1); - expect(files[0].path).toBe("oldfile.ts"); - expect(files[0].isNew).toBe(false); - expect(files[0].isDeleted).toBe(true); - expect(files[0].additions).toBe(0); - expect(files[0].deletions).toBe(2); - }); - - it("returns empty array for empty input", () => { - expect(parseDiff("")).toEqual([]); - expect(parseDiff(" ")).toEqual([]); - }); -}); - -describe("reconstructNewFile", () => { - it("reconstructs the new file version from hunks", () => { - const files = parseDiff(SIMPLE_DIFF); - const newFile = reconstructNewFile(files[0].hunks); - - expect(newFile.get(1)).toBe("const foo = 1;"); - expect(newFile.get(2)).toBe("const bar = 3;"); // Changed line - expect(newFile.get(3)).toBe("const baz = foo + bar;"); - // Note: blank lines in diffs should have a space prefix to be parsed - // as context lines. In this test the blank line has no prefix so it's - // not included. That's OK - real git diffs have the space. - expect(newFile.get(4)).toBe("export { foo, bar, baz };"); - - // Old value should not be present - expect(Array.from(newFile.values())).not.toContain("const bar = 2;"); - }); - - it("handles new file (all additions)", () => { - const files = parseDiff(NEW_FILE_DIFF); - const newFile = reconstructNewFile(files[0].hunks); - - expect(newFile.get(1)).toBe("const x = 1;"); - expect(newFile.get(2)).toBe("const y = 2;"); - expect(newFile.get(3)).toBe("export { x, y };"); - }); -}); - -describe("reconstructOldFile", () => { - it("reconstructs the old file version from hunks", () => { - const files = parseDiff(SIMPLE_DIFF); - const oldFile = reconstructOldFile(files[0].hunks); - - expect(oldFile.get(1)).toBe("const foo = 1;"); - expect(oldFile.get(2)).toBe("const bar = 2;"); // Original line - expect(oldFile.get(3)).toBe("const baz = foo + bar;"); - - // New value should not be present - expect(Array.from(oldFile.values())).not.toContain("const bar = 3;"); - }); - - it("handles deleted file (all removals)", () => { - const files = parseDiff(DELETED_FILE_DIFF); - const oldFile = reconstructOldFile(files[0].hunks); - - expect(oldFile.get(1)).toBe("const legacy = true;"); - expect(oldFile.get(2)).toBe("export { legacy };"); - }); -}); - -describe("highlightDiffFile", () => { - it("adds syntax highlighting tokens to TypeScript code", () => { - const files = parseDiff(SIMPLE_DIFF); - const highlighted = highlightDiffFile(files[0]); - - // Check that tokens are added - const hunk = highlighted.hunks[0]; - - // First content line: "const foo = 1;" - const constLine = hunk.lines[1]; - expect(constLine.tokens).toContainEqual( - expect.objectContaining({ text: "const", style: "keyword" }), - ); - expect(constLine.tokens).toContainEqual( - expect.objectContaining({ text: "1", style: "number" }), - ); - }); - - it("highlights both added and removed lines correctly", () => { - const files = parseDiff(SIMPLE_DIFF); - const highlighted = highlightDiffFile(files[0]); - const hunk = highlighted.hunks[0]; - - // Removed line: "const bar = 2;" - const removedLine = hunk.lines.find((l) => l.type === "remove" && l.content.includes("bar")); - expect(removedLine?.tokens).toContainEqual( - expect.objectContaining({ text: "2", style: "number" }), - ); - - // Added line: "const bar = 3;" - const addedLine = hunk.lines.find((l) => l.type === "add" && l.content.includes("bar")); - expect(addedLine?.tokens).toContainEqual( - expect.objectContaining({ text: "3", style: "number" }), - ); - }); - - it("does not modify unsupported file types", () => { - const file: ParsedDiffFile = { - path: "README.txt", - isNew: false, - isDeleted: false, - additions: 1, - deletions: 0, - hunks: [ - { - oldStart: 1, - oldCount: 1, - newStart: 1, - newCount: 2, - lines: [ - { type: "header", content: "@@ -1,1 +1,2 @@" }, - { type: "context", content: "Hello" }, - { type: "add", content: "World" }, - ], - }, - ], - }; - - const highlighted = highlightDiffFile(file); - - // Lines should not have tokens - expect(highlighted.hunks[0].lines[1].tokens).toBeUndefined(); - expect(highlighted.hunks[0].lines[2].tokens).toBeUndefined(); - }); - - it("adds syntax highlighting tokens to Rust code", () => { - const files = parseDiff(RUST_DIFF); - const highlighted = highlightDiffFile(files[0]); - const hunk = highlighted.hunks[0]; - - expect(hunk.lines[1].tokens).toContainEqual( - expect.objectContaining({ text: "fn", style: "keyword" }), - ); - - const addedLine = hunk.lines.find( - (line) => line.type === "add" && line.content.includes("version"), - ); - expect(addedLine?.tokens).toContainEqual( - expect.objectContaining({ text: "2", style: "number" }), - ); - }); - - it("adds syntax highlighting tokens to C code", () => { - const files = parseDiff(C_DIFF); - const highlighted = highlightDiffFile(files[0]); - const hunk = highlighted.hunks[0]; - - expect(hunk.lines[1].tokens?.length).toBeGreaterThan(0); - - const addedLine = hunk.lines.find( - (line) => line.type === "add" && line.content.includes("version"), - ); - expect(addedLine?.tokens).toContainEqual( - expect.objectContaining({ text: "2", style: "number" }), - ); - }); - - it("adds syntax highlighting tokens to Java code", () => { - const files = parseDiff(JAVA_DIFF); - const highlighted = highlightDiffFile(files[0]); - const hunk = highlighted.hunks[0]; - - expect(hunk.lines[1].tokens).toContainEqual( - expect.objectContaining({ text: "public", style: "keyword" }), - ); - - const addedLine = hunk.lines.find( - (line) => line.type === "add" && line.content.includes("version"), - ); - expect(addedLine?.tokens).toContainEqual( - expect.objectContaining({ text: "2", style: "number" }), - ); - }); - - it("adds syntax highlighting tokens to Objective-C file extensions", () => { - const files = parseDiff(OBJECTIVE_C_DIFF); - const highlighted = highlightDiffFile(files[0]); - const hunk = highlighted.hunks[0]; - - const addedLine = hunk.lines.find( - (line) => line.type === "add" && line.content.includes("version"), - ); - expect(addedLine?.tokens).toContainEqual( - expect.objectContaining({ text: "2", style: "number" }), - ); - }); - - it("adds syntax highlighting tokens to Go code", () => { - const files = parseDiff(GO_DIFF); - const highlighted = highlightDiffFile(files[0]); - const addedLine = highlighted.hunks[0].lines.find( - (line) => line.type === "add" && line.content.includes("version"), - ); - - expect(addedLine?.tokens).toContainEqual( - expect.objectContaining({ text: "2", style: "number" }), - ); - }); - - it("adds syntax highlighting tokens to PHP code", () => { - const files = parseDiff(PHP_DIFF); - const highlighted = highlightDiffFile(files[0]); - const addedLine = highlighted.hunks[0].lines.find( - (line) => line.type === "add" && line.content.includes("$version"), - ); - - expect(addedLine?.tokens).toContainEqual( - expect.objectContaining({ text: "2", style: "number" }), - ); - }); - - it("adds syntax highlighting tokens to YAML code", () => { - const files = parseDiff(YAML_DIFF); - const highlighted = highlightDiffFile(files[0]); - const addedLine = highlighted.hunks[0].lines.find( - (line) => line.type === "add" && line.content.includes("count"), - ); - - expect(addedLine?.tokens?.length).toBeGreaterThan(0); - }); - - it("adds syntax highlighting tokens to XML code", () => { - const files = parseDiff(XML_DIFF); - const highlighted = highlightDiffFile(files[0]); - const addedLine = highlighted.hunks[0].lines.find( - (line) => line.type === "add" && line.content.includes(""), - ); - - expect(addedLine?.tokens).toContainEqual(expect.objectContaining({ style: "tag" })); - }); -}); - -describe("parseAndHighlightDiff", () => { - it("parses and highlights in one step", () => { - const files = parseAndHighlightDiff(SIMPLE_DIFF); - - expect(files).toHaveLength(1); - expect(files[0].hunks[0].lines[1].tokens).toContainEqual( - expect.objectContaining({ text: "const", style: "keyword" }), - ); - }); - - it("handles multiple files", () => { - const multiFileDiff = SIMPLE_DIFF + "\n" + NEW_FILE_DIFF; - const files = parseAndHighlightDiff(multiFileDiff); - - expect(files).toHaveLength(2); - expect(files[0].path).toBe("example.ts"); - expect(files[1].path).toBe("newfile.ts"); - }); -}); diff --git a/packages/app/src/utils/diff-highlighter.ts b/packages/app/src/utils/diff-highlighter.ts deleted file mode 100644 index 1830df603..000000000 --- a/packages/app/src/utils/diff-highlighter.ts +++ /dev/null @@ -1,313 +0,0 @@ -import { highlightCode, isLanguageSupported, type HighlightToken } from "@getpaseo/highlight"; - -export interface DiffLine { - type: "add" | "remove" | "context" | "header"; - content: string; - lineNumber?: number; // Line number in the original/new file - tokens?: HighlightToken[]; -} - -export interface DiffHunk { - oldStart: number; - oldCount: number; - newStart: number; - newCount: number; - lines: DiffLine[]; -} - -export interface ParsedDiffFile { - path: string; - isNew: boolean; - isDeleted: boolean; - additions: number; - deletions: number; - hunks: DiffHunk[]; -} - -const DIFF_METADATA_PREFIXES = ["index ", "--- ", "+++ ", "new file mode", "deleted file mode"]; - -function isDiffMetadataLine(line: string): boolean { - return DIFF_METADATA_PREFIXES.some((prefix) => line.startsWith(prefix)); -} - -// Git's default patch headers use paired a/path and b/path prefixes, while -// diff.noprefix emits plain paths that may legitimately start with a/ or b/. -function usesDiffPathPrefixes(oldPath: string, newPath: string): boolean { - return oldPath.startsWith("a/") && newPath.startsWith("b/"); -} - -function extractPathFromMetadata(lines: string[], prefix: "--- " | "+++ "): string | null { - const line = lines.find((candidate) => candidate.startsWith(prefix)); - if (!line) { - return null; - } - - const path = line.slice(prefix.length).replace(/\t.*$/, "").trimEnd(); - return path === "/dev/null" ? null : path; -} - -function extractDiffPath(lines: string[]): string { - const firstLine = lines[0] ?? ""; - const prefixedPathMatch = firstLine.match(/^a\/(.+) b\/(.+)$/); - if (prefixedPathMatch) { - return prefixedPathMatch[2]; - } - - const metadataPath = - extractPathFromMetadata(lines, "+++ ") ?? extractPathFromMetadata(lines, "--- "); - if (metadataPath) { - return metadataPath; - } - - const pathMatch = firstLine.match(/^(\S+)\s+(\S+)$/); - if (pathMatch) { - const [, oldPath, newPath] = pathMatch; - const path = newPath === "/dev/null" ? oldPath : newPath; - return usesDiffPathPrefixes(oldPath, newPath) ? path.slice(2) : path; - } - return "unknown"; -} - -function pushContentLine(hunk: DiffHunk, line: string): { addition: number; deletion: number } { - if (line.startsWith("+")) { - hunk.lines.push({ type: "add", content: line.slice(1) }); - return { addition: 1, deletion: 0 }; - } - if (line.startsWith("-")) { - hunk.lines.push({ type: "remove", content: line.slice(1) }); - return { addition: 0, deletion: 1 }; - } - if (line.startsWith(" ")) { - hunk.lines.push({ type: "context", content: line.slice(1) }); - return { addition: 0, deletion: 0 }; - } - if (line.length > 0 && !line.startsWith("\\")) { - hunk.lines.push({ type: "context", content: line }); - } - return { addition: 0, deletion: 0 }; -} - -interface HunkParseResult { - hunks: DiffHunk[]; - additions: number; - deletions: number; -} - -function parseHunks(lines: string[]): HunkParseResult { - const hunks: DiffHunk[] = []; - let currentHunk: DiffHunk | null = null; - let additions = 0; - let deletions = 0; - - for (let i = 1; i < lines.length; i++) { - const line = lines[i]; - if (isDiffMetadataLine(line)) continue; - - const hunkMatch = line.match(/^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/); - if (hunkMatch) { - if (currentHunk) { - hunks.push(currentHunk); - } - currentHunk = { - oldStart: parseInt(hunkMatch[1], 10), - oldCount: parseInt(hunkMatch[2] ?? "1", 10), - newStart: parseInt(hunkMatch[3], 10), - newCount: parseInt(hunkMatch[4] ?? "1", 10), - lines: [{ type: "header", content: line.match(/^(@@ .+? @@)/)?.[1] ?? line }], - }; - continue; - } - - if (!currentHunk) continue; - - const delta = pushContentLine(currentHunk, line); - additions += delta.addition; - deletions += delta.deletion; - } - - if (currentHunk) { - hunks.push(currentHunk); - } - return { hunks, additions, deletions }; -} - -/** - * Parse a unified diff into structured data - */ -export function parseDiff(diffText: string): ParsedDiffFile[] { - if (!diffText || diffText.trim().length === 0) { - return []; - } - - const files: ParsedDiffFile[] = []; - const fileSections = diffText.split(/^diff --git /m).filter(Boolean); - - for (const section of fileSections) { - const lines = section.split("\n"); - - const isNew = section.includes("new file mode") || section.includes("--- /dev/null"); - const isDeleted = section.includes("deleted file mode") || section.includes("+++ /dev/null"); - const path = extractDiffPath(lines); - const { hunks, additions, deletions } = parseHunks(lines); - - files.push({ path, isNew, isDeleted, additions, deletions, hunks }); - } - - return files; -} - -/** - * Reconstruct the "new" version of a file from diff hunks. - * Returns a map of new line numbers to their content. - */ -export function reconstructNewFile(hunks: DiffHunk[]): Map { - const lines = new Map(); - - for (const hunk of hunks) { - let newLineNum = hunk.newStart; - - for (const line of hunk.lines) { - if (line.type === "header") continue; - - if (line.type === "add" || line.type === "context") { - lines.set(newLineNum, line.content); - newLineNum++; - } - // Remove lines don't appear in the new file - } - } - - return lines; -} - -/** - * Reconstruct the "old" version of a file from diff hunks. - * Returns a map of old line numbers to their content. - */ -export function reconstructOldFile(hunks: DiffHunk[]): Map { - const lines = new Map(); - - for (const hunk of hunks) { - let oldLineNum = hunk.oldStart; - - for (const line of hunk.lines) { - if (line.type === "header") continue; - - if (line.type === "remove" || line.type === "context") { - lines.set(oldLineNum, line.content); - oldLineNum++; - } - // Add lines don't appear in the old file - } - } - - return lines; -} - -/** - * Apply syntax highlighting to diff hunks. - * - * Strategy: - * 1. Reconstruct both old and new file versions from the hunks - * 2. Highlight each version as a complete file - * 3. Map highlighted tokens back to diff lines using line numbers - */ -export function highlightDiffFile(file: ParsedDiffFile): ParsedDiffFile { - if (!isLanguageSupported(file.path)) { - return file; - } - - // Reconstruct both versions - const newFileLines = reconstructNewFile(file.hunks); - const oldFileLines = reconstructOldFile(file.hunks); - - // Build complete file content strings for highlighting - const newFileContent = buildFileContent(newFileLines); - const oldFileContent = buildFileContent(oldFileLines); - - // Highlight both versions - const newHighlighted = highlightCode(newFileContent, file.path); - const oldHighlighted = highlightCode(oldFileContent, file.path); - - // Build lookup maps: line number -> tokens - const newTokensByLine = buildTokenLookup(newFileLines, newHighlighted); - const oldTokensByLine = buildTokenLookup(oldFileLines, oldHighlighted); - - // Apply tokens to hunks - const highlightedHunks = file.hunks.map((hunk) => { - let oldLineNum = hunk.oldStart; - let newLineNum = hunk.newStart; - - const highlightedLines = hunk.lines.map((line): DiffLine => { - if (line.type === "header") { - return line; - } - - let tokens: HighlightToken[] | undefined; - - if (line.type === "add") { - tokens = newTokensByLine.get(newLineNum); - newLineNum++; - } else if (line.type === "remove") { - tokens = oldTokensByLine.get(oldLineNum); - oldLineNum++; - } else if (line.type === "context") { - // Context lines exist in both - use new file version - tokens = newTokensByLine.get(newLineNum); - oldLineNum++; - newLineNum++; - } - - return tokens ? { ...line, tokens } : line; - }); - - return { ...hunk, lines: highlightedLines }; - }); - - return { ...file, hunks: highlightedHunks }; -} - -function buildFileContent(lineMap: Map): string { - if (lineMap.size === 0) return ""; - - const lineNumbers = Array.from(lineMap.keys()).sort((a, b) => a - b); - const minLine = lineNumbers[0]; - const maxLine = lineNumbers[lineNumbers.length - 1]; - - const lines: string[] = []; - for (let i = minLine; i <= maxLine; i++) { - lines.push(lineMap.get(i) ?? ""); - } - - return lines.join("\n"); -} - -function buildTokenLookup( - lineMap: Map, - highlighted: HighlightToken[][], -): Map { - const lookup = new Map(); - - if (lineMap.size === 0) return lookup; - - const lineNumbers = Array.from(lineMap.keys()).sort((a, b) => a - b); - const minLine = lineNumbers[0]; - - // highlighted array is 0-indexed, line numbers are 1-indexed - for (let i = 0; i < highlighted.length; i++) { - const lineNum = minLine + i; - if (lineMap.has(lineNum)) { - lookup.set(lineNum, highlighted[i]); - } - } - - return lookup; -} - -/** - * Parse and highlight a complete diff - */ -export function parseAndHighlightDiff(diffText: string): ParsedDiffFile[] { - const files = parseDiff(diffText); - return files.map(highlightDiffFile); -} diff --git a/packages/app/src/utils/diff-rendering.test.ts b/packages/app/src/utils/diff-rendering.test.ts index 4d6e35e89..384bd2291 100644 --- a/packages/app/src/utils/diff-rendering.test.ts +++ b/packages/app/src/utils/diff-rendering.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { + compactHighlightTokens, formatDiffContentText, formatDiffGutterText, hasVisibleDiffTokens, @@ -28,4 +29,23 @@ describe("diff-rendering", () => { expect(hasVisibleDiffTokens([{ text: "" }])).toBe(false); expect(hasVisibleDiffTokens([{ text: "const value = 1;" }])).toBe(true); }); + + it("compacts adjacent syntax tokens without changing their text or styles", () => { + expect( + compactHighlightTokens([ + { text: "const", style: "keyword" }, + { text: " ", style: null }, + { text: "value", style: null }, + { text: " ", style: null }, + { text: "=", style: "operator" }, + { text: "", style: "operator" }, + { text: " 1", style: "number" }, + ]), + ).toEqual([ + { text: "const", style: "keyword" }, + { text: " value ", style: null }, + { text: "=", style: "operator" }, + { text: " 1", style: "number" }, + ]); + }); }); diff --git a/packages/app/src/utils/diff-rendering.ts b/packages/app/src/utils/diff-rendering.ts index a189c56b6..c3cd9f8bb 100644 --- a/packages/app/src/utils/diff-rendering.ts +++ b/packages/app/src/utils/diff-rendering.ts @@ -2,6 +2,28 @@ interface HighlightLikeToken { text: string; } +interface StyledHighlightToken extends HighlightLikeToken { + style: string | null; +} + +export function compactHighlightTokens( + tokens: readonly StyledHighlightToken[], +): StyledHighlightToken[] { + const compacted: StyledHighlightToken[] = []; + for (const token of tokens) { + if (token.text.length === 0) { + continue; + } + const previous = compacted.at(-1); + if (previous?.style === token.style) { + previous.text += token.text; + continue; + } + compacted.push({ text: token.text, style: token.style }); + } + return compacted; +} + // Preserve row height when a gutter or diff cell is intentionally blank. // Non-breaking space because the gutter uses numberOfLines={1}, which // collapses a plain ASCII space to zero height on web. diff --git a/packages/server/src/server/utils/diff-highlighter.test.ts b/packages/server/src/server/utils/diff-highlighter.test.ts index 8816dd520..58e3c16b1 100644 --- a/packages/server/src/server/utils/diff-highlighter.test.ts +++ b/packages/server/src/server/utils/diff-highlighter.test.ts @@ -331,6 +331,28 @@ describe("reconstructOldFile", () => { }); describe("highlightDiffFromHunks", () => { + it("continues highlighting large diffs whose lines stay bounded", () => { + const sourceLines = Array.from( + { length: 101 }, + (_, index) => `const value${index} = "${"x".repeat(990)}";`, + ); + const [file] = parseDiff( + [ + "diff --git a/example.ts b/example.ts", + "--- /dev/null", + "+++ b/example.ts", + `@@ -0,0 +1,${sourceLines.length} @@`, + ...sourceLines.map((line) => `+${line}`), + ].join("\n"), + ); + + const highlighted = highlightDiffFromHunks(file); + const contentLines = highlighted.hunks[0].lines.slice(1); + + expect(contentLines[0].tokens).toContainEqual({ text: "const", style: "keyword" }); + expect(contentLines[100].tokens).toContainEqual({ text: "const", style: "keyword" }); + }); + it("adds syntax highlighting tokens to TypeScript code", () => { const files = parseDiff(SIMPLE_DIFF); const highlighted = highlightDiffFromHunks(files[0]); @@ -507,6 +529,40 @@ describe("highlightDiffFromHunks", () => { }); describe("highlightDiffWithFileContent", () => { + it("leaves files with an oversized line unhighlighted", async () => { + const oversizedLine = `const payload = "${"x".repeat(10_001)}";`; + const [file] = parseDiff( + [ + "diff --git a/example.ts b/example.ts", + "--- /dev/null", + "+++ b/example.ts", + "@@ -0,0 +1 @@", + `+${oversizedLine}`, + ].join("\n"), + ); + + const highlighted = await highlightDiffWithFileContent(file, ".", { + newFileContent: oversizedLine, + }); + + expect(highlighted.hunks[0].lines[1]).toEqual({ + type: "add", + content: oversizedLine, + }); + }); + + it("uses hunk context when the full file has an oversized line elsewhere", async () => { + const file = parseDiff(SIMPLE_DIFF)[0]; + const oversizedUnchangedLine = `const payload = "${"x".repeat(10_001)}";`; + + const highlighted = await highlightDiffWithFileContent(file, ".", { + newFileContent: `${oversizedUnchangedLine}\nconst bar = 3;`, + }); + const addedLine = highlighted.hunks[0].lines.find((line) => line.type === "add"); + + expect(addedLine?.tokens).toContainEqual({ text: "3", style: "number" }); + }); + it("uses the old file content to preserve syntax context for removed lines", async () => { const cwd = mkdtempSync(join(tmpdir(), "diff-highlight-old-file-")); diff --git a/packages/server/src/server/utils/diff-highlighter.ts b/packages/server/src/server/utils/diff-highlighter.ts index c016318e4..4f7f800dc 100644 --- a/packages/server/src/server/utils/diff-highlighter.ts +++ b/packages/server/src/server/utils/diff-highlighter.ts @@ -2,6 +2,8 @@ import { readFile } from "node:fs/promises"; import { resolve } from "node:path"; import { highlightCode, isLanguageSupported, type HighlightToken } from "@getpaseo/highlight"; +const MAX_DIFF_HIGHLIGHT_LINE_CHARS = 10_000; + export interface DiffLine { type: "add" | "remove" | "context" | "header"; content: string; @@ -227,6 +229,25 @@ function buildFileContent(lineMap: Map): string { return lines.join("\n"); } +function hasOversizedLine(content: string): boolean { + let lineStart = 0; + for (let index = 0; index <= content.length; index++) { + if (index !== content.length && content.charCodeAt(index) !== 10) continue; + if (index - lineStart > MAX_DIFF_HIGHLIGHT_LINE_CHARS) return true; + lineStart = index + 1; + } + return false; +} + +function hasOversizedDiffLine(file: ParsedDiffFile): boolean { + for (const hunk of file.hunks) { + for (const line of hunk.lines) { + if (line.content.length > MAX_DIFF_HIGHLIGHT_LINE_CHARS) return true; + } + } + return false; +} + function buildTokenLookup( lineMap: Map, highlighted: HighlightToken[][], @@ -251,7 +272,9 @@ function buildTokenLookup( function buildFullFileTokenLookup( fileContent: string, path: string, -): Map { +): Map | null { + if (hasOversizedLine(fileContent)) return null; + const lookup = new Map(); const highlighted = highlightCode(fileContent, path); @@ -284,13 +307,16 @@ function buildReconstructedTokenLookups(file: ParsedDiffFile): { * This is the fallback when actual file content is not available. */ export function highlightDiffFromHunks(file: ParsedDiffFile): ParsedDiffFile { - if (!isLanguageSupported(file.path)) { + if (!isLanguageSupported(file.path) || hasOversizedDiffLine(file)) { return file; } - const { newTokensByLine, oldTokensByLine } = buildReconstructedTokenLookups(file); - - return applyTokensToHunks(file, newTokensByLine, oldTokensByLine); + const reconstructedTokens = buildReconstructedTokenLookups(file); + return applyTokensToHunks( + file, + reconstructedTokens.newTokensByLine, + reconstructedTokens.oldTokensByLine, + ); } /** @@ -302,7 +328,7 @@ export async function highlightDiffWithFileContent( cwd: string, options: HighlightDiffWithFileContentOptions = {}, ): Promise { - if (!isLanguageSupported(file.path)) { + if (!isLanguageSupported(file.path) || hasOversizedDiffLine(file)) { return file; } @@ -311,18 +337,20 @@ export async function highlightDiffWithFileContent( let oldTokensByLine = reconstructedTokens.oldTokensByLine; if (typeof options.oldFileContent === "string") { - oldTokensByLine = buildFullFileTokenLookup(options.oldFileContent, file.path); + oldTokensByLine = + buildFullFileTokenLookup(options.oldFileContent, file.path) ?? oldTokensByLine; } if (typeof options.newFileContent === "string") { - newTokensByLine = buildFullFileTokenLookup(options.newFileContent, file.path); + newTokensByLine = + buildFullFileTokenLookup(options.newFileContent, file.path) ?? newTokensByLine; return applyTokensToHunks(file, newTokensByLine, oldTokensByLine); } const filePath = resolve(cwd, file.path); try { const fileContent = await readFile(filePath, "utf-8"); - newTokensByLine = buildFullFileTokenLookup(fileContent, file.path); + newTokensByLine = buildFullFileTokenLookup(fileContent, file.path) ?? newTokensByLine; } catch { // If file read fails (deleted file, etc.), fall back to reconstructed new-side tokens. }