From b93ec86fef52579ae655dde21714bedfbceb2f81 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 12 Jul 2026 14:59:18 -0700 Subject: [PATCH] Add Previous/Next change navigation buttons to the Diff View (#6668) * feat(diff): add Previous/Next change navigation buttons to Diff View Add up/down navigation buttons to the single-file Diff View toolbar so users can jump between change regions (hunks) without scrolling, matching IntelliJ/Android Studio. The buttons bridge the Monaco diff editor to the header via an instance-scoped DiffNavigationProvider and call Monaco's goToDiff('previous'|'next'); they disable when the file has no changes. Closes #6215 Co-authored-by: Orca * test(diff): cover stale onDidUpdateDiff guard in fast-swap Address CodeRabbit nitpick: prove that an update fired from a replaced editor is ignored after a new editor registers (the subscription is disposed on re-register). The fake editor's dispose now clears its callback, matching real Monaco subscription semantics. Co-authored-by: Orca * fix(diff): isolate navigation registration updates * fix(diff): use arrows for change navigation * fix(editor): tighten header action spacing --------- Co-authored-by: Orca Co-authored-by: Brennan Benson --- src/renderer/src/assets/main.css | 3 +- .../src/components/editor/DiffViewer.tsx | 14 +- .../src/components/editor/EditorPanel.tsx | 104 ++++++------ .../components/editor/EditorPanelHeader.tsx | 50 +++++- .../editor/diff-navigation-context.test.tsx | 156 ++++++++++++++++++ .../editor/diff-navigation-context.tsx | 115 +++++++++++++ src/renderer/src/i18n/locales/en.json | 4 +- src/renderer/src/i18n/locales/es.json | 4 +- src/renderer/src/i18n/locales/ja.json | 4 +- src/renderer/src/i18n/locales/ko.json | 4 +- src/renderer/src/i18n/locales/zh.json | 4 +- 11 files changed, 403 insertions(+), 59 deletions(-) create mode 100644 src/renderer/src/components/editor/diff-navigation-context.test.tsx create mode 100644 src/renderer/src/components/editor/diff-navigation-context.tsx diff --git a/src/renderer/src/assets/main.css b/src/renderer/src/assets/main.css index 25a5a4e75..1d4cbac05 100644 --- a/src/renderer/src/assets/main.css +++ b/src/renderer/src/assets/main.css @@ -970,7 +970,8 @@ .editor-header { display: flex; align-items: center; - gap: 12px; + /* Keep adjacent actions aligned with the app's compact toolbar spacing. */ + gap: 4px; min-height: 36px; padding: 6px 14px; background: var(--editor-surface); diff --git a/src/renderer/src/components/editor/DiffViewer.tsx b/src/renderer/src/components/editor/DiffViewer.tsx index c119abee4..dbeb8fca2 100644 --- a/src/renderer/src/components/editor/DiffViewer.tsx +++ b/src/renderer/src/components/editor/DiffViewer.tsx @@ -24,6 +24,7 @@ import { useDiffViewerLargeDiffLifecycle } from './useDiffViewerLargeDiffLifecyc import { getDiffViewerLargeDiffSaveAction } from './diff-viewer-large-diff-save-action' import type { DiffViewerProps } from './diff-viewer-props' import { buildDiffEditorWordWrapOptions } from './diff-editor-word-wrap-options' +import { useDiffEditorRegistration } from './diff-navigation-context' export default function DiffViewer({ modelKey, @@ -72,6 +73,7 @@ export default function DiffViewer({ (settings?.theme === 'system' && window.matchMedia('(prefers-color-scheme: dark)').matches) const diffEditorRef = useRef(null) + const { registerDiffEditor, unregisterDiffEditor } = useDiffEditorRegistration() const diffBodyRef = useRef(null) const lineNumberOptionsSubRef = useRef<{ dispose: () => void } | null>(null) const [modifiedEditor, setModifiedEditor] = useState(null) @@ -244,10 +246,16 @@ export default function DiffViewer({ // must not keep comment decorators or save handlers talking to disposed UI. lineNumberOptionsSubRef.current?.dispose() lineNumberOptionsSubRef.current = null + // Why: capture before nulling so we unregister the exact instance the + // navigator may still hold (identity guard no-ops a stale dispose). + const fallenBackEditor = diffEditorRef.current diffEditorRef.current = null + if (fallenBackEditor) { + unregisterDiffEditor(fallenBackEditor) + } setModifiedEditor(null) setPopover(null) - }, []) + }, [unregisterDiffEditor]) const handleSubmitComment = async (body: string): Promise => { if (!popover) { @@ -307,6 +315,7 @@ export default function DiffViewer({ const handleMount: DiffOnMount = useCallback( (diffEditor, monaco) => { diffEditorRef.current = diffEditor + registerDiffEditor(diffEditor) lineNumberOptionsSubRef.current?.dispose() lineNumberOptionsSubRef.current = applyDiffEditorLineNumberOptions(diffEditor, sideBySide) @@ -363,11 +372,12 @@ export default function DiffViewer({ lineNumberOptionsSubRef.current?.dispose() lineNumberOptionsSubRef.current = null diffEditorRef.current = null + unregisterDiffEditor(diffEditor) setModifiedEditor(null) setPopover(null) }) }, - [editable, setupCopy, modelKey, filePath, sideBySide] + [editable, setupCopy, modelKey, filePath, sideBySide, registerDiffEditor, unregisterDiffEditor] ) // Why: VS Code snapshots diff view state on deactivation, not on scroll events. diff --git a/src/renderer/src/components/editor/EditorPanel.tsx b/src/renderer/src/components/editor/EditorPanel.tsx index 2181dfc13..21b116ea2 100644 --- a/src/renderer/src/components/editor/EditorPanel.tsx +++ b/src/renderer/src/components/editor/EditorPanel.tsx @@ -10,6 +10,7 @@ import { requestEditorFileSave } from './editor-autosave' import { exportActiveMarkdownToPdf } from './export-active-markdown' import type { EditorToggleValue } from './EditorViewToggle' import { EditorPanelShell } from './EditorPanelShell' +import { DiffNavigationProvider } from './diff-navigation-context' import { canUseChangesModeForFile } from './editor-panel-file-mode' import { getEditorPanelRenderModel } from './editor-panel-render-model' import { useClosedEditorTabCleanup } from './useClosedEditorTabCleanup' @@ -351,56 +352,59 @@ function EditorPanelInner({ markdownTableOfContentsVisible[markdownDocumentStateFileId] ?? false return ( - void handleCopyPath()} - onOpenDiffTargetFile={handleOpenDiffTargetFile} - onOpenPreviewToSide={handleOpenPreviewToSide} - onOpenMarkdownPreview={handleOpenMarkdownPreview} - onOpenContainingFolder={handleOpenContainingFolder} - onToggleSideBySide={() => setSideBySide((prev) => !prev)} - onEditorToggleChange={handleEditorToggleChange} - onToggleMarkdownTableOfContents={() => - setMarkdownTableOfContentsVisible( - markdownDocumentStateFileId, - !isMarkdownTableOfContentsVisible - ) - } - onToggleMarkdownFrontmatter={() => - setMarkdownFrontmatterVisible(markdownDocumentStateFileId, !isMarkdownFrontmatterVisible) - } - onExportMarkdownToPdf={() => - void exportActiveMarkdownToPdf({ fileId: activeFile.id, root: panelRef.current }) - } - onContentChange={handleContentChange} - onContentChangeForFile={handleContentChangeForFile} - onDirtyStateHint={handleDirtyStateHint} - onSave={handleSave} - onSaveForFile={handleSaveForFile} - onReloadContent={reloadContent} - onCloseMarkdownTableOfContents={() => - setMarkdownTableOfContentsVisible(markdownDocumentStateFileId, false) - } - onCloseRenameDialog={closeRenameDialog} - onRenameConfirm={handleRenameConfirm} - markdownAnnotationsEnabled={markdownAnnotationsEnabled} - /> + // Why: each split pane needs an isolated bridge between its diff editor and header controls. + + void handleCopyPath()} + onOpenDiffTargetFile={handleOpenDiffTargetFile} + onOpenPreviewToSide={handleOpenPreviewToSide} + onOpenMarkdownPreview={handleOpenMarkdownPreview} + onOpenContainingFolder={handleOpenContainingFolder} + onToggleSideBySide={() => setSideBySide((prev) => !prev)} + onEditorToggleChange={handleEditorToggleChange} + onToggleMarkdownTableOfContents={() => + setMarkdownTableOfContentsVisible( + markdownDocumentStateFileId, + !isMarkdownTableOfContentsVisible + ) + } + onToggleMarkdownFrontmatter={() => + setMarkdownFrontmatterVisible(markdownDocumentStateFileId, !isMarkdownFrontmatterVisible) + } + onExportMarkdownToPdf={() => + void exportActiveMarkdownToPdf({ fileId: activeFile.id, root: panelRef.current }) + } + onContentChange={handleContentChange} + onContentChangeForFile={handleContentChangeForFile} + onDirtyStateHint={handleDirtyStateHint} + onSave={handleSave} + onSaveForFile={handleSaveForFile} + onReloadContent={reloadContent} + onCloseMarkdownTableOfContents={() => + setMarkdownTableOfContentsVisible(markdownDocumentStateFileId, false) + } + onCloseRenameDialog={closeRenameDialog} + onRenameConfirm={handleRenameConfirm} + markdownAnnotationsEnabled={markdownAnnotationsEnabled} + /> + ) } diff --git a/src/renderer/src/components/editor/EditorPanelHeader.tsx b/src/renderer/src/components/editor/EditorPanelHeader.tsx index 6cbb2b6f6..44816a40e 100644 --- a/src/renderer/src/components/editor/EditorPanelHeader.tsx +++ b/src/renderer/src/components/editor/EditorPanelHeader.tsx @@ -1,5 +1,5 @@ import { useMemo } from 'react' -import { Columns2, Eye, FileText, ListTree, Rows2 } from 'lucide-react' +import { ArrowDown, ArrowUp, Columns2, Eye, FileText, ListTree, Rows2 } from 'lucide-react' import { useAppStore } from '@/store' import { selectWorktreeDiffCommentsOrEmpty } from '@/store/worktree-diff-comments-selector' import type { OpenFile } from '@/store/slices/editor' @@ -14,6 +14,7 @@ import { DiffNotesSendMenu } from './DiffNotesSendMenu' import { EditorPanelMarkdownActionsMenu } from './EditorPanelMarkdownActionsMenu' import { translate } from '@/i18n/i18n' import { EditorPanelHeaderPath } from './EditorPanelHeaderPath' +import { useDiffNavigation } from './diff-navigation-context' type EditorPanelHeaderProps = { activeFile: OpenFile @@ -92,6 +93,7 @@ export function EditorPanelHeader({ () => diffComments.filter((comment) => comment.filePath === activeFile.relativePath), [activeFile.relativePath, diffComments] ) + const { changeCount, goToPreviousDiff, goToNextDiff } = useDiffNavigation() return (
@@ -203,6 +205,52 @@ export function EditorPanelHeader({ )} + {isDiffSurface && ( + + + + + + + {translate('auto.components.editor.EditorPanelHeader.2076ecfc9c', 'Previous change')} + + + + )} + {isDiffSurface && ( + + + + + + + {translate('auto.components.editor.EditorPanelHeader.631dab0df3', 'Next change')} + + + + )} {hasEditorToggle && ( void + fireUpdate: () => void + goToDiff: ReturnType + disposeUpdate: ReturnType +} + +function createFakeEditor(initialCount: number): FakeDiffEditor { + let count = initialCount + let updateCallback: (() => void) | null = null + const disposeUpdate = vi.fn(() => { + updateCallback = null + }) + const editor = { + getLineChanges: () => (count > 0 ? Array.from({ length: count }, () => ({})) : []), + goToDiff: vi.fn(), + onDidUpdateDiff: (cb: () => void) => { + updateCallback = cb + return { + dispose: disposeUpdate + } + }, + setLineChanges: (next: number) => { + count = next + }, + fireUpdate: () => updateCallback?.(), + disposeUpdate + } as unknown as FakeDiffEditor + return editor +} + +let captured: DiffNavigationContextValue | null = null +let registration: DiffEditorRegistrationContextValue | null = null +let registrationRenderCount = 0 + +function Probe(): null { + captured = useDiffNavigation() + return null +} + +function RegistrationProbe(): null { + registration = useDiffEditorRegistration() + registrationRenderCount += 1 + return null +} + +describe('DiffNavigationProvider', () => { + let container: HTMLDivElement | null = null + let root: Root | null = null + + function mount(): void { + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + act(() => { + root?.render( + + + + + ) + }) + } + + afterEach(() => { + if (root) { + act(() => root?.unmount()) + } + container?.remove() + container = null + root = null + captured = null + registration = null + registrationRenderCount = 0 + }) + + it('exposes the change count and routes nav actions to the registered editor', () => { + mount() + const editor = createFakeEditor(3) + act(() => registration?.registerDiffEditor(editor)) + + expect(captured?.changeCount).toBe(3) + + act(() => captured?.goToNextDiff()) + expect(editor.goToDiff).toHaveBeenCalledWith('next') + + act(() => captured?.goToPreviousDiff()) + expect(editor.goToDiff).toHaveBeenCalledWith('previous') + }) + + it('re-renders when onDidUpdateDiff flips the count 0 -> N (count is state)', () => { + mount() + const editor = createFakeEditor(0) + act(() => registration?.registerDiffEditor(editor)) + expect(captured?.changeCount).toBe(0) + + act(() => { + editor.setLineChanges(2) + editor.fireUpdate() + }) + expect(captured?.changeCount).toBe(2) + expect(registrationRenderCount).toBe(1) + }) + + it('ignores a stale unregister for an editor that is no longer current (identity guard)', () => { + mount() + const oldEditor = createFakeEditor(1) + const newEditor = createFakeEditor(4) + + // Fast-swap: new editor registers before the old one's dispose fires. + act(() => registration?.registerDiffEditor(oldEditor)) + act(() => registration?.registerDiffEditor(newEditor)) + expect(captured?.changeCount).toBe(4) + expect(oldEditor.disposeUpdate).toHaveBeenCalledOnce() + + // A stale update from the old editor must not flip the count back: registering + // the new editor disposed the old subscription, so its callback no longer fires. + act(() => { + oldEditor.setLineChanges(9) + oldEditor.fireUpdate() + }) + expect(captured?.changeCount).toBe(4) + + act(() => registration?.unregisterDiffEditor(oldEditor)) + + // New editor's count is intact and nav still routes to it. + expect(captured?.changeCount).toBe(4) + act(() => captured?.goToNextDiff()) + expect(newEditor.goToDiff).toHaveBeenCalledWith('next') + expect(oldEditor.goToDiff).not.toHaveBeenCalled() + }) + + it('disposes the active diff update subscription when the provider unmounts', () => { + mount() + const editor = createFakeEditor(1) + act(() => registration?.registerDiffEditor(editor)) + + act(() => root?.unmount()) + + expect(editor.disposeUpdate).toHaveBeenCalledOnce() + root = null + }) +}) diff --git a/src/renderer/src/components/editor/diff-navigation-context.tsx b/src/renderer/src/components/editor/diff-navigation-context.tsx new file mode 100644 index 000000000..da1f37c90 --- /dev/null +++ b/src/renderer/src/components/editor/diff-navigation-context.tsx @@ -0,0 +1,115 @@ +import { createContext, useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react' +import type { editor } from 'monaco-editor' + +export type DiffEditorRegistrationContextValue = { + registerDiffEditor: (editor: editor.IStandaloneDiffEditor) => void + unregisterDiffEditor: (editor: editor.IStandaloneDiffEditor) => void +} + +export type DiffNavigationContextValue = { + goToPreviousDiff: () => void + goToNextDiff: () => void + changeCount: number +} + +const noop = (): void => {} + +// Why: registration stays separate from changeCount so diff recomputation only +// rerenders the header controls, not the heavy Monaco DiffViewer consumer. +const DiffEditorRegistrationContext = createContext({ + registerDiffEditor: noop, + unregisterDiffEditor: noop +}) + +const DiffNavigationContext = createContext({ + goToPreviousDiff: noop, + goToNextDiff: noop, + changeCount: 0 +}) + +function countChanges(diffEditor: editor.IStandaloneDiffEditor): number { + return diffEditor.getLineChanges()?.length ?? 0 +} + +export function DiffNavigationProvider({ + children +}: { + children: React.ReactNode +}): React.JSX.Element { + const editorRef = useRef(null) + const updateSubRef = useRef<{ dispose: () => void } | null>(null) + // Why: changeCount must be state, not a ref — the header is a sibling consumer + // and only re-renders (enabling the buttons) when the value object identity + // changes on the 0 -> N flip once the diff computation lands. + const [changeCount, setChangeCount] = useState(0) + + const registerDiffEditor = useCallback((diffEditor: editor.IStandaloneDiffEditor) => { + editorRef.current = diffEditor + // Hold at most one update subscription; replace any prior editor's. + updateSubRef.current?.dispose() + updateSubRef.current = diffEditor.onDidUpdateDiff(() => { + // Why: ignore updates from an editor that is no longer current so a stale + // subscription in the fast-swap case can't write a wrong count. + if (editorRef.current === diffEditor) { + setChangeCount(countChanges(diffEditor)) + } + }) + setChangeCount(countChanges(diffEditor)) + }, []) + + const unregisterDiffEditor = useCallback((diffEditor: editor.IStandaloneDiffEditor) => { + // Why: identity guard for the fast-swap race — a stale dispose carrying the + // old editor must not wipe a freshly-registered new one. + if (editorRef.current !== diffEditor) { + return + } + updateSubRef.current?.dispose() + updateSubRef.current = null + editorRef.current = null + setChangeCount(0) + }, []) + + const goToPreviousDiff = useCallback(() => { + editorRef.current?.goToDiff('previous') + }, []) + + const goToNextDiff = useCallback(() => { + editorRef.current?.goToDiff('next') + }, []) + + useEffect(() => { + return () => { + updateSubRef.current?.dispose() + updateSubRef.current = null + } + }, []) + + const registrationValue = useMemo( + () => ({ registerDiffEditor, unregisterDiffEditor }), + [registerDiffEditor, unregisterDiffEditor] + ) + const navigationValue = useMemo( + () => ({ + goToPreviousDiff, + goToNextDiff, + changeCount + }), + [goToPreviousDiff, goToNextDiff, changeCount] + ) + + return ( + + + {children} + + + ) +} + +export function useDiffEditorRegistration(): DiffEditorRegistrationContextValue { + return useContext(DiffEditorRegistrationContext) +} + +export function useDiffNavigation(): DiffNavigationContextValue { + return useContext(DiffNavigationContext) +} diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 007096cb8..91d1c051f 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -11436,7 +11436,9 @@ "c98ce191da": "This diff has no modified-side file to open", "9b80bbe1de": "Open file tab", "f0fd4174b5": "Open file tab to use rich markdown editing", - "a10d9b8337": "Open file" + "a10d9b8337": "Open file", + "2076ecfc9c": "Previous change", + "631dab0df3": "Next change" }, "EditorPanelMarkdownActionsMenu": { "3e0ce48c24": "Export as PDF", diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 2053adb00..202deadee 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -11436,7 +11436,9 @@ "c98ce191da": "Este diff no tiene ningún archivo del lado modificado para abrir", "9b80bbe1de": "Abrir pestaña de archivo", "f0fd4174b5": "Abre la pestaña de archivo para usar la edición enriquecida de Markdown", - "a10d9b8337": "Abrir archivo" + "a10d9b8337": "Abrir archivo", + "2076ecfc9c": "Previous change", + "631dab0df3": "Next change" }, "EditorPanelMarkdownActionsMenu": { "3e0ce48c24": "Exportar como PDF", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 1c8114126..19e894507 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -11436,7 +11436,9 @@ "c98ce191da": "この差分には開くための変更側ファイルがありません", "9b80bbe1de": "ファイルタブを開く", "f0fd4174b5": "ファイル タブを開いてリッチ markdown 編集を使用する", - "a10d9b8337": "ファイルを開く" + "a10d9b8337": "ファイルを開く", + "2076ecfc9c": "Previous change", + "631dab0df3": "Next change" }, "EditorPanelMarkdownActionsMenu": { "3e0ce48c24": "PDFとしてエクスポート", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 1851bc405..48782e1f3 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -11436,7 +11436,9 @@ "c98ce191da": "이 차이점에는 열 수 있는 수정된 측면 파일이 없습니다.", "9b80bbe1de": "파일 탭 열기", "f0fd4174b5": "풍부한 markdown 편집을 사용하려면 파일 탭을 엽니다.", - "a10d9b8337": "파일 열기" + "a10d9b8337": "파일 열기", + "2076ecfc9c": "Previous change", + "631dab0df3": "Next change" }, "EditorPanelMarkdownActionsMenu": { "3e0ce48c24": "PDF로 내보내기", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 60632816a..f967af9d2 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -11436,7 +11436,9 @@ "c98ce191da": "此差异没有可打开的修改端文件", "9b80bbe1de": "打开文件选项卡", "f0fd4174b5": "打开文件选项卡以使用丰富的 Markdown 编辑", - "a10d9b8337": "打开文件" + "a10d9b8337": "打开文件", + "2076ecfc9c": "Previous change", + "631dab0df3": "Next change" }, "EditorPanelMarkdownActionsMenu": { "3e0ce48c24": "导出为 PDF",