mirror of
https://github.com/Studio-Saelix/sencho.git
synced 2026-09-01 05:07:59 +00:00
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.
This commit is contained in:
@@ -1,5 +1,5 @@
|
|||||||
import { Suspense } from 'react';
|
import { Suspense } from 'react';
|
||||||
import { DiffEditor } from '@/lib/monacoLoader';
|
import { SafeDiffEditor } from '@/lib/SafeDiffEditor';
|
||||||
import { Modal, ModalHeader, ModalFooter } from '@/components/ui/modal';
|
import { Modal, ModalHeader, ModalFooter } from '@/components/ui/modal';
|
||||||
import { Button } from '@/components/ui/button';
|
import { Button } from '@/components/ui/button';
|
||||||
import { BusyButton } from '@/components/ui/busy-button';
|
import { BusyButton } from '@/components/ui/busy-button';
|
||||||
@@ -43,7 +43,7 @@ export function ComposeDiffPreviewDialog({
|
|||||||
<div className="px-6 pb-4 pt-3">
|
<div className="px-6 pb-4 pt-3">
|
||||||
<div className="h-[55vh] border border-glass-border rounded-md overflow-hidden">
|
<div className="h-[55vh] border border-glass-border rounded-md overflow-hidden">
|
||||||
<Suspense fallback={<div className="w-full h-full" aria-busy="true" />}>
|
<Suspense fallback={<div className="w-full h-full" aria-busy="true" />}>
|
||||||
<DiffEditor
|
<SafeDiffEditor
|
||||||
height="100%"
|
height="100%"
|
||||||
language={language}
|
language={language}
|
||||||
theme={isDarkMode ? 'vs-dark' : 'vs'}
|
theme={isDarkMode ? 'vs-dark' : 'vs'}
|
||||||
|
|||||||
@@ -3,8 +3,8 @@ import { render, screen } from '@testing-library/react';
|
|||||||
import { ComposeDiffPreviewDialog } from '../ComposeDiffPreviewDialog';
|
import { ComposeDiffPreviewDialog } from '../ComposeDiffPreviewDialog';
|
||||||
import { resolveComposeDiffActionLabel } from '../resolveComposeDiffActionLabel';
|
import { resolveComposeDiffActionLabel } from '../resolveComposeDiffActionLabel';
|
||||||
|
|
||||||
vi.mock('@/lib/monacoLoader', () => ({
|
vi.mock('@/lib/SafeDiffEditor', () => ({
|
||||||
DiffEditor: () => <div data-testid="diff-editor" />,
|
SafeDiffEditor: () => <div data-testid="diff-editor" />,
|
||||||
}));
|
}));
|
||||||
|
|
||||||
describe('resolveComposeDiffActionLabel', () => {
|
describe('resolveComposeDiffActionLabel', () => {
|
||||||
@@ -45,4 +45,24 @@ describe('ComposeDiffPreviewDialog', () => {
|
|||||||
);
|
);
|
||||||
expect(screen.getByRole('button', { name: 'Save & reapply' })).toBeInTheDocument();
|
expect(screen.getByRole('button', { name: 'Save & reapply' })).toBeInTheDocument();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('unmounts the diff editor without throwing', () => {
|
||||||
|
const { unmount } = render(
|
||||||
|
<ComposeDiffPreviewDialog
|
||||||
|
open
|
||||||
|
onOpenChange={vi.fn()}
|
||||||
|
stackName="sencho"
|
||||||
|
fileName="docker-compose.yml"
|
||||||
|
language="yaml"
|
||||||
|
original="a"
|
||||||
|
modified="b"
|
||||||
|
actionLabel="Save"
|
||||||
|
confirming={false}
|
||||||
|
isDarkMode={false}
|
||||||
|
onConfirm={vi.fn()}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
expect(screen.getByTestId('diff-editor')).toBeInTheDocument();
|
||||||
|
expect(() => unmount()).not.toThrow();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
import { useState, Suspense } from 'react';
|
import { useState, Suspense } from 'react';
|
||||||
import { DiffEditor } from '@/lib/monacoLoader';
|
import { SafeDiffEditor } from '@/lib/SafeDiffEditor';
|
||||||
import { AlertTriangle, Loader2 } from 'lucide-react';
|
import { AlertTriangle, Loader2 } from 'lucide-react';
|
||||||
import { Modal, ModalHeader, ModalFooter, ConfirmModal } from '@/components/ui/modal';
|
import { Modal, ModalHeader, ModalFooter, ConfirmModal } from '@/components/ui/modal';
|
||||||
import { Tabs, TabsList, TabsTrigger, TabsHighlight, TabsHighlightItem } from '@/components/ui/tabs';
|
import { Tabs, TabsList, TabsTrigger, TabsHighlight, TabsHighlightItem } from '@/components/ui/tabs';
|
||||||
@@ -120,7 +120,7 @@ export function GitSourceDiffDialog({
|
|||||||
<div className="px-6 pb-4 pt-3">
|
<div className="px-6 pb-4 pt-3">
|
||||||
<div className="h-[55vh] border border-glass-border rounded-md overflow-hidden">
|
<div className="h-[55vh] border border-glass-border rounded-md overflow-hidden">
|
||||||
<Suspense fallback={<div className="w-full h-full" aria-busy="true" />}>
|
<Suspense fallback={<div className="w-full h-full" aria-busy="true" />}>
|
||||||
<DiffEditor
|
<SafeDiffEditor
|
||||||
height="100%"
|
height="100%"
|
||||||
language={effectiveTab === 'compose' ? 'yaml' : 'ini'}
|
language={effectiveTab === 'compose' ? 'yaml' : 'ini'}
|
||||||
theme={isDarkMode ? 'vs-dark' : 'vs'}
|
theme={isDarkMode ? 'vs-dark' : 'vs'}
|
||||||
|
|||||||
@@ -0,0 +1,49 @@
|
|||||||
|
import { useEffect, useRef } from 'react';
|
||||||
|
import type { DiffEditorProps } from '@monaco-editor/react';
|
||||||
|
import type { editor } from 'monaco-editor';
|
||||||
|
import { DiffEditor } from './monacoLoader';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* DiffEditor wrapper that owns model disposal. keepCurrentOriginalModel and
|
||||||
|
* keepCurrentModifiedModel stop @monaco-editor/react from disposing first.
|
||||||
|
* On unmount this resets the widget (setModel(null)) then disposes the models.
|
||||||
|
* Monaco 0.56 throws "TextModel got disposed before DiffEditorWidget model
|
||||||
|
* got reset" if the library disposes models while the widget still holds them.
|
||||||
|
*/
|
||||||
|
export function SafeDiffEditor({ onMount, ...props }: DiffEditorProps) {
|
||||||
|
const editorRef = useRef<editor.IStandaloneDiffEditor | null>(null);
|
||||||
|
const modelsRef = useRef<editor.IDiffEditorModel | null>(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 (
|
||||||
|
<DiffEditor
|
||||||
|
{...props}
|
||||||
|
keepCurrentOriginalModel
|
||||||
|
keepCurrentModifiedModel
|
||||||
|
onMount={(ed, monaco) => {
|
||||||
|
editorRef.current = ed;
|
||||||
|
modelsRef.current = ed.getModel();
|
||||||
|
onMount?.(ed, monaco);
|
||||||
|
}}
|
||||||
|
/>
|
||||||
|
);
|
||||||
|
}
|
||||||
@@ -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 <div data-testid="diff-editor" />;
|
||||||
|
},
|
||||||
|
}));
|
||||||
|
|
||||||
|
import { SafeDiffEditor } from '../SafeDiffEditor';
|
||||||
|
|
||||||
|
describe('SafeDiffEditor', () => {
|
||||||
|
it('keeps current models and resets the widget before disposing them', () => {
|
||||||
|
disposeCalls = [];
|
||||||
|
const { unmount } = render(
|
||||||
|
<SafeDiffEditor
|
||||||
|
height="100%"
|
||||||
|
language="yaml"
|
||||||
|
original="a"
|
||||||
|
modified="b"
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
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'));
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user