fix: LinkToolbar Event Listener leak #2335
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
YousefED
changed the title
| editor.domElement?.addEventListener("mouseover", mouseCursorCallback); | ||
| const domElement = editor.domElement; | ||
|
|
||
| // Q 1: why can domElement be available when <LinkToolbarController/> is rendered? |
There was a problem hiding this comment.
Why would domElement not be available? It would seem to me that is sort of the default state of things?
There was a problem hiding this comment.
Whoops my comment was wrong. Question should be why it can be unavailable (i.e. undefined).
| const domElement = editor.domElement; | ||
|
|
||
| // Q 1: why can domElement be available when <LinkToolbarController/> is rendered? | ||
| // Q 2: this useEffect will not necessarily run when editor.domElement changes |
There was a problem hiding this comment.
We need to enable to react eslint plugin, it would have caught this.
There was a problem hiding this comment.
I don't think it's just a matter of eslint; because the component will also not automatically rerender when domElement changes; accessing the domElement property is non-reactive, so it's "dangerous" to depend on this state from the React layer?
There was a problem hiding this comment.
Was this this observed, or a hypothetical?
because the component will also not automatically rerender when domElement changes;
My understanding is that this reference should be stable across renders.
| [link?.element], | ||
| ); | ||
|
|
||
| // Q3: similar to Q2; are we sure the component rerenders when editor.isEditable changes? |
There was a problem hiding this comment.
This should be derived from a useEditorState, rather than queried at render time
| // Q4: posAtDOM can fail if the editor view is not available | ||
| // (e.g. if the editor is not mounted) | ||
| // a) Unfortunately, TS doesn't give an error about this. Can we make this type safe? | ||
| // b) Double check other references of editor.prosemirrorView |
There was a problem hiding this comment.
posAtDom fails, or does accessing editor.prosemirrorView?
TS could never know if posAtDom fails at runtime
TS could say the editor.prosemirrorView is undefined
anytime editor.prosemirrorView or editor.prosemirrorState is used, it should be considered a failing of our API
There was a problem hiding this comment.
accessing posAtDom fails fails (because there's no underlying view in the tiptap layer).
anytime editor.prosemirrorView or editor.prosemirrorState is used, it should be considered a failing of our API
Agree, this seems like the root cause. Will start eng. roadmap and add this
Fix LinkToolbar Event Listener Leak on Remount
Summary
This PR addresses an issue where event listeners were not being correctly removed in LinkToolbarController during editor remounts, and adds a regression test to verify the fix.
closes #2327
Rationale
When the editor component is unmounted or rapidly re-mounted, the cleanup function in
useEffectcould previously fail to remove themouseoverevent listener if the underlyingdomElementreference had changed or become undefined. This changes ensuresremoveEventListeneris always called on the correct element instance that the listener was originally attached to.Changes
useEffectscope to ensureremoveEventListeneris called on the correct element during cleanup.Impact
Prevents runtime errors and potential memory leaks when the editor is mounted and unmounted frequently
Testing
Screenshots/Video
N/A
Checklist
Additional Notes
I've left several comments in the code raising questions about the current architecture for handling these event listeners, which we should discuss in a future refactor.