authorgravatar for me@t3.ggTheo Browne <me@t3.gg> 2026-09-04 07:49:02-07:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-09-04 07:49:02-07:00
log777f5bb2e11fe30e7fdcb5741b0b1d9bb20924d6
tree0e5e5baf69eaffca19886633378dc92ace775b49
parent246064993535e5d90107d3d7784ceef3cc883435
signature Signed by PGP key B5690EEEBB952194

perf(mobile): reuse diff rows during comment edits (#9693)

2 files changed, 247 insertions(+), 63 deletions(-)

apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts+121-1
...@@ -1,4 +1,4 @@...@@ -1,4 +1,4 @@
1import { describe, expect, it } from "vite-plus/test";1import { describe, expect, it, vi } from "vite-plus/test";
2import {2import {
3 DEFAULT_MOBILE_THEME_ID,3 DEFAULT_MOBILE_THEME_ID,
4 getMobileThemeVariables,4 getMobileThemeVariables,
...@@ -9,12 +9,14 @@ import {...@@ -9,12 +9,14 @@ import {
9import { readDefaultMobileThemeVariables } from "../../lib/mobileTheme.test-support";9import { readDefaultMobileThemeVariables } from "../../lib/mobileTheme.test-support";
1010
11import {11import {
12 buildNativeReviewDiffData,
12 createNativeReviewDiffTheme,13 createNativeReviewDiffTheme,
13 getCachedNativeReviewDiffData,14 getCachedNativeReviewDiffData,
14 type BuildNativeReviewDiffDataInput,15 type BuildNativeReviewDiffDataInput,
15} from "./nativeReviewDiffAdapter";16} from "./nativeReviewDiffAdapter";
16import type { ReviewInlineComment } from "./reviewCommentSelection";17import type { ReviewInlineComment } from "./reviewCommentSelection";
17import { buildReviewParsedDiff } from "./reviewModel";18import { buildReviewParsedDiff } from "./reviewModel";
19import * as ReviewWordDiffs from "./reviewWordDiffs";
1820
19const parsedDiff = buildReviewParsedDiff(21const parsedDiff = buildReviewParsedDiff(
20 [22 [
...@@ -46,6 +48,23 @@ function buildInput(comments: BuildNativeReviewDiffDataInput["comments"]) {...@@ -46,6 +48,23 @@ function buildInput(comments: BuildNativeReviewDiffDataInput["comments"]) {
46 return { parsedDiff, comments } satisfies BuildNativeReviewDiffDataInput;48 return { parsedDiff, comments } satisfies BuildNativeReviewDiffDataInput;
47}49}
4850
51function filesPatch(paths: ReadonlyArray<string>) {
52 return paths
53 .map((path) =>
54 [
55 `diff --git a/${path} b/${path}`,
56 `--- a/${path}`,
57 `+++ b/${path}`,
58 "@@ -1,2 +1,2 @@",
59 '-const first = renderPanel({ label: "before", enabled: true });',
60 '-const second = renderPanel({ label: "before", enabled: true });',
61 '+const first = renderPanel({ label: "after", enabled: true });',
62 '+const second = renderPanel({ label: "after", enabled: true });',
63 ].join("\n"),
64 )
65 .join("\n");
66}
67
49function appTheme(themeId: MobileThemeId, appearance: MobileThemeAppearance) {68function appTheme(themeId: MobileThemeId, appearance: MobileThemeAppearance) {
50 return themeId === DEFAULT_MOBILE_THEME_ID69 return themeId === DEFAULT_MOBILE_THEME_ID
51 ? readDefaultMobileThemeVariables(appearance)70 ? readDefaultMobileThemeVariables(appearance)
...@@ -67,6 +86,107 @@ describe("getCachedNativeReviewDiffData", () => {...@@ -67,6 +86,107 @@ describe("getCachedNativeReviewDiffData", () => {
6786
68 expect(equivalent).toBe(first);87 expect(equivalent).toBe(first);
69 expect(changed).not.toBe(first);88 expect(changed).not.toBe(first);
89 expect(changed.rows.find((row) => row.kind === "comment")?.commentText).toBe("Changed");
90 });
91
92 it("reuses source rows and word matching when a file comment changes", () => {
93 const diff = buildReviewParsedDiff(filesPatch(["example.ts", "second.ts"]), "comment-reuse");
94 const matchWords = vi.spyOn(ReviewWordDiffs, "computeWordAltDiffRanges");
95 try {
96 const base = getCachedNativeReviewDiffData({ parsedDiff: diff });
97 expect(matchWords).toHaveBeenCalledTimes(4);
98 expect(base.rows.filter((row) => row.wordDiffRanges?.length)).toHaveLength(8);
99 const firstComment = makeComment("First file comment");
100 const secondComment = {
101 ...makeComment("Second file comment"),
102 id: "comment-2",
103 filePath: "second.ts",
104 };
105 const first = getCachedNativeReviewDiffData({
106 parsedDiff: diff,
107 comments: [firstComment, secondComment],
108 });
109 const changed = getCachedNativeReviewDiffData({
110 parsedDiff: diff,
111 comments: [{ ...firstComment, text: "Changed first comment" }, secondComment],
112 });
113
114 expect(matchWords).toHaveBeenCalledTimes(4);
115 expect(changed.files).toBe(base.files);
116 expect(changed.commentTargetsByRowId).toBe(base.commentTargetsByRowId);
117 expect(changed.rowIdByCommentLineId).toBe(base.rowIdByCommentLineId);
118 expect(changed.rows.find((row) => row.id === secondComment.id)).toBe(
119 first.rows.find((row) => row.id === secondComment.id),
120 );
121 const sourceRows = changed.rows.filter((row) => row.kind !== "comment");
122 for (const [index, row] of sourceRows.entries()) {
123 expect(row).toBe(base.rows[index]);
124 }
125 const removed = getCachedNativeReviewDiffData({ parsedDiff: diff, comments: [] });
126 expect(removed.rows).toEqual(base.rows);
127 expect(matchWords).toHaveBeenCalledTimes(4);
128 expect(first.rows.find((row) => row.id === firstComment.id)?.commentText).toBe(
129 "First file comment",
130 );
131 } finally {
132 matchWords.mockRestore();
133 }
134 });
135
136 it("updates comment titles and locations without changing source coordinates", () => {
137 const diff = buildReviewParsedDiff(filesPatch(["example.ts", "second.ts"]), "comment-metadata");
138 const comment = makeComment("Review this line");
139 const first = getCachedNativeReviewDiffData({ parsedDiff: diff, comments: [comment] });
140 const renamed = getCachedNativeReviewDiffData({
141 parsedDiff: diff,
142 comments: [{ ...comment, sectionTitle: "Branch comparison" }],
143 });
144 expect(renamed.rows.find((row) => row.id === comment.id)?.commentSectionTitle).toBe(
145 "Branch comparison",
146 );
147 const moved = getCachedNativeReviewDiffData({
148 parsedDiff: diff,
149 comments: [{ ...comment, filePath: "second.ts", endIndex: 99, rangeLabel: "+2" }],
150 });
151 const commentIndex = moved.rows.findIndex((row) => row.id === comment.id);
152 expect(moved.rows[commentIndex]).toMatchObject({
153 kind: "comment",
154 filePath: "second.ts",
155 commentRangeLabel: "+2",
156 });
157 expect(moved.rows[commentIndex - 1]).toMatchObject({
158 kind: "line",
159 change: "add",
160 newLineNumber: 2,
161 });
162 expect(moved.rows.filter((row) => row.kind !== "comment")).toEqual(
163 first.rows.filter((row) => row.kind !== "comment"),
164 );
165 expect(first.rows.find((row) => row.id === comment.id)?.commentSectionTitle).toBe(
166 "Dirty worktree",
167 );
168 });
169
170 it("keeps cached IDs and targets local to each parsed section and file layout", () => {
171 const original = filesPatch(["example.ts", "second.ts"]);
172 const variants = [
173 buildReviewParsedDiff(original, "first-section"),
174 buildReviewParsedDiff(original, "second-section"),
175 buildReviewParsedDiff(
176 filesPatch(["inserted.ts", "example.ts", "second.ts"]),
177 "first-section",
178 ),
179 buildReviewParsedDiff(filesPatch(["second.ts", "example.ts"]), "first-section"),
180 buildReviewParsedDiff(filesPatch(["example.ts"]), "first-section"),
181 buildReviewParsedDiff(
182 original.replaceAll("@@ -1,2 +1,2 @@", "@@ -7,2 +12,2 @@"),
183 "first-section",
184 ),
185 ];
186 for (const diff of [...variants, variants[0]!]) {
187 const input = { parsedDiff: diff, comments: [makeComment("Coordinates")] };
188 expect(getCachedNativeReviewDiffData(input)).toEqual(buildNativeReviewDiffData(input));
189 }
70 });190 });
71});191});
72192
apps/mobile/src/features/review/nativeReviewDiffAdapter.ts+126-62
...@@ -105,10 +105,26 @@ export interface BuildNativeReviewDiffDataInput {...@@ -105,10 +105,26 @@ export interface BuildNativeReviewDiffDataInput {
105}105}
106106
107interface CachedNativeReviewDiffData {107interface CachedNativeReviewDiffData {
108 readonly prepared: PreparedNativeReviewDiffData;
108 readonly commentsKey: string;109 readonly commentsKey: string;
109 readonly data: NativeReviewDiffData;110 readonly data: NativeReviewDiffData;
110}111}
111112
113interface PreparedNativeReviewFileRows {
114 readonly fileId: string;
115 readonly filePath: string;
116 readonly lineCount: number;
117 readonly rows: ReadonlyArray<NativeReviewDiffRow>;
118 commentedRows: {
119 readonly commentsKey: string;
120 readonly rows: ReadonlyArray<NativeReviewDiffRow>;
121 } | null;
122}
123
124interface PreparedNativeReviewDiffData extends Omit<NativeReviewDiffData, "rows"> {
125 readonly fileRows: ReadonlyArray<PreparedNativeReviewFileRows>;
126}
127
112const nativeReviewDiffDataCache = new WeakMap<ReviewParsedDiff, CachedNativeReviewDiffData>();128const nativeReviewDiffDataCache = new WeakMap<ReviewParsedDiff, CachedNativeReviewDiffData>();
113129
114function buildReviewCommentsCacheKey(comments: ReadonlyArray<ReviewInlineComment>): string {130function buildReviewCommentsCacheKey(comments: ReadonlyArray<ReviewInlineComment>): string {
...@@ -116,19 +132,18 @@ function buildReviewCommentsCacheKey(comments: ReadonlyArray<ReviewInlineComment...@@ -116,19 +132,18 @@ function buildReviewCommentsCacheKey(comments: ReadonlyArray<ReviewInlineComment
116 return "none";132 return "none";
117 }133 }
118134
119 return comments135 return JSON.stringify(
120 .map((comment) =>136 comments.map((comment) => [
121 [137 comment.id,
122 comment.id,138 comment.sectionId,
123 comment.sectionId,139 comment.sectionTitle,
124 comment.filePath,140 comment.filePath,
125 comment.startIndex,141 comment.startIndex,
126 comment.endIndex,142 comment.endIndex,
127 comment.rangeLabel,143 comment.rangeLabel,
128 comment.text,144 comment.text,
129 ].join("\u001f"),145 ]),
130 )146 );
131 .join("\u001e");
132}147}
133148
134export function createNativeReviewDiffTheme(149export function createNativeReviewDiffTheme(
...@@ -378,12 +393,11 @@ function mapLineRow(...@@ -378,12 +393,11 @@ function mapLineRow(
378 };393 };
379}394}
380395
381function mapFileRows(396function prepareFileRows(
382 file: ReviewRenderableFile,397 file: ReviewRenderableFile,
383 comments: ReadonlyArray<ReviewInlineComment>,
384 commentTargetsByRowId: Map<string, NativeReviewDiffCommentTarget>,398 commentTargetsByRowId: Map<string, NativeReviewDiffCommentTarget>,
385 rowIdByCommentLineId: Map<string, string>,399 rowIdByCommentLineId: Map<string, string>,
386): ReadonlyArray<NativeReviewDiffRow> {400): PreparedNativeReviewFileRows {
387 const rows: NativeReviewDiffRow[] = [401 const rows: NativeReviewDiffRow[] = [
388 {402 {
389 kind: "file",403 kind: "file",
...@@ -398,22 +412,6 @@ function mapFileRows(...@@ -398,22 +412,6 @@ function mapFileRows(
398 ];412 ];
399413
400 const lineRows = file.rows.filter((row): row is ReviewRenderableLineRow => row.kind === "line");414 const lineRows = file.rows.filter((row): row is ReviewRenderableLineRow => row.kind === "line");
401 const commentsByEndIndex = new Map<number, ReviewInlineComment[]>();
402 comments.forEach((comment) => {
403 if (comment.filePath !== file.path) {
404 return;
405 }
406 const endIndex = Math.min(comment.endIndex, lineRows.length - 1);
407 if (endIndex < 0) {
408 return;
409 }
410 const existing = commentsByEndIndex.get(endIndex);
411 if (existing) {
412 existing.push(comment);
413 return;
414 }
415 commentsByEndIndex.set(endIndex, [comment]);
416 });
417 let lineIndex = 0;415 let lineIndex = 0;
418 file.rows.forEach((row, rowIndex) => {416 file.rows.forEach((row, rowIndex) => {
419 if (row.kind === "hunk") {417 if (row.kind === "hunk") {
...@@ -434,37 +432,70 @@ function mapFileRows(...@@ -434,37 +432,70 @@ function mapFileRows(
434 lines: lineRows,432 lines: lineRows,
435 lineIndex,433 lineIndex,
436 });434 });
437 const commentsForLine = commentsByEndIndex.get(lineIndex) ?? [];435 lineIndex += 1;
438 for (const comment of commentsForLine) {436 });
437
438 rows.push(...noticeRowsForFile(file));
439 return {
440 fileId: file.id,
441 filePath: file.path,
442 lineCount: lineRows.length,
443 // Comments must not split the source deletion/addition runs used for word matching.
444 rows: addNativeWordDiffRanges(rows),
445 commentedRows: null,
446 };
447}
448
449function insertFileComments(
450 file: PreparedNativeReviewFileRows,
451 comments: ReadonlyArray<ReviewInlineComment>,
452): ReadonlyArray<NativeReviewDiffRow> {
453 if (comments.length === 0) {
454 file.commentedRows = null;
455 return file.rows;
456 }
457 const commentsKey = buildReviewCommentsCacheKey(comments);
458 if (file.commentedRows?.commentsKey === commentsKey) {
459 return file.commentedRows.rows;
460 }
461
462 const commentsByEndIndex = new Map<number, ReviewInlineComment[]>();
463 for (const comment of comments) {
464 const endIndex = Math.min(comment.endIndex, file.lineCount - 1);
465 if (endIndex < 0) continue;
466 const existing = commentsByEndIndex.get(endIndex);
467 if (existing) {
468 existing.push(comment);
469 } else {
470 commentsByEndIndex.set(endIndex, [comment]);
471 }
472 }
473 const rows: NativeReviewDiffRow[] = [];
474 let lineIndex = 0;
475 for (const row of file.rows) {
476 rows.push(row);
477 if (row.kind !== "line") continue;
478 for (const comment of commentsByEndIndex.get(lineIndex) ?? []) {
439 rows.push({479 rows.push({
440 kind: "comment",480 kind: "comment",
441 id: comment.id,481 id: comment.id,
442 fileId: file.id,482 fileId: file.fileId,
443 filePath: file.path,483 filePath: file.filePath,
444 commentText: comment.text,484 commentText: comment.text,
445 commentRangeLabel: comment.rangeLabel,485 commentRangeLabel: comment.rangeLabel,
446 commentSectionTitle: comment.sectionTitle,486 commentSectionTitle: comment.sectionTitle,
447 });487 });
448 }488 }
449 lineIndex += 1;489 lineIndex += 1;
450 });490 }
451491 file.commentedRows = { commentsKey, rows };
452 rows.push(...noticeRowsForFile(file));
453 return rows;492 return rows;
454}493}
455494
456export function buildNativeReviewDiffData(495function prepareNativeReviewDiffData(parsedDiff: ReviewParsedDiff): PreparedNativeReviewDiffData {
457 input: BuildNativeReviewDiffDataInput,
458): NativeReviewDiffData;
459export function buildNativeReviewDiffData(parsedDiff: ReviewParsedDiff): NativeReviewDiffData;
460export function buildNativeReviewDiffData(
461 input: ReviewParsedDiff | BuildNativeReviewDiffDataInput,
462): NativeReviewDiffData {
463 const parsedDiff = "parsedDiff" in input ? input.parsedDiff : input;
464 const comments = "parsedDiff" in input ? (input.comments ?? []) : [];
465 if (parsedDiff.kind !== "files") {496 if (parsedDiff.kind !== "files") {
466 return {497 return {
467 rows: [],498 fileRows: [],
468 files: [],499 files: [],
469 commentTargetsByRowId: new Map(),500 commentTargetsByRowId: new Map(),
470 rowIdByCommentLineId: new Map(),501 rowIdByCommentLineId: new Map(),
...@@ -482,14 +513,12 @@ export function buildNativeReviewDiffData(...@@ -482,14 +513,12 @@ export function buildNativeReviewDiffData(
482 }));513 }));
483 const commentTargetsByRowId = new Map<string, NativeReviewDiffCommentTarget>();514 const commentTargetsByRowId = new Map<string, NativeReviewDiffCommentTarget>();
484 const rowIdByCommentLineId = new Map<string, string>();515 const rowIdByCommentLineId = new Map<string, string>();
485 const rows = addNativeWordDiffRanges(516 const fileRows = parsedDiff.files.map((file) =>
486 Arr.flatMap(parsedDiff.files, (file) =>517 prepareFileRows(file, commentTargetsByRowId, rowIdByCommentLineId),
487 mapFileRows(file, comments, commentTargetsByRowId, rowIdByCommentLineId),
488 ),
489 );518 );
490519
491 return {520 return {
492 rows,521 fileRows,
493 files,522 files,
494 commentTargetsByRowId,523 commentTargetsByRowId,
495 rowIdByCommentLineId,524 rowIdByCommentLineId,
...@@ -498,10 +527,47 @@ export function buildNativeReviewDiffData(...@@ -498,10 +527,47 @@ export function buildNativeReviewDiffData(
498 };527 };
499}528}
500529
530function buildCommentedNativeReviewDiffData(
531 prepared: PreparedNativeReviewDiffData,
532 comments: ReadonlyArray<ReviewInlineComment>,
533): NativeReviewDiffData {
534 const commentsByFilePath = new Map<string, ReviewInlineComment[]>();
535 for (const comment of comments) {
536 const existing = commentsByFilePath.get(comment.filePath);
537 if (existing) {
538 existing.push(comment);
539 } else {
540 commentsByFilePath.set(comment.filePath, [comment]);
541 }
542 }
543 return {
544 rows: Arr.flatMap(prepared.fileRows, (file) =>
545 insertFileComments(file, commentsByFilePath.get(file.filePath) ?? []),
546 ),
547 files: prepared.files,
548 commentTargetsByRowId: prepared.commentTargetsByRowId,
549 rowIdByCommentLineId: prepared.rowIdByCommentLineId,
550 additions: prepared.additions,
551 deletions: prepared.deletions,
552 };
553}
554
555export function buildNativeReviewDiffData(
556 input: BuildNativeReviewDiffDataInput,
557): NativeReviewDiffData;
558export function buildNativeReviewDiffData(parsedDiff: ReviewParsedDiff): NativeReviewDiffData;
559export function buildNativeReviewDiffData(
560 input: ReviewParsedDiff | BuildNativeReviewDiffDataInput,
561): NativeReviewDiffData {
562 const parsedDiff = "parsedDiff" in input ? input.parsedDiff : input;
563 const comments = "parsedDiff" in input ? (input.comments ?? []) : [];
564 return buildCommentedNativeReviewDiffData(prepareNativeReviewDiffData(parsedDiff), comments);
565}
566
501/**567/**
502 * Reuses the expensive flattened native row model across React development568 * Prepares source rows once per parsed diff, including its section-specific IDs.
503 * render probes and unrelated draft updates. Only the latest comment version569 * Comment edits reuse those rows, word ranges, and targets. Each file retains
504 * is retained for each parsed diff so editing a comment cannot grow the cache.570 * only its latest comment overlay, and the weak key releases old parsed diffs.
505 */571 */
506export function getCachedNativeReviewDiffData(572export function getCachedNativeReviewDiffData(
507 input: BuildNativeReviewDiffDataInput,573 input: BuildNativeReviewDiffDataInput,
...@@ -513,10 +579,8 @@ export function getCachedNativeReviewDiffData(...@@ -513,10 +579,8 @@ export function getCachedNativeReviewDiffData(
513 return cached.data;579 return cached.data;
514 }580 }
515581
516 const data = buildNativeReviewDiffData({582 const prepared = cached?.prepared ?? prepareNativeReviewDiffData(input.parsedDiff);
517 parsedDiff: input.parsedDiff,583 const data = buildCommentedNativeReviewDiffData(prepared, comments);
518 comments,584 nativeReviewDiffDataCache.set(input.parsedDiff, { prepared, commentsKey, data });
519 });
520 nativeReviewDiffDataCache.set(input.parsedDiff, { commentsKey, data });
521 return data;585 return data;
522}586}