diff --git a/config/scripts/markdown-note-line-scan-benchmark.mjs b/config/scripts/markdown-note-line-scan-benchmark.mjs new file mode 100644 index 00000000000..2b6565278d9 --- /dev/null +++ b/config/scripts/markdown-note-line-scan-benchmark.mjs @@ -0,0 +1,81 @@ +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 sourcePath = 'src/renderer/src/lib/markdown-review-notes.ts' +const baseline = process.argv[2] ?? '20ab9950654' +async function load(contents) { + const result = await build({ + stdin: { contents, loader: 'ts', resolveDir: dirname(resolve(sourcePath)) }, + 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}:${sourcePath}`], { encoding: 'utf8' }) +) +const after = await load(readFileSync(sourcePath, 'utf8')) +const results = [] +for (const [name, lineCount, width, count, iterations] of [ + ['small', 20, 40, 1, 1000], + ['long-lines', 1000, 1000, 20, 3], + ['many-lines', 20000, 80, 20, 3], + ['early-note', 20000, 80, 1, 1000] +]) { + const content = Array.from({ length: lineCount }, (_, i) => `${i}: ${'x'.repeat(width)}`).join( + '\r\n' + ) + const notes = Array.from({ length: count }, (_, i) => ({ + id: `${i}`, + worktreeId: 'bench', + filePath: 'README.md', + source: 'markdown', + lineNumber: name === 'early-note' ? 2 : lineCount - i, + body: 'Clarify this line', + createdAt: i, + side: 'modified' + })) + assert.equal( + after.formatMarkdownReviewNotes(notes, content), + before.formatMarkdownReviewNotes(notes, content) + ) + const arms = { before, after } + const samples = { before: [], after: [] } + function run(arm) { + const start = performance.now() + for (let i = 0; i < iterations; i++) { + arms[arm].formatMarkdownReviewNotes(notes, content) + } + return (performance.now() - start) / iterations + } + for (let i = 0; i < 6; i++) { + run('before') + run('after') + } + for (const pair of buildCounterbalancedSchedule(12, 'before', 'after')) { + for (const arm of pair) { + samples[arm].push(run(arm)) + } + } + const median = (xs) => xs.sort((a, b) => a - b)[Math.floor(xs.length / 2)] + results.push({ + name, + lineCount, + width, + count, + beforeMs: median(samples.before), + afterMs: median(samples.after) + }) +} +console.log( + JSON.stringify({ node: process.version, platform: process.platform, baseline, results }, null, 2) +) diff --git a/src/renderer/src/lib/markdown-review-notes.test.ts b/src/renderer/src/lib/markdown-review-notes.test.ts index 80c40089e1f..8de37bd7051 100644 --- a/src/renderer/src/lib/markdown-review-notes.test.ts +++ b/src/renderer/src/lib/markdown-review-notes.test.ts @@ -94,6 +94,44 @@ describe('markdown review notes', () => { expect(charCodeAt.mock.calls.length).toBeLessThan(64) }) + it('preserves empty, trailing, and mixed line endings across excerpt ranges', () => { + for (const content of [ + '', + 'one', + '\n', + 'one\n', + 'one\r\n\r\ntwo\r', + 'one\rtwo', + Array.from({ length: 15 }, (_, i) => `line ${i}`).join('\n') + ]) { + const sourceLines = content + .split('\n') + .map((line) => (line.endsWith('\r') ? line.slice(0, -1) : line)) + for (let start = -1; start <= sourceLines.length + 1; start += 1) { + for (let end = start; end <= sourceLines.length + 1; end += 1) { + const first = Math.max(1, start) + const selected = sourceLines.slice(first - 1, Math.max(first, end)) + const bounded = + selected.length <= 8 + ? selected + : [...selected.slice(0, 4), '...', ...selected.slice(-4)] + expect( + getMarkdownReviewExcerpt(content, note({ startLine: start, lineNumber: end })) + ).toBe(bounded.map((line) => `> ${line}`).join('\n')) + } + } + } + }) + + it('finds late excerpts without checking every prefix character in JavaScript', () => { + const content = `${'x'.repeat(100_000)}\r\ntarget\nignored` + const charCodeAt = vi.spyOn(String.prototype, 'charCodeAt') + const excerpt = getMarkdownReviewExcerpt(content, note({ lineNumber: 2 })) + const checks = charCodeAt.mock.calls.length + expect(excerpt).toBe('> target') + expect(checks).toBeLessThanOrEqual(2) + }) + it('prefers exact selected text for card highlights', () => { const highlighted = getMarkdownReviewHighlightedText( 'one\ntwo broad line\nthree', diff --git a/src/renderer/src/lib/markdown-review-notes.ts b/src/renderer/src/lib/markdown-review-notes.ts index c0831ef8edd..2c8bcb59d26 100644 --- a/src/renderer/src/lib/markdown-review-notes.ts +++ b/src/renderer/src/lib/markdown-review-notes.ts @@ -93,10 +93,9 @@ function forEachMarkdownReviewLine( ): void { let lineStart = 0 let lineNumber = 1 - for (let index = 0; index <= content.length; index += 1) { - if (index < content.length && content.charCodeAt(index) !== 10) { - continue - } + while (lineStart <= content.length) { + const newline = content.indexOf('\n', lineStart) + const index = newline === -1 ? content.length : newline const lineEnd = index > lineStart && content.charCodeAt(index - 1) === 13 ? index - 1 : index if (visit(content.slice(lineStart, lineEnd), lineNumber) === false) { return