-
-
Notifications
You must be signed in to change notification settings - Fork 670
fix: LinkToolbar Event Listener leak #2335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,14 +4,14 @@ import { Range } from "@tiptap/core"; | |
| import { FC, useEffect, useMemo, useState } from "react"; | ||
|
|
||
| import { useBlockNoteEditor } from "../../hooks/useBlockNoteEditor.js"; | ||
| import { LinkToolbar } from "./LinkToolbar.js"; | ||
| import { LinkToolbarProps } from "./LinkToolbarProps.js"; | ||
| import { useExtension } from "../../hooks/useExtension.js"; | ||
| import { FloatingUIOptions } from "../Popovers/FloatingUIOptions.js"; | ||
| import { | ||
| GenericPopover, | ||
| GenericPopoverReference, | ||
| } from "../Popovers/GenericPopover.js"; | ||
| import { LinkToolbar } from "./LinkToolbar.js"; | ||
| import { LinkToolbarProps } from "./LinkToolbarProps.js"; | ||
|
|
||
| export const LinkToolbarController = (props: { | ||
| linkToolbar?: FC<LinkToolbarProps>; | ||
|
|
@@ -98,15 +98,18 @@ export const LinkToolbarController = (props: { | |
| const destroyOnSelectionChangeHandler = | ||
| editor.onSelectionChange(textCursorCallback); | ||
|
|
||
| editor.domElement?.addEventListener("mouseover", mouseCursorCallback); | ||
| 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 | ||
|
||
| domElement?.addEventListener("mouseover", mouseCursorCallback); | ||
|
|
||
| return () => { | ||
| destroyOnChangeHandler(); | ||
| destroyOnSelectionChangeHandler(); | ||
|
|
||
| editor.domElement?.removeEventListener("mouseover", mouseCursorCallback); | ||
| domElement?.removeEventListener("mouseover", mouseCursorCallback); | ||
| }; | ||
| }, [editor, linkToolbar, link, toolbarPositionFrozen]); | ||
| }, [editor, editor.domElement, linkToolbar, link, toolbarPositionFrozen]); | ||
|
|
||
| const floatingUIOptions = useMemo<FloatingUIOptions>( | ||
| () => ({ | ||
|
|
@@ -161,6 +164,7 @@ export const LinkToolbarController = (props: { | |
| [link?.element], | ||
| ); | ||
|
|
||
| // Q3: similar to Q2; are we sure the component rerenders when editor.isEditable changes? | ||
|
||
| if (!editor.isEditable) { | ||
| return null; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import { BlockNoteEditor } from "@blocknote/core"; | ||
| import { BlockNoteView } from "@blocknote/mantine"; | ||
| import "@blocknote/mantine/style.css"; | ||
| import React from "react"; | ||
| import { flushSync } from "react-dom"; | ||
| import { createRoot } from "react-dom/client"; | ||
| import { afterEach, beforeEach, describe, expect, it } from "vitest"; | ||
|
|
||
| // https://github.com/TypeCellOS/BlockNote/pull/2335 | ||
| describe("BlockNoteView new editor + hover", () => { | ||
| let div: HTMLDivElement; | ||
|
|
||
| beforeEach(() => { | ||
| div = document.createElement("div"); | ||
| document.body.appendChild(div); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| document.body.removeChild(div); | ||
| }); | ||
|
|
||
| it("should not throw error when replacing editor in same container and mouseovering", async () => { | ||
| // 1. Setup container | ||
| const editor1 = BlockNoteEditor.create(); | ||
| const root = createRoot(div); | ||
|
|
||
| // 2. Render first editor twice | ||
| flushSync(() => { | ||
| root.render( | ||
| <React.StrictMode> | ||
| <BlockNoteView editor={editor1} /> | ||
| </React.StrictMode>, | ||
| ); | ||
| }); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 0)); | ||
| flushSync(() => { | ||
| root.render( | ||
| <React.StrictMode> | ||
| <BlockNoteView editor={editor1} /> | ||
| </React.StrictMode>, | ||
| ); | ||
| }); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 0)); | ||
|
|
||
| const editor1DomElement = editor1.domElement; | ||
| expect(editor1DomElement).toBeDefined(); | ||
|
|
||
| // 3. Replace with new editor in same container | ||
| // This causes LinkToolbarController of editor1 to unmount and editor2 to mount | ||
| const editor2 = BlockNoteEditor.create(); | ||
| flushSync(() => { | ||
| root.render( | ||
| <React.StrictMode> | ||
| <BlockNoteView editor={editor2} /> | ||
| </React.StrictMode>, | ||
| ); | ||
| }); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 0)); | ||
|
|
||
| const editor2DomElement = editor2.domElement; | ||
| expect(editor2DomElement).toBeDefined(); | ||
|
|
||
| // 4. Simulate mouseover on the OLD element. | ||
| // If the listener was leaked (due to editor.domElement being null/changed at cleanup), | ||
| // this will fire the callback. callback uses editor1 which might be in bad state -> Crash. | ||
|
|
||
| expect(() => { | ||
| editor1DomElement!.dispatchEvent( | ||
| new MouseEvent("mouseover", { bubbles: true }), | ||
| ); | ||
| }).not.toThrow(); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 0)); | ||
|
|
||
| // Cleanup | ||
| editor1._tiptapEditor.destroy(); | ||
| editor2._tiptapEditor.destroy(); | ||
| root.unmount(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accessing
posAtDomfails fails (because there's no underlying view in the tiptap layer).Agree, this seems like the root cause. Will start eng. roadmap and add this