perf: stop review acknowledgement summaries at the first readable line (#20263)

config/scripts/review-ack-first-line-benchmark.mjs

@@ -0,0 +1,86 @@
+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] ?? '20ab9950654'
+const file = 'src/renderer/src/components/right-sidebar/pr-comment-fixing-reply-body.ts'
+async function load(contents) {
+  const result = await build({
+    stdin: { contents, loader: 'ts', resolveDir: dirname(resolve(file)) },
+    bundle: true,
+    platform: 'node',
+    format: 'esm',
+    write: false
+  })
+  return import(
+    `data:text/javascript;base64,${Buffer.from(result.outputFiles[0].text).toString('base64')}`
+  )
+}
+const before = await load(
+  execFileSync('git', ['show', `${baseline}:${file}`], { encoding: 'utf8' })
+)
+const after = await load(readFileSync(file, 'utf8'))
+const results = []
+for (const [name, body] of [
+  ['short', 'Please rename this variable.'],
+  [
+    '100-lines',
+    `<!-- metadata -->\n## Review findings\n${'Code sample with   details\n'.repeat(100)}`
+  ],
+  [
+    '10000-lines',
+    `<!-- metadata -->\n## Review findings\n${'Code sample with   details\n'.repeat(10000)}`
+  ],
+  ['blank-10000-lines', '# > * - _ `\n'.repeat(10000)]
+]) {
+  const comments = Array.from({ length: 10 }, (_, id) => ({
+    id,
+    author: 'reviewer',
+    authorAvatarUrl: '',
+    createdAt: '',
+    url: '',
+    body
+  }))
+  const arms = { before, after }
+  assert.equal(
+    after.buildPRCommentBatchConversationReplyBody(comments),
+    before.buildPRCommentBatchConversationReplyBody(comments)
+  )
+  const run = (arm) => {
+    global.gc?.()
+    const start = performance.now()
+    const cpuStart = process.cpuUsage()
+    for (let i = 0; i < 10; i++) {
+      arms[arm].buildPRCommentBatchConversationReplyBody(comments)
+    }
+    const cpu = process.cpuUsage(cpuStart)
+    return { ms: (performance.now() - start) / 10, cpuMs: (cpu.user + cpu.system) / 10000 }
+  }
+  run('before')
+  run('after')
+  const samples = { before: [], after: [] }
+  for (const pair of buildCounterbalancedSchedule(10, 'before', 'after')) {
+    for (const arm of pair) {
+      samples[arm].push(run(arm))
+    }
+  }
+  const median = (values) => {
+    const sorted = [...values].sort((a, b) => a - b)
+    return (sorted[4] + sorted[5]) / 2
+  }
+  results.push({
+    name,
+    beforeCpuMs: median(samples.before.map((s) => s.cpuMs)),
+    afterCpuMs: median(samples.after.map((s) => s.cpuMs)),
+    beforeMs: median(samples.before.map((s) => s.ms)),
+    afterMs: median(samples.after.map((s) => s.ms)),
+    samples
+  })
+}
+console.log(
+  JSON.stringify({ baseline, node: process.version, platform: process.platform, results }, null, 2)
+)

src/renderer/src/components/right-sidebar/pr-comment-ack-summary.test.ts

@@ -0,0 +1,64 @@
+import { afterEach, describe, expect, it, vi } from 'vitest'
+import type { PRComment } from '../../../../shared/github/comment-types'
+import { describePRCommentAckTarget } from './pr-comment-fixing-reply-body'
+
+const comment = (body: string): PRComment => ({
+  id: 1,
+  author: 'alice',
+  authorAvatarUrl: '',
+  body,
+  createdAt: '',
+  url: ''
+})
+function originalSummary(body: string): string {
+  const line = body
+    .replace(/<!--[\s\S]*?-->/g, ' ')
+    .split('\n')
+    .map((line) =>
+      line
+        .replace(/^[\s>#*\-_`]+/, '')
+        .replace(/\s+/g, ' ')
+        .trim()
+    )
+    .find((line) => line.length > 0)
+  if (!line) {
+    return 'comment'
+  }
+  return `comment — ${line.length > 72 ? `${line.slice(0, 71).trimEnd()}…` : line}`
+}
+afterEach(() => vi.restoreAllMocks())
+
+describe('review acknowledgement summary', () => {
+  it.each([
+    '',
+    '\n\r\n\t',
+    '# > ** _ - `\n\nReadable',
+    '<!-- first\nsecond -->\n## Hello\r\nignored',
+    'one<!-- hidden\nline -->two\nignored',
+    '<!-- unclosed\nreadable',
+    '<!-- outer <!-- inner -->visible -->',
+    'a\rb\nc',
+    '\u00a0##\u2028Hello\u2029world',
+    'a'.repeat(71),
+    'a'.repeat(72),
+    'a'.repeat(73),
+    `${'a'.repeat(70)}  b`,
+    `${'a'.repeat(70)}😀tail`,
+    '\n<!-- only metadata -->\n',
+    'first\n<!-- tail\ncomment -->\nlast'
+  ])('preserves the existing label for %j', (body) => {
+    expect(describePRCommentAckTarget(comment(body))).toBe(originalSummary(body))
+  })
+
+  it('skips tail normalization and full-document line splitting', () => {
+    const input = comment(`## Heading\n${'tail with   whitespace\n'.repeat(10_000)}`)
+    const replace = vi.spyOn(String.prototype, 'replace')
+    const split = vi.spyOn(String.prototype, 'split')
+    const actual = describePRCommentAckTarget(input)
+    const replaces = replace.mock.calls.length
+    const splits = split.mock.calls.length
+    expect(actual).toBe('comment — Heading')
+    expect(replaces).toBe(3)
+    expect(splits).toBe(0)
+  })
+})

src/renderer/src/components/right-sidebar/pr-comment-fixing-reply-body.ts

@@ -24,22 +24,26 @@ const ACK_SNIPPET_MAX_LENGTH = 72
 
 /** First readable line of a comment body, minus HTML comments and markdown markers. */
 function summarizePRCommentBody(body: string): string {
-  const line = body
-    .replace(/<!--[\s\S]*?-->/g, ' ')
-    .split('\n')
-    .map((candidate) =>
-      candidate
-        .replace(/^[\s>#*\-_`]+/, '')
-        .replace(/\s+/g, ' ')
-        .trim()
-    )
-    .find((candidate) => candidate.length > 0)
-  if (!line) {
-    return ''
+  const cleaned = body.replace(/<!--[\s\S]*?-->/g, ' ')
+  let start = 0
+  while (start <= cleaned.length) {
+    const newline = cleaned.indexOf('\n', start)
+    const line = cleaned
+      .slice(start, newline === -1 ? cleaned.length : newline)
+      .replace(/^[\s>#*\-_`]+/, '')
+      .replace(/\s+/g, ' ')
+      .trim()
+    if (line) {
+      return line.length > ACK_SNIPPET_MAX_LENGTH
+        ? `${line.slice(0, ACK_SNIPPET_MAX_LENGTH - 1).trimEnd()}…`
+        : line
+    }
+    if (newline === -1) {
+      break
+    }
+    start = newline + 1
   }
-  return line.length > ACK_SNIPPET_MAX_LENGTH
-    ? `${line.slice(0, ACK_SNIPPET_MAX_LENGTH - 1).trimEnd()}…`
-    : line
+  return ''
 }
 
 /** Short "what this was" label so the batched reply names each item without quoting it whole. */