feat(diff-comments): handle upward drags and gutter-only presses

- Preserve anchor/focus directionality in selections to handle upward drags correctly
- Restrict gutter presses to line numbers only, leaving fold chevrons to Monaco
- Prevent add-note chord from opening composer while a gutter drag is in progress
This commit is contained in:
Jinjing
2026-09-19 01:03:51 -07:00
parent d859a43ba5
commit c02b1cf94e
10 changed files with 275 additions and 43 deletions
@@ -26,6 +26,7 @@ type AddButtonOverlayArgs = {
export type DiffCommentAddButtonOverlayHandle = {
dispose: () => void
setPendingRange: DiffCommentRangeDragHandle['setPendingRange']
isDragging: DiffCommentRangeDragHandle['isDragging']
}
export function installDiffCommentAddButtonOverlay({
@@ -153,6 +154,7 @@ export function installDiffCommentAddButtonOverlay({
editorDomNode.classList.remove('orca-diff-comment-range-dragging')
plus.remove()
},
setPendingRange: rangeDrag.setPendingRange
setPendingRange: rangeDrag.setPendingRange,
isDragging: rangeDrag.isDragging
}
}
@@ -4,7 +4,8 @@ import { installEditorAddReviewNoteShortcut } from '../editor/editor-shortcuts'
import { getDiffCommentPopoverTop } from './diff-comment-popover-position'
import {
clampFocusLineToCommentable,
getSelectionLineRange,
getSelectionAnchorFocus,
orderLineRange,
toDiffCommentLineTarget,
type DiffCommentLineTarget
} from './diff-comment-line-range'
@@ -30,14 +31,16 @@ export function resolveDiffCommentShortcutTarget(
if (!selection) {
return null
}
const range = getSelectionLineRange(selection)
if (commentableLineSet !== null && !commentableLineSet.has(range.startLine)) {
const { anchorLine, focusLine } = getSelectionAnchorFocus(selection)
if (commentableLineSet !== null && !commentableLineSet.has(anchorLine)) {
return null
}
return toDiffCommentLineTarget({
startLine: range.startLine,
endLine: clampFocusLineToCommentable(range.startLine, range.endLine, commentableLineSet)
})
return toDiffCommentLineTarget(
orderLineRange(
anchorLine,
clampFocusLineToCommentable(anchorLine, focusLine, commentableLineSet)
)
)
}
export function installDiffCommentAddNoteShortcut({
@@ -38,6 +38,8 @@ type FakeEditorOptions = {
* Monaco reports no position for a point over a DOM node it does not own.
*/
deadColumn?: { fromX: number; toX: number }
/** MouseTargetType the hit-test reports, for the gesture's gutter-target filter. */
gutterTargetType?: MonacoEditor.MouseTargetType
}
export function createFakeDiffCommentEditor(
@@ -120,7 +122,10 @@ export function createFakeDiffCommentEditor(
const lineNumber = lineAtClientY(clientY)
return lineNumber === null
? null
: { type: 3 /* GUTTER_LINE_NUMBERS */, position: { lineNumber } }
: {
type: options.gutterTargetType ?? 3 /* GUTTER_LINE_NUMBERS */,
position: { lineNumber }
}
},
onMouseMove: (listener: (e: { target: { position: { lineNumber: number } } }) => void) => {
mouseMoveListeners.push(listener)
@@ -2,7 +2,7 @@ import { describe, expect, it } from 'vitest'
import {
areLineRangesEqual,
clampFocusLineToCommentable,
getSelectionLineRange,
getSelectionAnchorFocus,
orderLineRange,
toDiffCommentLineTarget
} from './diff-comment-line-range'
@@ -63,38 +63,70 @@ describe('clampFocusLineToCommentable', () => {
})
})
describe('getSelectionLineRange', () => {
describe('getSelectionAnchorFocus', () => {
it('excludes a trailing line the selection only touches at column 1', () => {
expect(
getSelectionLineRange({
getSelectionAnchorFocus({
startLineNumber: 4,
startColumn: 3,
endLineNumber: 7,
endColumn: 1
endColumn: 1,
selectionStartLineNumber: 4,
positionLineNumber: 7
})
).toEqual({ startLine: 4, endLine: 6 })
).toEqual({ anchorLine: 4, focusLine: 6 })
})
it('keeps the last line when the selection reaches into it', () => {
expect(
getSelectionLineRange({
getSelectionAnchorFocus({
startLineNumber: 4,
startColumn: 3,
endLineNumber: 7,
endColumn: 5
endColumn: 5,
selectionStartLineNumber: 4,
positionLineNumber: 7
})
).toEqual({ startLine: 4, endLine: 7 })
).toEqual({ anchorLine: 4, focusLine: 7 })
})
it('reads a bare cursor as its own line', () => {
expect(
getSelectionLineRange({
getSelectionAnchorFocus({
startLineNumber: 4,
startColumn: 1,
endLineNumber: 4,
endColumn: 1
endColumn: 1,
selectionStartLineNumber: 4,
positionLineNumber: 4
})
).toEqual({ startLine: 4, endLine: 4 })
).toEqual({ anchorLine: 4, focusLine: 4 })
})
it('keeps the anchor at the bottom of an upward selection', () => {
expect(
getSelectionAnchorFocus({
startLineNumber: 12,
startColumn: 2,
endLineNumber: 41,
endColumn: 6,
selectionStartLineNumber: 41,
positionLineNumber: 12
})
).toEqual({ anchorLine: 41, focusLine: 12 })
})
it('drops the column-1 trailing line from an upward selection anchor', () => {
expect(
getSelectionAnchorFocus({
startLineNumber: 12,
startColumn: 2,
endLineNumber: 41,
endColumn: 1,
selectionStartLineNumber: 41,
positionLineNumber: 12
})
).toEqual({ anchorLine: 40, focusLine: 12 })
})
})
@@ -19,6 +19,12 @@ type LineSelection = {
endColumn: number
}
/** Monaco's `Selection`: ordered bounds plus the anchor/active endpoint the user dragged from. */
type DirectionalLineSelection = LineSelection & {
selectionStartLineNumber: number
positionLineNumber: number
}
export function orderLineRange(anchorLine: number, focusLine: number): DiffCommentLineRange {
return {
startLine: Math.min(anchorLine, focusLine),
@@ -78,6 +84,14 @@ export function getSelectionEndLine(selection: LineSelection): number {
return selection.endLineNumber
}
export function getSelectionLineRange(selection: LineSelection): DiffCommentLineRange {
return orderLineRange(selection.startLineNumber, getSelectionEndLine(selection))
// Why: `startLineNumber`/`endLineNumber` are sorted, so they lose which end the user dragged
// from — and clamping needs the anchor, or an upward selection clamps into the wrong hunk.
export function getSelectionAnchorFocus(selection: DirectionalLineSelection): {
anchorLine: number
focusLine: number
} {
const endLine = getSelectionEndLine(selection)
return selection.positionLineNumber < selection.selectionStartLineNumber
? { anchorLine: endLine, focusLine: selection.positionLineNumber }
: { anchorLine: selection.selectionStartLineNumber, focusLine: endLine }
}
@@ -6,6 +6,8 @@ import {
FAKE_EDITOR_TOP_PX,
type FakeDiffCommentEditor
} from './diff-comment-editor-test-fixture'
import * as monaco from 'monaco-editor'
import type { editor as monacoEditor } from 'monaco-editor'
import { getGutterPressLine, installDiffCommentRangeDrag } from './diff-comment-range-drag'
import type { DiffCommentLineRange } from './diff-comment-line-range'
@@ -81,11 +83,13 @@ function mountDrag(
resolvePressLine?: (event: PointerEvent) => number | null
unresolvableLines?: readonly number[]
deadColumn?: { fromX: number; toX: number }
gutterTargetType?: monacoEditor.MouseTargetType
} = {}
): DragHarness {
const fake = createFakeDiffCommentEditor({
unresolvableLines: options.unresolvableLines,
deadColumn: options.deadColumn
deadColumn: options.deadColumn,
gutterTargetType: options.gutterTargetType
})
const commits: DiffCommentLineRange[] = []
const dragStates: boolean[] = []
@@ -329,6 +333,68 @@ describe('diff comment gutter range drag', () => {
expect(drag.commits).toEqual([{ startLine: 7, endLine: 9 }])
drag.handle.dispose()
})
// Keyboard paths read this to stand aside; the drag range isn't committed until release.
it('reports the drag as owning the band from press to release', () => {
const drag = mountDrag()
expect(drag.handle.isDragging()).toBe(false)
drag.pressLine(12)
expect(drag.handle.isDragging()).toBe(true)
drag.moveToLine(17)
pumpFrame()
expect(drag.handle.isDragging()).toBe(true)
drag.release()
expect(drag.handle.isDragging()).toBe(false)
drag.handle.dispose()
})
})
describe('diff comment gutter range drag target types', () => {
// Monaco's folding controller toggles chevrons from its own mousedown on GUTTER_LINE_DECORATIONS,
// and the markdown annotations editor installs this drag with no commentable-line set — so
// claiming anything but the line numbers would eat the fold press on every line there.
function pressGutter(drag: DragHarness): Event {
const event = new Event('pointerdown', { bubbles: true, cancelable: true })
Object.assign(event, {
clientX: 30,
clientY: drag.fake.clientYForLine(11),
button: 0,
pointerType: 'mouse',
pointerId: 1
})
drag.fake.domNode.dispatchEvent(event)
return event
}
it('takes a press on the line numbers', () => {
const drag = mountDrag({
gutterTargetType: monaco.editor.MouseTargetType.GUTTER_LINE_NUMBERS
})
expect(pressGutter(drag).defaultPrevented).toBe(true)
drag.release()
expect(drag.commits).toEqual([{ startLine: 11, endLine: 11 }])
drag.handle.dispose()
})
it('leaves the rest of the gutter to Monaco', () => {
for (const type of [
monaco.editor.MouseTargetType.GUTTER_LINE_DECORATIONS,
monaco.editor.MouseTargetType.GUTTER_GLYPH_MARGIN
]) {
const drag = mountDrag({ gutterTargetType: type })
expect(pressGutter(drag).defaultPrevented).toBe(false)
drag.release()
expect(drag.commits).toEqual([])
expect(paintedRange(drag.fake)).toBeNull()
drag.handle.dispose()
}
})
})
describe('diff comment gutter range drag performance', () => {
@@ -14,8 +14,8 @@ import {
// Monaco's own gutter gesture (select this line) is pre-empted rather than fought after the fact.
// It registers its press handler as a bubble-phase `pointerdown` on the view DOM node — the very
// node `editor.getDomNode()` returns — so a capture-phase listener on that same node still runs
// first for any descendant target (line numbers, glyph margin, our "+"), and stopPropagation
// there keeps the event from ever reaching Monaco.
// first for any descendant target (line numbers, our "+"), and stopPropagation there keeps the
// event from ever reaching Monaco.
export type DiffCommentRangeDragEditor = Pick<
monacoEditor.ICodeEditor,
@@ -44,12 +44,15 @@ export type DiffCommentRangeDragHandle = {
dispose: () => void
/** The range of the open composer; keeps the band lit while the note is being written. */
setPendingRange: (range: DiffCommentLineRange | null) => void
/** True while a press owns the band, so keyboard paths can stand aside instead of racing it. */
isDragging: () => boolean
}
// Line numbers only: the rest of the gutter carries Monaco's own press handlers (fold chevrons
// live in GUTTER_LINE_DECORATIONS), and a capture-phase stopPropagation here would swallow them.
// The "+" press needs no entry — it resolves through the button, not a Monaco target.
const GUTTER_PRESS_TARGET_TYPES: ReadonlySet<monacoEditor.MouseTargetType> = new Set([
monaco.editor.MouseTargetType.GUTTER_GLYPH_MARGIN,
monaco.editor.MouseTargetType.GUTTER_LINE_NUMBERS,
monaco.editor.MouseTargetType.GUTTER_LINE_DECORATIONS
monaco.editor.MouseTargetType.GUTTER_LINE_NUMBERS
])
// Far enough into the text column to clear the gutter, close enough to stay on every line.
@@ -329,7 +332,8 @@ export function installDiffCommentRangeDrag({
}
pendingRange = range
repaint()
}
},
isDragging: () => drag !== null
}
}
@@ -58,15 +58,26 @@ function renderDecorator(fake: FakeDiffCommentEditor, initialProps: DecoratorPro
}
// Monaco's Selection carries a large method surface the shortcut never touches, so the double is
// built once here rather than cast at each call site.
// built once here rather than cast at each call site. `anchor` names the end the user dragged
// from, which Monaco reports separately from the sorted bounds.
function selectionOf(
startLineNumber: number,
startColumn: number,
endLineNumber: number,
endColumn: number
endColumn: number,
anchor: 'start' | 'end' = 'start'
): Selection {
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: resolveDiffCommentShortcutTarget reads only these four fields.
return { startLineNumber, startColumn, endLineNumber, endColumn } as Selection
const selectionStartLineNumber = anchor === 'start' ? startLineNumber : endLineNumber
const positionLineNumber = anchor === 'start' ? endLineNumber : startLineNumber
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: resolveDiffCommentShortcutTarget reads only these six fields.
return {
startLineNumber,
startColumn,
endLineNumber,
endColumn,
selectionStartLineNumber,
positionLineNumber
} as Selection
}
function paintedRange(fake: FakeDiffCommentEditor): { startLine: number; endLine: number } | null {
@@ -80,7 +91,13 @@ function firePointerEvent(
init: { clientX: number; clientY: number }
): void {
const event = new Event(type, { bubbles: true, cancelable: true })
Object.assign(event, { ...init, button: 0, pointerType: 'mouse', pointerId: 1, ctrlKey: false })
Object.assign(event, {
...init,
button: 0,
pointerType: 'mouse',
pointerId: 1,
ctrlKey: false
})
node.dispatchEvent(event)
}
@@ -150,7 +167,9 @@ describe('useDiffCommentDecorator range highlight', () => {
it('does not rewrite the decoration while the composer only moves with the scroll', () => {
const fake = createFakeDiffCommentEditor()
const hook = renderDecorator(fake, { pendingCommentTarget: { lineNumber: 14, startLine: 9 } })
const hook = renderDecorator(fake, {
pendingCommentTarget: { lineNumber: 14, startLine: 9 }
})
const writes = fake.decorationWrites()
// A scroll rewrites the composer's `top` and hands down a fresh object every frame.
@@ -190,15 +209,24 @@ describe('useDiffCommentDecorator drag affordance', () => {
const parkedTop = plus!.style.top
// Press the button itself, then drag down the same column.
firePointerEvent(plus!, 'pointerdown', { clientX: 8, clientY: fake.clientYForLine(5) })
firePointerEvent(document, 'pointermove', { clientX: 8, clientY: fake.clientYForLine(11) })
firePointerEvent(plus!, 'pointerdown', {
clientX: 8,
clientY: fake.clientYForLine(5)
})
firePointerEvent(document, 'pointermove', {
clientX: 8,
clientY: fake.clientYForLine(11)
})
pumpFrame()
expect(plus!.style.top, 'the "+" did not follow the drag').not.toBe(parkedTop)
expect(plus!.style.pointerEvents, 'the "+" must not block its own hit-test').toBe('none')
expect(fake.decorations()[0]).toMatchObject({ startLine: 5, endLine: 11 })
firePointerEvent(document, 'pointerup', { clientX: 8, clientY: fake.clientYForLine(11) })
firePointerEvent(document, 'pointerup', {
clientX: 8,
clientY: fake.clientYForLine(11)
})
expect(plus!.style.pointerEvents).toBe('')
})
})
@@ -257,6 +285,24 @@ describe('useDiffCommentDecorator add-note chord', () => {
expect(onAddCommentClick).not.toHaveBeenCalled()
})
it('clamps an upward selection from its anchor, not the hunk above it', () => {
const fake = createFakeDiffCommentEditor()
// Anchored at 41 in the lower hunk, extended up to 12 in the upper one.
vi.spyOn(fake.editor, 'getSelection').mockReturnValue(selectionOf(12, 2, 41, 6, 'end'))
const onAddCommentClick = vi.fn()
renderDecorator(fake, {
addNoteShortcutEnabled: true,
commentableLineNumbers: [10, 11, 12, 13, 14, 15, 16, 40, 41, 42],
onAddCommentClick
})
pressAddReviewNoteChord(fake.domNode)
expect(onAddCommentClick).toHaveBeenCalledWith(
expect.objectContaining({ lineNumber: 41, startLine: 40 })
)
})
it('leaves the chord unconsumed when the selection is outside the commentable lines', () => {
const fake = createFakeDiffCommentEditor()
vi.spyOn(fake.editor, 'getSelection').mockReturnValue(selectionOf(40, 1, 41, 4))
@@ -298,6 +344,28 @@ describe('useDiffCommentDecorator add-note chord', () => {
expect(onAddCommentClick).not.toHaveBeenCalled()
})
it('stands aside while a gutter drag still owns the band', () => {
const fake = createFakeDiffCommentEditor()
// The pre-drag selection the chord would otherwise open a composer on.
vi.spyOn(fake.editor, 'getSelection').mockReturnValue(selectionOf(30, 1, 32, 4))
const onAddCommentClick = vi.fn()
renderDecorator(fake, { addNoteShortcutEnabled: true, onAddCommentClick })
firePointerEvent(fake.domNode, 'pointerdown', { clientX: 30, clientY: fake.clientYForLine(5) })
firePointerEvent(document, 'pointermove', { clientX: 30, clientY: fake.clientYForLine(11) })
pumpFrame()
pressAddReviewNoteChord(fake.domNode)
expect(onAddCommentClick).not.toHaveBeenCalled()
// Release still commits the swept range, and only that range.
firePointerEvent(document, 'pointerup', { clientX: 30, clientY: fake.clientYForLine(11) })
expect(onAddCommentClick).toHaveBeenCalledTimes(1)
expect(onAddCommentClick).toHaveBeenCalledWith(
expect.objectContaining({ lineNumber: 11, startLine: 5 })
)
})
it('claims the chord synchronously so a same-turn repeat cannot reopen the draft', () => {
const fake = createFakeDiffCommentEditor()
vi.spyOn(fake.editor, 'getSelection').mockReturnValue(selectionOf(9, 1, 14, 8))
@@ -166,8 +166,11 @@ export function useDiffCommentDecorator({
editor,
commentableLineSet,
// The composer consumes the chord itself once open (DiffCommentPopover's guard); claiming
// it here as well would remount the composer over the user's draft.
isComposerOpen: () => pendingCommentRangeRef.current !== null,
// it here as well would remount the composer over the user's draft. A live gutter drag owns
// the band the same way, and its range isn't committed yet — opening from the stale editor
// selection would remount the composer the moment the press lands.
isComposerOpen: () =>
pendingCommentRangeRef.current !== null || overlayRef.current?.isDragging() === true,
onOpenComposer: (args) => {
// Claim synchronously so a second chord in the same event turn cannot open another draft
// before React commits the parent state update.
+38 -3
View File
@@ -56,19 +56,19 @@ async function seedDiffFile(page: Page, worktreeId: string, relative: string): P
// Centre of a line's number cell — the column the "+" lives in and the gesture starts from.
async function gutterPoint(page: Page, lineNumber: number): Promise<{ x: number; y: number }> {
const point = await page.evaluate((line: string) => {
const point = await page.evaluate((lineNumber: number) => {
const editor = document.querySelector('.monaco-editor.modified-in-monaco-diff-editor')
if (!editor) {
return null
}
for (const cell of editor.querySelectorAll('.margin .line-numbers')) {
if (cell.textContent?.trim() === line) {
if (Number.parseInt(cell.textContent?.trim() ?? '', 10) === lineNumber) {
const rect = cell.getBoundingClientRect()
return { x: Math.round(rect.x + rect.width / 2), y: Math.round(rect.y + rect.height / 2) }
}
}
return null
}, String(lineNumber))
}, lineNumber)
if (!point) {
throw new Error(`line ${lineNumber} is not rendered in the modified gutter`)
}
@@ -171,6 +171,41 @@ test.describe('Diff note line range', () => {
await expect(orcaPage.locator(BAND)).toHaveCount(0)
})
// Bottom-to-top: the anchor is the lower line, so the committed range only reads in document
// order if the drag keeps anchor and focus apart instead of sorting them as it goes.
test('dragging the gutter upward commits the same range as dragging down', async ({
orcaPage
}) => {
const worktreeId = await waitForActiveWorktree(orcaPage)
await seedDiffFile(orcaPage, worktreeId, 'src/diff-note-range-drag-up.ts')
const from = await gutterPoint(orcaPage, 9)
const to = await gutterPoint(orcaPage, 4)
await orcaPage.mouse.move(from.x, from.y)
await orcaPage.mouse.down()
await expect(orcaPage.locator(BAND)).toHaveCount(1)
await orcaPage.mouse.move(to.x, (from.y + to.y) / 2)
await orcaPage.mouse.move(to.x, to.y)
await expect(
orcaPage.locator(BAND),
'the band did not grow upward while the button was held'
).toHaveCount(6)
await orcaPage.mouse.up()
await expect(orcaPage.locator(COMPOSER_LABEL)).toHaveText('Lines 4-9')
expect(await orcaPage.evaluate(() => window.getSelection()?.toString() ?? '')).toBe('')
await submitNote(orcaPage, 'Dragged bottom to top.')
expect(await readNotes(orcaPage, worktreeId)).toEqual([
{ startLine: 4, lineNumber: 9, body: 'Dragged bottom to top.' }
])
await expect(orcaPage.locator('.orca-diff-comment-card').first()).toContainText('lines 4-9')
})
// The gesture that used to collapse to a single line: the press starts on the "+", a node
// Monaco does not own, so hit-testing under the pointer resolved nothing for the whole drag.
test('dragging from the "+" itself selects a range and the button rides the selection', async ({