Memoize debounced submit and add cleanup#603
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the TextEditor component by introducing a debounced submitChange function memoized with useMemo and adding a cleanup effect. Feedback suggests using flush() instead of cancel() in the cleanup to prevent data loss of pending changes. Additionally, it is recommended to memoize the handleChange function using useCallback and update the imports accordingly to optimize performance and prevent unnecessary re-renders of the editor component.
|
|
||
| useEffect(() => { | ||
| return () => { | ||
| submitChange.cancel(); |
There was a problem hiding this comment.
Using cancel() in the cleanup effect will discard any pending changes if the component unmounts or the file path changes before the 500ms debounce timer expires. For a text editor where data persistence is critical, it is recommended to use flush() instead. This ensures that any final keystrokes are submitted to the API before the debounced function is disposed.
| submitChange.cancel(); | |
| submitChange.flush(); |
| import * as monaco from 'monaco-editor'; | ||
| import Editor, { Monaco } from '@monaco-editor/react'; | ||
| import { useEffect, useRef } from 'react'; | ||
| import { useEffect, useMemo, useRef } from 'react'; |
There was a problem hiding this comment.
The useCallback hook should be imported from 'react' to allow memoizing the handleChange callback, which is a best practice when passing functions to complex child components like the Monaco Editor.
| import { useEffect, useMemo, useRef } from 'react'; | |
| import { useCallback, useEffect, useMemo, useRef } from 'react'; |
| const handleChange = (value: string | undefined, ev: monaco.editor.IModelContentChangedEvent) => { | ||
| if (!isEditorReady.value) return; | ||
| submitChange(value, ev); | ||
| }; |
There was a problem hiding this comment.
The handleChange function is currently recreated on every render. Since it is passed as a prop to the Editor component, it should be memoized using useCallback. This ensures that the Editor component doesn't receive a new function reference unless its dependencies change, which can prevent unnecessary internal updates or side effects within the editor wrapper.
| const handleChange = (value: string | undefined, ev: monaco.editor.IModelContentChangedEvent) => { | |
| if (!isEditorReady.value) return; | |
| submitChange(value, ev); | |
| }; | |
| const handleChange = useCallback((value: string | undefined, ev: monaco.editor.IModelContentChangedEvent) => { | |
| if (!isEditorReady.value) return; | |
| submitChange(value, ev); | |
| }, [submitChange, isEditorReady]); |
# Conflicts: # packages/origine2/src/pages/editor/TextEditor/TextEditor.tsx
Close #601