perf: Pre-index review comments into Map to eliminate O(n) filtering
Build a Map<string, ReviewComment[]> once in ReviewTab and thread it
down through DiffViewer → FileCard → HunkRows, replacing O(n) filter
calls with O(1) map lookups. With 200 comments and 5000 diff lines,
this reduces ~1M iterations per render cycle to ~5K.
Key: "${filePath}:${lineNumber}:${lineType}" for line-level comments,
"${filePath}:file" for file-level (lineNumber === null) comments.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -1,9 +1,20 @@
|
|||||||
import type { FileDiff, DiffLine, ReviewComment } from "./types";
|
import type { FileDiff, DiffLine, ReviewComment } from "./types";
|
||||||
import { FileCard } from "./FileCard";
|
import { FileCard } from "./FileCard";
|
||||||
|
|
||||||
|
function getFileCommentMap(
|
||||||
|
commentsByLine: Map<string, ReviewComment[]>,
|
||||||
|
filePath: string,
|
||||||
|
): Map<string, ReviewComment[]> {
|
||||||
|
const result = new Map<string, ReviewComment[]>();
|
||||||
|
for (const [key, val] of commentsByLine) {
|
||||||
|
if (key.startsWith(`${filePath}:`)) result.set(key, val);
|
||||||
|
}
|
||||||
|
return result;
|
||||||
|
}
|
||||||
|
|
||||||
interface DiffViewerProps {
|
interface DiffViewerProps {
|
||||||
files: FileDiff[];
|
files: FileDiff[];
|
||||||
comments: ReviewComment[];
|
commentsByLine: Map<string, ReviewComment[]>;
|
||||||
onAddComment: (
|
onAddComment: (
|
||||||
filePath: string,
|
filePath: string,
|
||||||
lineNumber: number,
|
lineNumber: number,
|
||||||
@@ -21,7 +32,7 @@ interface DiffViewerProps {
|
|||||||
|
|
||||||
export function DiffViewer({
|
export function DiffViewer({
|
||||||
files,
|
files,
|
||||||
comments,
|
commentsByLine,
|
||||||
onAddComment,
|
onAddComment,
|
||||||
onResolveComment,
|
onResolveComment,
|
||||||
onUnresolveComment,
|
onUnresolveComment,
|
||||||
@@ -37,7 +48,7 @@ export function DiffViewer({
|
|||||||
<div key={file.newPath} ref={(el) => onRegisterRef?.(file.newPath, el)}>
|
<div key={file.newPath} ref={(el) => onRegisterRef?.(file.newPath, el)}>
|
||||||
<FileCard
|
<FileCard
|
||||||
file={file}
|
file={file}
|
||||||
comments={comments.filter((c) => c.filePath === file.newPath)}
|
commentsByLine={getFileCommentMap(commentsByLine, file.newPath)}
|
||||||
onAddComment={onAddComment}
|
onAddComment={onAddComment}
|
||||||
onResolveComment={onResolveComment}
|
onResolveComment={onResolveComment}
|
||||||
onUnresolveComment={onUnresolveComment}
|
onUnresolveComment={onUnresolveComment}
|
||||||
|
|||||||
@@ -43,7 +43,7 @@ const leftBorderClass: Record<FileChangeType, string> = {
|
|||||||
|
|
||||||
interface FileCardProps {
|
interface FileCardProps {
|
||||||
file: FileDiff;
|
file: FileDiff;
|
||||||
comments: ReviewComment[];
|
commentsByLine: Map<string, ReviewComment[]>;
|
||||||
onAddComment: (
|
onAddComment: (
|
||||||
filePath: string,
|
filePath: string,
|
||||||
lineNumber: number,
|
lineNumber: number,
|
||||||
@@ -60,7 +60,7 @@ interface FileCardProps {
|
|||||||
|
|
||||||
export function FileCard({
|
export function FileCard({
|
||||||
file,
|
file,
|
||||||
comments,
|
commentsByLine,
|
||||||
onAddComment,
|
onAddComment,
|
||||||
onResolveComment,
|
onResolveComment,
|
||||||
onUnresolveComment,
|
onUnresolveComment,
|
||||||
@@ -70,7 +70,16 @@ export function FileCard({
|
|||||||
onToggleViewed = () => {},
|
onToggleViewed = () => {},
|
||||||
}: FileCardProps) {
|
}: FileCardProps) {
|
||||||
const [expanded, setExpanded] = useState(true);
|
const [expanded, setExpanded] = useState(true);
|
||||||
const commentCount = comments.length;
|
|
||||||
|
const commentCount = useMemo(
|
||||||
|
() =>
|
||||||
|
Array.from(commentsByLine.values()).reduce(
|
||||||
|
(sum, arr) => sum + arr.length,
|
||||||
|
0,
|
||||||
|
),
|
||||||
|
[commentsByLine],
|
||||||
|
);
|
||||||
|
|
||||||
const badge = changeTypeBadge[file.changeType];
|
const badge = changeTypeBadge[file.changeType];
|
||||||
|
|
||||||
// Flatten all hunk lines for syntax highlighting
|
// Flatten all hunk lines for syntax highlighting
|
||||||
@@ -158,7 +167,7 @@ export function FileCard({
|
|||||||
key={hi}
|
key={hi}
|
||||||
hunk={hunk}
|
hunk={hunk}
|
||||||
filePath={file.newPath}
|
filePath={file.newPath}
|
||||||
comments={comments}
|
commentsByLine={commentsByLine}
|
||||||
onAddComment={onAddComment}
|
onAddComment={onAddComment}
|
||||||
onResolveComment={onResolveComment}
|
onResolveComment={onResolveComment}
|
||||||
onUnresolveComment={onUnresolveComment}
|
onUnresolveComment={onUnresolveComment}
|
||||||
|
|||||||
@@ -6,7 +6,7 @@ import type { LineTokenMap } from "./use-syntax-highlight";
|
|||||||
interface HunkRowsProps {
|
interface HunkRowsProps {
|
||||||
hunk: { header: string; lines: DiffLine[] };
|
hunk: { header: string; lines: DiffLine[] };
|
||||||
filePath: string;
|
filePath: string;
|
||||||
comments: ReviewComment[];
|
commentsByLine: Map<string, ReviewComment[]>;
|
||||||
onAddComment: (
|
onAddComment: (
|
||||||
filePath: string,
|
filePath: string,
|
||||||
lineNumber: number,
|
lineNumber: number,
|
||||||
@@ -23,7 +23,7 @@ interface HunkRowsProps {
|
|||||||
export function HunkRows({
|
export function HunkRows({
|
||||||
hunk,
|
hunk,
|
||||||
filePath,
|
filePath,
|
||||||
comments,
|
commentsByLine,
|
||||||
onAddComment,
|
onAddComment,
|
||||||
onResolveComment,
|
onResolveComment,
|
||||||
onUnresolveComment,
|
onUnresolveComment,
|
||||||
@@ -81,9 +81,9 @@ export function HunkRows({
|
|||||||
|
|
||||||
{hunk.lines.map((line, li) => {
|
{hunk.lines.map((line, li) => {
|
||||||
const lineKey = line.newLineNumber ?? line.oldLineNumber ?? li;
|
const lineKey = line.newLineNumber ?? line.oldLineNumber ?? li;
|
||||||
const lineComments = comments.filter(
|
// O(1) map lookup — replaces the previous O(n) filter
|
||||||
(c) => c.lineNumber === lineKey && c.lineType === line.type,
|
const lineComments =
|
||||||
);
|
commentsByLine.get(`${filePath}:${lineKey}:${line.type}`) ?? [];
|
||||||
const isCommenting =
|
const isCommenting =
|
||||||
commentingLine?.lineNumber === lineKey &&
|
commentingLine?.lineNumber === lineKey &&
|
||||||
commentingLine?.lineType === line.type;
|
commentingLine?.lineType === line.type;
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ import { DiffViewer } from "./DiffViewer";
|
|||||||
import { ReviewSidebar } from "./ReviewSidebar";
|
import { ReviewSidebar } from "./ReviewSidebar";
|
||||||
import { ReviewHeader } from "./ReviewHeader";
|
import { ReviewHeader } from "./ReviewHeader";
|
||||||
import { InitiativeReview } from "./InitiativeReview";
|
import { InitiativeReview } from "./InitiativeReview";
|
||||||
|
import { buildCommentIndex } from "./comment-index";
|
||||||
import type { ReviewStatus, DiffLine } from "./types";
|
import type { ReviewStatus, DiffLine } from "./types";
|
||||||
|
|
||||||
interface ReviewTabProps {
|
interface ReviewTabProps {
|
||||||
@@ -169,6 +170,11 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
|||||||
}));
|
}));
|
||||||
}, [commentsQuery.data]);
|
}, [commentsQuery.data]);
|
||||||
|
|
||||||
|
const commentsByLine = useMemo(
|
||||||
|
() => buildCommentIndex(comments),
|
||||||
|
[comments],
|
||||||
|
);
|
||||||
|
|
||||||
const createCommentMutation = trpc.createReviewComment.useMutation({
|
const createCommentMutation = trpc.createReviewComment.useMutation({
|
||||||
onSuccess: () => {
|
onSuccess: () => {
|
||||||
utils.listReviewComments.invalidate({ phaseId: activePhaseId! });
|
utils.listReviewComments.invalidate({ phaseId: activePhaseId! });
|
||||||
@@ -400,7 +406,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
|||||||
) : (
|
) : (
|
||||||
<DiffViewer
|
<DiffViewer
|
||||||
files={files}
|
files={files}
|
||||||
comments={comments}
|
commentsByLine={commentsByLine}
|
||||||
onAddComment={handleAddComment}
|
onAddComment={handleAddComment}
|
||||||
onResolveComment={handleResolveComment}
|
onResolveComment={handleResolveComment}
|
||||||
onUnresolveComment={handleUnresolveComment}
|
onUnresolveComment={handleUnresolveComment}
|
||||||
|
|||||||
134
apps/web/src/components/review/comment-index.test.tsx
Normal file
134
apps/web/src/components/review/comment-index.test.tsx
Normal file
@@ -0,0 +1,134 @@
|
|||||||
|
// @vitest-environment happy-dom
|
||||||
|
import "@testing-library/jest-dom/vitest";
|
||||||
|
import { render, screen } from "@testing-library/react";
|
||||||
|
import { vi, describe, it, expect } from "vitest";
|
||||||
|
import { buildCommentIndex } from "./comment-index";
|
||||||
|
import type { ReviewComment } from "./types";
|
||||||
|
|
||||||
|
// ── Stub CommentThread and CommentForm so LineWithComments renders without deps ──
|
||||||
|
vi.mock("./CommentThread", () => ({
|
||||||
|
CommentThread: () => <div data-testid="comment-thread" />,
|
||||||
|
}));
|
||||||
|
vi.mock("./CommentForm", () => ({
|
||||||
|
CommentForm: vi.fn().mockReturnValue(<div data-testid="comment-form" />),
|
||||||
|
}));
|
||||||
|
vi.mock("./use-syntax-highlight", () => ({
|
||||||
|
useHighlightedFile: () => null,
|
||||||
|
}));
|
||||||
|
|
||||||
|
// ── Helpers ──────────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
function makeComment(overrides: Partial<ReviewComment> & { id: string }): ReviewComment {
|
||||||
|
return {
|
||||||
|
id: overrides.id,
|
||||||
|
filePath: overrides.filePath ?? "src/foo.ts",
|
||||||
|
lineNumber: overrides.lineNumber !== undefined ? overrides.lineNumber : 1,
|
||||||
|
lineType: overrides.lineType ?? "added",
|
||||||
|
body: overrides.body ?? "comment body",
|
||||||
|
author: overrides.author ?? "alice",
|
||||||
|
createdAt: overrides.createdAt ?? "2024-01-01T00:00:00Z",
|
||||||
|
resolved: overrides.resolved ?? false,
|
||||||
|
parentCommentId: overrides.parentCommentId ?? null,
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
// ── buildCommentIndex — pure function tests ───────────────────────────────────
|
||||||
|
|
||||||
|
describe("buildCommentIndex", () => {
|
||||||
|
it("happy path — basic indexing", () => {
|
||||||
|
const c1 = makeComment({ id: "1", filePath: "src/foo.ts", lineNumber: 10, lineType: "added" });
|
||||||
|
const c2 = makeComment({ id: "2", filePath: "src/bar.ts", lineNumber: 5, lineType: "context" });
|
||||||
|
const map = buildCommentIndex([c1, c2]);
|
||||||
|
expect(map.get("src/foo.ts:10:added")).toEqual([c1]);
|
||||||
|
expect(map.get("src/bar.ts:5:context")).toEqual([c2]);
|
||||||
|
expect(map.size).toBe(2);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("same-line accumulation — two comments land in same array", () => {
|
||||||
|
const c1 = makeComment({ id: "a", filePath: "src/x.ts", lineNumber: 20, lineType: "added" });
|
||||||
|
const c2 = makeComment({ id: "b", filePath: "src/x.ts", lineNumber: 20, lineType: "added" });
|
||||||
|
const map = buildCommentIndex([c1, c2]);
|
||||||
|
expect(map.get("src/x.ts:20:added")).toEqual([c1, c2]);
|
||||||
|
expect(map.size).toBe(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("cross-type isolation — same lineNumber but different lineType produces separate entries", () => {
|
||||||
|
const added = makeComment({ id: "a", filePath: "src/x.ts", lineNumber: 10, lineType: "added" });
|
||||||
|
const removed = makeComment({ id: "r", filePath: "src/x.ts", lineNumber: 10, lineType: "removed" });
|
||||||
|
const map = buildCommentIndex([added, removed]);
|
||||||
|
expect(map.get("src/x.ts:10:added")).toEqual([added]);
|
||||||
|
expect(map.get("src/x.ts:10:removed")).toEqual([removed]);
|
||||||
|
expect(map.size).toBe(2);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("null lineNumber — file-level comment stored under filePath:file", () => {
|
||||||
|
const fileComment = makeComment({ id: "f", filePath: "src/z.ts", lineNumber: null, lineType: "context" });
|
||||||
|
const map = buildCommentIndex([fileComment]);
|
||||||
|
expect(map.get("src/z.ts:file")).toEqual([fileComment]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("empty input — returns empty map", () => {
|
||||||
|
expect(buildCommentIndex([])).toEqual(new Map());
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
// ── LineWithComments — component tests ───────────────────────────────────────
|
||||||
|
|
||||||
|
import { LineWithComments } from "./LineWithComments";
|
||||||
|
import type { DiffLine } from "./types";
|
||||||
|
|
||||||
|
const addedLine: DiffLine = {
|
||||||
|
type: "added",
|
||||||
|
content: "const x = 1;",
|
||||||
|
oldLineNumber: null,
|
||||||
|
newLineNumber: 5,
|
||||||
|
};
|
||||||
|
|
||||||
|
const noop = () => {};
|
||||||
|
|
||||||
|
describe("LineWithComments", () => {
|
||||||
|
it("renders comment button with title when lineComments is non-empty", () => {
|
||||||
|
const lineComments = [
|
||||||
|
makeComment({ id: "c1", filePath: "src/foo.ts", lineNumber: 5, lineType: "added" }),
|
||||||
|
];
|
||||||
|
render(
|
||||||
|
<table>
|
||||||
|
<tbody>
|
||||||
|
<LineWithComments
|
||||||
|
line={addedLine}
|
||||||
|
lineKey={5}
|
||||||
|
lineComments={lineComments}
|
||||||
|
isCommenting={false}
|
||||||
|
onStartComment={noop}
|
||||||
|
onCancelComment={noop}
|
||||||
|
onSubmitComment={noop}
|
||||||
|
onResolveComment={noop}
|
||||||
|
onUnresolveComment={noop}
|
||||||
|
/>
|
||||||
|
</tbody>
|
||||||
|
</table>,
|
||||||
|
);
|
||||||
|
expect(screen.getByTitle(/1 comment/)).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not render comment thread row when lineComments is empty", () => {
|
||||||
|
render(
|
||||||
|
<table>
|
||||||
|
<tbody>
|
||||||
|
<LineWithComments
|
||||||
|
line={addedLine}
|
||||||
|
lineKey={5}
|
||||||
|
lineComments={[]}
|
||||||
|
isCommenting={false}
|
||||||
|
onStartComment={noop}
|
||||||
|
onCancelComment={noop}
|
||||||
|
onSubmitComment={noop}
|
||||||
|
onResolveComment={noop}
|
||||||
|
onUnresolveComment={noop}
|
||||||
|
/>
|
||||||
|
</tbody>
|
||||||
|
</table>,
|
||||||
|
);
|
||||||
|
expect(document.querySelector("[data-comment-id]")).toBeNull();
|
||||||
|
});
|
||||||
|
});
|
||||||
25
apps/web/src/components/review/comment-index.ts
Normal file
25
apps/web/src/components/review/comment-index.ts
Normal file
@@ -0,0 +1,25 @@
|
|||||||
|
import type { ReviewComment } from "./types";
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Build a Map keyed by `"${filePath}:${lineNumber}:${lineType}"` for line-level
|
||||||
|
* comments, or `"${filePath}:file"` for file-level comments (lineNumber === null).
|
||||||
|
*
|
||||||
|
* The compound key (filePath + lineNumber + lineType) is required because
|
||||||
|
* added and removed lines can share the same numeric position in a replacement
|
||||||
|
* hunk (e.g., old line 10 removed, new line 10 added).
|
||||||
|
*/
|
||||||
|
export function buildCommentIndex(
|
||||||
|
comments: ReviewComment[],
|
||||||
|
): Map<string, ReviewComment[]> {
|
||||||
|
const map = new Map<string, ReviewComment[]>();
|
||||||
|
for (const comment of comments) {
|
||||||
|
const key =
|
||||||
|
comment.lineNumber != null
|
||||||
|
? `${comment.filePath}:${comment.lineNumber}:${comment.lineType}`
|
||||||
|
: `${comment.filePath}:file`;
|
||||||
|
const existing = map.get(key);
|
||||||
|
if (existing) existing.push(comment);
|
||||||
|
else map.set(key, [comment]);
|
||||||
|
}
|
||||||
|
return map;
|
||||||
|
}
|
||||||
@@ -28,7 +28,7 @@ export interface FileDiff {
|
|||||||
export interface ReviewComment {
|
export interface ReviewComment {
|
||||||
id: string;
|
id: string;
|
||||||
filePath: string;
|
filePath: string;
|
||||||
lineNumber: number; // new-side line number (or old-side for deletions)
|
lineNumber: number | null; // null = file-level comment
|
||||||
lineType: "added" | "removed" | "context";
|
lineType: "added" | "removed" | "context";
|
||||||
body: string;
|
body: string;
|
||||||
author: string;
|
author: string;
|
||||||
|
|||||||
Reference in New Issue
Block a user