Skip to content

Commit dd67a72

Browse files
watany-devwatany-dev
authored
Merge pull request #99 from watany-dev/claude/benchmark-optimize-perf-pqmdlw
perf: speed up inline-comment diff rendering with integer-keyed index
2 parents b7007cb + 34d7459 commit dd67a72

3 files changed

Lines changed: 119 additions & 45 deletions

File tree

‎src/utils/displayLines.test.ts‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
import type { Difference } from "@aws-sdk/client-codecommit";
2+
import { describe, expect, it } from "vitest";
3+
import type { CommentThread, ReactionsByComment } from "../services/codecommit.js";
4+
import { buildDisplayLines } from "./displayLines.js";
5+
6+
const NO_REACTIONS: ReactionsByComment = new Map();
7+
8+
function makeDiff(): {
9+
differences: Difference[];
10+
diffTexts: Map<string, { before: string; after: string }>;
11+
diffTextStatus: Map<string, "loading" | "loaded" | "error">;
12+
blobKey: string;
13+
} {
14+
const differences: Difference[] = [
15+
{
16+
beforeBlob: { blobId: "b1", path: "src/a.ts" },
17+
afterBlob: { blobId: "a1", path: "src/a.ts" },
18+
changeType: "M",
19+
},
20+
];
21+
const blobKey = "b1:a1";
22+
const diffTexts = new Map([[blobKey, { before: "old1\nold2", after: "new1\nnew2" }]]);
23+
const diffTextStatus = new Map<string, "loading" | "loaded" | "error">([[blobKey, "loaded"]]);
24+
return { differences, diffTexts, diffTextStatus, blobKey };
25+
}
26+
27+
describe("buildDisplayLines inline threads", () => {
28+
it("anchors BEFORE-version threads and renders multiple threads on the same line", () => {
29+
const { differences, diffTexts, diffTextStatus } = makeDiff();
30+
// Two threads anchored to the same BEFORE line (filePosition 1) on the same file.
31+
const commentThreads: CommentThread[] = [
32+
{
33+
location: { filePath: "src/a.ts", filePosition: 1, relativeFileVersion: "BEFORE" },
34+
comments: [{ commentId: "c1", content: "first", authorArn: "arn:aws:iam::1:user/alice" }],
35+
},
36+
{
37+
location: { filePath: "src/a.ts", filePosition: 1, relativeFileVersion: "BEFORE" },
38+
comments: [{ commentId: "c2", content: "second", authorArn: "arn:aws:iam::1:user/bob" }],
39+
},
40+
];
41+
42+
const lines = buildDisplayLines(
43+
differences,
44+
diffTexts,
45+
diffTextStatus,
46+
new Map(),
47+
commentThreads,
48+
new Map(),
49+
NO_REACTIONS,
50+
);
51+
52+
const inlineTexts = lines.filter((l) => l.type === "inline-comment").map((l) => l.text);
53+
expect(inlineTexts).toContain("💬 alice: first");
54+
expect(inlineTexts).toContain("💬 bob: second");
55+
});
56+
});

‎src/utils/displayLines.ts‎

Lines changed: 54 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -136,37 +136,43 @@ function getSliceLimits(beforeCount: number, afterCount: number, totalLimit: num
136136
}
137137
/* v8 ignore stop */
138138

139+
type ThreadEntry = { thread: CommentThread; index: number };
140+
141+
/** Inline threads for a single file, bucketed by line number for each version. */
142+
interface FileThreadIndex {
143+
before: Map<number, ThreadEntry[]>;
144+
after: Map<number, ThreadEntry[]>;
145+
}
146+
147+
/**
148+
* Returns the inline-thread entries anchored to `line`, or undefined when none.
149+
* Uses integer line-number lookups so the hot loop never concatenates keys.
150+
*/
139151
function findMatchingThreadEntries(
140-
threadsByKey: Map<string, { thread: CommentThread; index: number }[]>,
141-
filePath: string,
152+
fileThreads: FileThreadIndex,
142153
line: DisplayLine,
143-
): { thread: CommentThread; index: number }[] {
144-
const results: { thread: CommentThread; index: number }[] = [];
145-
146-
if (line.type === "delete" && line.beforeLineNumber) {
147-
const key = `${filePath}:${line.beforeLineNumber}:BEFORE`;
148-
results.push(...(threadsByKey.get(key) ?? []));
154+
): ThreadEntry[] | undefined {
155+
if (line.type === "delete") {
156+
return line.beforeLineNumber ? fileThreads.before.get(line.beforeLineNumber) : undefined;
149157
}
150158

151-
if (line.type === "add" && line.afterLineNumber) {
152-
const key = `${filePath}:${line.afterLineNumber}:AFTER`;
153-
results.push(...(threadsByKey.get(key) ?? []));
159+
if (line.type === "add") {
160+
return line.afterLineNumber ? fileThreads.after.get(line.afterLineNumber) : undefined;
154161
}
155162

156163
/* v8 ignore start -- context lines always have both line numbers in practice */
157164
if (line.type === "context") {
158-
if (line.beforeLineNumber) {
159-
const key = `${filePath}:${line.beforeLineNumber}:BEFORE`;
160-
results.push(...(threadsByKey.get(key) ?? []));
161-
}
162-
if (line.afterLineNumber) {
163-
const key = `${filePath}:${line.afterLineNumber}:AFTER`;
164-
results.push(...(threadsByKey.get(key) ?? []));
165-
}
165+
const before = line.beforeLineNumber
166+
? fileThreads.before.get(line.beforeLineNumber)
167+
: undefined;
168+
const after = line.afterLineNumber ? fileThreads.after.get(line.afterLineNumber) : undefined;
169+
if (before && after) return [...before, ...after];
170+
return before ?? after;
166171
}
167172
/* v8 ignore stop */
168173

169-
return results;
174+
/* v8 ignore next -- diff lines are only ever add/delete/context */
175+
return undefined;
170176
}
171177

172178
export function buildDisplayLines(
@@ -181,15 +187,25 @@ export function buildDisplayLines(
181187
): DisplayLine[] {
182188
const lines: DisplayLine[] = [];
183189

184-
// Index inline comments by file:position:version for efficient lookup
185-
const inlineThreadsByKey = new Map<string, { thread: CommentThread; index: number }[]>();
190+
// Index inline comments by file, then by line number per version, so the
191+
// diff loop below can resolve anchors with integer lookups (no key strings).
192+
const inlineThreadsByFile = new Map<string, FileThreadIndex>();
186193
for (let i = 0; i < commentThreads.length; i++) {
187194
const thread = commentThreads[i]!;
188-
if (thread.location) {
189-
const key = `${thread.location.filePath}:${thread.location.filePosition}:${thread.location.relativeFileVersion}`;
190-
const existing = inlineThreadsByKey.get(key) ?? [];
195+
const location = thread.location;
196+
if (!location) continue;
197+
198+
let fileIndex = inlineThreadsByFile.get(location.filePath);
199+
if (!fileIndex) {
200+
fileIndex = { before: new Map(), after: new Map() };
201+
inlineThreadsByFile.set(location.filePath, fileIndex);
202+
}
203+
const bucket = location.relativeFileVersion === "BEFORE" ? fileIndex.before : fileIndex.after;
204+
const existing = bucket.get(location.filePosition);
205+
if (existing) {
191206
existing.push({ thread, index: i });
192-
inlineThreadsByKey.set(key, existing);
207+
} else {
208+
bucket.set(location.filePosition, [{ thread, index: i }]);
193209
}
194210
}
195211

@@ -220,23 +236,28 @@ export function buildDisplayLines(
220236
afterLines.length,
221237
displayLimit,
222238
);
223-
diffLines = computeSimpleDiff(
224-
beforeLines.slice(0, beforeLimit),
225-
afterLines.slice(0, afterLimit),
226-
);
239+
// Skip the slice copy when the limit already covers the whole array
240+
// (the common non-truncated case); computeSimpleDiff never mutates its inputs.
241+
const beforeSlice =
242+
beforeLimit === beforeLines.length ? beforeLines : beforeLines.slice(0, beforeLimit);
243+
const afterSlice =
244+
afterLimit === afterLines.length ? afterLines : afterLines.slice(0, afterLimit);
245+
diffLines = computeSimpleDiff(beforeSlice, afterSlice);
227246
// Enrich once here so the hot loop below can push lines without per-call copies
228247
for (const dl of diffLines) {
229248
dl.filePath = filePath;
230249
dl.diffKey = blobKey;
231250
}
232251
diffCache?.set(cacheKey, diffLines);
233252
}
234-
const hasInlineThreads = inlineThreadsByKey.size > 0;
253+
// Only files that actually have inline threads pay the per-line lookup cost.
254+
const fileThreads = inlineThreadsByFile.get(filePath);
235255
for (const dl of diffLines) {
236256
lines.push(dl);
237257

238-
if (!hasInlineThreads) continue;
239-
const matchingEntries = findMatchingThreadEntries(inlineThreadsByKey, filePath, dl);
258+
if (!fileThreads) continue;
259+
const matchingEntries = findMatchingThreadEntries(fileThreads, dl);
260+
if (!matchingEntries) continue;
240261
for (const { thread, index: threadIdx } of matchingEntries) {
241262
appendThreadLines(
242263
lines,

‎src/utils/formatDiff.ts‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -51,16 +51,19 @@ function hasNearbyMatch(lines: string[], start: number, target: string): boolean
5151
*/
5252
export function computeSimpleDiff(beforeLines: string[], afterLines: string[]): DisplayLine[] {
5353
const result: DisplayLine[] = [];
54+
// Inputs are never mutated, so hoist lengths out of the hot loop.
55+
const blen = beforeLines.length;
56+
const alen = afterLines.length;
5457
let bi = 0; // Index for beforeLines
5558
let ai = 0; // Index for afterLines
5659

5760
// Process both arrays until all lines are consumed
58-
while (bi < beforeLines.length || ai < afterLines.length) {
61+
while (bi < blen || ai < alen) {
5962
const beforeLine = beforeLines[bi];
6063
const afterLine = afterLines[ai];
6164

6265
// Case 1: Lines match at current position - add as context
63-
if (bi < beforeLines.length && ai < afterLines.length && beforeLine === afterLine) {
66+
if (bi < blen && ai < alen && beforeLine === afterLine) {
6467
result.push({
6568
type: "context",
6669
text: ` ${beforeLine}`,
@@ -75,10 +78,7 @@ export function computeSimpleDiff(beforeLines: string[], afterLines: string[]):
7578
const startAi = ai;
7679

7780
// Process deletions: consume lines from 'before' that don't match current 'after'
78-
while (
79-
bi < beforeLines.length &&
80-
(ai >= afterLines.length || beforeLines[bi] !== afterLines[ai])
81-
) {
81+
while (bi < blen && (ai >= alen || beforeLines[bi] !== afterLines[ai])) {
8282
const bl = beforeLines[bi]!;
8383
// Optimization: stop if this line appears within the lookahead window in 'after'
8484
if (hasNearbyMatch(afterLines, ai, bl)) break;
@@ -91,10 +91,7 @@ export function computeSimpleDiff(beforeLines: string[], afterLines: string[]):
9191
}
9292

9393
// Process additions: consume lines from 'after' that don't match current 'before'
94-
while (
95-
ai < afterLines.length &&
96-
(bi >= beforeLines.length || afterLines[ai] !== beforeLines[bi])
97-
) {
94+
while (ai < alen && (bi >= blen || afterLines[ai] !== beforeLines[bi])) {
9895
const al = afterLines[ai]!;
9996
// Optimization: stop if this line appears within the lookahead window in 'before'
10097
if (hasNearbyMatch(beforeLines, bi, al)) break;
@@ -109,15 +106,15 @@ export function computeSimpleDiff(beforeLines: string[], afterLines: string[]):
109106
// Safety: if both loops broke without advancing, force progress to prevent infinite loop
110107
/* v8 ignore start -- defensive guard; the greedy algorithm always advances in normal cases */
111108
if (bi === startBi && ai === startAi) {
112-
if (bi < beforeLines.length) {
109+
if (bi < blen) {
113110
result.push({
114111
type: "delete",
115112
text: `-${beforeLines[bi]}`,
116113
beforeLineNumber: bi + 1,
117114
});
118115
bi++;
119116
}
120-
if (ai < afterLines.length) {
117+
if (ai < alen) {
121118
result.push({
122119
type: "add",
123120
text: `+${afterLines[ai]}`,

0 commit comments

Comments
 (0)