diff --git a/config/scripts/review-ack-first-line-benchmark.mjs b/config/scripts/review-ack-first-line-benchmark.mjs new file mode 100644 index 00000000000..57069a07e80 --- /dev/null +++ b/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', + `\n## Review findings\n${'Code sample with details\n'.repeat(100)}` + ], + [ + '10000-lines', + `\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) +) diff --git a/src/renderer/src/components/right-sidebar/pr-comment-ack-summary.test.ts b/src/renderer/src/components/right-sidebar/pr-comment-ack-summary.test.ts new file mode 100644 index 00000000000..c4b2f83ac83 --- /dev/null +++ b/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(//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', + '\n## Hello\r\nignored', + 'onetwo\nignored', + '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\n', + 'first\n\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) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/pr-comment-fixing-reply-body.ts b/src/renderer/src/components/right-sidebar/pr-comment-fixing-reply-body.ts index 5122a653ef9..5a12b6bc17a 100644 --- a/src/renderer/src/components/right-sidebar/pr-comment-fixing-reply-body.ts +++ b/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(//g, ' ') - .split('\n') - .map((candidate) => - candidate - .replace(/^[\s>#*\-_`]+/, '') - .replace(/\s+/g, ' ') - .trim() - ) - .find((candidate) => candidate.length > 0) - if (!line) { - return '' + const cleaned = body.replace(//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. */