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. }