Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces a new React hook Changes
Sequence DiagramsequenceDiagram
participant Component as React Component
participant Hook as useEditorDOMElement()
participant Editor as BlockNoteEditor
participant TipTap as TipTap Editor
participant State as useEditorState()
Component->>Hook: Call useEditorDOMElement()
Hook->>Editor: useBlockNoteEditor() - get editor
alt editor.headless is true
Hook->>TipTap: Register "create" event listener on _tiptapEditor
TipTap-->>Hook: "create" event fires during initialization
Hook->>Hook: Set initialized = true
end
Hook->>State: useEditorState() - track domElement
State->>Editor: Select ctx.editor?.domElement
State-->>Hook: Return reactive editorDOMElement value
Hook-->>Component: editorDOMElement
Component->>Component: Update event listeners/references with editorDOMElement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
I'd really prefer not to do this if at all possible. This causes jank for everyone else. I'd prefer something at the read-side (e.g. making the reading of the domElement reactive, or making the component that needs this element on render to remount). But remounting the whole editor is way too much
nperez0111
left a comment
There was a problem hiding this comment.
I'd prefer a different solution here, this is too much. Especially for an issue we can't reliably reproduce
@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: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react/src/components/SuggestionMenu/hooks/useSuggestionMenuKeyboardNavigation.ts (1)
9-9: Consider removing_editorfrom the signature if external consumers don't depend on it.Since the hook is exported publicly and the parameter is unused, removal would be a breaking change. If this hook isn't consumed by external packages, dropping the argument would simplify the API and avoid confusion for maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/react/src/components/SuggestionMenu/GridSuggestionMenu/hooks/useGridSuggestionMenuKeyboardNavigation.ts`:
- Around line 8-15: The hook useGridSuggestionMenuKeyboardNavigation currently
accepts an explicit _editor argument but ignores it by always using
useEditorDOMElement() from context; update the implementation to pass the
explicit editor through to useEditorDOMElement (i.e., call
useEditorDOMElement(editor) or similar) and modify useEditorDOMElement to accept
an optional editorOverride parameter that falls back to useBlockNoteEditor when
not provided so keyboard listeners target the provided editor instance and
restore correct behavior for multi-editor/custom usage.
In `@packages/react/src/hooks/useEditorDomElement.ts`:
- Around line 24-33: The handler passed to editor._tiptapEditor.on("create")
uses setInitialized(true) which is idempotent and prevents re-renders after the
first create; change the effect to update state with a non-idempotent update
(e.g. flip a boolean or increment a counter) so each "create" event forces a
render. Update the useEffect in useEditorDomElement.ts around setInitialized and
the editor._tiptapEditor.on/off("create") handlers to call setInitialized(prev
=> !prev) or setInitialized(n => n + 1) instead of setInitialized(true) so
descendant consumers see a fresh editor.domElement on every create.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1dc22a0-e78c-431f-8f52-e29b4f4e4561
📒 Files selected for processing (10)
packages/core/src/editor/BlockNoteEditor.tspackages/react/src/components/FormattingToolbar/DefaultButtons/CreateLinkButton.tsxpackages/react/src/components/LinkToolbar/LinkToolbarController.tsxpackages/react/src/components/Popovers/PositionPopover.tsxpackages/react/src/components/SuggestionMenu/GridSuggestionMenu/GridSuggestionMenuController.tsxpackages/react/src/components/SuggestionMenu/GridSuggestionMenu/hooks/useGridSuggestionMenuKeyboardNavigation.tspackages/react/src/components/SuggestionMenu/SuggestionMenuController.tsxpackages/react/src/components/SuggestionMenu/hooks/useSuggestionMenuKeyboardNavigation.tspackages/react/src/hooks/useEditorDomElement.tspackages/react/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/editor/BlockNoteEditor.ts
| _editor: BlockNoteEditor<any, any, any>, | ||
| query: string, | ||
| items: Item[], | ||
| columns: number, | ||
| onItemClick?: (item: Item) => void, | ||
| ) { | ||
| const editorDOMElement = useEditorDOMElement(); | ||
| const [selectedIndex, setSelectedIndex] = useState<number>(0); |
There was a problem hiding this comment.
Don’t ignore the explicit editor argument in this exported hook.
The implementation now binds keyboard listeners using context (useEditorDOMElement()), while Line 8 still accepts an editor argument. That can desync targets in multi-editor/custom usage and is a behavioral regression for consumers passing an explicit editor.
💡 Suggested fix
export function useGridSuggestionMenuKeyboardNavigation<Item>(
- _editor: BlockNoteEditor<any, any, any>,
+ editor: BlockNoteEditor<any, any, any>,
query: string,
items: Item[],
columns: number,
onItemClick?: (item: Item) => void,
) {
- const editorDOMElement = useEditorDOMElement();
+ const editorDOMElement = useEditorDOMElement(editor);And update useEditorDOMElement to accept an optional editor override:
export function useEditorDOMElement(
editorOverride?: BlockNoteEditor<any, any, any>,
) {
const contextEditor = useBlockNoteEditor<any, any, any>();
const editor = editorOverride ?? contextEditor;
// ...
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/react/src/components/SuggestionMenu/GridSuggestionMenu/hooks/useGridSuggestionMenuKeyboardNavigation.ts`
around lines 8 - 15, The hook useGridSuggestionMenuKeyboardNavigation currently
accepts an explicit _editor argument but ignores it by always using
useEditorDOMElement() from context; update the implementation to pass the
explicit editor through to useEditorDOMElement (i.e., call
useEditorDOMElement(editor) or similar) and modify useEditorDOMElement to accept
an optional editorOverride parameter that falls back to useBlockNoteEditor when
not provided so keyboard listeners target the provided editor instance and
restore correct behavior for multi-editor/custom usage.
| const [, setInitialized] = useState(!editor.headless); | ||
| useEffect(() => { | ||
| if (!editor.headless) { | ||
| return; | ||
| } | ||
| const handler = () => setInitialized(true); | ||
| editor._tiptapEditor.on("create", handler); | ||
| return () => { | ||
| editor._tiptapEditor.off("create", handler); | ||
| }; |
There was a problem hiding this comment.
Force a re-render on every create event, not just the first one.
setInitialized(true) is idempotent, so subsequent mount cycles for the same editor instance won’t trigger a render. That can leave descendants bound to stale/missing editor.domElement after remount.
💡 Suggested fix
- const [, setInitialized] = useState(!editor.headless);
+ const [, bumpMountVersion] = useState(0);
useEffect(() => {
if (!editor.headless) {
return;
}
- const handler = () => setInitialized(true);
+ const handler = () => bumpMountVersion((v) => v + 1);
editor._tiptapEditor.on("create", handler);
return () => {
editor._tiptapEditor.off("create", handler);
};
}, [editor]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [, setInitialized] = useState(!editor.headless); | |
| useEffect(() => { | |
| if (!editor.headless) { | |
| return; | |
| } | |
| const handler = () => setInitialized(true); | |
| editor._tiptapEditor.on("create", handler); | |
| return () => { | |
| editor._tiptapEditor.off("create", handler); | |
| }; | |
| const [, bumpMountVersion] = useState(0); | |
| useEffect(() => { | |
| if (!editor.headless) { | |
| return; | |
| } | |
| const handler = () => bumpMountVersion((v) => v + 1); | |
| editor._tiptapEditor.on("create", handler); | |
| return () => { | |
| editor._tiptapEditor.off("create", handler); | |
| }; | |
| }, [editor]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/src/hooks/useEditorDomElement.ts` around lines 24 - 33, The
handler passed to editor._tiptapEditor.on("create") uses setInitialized(true)
which is idempotent and prevents re-renders after the first create; change the
effect to update state with a non-idempotent update (e.g. flip a boolean or
increment a counter) so each "create" event forces a render. Update the
useEffect in useEditorDomElement.ts around setInitialized and the
editor._tiptapEditor.on/off("create") handlers to call setInitialized(prev =>
!prev) or setInitialized(n => n + 1) instead of setInitialized(true) so
descendant consumers see a fresh editor.domElement on every create.
Summary
This PR makes the editor container component re-render when the editor view is mounted. This fixes certain cases where descendants rely on
editor.domElementto be defined. However, when it becomes defined, there is nothing which explicitly triggers a re-render, meaning things like event listeners may not get attached until some other unrelated effect triggers a re-render.Closes #2546
Rationale
See above.
Changes
BlockNoteEditoronMountandonUnmountreturn callbacks to destroy the listeners.BlockNoteViewComponentforce a re-render when the TipTap editor'screateevent fires.Impact
N/A
Testing
I wasn't able to reproduce the issue in #2546 locally, and have not seen other cases of this elsewhere. Therefore, no tests have been added.
Screenshots/Video
N/A
Checklist
Additional Notes
Summary by CodeRabbit
useEditorDOMElement()hook for accessing the editor's DOM element in React components.onMount()andonUnmount()methods now return unsubscribe/removal functions for improved event listener management.