diff --git a/src/utils/displayLines.test.ts b/src/utils/displayLines.test.ts new file mode 100644 index 0000000..c90a0bd --- /dev/null +++ b/src/utils/displayLines.test.ts @@ -0,0 +1,56 @@ +import type { Difference } from "@aws-sdk/client-codecommit"; +import { describe, expect, it } from "vitest"; +import type { CommentThread, ReactionsByComment } from "../services/codecommit.js"; +import { buildDisplayLines } from "./displayLines.js"; + +const NO_REACTIONS: ReactionsByComment = new Map(); + +function makeDiff(): { + differences: Difference[]; + diffTexts: Map; + diffTextStatus: Map; + blobKey: string; +} { + const differences: Difference[] = [ + { + beforeBlob: { blobId: "b1", path: "src/a.ts" }, + afterBlob: { blobId: "a1", path: "src/a.ts" }, + changeType: "M", + }, + ]; + const blobKey = "b1:a1"; + const diffTexts = new Map([[blobKey, { before: "old1\nold2", after: "new1\nnew2" }]]); + const diffTextStatus = new Map([[blobKey, "loaded"]]); + return { differences, diffTexts, diffTextStatus, blobKey }; +} + +describe("buildDisplayLines inline threads", () => { + it("anchors BEFORE-version threads and renders multiple threads on the same line", () => { + const { differences, diffTexts, diffTextStatus } = makeDiff(); + // Two threads anchored to the same BEFORE line (filePosition 1) on the same file. + const commentThreads: CommentThread[] = [ + { + location: { filePath: "src/a.ts", filePosition: 1, relativeFileVersion: "BEFORE" }, + comments: [{ commentId: "c1", content: "first", authorArn: "arn:aws:iam::1:user/alice" }], + }, + { + location: { filePath: "src/a.ts", filePosition: 1, relativeFileVersion: "BEFORE" }, + comments: [{ commentId: "c2", content: "second", authorArn: "arn:aws:iam::1:user/bob" }], + }, + ]; + + const lines = buildDisplayLines( + differences, + diffTexts, + diffTextStatus, + new Map(), + commentThreads, + new Map(), + NO_REACTIONS, + ); + + const inlineTexts = lines.filter((l) => l.type === "inline-comment").map((l) => l.text); + expect(inlineTexts).toContain("💬 alice: first"); + expect(inlineTexts).toContain("💬 bob: second"); + }); +}); diff --git a/src/utils/displayLines.ts b/src/utils/displayLines.ts index f39e36f..92fbdf2 100644 --- a/src/utils/displayLines.ts +++ b/src/utils/displayLines.ts @@ -136,37 +136,43 @@ function getSliceLimits(beforeCount: number, afterCount: number, totalLimit: num } /* v8 ignore stop */ +type ThreadEntry = { thread: CommentThread; index: number }; + +/** Inline threads for a single file, bucketed by line number for each version. */ +interface FileThreadIndex { + before: Map; + after: Map; +} + +/** + * Returns the inline-thread entries anchored to `line`, or undefined when none. + * Uses integer line-number lookups so the hot loop never concatenates keys. + */ function findMatchingThreadEntries( - threadsByKey: Map, - filePath: string, + fileThreads: FileThreadIndex, line: DisplayLine, -): { thread: CommentThread; index: number }[] { - const results: { thread: CommentThread; index: number }[] = []; - - if (line.type === "delete" && line.beforeLineNumber) { - const key = `${filePath}:${line.beforeLineNumber}:BEFORE`; - results.push(...(threadsByKey.get(key) ?? [])); +): ThreadEntry[] | undefined { + if (line.type === "delete") { + return line.beforeLineNumber ? fileThreads.before.get(line.beforeLineNumber) : undefined; } - if (line.type === "add" && line.afterLineNumber) { - const key = `${filePath}:${line.afterLineNumber}:AFTER`; - results.push(...(threadsByKey.get(key) ?? [])); + if (line.type === "add") { + return line.afterLineNumber ? fileThreads.after.get(line.afterLineNumber) : undefined; } /* v8 ignore start -- context lines always have both line numbers in practice */ if (line.type === "context") { - if (line.beforeLineNumber) { - const key = `${filePath}:${line.beforeLineNumber}:BEFORE`; - results.push(...(threadsByKey.get(key) ?? [])); - } - if (line.afterLineNumber) { - const key = `${filePath}:${line.afterLineNumber}:AFTER`; - results.push(...(threadsByKey.get(key) ?? [])); - } + const before = line.beforeLineNumber + ? fileThreads.before.get(line.beforeLineNumber) + : undefined; + const after = line.afterLineNumber ? fileThreads.after.get(line.afterLineNumber) : undefined; + if (before && after) return [...before, ...after]; + return before ?? after; } /* v8 ignore stop */ - return results; + /* v8 ignore next -- diff lines are only ever add/delete/context */ + return undefined; } export function buildDisplayLines( @@ -181,15 +187,25 @@ export function buildDisplayLines( ): DisplayLine[] { const lines: DisplayLine[] = []; - // Index inline comments by file:position:version for efficient lookup - const inlineThreadsByKey = new Map(); + // Index inline comments by file, then by line number per version, so the + // diff loop below can resolve anchors with integer lookups (no key strings). + const inlineThreadsByFile = new Map(); for (let i = 0; i < commentThreads.length; i++) { const thread = commentThreads[i]!; - if (thread.location) { - const key = `${thread.location.filePath}:${thread.location.filePosition}:${thread.location.relativeFileVersion}`; - const existing = inlineThreadsByKey.get(key) ?? []; + const location = thread.location; + if (!location) continue; + + let fileIndex = inlineThreadsByFile.get(location.filePath); + if (!fileIndex) { + fileIndex = { before: new Map(), after: new Map() }; + inlineThreadsByFile.set(location.filePath, fileIndex); + } + const bucket = location.relativeFileVersion === "BEFORE" ? fileIndex.before : fileIndex.after; + const existing = bucket.get(location.filePosition); + if (existing) { existing.push({ thread, index: i }); - inlineThreadsByKey.set(key, existing); + } else { + bucket.set(location.filePosition, [{ thread, index: i }]); } } @@ -220,10 +236,13 @@ export function buildDisplayLines( afterLines.length, displayLimit, ); - diffLines = computeSimpleDiff( - beforeLines.slice(0, beforeLimit), - afterLines.slice(0, afterLimit), - ); + // Skip the slice copy when the limit already covers the whole array + // (the common non-truncated case); computeSimpleDiff never mutates its inputs. + const beforeSlice = + beforeLimit === beforeLines.length ? beforeLines : beforeLines.slice(0, beforeLimit); + const afterSlice = + afterLimit === afterLines.length ? afterLines : afterLines.slice(0, afterLimit); + diffLines = computeSimpleDiff(beforeSlice, afterSlice); // Enrich once here so the hot loop below can push lines without per-call copies for (const dl of diffLines) { dl.filePath = filePath; @@ -231,12 +250,14 @@ export function buildDisplayLines( } diffCache?.set(cacheKey, diffLines); } - const hasInlineThreads = inlineThreadsByKey.size > 0; + // Only files that actually have inline threads pay the per-line lookup cost. + const fileThreads = inlineThreadsByFile.get(filePath); for (const dl of diffLines) { lines.push(dl); - if (!hasInlineThreads) continue; - const matchingEntries = findMatchingThreadEntries(inlineThreadsByKey, filePath, dl); + if (!fileThreads) continue; + const matchingEntries = findMatchingThreadEntries(fileThreads, dl); + if (!matchingEntries) continue; for (const { thread, index: threadIdx } of matchingEntries) { appendThreadLines( lines, diff --git a/src/utils/formatDiff.ts b/src/utils/formatDiff.ts index 84eda1f..aa126e4 100644 --- a/src/utils/formatDiff.ts +++ b/src/utils/formatDiff.ts @@ -51,16 +51,19 @@ function hasNearbyMatch(lines: string[], start: number, target: string): boolean */ export function computeSimpleDiff(beforeLines: string[], afterLines: string[]): DisplayLine[] { const result: DisplayLine[] = []; + // Inputs are never mutated, so hoist lengths out of the hot loop. + const blen = beforeLines.length; + const alen = afterLines.length; let bi = 0; // Index for beforeLines let ai = 0; // Index for afterLines // Process both arrays until all lines are consumed - while (bi < beforeLines.length || ai < afterLines.length) { + while (bi < blen || ai < alen) { const beforeLine = beforeLines[bi]; const afterLine = afterLines[ai]; // Case 1: Lines match at current position - add as context - if (bi < beforeLines.length && ai < afterLines.length && beforeLine === afterLine) { + if (bi < blen && ai < alen && beforeLine === afterLine) { result.push({ type: "context", text: ` ${beforeLine}`, @@ -75,10 +78,7 @@ export function computeSimpleDiff(beforeLines: string[], afterLines: string[]): const startAi = ai; // Process deletions: consume lines from 'before' that don't match current 'after' - while ( - bi < beforeLines.length && - (ai >= afterLines.length || beforeLines[bi] !== afterLines[ai]) - ) { + while (bi < blen && (ai >= alen || beforeLines[bi] !== afterLines[ai])) { const bl = beforeLines[bi]!; // Optimization: stop if this line appears within the lookahead window in 'after' if (hasNearbyMatch(afterLines, ai, bl)) break; @@ -91,10 +91,7 @@ export function computeSimpleDiff(beforeLines: string[], afterLines: string[]): } // Process additions: consume lines from 'after' that don't match current 'before' - while ( - ai < afterLines.length && - (bi >= beforeLines.length || afterLines[ai] !== beforeLines[bi]) - ) { + while (ai < alen && (bi >= blen || afterLines[ai] !== beforeLines[bi])) { const al = afterLines[ai]!; // Optimization: stop if this line appears within the lookahead window in 'before' if (hasNearbyMatch(beforeLines, bi, al)) break; @@ -109,7 +106,7 @@ export function computeSimpleDiff(beforeLines: string[], afterLines: string[]): // Safety: if both loops broke without advancing, force progress to prevent infinite loop /* v8 ignore start -- defensive guard; the greedy algorithm always advances in normal cases */ if (bi === startBi && ai === startAi) { - if (bi < beforeLines.length) { + if (bi < blen) { result.push({ type: "delete", text: `-${beforeLines[bi]}`, @@ -117,7 +114,7 @@ export function computeSimpleDiff(beforeLines: string[], afterLines: string[]): }); bi++; } - if (ai < afterLines.length) { + if (ai < alen) { result.push({ type: "add", text: `+${afterLines[ai]}`,