From a5f28f265cad17669883d7dacb5bf3cc99e91dbb Mon Sep 17 00:00:00 2001 From: Wooseong Kim <2222333+innocarpe@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:51:56 +0900 Subject: [PATCH] Fix word wrap for both panes in side-by-side diffs Forward wrapping to both diff panes through the existing editor option path and clean up listeners. Co-authored-by: Wooseong Kim Co-authored-by: Cursor Co-authored-by: Neil --- .../src/components/editor/DiffSectionBody.tsx | 60 +++++- ...ffSectionBody.word-wrap-lifecycle.test.tsx | 182 ++++++++++++++++++ .../src/components/editor/DiffViewer.tsx | 59 ++++-- .../DiffViewer.word-wrap-lifecycle.test.tsx | 154 +++++++++++++++ .../diff-editor-word-wrap-options.test.ts | 165 +++++++++++++++- .../editor/diff-editor-word-wrap-options.ts | 91 ++++++++- tests/e2e/diff-word-wrap.spec.ts | 117 +++++++++++ 7 files changed, 804 insertions(+), 24 deletions(-) create mode 100644 src/renderer/src/components/editor/DiffSectionBody.word-wrap-lifecycle.test.tsx create mode 100644 src/renderer/src/components/editor/DiffViewer.word-wrap-lifecycle.test.tsx create mode 100644 tests/e2e/diff-word-wrap.spec.ts diff --git a/src/renderer/src/components/editor/DiffSectionBody.tsx b/src/renderer/src/components/editor/DiffSectionBody.tsx index 5ac46e17538..02cb001093a 100644 --- a/src/renderer/src/components/editor/DiffSectionBody.tsx +++ b/src/renderer/src/components/editor/DiffSectionBody.tsx @@ -1,3 +1,5 @@ +import { useEffect, useLayoutEffect, useRef } from 'react' +import type { editor } from 'monaco-editor' import { lazyWithRetry as lazy } from '@/lib/lazy-with-retry' import { AlertCircle, RefreshCw } from 'lucide-react' import { DiffEditor, type DiffOnMount } from '@monaco-editor/react' @@ -10,7 +12,10 @@ import { translate } from '@/i18n/i18n' import { LargeDiffFallback } from './LargeDiffFallback' import { LargeDiffLoadPrompt } from './LargeDiffLoadPrompt' import { buildDiffEditorWhitespaceOptions } from './diff-editor-whitespace-options' -import { buildDiffEditorWordWrapOptions } from './diff-editor-word-wrap-options' +import { + buildDiffEditorWordWrapOptions, + syncDiffEditorOriginalWordWrap +} from './diff-editor-word-wrap-options' import { monacoFindOptions } from './monaco-find-options' import { installDiffEditorShiftWheelScroll } from './diff-editor-shift-wheel-scroll' @@ -58,12 +63,57 @@ export function DiffSectionBody({ onMount }: DiffSectionBodyProps): React.JSX.Element { const renderLimit = section.largeDiffRenderLimit?.limited ? section.largeDiffRenderLimit : null - const handleEditorMount: DiffOnMount = (editor, monaco) => { - const cleanupShiftWheelScroll = installDiffEditorShiftWheelScroll(editor) - editor.onDidDispose(cleanupShiftWheelScroll) - onMount(editor, monaco) + const diffEditorRef = useRef(null) + const wordWrapOptionsSubRef = useRef<{ dispose: () => void } | null>(null) + const wordWrapMountFrameRef = useRef(0) + const diffWordWrapRef = useRef(diffWordWrap) + useLayoutEffect(() => { + diffWordWrapRef.current = diffWordWrap + }, [diffWordWrap]) + const handleEditorMount: DiffOnMount = (diffEditor, monaco) => { + diffEditorRef.current = diffEditor + const cleanupShiftWheelScroll = installDiffEditorShiftWheelScroll(diffEditor) + diffEditor.getModifiedEditor().onDidDispose(() => { + cleanupShiftWheelScroll() + if (diffEditorRef.current !== diffEditor) { + return + } + cancelAnimationFrame(wordWrapMountFrameRef.current) + wordWrapOptionsSubRef.current?.dispose() + wordWrapOptionsSubRef.current = null + diffEditorRef.current = null + }) + // Why: Monaco applies the inline-layout wrap override after mount, once width is known. + wordWrapMountFrameRef.current = requestAnimationFrame(() => { + if (diffEditorRef.current !== diffEditor) { + return + } + wordWrapOptionsSubRef.current?.dispose() + wordWrapOptionsSubRef.current = syncDiffEditorOriginalWordWrap( + diffEditor, + diffWordWrapRef.current + ) + }) + onMount(diffEditor, monaco) } + useEffect(() => { + cancelAnimationFrame(wordWrapMountFrameRef.current) + const diffEditor = diffEditorRef.current + if (!diffEditor) { + return () => { + cancelAnimationFrame(wordWrapMountFrameRef.current) + } + } + wordWrapOptionsSubRef.current?.dispose() + wordWrapOptionsSubRef.current = syncDiffEditorOriginalWordWrap(diffEditor, diffWordWrap) + return () => { + cancelAnimationFrame(wordWrapMountFrameRef.current) + wordWrapOptionsSubRef.current?.dispose() + wordWrapOptionsSubRef.current = null + } + }, [diffWordWrap, sideBySide]) + return (
{ + function createEditor() { + const listeners = new Set<() => void>() + const modified = { + onDidDispose: (listener: () => void) => { + listeners.add(listener) + return { dispose: () => listeners.delete(listener) } + } + } + return { + editor: { + getModifiedEditor: () => modified, + onDidDispose: vi.fn(() => ({ dispose: vi.fn() })) + }, + disposeModified: () => listeners.forEach((listener) => listener()) + } + } + const mountedEditors: ReturnType[] = [] + return { + mountedEditors, + syncWordWrap: vi.fn(() => ({ dispose: vi.fn() })), + cleanupShiftWheel: vi.fn(), + createEditor + } +}) + +vi.mock('@monaco-editor/react', () => ({ + DiffEditor: ({ + onMount + }: { + onMount: (editor: (typeof mountedEditors)[number]['editor']) => void + }) => { + const mount = useRef(onMount) + useEffect(() => { + const instance = createEditor() + mountedEditors.push(instance) + let disposed = false + // Monaco's React wrapper mounts asynchronously, after the parent's first effect. + queueMicrotask(() => { + if (!disposed) { + mount.current(instance.editor) + } + }) + return () => { + disposed = true + instance.disposeModified() + } + }, []) + return
+ } +})) +vi.mock('./diff-editor-word-wrap-options', () => ({ + buildDiffEditorWordWrapOptions: () => ({}), + syncDiffEditorOriginalWordWrap: syncWordWrap +})) +vi.mock('./diff-editor-shift-wheel-scroll', () => ({ + installDiffEditorShiftWheelScroll: () => cleanupShiftWheel +})) +vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) + +const props: ComponentProps = { + section: { + key: 'README.md', + path: 'README.md', + status: 'M', + originalContent: 'original', + modifiedContent: 'modified', + collapsed: false, + loading: false, + dirty: false, + diffResult: null, + largeDiffRenderLimit: null + }, + index: 0, + sectionBodyHeight: 300, + useIntrinsicImageHeight: false, + isBranchMode: false, + sideBySide: true, + isDark: false, + language: 'markdown', + modelPathBase: 'wrap-lifecycle', + isEditable: false, + diffEditorFontSize: 13, + diffWordWrap: true, + onRetrySection: vi.fn(), + onLoadDeferredSection: vi.fn(), + onSaveLimitedDiff: vi.fn(), + onMount: vi.fn() +} +const frames = new Map() + +beforeEach(() => { + mountedEditors.length = 0 + syncWordWrap.mockClear() + cleanupShiftWheel.mockClear() + frames.clear() + let nextFrame = 1 + vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => { + const id = nextFrame++ + frames.set(id, callback) + return id + }) + vi.stubGlobal('cancelAnimationFrame', (id: number) => frames.delete(id)) +}) +afterEach(() => { + cleanup() + vi.unstubAllGlobals() +}) + +async function finishMount(): Promise { + await act(async () => { + await Promise.resolve() + }) +} +function runMountFrame(): void { + const queued = [...frames.values()] + frames.clear() + act(() => queued.forEach((callback) => callback(0))) +} + +describe('combined diff word-wrap lifecycle', () => { + it('cancels the pending mount frame when the section returns to loading', async () => { + const view = render() + await finishMount() + expect(frames.size).toBe(1) + + view.rerender() + + expect(frames.size).toBe(0) + expect(syncWordWrap).not.toHaveBeenCalled() + expect(cleanupShiftWheel).toHaveBeenCalledOnce() + }) + + it('disposes synchronization and ignores preference changes while the editor is absent', async () => { + const view = render() + await finishMount() + runMountFrame() + const subscription = syncWordWrap.mock.results[0]?.value + expect(subscription).toBeDefined() + + view.rerender() + expect(subscription?.dispose).toHaveBeenCalledOnce() + + view.rerender( + + ) + expect(syncWordWrap).toHaveBeenCalledTimes(1) + }) + + it('uses the current wrap preference when a loading section remounts its editor', async () => { + const view = render() + await finishMount() + runMountFrame() + const oldEditor = mountedEditors[0] + + view.rerender( + + ) + view.rerender() + await finishMount() + expect(frames.size).toBe(1) + act(() => oldEditor?.disposeModified()) + expect(frames.size).toBe(1) + runMountFrame() + + expect(syncWordWrap).toHaveBeenLastCalledWith(mountedEditors[1]?.editor, false) + }) +}) diff --git a/src/renderer/src/components/editor/DiffViewer.tsx b/src/renderer/src/components/editor/DiffViewer.tsx index a2697069df0..cf262bfb3bb 100644 --- a/src/renderer/src/components/editor/DiffViewer.tsx +++ b/src/renderer/src/components/editor/DiffViewer.tsx @@ -21,7 +21,10 @@ import { useDiffViewerFirstChangeAutoScroll } from './useDiffViewerFirstChangeAu import { getDiffViewerLargeDiffSaveAction } from './diff-viewer-large-diff-save-action' import type { DiffViewerProps } from './diff-viewer-props' import { buildDiffEditorWhitespaceOptions } from './diff-editor-whitespace-options' -import { buildDiffEditorWordWrapOptions } from './diff-editor-word-wrap-options' +import { + buildDiffEditorWordWrapOptions, + syncDiffEditorOriginalWordWrap +} from './diff-editor-word-wrap-options' import { buildDiffEditorHideUnchangedOptions } from './diff-editor-hide-unchanged-options' import { useDiffEditorRegistration } from './diff-navigation-context' import { preserveDiffViewStateAcrossModelSwaps } from './diff-model-swap-view-state' @@ -67,10 +70,17 @@ export default function DiffViewer({ ) const terminalFontSize = settings?.terminalFontSize ?? 13, diffEditorFontSize = computeDiffEditorFontSize(terminalFontSize, editorFontZoomLevel) + const diffWordWrap = settings?.diffWordWrap const diffEditorRef = useRef(null) const { registerDiffEditor, unregisterDiffEditor } = useDiffEditorRegistration() const lineNumberOptionsSubRef = useRef<{ dispose: () => void } | null>(null) + const wordWrapOptionsSubRef = useRef<{ dispose: () => void } | null>(null) + const wordWrapMountFrameRef = useRef(0) + const diffWordWrapRef = useRef(diffWordWrap) + useLayoutEffect(() => { + diffWordWrapRef.current = diffWordWrap + }, [diffWordWrap]) const [modifiedEditor, setModifiedEditor] = useState(null) const renderLimit = useMemo( @@ -161,6 +171,9 @@ export default function DiffViewer({ // Why: on fallback transition, drop stale Monaco refs so decorators/save handlers don't talk to disposed UI. lineNumberOptionsSubRef.current?.dispose() lineNumberOptionsSubRef.current = null + cancelAnimationFrame(wordWrapMountFrameRef.current) + wordWrapOptionsSubRef.current?.dispose() + wordWrapOptionsSubRef.current = null // Why: capture before nulling so we unregister the exact instance (identity guard no-ops a stale dispose). const fallenBackEditor = diffEditorRef.current diffEditorRef.current = null @@ -193,11 +206,35 @@ export default function DiffViewer({ (diffEditor, monaco) => { diffEditorRef.current = diffEditor registerDiffEditor(diffEditor) + // Why: Monaco applies the inline-layout wrap override after mount, once width is known. + wordWrapMountFrameRef.current = requestAnimationFrame(() => { + if (diffEditorRef.current !== diffEditor) { + return + } + wordWrapOptionsSubRef.current?.dispose() + wordWrapOptionsSubRef.current = syncDiffEditorOriginalWordWrap( + diffEditor, + diffWordWrapRef.current + ) + }) lineNumberOptionsSubRef.current?.dispose() lineNumberOptionsSubRef.current = applyDiffEditorLineNumberOptions(diffEditor, sideBySide) const originalEditor = diffEditor.getOriginalEditor() const modifiedEditor = diffEditor.getModifiedEditor() + modifiedEditor.onDidDispose(() => { + if (diffEditorRef.current !== diffEditor) { + return + } + cancelAnimationFrame(wordWrapMountFrameRef.current) + lineNumberOptionsSubRef.current?.dispose() + lineNumberOptionsSubRef.current = null + wordWrapOptionsSubRef.current?.dispose() + wordWrapOptionsSubRef.current = null + unregisterDiffEditor(diffEditor) + diffEditorRef.current = null + setModifiedEditor(null) + }) diffEditor.onDidDispose(preserveDiffViewStateAcrossModelSwaps(diffEditor).dispose) setupCopy(originalEditor, monaco, filePath, propsRef) @@ -237,14 +274,6 @@ export default function DiffViewer({ } else { diffEditor.focus() } - - diffEditor.onDidDispose(() => { - lineNumberOptionsSubRef.current?.dispose() - lineNumberOptionsSubRef.current = null - diffEditorRef.current = null - unregisterDiffEditor(diffEditor) - setModifiedEditor(null) - }) }, [editable, setupCopy, modelKey, filePath, sideBySide, registerDiffEditor, unregisterDiffEditor] ) @@ -263,17 +292,25 @@ export default function DiffViewer({ }, [modelKey]) useEffect(() => { + cancelAnimationFrame(wordWrapMountFrameRef.current) const diffEditor = diffEditorRef.current if (!diffEditor) { - return + return () => { + cancelAnimationFrame(wordWrapMountFrameRef.current) + } } lineNumberOptionsSubRef.current?.dispose() lineNumberOptionsSubRef.current = applyDiffEditorLineNumberOptions(diffEditor, sideBySide) + wordWrapOptionsSubRef.current?.dispose() + wordWrapOptionsSubRef.current = syncDiffEditorOriginalWordWrap(diffEditor, diffWordWrap) return () => { + cancelAnimationFrame(wordWrapMountFrameRef.current) lineNumberOptionsSubRef.current?.dispose() lineNumberOptionsSubRef.current = null + wordWrapOptionsSubRef.current?.dispose() + wordWrapOptionsSubRef.current = null } - }, [sideBySide]) + }, [diffWordWrap, sideBySide]) return (
diff --git a/src/renderer/src/components/editor/DiffViewer.word-wrap-lifecycle.test.tsx b/src/renderer/src/components/editor/DiffViewer.word-wrap-lifecycle.test.tsx new file mode 100644 index 00000000000..c95dfebca4b --- /dev/null +++ b/src/renderer/src/components/editor/DiffViewer.word-wrap-lifecycle.test.tsx @@ -0,0 +1,154 @@ +// @vitest-environment happy-dom +import { act, cleanup, render } from '@testing-library/react' +import { useEffect, useRef } from 'react' +import { afterEach, describe, expect, it, vi } from 'vitest' +import DiffViewer from './DiffViewer' +import { DiffNavigationProvider, useDiffNavigation } from './diff-navigation-context' +import { getLargeDiffRenderLimitFromCounts } from './large-diff-render-limit' + +const fixture = vi.hoisted(() => { + function createEditor() { + const disposals = new Set<() => void>() + const modifiedEditor = { + onDidDispose: (callback: () => void) => { + disposals.add(callback) + return { dispose: () => disposals.delete(callback) } + } + } + const disposeUpdate = vi.fn() + const editor = { + getOriginalEditor: () => ({}), + getModifiedEditor: () => modifiedEditor, + onDidDispose: vi.fn(() => ({ dispose: vi.fn() })), + onDidUpdateDiff: () => ({ dispose: disposeUpdate }), + getLineChanges: () => [{}], + goToDiff: vi.fn(), + saveViewState: () => null, + focus: vi.fn() + } + return { editor, disposeUpdate, dispose: () => disposals.forEach((callback) => callback()) } + } + const editors: ReturnType[] = [] + const state = { + settings: { diffWordWrap: true }, + editorFontZoomLevel: 0, + addDiffComment: vi.fn(), + deleteDiffComment: vi.fn(), + updateDiffComment: vi.fn(), + scrollToDiffCommentId: null, + setScrollToDiffCommentId: vi.fn() + } + return { createEditor, editors, state } +}) + +vi.mock('@monaco-editor/react', () => ({ + DiffEditor: ({ + onMount + }: { + onMount: (editor: ReturnType['editor']) => void + }) => { + const mount = useRef(onMount) + useEffect(() => { + const instance = fixture.createEditor() + fixture.editors.push(instance) + let disposed = false + queueMicrotask(() => { + if (!disposed) { + mount.current(instance.editor) + } + }) + return () => { + disposed = true + instance.dispose() + } + }, []) + return
+ } +})) +vi.mock('@/store', () => ({ + useAppStore: (selector: (state: typeof fixture.state) => T) => selector(fixture.state) +})) +vi.mock('@/store/worktree-diff-comments-selector', () => ({ + selectWorktreeDiffComments: () => undefined +})) +vi.mock('@/lib/monaco-setup', () => ({ + monaco: { Uri: { parse: (path: string) => path }, editor: { getModel: () => null } } +})) +vi.mock('./useContextualCopySetup', () => ({ + useContextualCopySetup: () => ({ setupCopy: vi.fn(), toastNode: null }) +})) +vi.mock('../diff-comments/useDiffCommentDecorator', () => ({ useDiffCommentDecorator: vi.fn() })) +vi.mock('./useDiffViewerFirstChangeAutoScroll', () => ({ + useDiffViewerFirstChangeAutoScroll: vi.fn() +})) +vi.mock('@/hooks/use-document-dark-theme', () => ({ useDocumentDarkTheme: () => false })) +vi.mock('./diff-editor-line-number-options', () => ({ + applyDiffEditorLineNumberOptions: () => ({ dispose: vi.fn() }) +})) +vi.mock('./diff-editor-word-wrap-options', () => ({ + buildDiffEditorWordWrapOptions: () => ({}), + syncDiffEditorOriginalWordWrap: () => ({ dispose: vi.fn() }) +})) +vi.mock('./diff-model-swap-view-state', () => ({ + preserveDiffViewStateAcrossModelSwaps: () => ({ dispose: vi.fn() }) +})) +vi.mock('./editor-shortcuts', () => ({ installMonacoDiffChangeNavigationShortcut: () => vi.fn() })) +vi.mock('./LargeDiffFallback', () => ({ LargeDiffFallback: () =>
Large diff
})) + +function NavigationProbe(): React.JSX.Element { + const navigation = useDiffNavigation() + return ( + + ) +} + +function Surface({ limited }: { limited: boolean }): React.JSX.Element { + return ( + + + + + ) +} + +afterEach(() => { + cleanup() + fixture.editors.length = 0 +}) + +describe('file diff word-wrap lifecycle', () => { + it('unregisters navigation when inner disposal precedes the large-diff fallback effect', async () => { + const view = render() + await act(async () => { + await Promise.resolve() + }) + const mounted = fixture.editors[0] + const next = view.getByRole('button', { name: 'Next change (1)' }) + expect(next.hasAttribute('disabled')).toBe(false) + act(() => next.click()) + expect(mounted?.editor.goToDiff).toHaveBeenCalledWith('next') + + view.rerender() + + expect(view.getByRole('button', { name: 'Next change (0)' }).hasAttribute('disabled')).toBe( + true + ) + expect(mounted?.disposeUpdate).toHaveBeenCalledOnce() + }) +}) diff --git a/src/renderer/src/components/editor/diff-editor-word-wrap-options.test.ts b/src/renderer/src/components/editor/diff-editor-word-wrap-options.test.ts index 9d542cae07b..6230059e86e 100644 --- a/src/renderer/src/components/editor/diff-editor-word-wrap-options.test.ts +++ b/src/renderer/src/components/editor/diff-editor-word-wrap-options.test.ts @@ -1,13 +1,166 @@ -import { describe, expect, it } from 'vitest' -import { buildDiffEditorWordWrapOptions } from './diff-editor-word-wrap-options' +// @vitest-environment happy-dom +import { describe, expect, it, vi } from 'vitest' +import type { editor } from 'monaco-editor' +import { + buildDiffEditorWordWrapOptions, + syncDiffEditorOriginalWordWrap +} from './diff-editor-word-wrap-options' describe('buildDiffEditorWordWrapOptions', () => { it('keeps long diff lines unwrapped by default', () => { - expect(buildDiffEditorWordWrapOptions(undefined)).toEqual({ wordWrap: 'off' }) - expect(buildDiffEditorWordWrapOptions(false)).toEqual({ wordWrap: 'off' }) + expect(buildDiffEditorWordWrapOptions(undefined)).toEqual({ + wordWrap: 'off', + diffWordWrap: 'off' + }) + expect(buildDiffEditorWordWrapOptions(false)).toEqual({ + wordWrap: 'off', + diffWordWrap: 'off' + }) }) - it('enables Monaco diff word wrapping when the diff preference is on', () => { - expect(buildDiffEditorWordWrapOptions(true)).toEqual({ wordWrap: 'on' }) + it('enables Monaco diff word wrapping on both panes when the diff preference is on', () => { + expect(buildDiffEditorWordWrapOptions(true)).toEqual({ + wordWrap: 'on', + diffWordWrap: 'on' + }) + }) +}) + +describe('syncDiffEditorOriginalWordWrap', () => { + function fakeEditor() { + const listeners = new Set<() => void>() + let options: editor.IEditorOptions = {} + const editorStub = { + updateOptions: vi.fn((next: editor.IEditorOptions) => { + options = { ...options, ...next } + }), + getRawOptions: () => options, + onDidChangeConfiguration: (listener: () => void) => { + listeners.add(listener) + return { + dispose: () => { + listeners.delete(listener) + } + } + }, + emitDidChangeConfiguration: () => { + listeners.forEach((listener) => listener()) + } + } + return editorStub + } + + function monacoDiffHost(sideBySide: boolean): HTMLElement { + const host = document.createElement('div') + const widget = document.createElement('div') + widget.classList.add('monaco-diff-editor') + widget.classList.toggle('side-by-side', sideBySide) + host.append(widget) + return host + } + + function fakeDiffEditor(root = monacoDiffHost(true)): { + diffEditor: Parameters[0] + original: ReturnType + modified: ReturnType + } { + const original = fakeEditor() + const modified = fakeEditor() + return { + original, + modified, + diffEditor: { + getOriginalEditor: () => original, + getModifiedEditor: () => modified, + getContainerDomNode: () => root + } + } + } + + it('clears the original pane override that stays off after Monaco leaves inline layout', () => { + const { diffEditor, original, modified } = fakeDiffEditor() + + syncDiffEditorOriginalWordWrap(diffEditor, true) + + expect(original.updateOptions).toHaveBeenCalledWith({ + wordWrap: 'on', + wordWrapOverride2: 'inherit' + }) + expect(modified.updateOptions).toHaveBeenCalledWith({ wordWrap: 'on' }) + }) + + it('keeps both panes unwrapped when the preference is off', () => { + const { diffEditor, original, modified } = fakeDiffEditor() + + syncDiffEditorOriginalWordWrap(diffEditor, false) + + expect(original.updateOptions).toHaveBeenCalledWith({ + wordWrap: 'off', + wordWrapOverride2: 'off' + }) + expect(modified.updateOptions).toHaveBeenCalledWith({ wordWrap: 'off' }) + }) + + it('reapplies the original pane wrap after Monaco clears it, and stops after dispose', async () => { + const root = monacoDiffHost(true) + expect(root.classList.contains('side-by-side')).toBe(false) + const { diffEditor, original } = fakeDiffEditor(root) + const disposable = syncDiffEditorOriginalWordWrap(diffEditor, true) + original.updateOptions.mockClear() + + original.updateOptions({ wordWrapOverride2: 'off' }) + original.emitDidChangeConfiguration() + await Promise.resolve() + + expect(original.getRawOptions().wordWrapOverride2).toBe('inherit') + + original.updateOptions.mockClear() + disposable.dispose() + original.updateOptions({ wordWrapOverride2: 'off' }) + original.emitDidChangeConfiguration() + await Promise.resolve() + + expect(original.getRawOptions().wordWrapOverride2).toBe('off') + expect(original.updateOptions).toHaveBeenCalledTimes(1) + }) + + it('leaves the hidden original pane unwrapped while Monaco is inline', async () => { + const root = monacoDiffHost(false) + const { diffEditor, original } = fakeDiffEditor(root) + + syncDiffEditorOriginalWordWrap(diffEditor, true) + + expect(original.getRawOptions().wordWrapOverride2).toBe('off') + + original.updateOptions({ wordWrapOverride2: 'inherit' }) + original.emitDidChangeConfiguration() + await Promise.resolve() + + expect(original.getRawOptions().wordWrapOverride2).toBe('off') + }) + + it('observes the settled layout after an inline-to-side-by-side transition', async () => { + const host = monacoDiffHost(false) + const { diffEditor, original, modified } = fakeDiffEditor(host) + const disposable = syncDiffEditorOriginalWordWrap(diffEditor, true) + + original.emitDidChangeConfiguration() + modified.emitDidChangeConfiguration() + host.querySelector('.monaco-diff-editor')?.classList.add('side-by-side') + await Promise.resolve() + + expect(original.getRawOptions().wordWrapOverride2).toBe('inherit') + disposable.dispose() + }) + + it('cancels a queued update when the editor is disposed', async () => { + const { diffEditor, original } = fakeDiffEditor() + const disposable = syncDiffEditorOriginalWordWrap(diffEditor, true) + original.updateOptions({ wordWrapOverride2: 'off' }) + original.emitDidChangeConfiguration() + disposable.dispose() + await Promise.resolve() + + expect(original.getRawOptions().wordWrapOverride2).toBe('off') }) }) diff --git a/src/renderer/src/components/editor/diff-editor-word-wrap-options.ts b/src/renderer/src/components/editor/diff-editor-word-wrap-options.ts index c7f1e2cbea2..11369751171 100644 --- a/src/renderer/src/components/editor/diff-editor-word-wrap-options.ts +++ b/src/renderer/src/components/editor/diff-editor-word-wrap-options.ts @@ -1,9 +1,96 @@ import type { editor } from 'monaco-editor' +export function diffEditorWordWrapMode(diffWordWrap: boolean | undefined): 'on' | 'off' { + return diffWordWrap === true ? 'on' : 'off' +} + export function buildDiffEditorWordWrapOptions( diffWordWrap: boolean | undefined -): Pick { +): Pick { + const wrap = diffEditorWordWrapMode(diffWordWrap) return { - wordWrap: diffWordWrap === true ? 'on' : 'off' + wordWrap: wrap, + // Why: `wordWrap` alone reaches the modified pane; the original pane follows `diffWordWrap`. + diffWordWrap: wrap + } +} + +type Disposable = { dispose: () => void } + +type WordWrapEditor = Pick & { + onDidChangeConfiguration: (listener: () => void) => Disposable +} + +type WordWrapDiffEditor = Pick & { + getOriginalEditor: () => WordWrapEditor + getModifiedEditor: () => WordWrapEditor +} + +function diffEditorIsSideBySide(diffEditor: WordWrapDiffEditor): boolean { + const host = diffEditor.getContainerDomNode?.() + if (!host) { + return true + } + // Why: createDiffEditor's container never receives the class. Monaco appends + // `div.monaco-diff-editor` and toggles `side-by-side` on that child. + const widget = host.classList.contains('monaco-diff-editor') + ? host + : host.querySelector?.('.monaco-diff-editor') + if (!widget) { + return true + } + return widget.classList.contains('side-by-side') +} + +export function syncDiffEditorOriginalWordWrap( + diffEditor: WordWrapDiffEditor, + diffWordWrap: boolean | undefined +): Disposable { + const originalEditor = diffEditor.getOriginalEditor() + const modifiedEditor = diffEditor.getModifiedEditor() + let disposed = false + let scheduled = false + + const apply = (): void => { + if (disposed) { + return + } + const wrap = diffEditorWordWrapMode(diffWordWrap) + // Why: inline layout hides the original editor and sets wordWrapOverride2 to off. + // Side-by-side only writes wordWrapOverride1, so the off value sticks after a widen (#24199). + const showOriginal = diffEditorIsSideBySide(diffEditor) + const override = wrap === 'on' && showOriginal ? 'inherit' : 'off' + const originalOptions = originalEditor.getRawOptions() + if (originalOptions.wordWrap !== wrap || originalOptions.wordWrapOverride2 !== override) { + originalEditor.updateOptions({ wordWrap: wrap, wordWrapOverride2: override }) + } + if (modifiedEditor.getRawOptions().wordWrap !== wrap) { + modifiedEditor.updateOptions({ wordWrap: wrap }) + } + } + + // Why: Monaco updates the inner editor and the side-by-side class in the same turn. + // Applying on the next microtask sees the class after that turn settles. + const schedule = (): void => { + if (disposed || scheduled) { + return + } + scheduled = true + queueMicrotask(() => { + scheduled = false + apply() + }) + } + + apply() + const originalSub = originalEditor.onDidChangeConfiguration(schedule) + const modifiedSub = modifiedEditor.onDidChangeConfiguration(schedule) + + return { + dispose: () => { + disposed = true + originalSub.dispose() + modifiedSub.dispose() + } } } diff --git a/tests/e2e/diff-word-wrap.spec.ts b/tests/e2e/diff-word-wrap.spec.ts new file mode 100644 index 00000000000..2b2941605df --- /dev/null +++ b/tests/e2e/diff-word-wrap.spec.ts @@ -0,0 +1,117 @@ +import { execFileSync } from 'node:child_process' +import { writeFileSync } from 'node:fs' +import path from 'node:path' +import type { Page } from '@stablyai/playwright-test' +import { test, expect } from './helpers/orca-app' +import { + cleanupGoldenWorktree, + createGoldenWorktree, + openGoldenSourceControl +} from './helpers/golden-source-control' +import { waitForSessionReady } from './helpers/store' + +type DiffSurface = 'file' | 'combined' + +async function toggleWordWrap(page: Page, surface: DiffSurface): Promise { + if (surface === 'combined') { + await page.getByRole('button', { name: /^Wrap (On|Off)$/ }).click() + return + } + await page.getByRole('button', { name: 'More actions', exact: true }).click() + await page.getByRole('menuitemcheckbox', { name: 'Word Wrap', exact: true }).click() +} + +async function expectWrappedParagraphs(page: Page): Promise { + for (const side of ['original', 'modified']) { + const pane = page.locator(`.${side}-in-monaco-diff-editor`) + await expect + .poll(() => pane.locator('.view-line').count(), { + message: `${side} paragraphs should occupy multiple wrapped rows` + }) + .toBeGreaterThan(10) + await expect + .poll(() => + pane.evaluate((element) => { + const viewport = element.querySelector('.monaco-scrollable-element') + if (!viewport) { + throw new Error('Missing Monaco text viewport') + } + const textWidth = Math.max( + ...Array.from( + element.querySelectorAll('.view-line > span'), + (line) => line.getBoundingClientRect().width + ) + ) + return textWidth - viewport.getBoundingClientRect().width + }) + ) + .toBeLessThanOrEqual(1) + } +} + +for (const surface of ['file', 'combined'] as const) { + test(`${surface} diff word wrap applies to both panes after toggles and narrow inline layout`, async ({ + orcaPage, + testRepoPath, + registerPostElectronShutdownCleanup + }, testInfo) => { + const fixture = createGoldenWorktree(testRepoPath, `diff-word-wrap-${surface}`) + registerPostElectronShutdownCleanup(async () => cleanupGoldenWorktree(testRepoPath, fixture)) + const paragraph = + 'This long Markdown paragraph compares the original and modified panes at their own widths. '.repeat( + 9 + ) + const readmePath = path.join(fixture.worktreePath, 'README.md') + const content = `# Diff word wrap\n\n${paragraph}Original paragraph end.\n\n${paragraph}Second original paragraph end.\n` + writeFileSync(readmePath, content) + execFileSync('git', ['add', 'README.md'], { cwd: fixture.worktreePath, stdio: 'pipe' }) + execFileSync('git', ['commit', '-m', 'Seed long Markdown paragraphs'], { + cwd: fixture.worktreePath, + stdio: 'pipe' + }) + writeFileSync( + readmePath, + content.replaceAll('original', 'modified').replaceAll('Original', 'Modified') + ) + + await orcaPage.setViewportSize({ width: 1600, height: 850 }) + await waitForSessionReady(orcaPage) + await orcaPage.evaluate(async () => { + await window.__store?.getState().updateSettings({ + diffDefaultView: 'side-by-side', + diffWordWrap: false + }) + }) + await openGoldenSourceControl(orcaPage, testRepoPath, fixture) + const changes = orcaPage.getByRole('button', { name: /^Changes \d+$/ }).locator('..') + await ( + surface === 'combined' + ? changes.getByRole('button', { name: 'View all', exact: true }) + : changes + .locator('../..') + .locator('[data-testid="source-control-entry"]') + .filter({ hasText: 'README.md' }) + ).click() + await orcaPage.evaluate(() => window.__store?.getState().setRightSidebarOpen(false)) + const diff = orcaPage.locator('.monaco-diff-editor') + await expect(diff).toHaveClass(/side-by-side/) + await toggleWordWrap(orcaPage, surface) + await orcaPage.screenshot({ path: testInfo.outputPath('wrap-on.png') }) + await expectWrappedParagraphs(orcaPage) + + await orcaPage.setViewportSize({ width: 1000, height: 850 }) + await expect(diff).not.toHaveClass(/side-by-side/) + await orcaPage.setViewportSize({ width: 1600, height: 850 }) + await expect(diff).toHaveClass(/side-by-side/) + await expectWrappedParagraphs(orcaPage) + + await toggleWordWrap(orcaPage, surface) + for (const side of ['original', 'modified']) { + await expect + .poll(() => orcaPage.locator(`.${side}-in-monaco-diff-editor .view-line`).count()) + .toBeLessThanOrEqual(6) + } + await toggleWordWrap(orcaPage, surface) + await expectWrappedParagraphs(orcaPage) + }) +}