From 2e37fc45717f7eb898ca98f89ea0c17e218f57d4 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:36:25 -0700 Subject: [PATCH] fix(editor): scroll combined diffs with keyboard navigation (#35004) * fix(editor): keep combined diff keyboard cursors visible * fix(editor): retain focused diff sections through height changes * test(editor): bound combined diff reveal and measurement work * fix(editor): share combined diff viewport intent across review hosts * test: verify worktree tree navigation cancels cursor reveal * test: assert cursor reveal clears before tree scroll * test: use typed diff instrumentation and fixture convention * fix: declare implemented diff content size event * Simplify combined diff cursor reveal and keep focused regression tests Keep keyboard cursor geometry mapped to the surrounding viewport and sticky header. Remove speculative focused-row retention, deferred following and its manual-input cancellation; restore existing section-height measurement. Retain split/inline real-Monaco keyboard cases, click-source preservation, and both PR Files hosts covering the review-found missing viewport markers. Drop redundant timing, tree and operation-count fixtures and restore render-range baseline tests. --- .../src/components/editor/DiffSectionBody.tsx | 3 + ...ffSectionBody.word-wrap-lifecycle.test.tsx | 1 + .../components/editor/DiffSectionHeader.tsx | 2 + .../combined-diff-cursor-reveal.test.tsx | 196 ++++++++++++++++++ .../editor/combined-diff-cursor-reveal.ts | 41 ++++ .../combined-diff-review-host.test-fixture.ts | 83 ++++++++ .../editor/combined-diff-viewport.ts | 6 + .../combined-diff-section-list.tsx | 7 +- .../pr-files-combined-diff-body.tsx | 11 +- .../files/combined-diff-viewer.tsx | 11 +- 10 files changed, 358 insertions(+), 3 deletions(-) create mode 100644 src/renderer/src/components/editor/combined-diff-cursor-reveal.test.tsx create mode 100644 src/renderer/src/components/editor/combined-diff-cursor-reveal.ts create mode 100644 src/renderer/src/components/editor/combined-diff-review-host.test-fixture.ts create mode 100644 src/renderer/src/components/editor/combined-diff-viewport.ts diff --git a/src/renderer/src/components/editor/DiffSectionBody.tsx b/src/renderer/src/components/editor/DiffSectionBody.tsx index 436dfe665d8..33ad8bc63ff 100644 --- a/src/renderer/src/components/editor/DiffSectionBody.tsx +++ b/src/renderer/src/components/editor/DiffSectionBody.tsx @@ -19,6 +19,7 @@ import { } from './diff-editor-word-wrap-options' import { monacoFindOptions } from './monaco-find-options' import { installDiffEditorShiftWheelScroll } from './diff-editor-shift-wheel-scroll' +import { installCombinedDiffCursorReveal } from './combined-diff-cursor-reveal' import { usePluginSyntaxLanguage } from './use-plugin-syntax-language' import { PLUGIN_SYNTAX_EDITOR_OPTIONS, @@ -87,8 +88,10 @@ export function DiffSectionBody({ diffEditorRef.current = diffEditor applyModelLanguage([diffEditor.getModel()?.original, diffEditor.getModel()?.modified], language) const cleanupShiftWheelScroll = installDiffEditorShiftWheelScroll(diffEditor) + const cleanupCursorReveal = installCombinedDiffCursorReveal(diffEditor) diffEditor.getModifiedEditor().onDidDispose(() => { cleanupShiftWheelScroll() + cleanupCursorReveal() if (diffEditorRef.current !== diffEditor) { return } diff --git a/src/renderer/src/components/editor/DiffSectionBody.word-wrap-lifecycle.test.tsx b/src/renderer/src/components/editor/DiffSectionBody.word-wrap-lifecycle.test.tsx index 05d26475f9b..8b4c6fa245a 100644 --- a/src/renderer/src/components/editor/DiffSectionBody.word-wrap-lifecycle.test.tsx +++ b/src/renderer/src/components/editor/DiffSectionBody.word-wrap-lifecycle.test.tsx @@ -16,6 +16,7 @@ const { mountedEditors, syncWordWrap, cleanupShiftWheel, createEditor } = vi.hoi return { editor: { getModifiedEditor: () => modified, + getContainerDomNode: () => document.createElement('div'), getModel: () => null, onDidDispose: vi.fn(() => ({ dispose: vi.fn() })) }, diff --git a/src/renderer/src/components/editor/DiffSectionHeader.tsx b/src/renderer/src/components/editor/DiffSectionHeader.tsx index 077e2868ef3..910b13cbfdf 100644 --- a/src/renderer/src/components/editor/DiffSectionHeader.tsx +++ b/src/renderer/src/components/editor/DiffSectionHeader.tsx @@ -1,3 +1,4 @@ +import { combinedDiffHeaderAttributes } from './combined-diff-viewport' import { ChevronDown, ChevronRight, ExternalLink, Eye } from 'lucide-react' import type { MouseEvent, ReactElement, ReactNode } from 'react' import { translate } from '@/i18n/i18n' @@ -29,6 +30,7 @@ export function DiffSectionHeader({ const displayPath = splitPathForDisplay(path) return (
diff --git a/src/renderer/src/components/editor/combined-diff-cursor-reveal.test.tsx b/src/renderer/src/components/editor/combined-diff-cursor-reveal.test.tsx new file mode 100644 index 00000000000..82ec05b0195 --- /dev/null +++ b/src/renderer/src/components/editor/combined-diff-cursor-reveal.test.tsx @@ -0,0 +1,196 @@ +// @vitest-environment happy-dom +import { useLayoutEffect, useRef } from 'react' +import { act, cleanup, render } from '@testing-library/react' +import { afterAll, afterEach, expect, it, vi } from 'vitest' +import * as monaco from 'monaco-editor/esm/vs/editor/editor.api.js' +import type { DiffOnMount } from '@monaco-editor/react' +import { installCombinedDiffCursorReveal } from './combined-diff-cursor-reveal' +import { TooltipProvider } from '@/components/ui/tooltip' +import { + prFilesDiffScrollTopCache, + prFilesDiffViewStateCache +} from '../pull-request-page/cache/files-diff-view' +import { + reviewHostFixture, + reviewHostComponents, + originalContent, + modifiedContent +} from './combined-diff-review-host.test-fixture' + +// Happy DOM has no canvas adapter; drawing does not participate in cursor geometry. +const canvasContext = Object.getOwnPropertyDescriptor(HTMLCanvasElement.prototype, 'getContext')! +Object.defineProperty(HTMLCanvasElement.prototype, 'getContext', { + configurable: true, + value: () => ({ + clearRect() {}, + fillRect() {}, + beginPath() {}, + moveTo() {}, + lineTo() {}, + stroke() {} + }) +}) +afterAll(() => Object.defineProperty(HTMLCanvasElement.prototype, 'getContext', canvasContext)) +const disposers: (() => void)[] = [] +let diff: monaco.editor.IStandaloneDiffEditor +let viewport: HTMLDivElement + +function layoutBox(node: HTMLElement, rectangle: () => DOMRect) { + node.getBoundingClientRect = rectangle + node.getClientRects = () => Object.assign([rectangle()], { item: () => rectangle() }) +} +function configureViewport(node: HTMLDivElement) { + if (viewport === node) { + return + } + viewport = node + Object.defineProperties(node, { + clientHeight: { value: 300, configurable: true }, + offsetHeight: { value: 300, configurable: true }, + offsetWidth: { value: 1000, configurable: true }, + scrollHeight: { value: 2500, configurable: true } + }) + layoutBox(node, () => new DOMRect(0, 100, 1000, 300)) +} +function mountEditor(node: HTMLDivElement, options: monaco.editor.IDiffEditorConstructionOptions) { + const editor = monaco.editor.createDiffEditor(node, { + ...options, + automaticLayout: false, + dimension: { width: 1000, height: 2200 }, + lineHeight: 20, + fontSize: 13, + renderSideBySideInlineBreakpoint: 0, + hideUnchangedRegions: { enabled: false } + }) + const original = monaco.editor.createModel(originalContent) + const modified = monaco.editor.createModel(modifiedContent) + editor.setModel({ original, modified }) + for (const pane of [editor.getOriginalEditor(), editor.getModifiedEditor()]) { + layoutBox(pane.getContainerDomNode(), () => new DOMRect(0, 130 - viewport.scrollTop, 500, 2200)) + } + layoutBox(node, () => new DOMRect(0, 130 - viewport.scrollTop, 1000, 2200)) + const header = node + .closest('[data-combined-diff-section-row]') + ?.querySelector('[data-combined-diff-section-header]') + if (header) { + layoutBox(header, () => new DOMRect(0, 100, 1000, 30)) + } + return { + editor, + dispose: () => { + editor.dispose() + original.dispose() + modified.dispose() + } + } +} +vi.mock('@monaco-editor/react', () => ({ + DiffEditor: ({ + onMount, + options + }: { + onMount: DiffOnMount + options: monaco.editor.IDiffEditorConstructionOptions + }) => { + const ref = useRef(null) + const initial = useRef({ onMount, options }) + useLayoutEffect(() => { + if (!ref.current) { + throw new Error('Missing editor host') + } + const mounted = mountEditor(ref.current, initial.current.options) + diff = mounted.editor + initial.current.onMount(diff, monaco) + return mounted.dispose + }, []) + return
+ } +})) +afterEach(() => { + cleanup() + disposers.splice(0).forEach((dispose) => dispose()) + prFilesDiffScrollTopCache.clear() + prFilesDiffViewStateCache.clear() + document.body.replaceChildren() + reviewHostFixture.configureViewport = undefined +}) + +function assertCursorVisible(pane: monaco.editor.IStandaloneCodeEditor) { + const cursor = pane.getScrolledVisiblePosition(pane.getPosition()!)! + const top = pane.getContainerDomNode().getBoundingClientRect().top + cursor.top + expect(top).toBeGreaterThanOrEqual(130) + expect(top + cursor.height).toBeLessThanOrEqual(400) +} +function navigate(pane: monaco.editor.IStandaloneCodeEditor) { + pane.focus() + pane.setPosition({ lineNumber: 1, column: 1 }) + viewport.scrollTop = 0 + for (let index = 0; index < 30; index++) { + pane.trigger('keyboard', 'cursorDown', null) + } + expect(pane.getPosition()?.lineNumber).toBe(31) + expect(viewport.scrollTop).toBeGreaterThan(0) + assertCursorVisible(pane) + pane.trigger('keyboard', 'cursorTop', null) + assertCursorVisible(pane) +} + +it.each([true, false])( + 'reveals keyboard navigation in the outer viewport (split=%s)', + async (split) => { + const outer = document.createElement('div') + outer.setAttribute('data-combined-diff-viewport', '') + outer.innerHTML = + '
' + document.body.append(outer) + configureViewport(outer) + const mounted = mountEditor(outer.querySelector('.editor-host')!, { + renderSideBySide: split + }) + disposers.push(mounted.dispose) + disposers.push(installCombinedDiffCursorReveal(mounted.editor)) + await vi.waitFor(() => expect(mounted.editor.getLineChanges()).not.toBeNull()) + for (const pane of [mounted.editor.getOriginalEditor(), mounted.editor.getModifiedEditor()]) { + navigate(pane) + const offset = viewport.scrollTop + pane.setPosition({ lineNumber: 90, column: 1 }, 'mouse') + expect(viewport.scrollTop).toBe(offset) + pane.setPosition({ lineNumber: 1, column: 1 }, 'api') + expect(viewport.scrollTop).toBe(offset) + } + } +) + +it.each(['page', 'dialog'] as const)( + 'reveals keyboard navigation through the actual PR Files %s host', + async (host) => { + reviewHostFixture.configureViewport = configureViewport + const Viewer = reviewHostComponents[host] + render( + + ()} + onCommentAdded={() => {}} + onViewedChange={async () => true} + /> + + ) + await vi.waitFor(() => expect(viewport?.querySelector('.monaco-diff-editor')).toBeTruthy()) + await vi.waitFor(() => expect(diff.getLineChanges()).not.toBeNull()) + expect(viewport.hasAttribute('data-combined-diff-viewport')).toBe(true) + expect( + viewport.querySelector('[data-combined-diff-section-row] [data-combined-diff-section-header]') + ).not.toBeNull() + act(() => navigate(diff.getModifiedEditor())) + } +) diff --git a/src/renderer/src/components/editor/combined-diff-cursor-reveal.ts b/src/renderer/src/components/editor/combined-diff-cursor-reveal.ts new file mode 100644 index 00000000000..830765892ef --- /dev/null +++ b/src/renderer/src/components/editor/combined-diff-cursor-reveal.ts @@ -0,0 +1,41 @@ +import type { editor } from 'monaco-editor' +import { + COMBINED_DIFF_VIEWPORT_SELECTOR, + COMBINED_DIFF_ROW_SELECTOR, + COMBINED_DIFF_HEADER_SELECTOR +} from './combined-diff-viewport' +import { hasLayoutBox } from '@/hooks/virtualized-hidden-row-measurement' + +export function installCombinedDiffCursorReveal(diff: editor.IStandaloneDiffEditor): () => void { + const root = diff.getContainerDomNode() + const viewport = root.closest(COMBINED_DIFF_VIEWPORT_SELECTOR) + if (!viewport) { + return () => {} + } + const header = root + .closest(COMBINED_DIFF_ROW_SELECTOR) + ?.querySelector(COMBINED_DIFF_HEADER_SELECTOR) + const subscriptions = [diff.getOriginalEditor(), diff.getModifiedEditor()].map((pane) => + pane.onDidChangeCursorPosition((event) => { + const node = pane.getContainerDomNode() + if (event.source !== 'keyboard' || !node || !hasLayoutBox(node) || !hasLayoutBox(viewport)) { + return + } + const cursor = pane.getScrolledVisiblePosition(event.position) + if (!cursor) { + return + } + const viewportTop = viewport.getBoundingClientRect().top + viewport.clientTop + const top = node.getBoundingClientRect().top + cursor.top + const visibleTop = viewportTop + (header?.getBoundingClientRect().height ?? 0) + const visibleBottom = viewportTop + viewport.clientHeight + const delta = + top < visibleTop ? top - visibleTop : Math.max(0, top + cursor.height - visibleBottom) + // Keyboard reveals are user intent; unmarked scrolls update the existing outer anchor. + if (delta !== 0) { + viewport.scrollTop += delta + } + }) + ) + return () => subscriptions.forEach((subscription) => subscription.dispose()) +} diff --git a/src/renderer/src/components/editor/combined-diff-review-host.test-fixture.ts b/src/renderer/src/components/editor/combined-diff-review-host.test-fixture.ts new file mode 100644 index 00000000000..c9bf539a8a2 --- /dev/null +++ b/src/renderer/src/components/editor/combined-diff-review-host.test-fixture.ts @@ -0,0 +1,83 @@ +import React from 'react' +import { vi } from 'vitest' +import type * as VirtualBindings from '@tanstack/react-virtual' +import type { DiffSection } from './diff-section-types' +import { PRFilesCombinedDiffViewer as PageViewer } from '../pull-request-page/files/combined-diff-viewer' +import { PRFilesCombinedDiffViewer as DialogViewer } from '../github-item-dialog/inspect-pull-request/pr-files-combined-diff-viewer' + +export const reviewHostFixture: { + configureViewport?: (node: HTMLDivElement) => void +} = {} +export const reviewHostComponents = { page: PageViewer, dialog: DialogViewer } +export const originalContent = Array.from({ length: 100 }, (_, i) => `line ${i}`).join('\n') +export const modifiedContent = originalContent.replace('line 10\n', 'changed\n') + +type SectionSetter = React.Dispatch> +const loadSection = ({ index, setSections }: { index: number; setSections: SectionSetter }) => + setSections((prev) => + prev[index]?.loading + ? prev.map((section, row) => + row === index ? { ...section, originalContent, modifiedContent, loading: false } : section + ) + : prev + ) +vi.mock('@tanstack/react-virtual', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useVirtualizer: ( + options: Parameters>[0] + ) => + actual.useVirtualizer({ + ...options, + initialRect: { width: 1000, height: 300 }, + getScrollElement: () => { + const node = options.getScrollElement() + if (node) { + reviewHostFixture.configureViewport?.(node) + } + return node + } + }) + } +}) +vi.mock('@/store', () => ({ + useAppStore: (selector: (state: Record) => unknown) => + selector({ + settings: { theme: 'dark', diffWordWrap: true }, + editorFontZoomLevel: 0, + worktreesByRepo: {}, + runtimeEnvironments: [], + folderWorkspaces: [] + }) +})) +vi.mock('@/hooks/use-document-dark-theme', () => ({ useDocumentDarkTheme: () => false })) +vi.mock('@/lib/monaco-setup', async () => ({ + monaco: await import('monaco-editor/esm/vs/editor/editor.api.js') +})) +vi.mock('../diff-comments/useDiffCommentDecorator', () => ({ useDiffCommentDecorator: () => {} })) +vi.mock('./combined-diff/browse-files/combined-diff-file-tree', () => ({ + CombinedDiffFileTree: () => null +})) +vi.mock('../pull-request-page/files/toolbar', () => ({ PRFilesDiffToolbar: () => null })) +vi.mock('../github-item-dialog/inspect-pull-request/pr-files-combined-diff-toolbar', () => ({ + PRFilesCombinedDiffToolbar: () => null +})) +vi.mock('../pull-request-page/files/section-loader', () => ({ + usePRFileSectionLoader: ({ setSections }: { setSections: SectionSetter }) => ({ + loadSection: React.useCallback( + (index: number) => loadSection({ index, setSections }), + [setSections] + ), + retrySection: () => {}, + toggleSection: () => {}, + setAllSectionsCollapsed: () => {} + }) +})) +vi.mock('../github-item-dialog/inspect-pull-request/pr-files-combined-diff-load', () => ({ + loadPRFilesCombinedDiffSection: (args: Parameters[0]) => loadSection(args), + retryPRFilesCombinedDiffSection: () => {}, + togglePRFilesCombinedDiffSection: () => {}, + setAllPRFilesCombinedDiffSectionsCollapsed: () => {}, + addPRFilesCombinedDiffLineComment: () => Promise.resolve(false) +})) diff --git a/src/renderer/src/components/editor/combined-diff-viewport.ts b/src/renderer/src/components/editor/combined-diff-viewport.ts new file mode 100644 index 00000000000..a9ac31f3946 --- /dev/null +++ b/src/renderer/src/components/editor/combined-diff-viewport.ts @@ -0,0 +1,6 @@ +export const combinedDiffViewportAttributes = { 'data-combined-diff-viewport': '' } as const +export const combinedDiffRowAttributes = { 'data-combined-diff-section-row': '' } as const +export const combinedDiffHeaderAttributes = { 'data-combined-diff-section-header': '' } as const +export const COMBINED_DIFF_VIEWPORT_SELECTOR = '[data-combined-diff-viewport]' +export const COMBINED_DIFF_ROW_SELECTOR = '[data-combined-diff-section-row]' +export const COMBINED_DIFF_HEADER_SELECTOR = '[data-combined-diff-section-header]' diff --git a/src/renderer/src/components/editor/combined-diff/scroll-viewport/combined-diff-section-list.tsx b/src/renderer/src/components/editor/combined-diff/scroll-viewport/combined-diff-section-list.tsx index ff1a0a24874..558ac9e28ff 100644 --- a/src/renderer/src/components/editor/combined-diff/scroll-viewport/combined-diff-section-list.tsx +++ b/src/renderer/src/components/editor/combined-diff/scroll-viewport/combined-diff-section-list.tsx @@ -1,3 +1,7 @@ +import { + combinedDiffViewportAttributes, + combinedDiffRowAttributes +} from '../../combined-diff-viewport' import type React from 'react' import { useMemo } from 'react' import type { Virtualizer } from '@tanstack/react-virtual' @@ -82,6 +86,7 @@ export function CombinedDiffSectionList({ return (
-
+
{virtualizer.getVirtualItems().map((virtualItem) => { const section = sections[virtualItem.index] @@ -118,6 +126,7 @@ export function PRFilesCombinedDiffBody({ return (
-
+
{virtualizer.getVirtualItems().map((virtualItem) => { const section = sections[virtualItem.index] @@ -339,6 +347,7 @@ export function PRFilesCombinedDiffViewer({ return (