perf(mobile): index review notes by file before building the queue (#20228)

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 <baseline-ref>'
+  )
+}
+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)
+)

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<string, DiffComment[]>()
+    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 })

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> = {}): 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)
+  })
+})