diff --git a/docs/development.md b/docs/development.md index d45ec589fc..873f9cabb9 100644 --- a/docs/development.md +++ b/docs/development.md @@ -273,6 +273,26 @@ Playwright dispatch lateness. Set `PASEO_PROFILE_TYPING_KEYS`, `PASEO_PROFILE_TY `PASEO_PROFILE_CPU_PATH` and `PASEO_PROFILE_TRACE_PATH` captures run separately after the latency measurements so profiling overhead does not contaminate them. +For diff layout, two benchmarks share one corpus +(`packages/app/src/git/diff-document/benchmark-corpus.ts`: a real source file, a changelog with +600-character lines, generated code with ligature punctuation, and JSON). The node bench runs the +shared document model with the web and native measurers replaced by counters, so it reports how +many strings each backend would have to shape, which is the cost that survives the move to canvas +or Skia: + +```bash +cd packages/app && npx vitest bench --project unit src/git/diff-document/model.bench.ts +``` + +The browser benchmark opens the same corpus as a working diff, toggles line wrap from the toolbar, +and reports pointer-down to painted latency, the longest main-thread task, and the `DiffDocument` +React commit time for cold and warm toggles, then scroll frame times in both modes: + +```bash +cd packages/app && PASEO_DIFF_PERF_E2E=1 npx playwright test --project=browser \ + e2e/browser/diff-performance.spec.ts -g "toggling line wrap" +``` + Set `PASEO_PROFILE_TYPING_SCENARIO=height-growth` to alternate `Shift+Enter` and printable input. That report includes input and composer height changes plus React work grouped into composer, stream, and ancestor/root scopes. Ancestor/root timings include descendant work because the Profiler diff --git a/packages/app/e2e/browser/diff-performance.spec.ts b/packages/app/e2e/browser/diff-performance.spec.ts index c11c82f279..0358b6359e 100644 --- a/packages/app/e2e/browser/diff-performance.spec.ts +++ b/packages/app/e2e/browser/diff-performance.spec.ts @@ -1,6 +1,7 @@ import { writeFile } from "node:fs/promises"; import path from "node:path"; import type { CDPSession, Locator, Page, TestInfo } from "@playwright/test"; +import { pullRequestCorpus } from "../../src/git/diff-document/benchmark-corpus"; import { buildHostWorkspaceRoute } from "../../src/utils/host-routes"; import { test, expect } from "../support/fixtures"; import { getServerId } from "../support/helpers/server-id"; @@ -296,8 +297,187 @@ diffPerfDescribe("Diff canvas performance", () => { await repo.cleanup().catch(() => undefined); } }); + test("toggling line wrap on a pull-request-sized diff", async ({ page }, testInfo) => { + test.setTimeout(300_000); + const corpus = await createPullRequestCorpusRepository(); + const client = await connectSeedClient(); + + try { + const created = await client.createWorkspace({ + source: { kind: "directory", path: corpus.repo.path }, + }); + if (!created.workspace) { + throw new Error(created.error ?? "Failed to create wrap performance workspace"); + } + + await configureUnwrappedUnifiedDiff(page); + await instrumentDiffLatency(page); + await page.setViewportSize({ width: 1400, height: 900 }); + await page.goto(buildHostWorkspaceRoute(getServerId(), created.workspace.id)); + await waitForWorkspaceTabsVisible(page); + const openStartedAt = performance.now(); + await openChangesPanel(page, 90_000); + await expect(page.getByTestId("git-diff-canvas")).toBeVisible({ timeout: 30_000 }); + const open = { + readyMs: Math.round(performance.now() - openStartedAt), + ...(await readDiffDocumentRenders(page)), + }; + + const toggles: WrapToggleMeasurement[] = []; + for (const name of [ + "wrap on (cold)", + "wrap off (cold)", + "wrap on (warm)", + "wrap off (warm)", + ]) { + toggles.push(await measureWrapToggle(page, name)); + } + const unwrappedScroll = await continuouslyScrollCanvas(page); + toggles.push(await measureWrapToggle(page, "wrap on (warm, scrolled)")); + const wrappedScroll = await continuouslyScrollCanvas(page); + + const report = { + scenario: "wrap-toggle", + sourceFiles: corpus.fileCount, + sourceLines: corpus.lineCount, + open, + toggles, + scroll: { + unwrapped: summarizeScroll(unwrappedScroll), + wrapped: summarizeScroll(wrappedScroll), + }, + }; + await attachPerformanceReport(testInfo, report); + console.log(`[perf] Wrap toggle: ${JSON.stringify(report)}`); + + const [wrapOn, wrapOff] = toggles; + expect(wrapOn!.scrollHeightAfter).toBeGreaterThan(wrapOn!.scrollHeightBefore); + expect(wrapOff!.scrollHeightAfter).toBe(wrapOn!.scrollHeightBefore); + } finally { + await client.close().catch(() => undefined); + await corpus.repo.cleanup().catch(() => undefined); + } + }); }); +interface WrapToggleMeasurement { + name: string; + /** Pointer down on the toolbar button to two painted frames after the document resized. */ + latencyMs: number; + scrollHeightBefore: number; + scrollHeightAfter: number; + longestTaskMs: number; + longTaskTotalMs: number; + diffDocumentCommits: number; + diffDocumentRenderMs: number; +} + +interface LongTask { + startTime: number; + duration: number; +} + +interface DiffLatencyScope { + __PASEO_RENDER_PROFILE_ENABLED__?: boolean; + __PASEO_RENDER_PROFILE__?: Array<{ id: string; actualDuration: number }>; + __PASEO_RESET_RENDER_PROFILE__?: () => void; + __PASEO_LONG_TASKS__?: LongTask[]; + __PASEO_WRAP_TOGGLE_AT__?: number; +} + +async function createPullRequestCorpusRepository() { + const files = pullRequestCorpus(); + const repo = await createTempGitRepo("diff-wrap-perf-", { + files: files.map((file) => ({ path: file.path, content: "baseline\n" })), + }); + for (const file of files) { + await writeFile(path.join(repo.path, file.path), `${file.lines.join("\n")}\n`); + } + return { + repo, + fileCount: files.length, + lineCount: files.reduce((count, file) => count + file.lines.length, 0), + }; +} + +async function instrumentDiffLatency(page: Page): Promise { + await page.addInitScript(() => { + const scope = globalThis as typeof globalThis & DiffLatencyScope; + scope.__PASEO_RENDER_PROFILE_ENABLED__ = true; + scope.__PASEO_LONG_TASKS__ = []; + new PerformanceObserver((list) => { + for (const entry of list.getEntries()) { + scope.__PASEO_LONG_TASKS__?.push({ startTime: entry.startTime, duration: entry.duration }); + } + }).observe({ type: "longtask", buffered: true }); + }); +} + +async function readDiffDocumentRenders( + page: Page, +): Promise<{ diffDocumentCommits: number; diffDocumentRenderMs: number }> { + return page.evaluate(() => { + const scope = globalThis as typeof globalThis & DiffLatencyScope; + const samples = (scope.__PASEO_RENDER_PROFILE__ ?? []).filter( + (sample) => sample.id === "DiffDocument", + ); + return { + diffDocumentCommits: samples.length, + diffDocumentRenderMs: Math.round( + samples.reduce((total, sample) => total + sample.actualDuration, 0), + ), + }; + }); +} + +async function measureWrapToggle(page: Page, name: string): Promise { + const scroll = page.getByTestId("git-diff-scroll"); + const scrollHeightBefore = await scroll.evaluate((element) => element.scrollHeight); + await page.evaluate(() => { + const scope = globalThis as typeof globalThis & DiffLatencyScope; + scope.__PASEO_RESET_RENDER_PROFILE__?.(); + scope.__PASEO_LONG_TASKS__ = []; + scope.__PASEO_WRAP_TOGGLE_AT__ = undefined; + document.addEventListener( + "pointerdown", + () => { + scope.__PASEO_WRAP_TOGGLE_AT__ = performance.now(); + }, + { capture: true, once: true }, + ); + }); + await page.getByTestId("changes-toggle-wrap-lines").click(); + const measured = await scroll.evaluate(async (element, before) => { + const scope = globalThis as typeof globalThis & DiffLatencyScope; + const deadline = performance.now() + 240_000; + while (element.scrollHeight === before && performance.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 5)); + } + await new Promise((resolve) => + requestAnimationFrame(() => requestAnimationFrame(() => resolve())), + ); + const doneAt = performance.now(); + // Long task entries are delivered after the task ends; give the observer a turn. + await new Promise((resolve) => setTimeout(resolve, 50)); + const startedAt = scope.__PASEO_WRAP_TOGGLE_AT__ ?? doneAt; + const longTasks = (scope.__PASEO_LONG_TASKS__ ?? []).filter( + (task) => task.startTime + task.duration >= startedAt && task.startTime <= doneAt, + ); + return { + latencyMs: Math.round(doneAt - startedAt), + scrollHeightAfter: element.scrollHeight, + longestTaskMs: Math.round(Math.max(0, ...longTasks.map((task) => task.duration))), + longTaskTotalMs: Math.round(longTasks.reduce((total, task) => total + task.duration, 0)), + }; + }, scrollHeightBefore); + return { name, scrollHeightBefore, ...measured, ...(await readDiffDocumentRenders(page)) }; +} + +function summarizeScroll(report: Awaited>) { + const { frameSignatures: _frameSignatures, ...summary } = report; + return summary; +} + async function measureAdditionalPerformanceScenario( page: Page, cdp: CDPSession, diff --git a/packages/app/src/git/diff-document/benchmark-corpus.ts b/packages/app/src/git/diff-document/benchmark-corpus.ts new file mode 100644 index 0000000000..469d437e8d --- /dev/null +++ b/packages/app/src/git/diff-document/benchmark-corpus.ts @@ -0,0 +1,65 @@ +import { readFileSync } from "node:fs"; +import path from "node:path"; + +/** + * The diff shapes the layout benchmarks measure, shared by the node bench + * (`model.bench.ts`) and the browser benchmark (`e2e/browser/diff-performance.spec.ts`) + * so both report against the same text. + */ +export interface BenchmarkCorpusFile { + path: string; + lines: string[]; +} + +function repeatToLength(unit: string, length: number): string { + return unit.repeat(Math.ceil(length / unit.length)).slice(0, length); +} + +/** A real source file: short lines, a few with ligature punctuation. */ +export function realCodeCorpusFile(): BenchmarkCorpusFile { + const source = readFileSync(path.resolve(__dirname, "../diff-pane.tsx"), "utf8"); + return { path: "packages/app/src/git/diff-pane.tsx", lines: source.split("\n") }; +} + +/** The CHANGELOG shape that once froze the renderer: 42 lines, longest ~600 characters. */ +export function changelogCorpusFile(): BenchmarkCorpusFile { + const sentence = + "- Fixed an issue where the diff viewer re-measured every line -> on toggle, which made wrapping painfully slow. "; + return { + path: "CHANGELOG.md", + lines: Array.from({ length: 42 }, (_, index) => + repeatToLength(sentence, 200 + ((index * 97) % 398)), + ), + }; +} + +/** Generated TypeScript: ligature punctuation on every line, every tenth line long. */ +export function generatedCodeCorpusFile(lineCount: number): BenchmarkCorpusFile { + return { + path: "src/generated.ts", + lines: Array.from({ length: lineCount }, (_, index) => { + const base = `const handler${index} = async (event) => items.filter((item) => item.id !== ${index} && item.kind === "selected");`; + return index % 10 === 9 ? repeatToLength(`${base} // `, 320) : base; + }), + }; +} + +/** Serialized data: no ligatures, every line wraps several times. */ +export function jsonCorpusFile(lineCount: number): BenchmarkCorpusFile { + return { + path: "fixtures/data.json", + lines: Array.from({ length: lineCount }, (_, index) => + repeatToLength(`{"id": ${index}, "label": "row ${index}", "tags": ["alpha", "beta"]}, `, 300), + ), + }; +} + +/** A pull-request-sized working diff: one of each shape above. */ +export function pullRequestCorpus(): BenchmarkCorpusFile[] { + return [ + realCodeCorpusFile(), + changelogCorpusFile(), + generatedCodeCorpusFile(1200), + jsonCorpusFile(300), + ]; +} diff --git a/packages/app/src/git/diff-document/model.bench.ts b/packages/app/src/git/diff-document/model.bench.ts new file mode 100644 index 0000000000..2fb25d502a --- /dev/null +++ b/packages/app/src/git/diff-document/model.bench.ts @@ -0,0 +1,317 @@ +import type { ParsedDiffFile } from "@getpaseo/protocol/messages"; +import { bench, describe } from "vitest"; +import { + changelogCorpusFile, + generatedCodeCorpusFile, + jsonCorpusFile, + pullRequestCorpus, + realCodeCorpusFile, + type BenchmarkCorpusFile, +} from "./benchmark-corpus"; +import { buildDiffDocumentModel } from "./model"; +import { + createAdditiveAdvances, + createChunkedAdvanceMeasurer, + createChunkedWidthMeasurer, + createMeasuredAdvances, + requiresShaping, +} from "./text-measurement"; +import type { BuildDiffDocumentModelInput, TextMeasurer } from "./types"; + +// The pure layout work the diff surface does when a diff opens or line wrap is +// toggled, without a browser or Skia. Counts are the backend-independent cost +// owners (how many strings a shaper has to see), timings are the JS overhead +// around them: +// npx vitest bench --project unit src/git/diff-document/model.bench.ts + +const DESKTOP_VIEWPORT = { width: 1400, height: 900 }; +const PHONE_VIEWPORT = { width: 390, height: 800 }; +const CELL_WIDTH = 7.8; + +interface MeasurementStats { + /** Whole strings handed to the backend. */ + calls: number; + /** Characters inside those strings; canvas and Skia cost scales with this. */ + characters: number; +} + +interface Backend { + measurer: TextMeasurer; + stats: MeasurementStats; +} + +function monospaceWidth(text: string): number { + let width = 0; + for (const character of text) width += character.charCodeAt(0) > 0x2e7f ? 2 : 1; + return width * CELL_WIDTH; +} + +function countingMeasure(stats: MeasurementStats): (text: string) => number { + return (text) => { + stats.calls += 1; + stats.characters += text.length; + return monospaceWidth(text); + }; +} + +/** Previous web backend, retained to isolate the cost of width-only probes. */ +function webBackendWithoutWidthProbe(): Backend { + const stats: MeasurementStats = { calls: 0, characters: 0 }; + const measure = countingMeasure(stats); + return { stats, measurer: { measure, measureAdvances: createMeasuredAdvances(measure) } }; +} + +/** + * `createWebTextMeasurer` in surface.web.tsx, with canvas replaced by a counter. + * Width-only probes shape one string per chunk, like the native measurer; the + * widths are the same sums, in the same order, as the last cumulative advance. + */ +function webBackend(): Backend { + const stats: MeasurementStats = { calls: 0, characters: 0 }; + const measure = countingMeasure(stats); + const additive = createAdditiveAdvances(measure); + return { + stats, + measurer: { + measure, + measureAdvances: createMeasuredAdvances(measure), + measureWidth: createChunkedWidthMeasurer((graphemes) => { + const text = graphemes.join(""); + return requiresShaping(text) ? measure(text) : (additive(graphemes).at(-1) ?? 0); + }), + }, + }; +} + +/** `createNativeTextMeasurer` in text.native.ts, with Skia replaced by a counter. */ +function nativeBackend(): Backend { + const stats: MeasurementStats = { calls: 0, characters: 0 }; + const widths = new Map(); + const additive = (graphemes: readonly string[]) => { + let advance = 0; + return graphemes.map((grapheme) => { + let width = widths.get(grapheme); + if (width === undefined) { + stats.calls += 1; + stats.characters += grapheme.length; + width = monospaceWidth(grapheme); + widths.set(grapheme, width); + } + return (advance += width); + }); + }; + const shape = countingMeasure(stats); + return { + stats, + measurer: { + measure(text) { + return requiresShaping(text) ? shape(text) : (additive([...text]).at(-1) ?? 0); + }, + measureWidth: createChunkedWidthMeasurer((graphemes) => { + const text = graphemes.join(""); + return requiresShaping(text) ? shape(text) : (additive(graphemes).at(-1) ?? 0); + }), + measureAdvances: createChunkedAdvanceMeasurer({ + requiresShaping, + measureAdditive: additive, + measureShaped(graphemes) { + shape(graphemes.join("")); + let advance = 0; + return graphemes.map((grapheme) => (advance += monospaceWidth(grapheme))); + }, + }), + }, + }; +} + +function addedFile(file: BenchmarkCorpusFile): ParsedDiffFile { + return { + path: file.path, + isNew: true, + isDeleted: false, + additions: file.lines.length, + deletions: 0, + hunks: [ + { + oldStart: 0, + oldCount: 0, + newStart: 1, + newCount: file.lines.length, + lines: file.lines.map((content) => ({ + type: "add" as const, + content, + tokens: [{ text: content, style: null }], + })), + }, + ], + }; +} + +interface Scenario { + name: string; + files: ParsedDiffFile[]; +} + +const SCENARIOS: Scenario[] = [ + { name: "real code (diff-pane.tsx)", files: [addedFile(realCodeCorpusFile())] }, + { name: "changelog (42 long prose lines)", files: [addedFile(changelogCorpusFile())] }, + { name: "generated code (1200 lines)", files: [addedFile(generatedCodeCorpusFile(1200))] }, + { name: "json (300 lines x 300 chars)", files: [addedFile(jsonCorpusFile(300))] }, + { name: "pull request (all of the above)", files: pullRequestCorpus().map(addedFile) }, +]; + +function input( + scenario: Scenario, + options: { + wrapLines: boolean; + viewport: { width: number; height: number }; + measureText: TextMeasurer; + }, +): BuildDiffDocumentModelInput { + return { + files: scenario.files, + collapsedFilePaths: new Set(), + layout: "unified", + wrapLines: options.wrapLines, + viewportWidth: options.viewport.width, + typography: { family: "monospace", size: 13, lineHeight: 20 }, + measureText: options.measureText, + palette: { + surface: "#000", + headerSurface: "#111", + border: "#222", + foreground: "#fff", + foregroundMuted: "#aaa", + addition: "green", + deletion: "red", + additionBackground: "#010", + deletionBackground: "#100", + emptyBackground: "#111", + selection: "blue", + headerActiveSurface: "#222", + headerBorder: "#333", + statusSuccess: "green", + statusDanger: "red", + statusWarning: "orange", + syntax: {}, + }, + labels: { binary: "Binary", tooLarge: "Too large" }, + // What the surfaces pass at scrollTop 0: five viewports tall. + materializationWindow: { top: 0, height: options.viewport.height * 5 }, + }; +} + +interface Variant { + name: string; + wrapLines: boolean; + viewport: { width: number; height: number }; + backend: () => Backend; +} + +const VARIANTS: Variant[] = [ + { + name: "unwrapped desktop | web", + wrapLines: false, + viewport: DESKTOP_VIEWPORT, + backend: webBackend, + }, + { + name: "wrapped desktop | web", + wrapLines: true, + viewport: DESKTOP_VIEWPORT, + backend: webBackend, + }, + { + name: "wrapped desktop | web without width probe", + wrapLines: true, + viewport: DESKTOP_VIEWPORT, + backend: webBackendWithoutWidthProbe, + }, + { + name: "wrapped phone | web", + wrapLines: true, + viewport: PHONE_VIEWPORT, + backend: webBackend, + }, + { + name: "unwrapped desktop | native", + wrapLines: false, + viewport: DESKTOP_VIEWPORT, + backend: nativeBackend, + }, + { + name: "wrapped desktop | native", + wrapLines: true, + viewport: DESKTOP_VIEWPORT, + backend: nativeBackend, + }, + { + name: "wrapped phone | native", + wrapLines: true, + viewport: PHONE_VIEWPORT, + backend: nativeBackend, + }, +]; + +function countModel(scenario: Scenario, variant: Variant) { + const { measurer, stats } = variant.backend(); + const started = performance.now(); + const model = buildDiffDocumentModel( + input(scenario, { + wrapLines: variant.wrapLines, + viewport: variant.viewport, + measureText: measurer, + }), + ); + const elapsedMs = performance.now() - started; + let materializedCells = 0; + let fragments = 0; + let graphemes = 0; + for (const row of model.rows) { + if (row.kind !== "line") continue; + for (const cell of row.cells) { + if (!cell || cell.fragments.length === 0) continue; + materializedCells += 1; + fragments += cell.fragments.length; + for (const fragment of cell.fragments) graphemes += fragment.graphemes.length; + } + } + const sourceLines = scenario.files.reduce( + (count, file) => count + file.hunks.reduce((total, hunk) => total + hunk.lines.length, 0), + 0, + ); + return { + sourceLines, + materializedCells, + fragments, + graphemeObjects: graphemes, + measureCalls: stats.calls, + measuredCharacters: stats.characters, + coldMs: Number(elapsedMs.toFixed(1)), + }; +} + +const report: Record> = {}; +for (const scenario of SCENARIOS) { + for (const variant of VARIANTS) { + report[`${scenario.name} | ${variant.name}`] = countModel(scenario, variant); + } +} +console.table(report); + +for (const scenario of SCENARIOS) { + describe(scenario.name, () => { + for (const variant of VARIANTS) { + if (variant.backend === nativeBackend) continue; + bench(variant.name, () => { + buildDiffDocumentModel( + input(scenario, { + wrapLines: variant.wrapLines, + viewport: variant.viewport, + measureText: variant.backend().measurer, + }), + ); + }); + } + }); +} diff --git a/packages/app/src/git/diff-document/model.test.ts b/packages/app/src/git/diff-document/model.test.ts index ace965c1ce..9b55a242db 100644 --- a/packages/app/src/git/diff-document/model.test.ts +++ b/packages/app/src/git/diff-document/model.test.ts @@ -10,7 +10,7 @@ import { resolveRelayoutScrollTop, shouldApplyRelayoutScroll, } from "./model"; -import type { BuildDiffDocumentModelInput, TextMeasurer } from "./types"; +import type { BuildDiffDocumentModelInput, DiffCell, TextMeasurer } from "./types"; const measurer: TextMeasurer = { measure: (text) => Array.from(text).length * 10 }; @@ -639,17 +639,119 @@ describe("diff document model", () => { expect(firstLineCells(last, "b.ts")).toBe(firstLineCells(first, "b.ts")); }); - it("keeps wrapped geometry fully measured", () => { + it("replaces estimated wrapped heights through reuse and preserves the scroll anchor", () => { + const changed = fileWithChangedLine("x".repeat(100)); + const files = [changed, file("later.ts")]; + // Deliberately inaccurate M width makes the estimate differ from exact layout. + const measureText = { + measure: (text: string) => (text === "M" ? 5 : Array.from(text).length * 10), + }; + const options = input({ files, measureText }); + const first = buildDiffDocumentModel({ + ...options, + materializationWindow: { top: 30, height: 1 }, + }); + const eager = buildDiffDocumentModel(options); + const exact = buildDiffDocumentModel({ ...options, reuseFrom: [first] }); + expect(first.height).not.toBe(eager.height); + expect(exact.rows).toEqual(eager.rows); + expect(exact.height).toBe(eager.height); + expect(firstLineCells(exact, changed.path)).toBe(firstLineCells(first, changed.path)); + const scrollTop = first.files[1]!.bodyTop + 3; + const nextScrollTop = resolveRelayoutScrollTop(first, exact, scrollTop); + expect(nextScrollTop).toBe(exact.files[1]!.bodyTop + 3); + expect(captureScrollAnchor(exact, nextScrollTop)).toEqual( + captureScrollAnchor(first, scrollTop), + ); + }); + + it("preserves the wrapped source offset when resize leaves the anchor outside the window", () => { + const source = file(); + source.hunks[0]!.lines = Array.from({ length: 200 }, () => ({ + type: "add" as const, + content: "x".repeat(200), + })); + const options = input({ files: [source], viewportWidth: 500 }); + const before = buildDiffDocumentModel(options); + const scrollTop = before.rows[100]!.top + 36; + const anchor = captureScrollAnchor(before, scrollTop)!; + expect(anchor).toMatchObject({ kind: "text", cellOffset: 90 }); + const resizedOptions = { ...options, viewportWidth: 180 }; + const estimated = buildDiffDocumentModel({ + ...resizedOptions, + materializationWindow: { + top: Math.floor(scrollTop / 800) * 800 - 800, + height: 2000, + }, + }); + const estimatedRow = estimated.rows[100]!; + expect(estimatedRow.kind === "line" && estimatedRow.cells[0]?.fragments).toEqual([]); + const restored = resolveRelayoutScrollTop(before, estimated, scrollTop); + const recaptured = captureScrollAnchor(estimated, restored)!; + expect(recaptured).toMatchObject({ kind: "text", cellOffset: 78 }); + + const materialized = buildDiffDocumentModel({ + ...resizedOptions, + materializationWindow: { top: restored - 800, height: 2000 }, + reuseFrom: [estimated], + }); + const eager = buildDiffDocumentModel(resizedOptions); + const expected = resolveRelayoutScrollTop(before, eager, scrollTop); + expect(expected).toBe(28_938); + expect(Math.abs(restored - expected)).toBeLessThan(before.lineHeight); + expect(resolveRelayoutScrollTop(estimated, materialized, restored)).toBe(expected); + }); + + it("materializes the relayout anchor even when off-screen height estimates are inaccurate", () => { + const source = file(); + source.hunks[0]!.lines = Array.from({ length: 200 }, () => ({ + type: "add" as const, + content: "x".repeat(200), + })); + const options = input({ files: [source], viewportWidth: 500 }); + const before = buildDiffDocumentModel(options); + const scrollTop = before.rows[100]!.top + 38; + const anchor = captureScrollAnchor(before, scrollTop)!; + const resizedOptions = { + ...options, + viewportWidth: 180, + measureText: { measure: (text: string) => (text === "M" ? 5 : text.length * 10) }, + }; + const resized = buildDiffDocumentModel({ + ...resizedOptions, + materializationWindow: { top: scrollTop - 800, height: 2000 }, + materializationAnchor: anchor, + }); + const row = resized.rows[100]!; + expect(row.kind === "line" && row.cells[0]!.fragments.length).toBeGreaterThan(0); + const restored = resolveRelayoutScrollTop(before, resized, scrollTop); + const materialized = buildDiffDocumentModel({ + ...resizedOptions, + materializationWindow: { top: restored - 800, height: 2000 }, + materializationAnchor: captureScrollAnchor(resized, restored), + reuseFrom: [resized], + }); + const nextScrollTop = resolveRelayoutScrollTop(resized, materialized, restored); + const nextAnchor = captureScrollAnchor(materialized, nextScrollTop); + // Offset 90 belongs to the fragment beginning at 78 after narrowing. + expect(nextAnchor).toMatchObject({ kind: "text", cellOffset: 78, viewportDelta: -2 }); + expect(nextScrollTop).toBe(resolveScrollAnchor(materialized, anchor)); + }); + + it("materializes wrapped text only inside the window", () => { const model = buildDiffDocumentModel( input({ files: [file("a.ts"), file("b.ts")], wrapLines: true, - materializationWindow: { top: 0, height: FILE_HEADER_HEIGHT }, + materializationWindow: { top: FILE_HEADER_HEIGHT, height: 1 }, }), ); expect(measuredFragments(model, "a.ts")).toBeGreaterThan(0); - expect(measuredFragments(model, "b.ts")).toBeGreaterThan(0); + expect(measuredFragments(model, "b.ts")).toBe(0); + expect( + model.rows.filter((row) => row.kind === "line" && row.cells.some(hasFragments)), + ).toHaveLength(1); }); }); @@ -798,3 +900,7 @@ function reviewActionsWithEditor( onDeleteComment() {}, }; } + +function hasFragments(cell: DiffCell | null): boolean { + return cell !== null && cell.fragments.length > 0; +} diff --git a/packages/app/src/git/diff-document/model.ts b/packages/app/src/git/diff-document/model.ts index fb2b9a04f0..67efe06c2b 100644 --- a/packages/app/src/git/diff-document/model.ts +++ b/packages/app/src/git/diff-document/model.ts @@ -154,27 +154,38 @@ function appendReusableFileRows(candidate: { }): FileRowsResult { const reusedRows = candidate.model.rows.slice(candidate.file.rowStart, candidate.file.rowEnd); const retainsRowGeometry = - !candidate.input.wrapLines && candidate.model.reviewGeometryKey === candidate.reviewGeometryKey && candidate.file.top === candidate.fileTop && candidate.file.fileIndex === candidate.fileIndex && candidate.file.rowStart === candidate.rows.length; let top = candidate.bodyTop; for (const reusedRow of reusedRows) { - let nextRow = retainsRowGeometry ? reusedRow : repositionReusedRow(reusedRow, candidate, top); + let nextRow = + retainsRowGeometry && reusedRow.top === top + ? reusedRow + : repositionReusedRow(reusedRow, candidate, top); if ( nextRow.kind === "line" && - intersectsMaterializationWindow(candidate.input, top, top + nextRow.height) && + (intersectsMaterializationWindow(candidate.input, top, top + nextRow.height) || + containsMaterializationAnchor(candidate.input, candidate.file.path, nextRow.cells)) && nextRow.cells.some((cell) => cell && cell.fragments.length === 0) ) { + const cells = materializeCells( + nextRow.cells, + candidate.file.file, + candidate.file.gutterWidth, + candidate.input, + ); nextRow = { ...nextRow, - cells: materializeCells( - nextRow.cells, - candidate.file.file, - candidate.file.gutterWidth, - candidate.input, - ), + cells, + height: + Math.max( + candidate.input.typography.lineHeight, + ...cells.map( + (cell) => (cell?.fragments.length ?? 1) * candidate.input.typography.lineHeight, + ), + ) + nextRow.reviewHeight, }; } candidate.rows.push(nextRow); @@ -240,6 +251,7 @@ function appendNewFileRows(candidate: { const lines = geometryLines( lineSources(candidate.file, candidate.input.layout, false), candidate.input, + candidate.gutterWidth, ); const fileBottom = candidate.bodyTop + @@ -252,8 +264,8 @@ function appendNewFileRows(candidate: { let maximumHorizontalOverflow = 0; for (const line of lines) { const shouldMaterialize = - candidate.input.wrapLines || - intersectsMaterializationWindow(candidate.input, top, top + line.height); + intersectsMaterializationWindow(candidate.input, top, top + line.height) || + containsMaterializationAnchor(candidate.input, candidate.file.path, line.cells); const cells = shouldMaterialize ? materializeCells(line.cells, candidate.file, candidate.gutterWidth, candidate.input) : line.cells; @@ -275,7 +287,7 @@ function appendNewFileRows(candidate: { candidate.input.typography.lineHeight, ...cells.map((cell) => (cell?.fragments.length ?? 1) * candidate.input.typography.lineHeight), ); - const height = textHeight + line.reviewHeight; + const height = shouldMaterialize ? textHeight + line.reviewHeight : line.height; candidate.rows.push({ kind: "line", index: candidate.rows.length, @@ -412,13 +424,14 @@ function cellSource(line: SplitDiffDisplayLine | null, includeTokens: boolean): }; } -function geometryCell(source: CellSource): DiffCell { +function geometryCell(source: CellSource, estimatedWrapColumns: number): DiffCell { return { type: source.type, content: source.content, lineNumber: source.lineNumber, tokens: [], fragments: [], + estimatedWrapColumns, reviewTarget: source.reviewTarget, sourceIdentity: source.sourceIdentity, }; @@ -427,20 +440,87 @@ function geometryCell(source: CellSource): DiffCell { function geometryLines( sources: Array<[CellSource] | [CellSource | null, CellSource | null]>, input: BuildDiffDocumentModelInput, + gutterWidth: number, ): GeometryLine[] { + const characterWidth = input.wrapLines ? input.measureText.measure("M") : 0; return sources.map((sourceCells) => { + const columns = input.wrapLines + ? Math.max( + 1, + Math.floor( + (input.viewportWidth / sourceCells.length - gutterWidth - CODE_HORIZONTAL_PADDING) / + characterWidth, + ), + ) + : Infinity; const cells = sourceCells.map((source) => - source ? geometryCell(source) : null, + source ? geometryCell(source, columns) : null, ) as DiffLineRow["cells"]; const reviewHeight = reviewHeightForCells(cells, input); return { cells, reviewHeight, - height: input.typography.lineHeight + reviewHeight, + // Unseen wrapped rows use a monospace estimate; materialization replaces + // it with the exact fragment count without changing source identity. + height: + input.typography.lineHeight * + Math.max( + 1, + ...cells.map((cell) => { + return cell && input.wrapLines + ? Math.ceil(estimatedDisplayOffset(cell.content) / columns) + : 1; + }), + ) + + reviewHeight, }; }); } +// Use the same tab expansion and column count for estimated heights and anchors. +function estimatedDisplayOffset(text: string): number { + return text.length + (text.match(/\t/g)?.length ?? 0) * 3; +} + +function estimatedVisualLine(cell: DiffCell, sourceOffset: number): number { + return Math.floor( + estimatedDisplayOffset(cell.content.slice(0, sourceOffset)) / + (cell.estimatedWrapColumns ?? Infinity), + ); +} + +function estimatedFragment(cell: DiffCell, visualIndex: number, lineHeight: number) { + const columns = cell.estimatedWrapColumns ?? Infinity; + const lastIndex = Math.max(0, Math.ceil(estimatedDisplayOffset(cell.content) / columns) - 1); + const index = Math.min(visualIndex, lastIndex); + const displayOffset = index === 0 ? 0 : index * columns; + let start = Math.min(displayOffset, cell.content.length); + if (cell.content.includes("\t")) { + let offset = 0; + start = 0; + while (start < cell.content.length && offset < displayOffset) { + offset += cell.content[start] === "\t" ? 4 : 1; + start += 1; + } + } + return { start, top: index * lineHeight }; +} + +// A resize can move the anchor outside the old window. Measure that row in the +// same pass: estimated columns cannot identify exact shaped-text fragments. +function containsMaterializationAnchor( + input: BuildDiffDocumentModelInput, + path: string, + cells: DiffLineRow["cells"], +): boolean { + const anchor = input.materializationAnchor; + return ( + anchor?.kind === "text" && + anchor.path === path && + cells.some((cell) => cell && sameSourceIdentity(cell.sourceIdentity, anchor.sourceIdentity)) + ); +} + function intersectsMaterializationWindow( input: BuildDiffDocumentModelInput, fileTop: number, @@ -795,17 +875,27 @@ export function captureScrollAnchor( } const textY = Math.max(0, Math.min(scrollTop - row.top, row.height - row.reviewHeight)); const visualIndex = Math.max(0, Math.floor(textY / model.lineHeight)); - let cellIndex = row.cells.findIndex((cell) => Boolean(cell?.fragments[visualIndex])); + let cellIndex = row.cells.findIndex( + (cell) => + cell && + (cell.fragments.length > 0 + ? Boolean(cell.fragments[visualIndex]) + : estimatedFragment(cell, visualIndex, model.lineHeight).top === + visualIndex * model.lineHeight), + ); if (cellIndex < 0) cellIndex = row.cells.findIndex((cell) => cell !== null); const cell = row.cells[cellIndex]; if (!cell) return { kind: "header", path: file.path, viewportDelta: row.top - scrollTop }; - const fragment = cell.fragments[Math.min(cell.fragments.length - 1, visualIndex)]; - const anchorY = row.top + (fragment?.top ?? 0); + const fragment = + cell.fragments.length > 0 + ? cell.fragments[Math.min(cell.fragments.length - 1, visualIndex)]! + : estimatedFragment(cell, visualIndex, model.lineHeight); + const anchorY = row.top + fragment.top; return { kind: "text", path: file.path, sourceIdentity: cell.sourceIdentity, - cellOffset: fragment?.start ?? 0, + cellOffset: fragment.start, viewportDelta: anchorY - scrollTop, }; } @@ -831,8 +921,10 @@ export function resolveScrollAnchor(model: DiffDocumentModel, anchor: DiffScroll const fragment = cell.fragments.find( (candidate) => candidate.start <= anchor.cellOffset && anchor.cellOffset < candidate.end, - ) ?? cell.fragments.at(-1); - return Math.max(0, row.top + (fragment?.top ?? 0) - anchor.viewportDelta); + ) ?? + cell.fragments.at(-1) ?? + estimatedFragment(cell, estimatedVisualLine(cell, anchor.cellOffset), model.lineHeight); + return Math.max(0, row.top + fragment.top - anchor.viewportDelta); } export function resolveRelayoutScrollTop( diff --git a/packages/app/src/git/diff-document/surface.native.tsx b/packages/app/src/git/diff-document/surface.native.tsx index 2063198712..00a47437cd 100644 --- a/packages/app/src/git/diff-document/surface.native.tsx +++ b/packages/app/src/git/diff-document/surface.native.tsx @@ -38,6 +38,7 @@ import { } from "./horizontal-offsets"; import { buildDiffDocumentModel, + captureScrollAnchor, resolveRelayoutScrollTop, retainReusableModels, shouldApplyRelayoutScroll, @@ -138,6 +139,9 @@ export function DiffSurface(props: DiffSurfaceProps) { tooLarge: t("workspace.git.diff.tooLarge"), }, materializationWindow: diffMaterializationWindow(fileWindowTop, viewport.height), + materializationAnchor: previousModelRef.current + ? captureScrollAnchor(previousModelRef.current, scrollTop.get()) + : null, reuseFrom, }); reusableModelRef.current = { @@ -148,6 +152,7 @@ export function DiffSurface(props: DiffSurfaceProps) { }, [ measurement, fileWindowTop, + scrollTop, props.collapsedFilePaths, props.displayPreferences.layout, props.displayPreferences.wrapLines, diff --git a/packages/app/src/git/diff-document/surface.web.tsx b/packages/app/src/git/diff-document/surface.web.tsx index 97801b708f..393f6dfcd7 100644 --- a/packages/app/src/git/diff-document/surface.web.tsx +++ b/packages/app/src/git/diff-document/surface.web.tsx @@ -28,10 +28,20 @@ import { } from "./hit-testing"; import { retainHorizontalOffsetMapForPaths } from "./horizontal-offsets"; import { HorizontalScroll } from "./horizontal-scroll.web"; -import { buildDiffDocumentModel, FILE_HEADER_HEIGHT, resolveRelayoutScrollTop } from "./model"; +import { + buildDiffDocumentModel, + captureScrollAnchor, + FILE_HEADER_HEIGHT, + resolveRelayoutScrollTop, +} from "./model"; import { paintWebFileHeader, paintWebViewport } from "./paint.web"; import { hasPointerDragStarted } from "./pointer-gesture"; -import { createMeasuredAdvances } from "./text-measurement"; +import { + createMeasuredAdvances, + createAdditiveAdvances, + createChunkedWidthMeasurer, + requiresShaping, +} from "./text-measurement"; import { retainDiffViewport } from "./viewport"; import type { DiffHit, @@ -183,6 +193,9 @@ export function DiffSurface(props: DiffSurfaceProps) { tooLarge: t("workspace.git.diff.tooLarge"), }, materializationWindow: diffMaterializationWindow(fileWindowTop, viewport.height), + materializationAnchor: previousModelRef.current + ? captureScrollAnchor(previousModelRef.current, scrollTopRef.current) + : null, }); return next; }, [ @@ -1166,5 +1179,13 @@ function createWebTextMeasurer(typography: DiffTypography): TextMeasurer { }; // Canvas exposes no glyph coverage, so `requiresShaping` alone decides which // runs a shaper has to see -- there is no `glyphIds` to fall back on. - return { measure, measureAdvances: createMeasuredAdvances((text) => measure(text)) }; + const additive = createAdditiveAdvances(measure); + return { + measure, + measureAdvances: createMeasuredAdvances(measure), + measureWidth: createChunkedWidthMeasurer((graphemes) => { + const text = graphemes.join(""); + return requiresShaping(text) ? measure(text) : (additive(graphemes).at(-1) ?? 0); + }), + }; } diff --git a/packages/app/src/git/diff-document/types.ts b/packages/app/src/git/diff-document/types.ts index 4ac8125445..d98880eb8c 100644 --- a/packages/app/src/git/diff-document/types.ts +++ b/packages/app/src/git/diff-document/types.ts @@ -112,6 +112,8 @@ export interface DiffCell { lineNumber: number | null; tokens: DiffTokenRun[]; fragments: DiffFragment[]; + /** Monospace columns per visual line until this cell is measured. */ + estimatedWrapColumns?: number; reviewTarget: ReviewableDiffTarget | null; sourceIdentity: DiffSourceIdentity; } @@ -185,6 +187,8 @@ export interface BuildDiffDocumentModelInput { reviewActions?: InlineReviewActions; labels: { binary: string; tooLarge: string }; materializationWindow?: { top: number; height: number }; + /** Also measure the anchored row so relayout can restore its exact fragment. */ + materializationAnchor?: DiffScrollAnchor | null; /** A geometry-compatible model whose unchanged file measurements may be reused. */ reuseFrom?: readonly DiffDocumentModel[]; } diff --git a/packages/app/src/git/diff-document/workspace-cache.test.ts b/packages/app/src/git/diff-document/workspace-cache.test.ts index ddf9972a46..a264d417ca 100644 --- a/packages/app/src/git/diff-document/workspace-cache.test.ts +++ b/packages/app/src/git/diff-document/workspace-cache.test.ts @@ -95,6 +95,37 @@ describe("diff document workspace cache", () => { expect(stats.calls).toBe(0); }); + it("materializes a new anchor when the viewport window is unchanged", () => { + const cache = createDiffDocumentWorkspaceCache(); + const source = diffFile(); + source.hunks[0]!.lines = Array.from({ length: 100 }, () => ({ + type: "add" as const, + content: "x".repeat(200), + })); + const { measureText } = countingMeasurer(); + const input = modelInput([source], measureText, { + wrapLines: true, + materializationWindow: { top: 30, height: 54 }, + }); + const first = cache.buildModel(input); + const anchored = cache.buildModel({ + ...input, + materializationAnchor: { + kind: "text", + path: source.path, + sourceIdentity: { hunkIndex: 0, lineIndex: 50, side: "new" }, + cellOffset: 90, + viewportDelta: -2, + }, + }); + const firstRow = first.rows[50]!; + const anchoredRow = anchored.rows[50]!; + expect(firstRow.kind === "line" && firstRow.cells[0]!.fragments.length).toBe(0); + expect(anchoredRow.kind === "line" && anchoredRow.cells[0]!.fragments.length).toBeGreaterThan( + 0, + ); + }); + it("retains text cells for unchanged files when another file changes", () => { const cache = createDiffDocumentWorkspaceCache(); const { measureText } = countingMeasurer(); diff --git a/packages/app/src/git/diff-document/workspace-cache.tsx b/packages/app/src/git/diff-document/workspace-cache.tsx index 5b9921e31d..dac4d96c2e 100644 --- a/packages/app/src/git/diff-document/workspace-cache.tsx +++ b/packages/app/src/git/diff-document/workspace-cache.tsx @@ -128,7 +128,9 @@ function exactModelKey(input: Omit): s const materializationWindow = input.materializationWindow ? [input.materializationWindow.top, input.materializationWindow.height] : null; - return JSON.stringify([collapsedFilePaths, reviewGeometry, materializationWindow]); + const anchor = input.materializationAnchor; + const anchoredRow = anchor?.kind === "text" ? [anchor.path, anchor.sourceIdentity] : null; + return JSON.stringify([collapsedFilePaths, reviewGeometry, materializationWindow, anchoredRow]); } function typographyKey(typography: DiffTypography): string {