From 2628f112d12f513bb91863cbf65ff6f6f2da78e4 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Tue, 20 Oct 2026 15:51:13 -0700 Subject: [PATCH] fix(session-ui): reuse Pierre file views, window medium files, cache parsed diffs --- .../component-tests/file-viewer.fixture.tsx | 76 +++++++++ .../component-tests/file-viewer.spec.ts | 52 ++++++ packages/session-ui/src/components/file.tsx | 151 ++++++++++++------ .../src/components/session-diff.test.ts | 24 +++ .../session-ui/src/components/session-diff.ts | 49 +++--- packages/session-ui/src/pierre/virtualizer.ts | 4 + ...-review-file-preview-v2-virtualize.test.ts | 14 +- ...ssion-review-file-preview-v2-virtualize.ts | 4 +- 8 files changed, 300 insertions(+), 74 deletions(-) create mode 100644 packages/session-ui/component-tests/file-viewer.fixture.tsx create mode 100644 packages/session-ui/component-tests/file-viewer.spec.ts diff --git a/packages/session-ui/component-tests/file-viewer.fixture.tsx b/packages/session-ui/component-tests/file-viewer.fixture.tsx new file mode 100644 index 000000000000..1b9ebff40054 --- /dev/null +++ b/packages/session-ui/component-tests/file-viewer.fixture.tsx @@ -0,0 +1,76 @@ +import { render } from "solid-js/web" +import { createSignal } from "solid-js" +import type { SelectedLineRange } from "@pierre/diffs" +import { File } from "../src/components/file" +import { normalize } from "../src/components/session-diff" + +const source = (lines: number, revision = 0) => + Array.from( + { length: lines }, + (_, index) => `const value${index} = ${index % 10 === 0 ? index + revision : index}\n`, + ).join("") + +const [overflow, setOverflow] = createSignal<"wrap" | "scroll">("wrap") + +const [contents, setContents] = createSignal("") + +const [selected, setSelected] = createSignal(null) + +const [commented, setCommented] = createSignal([]) + +let first: Element | null = null + +export const controls = { + setOverflow, + setSelected, + setCommented, + append: (line: string) => setContents((value) => value + line), + // Keep the first Pierre host so a later check can tell an in-place update from a rebuild. + keepHost: () => (first = document.querySelector("[data-testid='file-viewer-fixture'] diffs-container")), + sameHost: () => + first !== null && first === document.querySelector("[data-testid='file-viewer-fixture'] diffs-container"), +} + +function host() { + const element = document.createElement("div") + element.dataset.testid = "file-viewer-fixture" + element.style.cssText = + "position: fixed; inset: 0 auto auto 0; z-index: 1; width: 800px; height: 400px; overflow: auto" + document.body.appendChild(element) + + return element +} + +export function mountTextViewer(lines: number) { + setContents(source(lines)) + render( + () => ( + + ), + host(), + ) +} + +export function mountDiffViewer(lines: number) { + const view = normalize({ file: "a.ts", before: source(lines), after: source(lines, 1), additions: 0, deletions: 0 }) + render( + () => ( + + ), + host(), + ) +} diff --git a/packages/session-ui/component-tests/file-viewer.spec.ts b/packages/session-ui/component-tests/file-viewer.spec.ts new file mode 100644 index 000000000000..3d0d3803d15b --- /dev/null +++ b/packages/session-ui/component-tests/file-viewer.spec.ts @@ -0,0 +1,52 @@ +import { expect, sourceURL, story } from "../../storybook/playwright/story" + +const fixture = sourceURL(new URL("./file-viewer.fixture.tsx", import.meta.url)) + +story("updates a text file in place when its props change", async ({ mount, page }) => { + await mount("components-markdown--compact-result") + await page.evaluate(async (fixture) => (await import(fixture)).mountTextViewer(40), fixture) + const root = page.getByTestId("file-viewer-fixture") + await expect(root.locator('[data-line="40"]')).toHaveCount(1) + await page.evaluate(async (fixture) => (await import(fixture)).controls.keepHost(), fixture) + + await page.evaluate(async (fixture) => (await import(fixture)).controls.setOverflow("scroll"), fixture) + await expect(root.locator('pre[data-overflow="scroll"]')).toHaveCount(1) + await page.evaluate(async (fixture) => (await import(fixture)).controls.append("const appended = true\n"), fixture) + await expect(root.locator('[data-line="41"]')).toContainText("appended") + + expect(await page.evaluate(async (fixture) => (await import(fixture)).controls.sameHost(), fixture)).toBe(true) +}) + +story("windows a medium text file and keeps selection and comment marks", async ({ mount, page }) => { + await mount("components-markdown--compact-result") + await page.evaluate(async (fixture) => (await import(fixture)).mountTextViewer(480), fixture) + const root = page.getByTestId("file-viewer-fixture") + await expect(root.locator('[data-line="1"]')).toBeInViewport() + expect(await root.locator("[data-line]").count()).toBeLessThan(200) + + await page.evaluate(async (fixture) => { + const { controls } = await import(fixture) + controls.setSelected({ start: 3, end: 5 }) + controls.setCommented([{ start: 8, end: 8 }]) + }, fixture) + await expect(root.locator("[data-line][data-selected-line]")).toHaveCount(3) + await expect(root.locator('[data-line="8"][data-comment-selected]')).toHaveCount(1) + + // File links jump by setting the scroll offset from the line number; the window must follow it. + await root.evaluate((element) => (element.scrollTop = (400 - 4) * 24)) + await expect(root.locator('[data-line="400"]')).toBeInViewport() +}) + +story("scrolls a virtualized diff to an off-screen selection", async ({ mount, page }) => { + await mount("components-markdown--compact-result") + await page.evaluate(async (fixture) => (await import(fixture)).mountDiffViewer(480), fixture) + const root = page.getByTestId("file-viewer-fixture") + await expect(root.locator('[data-line="1"]').first()).toBeVisible() + await expect(root.locator('[data-line="400"]')).toHaveCount(0) + + await page.evaluate( + async (fixture) => (await import(fixture)).controls.setSelected({ start: 400, end: 400, side: "additions" }), + fixture, + ) + await expect(root.locator('[data-line="400"][data-selected-line]').first()).toBeInViewport() +}) diff --git a/packages/session-ui/src/components/file.tsx b/packages/session-ui/src/components/file.tsx index 94e98039f986..1d38682a12b6 100644 --- a/packages/session-ui/src/components/file.tsx +++ b/packages/session-ui/src/components/file.tsx @@ -20,7 +20,17 @@ import { import { type PreloadFileDiffResult, type PreloadMultiFileDiffResult } from "@pierre/diffs/ssr" import { createMediaQuery } from "@solid-primitives/media" import { makeEventListener } from "@solid-primitives/event-listener" -import { ComponentProps, createEffect, createMemo, createSignal, onCleanup, onMount, Show, splitProps } from "solid-js" +import { + ComponentProps, + createEffect, + createMemo, + createSignal, + on, + onCleanup, + onMount, + Show, + splitProps, +} from "solid-js" import { createDefaultOptions, styleVariables } from "../pierre" import { markCommentedDiffLines, markCommentedFileLines } from "../pierre/commented-lines" import { fixDiffSelection, findDiffSide, type DiffSelectionSide } from "../pierre/diff-selection" @@ -42,13 +52,11 @@ import { readShadowLineSelection, } from "../pierre/file-selection" import { createLineNumberSelectionBridge, restoreShadowTextSelection } from "../pierre/selection-bridge" -import { acquireVirtualizer, virtualMetrics } from "../pierre/virtualizer" +import { acquireVirtualizer, virtualizeLineThreshold, virtualMetrics } from "../pierre/virtualizer" import { getWorkerPool } from "../pierre/worker" import { FileMedia, type FileMediaOptions } from "./file-media" import { FileSearchBar } from "./file-search" -const VIRTUALIZE_BYTES = 500_000 - const codeMetrics = { ...DEFAULT_VIRTUAL_FILE_METRICS, lineHeight: 24, @@ -687,6 +695,22 @@ function createSharedVirtualStrategy(host: () => HTMLDivElement | undefined, ena } } +// Pierre draws nothing until its worker pool initializes, and a virtualizer that first sees an empty file at the top +// of its scroller anchors the file's bottom edge, then scrolls to the end once the rows arrive. +function createPoolReady() { + const [settled, setSettled] = createSignal(false) + + return (pool: ReturnType) => { + if (!pool || pool.isInitialized() || settled()) return true + void pool + .initialize() + .catch(() => undefined) + .then(() => setSettled(true)) + + return false + } +} + function parseLine(node: HTMLElement) { if (!node.dataset.line) return const value = parseInt(node.dataset.line, 10) @@ -797,52 +821,22 @@ function ViewerShell(props: { function TextViewer(props: TextFileProps) { let instance: PierreFile | VirtualizedFile | undefined + let instanceVirtualizer: Virtualizer | undefined let viewer!: Viewer const [local, others] = splitProps(props, textKeys) - const text = () => { - const value = local.file.contents as unknown - - if (typeof value === "string") return value - - if (Array.isArray(value)) return value.join("\n") - - if (value == null) return "" - - // oxlint-disable-next-line no-base-to-string -- file contents cast to unknown, coercion is intentional - return String(value) - } - const lineCount = () => { - const value = text() + const value = local.file.contents const total = value.split("\n").length - (value.endsWith("\n") ? 1 : 0) return Math.max(1, total) } - const bytes = createMemo(() => { - const value = local.file.contents as unknown - - if (typeof value === "string") return value.length - - if (Array.isArray(value)) { - return value.reduce( - // oxlint-disable-next-line no-base-to-string -- array parts coerced intentionally - (sum, part) => sum + (typeof part === "string" ? part.length + 1 : String(part).length + 1), - 0, - ) - } - - if (value == null) return 0 - - // oxlint-disable-next-line no-base-to-string -- file contents cast to unknown, coercion is intentional - return String(value).length - }) - - const virtual = createMemo(() => bytes() > VIRTUALIZE_BYTES) + const virtual = createMemo(() => lineCount() > virtualizeLineThreshold) const virtuals = createLocalVirtualStrategy(() => viewer.wrapper, virtual) + const poolReady = createPoolReady() const lineFromMouseEvent = (event: MouseEvent): MouseHit => mouseHit(event, parseLine) @@ -995,21 +989,28 @@ function TextViewer(props: TextFileProps) { const workerPool = getWorkerPool() const virtualizer = virtuals.get() + if (virtualizer && !poolReady(workerPool)) return + // An instance keeps the virtualizer it was built with; options and contents update in place. + const reset = instance !== undefined && instanceVirtualizer !== virtualizer + const forceRender = !reset && instance !== undefined && !areOptionsEqual(instance.options, opts) + renderViewer({ viewer, current: instance, - reset: instance !== undefined, + reset, create: () => virtualizer ? new VirtualizedFile(opts, virtualizer, codeMetrics, workerPool) : new PierreFile(opts, workerPool), + update: (value) => value.setOptions(opts), assign: (value) => { instance = value + instanceVirtualizer = virtualizer }, draw: (value) => { - const contents = text() value.render({ - file: typeof local.file.contents === "string" ? local.file : { ...local.file, contents }, + file: local.file, + forceRender, lineAnnotations: [], containerWrapper: viewer.container, }) @@ -1021,7 +1022,7 @@ function TextViewer(props: TextFileProps) { useAnnotationRerender>({ viewer, current: () => instance, - annotations: () => (local.annotations as LineAnnotation[] | undefined) ?? [], + annotations: () => local.annotations ?? [], }) // -- cleanup -- @@ -1029,6 +1030,7 @@ function TextViewer(props: TextFileProps) { onCleanup(() => { instance?.cleanUp() instance = undefined + instanceVirtualizer = undefined virtuals.cleanup() }) @@ -1057,12 +1059,19 @@ function DiffViewer(props: DiffFileProps) { const lineFromMouseEvent = (event: MouseEvent): MouseHit => mouseHit(event, findDiffLineNumber, diffMouseSide) + // Rows outside a virtualized window are not in the DOM yet; keep the range so Pierre applies it when they render. + const fixSelection = (range: SelectedLineRange | null) => { + const fixed = fixDiffSelection(viewer.getRoot(), range) + + return fixed === null && instance instanceof VirtualizedFileDiff ? range : fixed + } + const setSelectedLines = (range: SelectedLineRange | null, preserve?: { root: ShadowRoot; text: Range }) => { const active = instance if (!active) return - const fixed = fixDiffSelection(viewer.getRoot(), range) + const fixed = fixSelection(range) if (fixed === undefined) { viewer.lastSelection = range @@ -1147,6 +1156,8 @@ function DiffViewer(props: DiffFileProps) { () => local.virtualize !== false, ) + const poolReady = createPoolReady() + const large = createMemo(() => { if (local.fileDiff) { const before = local.fileDiff.deletionLines.join("") @@ -1155,8 +1166,8 @@ function DiffViewer(props: DiffFileProps) { return Math.max(before.length, after.length) > 500_000 } - const before = typeof local.before?.contents === "string" ? local.before.contents : "" - const after = typeof local.after?.contents === "string" ? local.after.contents : "" + const before = local.before?.contents ?? "" + const after = local.after?.contents ?? "" return Math.max(before.length, after.length) > 500_000 }) @@ -1169,7 +1180,7 @@ function DiffViewer(props: DiffFileProps) { const lineCallbacks = createLineCallbacks({ viewer, - normalize: (range) => fixDiffSelection(viewer.getRoot(), range), + normalize: fixSelection, onLineSelected: (range) => local.onLineSelected?.(range), onLineSelectionEnd: (range) => local.onLineSelectionEnd?.(range), onLineNumberSelectionEnd: (range) => local.onLineNumberSelectionEnd?.(range), @@ -1189,6 +1200,45 @@ function DiffViewer(props: DiffFileProps) { return { ...perf, disableLineNumbers: props.disableLineNumbers ?? true } }) + // A virtualized diff only renders rows near the viewport, so jumping to a selection must scroll it into range first. + let pendingReveal: SelectedLineRange | undefined + + const revealSelection = () => { + const active = instance + const range = pendingReveal + + if (!active || !range) return + + if (!(active instanceof VirtualizedFileDiff) || !instanceVirtualizer) { + pendingReveal = undefined + + return + } + + const position = active.getLinePosition(Math.min(range.start, range.end), range.side) + const root = instanceVirtualizer.getRoot() + + // Keep the request until the diff has a layout to measure; the next render retries it. + if (!position || !root) return + pendingReveal = undefined + const top = instanceVirtualizer.getOffsetInScrollContainer(viewer.container) + position.top + const scrollTop = instanceVirtualizer.getScrollTop() + const height = root instanceof Document ? window.innerHeight : root.clientHeight + + if (top >= scrollTop && top + position.height <= scrollTop + height) return + instanceVirtualizer.scrollTo({ top: top - height / 2 }) + } + + createEffect( + on( + () => local.selectedLines, + (range) => { + pendingReveal = range ?? undefined + revealSelection() + }, + ), + ) + const notify = (done?: VoidFunction) => { notifyRendered({ viewer, @@ -1197,6 +1247,7 @@ function DiffViewer(props: DiffFileProps) { onReady: () => { done?.() setSelectedLines(viewer.lastSelection) + revealSelection() viewer.find.refresh({ reset: true }) local.onRendered?.() }, @@ -1215,8 +1266,10 @@ function DiffViewer(props: DiffFileProps) { // Worker render options override per-viewer options, including the large-file fallback. const workerPool = getWorkerPool(large() ? "none" : "word-line") const virtualizer = virtuals.get() - const beforeContents = typeof local.before?.contents === "string" ? local.before.contents : "" - const afterContents = typeof local.after?.contents === "string" ? local.after.contents : "" + + if (virtualizer && !poolReady(workerPool)) return + const beforeContents = local.before?.contents ?? "" + const afterContents = local.after?.contents ?? "" const done = preserve(viewer) onCleanup(done) @@ -1303,7 +1356,7 @@ function DiffViewer(props: DiffFileProps) { useAnnotationRerender>({ viewer, current: () => instance, - annotations: () => (local.annotations as DiffLineAnnotation[] | undefined) ?? [], + annotations: () => local.annotations ?? [], }) // -- cleanup -- diff --git a/packages/session-ui/src/components/session-diff.test.ts b/packages/session-ui/src/components/session-diff.test.ts index 675506353257..6ef935e5a655 100644 --- a/packages/session-ui/src/components/session-diff.test.ts +++ b/packages/session-ui/src/components/session-diff.test.ts @@ -208,6 +208,30 @@ describe("session diff", () => { expect(normalize({ ...diff, after: "three\n" }).fileDiff.cacheKey).not.toBe(first.cacheKey) }) + test("reuses the parsed content diff until its inputs change", () => { + const diff = { file: "cached.ts", before: "one\n", after: "two\n", additions: 1, deletions: 1 } + const first = normalize(diff).fileDiff + + expect(normalize({ ...diff }).fileDiff).toBe(first) + + const changed = normalize({ ...diff, after: "three\n" }).fileDiff + expect(changed).not.toBe(first) + expect(changed.additionLines).toEqual(["three\n"]) + expect(normalize({ ...diff, file: "cached.py" }).fileDiff).not.toBe(first) + }) + + test("evicts the oldest parsed content diff once the cache is full", () => { + const diff = { file: "evicted-content.ts", before: "one\n", after: "two\n", additions: 1, deletions: 1 } + const first = normalize(diff).fileDiff + + Array.from({ length: 20 }, (_, index) => normalize({ ...diff, after: `two${index}\n` })) + + const restored = normalize(diff).fileDiff + expect(restored).not.toBe(first) + expect(restored.additionLines).toEqual(first.additionLines) + expect(restored.cacheKey).toBe(first.cacheKey) + }) + test("does not reuse an evicted highlight identity for different content", () => { const patch = "@@ -1 +1 @@\n-old\n+new\n" const first = resolveFileDiff({ file: "evicted.ts", patch }) diff --git a/packages/session-ui/src/components/session-diff.ts b/packages/session-ui/src/components/session-diff.ts index abd0ba4d32ac..b9a042594b25 100644 --- a/packages/session-ui/src/components/session-diff.ts +++ b/packages/session-ui/src/components/session-diff.ts @@ -28,7 +28,7 @@ export type ViewDiff = { const diffCacheLimit = 16 -const patchFileDiffCache = new Map() +const fileDiffCache = new Map() export function resolveFileDiff(diff: DiffSource) { // Untyped persisted payloads may carry `patch: null`; those render from their contents. @@ -55,27 +55,36 @@ export function text(diff: ViewDiff, side: "deletions" | "additions") { function fileDiffFromPatch(file: string, patch: string) { const key = `${file}\0${patch}` - const hit = patchFileDiffCache.get(key) + + return cachedFileDiff(`patch\0${key}`, () => { + const complete = completePatch(patch) + const input = complete ? undefined : patchInput(file, patch) + + const value = complete + ? fileDiffFromCompletePatch(file, complete) + : ((input ? parsePatchFiles(input)[0]?.files[0] : undefined) ?? emptyFileDiff(file)) + + // The patch text fixes both the content and the producer's alignment, so equal keys always mean equal hunks. + value.cacheKey = highlightKey(key) + + return value + }) +} + +function cachedFileDiff(key: string, parse: () => FileDiffMetadata) { + const hit = fileDiffCache.get(key) if (hit) { - patchFileDiffCache.delete(key) - patchFileDiffCache.set(key, hit) + fileDiffCache.delete(key) + fileDiffCache.set(key, hit) return hit } - const complete = completePatch(patch) - const input = complete ? undefined : patchInput(file, patch) - - const value = complete - ? fileDiffFromCompletePatch(file, complete) - : ((input ? parsePatchFiles(input)[0]?.files[0] : undefined) ?? emptyFileDiff(file)) - - // The patch text fixes both the content and the producer's alignment, so equal keys always mean equal hunks. - value.cacheKey = highlightKey(key) - patchFileDiffCache.set(key, value) + const value = parse() + fileDiffCache.set(key, value) - while (patchFileDiffCache.size > diffCacheLimit) patchFileDiffCache.delete(patchFileDiffCache.keys().next().value!) + while (fileDiffCache.size > diffCacheLimit) fileDiffCache.delete(fileDiffCache.keys().next().value!) return value } @@ -255,10 +264,14 @@ function patchInput(file: string, patch: string) { function fileDiffFromContent(file: string, before: string, after: string) { if (!before && !after) return emptyFileDiff(file) - const value = parseDiffFromFile({ name: file, contents: before }, { name: file, contents: after }) - value.cacheKey = highlightKey(`${file}\0${before}\0${after}`) + const key = `${file}\0${before}\0${after}` - return value + return cachedFileDiff(`content\0${key}`, () => { + const value = parseDiffFromFile({ name: file, contents: before }, { name: file, contents: after }) + value.cacheKey = highlightKey(key) + + return value + }) } // Pierre reuses and dedups worker highlighting only for diffs that carry a cacheKey, and it treats diffs with equal diff --git a/packages/session-ui/src/pierre/virtualizer.ts b/packages/session-ui/src/pierre/virtualizer.ts index b8661e6b7178..bfa99face0ed 100644 --- a/packages/session-ui/src/pierre/virtualizer.ts +++ b/packages/session-ui/src/pierre/virtualizer.ts @@ -13,6 +13,10 @@ type Entry = { const cache = new WeakMap() +// Pierre keeps the viewport plus 1,000px of overscan on each side (about 100 rows) mounted; measured, windowing +// breaks even on main-thread render time near 200 lines and wins on DOM size and render time beyond it. +export const virtualizeLineThreshold = 200 + export const virtualMetrics: Partial = { lineHeight: 24, hunkSeparatorHeight: 24, diff --git a/packages/session-ui/src/v2/components/session-review-file-preview-v2-virtualize.test.ts b/packages/session-ui/src/v2/components/session-review-file-preview-v2-virtualize.test.ts index 3bbf173c8bbd..d3bdbcd97a88 100644 --- a/packages/session-ui/src/v2/components/session-review-file-preview-v2-virtualize.test.ts +++ b/packages/session-ui/src/v2/components/session-review-file-preview-v2-virtualize.test.ts @@ -1,13 +1,17 @@ import { describe, expect, test } from "bun:test" +import { virtualizeLineThreshold } from "../../pierre/virtualizer" import { shouldVirtualizeReviewDiff } from "./session-review-file-preview-v2-virtualize" describe("shouldVirtualizeReviewDiff", () => { - test("renders small diffs directly", () => { - expect(shouldVirtualizeReviewDiff({ additionLines: 500, deletionLines: 500 })).toBe(false) + test("renders short diffs directly", () => { + expect( + shouldVirtualizeReviewDiff({ additionLines: virtualizeLineThreshold, deletionLines: virtualizeLineThreshold }), + ).toBe(false) }) - test("virtualizes large diffs", () => { - expect(shouldVirtualizeReviewDiff({ additionLines: 501, deletionLines: 1 })).toBe(true) - expect(shouldVirtualizeReviewDiff({ additionLines: 1, deletionLines: 501 })).toBe(true) + test("virtualizes medium and large diffs", () => { + expect(shouldVirtualizeReviewDiff({ additionLines: virtualizeLineThreshold + 1, deletionLines: 1 })).toBe(true) + expect(shouldVirtualizeReviewDiff({ additionLines: 1, deletionLines: virtualizeLineThreshold + 1 })).toBe(true) + expect(shouldVirtualizeReviewDiff({ additionLines: 480, deletionLines: 480 })).toBe(true) }) }) diff --git a/packages/session-ui/src/v2/components/session-review-file-preview-v2-virtualize.ts b/packages/session-ui/src/v2/components/session-review-file-preview-v2-virtualize.ts index 7795a859619f..86595f639ce8 100644 --- a/packages/session-ui/src/v2/components/session-review-file-preview-v2-virtualize.ts +++ b/packages/session-ui/src/v2/components/session-review-file-preview-v2-virtualize.ts @@ -1,5 +1,5 @@ -const lineThreshold = 500 +import { virtualizeLineThreshold } from "../../pierre/virtualizer" export function shouldVirtualizeReviewDiff(input: { additionLines: number; deletionLines: number }) { - return Math.max(input.additionLines, input.deletionLines) > lineThreshold + return Math.max(input.additionLines, input.deletionLines) > virtualizeLineThreshold }