From 93a5ba9f21f4900acabf37f39ec2efffff469d3e Mon Sep 17 00:00:00 2001 From: Anso Date: Thu, 13 Aug 2026 10:32:26 -0400 Subject: [PATCH] fix(editor): dispose compose DiffEditor models after widget reset (#1828) Closing the compose or git diff dialog threw a Monaco console error because the library disposed TextModels while DiffEditorWidget still held them. --- .../components/ComposeDiffPreviewDialog.tsx | 4 +- .../ComposeDiffPreviewDialog.test.tsx | 24 +++++++- .../components/stack/GitSourceDiffDialog.tsx | 4 +- frontend/src/lib/SafeDiffEditor.tsx | 49 ++++++++++++++++ .../src/lib/__tests__/SafeDiffEditor.test.tsx | 57 +++++++++++++++++++ 5 files changed, 132 insertions(+), 6 deletions(-) create mode 100644 frontend/src/lib/SafeDiffEditor.tsx create mode 100644 frontend/src/lib/__tests__/SafeDiffEditor.test.tsx diff --git a/frontend/src/components/ComposeDiffPreviewDialog.tsx b/frontend/src/components/ComposeDiffPreviewDialog.tsx index d6d788b6..18e36d6e 100644 --- a/frontend/src/components/ComposeDiffPreviewDialog.tsx +++ b/frontend/src/components/ComposeDiffPreviewDialog.tsx @@ -1,5 +1,5 @@ import { Suspense } from 'react'; -import { DiffEditor } from '@/lib/monacoLoader'; +import { SafeDiffEditor } from '@/lib/SafeDiffEditor'; import { Modal, ModalHeader, ModalFooter } from '@/components/ui/modal'; import { Button } from '@/components/ui/button'; import { BusyButton } from '@/components/ui/busy-button'; @@ -43,7 +43,7 @@ export function ComposeDiffPreviewDialog({
}> - ({ - DiffEditor: () =>
, +vi.mock('@/lib/SafeDiffEditor', () => ({ + SafeDiffEditor: () =>
, })); describe('resolveComposeDiffActionLabel', () => { @@ -45,4 +45,24 @@ describe('ComposeDiffPreviewDialog', () => { ); expect(screen.getByRole('button', { name: 'Save & reapply' })).toBeInTheDocument(); }); + + it('unmounts the diff editor without throwing', () => { + const { unmount } = render( + , + ); + expect(screen.getByTestId('diff-editor')).toBeInTheDocument(); + expect(() => unmount()).not.toThrow(); + }); }); diff --git a/frontend/src/components/stack/GitSourceDiffDialog.tsx b/frontend/src/components/stack/GitSourceDiffDialog.tsx index 8b5a9269..2f9d8551 100644 --- a/frontend/src/components/stack/GitSourceDiffDialog.tsx +++ b/frontend/src/components/stack/GitSourceDiffDialog.tsx @@ -1,5 +1,5 @@ import { useState, Suspense } from 'react'; -import { DiffEditor } from '@/lib/monacoLoader'; +import { SafeDiffEditor } from '@/lib/SafeDiffEditor'; import { AlertTriangle, Loader2 } from 'lucide-react'; import { Modal, ModalHeader, ModalFooter, ConfirmModal } from '@/components/ui/modal'; import { Tabs, TabsList, TabsTrigger, TabsHighlight, TabsHighlightItem } from '@/components/ui/tabs'; @@ -120,7 +120,7 @@ export function GitSourceDiffDialog({
}> - (null); + const modelsRef = useRef(null); + + useEffect(() => { + return () => { + const diffEditor = editorRef.current; + const models = modelsRef.current; + modelsRef.current = null; + try { + diffEditor?.setModel(null); + diffEditor?.dispose(); + } catch { + // Widget already torn down by monaco-react. + } + try { + models?.original.dispose(); + models?.modified.dispose(); + } catch { + // Models already disposed. + } + }; + }, []); + + return ( + { + editorRef.current = ed; + modelsRef.current = ed.getModel(); + onMount?.(ed, monaco); + }} + /> + ); +} diff --git a/frontend/src/lib/__tests__/SafeDiffEditor.test.tsx b/frontend/src/lib/__tests__/SafeDiffEditor.test.tsx new file mode 100644 index 00000000..5bce5466 --- /dev/null +++ b/frontend/src/lib/__tests__/SafeDiffEditor.test.tsx @@ -0,0 +1,57 @@ +import { describe, it, expect, vi } from 'vitest'; +import { render } from '@testing-library/react'; +import type { DiffEditorProps } from '@monaco-editor/react'; + +let lastDiffProps: DiffEditorProps | null = null; +let disposeCalls: string[] = []; + +vi.mock('@/lib/monacoLoader', () => ({ + DiffEditor: (props: DiffEditorProps) => { + lastDiffProps = props; + if (props.onMount) { + const original = { dispose: () => disposeCalls.push('original.dispose') }; + const modified = { dispose: () => disposeCalls.push('modified.dispose') }; + let attached: { original: typeof original; modified: typeof modified } | null = { + original, + modified, + }; + props.onMount({ + getModel: () => attached, + setModel: (next: typeof attached) => { + disposeCalls.push('setModel'); + attached = next; + }, + dispose: () => { + if (attached) { + throw new Error('TextModel got disposed before DiffEditorWidget model got reset'); + } + disposeCalls.push('editor.dispose'); + }, + } as never, {} as never); + } + return
; + }, +})); + +import { SafeDiffEditor } from '../SafeDiffEditor'; + +describe('SafeDiffEditor', () => { + it('keeps current models and resets the widget before disposing them', () => { + disposeCalls = []; + const { unmount } = render( + , + ); + expect(lastDiffProps?.keepCurrentOriginalModel).toBe(true); + expect(lastDiffProps?.keepCurrentModifiedModel).toBe(true); + unmount(); + expect(disposeCalls.indexOf('setModel')).toBeGreaterThanOrEqual(0); + expect(disposeCalls.indexOf('setModel')).toBeLessThan(disposeCalls.indexOf('original.dispose')); + expect(disposeCalls.indexOf('setModel')).toBeLessThan(disposeCalls.indexOf('modified.dispose')); + expect(disposeCalls.indexOf('editor.dispose')).toBeGreaterThan(disposeCalls.indexOf('setModel')); + }); +});