fix: React custom blocks cause infinite rerender on mobile (BLO-1212)#2830
fix: React custom blocks cause infinite rerender on mobile (BLO-1212)#2830matthewlipski wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds an ChangesReact node-view mutation filtering
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 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)
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 |
@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: |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react/src/schema/ReactBlockSpec.tsx (1)
318-350: 🏗️ Heavy liftAdd a mobile regression test for mutation-type filtering.
Lines [318-350] implement behavior that is sensitive to mutation type and target; without regression coverage, this is easy to break. Add a test that asserts wrapper
attributesmutations are ignored, while contentchildList/characterDatamutations are not ignored.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/src/schema/ReactBlockSpec.tsx` around lines 318 - 350, Add a regression test for the ReactBlockSpec.ignoreMutation behavior: create a test that constructs a node-view using the ReactBlockSpec (or the same block spec used in tests) and simulates mutations against the wrapper element and the inner editable content; assert that an attributes mutation on the editable wrapper returns true (ignored), while childList and characterData mutations on the content return false (not ignored). Target the ignoreMutation function in ReactBlockSpec.tsx and exercise cases for mutation.type === "selection" (should return false), an attributes mutation with mutation.target === content wrapper (true), and content childList/characterData mutations where mutation.target is inside [data-node-view-content] (false).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/src/schema/ReactBlockSpec.tsx`:
- Around line 343-347: In ReactBlockSpec.tsx inside the ignoreMutation
implementation (the ignoreMutation function used by the ReactNodeView), stop
returning true for all mutation types when mutation.target === content; instead
only ignore wrapper attribute changes by checking that mutation.type ===
"attributes" && mutation.target === content before returning true so
childList/textInput mutations on the [data-node-view-content] element are not
suppressed.
---
Nitpick comments:
In `@packages/react/src/schema/ReactBlockSpec.tsx`:
- Around line 318-350: Add a regression test for the
ReactBlockSpec.ignoreMutation behavior: create a test that constructs a
node-view using the ReactBlockSpec (or the same block spec used in tests) and
simulates mutations against the wrapper element and the inner editable content;
assert that an attributes mutation on the editable wrapper returns true
(ignored), while childList and characterData mutations on the content return
false (not ignored). Target the ignoreMutation function in ReactBlockSpec.tsx
and exercise cases for mutation.type === "selection" (should return false), an
attributes mutation with mutation.target === content wrapper (true), and content
childList/characterData mutations where mutation.target is inside
[data-node-view-content] (false).
🪄 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: 715e9430-fa98-4470-b143-703543153e9f
📒 Files selected for processing (1)
packages/react/src/schema/ReactBlockSpec.tsx
| // Also ignore mutations for the editable content wrapper | ||
| // element. | ||
| if (mutation.target === content) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ProseMirror NodeView ignoreMutation, should childListmutations targetingcontentDOM be ignored, or treated as content changes that must be handled?
💡 Result:
In ProseMirror NodeViews, childList mutations targeting contentDOM should generally not be ignored [1]. If you ignore mutations within contentDOM, you prevent ProseMirror from detecting changes to the document content, which effectively breaks the editor's ability to sync the DOM with the document state [1][2][3]. The ignoreMutation method is intended to give you control over which DOM changes ProseMirror should observe [2][3]. By returning false, you instruct ProseMirror to re-read the selection or re-parse the range around the mutation [2][3]. Since contentDOM is the container ProseMirror manages to hold the node's children, it is crucial that ProseMirror remains aware of modifications made to it so it can reconcile them with the underlying document model [4][1]. Common practices for ignoreMutation include: 1. Always returning false for mutations that occur within contentDOM, as these are legitimate document content changes [1]. 2. Returning true for mutations that occur in parts of the NodeView's DOM that ProseMirror does not manage (such as custom UI wrappers or non-editable nodes) [1]. 3. Being careful with mutations that occur on the container itself; in some cases, it is necessary to allow ProseMirror to handle these to prevent state desynchronization or issues with cursor positioning [1]. If you find that your NodeView is causing performance issues or unwanted re-renders, it is usually better to ensure that your update method is correctly implemented or that your contentDOM is properly configured, rather than broadly ignoring mutations within it [5][6]. Ignoring mutations within contentDOM is generally counterproductive to the editor's core functionality [1][2].
Citations:
- 1: https://discuss.prosemirror.net/t/nodeviews-with-contentdom-stops-the-cursor-movement-for-a-node-with-text-content/3208
- 2: https://cdn.jsdelivr.net/npm/prosemirror-view@1.32.2/dist/index.d.ts
- 3: https://prosekit.dev/references/pm/view/
- 4: https://cdn.jsdelivr.net/npm/prosemirror-view@1.24.0/dist/index.d.ts
- 5: NodeView reconstruction ProseMirror/prosemirror#851
- 6: https://github.com/ProseMirror/website/blob/master/markdown/guide/view.md
Don’t ignore all wrapper-targeted mutations.
In packages/react/src/schema/ReactBlockSpec.tsx lines ~343-347, ignoreMutation returns true for all mutation types whenever mutation.target === content (the closest [data-node-view-content] wrapper), which also suppresses childList mutations that can represent real editable-content updates (e.g., mobile/IME). Narrow the ignore to wrapper attributes mutations only.
Proposed fix
- if (mutation.target === content) {
+ if (
+ mutation.target === content &&
+ mutation.type === "attributes"
+ ) {
return true;
}📝 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.
| // Also ignore mutations for the editable content wrapper | |
| // element. | |
| if (mutation.target === content) { | |
| return true; | |
| } | |
| // Also ignore mutations for the editable content wrapper | |
| // element. | |
| if ( | |
| mutation.target === content && | |
| mutation.type === "attributes" | |
| ) { | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react/src/schema/ReactBlockSpec.tsx` around lines 343 - 347, In
ReactBlockSpec.tsx inside the ignoreMutation implementation (the ignoreMutation
function used by the ReactNodeView), stop returning true for all mutation types
when mutation.target === content; instead only ignore wrapper attribute changes
by checking that mutation.type === "attributes" && mutation.target === content
before returning true so childList/textInput mutations on the
[data-node-view-content] element are not suppressed.
1 participant
Summary
This PR fixes an issue with React custom blocks sometimes causing infinite re-render loops on mobile. This can be seen most easily with the Alert Block with Full UX example, where attempting to insert an alert block via the slash menu will result in an infinite re-render loop and crash. This can be replicated via the mobile emulator in Chrome dev tools.
The reason this happens is that the way ProseMirror detects when a block's editable content has changed is different on desktop vs mobile. On desktop, it's detected using key events within the editable content wrapper element. On mobile though, key inputs don't use actually use typical key events, but insert characters using IME composition (see here and here). Therefore, ProseMirror instead detects changes in the editable content based on DOM mutations. Yet for some reason, re-renders are triggered by DOM mutations anywhere in the block, not just within its editable content.
And so the problem is that React's own rendering also causes DOM mutations, leading to ProseMirror triggering a re-render, causing React to render the block again, and starting an infinite loop. Therefore, we need to explicitly tell ProseMirror to ignore mutations outside the editable content to avoid this from happening.
Closes #2804
Rationale
This is a severe issue on mobile.
Changes
ignoreMutationtocreateReactBlockSpecto ignore DOM mutations outside of block's editable content.Impact
N/A
Testing
N/A (we don't have testing infrastructure on mobile)
Screenshots/Video
N/A
Checklist
Additional Notes
N/A
Summary by CodeRabbit