From d57e55472c6459111f4cfef147fe1e25bd2042f4 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Wed, 28 Oct 2026 17:43:51 -0700 Subject: [PATCH] perf(editor): skip full-text reads for ignored Monaco content events (#34920) Subscribe to onDidChangeModelContent directly instead of the @monaco-editor/react onChange prop, which calls editor.getValue() before Orca's ownership and programmatic-sync checks run. Programmatic echoes (external reloads, shared-record commits, read-only live tail) no longer materialize the full model text only to discard it. Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- .../src/components/editor/MonacoEditor.tsx | 2 +- .../editor-shared-record-clean-view.test.ts | 7 +- ...monaco-content-sync-bridge.events.test.tsx | 89 +++++++++++++++++++ .../editor/use-monaco-content-sync-bridge.ts | 62 +++++++------ 4 files changed, 132 insertions(+), 28 deletions(-) create mode 100644 src/renderer/src/components/editor/use-monaco-content-sync-bridge.events.test.tsx diff --git a/src/renderer/src/components/editor/MonacoEditor.tsx b/src/renderer/src/components/editor/MonacoEditor.tsx index 65e590354e3..f705eeafff8 100644 --- a/src/renderer/src/components/editor/MonacoEditor.tsx +++ b/src/renderer/src/components/editor/MonacoEditor.tsx @@ -137,6 +137,7 @@ export default function MonacoEditor({ useMonacoRevealScheduler() const contentSync = useMonacoContentSyncBridge({ editorRef, + mountedEditor, content, contentRef, contentSyncModeRef, @@ -280,7 +281,6 @@ export default function MonacoEditor({ // Use defaultValue because Orca owns later content sync; controlled values would duplicate setValue. defaultValue={content} theme={isDark ? 'vs-dark' : 'vs'} - onChange={contentSync.handleChange} onMount={handleMount} options={{ dropIntoEditor: { enabled: false }, diff --git a/src/renderer/src/components/editor/editor-shared-record-clean-view.test.ts b/src/renderer/src/components/editor/editor-shared-record-clean-view.test.ts index fcce3f21d4f..6e6f1f25164 100644 --- a/src/renderer/src/components/editor/editor-shared-record-clean-view.test.ts +++ b/src/renderer/src/components/editor/editor-shared-record-clean-view.test.ts @@ -81,9 +81,12 @@ function openSharedMarkdown() { .result.current const viewText = (id: string): string => store.getState().editorDrafts[id] ?? fileContents[id].content - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: content sync only calls getModel and pushUndoStop. + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the bridge uses the real model event and text through this editor facade. const editorA = { getModel: () => model, + getValue: () => model.getValue(), + onDidChangeModelContent: (listener: (event: editor.IModelContentChangedEvent) => void) => + model.onDidChangeContent(listener), pushUndoStop: () => true } as unknown as editor.IStandaloneCodeEditor const contentRef = { current: viewText(a.id) } @@ -91,6 +94,7 @@ function openSharedMarkdown() { ({ content }: { content: string }) => useMonacoContentSyncBridge({ editorRef: { current: editorA }, + mountedEditor: editorA, content, contentRef, contentSyncModeRef: { current: 'undoable' }, @@ -107,7 +111,6 @@ function openSharedMarkdown() { const typeInA = (text: string): void => { act(() => { model.setValue(text) - bridge.result.current.handleChange(text) }) renderA() } diff --git a/src/renderer/src/components/editor/use-monaco-content-sync-bridge.events.test.tsx b/src/renderer/src/components/editor/use-monaco-content-sync-bridge.events.test.tsx new file mode 100644 index 00000000000..2afffe3c3d0 --- /dev/null +++ b/src/renderer/src/components/editor/use-monaco-content-sync-bridge.events.test.tsx @@ -0,0 +1,89 @@ +// @vitest-environment happy-dom +import { act, cleanup, renderHook } from '@testing-library/react' +import * as monaco from 'monaco-editor/esm/vs/editor/editor.api.js' +import type { editor } from 'monaco-editor' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { useMonacoContentSyncBridge } from './use-monaco-content-sync-bridge' +import { + beginProgrammaticContentSync, + endProgrammaticContentSync +} from './monaco-programmatic-sync' + +vi.mock('./editor-stale-mount-guard', () => ({ mayPushTabTextIntoModel: () => true })) + +afterEach(() => { + cleanup() + for (const model of monaco.editor.getModels()) { + model.dispose() + } +}) + +function mountBridge(model: editor.ITextModel) { + const onContentChange = vi.fn() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the bridge uses this facade's real model event, text, and undo boundary. + const mountedEditor = { + getModel: () => model, + getValue: () => model.getValue(), + onDidChangeModelContent: (listener: (event: editor.IModelContentChangedEvent) => void) => + model.onDidChangeContent(listener), + pushUndoStop: () => { + model.pushStackElement() + return true + } + } as unknown as editor.IStandaloneCodeEditor + const editorRef: { current: editor.IStandaloneCodeEditor | null } = { current: mountedEditor } + const bridge = renderHook( + ({ content }) => + useMonacoContentSyncBridge({ + editorRef, + mountedEditor, + content, + contentRef: { current: content }, + contentSyncModeRef: { current: 'undoable' }, + modelKey: model.uri.toString(), + onContentChange + }), + { initialProps: { content: model.getValue() } } + ) + return { ...bridge, onContentChange, editorRef } +} + +describe('Monaco content event publication', () => { + it('does not publish programmatic echoes, while later user edits reach both panes', () => { + const model = monaco.editor.createModel('initial') + const first = mountBridge(model) + const sibling = mountBridge(model) + first.rerender({ content: 'external update' }) + expect(model.getValue()).toBe('external update') + + const key = model.uri.toString() + beginProgrammaticContentSync(key) + try { + act(() => model.setValue('another owner update')) + } finally { + endProgrammaticContentSync(key) + } + expect(first.onContentChange).not.toHaveBeenCalled() + expect(sibling.onContentChange).not.toHaveBeenCalled() + + act(() => model.setValue('user edit')) + expect(first.onContentChange).toHaveBeenCalledExactlyOnceWith('user edit') + expect(sibling.onContentChange).toHaveBeenCalledExactlyOnceWith('user edit') + }) + + it('does not publish from an editor that no longer owns the mounted ref', () => { + const model = monaco.editor.createModel('initial') + const bridge = mountBridge(model) + bridge.editorRef.current = null + act(() => model.setValue('retired editor')) + expect(bridge.onContentChange).not.toHaveBeenCalled() + }) + + it('does not publish after unmount', () => { + const model = monaco.editor.createModel('initial') + const bridge = mountBridge(model) + bridge.unmount() + act(() => model.setValue('after close')) + expect(bridge.onContentChange).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/editor/use-monaco-content-sync-bridge.ts b/src/renderer/src/components/editor/use-monaco-content-sync-bridge.ts index 6a20cd74ae1..b8332ba5c71 100644 --- a/src/renderer/src/components/editor/use-monaco-content-sync-bridge.ts +++ b/src/renderer/src/components/editor/use-monaco-content-sync-bridge.ts @@ -1,4 +1,4 @@ -import { useCallback, useLayoutEffect, useRef, type MutableRefObject } from 'react' +import { useLayoutEffect, useRef, type MutableRefObject } from 'react' import type { editor } from 'monaco-editor' import { syncContentUpdate, type MonacoContentSyncMode } from './monaco-content-sync' import { mayPushTabTextIntoModel } from './editor-stale-mount-guard' @@ -13,12 +13,12 @@ export type MonacoContentSyncBridge = { lastSyncedContentRef: MutableRefObject contentSyncModeRef: MutableRefObject isApplyingProgrammaticContentRef: MutableRefObject - handleChange: (value: string | undefined) => void } /** Callers update the content/mode refs during render so mount handlers can read current props before commit. */ export function useMonacoContentSyncBridge(params: { editorRef: MutableRefObject + mountedEditor: editor.IStandaloneCodeEditor | null content: string contentRef: MutableRefObject contentSyncModeRef: MutableRefObject @@ -27,34 +27,47 @@ export function useMonacoContentSyncBridge(params: { fileId?: string onContentChange: (content: string) => void }): MonacoContentSyncBridge { - const { editorRef, content, contentRef, contentSyncModeRef, modelKey, onContentChange, fileId } = - params + const { + editorRef, + mountedEditor, + content, + contentRef, + contentSyncModeRef, + modelKey, + onContentChange, + fileId + } = params const lastSyncedContentRef = useRef(content) // Suppress reconciliation’s real edit events so programmatic updates do not mark the file dirty. const isApplyingProgrammaticContentRef = useRef(false) - const handleChange = useCallback( - (value: string | undefined) => { - if (value !== undefined) { - if (editorRef.current?.getModel()?.uri.toString() !== modelKey) { - return - } - if ( - shouldIgnoreMonacoContentChange({ - modelKey, - isApplyingProgrammaticContent: isApplyingProgrammaticContentRef.current - }) - ) { - return - } - lastSyncedContentRef.current = value - onContentChange(value) + useLayoutEffect(() => { + if (!mountedEditor) { + return + } + const subscription = mountedEditor.onDidChangeModelContent(() => { + if ( + editorRef.current !== mountedEditor || + mountedEditor.getModel()?.uri.toString() !== modelKey + ) { + return } - }, - [editorRef, modelKey, onContentChange] - ) + if ( + shouldIgnoreMonacoContentChange({ + modelKey, + isApplyingProgrammaticContent: isApplyingProgrammaticContentRef.current + }) + ) { + return + } + const value = mountedEditor.getValue() + lastSyncedContentRef.current = value + onContentChange(value) + }) + return () => subscription.dispose() + }, [editorRef, modelKey, mountedEditor, onContentChange]) // Reconcile external prop changes before paint to avoid stale-text flashes; mount handles retained models separately. useLayoutEffect(() => { @@ -82,7 +95,6 @@ export function useMonacoContentSyncBridge(params: { contentRef, lastSyncedContentRef, contentSyncModeRef, - isApplyingProgrammaticContentRef, - handleChange + isApplyingProgrammaticContentRef } }