From a8a80873e24ec4c28ab9cd989735bf8a317a0c19 Mon Sep 17 00:00:00 2001 From: Tasso Evangelista Date: Fri, 12 Jun 2026 00:28:07 -0300 Subject: [PATCH] fix: CodeMirror component not unmounting properly (#40902) --- .changeset/bumpy-coats-reply.md | 5 + .../inputs/CodeMirror/CodeMirror.spec.tsx | 114 +++++++++++ .../Setting/inputs/CodeMirror/CodeMirror.tsx | 178 +++++++++++------- yarn.lock | 4 +- 4 files changed, 227 insertions(+), 74 deletions(-) create mode 100644 .changeset/bumpy-coats-reply.md create mode 100644 apps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirror.spec.tsx diff --git a/.changeset/bumpy-coats-reply.md b/.changeset/bumpy-coats-reply.md new file mode 100644 index 0000000000000..d261107317837 --- /dev/null +++ b/.changeset/bumpy-coats-reply.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Fixes a memory leakage on the CodeMirror component (used by `code`-typed settings) diff --git a/apps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirror.spec.tsx b/apps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirror.spec.tsx new file mode 100644 index 0000000000000..65687901a0d17 --- /dev/null +++ b/apps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirror.spec.tsx @@ -0,0 +1,114 @@ +import { act, render, waitFor } from '@testing-library/react'; + +import CodeMirror from './CodeMirror'; + +type ChangeHandler = (doc: { getValue: () => string }) => void; + +const editor = { + on: jest.fn(), + off: jest.fn(), + setOption: jest.fn(), + setValue: jest.fn(), + getValue: jest.fn(), + toTextArea: jest.fn(), +}; + +const fromTextArea = jest.fn(() => editor); + +jest.mock('codemirror', () => ({ + __esModule: true, + default: { fromTextArea: (...args: unknown[]) => fromTextArea(...(args as [])) }, +})); + +jest.mock('codemirror/addon/edit/matchbrackets', () => ({}), { virtual: true }); +jest.mock('codemirror/addon/edit/closebrackets', () => ({}), { virtual: true }); +jest.mock('codemirror/addon/edit/matchtags', () => ({}), { virtual: true }); +jest.mock('codemirror/addon/edit/trailingspace', () => ({}), { virtual: true }); +jest.mock('codemirror/addon/search/match-highlighter', () => ({}), { virtual: true }); +jest.mock('codemirror/lib/codemirror.css', () => ({}), { virtual: true }); +jest.mock('../../../../../../../app/ui/client/lib/codeMirror/codeMirror', () => ({}), { virtual: true }); + +const flushAsync = () => act(() => Promise.resolve()); + +beforeEach(() => { + editor.on.mockClear(); + editor.off.mockClear(); + editor.setOption.mockClear(); + editor.setValue.mockClear(); + editor.getValue.mockReset(); + editor.getValue.mockReturnValue(''); + editor.toTextArea.mockClear(); + fromTextArea.mockClear(); +}); + +it('initializes CodeMirror on mount with the initial value', async () => { + render(); + + await waitFor(() => expect(fromTextArea).toHaveBeenCalledTimes(1)); + expect(editor.setValue).toHaveBeenCalledWith('hello'); + expect(editor.on).toHaveBeenCalledWith('change', expect.any(Function)); +}); + +it('tears down the editor on unmount', async () => { + const { unmount } = render(); + + await waitFor(() => expect(fromTextArea).toHaveBeenCalledTimes(1)); + + unmount(); + + expect(editor.off).toHaveBeenCalledWith('change', expect.any(Function)); + expect(editor.toTextArea).toHaveBeenCalledTimes(1); +}); + +it('updates options without recreating the editor', async () => { + const { rerender } = render(); + + await waitFor(() => expect(fromTextArea).toHaveBeenCalledTimes(1)); + editor.setOption.mockClear(); + + rerender(); + await flushAsync(); + + expect(fromTextArea).toHaveBeenCalledTimes(1); + expect(editor.toTextArea).not.toHaveBeenCalled(); + expect(editor.setOption).toHaveBeenCalledWith('mode', 'xml'); + expect(editor.setOption).toHaveBeenCalledWith('readOnly', true); +}); + +it('syncs external value changes into the editor', async () => { + const { rerender } = render(); + + await waitFor(() => expect(fromTextArea).toHaveBeenCalledTimes(1)); + editor.getValue.mockReturnValue('a'); + editor.setValue.mockClear(); + + rerender(); + await flushAsync(); + + expect(editor.setValue).toHaveBeenCalledWith('b'); +}); + +it('does not re-set the value when it already matches the editor content', async () => { + const { rerender } = render(); + + await waitFor(() => expect(fromTextArea).toHaveBeenCalledTimes(1)); + editor.getValue.mockReturnValue('a'); + editor.setValue.mockClear(); + + rerender(); + await flushAsync(); + + expect(editor.setValue).not.toHaveBeenCalled(); +}); + +it('forwards editor changes to onChange', async () => { + const onChange = jest.fn(); + render(); + + await waitFor(() => expect(editor.on).toHaveBeenCalled()); + + const handler = editor.on.mock.calls[0][1]; + act(() => handler({ getValue: () => 'typed' })); + + expect(onChange).toHaveBeenCalledWith('typed'); +}); diff --git a/apps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirror.tsx b/apps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirror.tsx index 183cc885c26ab..6262048633166 100644 --- a/apps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirror.tsx +++ b/apps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirror.tsx @@ -1,9 +1,29 @@ import { useStableCallback } from '@rocket.chat/fuselage-hooks'; -import type { Editor, EditorFromTextArea } from 'codemirror'; -import { useCallback, useEffect, useRef, useState } from 'react'; +import type { Editor, EditorConfiguration, EditorFromTextArea } from 'codemirror'; +import { useEffect, useRef, useState } from 'react'; const defaultGutters = ['CodeMirror-linenumbers', 'CodeMirror-foldgutter']; +type CodeMirrorModule = typeof import('codemirror'); + +let codeMirrorPromise: Promise | undefined; + +const loadCodeMirror = (): Promise => { + if (!codeMirrorPromise) { + codeMirrorPromise = Promise.all([ + import('codemirror'), + import('../../../../../../../app/ui/client/lib/codeMirror/codeMirror'), + import('codemirror/addon/edit/matchbrackets'), + import('codemirror/addon/edit/closebrackets'), + import('codemirror/addon/edit/matchtags'), + import('codemirror/addon/edit/trailingspace'), + import('codemirror/addon/search/match-highlighter'), + import('codemirror/lib/codemirror.css'), + ]).then(([cm]) => (cm as unknown as { default: CodeMirrorModule }).default ?? cm); + } + return codeMirrorPromise; +}; + type CodeMirrorProps = { id: string; placeholder?: string; @@ -37,91 +57,105 @@ function CodeMirror({ showTrailingSpace = true, highlightSelectionMatches = true, readOnly, - value: valueProp, + value, defaultValue, onChange, ...props }: CodeMirrorProps) { - const [value, setValue] = useState(valueProp || defaultValue); const handleChange = useStableCallback(onChange); + const [textArea, setTextArea] = useState(null); + const [codeMirror, setCodeMirror] = useState(null); const editorRef = useRef(null); - const textAreaRef = useCallback( - async (node: HTMLTextAreaElement | null) => { - if (!node) return; - - try { - const { default: CodeMirror } = await import('codemirror'); - await Promise.all([ - import('../../../../../../../app/ui/client/lib/codeMirror/codeMirror'), - import('codemirror/addon/edit/matchbrackets'), - import('codemirror/addon/edit/closebrackets'), - import('codemirror/addon/edit/matchtags'), - import('codemirror/addon/edit/trailingspace'), - import('codemirror/addon/search/match-highlighter'), - import('codemirror/lib/codemirror.css'), - ]); - - editorRef.current = CodeMirror.fromTextArea(node, { - lineNumbers, - lineWrapping, - mode, - gutters, - foldGutter, - matchBrackets, - autoCloseBrackets, - matchTags, - showTrailingSpace, - highlightSelectionMatches, - readOnly, - }); - - editorRef.current.on('change', (doc: Editor) => { - const newValue = doc.getValue(); - setValue(newValue); - handleChange(newValue); - }); - - return () => { - if (node.parentNode) { - editorRef.current?.toTextArea(); - } - }; - } catch (error) { + + // Latest-prop refs read by the init effect without forcing it to re-run. + const initialValueRef = useRef(value ?? defaultValue ?? ''); + const optionsRef = useRef({}); + optionsRef.current = { + lineNumbers, + lineWrapping, + mode, + gutters, + foldGutter, + matchBrackets, + autoCloseBrackets, + matchTags, + showTrailingSpace, + highlightSelectionMatches, + readOnly, + }; + + useEffect(() => { + let cancelled = false; + loadCodeMirror() + .then((mod) => { + if (!cancelled) setCodeMirror(() => mod); + }) + .catch((error) => { console.error('CodeMirror initialization failed:', error); - } - }, - [ - autoCloseBrackets, - foldGutter, - gutters, - highlightSelectionMatches, - lineNumbers, - lineWrapping, - matchBrackets, - matchTags, - mode, - handleChange, - readOnly, - showTrailingSpace, - ], - ); + }); + return () => { + cancelled = true; + }; + }, []); useEffect(() => { - setValue(valueProp); - }, [valueProp]); + if (!textArea || !codeMirror) return; + + const editor = codeMirror.fromTextArea(textArea, optionsRef.current); + editor.setValue(initialValueRef.current); + editorRef.current = editor; + + const handleEditorChange = (doc: Editor) => { + handleChange(doc.getValue()); + }; + editor.on('change', handleEditorChange); + + return () => { + editor.off('change', handleEditorChange); + editor.toTextArea(); + editorRef.current = null; + }; + }, [textArea, codeMirror, handleChange]); useEffect(() => { - if (!editorRef.current) { - return; - } + const editor = editorRef.current; + if (!editor) return; + editor.setOption('lineNumbers', lineNumbers); + editor.setOption('lineWrapping', lineWrapping); + editor.setOption('mode', mode); + editor.setOption('gutters', gutters); + editor.setOption('foldGutter', foldGutter); + editor.setOption('matchBrackets', matchBrackets); + editor.setOption('autoCloseBrackets', autoCloseBrackets); + editor.setOption('matchTags', matchTags); + editor.setOption('showTrailingSpace', showTrailingSpace); + editor.setOption('highlightSelectionMatches', highlightSelectionMatches); + editor.setOption('readOnly', readOnly); + }, [ + lineNumbers, + lineWrapping, + mode, + gutters, + foldGutter, + matchBrackets, + autoCloseBrackets, + matchTags, + showTrailingSpace, + highlightSelectionMatches, + readOnly, + ]); - if (value !== editorRef.current.getValue()) { - editorRef.current.setValue(value ?? ''); + useEffect(() => { + const editor = editorRef.current; + if (!editor) return; + const next = value ?? ''; + if (editor.getValue() !== next) { + editor.setValue(next); } - }, [textAreaRef, value]); + }, [value]); - return