mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 00:02:31 +00:00
perf(editor): reuse Markdown source blocks while positioning review notes (#18895)
This commit is contained in:
@@ -139,7 +139,7 @@ export function getRichMarkdownAnnotationHighlightRangesForComment(
|
||||
comment: DiffComment,
|
||||
markdownSourceLineOffset: number,
|
||||
// Why optional: callers looping over comments pass one shared build.
|
||||
prebuiltBlocks?: RichMarkdownCommentBlock[]
|
||||
prebuiltBlocks?: readonly RichMarkdownCommentBlock[]
|
||||
): RichMarkdownAnnotationHighlightRange[] {
|
||||
const blocks = prebuiltBlocks ?? buildRichMarkdownCommentBlocks(editor)
|
||||
const selectedText = comment.selectedText?.trim()
|
||||
@@ -192,13 +192,15 @@ export function getRichMarkdownCommentAnchorTop(
|
||||
block: RichMarkdownCommentBlock,
|
||||
containerRect: DOMRect,
|
||||
containerScrollTop: number,
|
||||
markdownSourceLineOffset: number
|
||||
markdownSourceLineOffset: number,
|
||||
prebuiltBlocks?: readonly RichMarkdownCommentBlock[]
|
||||
): number | null {
|
||||
try {
|
||||
const ranges = getRichMarkdownAnnotationHighlightRangesForComment(
|
||||
editor,
|
||||
comment,
|
||||
markdownSourceLineOffset
|
||||
markdownSourceLineOffset,
|
||||
prebuiltBlocks
|
||||
)
|
||||
// Why: range notes should sort by the start of the selected text. Anchoring
|
||||
// to the end puts overlapping ranges with the same final line in creation
|
||||
|
||||
@@ -1,9 +1,7 @@
|
||||
import type { Editor } from '@tiptap/react'
|
||||
import type { DiffComment } from '../../../../shared/diff-comment-types'
|
||||
import {
|
||||
buildRichMarkdownCommentBlocks,
|
||||
getRichMarkdownCommentAnchorTop
|
||||
} from './rich-markdown-review-annotations'
|
||||
import { getRichMarkdownCommentAnchorTop } from './rich-markdown-review-annotations'
|
||||
import { getRichMarkdownReviewRailBlocks } from './rich-markdown-review-rail-blocks'
|
||||
import {
|
||||
stackRichMarkdownReviewNotePositions,
|
||||
type RichMarkdownReviewNotePosition
|
||||
@@ -23,7 +21,7 @@ export function measureRichMarkdownReviewNotePositions({
|
||||
markdownSourceLineOffset
|
||||
}: MeasureRichMarkdownReviewNotePositionsOptions): RichMarkdownReviewNotePosition[] {
|
||||
const containerRect = container.getBoundingClientRect()
|
||||
const blocks = buildRichMarkdownCommentBlocks(editor)
|
||||
const blocks = getRichMarkdownReviewRailBlocks(editor)
|
||||
const nextPositions = markdownComments
|
||||
.map((comment): RichMarkdownReviewNotePosition | null => {
|
||||
const bodyLineNumber = Math.max(1, comment.lineNumber - markdownSourceLineOffset)
|
||||
@@ -39,7 +37,8 @@ export function measureRichMarkdownReviewNotePositions({
|
||||
block,
|
||||
containerRect,
|
||||
container.scrollTop,
|
||||
markdownSourceLineOffset
|
||||
markdownSourceLineOffset,
|
||||
blocks
|
||||
)
|
||||
return top === null ? null : { comment, top }
|
||||
})
|
||||
|
||||
@@ -0,0 +1,154 @@
|
||||
// @vitest-environment happy-dom
|
||||
// Run: ORCA_REVIEW_RAIL_BENCH=1 pnpm test src/renderer/src/components/editor/rich-markdown-review-rail-benchmark.test.ts
|
||||
import { expect, it, vi } from 'vitest'
|
||||
import { Editor as TiptapEditor } from '@tiptap/core'
|
||||
import { createRichMarkdownExtensions } from './rich-markdown-extensions'
|
||||
import { createRichMarkdownEditorCodec } from './rich-markdown-source-transport'
|
||||
import { measureRichMarkdownReviewNotePositions } from './rich-markdown-review-note-positioning'
|
||||
|
||||
// Baseline measurement body from the parent revision, before sharing source blocks.
|
||||
import type { Editor } from '@tiptap/react'
|
||||
import type { DiffComment } from '../../../../shared/diff-comment-types'
|
||||
import {
|
||||
buildRichMarkdownCommentBlocks,
|
||||
getRichMarkdownCommentAnchorTop
|
||||
} from './rich-markdown-review-annotations'
|
||||
import {
|
||||
stackRichMarkdownReviewNotePositions,
|
||||
type RichMarkdownReviewNotePosition
|
||||
} from './rich-markdown-review-note-layout'
|
||||
|
||||
type MeasureRichMarkdownReviewNotePositionsOptions = {
|
||||
container: HTMLDivElement
|
||||
editor: Editor
|
||||
markdownComments: DiffComment[]
|
||||
markdownSourceLineOffset: number
|
||||
}
|
||||
|
||||
function measureBaseline({
|
||||
container,
|
||||
editor,
|
||||
markdownComments,
|
||||
markdownSourceLineOffset
|
||||
}: MeasureRichMarkdownReviewNotePositionsOptions): RichMarkdownReviewNotePosition[] {
|
||||
const containerRect = container.getBoundingClientRect()
|
||||
const blocks = buildRichMarkdownCommentBlocks(editor)
|
||||
const nextPositions = markdownComments
|
||||
.map((comment): RichMarkdownReviewNotePosition | null => {
|
||||
const bodyLineNumber = Math.max(1, comment.lineNumber - markdownSourceLineOffset)
|
||||
const block = blocks.find(
|
||||
(candidate) => candidate.startLine <= bodyLineNumber && bodyLineNumber <= candidate.endLine
|
||||
)
|
||||
if (!block) {
|
||||
return null
|
||||
}
|
||||
const top = getRichMarkdownCommentAnchorTop(
|
||||
editor,
|
||||
comment,
|
||||
block,
|
||||
containerRect,
|
||||
container.scrollTop,
|
||||
markdownSourceLineOffset
|
||||
)
|
||||
return top === null ? null : { comment, top }
|
||||
})
|
||||
.filter((position): position is RichMarkdownReviewNotePosition => position !== null)
|
||||
return stackRichMarkdownReviewNotePositions(
|
||||
nextPositions,
|
||||
measureReviewNoteHeights(container, nextPositions)
|
||||
)
|
||||
}
|
||||
|
||||
function measureReviewNoteHeights(
|
||||
container: HTMLDivElement,
|
||||
positions: RichMarkdownReviewNotePosition[]
|
||||
): Map<string, number> {
|
||||
const measuredHeights = new Map<string, number>()
|
||||
for (const pos of positions) {
|
||||
const el = container.querySelector(`[data-rich-markdown-review-note-id="${pos.comment.id}"]`)
|
||||
if (el) {
|
||||
measuredHeights.set(pos.comment.id, el.getBoundingClientRect().height)
|
||||
}
|
||||
}
|
||||
return measuredHeights
|
||||
}
|
||||
|
||||
it.skipIf(process.env.ORCA_REVIEW_RAIL_BENCH !== '1')(
|
||||
'benchmarks full review rail measurements',
|
||||
() => {
|
||||
for (const blockCount of [250, 1000]) {
|
||||
const editor = new TiptapEditor({
|
||||
element: document.createElement('div'),
|
||||
extensions: createRichMarkdownExtensions({ codec: createRichMarkdownEditorCodec() }),
|
||||
content: {
|
||||
type: 'doc',
|
||||
content: Array.from({ length: blockCount }, (_, index) => ({
|
||||
type: 'paragraph',
|
||||
content: [{ type: 'text', text: `Paragraph ${index} with reviewable content.` }]
|
||||
}))
|
||||
}
|
||||
})
|
||||
try {
|
||||
vi.spyOn(editor.view, 'coordsAtPos').mockReturnValue({
|
||||
top: 100,
|
||||
bottom: 120,
|
||||
left: 0,
|
||||
right: 10
|
||||
})
|
||||
const container = document.createElement('div')
|
||||
const markdownComments: DiffComment[] = Array.from({ length: 5 }, (_, index) => ({
|
||||
id: `note-${index}`,
|
||||
worktreeId: 'workspace',
|
||||
filePath: 'notes.md',
|
||||
source: 'markdown',
|
||||
lineNumber: 1 + index * 40,
|
||||
body: 'Review',
|
||||
createdAt: index,
|
||||
side: 'modified'
|
||||
}))
|
||||
const args = { editor, container, markdownComments, markdownSourceLineOffset: 0 }
|
||||
const serialize = vi.spyOn(editor.markdown!, 'serialize')
|
||||
const baseline = measureBaseline(args)
|
||||
const baselineCalls = serialize.mock.calls.length
|
||||
serialize.mockClear()
|
||||
expect(measureRichMarkdownReviewNotePositions(args)).toEqual(baseline)
|
||||
const coldCalls = serialize.mock.calls.length
|
||||
serialize.mockClear()
|
||||
expect(measureRichMarkdownReviewNotePositions(args)).toEqual(baseline)
|
||||
const warmCalls = serialize.mock.calls.length
|
||||
serialize.mockRestore()
|
||||
const time = (run: () => unknown) => {
|
||||
const start = performance.now()
|
||||
for (let index = 0; index < 20; index++) {
|
||||
run()
|
||||
}
|
||||
return (performance.now() - start) / 20
|
||||
}
|
||||
const before: number[] = []
|
||||
const after: number[] = []
|
||||
for (let round = 0; round < 5; round++) {
|
||||
before.push(time(() => measureBaseline(args)))
|
||||
after.push(time(() => measureRichMarkdownReviewNotePositions(args)))
|
||||
}
|
||||
const median = (values: number[]) => values.sort((a, b) => a - b)[2]!
|
||||
const result = {
|
||||
blockCount,
|
||||
comments: markdownComments.length,
|
||||
beforeMs: median(before),
|
||||
afterMs: median(after),
|
||||
baselineCalls,
|
||||
coldCalls,
|
||||
warmCalls
|
||||
}
|
||||
process.stdout.write(`${JSON.stringify(result)}\n`)
|
||||
expect(baselineCalls).toBe((2 * blockCount - 1) * 6)
|
||||
expect(coldCalls).toBe(2 * blockCount - 1)
|
||||
expect(warmCalls).toBe(0)
|
||||
} finally {
|
||||
editor.destroy()
|
||||
vi.restoreAllMocks()
|
||||
}
|
||||
}
|
||||
},
|
||||
120_000
|
||||
)
|
||||
@@ -0,0 +1,139 @@
|
||||
// @vitest-environment happy-dom
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import { Editor, type JSONContent } from '@tiptap/core'
|
||||
import type { DiffComment } from '../../../../shared/diff-comment-types'
|
||||
import { createRichMarkdownExtensions } from './rich-markdown-extensions'
|
||||
import { createRichMarkdownEditorCodec } from './rich-markdown-source-transport'
|
||||
import { buildRichMarkdownCommentBlocks } from './rich-markdown-review-annotations'
|
||||
import { getRichMarkdownReviewRailBlocks } from './rich-markdown-review-rail-blocks'
|
||||
import { measureRichMarkdownReviewNotePositions } from './rich-markdown-review-note-positioning'
|
||||
|
||||
const editors: Editor[] = []
|
||||
|
||||
function createEditor(
|
||||
content: string | JSONContent = '# Heading\n\nFirst paragraph\n\n- One\n- Two'
|
||||
) {
|
||||
const editor = new Editor({
|
||||
element: document.createElement('div'),
|
||||
extensions: createRichMarkdownExtensions({ codec: createRichMarkdownEditorCodec() }),
|
||||
content,
|
||||
...(typeof content === 'string' ? { contentType: 'markdown' as const } : {})
|
||||
})
|
||||
editors.push(editor)
|
||||
// Settle the trailing-node plugin before measuring selection-only transactions.
|
||||
editor.view.dispatch(editor.state.tr.setMeta('addToHistory', false))
|
||||
return editor
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
for (const editor of editors.splice(0)) {
|
||||
editor.destroy()
|
||||
}
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
describe('review rail source block reuse', () => {
|
||||
it.each([
|
||||
'```ts\nconst value = 1\n\nvalue++\n```\n\nAfter code',
|
||||
'| First | Second |\n| --- | --- |\n| one | two |\n\nAfter table',
|
||||
'<details><summary>Toggle</summary>\n\nInside\n\n</details>\n\nAfter toggle'
|
||||
])('preserves multiline block boundaries for %s', (source) => {
|
||||
const editor = createEditor(source)
|
||||
expect(getRichMarkdownReviewRailBlocks(editor)).toEqual(buildRichMarkdownCommentBlocks(editor))
|
||||
})
|
||||
|
||||
it('preserves block lines and reuses them after selection-only transactions', () => {
|
||||
const editor = createEditor()
|
||||
const expected = buildRichMarkdownCommentBlocks(editor)
|
||||
const serialize = vi.spyOn(editor.markdown!, 'serialize')
|
||||
const blocks = getRichMarkdownReviewRailBlocks(editor)
|
||||
expect(blocks).toEqual(expected)
|
||||
expect(serialize).toHaveBeenCalledTimes(2 * editor.state.doc.childCount - 1)
|
||||
serialize.mockClear()
|
||||
editor.commands.setTextSelection(3)
|
||||
expect(getRichMarkdownReviewRailBlocks(editor)).toBe(blocks)
|
||||
expect(serialize).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('rebuilds after edits and undo while keeping editors isolated', () => {
|
||||
const editor = createEditor()
|
||||
const original = getRichMarkdownReviewRailBlocks(editor)
|
||||
editor.commands.insertContentAt(1, 'changed\ntext')
|
||||
const changed = getRichMarkdownReviewRailBlocks(editor)
|
||||
expect(changed).not.toBe(original)
|
||||
expect(changed).toEqual(buildRichMarkdownCommentBlocks(editor))
|
||||
editor.commands.undo()
|
||||
expect(getRichMarkdownReviewRailBlocks(editor)).toEqual(original)
|
||||
expect(getRichMarkdownReviewRailBlocks(createEditor())).not.toBe(original)
|
||||
})
|
||||
|
||||
it('invalidates an in-place serializer replacement and a manager replacement', () => {
|
||||
const editor = createEditor()
|
||||
const original = getRichMarkdownReviewRailBlocks(editor)
|
||||
const serialize = editor.markdown!.serialize.bind(editor.markdown)
|
||||
vi.spyOn(editor.markdown!, 'serialize').mockImplementation(
|
||||
(content) => `${serialize(content)}\nextra line`
|
||||
)
|
||||
const changed = getRichMarkdownReviewRailBlocks(editor)
|
||||
expect(changed).not.toEqual(original)
|
||||
expect(changed).toEqual(buildRichMarkdownCommentBlocks(editor))
|
||||
editor.markdown = createEditor().markdown
|
||||
expect(getRichMarkdownReviewRailBlocks(editor)).toEqual(original)
|
||||
})
|
||||
|
||||
it('does not reuse fallback lines after a missing serializer becomes available', () => {
|
||||
const editor = createEditor()
|
||||
const markdown = editor.markdown
|
||||
editor.markdown = undefined
|
||||
const fallback = getRichMarkdownReviewRailBlocks(editor)
|
||||
expect(fallback).toEqual(buildRichMarkdownCommentBlocks(editor))
|
||||
editor.markdown = markdown
|
||||
expect(getRichMarkdownReviewRailBlocks(editor)).not.toEqual(fallback)
|
||||
expect(getRichMarkdownReviewRailBlocks(editor)).toEqual(buildRichMarkdownCommentBlocks(editor))
|
||||
})
|
||||
|
||||
it('avoids serialization for every comment and repeated scroll while refreshing geometry', () => {
|
||||
const editor = createEditor()
|
||||
let sourceTop = 100
|
||||
const coords = vi.spyOn(editor.view, 'coordsAtPos').mockImplementation(() => ({
|
||||
top: sourceTop,
|
||||
bottom: sourceTop + 20,
|
||||
left: 0,
|
||||
right: 10
|
||||
}))
|
||||
const container = document.createElement('div')
|
||||
const comment: DiffComment = {
|
||||
id: 'note',
|
||||
worktreeId: 'workspace',
|
||||
filePath: 'notes.md',
|
||||
source: 'markdown',
|
||||
lineNumber: 1,
|
||||
body: 'Review',
|
||||
createdAt: 1,
|
||||
side: 'modified'
|
||||
}
|
||||
const markdownComments = Array.from({ length: 5 }, (_, index) => ({
|
||||
...comment,
|
||||
id: `note-${index}`,
|
||||
selectedText: index === 0 ? 'Heading' : undefined
|
||||
}))
|
||||
const serialize = vi.spyOn(editor.markdown!, 'serialize')
|
||||
const measure = () =>
|
||||
measureRichMarkdownReviewNotePositions({
|
||||
editor,
|
||||
container,
|
||||
markdownComments,
|
||||
markdownSourceLineOffset: 0
|
||||
})
|
||||
expect(measure()[0]?.top).toBe(100)
|
||||
expect(serialize).toHaveBeenCalledTimes(2 * editor.state.doc.childCount - 1)
|
||||
serialize.mockClear()
|
||||
sourceTop = 200
|
||||
container.scrollTop = 30
|
||||
for (let index = 0; index < 60; index++) {
|
||||
expect(measure()[0]?.top).toBe(230)
|
||||
}
|
||||
expect(serialize).not.toHaveBeenCalled()
|
||||
expect(coords).toHaveBeenCalledTimes(61 * markdownComments.length)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,31 @@
|
||||
import type { Editor } from '@tiptap/core'
|
||||
import {
|
||||
buildRichMarkdownCommentBlocks,
|
||||
type RichMarkdownCommentBlock
|
||||
} from './rich-markdown-review-annotations'
|
||||
|
||||
type ReviewRailBlocks = {
|
||||
doc: Editor['state']['doc']
|
||||
markdown: Editor['markdown']
|
||||
serialize: NonNullable<Editor['markdown']>['serialize'] | undefined
|
||||
blocks: readonly RichMarkdownCommentBlock[]
|
||||
}
|
||||
|
||||
// Keep only the current document per editor; scrolling changes geometry, not source lines.
|
||||
const blocksByEditor = new WeakMap<Editor, ReviewRailBlocks>()
|
||||
|
||||
export function getRichMarkdownReviewRailBlocks(
|
||||
editor: Editor
|
||||
): readonly RichMarkdownCommentBlock[] {
|
||||
const doc = editor.state.doc
|
||||
const markdown = editor.markdown
|
||||
const serialize = markdown?.serialize
|
||||
const cached = blocksByEditor.get(editor)
|
||||
if (cached?.doc === doc && cached.markdown === markdown && cached.serialize === serialize) {
|
||||
return cached.blocks
|
||||
}
|
||||
|
||||
const blocks = buildRichMarkdownCommentBlocks(editor)
|
||||
blocksByEditor.set(editor, { doc, markdown, serialize, blocks })
|
||||
return blocks
|
||||
}
|
||||
Reference in New Issue
Block a user