From 88df58efa06658d855fa9199e67ce89ba46651e4 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 12 Sep 2026 19:39:45 -0700 Subject: [PATCH] perf(mobile): index review notes by file before building the queue (#20228) --- .../mobile-review-note-index-benchmark.mjs | 102 ++++++++++++++++ .../src/session/mobile-diff-review-queue.ts | 26 +++- .../session/mobile-review-note-index.test.ts | 113 ++++++++++++++++++ 3 files changed, 237 insertions(+), 4 deletions(-) create mode 100644 config/scripts/mobile-review-note-index-benchmark.mjs create mode 100644 mobile/src/session/mobile-review-note-index.test.ts diff --git a/config/scripts/mobile-review-note-index-benchmark.mjs b/config/scripts/mobile-review-note-index-benchmark.mjs new file mode 100644 index 00000000000..7eecb488692 --- /dev/null +++ b/config/scripts/mobile-review-note-index-benchmark.mjs @@ -0,0 +1,102 @@ +import assert from 'node:assert/strict' +import { execFileSync } from 'node:child_process' +import { readFileSync } from 'node:fs' +import { dirname, resolve } from 'node:path' +import { performance } from 'node:perf_hooks' +import { build } from 'esbuild' +import { buildCounterbalancedSchedule } from './counterbalanced-benchmark-schedule.mjs' + +const baseline = process.argv[2] +if (!baseline) { + throw new Error( + 'Usage: node config/scripts/mobile-review-note-index-benchmark.mjs ' + ) +} +async function load(file, contents, name) { + const result = await build({ + stdin: { contents, loader: 'ts', resolveDir: dirname(resolve(file)) }, + bundle: true, + platform: 'node', + format: 'esm', + write: false, + logLevel: 'silent', + tsconfigRaw: {} + }) + return ( + await import( + `data:text/javascript;base64,${Buffer.from(result.outputFiles[0].text).toString('base64')}` + ) + )[name] +} +const file = 'mobile/src/session/mobile-diff-review-queue.ts' +const name = 'buildMobileDiffReviewQueue' +const arms = { + before: await load( + file, + execFileSync('git', ['show', `${baseline}:${file}`], { encoding: 'utf8' }), + name + ), + after: await load(file, readFileSync(file, 'utf8'), name) +} +const results = [] +for (const [files, notes] of [ + [0, 1000], + [1, 1000], + [50, 0], + [50, 50], + [1000, 1000], + [1000, 5000] +]) { + const input = { + worktreeId: 'workspace', + statusEntries: Array.from({ length: files }, (_, index) => ({ + path: `file-${index}.ts`, + area: 'unstaged', + status: 'modified' + })), + branchEntries: [], + reviewState: { version: 1, files: {} }, + comments: Array.from({ length: notes }, (_, index) => ({ + id: `note-${index}`, + worktreeId: 'workspace', + filePath: `file-${index % Math.max(1, files)}.ts`, + body: 'note', + createdAt: 1, + lineNumber: 1, + side: 'modified', + ...(index % 5 === 0 ? { scope: 'staged' } : {}), + ...(index % 7 === 0 ? { sentAt: 1 } : {}) + })) + } + assert.deepEqual(arms.after(input), arms.before(input)) + const iterations = files < 100 ? 100 : 10 + function run(arm) { + const start = performance.now() + for (let i = 0; i < iterations; i++) { + arms[arm](input) + } + return (performance.now() - start) / iterations + } + const samples = { before: [], after: [] } + run('before') + run('after') + for (const pair of buildCounterbalancedSchedule(10, 'before', 'after')) { + for (const arm of pair) { + samples[arm].push(run(arm)) + } + } + function median(values) { + const sorted = [...values].sort((a, b) => a - b) + return (sorted[4] + sorted[5]) / 2 + } + results.push({ + files, + notes, + beforeMs: median(samples.before), + afterMs: median(samples.after), + samples + }) +} +console.log( + JSON.stringify({ baseline, node: process.version, platform: process.platform, results }, null, 2) +) diff --git a/mobile/src/session/mobile-diff-review-queue.ts b/mobile/src/session/mobile-diff-review-queue.ts index 68c8e6a17e4..b0b2cd13839 100644 --- a/mobile/src/session/mobile-diff-review-queue.ts +++ b/mobile/src/session/mobile-diff-review-queue.ts @@ -200,7 +200,8 @@ function statusEntryToQueueItem( function branchEntryToQueueItem( entry: MobileGitBranchChangeEntry, - input: BuildMobileDiffReviewQueueInput + input: BuildMobileDiffReviewQueueInput, + comments: readonly DiffComment[] ): MobileDiffReviewQueueItem { const scope: DiffReviewScope = 'branch' const key = createMobileDiffReviewFileKey(scope, 'branch', entry.path, entry.oldPath) @@ -208,7 +209,7 @@ function branchEntryToQueueItem( const reviewFileState = input.reviewState.files[key] const counts = queueNoteCounts( { filePath: entry.path, oldPath: entry.oldPath, scope, diffIdentity }, - input.comments + comments ) return { key, @@ -236,11 +237,28 @@ function branchEntryToQueueItem( export function buildMobileDiffReviewQueue( input: BuildMobileDiffReviewQueueInput ): MobileDiffReviewQueueItem[] { + let commentsForPath = (_path: string): readonly DiffComment[] => input.comments + if (input.comments.length > 0 && input.statusEntries.length + input.branchEntries.length > 1) { + const commentsByPath = new Map() + const emptyComments: readonly DiffComment[] = [] + for (const comment of input.comments) { + const path = comment.filePath + const comments = commentsByPath.get(path) + if (comments) { + comments.push(comment) + } else { + commentsByPath.set(path, [comment]) + } + } + commentsForPath = (path) => commentsByPath.get(path) ?? emptyComments + } const queue = [ ...input.statusEntries.map((entry) => - statusEntryToQueueItem(entry, input.comments, input.reviewState) + statusEntryToQueueItem(entry, commentsForPath(entry.path), input.reviewState) ), - ...input.branchEntries.map((entry) => branchEntryToQueueItem(entry, input)) + ...input.branchEntries.map((entry) => + branchEntryToQueueItem(entry, input, commentsForPath(entry.path)) + ) ] if (queue.length > 1) { const collator = new Intl.Collator(undefined, { numeric: true }) diff --git a/mobile/src/session/mobile-review-note-index.test.ts b/mobile/src/session/mobile-review-note-index.test.ts new file mode 100644 index 00000000000..d1a82bdbd43 --- /dev/null +++ b/mobile/src/session/mobile-review-note-index.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it } from 'vitest' +import type { DiffComment } from '../../../src/shared/diff-comment-types' +import { + buildMobileDiffReviewQueue, + type BuildMobileDiffReviewQueueInput +} from './mobile-diff-review-queue' + +function note(filePath: string, overrides: Partial = {}): DiffComment { + return { + id: filePath, + filePath, + worktreeId: 'workspace', + lineNumber: 1, + body: 'note', + createdAt: 1, + side: 'modified', + ...overrides + } +} +function input(comments: DiffComment[]): BuildMobileDiffReviewQueueInput { + return { + worktreeId: 'workspace', + statusEntries: [ + { path: 'new.ts', oldPath: 'old.ts', area: 'unstaged', status: 'renamed' }, + { path: 'new.ts', oldPath: 'other.ts', area: 'staged', status: 'renamed' }, + { path: 'NEW.ts', area: 'untracked', status: 'untracked' } + ], + branchEntries: [{ path: 'new.ts', oldPath: 'old.ts', status: 'modified' }], + comments, + reviewState: { version: 1, files: {} } + } +} + +describe('mobile review note indexing', () => { + it('preserves exact paths, legacy wildcards, rename/scope filters, and stale/unsent counts', () => { + const comments = [ + note('new.ts'), + note('new.ts', { scope: 'staged', sentAt: 0 }), + note('new.ts', { scope: 'branch', oldPath: 'old.ts', diffIdentity: 'stale' }), + note('new.ts', { oldPath: 'other.ts' }), + note('new.ts', { oldPath: 'missing.ts' }), + note('new.ts', { source: 'markdown' }), + note('NEW.ts'), + note('elsewhere.ts') + ] + const options = input(comments) + const first = buildMobileDiffReviewQueue(options) + expect( + Object.fromEntries( + first.map(({ scope, filePath, noteCount, unsentNoteCount, staleNoteCount }) => [ + `${scope}:${filePath}`, + [noteCount, unsentNoteCount, staleNoteCount] + ]) + ) + ).toEqual({ + 'unstaged:NEW.ts': [1, 1, 0], + 'unstaged:new.ts': [1, 1, 0], + 'staged:new.ts': [3, 2, 0], + 'branch:new.ts': [2, 2, 1] + }) + comments[0].sentAt = 2 + expect( + buildMobileDiffReviewQueue(options).find((item) => item.scope === 'staged')?.unsentNoteCount + ).toBe(1) + expect( + buildMobileDiffReviewQueue({ ...options, comments: [] }).every((item) => item.noteCount === 0) + ).toBe(true) + }) + + it('reads comment paths in proportion to comments and matching rows, not their product', () => { + let reads = 0 + const comments = Array.from({ length: 1000 }, (_, index) => ({ + ...note(`file-${index}.ts`), + get filePath() { + reads++ + return `file-${index}.ts` + } + })) + const options = input(comments) + options.statusEntries = comments.map((_, index) => ({ + path: `file-${index}.ts`, + area: 'unstaged', + status: 'modified' + })) + options.branchEntries = [] + const queue = buildMobileDiffReviewQueue(options) + expect(queue.every((item) => item.noteCount === 1 && item.unsentNoteCount === 1)).toBe(true) + expect(reads).toBe(2000) + }) + + it('skips indexing when there are no rows or just one row', () => { + let reads = 0 + const comments = [ + { + ...note('one.ts'), + get filePath() { + reads++ + return 'one.ts' + } + } + ] + const options = { ...input(comments), statusEntries: [], branchEntries: [] } + expect(buildMobileDiffReviewQueue(options)).toEqual([]) + expect(reads).toBe(0) + expect( + buildMobileDiffReviewQueue({ + ...options, + branchEntries: [{ path: 'one.ts', status: 'modified' }] + })[0].noteCount + ).toBe(1) + expect(reads).toBe(1) + }) +})