fix: preserve diff note gutter eligibility and labels

This commit is contained in:
Neil
2026-09-07 15:48:09 -07:00
parent 838a7c834b
commit 46bdddb53a
13 changed files with 303 additions and 27 deletions
+70
View File
@@ -37,6 +37,76 @@ index 9bdaaa3ef199f3e443f77106a9ac7eddcc4b05b6..461364d3af60a954a766ef285ddef147
const fileDiff = this.getLatestDiff();
if (fileDiff == null || fileDiff.editSessionDirty !== true) return false;
const { collapsedContextThreshold = 1 } = this.options;
diff --git a/dist/managers/InteractionManager.d.ts b/dist/managers/InteractionManager.d.ts
index 9f243f97a6eb28c22176b6b3abba022756571fdc..806bb52ac15de68044ea5a5cfbb5016eae38ccae 100644
--- a/dist/managers/InteractionManager.d.ts
+++ b/dist/managers/InteractionManager.d.ts
@@ -43,6 +43,8 @@ interface InteractionManagerBaseOptions<TMode extends InteractionManagerMode> {
lineHoverHighlight?: 'disabled' | 'both' | 'number' | 'line';
enableTokenInteractionsOnWhitespace?: boolean;
enableGutterUtility?: boolean;
+ canUseGutterUtility?(range: SelectedLineRange): boolean;
+ gutterUtilityLabel?: string;
onGutterUtilityClick?(range: SelectedLineRange): unknown;
onLineClick?(props: EventClickProps<TMode>): unknown;
onLineNumberClick?(props: EventClickProps<TMode>): unknown;
diff --git a/dist/managers/InteractionManager.js b/dist/managers/InteractionManager.js
index 8339405526a0a74638708738da09a6c2190cd069..7647ec597f11cbf52a5b6ba0e5e76d9b9ccfdaf6 100644
--- a/dist/managers/InteractionManager.js
+++ b/dist/managers/InteractionManager.js
@@ -445,7 +445,7 @@ var InteractionManager = class {
this.updateSelection(point.lineNumber, point.side);
}
const completedRange = this.buildSelectedLineRange(session.anchor, session.current);
- onGutterUtilityClick?.({ ...completedRange });
+ if (this.options.canUseGutterUtility?.(completedRange) !== false) onGutterUtilityClick?.({ ...completedRange });
this.selectionAnchor = void 0;
this.notifySelectionEnd(completedRange);
this.notifySelectionCommitted(completedRange);
@@ -567,6 +567,25 @@ var InteractionManager = class {
}
showUtilityOnLine(line) {
if (this.gutterUtilityContainer == null) return;
+ const range = this.getCurrentSelectionRange() ?? {
+ start: line.lineNumber,
+ end: line.lineNumber,
+ side: line.type === "diff-line" ? line.annotationSide : undefined
+ };
+ if (this.options.canUseGutterUtility?.(range) === false) {
+ this.hideUtility();
+ return;
+ }
+ if (this.gutterUtilityButton != null) {
+ const label = this.options.gutterUtilityLabel;
+ if (label != null) {
+ this.gutterUtilityButton.setAttribute("aria-label", label);
+ this.gutterUtilityButton.title = label;
+ } else {
+ this.gutterUtilityButton.removeAttribute("aria-label");
+ this.gutterUtilityButton.removeAttribute("title");
+ }
+ }
this.gutterUtilityLine = line;
line.numberElement.appendChild(this.gutterUtilityContainer);
}
@@ -1066,7 +1085,7 @@ var InteractionManager = class {
if (!split) return lineIndexes[0];
}
};
-function pluckInteractionOptions({ enableTokenInteractionsOnWhitespace, enableGutterUtility, lineHoverHighlight, onGutterUtilityClick, onLineClick, onLineEnter, onLineLeave, onLineNumberClick, onTokenClick, onTokenEnter, onTokenLeave, renderGutterUtility, __debugPointerEvents, enableLineSelection, controlledSelection, onLineSelected, onLineSelectionStart, onLineSelectionChange, onLineSelectionEnd }, onHunkExpand, getLineIndex, onMergeConflictActionClick) {
+function pluckInteractionOptions({ enableTokenInteractionsOnWhitespace, enableGutterUtility, canUseGutterUtility, gutterUtilityLabel, lineHoverHighlight, onGutterUtilityClick, onLineClick, onLineEnter, onLineLeave, onLineNumberClick, onTokenClick, onTokenEnter, onTokenLeave, renderGutterUtility, __debugPointerEvents, enableLineSelection, controlledSelection, onLineSelected, onLineSelectionStart, onLineSelectionChange, onLineSelectionEnd }, onHunkExpand, getLineIndex, onMergeConflictActionClick) {
return {
enableTokenInteractionsOnWhitespace,
enableGutterUtility: resolveEnableGutterUtilityOption({
@@ -1077,6 +1096,8 @@ function pluckInteractionOptions({ enableTokenInteractionsOnWhitespace, enableGu
usesCustomGutterUtility: renderGutterUtility != null,
lineHoverHighlight,
onGutterUtilityClick,
+ canUseGutterUtility,
+ gutterUtilityLabel,
onHunkExpand,
onMergeConflictActionClick,
onLineClick,
diff --git a/dist/renderers/DiffHunksRenderer.js b/dist/renderers/DiffHunksRenderer.js
index 4e6aeb4cd0a5dc1f9dd094b874f338cafceff29b..fc3bcf89f79ea56dba0a8a84b507fc97a370ff49 100644
--- a/dist/renderers/DiffHunksRenderer.js
+5
View File
@@ -16,6 +16,11 @@ in a worker. The opt-out preserves completion notifications, live document text,
and edit history; their hunk metadata retains its edit-session shape. Upstream's
default cleanup behavior remains unchanged.
The gutter utility also accepts an optional range predicate and accessible label.
Orca uses these to hide note controls on original or ineligible review lines,
reject ranges crossing an ineligible line, and retain each surface's note label.
Drag completion rechecks the predicate. Other consumers retain the upstream defaults.
Keep `useTokenTransformer: true` on the pool. Coverage lives in
`pierre-diff-worker-edit-cache.test.ts` and the large-diff Electron specs. Remove
the patch when an upstream release provides the same behavior.
+3 -3
View File
@@ -109,7 +109,7 @@ overrides:
monaco-editor>dompurify: 3.4.13
patchedDependencies:
'@pierre/diffs@1.4.1': 74a8bda29a238419791ec16428692b964f016d852c196428c13f31fb78412eb3
'@pierre/diffs@1.4.1': cbcdcdfecdf4f666a96870b0bac2297cdb8cbcc5fbb12aece2280772aa8f8362
'@vscode/windows-process-tree@0.8.0': f8ea245391c94da5770045aeea01fa6de466c2199c6ef46b5b769b398aa9823e
'@xterm/addon-ligatures@0.11.0-beta.300': 47405b9994b5acf1b4e90b49250358c1ca03649854d59560e7732b72fe336920
'@xterm/addon-search@0.17.0-beta.300': eee5338dd2621ece46e79c61ec06766cd7fadaf79ffdb24e2a8ab68e97ef31f0
@@ -143,7 +143,7 @@ importers:
version: 2.5.6
'@pierre/diffs':
specifier: 1.4.1
version: 1.4.1(patch_hash=74a8bda29a238419791ec16428692b964f016d852c196428c13f31fb78412eb3)(@shikijs/themes@4.4.3)(react-dom@19.2.8(react@19.2.8))(react@19.2.8)
version: 1.4.1(patch_hash=cbcdcdfecdf4f666a96870b0bac2297cdb8cbcc5fbb12aece2280772aa8f8362)(@shikijs/themes@4.4.3)(react-dom@19.2.8(react@19.2.8))(react@19.2.8)
'@xterm/addon-serialize':
specifier: 0.15.0-beta.300
version: 0.15.0-beta.300(patch_hash=851eac3d75e6d8c013b9f4c053e61d824b23965cb19ecc28e335e05059f3a294)(@xterm/xterm@6.1.0-beta.303(patch_hash=98756bcedc402bcdb7c6ab7b015d2e59cd18e97b03a2c06a27e95bb3ba429d9d))
@@ -8280,7 +8280,7 @@ snapshots:
tslib: 2.8.1
webcrypto-core: 1.9.2
'@pierre/diffs@1.4.1(patch_hash=74a8bda29a238419791ec16428692b964f016d852c196428c13f31fb78412eb3)(@shikijs/themes@4.4.3)(react-dom@19.2.8(react@19.2.8))(react@19.2.8)':
'@pierre/diffs@1.4.1(patch_hash=cbcdcdfecdf4f666a96870b0bac2297cdb8cbcc5fbb12aece2280772aa8f8362)(@shikijs/themes@4.4.3)(react-dom@19.2.8(react@19.2.8))(react@19.2.8)':
dependencies:
'@pierre/theme': 2.0.0
'@pierre/theming': 1.0.1(@pierre/theme@2.0.0)(@shikijs/themes@4.4.3)(react-dom@19.2.8(react@19.2.8))(react@19.2.8)(shiki@4.4.3)
@@ -2,6 +2,7 @@ import * as monaco from 'monaco-editor'
import type { editor as monacoEditor, IDisposable } from 'monaco-editor'
import type { RefObject } from 'react'
import { getDiffCommentPopoverTop } from './diff-comment-popover-position'
import { canCommentOnRange } from './diff-comment-range'
// Monaco glyph decorations don't expose usable click events, so we own an absolutely-positioned "+" button that follows the hovered line.
@@ -87,20 +88,6 @@ export function installDiffCommentAddButtonOverlay({
return commentableLineSet === null || commentableLineSet.has(lineNumber)
}
const canCommentOnRange = (startLine: number, endLine: number): boolean => {
if (commentableLineSet === null) {
return true
}
const from = Math.min(startLine, endLine)
const to = Math.max(startLine, endLine)
for (let line = from; line <= to; line++) {
if (!commentableLineSet.has(line)) {
return false
}
}
return true
}
const positionAtLine = (lineNumber: number): void => {
const lineTop = editor.getTopForLineNumber(lineNumber) - editor.getScrollTop()
const top = Math.round(lineTop + (getLineHeight() - BUTTON_SIZE) / 2)
@@ -122,7 +109,7 @@ export function installDiffCommentAddButtonOverlay({
if (!currentDrag) {
return
}
if (!canCommentOnRange(currentDrag.startLine, currentDrag.endLine)) {
if (!canCommentOnRange(currentDrag.startLine, currentDrag.endLine, commentableLineSet)) {
return
}
const startLine = Math.min(currentDrag.startLine, currentDrag.endLine)
@@ -147,7 +134,7 @@ export function installDiffCommentAddButtonOverlay({
line == null ||
line === dragState.endLine ||
!canCommentOnLine(line) ||
!canCommentOnRange(dragState.startLine, line)
!canCommentOnRange(dragState.startLine, line, commentableLineSet)
) {
return
}
@@ -0,0 +1,27 @@
export function canCommentOnRange(
startLine: number,
endLine: number,
commentableLines: ReadonlySet<number> | null
): boolean {
if (
!Number.isInteger(startLine) ||
!Number.isInteger(endLine) ||
Math.min(startLine, endLine) < 1
) {
return false
}
if (commentableLines === null) {
return true
}
const from = Math.min(startLine, endLine)
const to = Math.max(startLine, endLine)
if (to - from + 1 > commentableLines.size) {
return false
}
for (let line = from; line <= to; line++) {
if (!commentableLines.has(line)) {
return false
}
}
return true
}
@@ -253,6 +253,7 @@ export function DiffSectionItem({
onEditChange={handleEditChange}
onPostRender={handlePostRender}
onAddComment={hasLineCommentAction ? handleAddComment : undefined}
commentableLineNumbers={commentableLineNumbers}
pendingComment={pendingComment}
addCommentPlaceholder={addLineCommentPlaceholder}
addCommentLabel={addLineCommentLabel}
@@ -266,6 +267,7 @@ export function DiffSectionItem({
addLineCommentLabel,
addLineCommentPlaceholder,
comments,
commentableLineNumbers,
fileDiff,
parseError,
retryParse,
@@ -28,6 +28,7 @@ import { usePierreDiffFind } from './use-pierre-diff-find'
import { installPierreContextualCopy } from './pierre-diff-context-copy'
import { editorShortcutMatches } from '../editor-shortcuts'
import { usePierreDiffNoteNavigation } from './use-pierre-diff-note-navigation'
import { canCommentOnPierreRange } from './pierre-diff-comment-range'
export type PierreDiffInstance = PierreFileDiff<PierreDiffAnnotationData> &
Partial<Pick<VirtualizedFileDiff, 'getLinePosition'>>
@@ -53,6 +54,7 @@ export type PierreDiffSurfaceProps = {
onPostRender?: (node: HTMLElement, phase: PostRenderPhase, instance: PierreDiffInstance) => void
/** Gutter affordance for starting a note; omit to hide it. */
onAddComment?: (range: { lineNumber: number; startLine?: number }) => void
commentableLineNumbers?: readonly number[]
/** Open note draft, rendered inline on its anchor line. */
pendingComment?: { lineNumber: number; startLine?: number } | null
addCommentPlaceholder?: string
@@ -82,6 +84,7 @@ export function PierreDiffSurface({
onEditChange,
onPostRender,
onAddComment,
commentableLineNumbers,
pendingComment,
addCommentPlaceholder,
addCommentLabel,
@@ -101,6 +104,10 @@ export function PierreDiffSurface({
const { editEnabled, handleContainerKeyDown, handleContainerBlur, handleEditorAttach } =
usePierreDiffFind({ isEditable, containerRef })
const navigateToNote = usePierreDiffNoteNavigation({ worktreeId, filePath, comments })
const commentableLines = useMemo(
() => (commentableLineNumbers ? new Set(commentableLineNumbers) : null),
[commentableLineNumbers]
)
// Why: Monaco's diff panes owned `editor.copyContext`; restore it for Pierre rows.
const fileInfoRef = useRef({ relativePath: filePath, language: language ?? '' })
@@ -121,9 +128,12 @@ export function PierreDiffSurface({
collapseUnchanged
}),
enableGutterUtility: Boolean(onAddComment),
canUseGutterUtility: (range: SelectedLineRange) =>
canCommentOnPierreRange(range, commentableLines),
gutterUtilityLabel: addCommentLabel ?? 'Add note for the AI',
onGutterUtilityClick: onAddComment
? (range: SelectedLineRange) => {
if (range.side === 'deletions' || range.endSide === 'deletions') {
if (!canCommentOnPierreRange(range, commentableLines)) {
return
}
onAddComment({
@@ -137,7 +147,16 @@ export function PierreDiffSurface({
navigateToNote(node, phase, instance)
}
}),
[settings, sideBySide, collapseUnchanged, onPostRender, onAddComment, navigateToNote]
[
settings,
sideBySide,
collapseUnchanged,
onPostRender,
onAddComment,
navigateToNote,
commentableLines,
addCommentLabel
]
)
const style = useMemo(
() => buildPierreDiffStyle(settings, editorFontZoomLevel),
@@ -0,0 +1,32 @@
import { describe, expect, it } from 'vitest'
import { canCommentOnPierreRange } from './pierre-diff-comment-range'
describe('Pierre comment eligibility', () => {
it('requires every line in a review range, including backwards selections', () => {
const eligible = new Set([10, 11, 12, 14])
expect(canCommentOnPierreRange({ start: 10, end: 12, side: 'additions' }, eligible)).toBe(true)
expect(canCommentOnPierreRange({ start: 12, end: 10, side: 'additions' }, eligible)).toBe(true)
expect(canCommentOnPierreRange({ start: 10, end: 14, side: 'additions' }, eligible)).toBe(false)
expect(canCommentOnPierreRange({ start: 14, end: 10, side: 'additions' }, eligible)).toBe(false)
expect(canCommentOnPierreRange({ start: 10, end: 10, side: 'additions' }, new Set())).toBe(
false
)
})
it('rejects original-side and cross-side selections', () => {
expect(canCommentOnPierreRange({ start: 1, end: 2, side: 'deletions' }, null)).toBe(false)
expect(
canCommentOnPierreRange({ start: 1, end: 2, side: 'additions', endSide: 'deletions' }, null)
).toBe(false)
expect(
canCommentOnPierreRange({ start: 1, end: 2, side: 'deletions', endSide: 'additions' }, null)
).toBe(false)
})
it('allows local modified context while rejecting invalid anchors', () => {
expect(canCommentOnPierreRange({ start: 1, end: 20 }, null)).toBe(true)
for (const start of [0, -1, 1.5, Infinity, Number.NaN]) {
expect(canCommentOnPierreRange({ start, end: 20 }, null)).toBe(false)
}
})
})
@@ -0,0 +1,13 @@
import type { SelectedLineRange } from '@pierre/diffs'
import { canCommentOnRange } from '../../diff-comments/diff-comment-range'
export function canCommentOnPierreRange(
range: SelectedLineRange,
commentableLines: ReadonlySet<number> | null
): boolean {
return (
range.side !== 'deletions' &&
range.endSide !== 'deletions' &&
canCommentOnRange(range.start, range.end, commentableLines)
)
}
@@ -0,0 +1,62 @@
// @vitest-environment happy-dom
import { afterEach, expect, it } from 'vitest'
import { InteractionManager, pluckInteractionOptions } from '@pierre/diffs'
import { canCommentOnPierreRange } from './pierre-diff-comment-range'
afterEach(() => document.body.replaceChildren())
it('hides the gutter action on original and ineligible review lines and labels eligible lines', () => {
const pre = document.createElement('pre')
pre.setAttribute('data-diff-type', 'split')
for (const side of ['deletions', 'additions']) {
const code = document.createElement('code')
code.setAttribute('data-code', '')
code.setAttribute(`data-${side}`, '')
for (let line = 1; line <= 3; line++) {
const number = document.createElement('div')
number.setAttribute('data-column-number', String(line))
number.setAttribute('data-line-index', `${line - 1},${line - 1}`)
number.setAttribute('data-line-type', 'context')
const content = number.cloneNode() as HTMLElement
content.removeAttribute('data-column-number')
content.setAttribute('data-line', String(line))
content.textContent = 'example'
code.append(number, content)
}
pre.append(code)
}
document.body.append(pre)
const manager = new InteractionManager(
'diff',
pluckInteractionOptions(
{
enableGutterUtility: true,
gutterUtilityLabel: 'Add review comment',
canUseGutterUtility: (range) => canCommentOnPierreRange(range, new Set([1, 3]))
},
() => {}
)
)
manager.setup(pre)
const hover = (side: string, line: number) => {
pre
.querySelector(`[data-${side}] [data-line="${line}"]`)!
.dispatchEvent(new PointerEvent('pointermove', { bubbles: true, pointerType: 'mouse' }))
}
try {
hover('deletions', 1)
expect(pre.querySelector('[data-utility-button]')).toBeNull()
hover('additions', 1)
expect(pre.querySelector('[data-utility-button]')?.getAttribute('aria-label')).toBe(
'Add review comment'
)
hover('additions', 2)
expect(pre.querySelector('[data-utility-button]')).toBeNull()
hover('additions', 3)
expect(pre.querySelector('[data-utility-button]')?.getAttribute('title')).toBe(
'Add review comment'
)
} finally {
manager.cleanUp()
}
})
@@ -357,11 +357,11 @@ function getLargestBackwardScrollJump(samples: readonly ScrollProbeSample[]): nu
async function clickVisibleDiffLine(page: Page): Promise<void> {
// Diff rows render asynchronously after switching tabs.
let linePoint: { x: number; y: number } | null = null
const linePoint: { current: { x: number; y: number } | null } = { current: null }
await expect
.poll(
async () => {
linePoint = await page.evaluate(() => {
linePoint.current = await page.evaluate(() => {
const container = document.querySelector<HTMLElement>('.combined-diff-scroll-container')
if (!container) {
return null
@@ -391,16 +391,16 @@ async function clickVisibleDiffLine(page: Page): Promise<void> {
y: rect.top + rect.height / 2
}
})
return linePoint !== null
return linePoint.current !== null
},
{ timeout: 10_000, message: 'visible combined diff line not found' }
)
.toBe(true)
if (!linePoint) {
if (!linePoint.current) {
throw new Error('visible combined diff line not found')
}
await page.mouse.click(linePoint.x, linePoint.y)
await page.mouse.click(linePoint.current.x, linePoint.current.y)
}
test.describe('Combined diff scroll restore', () => {
+56
View File
@@ -0,0 +1,56 @@
import { rmSync, writeFileSync } from 'node:fs'
import { test, expect } from './helpers/orca-app'
import { waitForSessionReady } from './helpers/store'
import { addAndActivateRepo } from './helpers/isolated-repo-activation'
import { createIsolatedLargeDiffRepo } from './large-diff-repro-fixtures'
test.use({ seedTestRepo: false })
for (const layout of ['side-by-side', 'inline'] as const) {
test(`note gutter supports modified ranges in ${layout} diffs`, async ({
orcaPage,
registerPostElectronShutdownCleanup
}, testInfo) => {
const original = 'export const one = 1\nexport const two = 2\nexport const three = 3\n'
const fixture = createIsolatedLargeDiffRepo(original)
registerPostElectronShutdownCleanup(async () =>
rmSync(fixture.repoPath, { recursive: true, force: true })
)
writeFileSync(fixture.absolutePath, original.replaceAll('= ', '= 9'))
await waitForSessionReady(orcaPage)
await addAndActivateRepo(orcaPage, fixture.repoPath)
await orcaPage.evaluate(
(diffDefaultView) => window.__store!.getState().updateSettings({ diffDefaultView }),
layout
)
await orcaPage.getByRole('button', { name: /^Source Control/ }).click()
await orcaPage.locator('[data-testid="source-control-entry"]').first().click()
const host = orcaPage.locator('diffs-container')
const originalLine = host.locator('[data-content] [data-line-type="change-deletion"]').first()
const modifiedLine = host.locator('[data-content] [data-line-type="change-addition"]').first()
const addButton = host.getByRole('button', { name: 'Add note for the AI', exact: true })
await originalLine.hover({ timeout: 20_000 })
await expect(host.locator('[data-utility-button]')).toHaveCount(0)
await modifiedLine.hover()
await expect(addButton).toBeVisible()
const buttonBox = await addButton.boundingBox()
const lastNumber = host.locator('[data-column-number="3"][data-line-type="change-addition"]')
const lastBox = await lastNumber.boundingBox()
if (!buttonBox || !lastBox) {
throw new Error('Missing gutter layout')
}
await orcaPage.mouse.move(buttonBox.x + buttonBox.width / 2, buttonBox.y + buttonBox.height / 2)
await orcaPage.mouse.down()
await orcaPage.mouse.move(lastBox.x + lastBox.width / 2, lastBox.y + lastBox.height / 2, {
steps: 5
})
await orcaPage.mouse.up()
const draft = host.locator('.orca-diff-comment-popover-textarea')
await expect(draft).toBeVisible()
await draft.fill('Review these three modified lines')
await host.getByRole('button', { name: 'Add note', exact: true }).click()
const card = host.locator('.orca-diff-comment-card')
await expect(card).toContainText('Review these three modified lines')
await expect(card).toContainText('Note lines 1-3')
await orcaPage.screenshot({ path: testInfo.outputPath(`${layout}-range-note.png`) })
})
}
+4 -1
View File
@@ -189,7 +189,10 @@ test.describe('Large diff freeze repro', () => {
// Why: reproduce stale snapshot behavior by opening combined diffs
// as "unstaged" using entries captured from the staged status snapshot.
const staleUnstagedEntries = entries.map((entry) => ({ ...entry, area: 'unstaged' }))
const staleUnstagedEntries = entries.map((entry) => ({
...entry,
area: 'unstaged' as const
}))
const intervalMs = 50
const samples: number[] = []
let last = performance.now()