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) {