From 8e2e4d8c384027fafaa726570b7afb5c609d114c Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Fri, 30 Oct 2026 14:18:40 -0700 Subject: [PATCH] perf(web): the pull request code tab derives its file model once per diff load Every render of the code tab hashed every loaded patch, reordered every file by its imports, recounted every file's lines, and scanned every review thread for every file. On a large pull request that is about two seconds a minute of renders, though the files only change when a diff page loads. The model is now kept per loaded slice list, and threads are grouped by path once per thread list. Co-Authored-By: Claude Opus 5.5 --- .../pullRequest/PullRequestCodeTab.tsx | 116 ++++++++++++------ 1 file changed, 79 insertions(+), 37 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 94f4ccda82558..9f97243f88a8f 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -175,6 +175,76 @@ function parseSlice(slice: DiffSlice, cacheKey: string, ignoreWhitespace: boolea return parsed; } +type DiffModel = { + readonly parsedSlices: ReadonlyArray; + readonly loadedFiles: ReadonlyArray["files"][number]>; + readonly sourceFileStats: ReadonlyMap>; +}; +const diffModels = new WeakMap, Map>(); +const patchHashes = new WeakMap(); + +// Hashing, ordering and counting every file of a huge pull request costs a second, and the tab +// renders far more often than its slices change, so the model is kept per slice list. +function diffModel( + slices: ReadonlyArray, + keyPrefix: string, + ignoreWhitespace: boolean, +): DiffModel { + let models = diffModels.get(slices); + if (models === undefined) diffModels.set(slices, (models = new Map())); + const cached = models.get(keyPrefix); + if (cached !== undefined) return cached; + const parsedSlices = slices.map((slice) => { + let hash = patchHashes.get(slice); + if (hash === undefined) patchHashes.set(slice, (hash = fnv1a32(slice.patch))); + // The cache key carries the theme, which the tokenizer caches against, and the patch's + // hash: a refreshed page reuses its cursor, so position alone would return a stale parse. + return parseSlice(slice, `${keyPrefix}:${slice.cursor ?? "first"}:${hash}`, ignoreWhitespace); + }); + const model: DiffModel = { + parsedSlices, + loadedFiles: orderDiffFilesLikeTree( + parsedSlices.flatMap((parsed) => + parsed?.kind === "files" ? orderDiffFiles(parsed.files) : [], + ), + ), + sourceFileStats: new Map( + parsedSlices.flatMap((parsed) => + parsed?.kind === "files" + ? parsed.files.map( + (file, index) => + [ + buildFileDiffRenderKey(file), + getDiffLineStat([parsed.sourceFiles[index] ?? file]), + ] as const, + ) + : [], + ), + ), + }; + models.set(keyPrefix, model); + return model; +} + +const threadsByPathCache = new WeakMap< + ReadonlyArray, + ReadonlyMap> +>(); + +function reviewThreadsByPath(threads: ReadonlyArray) { + let byPath = threadsByPathCache.get(threads); + if (byPath === undefined) { + const grouped = new Map(); + for (const thread of threads) { + const list = grouped.get(thread.path); + if (list) list.push(thread); + else grouped.set(thread.path, [thread]); + } + threadsByPathCache.set(threads, (byPath = grouped)); + } + return byPath; +} + interface FileViewedToggleState { readonly enabled: boolean; readonly isViewed: (path: string) => boolean; @@ -490,20 +560,10 @@ function PullRequestCodeTab({ // Agent context is local, so it does not require host comment permission. const canSelectLines = commit === null && (review.inlineComment || onAddToAgentSelection !== undefined); - // The cache key carries the theme, which is what the tokenizer caches against, so a theme - // change is still a fresh parse. The patch's own hash is part of it too: a refreshed page - // reuses its cursor, and a key of position alone would hand back the parse it replaced. - const parsedSlices = loadedSlices.map((slice) => - parseSlice( - slice, - `pull-request:${scopeKey}:${resolvedTheme}:${ignoreWhitespace}:${slice.cursor ?? "first"}:${fnv1a32(slice.patch)}`, - ignoreWhitespace, - ), - ); - const loadedFiles = orderDiffFilesLikeTree( - parsedSlices.flatMap((parsed) => - parsed?.kind === "files" ? orderDiffFiles(parsed.files) : [], - ), + const { parsedSlices, loadedFiles, sourceFileStats } = diffModel( + loadedSlices, + `pull-request:${scopeKey}:${resolvedTheme}:${ignoreWhitespace}`, + ignoreWhitespace, ); const testFileCount = loadedFiles.filter((file) => isTestChangedFile(resolveFileDiffPath(file)), @@ -522,33 +582,16 @@ function PullRequestCodeTab({ slice.omittedFileStats.map((file) => [file.path, file] as const), ), ); - const sourceFileStats = new Map( - parsedSlices.flatMap((parsed) => - parsed?.kind === "files" - ? parsed.files.map( - (file, index) => - [ - buildFileDiffRenderKey(file), - getDiffLineStat([parsed.sourceFiles[index] ?? file]), - ] as const, - ) - : [], - ), - ); const fileTreeEntries = diffFileTreeEntries(files); + const threadsByPath = reviewThreadsByPath(detail.reviewThreads); // File membership does not guarantee a rendered hunk for the thread. const placedThreadIds = (() => { const placed = new Set(); // Commit diffs use different line coordinates; list conversations separately. if (commit !== null) return placed; for (const file of files) { - const path = resolveFileDiffPath(file); - for (const thread of detail.reviewThreads) { - if ( - thread.path === path && - thread.line !== null && - isLineInFileDiff(file, thread.side, thread.line) - ) { + for (const thread of threadsByPath.get(resolveFileDiffPath(file)) ?? []) { + if (thread.line !== null && isLineInFileDiff(file, thread.side, thread.line)) { placed.add(thread.id); } } @@ -575,9 +618,8 @@ function PullRequestCodeTab({ return created; }; - for (const thread of detail.reviewThreads) { - if (thread.path !== path || thread.line === null) continue; - if (!placedThreadIds.has(thread.id)) continue; + for (const thread of threadsByPath.get(path) ?? []) { + if (thread.line === null || !placedThreadIds.has(thread.id)) continue; groupAt(thread.side, thread.line).threads.push(thread); } if (commit === null) {