fix: stop stale node view positions crashing the editor - #2938
Conversation
Bumps every `@tiptap/*` range from `^3.13.0` (resolving to 3.22.4) to `^3.29.2`, including the `pnpm-workspace.yaml` overrides that actually pin `@tiptap/core` and `@tiptap/pm`. Also bumps our direct `prosemirror-*` dependencies to match the ranges `@tiptap/pm@3.29.2` declares. Without this, `prosemirror-model` and `prosemirror-view` each resolved to two copies, which broke type-checking across the packages that import them directly.
`getPos()` is derived from ProseMirror's view-desc tree, but `updateStateInner` assigns the new state before it reconciles that tree. Anything rendering part-way through reconciliation - a re-entrant dispatch from a node view effect, TipTap's `flushSync` while mounting a node view - therefore reads a position that no longer lines up with `view.state.doc`. Resolving it threw `RangeError: Position N out of range`, or `Node should be a bnBlock, but is instead: doc` when the position was in range but pointed at the wrong node. Under React 19 that tears down the consumer's tree rather than being rethrown. Upstream considers this TipTap's problem (ProseMirror/prosemirror#1532) and TipTap considers `flushSync` unavoidable, so recover on our side: - core `getBlockFromNodeView`: falls back to building the block from the node alone. Reached only when a re-entrant dispatch superseded the document, so the node is gone from it and there is no container to read an id from - `type`, `props` and `content` are correct, `id` is freshly generated and belongs to no block. - react `useNodeViewBlock`: falls back to the last block it resolved, seeded from the one core resolved at construction. Both are silent and transient; ProseMirror rebuilds the node view against the current document immediately after, so a stale frame is invisible where a throw is not.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change updates Tiptap and ProseMirror versions. Core and React node views now recover from stale or invalid positions. Tests cover fallback resolution, block retention, reconciliation, and document changes. ChangesNode-view block recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReactBlockSpec
participant useNodeViewBlock
participant getBlockFromNodeView
participant ProseMirrorDoc
ReactBlockSpec->>useNodeViewBlock: provide node-view props and initial block
useNodeViewBlock->>getBlockFromNodeView: resolve block from getPos, node, and doc
getBlockFromNodeView->>ProseMirrorDoc: inspect current node position
ProseMirrorDoc-->>getBlockFromNodeView: return block or position failure
getBlockFromNodeView-->>useNodeViewBlock: return resolved or fallback block
useNodeViewBlock-->>ReactBlockSpec: render with current block
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/schema/blocks/internal.ts (1)
79-94: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRoute the React node-view hook through
getBlockFromNodeView.
packages/core/src/schema/blocks/createSpec.tsalready usesgetBlockFromNodeView, butpackages/react/src/schema/useNodeViewBlock.tsstill callsgetBlockFromPosdirectly during rendering. A stalegetPos()in that render path can throw fromnodeToBlock; use the tolerant wrapper with the React node so the fallback construction applies consistently.🤖 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/core/src/schema/blocks/internal.ts` around lines 79 - 94, Update the React node-view hook to use getBlockFromNodeView with the React node instead of calling getBlockFromPos directly during rendering. Preserve the existing fallback behavior so stale getPos() values do not propagate into nodeToBlock; use the existing getBlockFromNodeView implementation and symbols in useNodeViewBlock.
🧹 Nitpick comments (3)
packages/core/src/schema/blocks/internal.test.ts (1)
56-65: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the "wrong node" test with content assertions.
This test only checks that
getBlockFromNodeViewdoes not throw. The sibling test at Line 32 checkstype,props, andcontentafter the fallback fires. Apply the same assertions here to confirm the fallback produces a correct block, not just a non-throwing one, when the resolved position lands on the wrong node.✅ Proposed stronger assertions
it("recovers from an in-range position that resolves to the wrong node", () => { // Position 0 resolves to the doc rather than a block container — the shape // a bounds check alone would not catch. const doc = editor.prosemirrorState.doc; const orphan = editor.pmSchema.nodes.paragraph.create( null, editor.pmSchema.text("orphaned content"), ); - expect(() => getBlockFromNodeView(() => 0, orphan, doc)).not.toThrow(); + const block = getBlockFromNodeView(() => 0, orphan, doc) as any; + expect(block.type).toBe("paragraph"); + expect(block.content[0].text).toBe("orphaned content"); });🤖 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/core/src/schema/blocks/internal.test.ts` around lines 56 - 65, Strengthen the “recovers from an in-range position that resolves to the wrong node” test by capturing the result of getBlockFromNodeView and asserting its type, props, and content, matching the sibling fallback test. Preserve the existing setup and verify the fallback returns a correct block rather than only confirming that no exception is thrown.tests/src/unit/react/useNodeViewBlock.test.tsx (1)
61-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a StrictMode regression test for the double-invoke claim.
useNodeViewBlock.tsjustifies the render-phase ref write partly by noting it tolerates "StrictMode's double invoke". None of the tests here mount the probe inside<React.StrictMode>. Add one test that wraps<Probe />in<React.StrictMode>and asserts the resolved block is still correct after the double render, to lock in that specific claim.🤖 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 `@tests/src/unit/react/useNodeViewBlock.test.tsx` around lines 61 - 79, Add a regression test for renderHook’s Probe mounted inside React.StrictMode, asserting useNodeViewBlock still resolves the expected block after StrictMode’s double render. Keep the existing non-StrictMode helper behavior unchanged and target the hook resolution returned by renderHook.packages/react/src/schema/useNodeViewBlock.ts (1)
42-54: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify render-phase ref mutation and avoid silently swallowing unrelated errors.
lastBlockRef.currentis written directly in the render body, not in an effect. React's own guidance treats ref writes during render as unsafe unless idempotent for a given render pass, and does not guarantee this is safe if React discards or restarts the render before commit under concurrent scheduling. Confirm that BlockNote's node-view rendering paths never hit that scenario, or document why it cannot.Separately, the bare
catch {}on Line 50 discards every exception fromgetBlockFromPos, not only the two documented shapes (out-of-range position, wrong node type). A genuine, unrelated bug inprops.getPosordocresolution would also be swallowed silently, with no visibility for debugging.♻️ Optional: surface unexpected errors without changing the fallback behavior
try { lastBlockRef.current = getBlockFromPos(props.getPos, doc); - } catch { + } catch (error) { + if (process.env.NODE_ENV !== "production") { + // eslint-disable-next-line no-console + console.debug("useNodeViewBlock: falling back to last known block", error); + } // Expected and self-correcting, so deliberately silent: ProseMirror // re-renders the node view with a usable position immediately after, and // there is nothing a consumer could do about it in the meantime. }Please confirm with the web whether React guarantees ref writes performed synchronously during a function component's render body are preserved correctly across a render that gets discarded/restarted by a concurrent feature (e.g., a Suspense-triggered restart), or whether that remains formally unsupported.
🤖 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/useNodeViewBlock.ts` around lines 42 - 54, Update the lastBlockRef/getBlockFromPos handling to avoid relying on unsupported render-phase ref mutation under concurrent React, or document and verify the node-view lifecycle invariant that makes this safe. Replace the bare catch with handling only the documented invalid-position or wrong-node failures, while rethrowing or surfacing unexpected errors from props.getPos or document resolution. Preserve the existing last-good-value fallback behavior.
🤖 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.
Outside diff comments:
In `@packages/core/src/schema/blocks/internal.ts`:
- Around line 79-94: Update the React node-view hook to use getBlockFromNodeView
with the React node instead of calling getBlockFromPos directly during
rendering. Preserve the existing fallback behavior so stale getPos() values do
not propagate into nodeToBlock; use the existing getBlockFromNodeView
implementation and symbols in useNodeViewBlock.
---
Nitpick comments:
In `@packages/core/src/schema/blocks/internal.test.ts`:
- Around line 56-65: Strengthen the “recovers from an in-range position that
resolves to the wrong node” test by capturing the result of getBlockFromNodeView
and asserting its type, props, and content, matching the sibling fallback test.
Preserve the existing setup and verify the fallback returns a correct block
rather than only confirming that no exception is thrown.
In `@packages/react/src/schema/useNodeViewBlock.ts`:
- Around line 42-54: Update the lastBlockRef/getBlockFromPos handling to avoid
relying on unsupported render-phase ref mutation under concurrent React, or
document and verify the node-view lifecycle invariant that makes this safe.
Replace the bare catch with handling only the documented invalid-position or
wrong-node failures, while rethrowing or surfacing unexpected errors from
props.getPos or document resolution. Preserve the existing last-good-value
fallback behavior.
In `@tests/src/unit/react/useNodeViewBlock.test.tsx`:
- Around line 61-79: Add a regression test for renderHook’s Probe mounted inside
React.StrictMode, asserting useNodeViewBlock still resolves the expected block
after StrictMode’s double render. Keep the existing non-StrictMode helper
behavior unchanged and target the hook resolution returned by renderHook.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 02d6ae3f-fa70-4ac3-8fa7-6ff9fbbcd4ee
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
docs/package.jsonexamples/08-extensions/01-tiptap-arrow-conversion/package.jsonpackage.jsonpackages/core/package.jsonpackages/core/src/schema/blocks/createSpec.tspackages/core/src/schema/blocks/internal.test.tspackages/core/src/schema/blocks/internal.tspackages/react/package.jsonpackages/react/src/index.tspackages/react/src/schema/ReactBlockSpec.tsxpackages/react/src/schema/useNodeViewBlock.tspackages/server-util/package.jsonpackages/xl-ai/package.jsonpackages/xl-multi-column/package.jsonpnpm-workspace.yamltests/package.jsontests/src/unit/react/staleNodeViewPos.test.tsxtests/src/unit/react/useNodeViewBlock.test.tsx
Each test created a `BlockNoteEditor` and never destroyed it, leaking a ProseMirror `DOMObserver` per test. Its `stop()` schedules a `flush()` 20ms later, which could outlive the test environment and fail the run with `ReferenceError: document is not defined` - reported against whichever test file happened to be running at the time. Destroying the editor nulls `view.docView`, so the late `flush()` hits its early-return guard.
Building the standalone block inside the `catch` lets it rethrow the original error directly, dropping the `positionError` local. Behaviour is unchanged - a `throw` inside a `catch` is not caught by its own `try`, so an error from `createAndFill`/`nodeToBlock` still propagates as before.
nperez0111
changed the title
1 participant
Summary
Upgrades every
@tiptap/*dependency from^3.13.0(resolving to 3.22.4) to^3.29.2, and stops a node view's stalegetPos()from throwing out of the render path and tearing down the consumer's React tree.Fixes #2937
Rationale
getPos()is derived from ProseMirror's view-desc tree, butEditorView.updateStateInnerassigns the new state before it reconciles that tree — so anything rendering part-way through reconciliation reads a position that no longer lines up withview.state.doc, and resolving it threwRangeError: Position N out of range(orNode should be a bnBlock, but is instead: docwhen the position was in range but pointed at the wrong node). Upstream considers this TipTap's problem (ProseMirror/prosemirror#1532) and TipTap considersflushSyncunavoidable, so we recover on our side instead; TipTap's own partial fix (ueberdosis/tiptap#8106, in 3.28.0) only catches theTypeErrorthrown insideposBeforeChildand doesn't cover either shape above.Changes
@tiptap/*ranges to^3.29.2, including thepnpm-workspace.yamloverrides that actually pin@tiptap/coreand@tiptap/pm, plus our directprosemirror-*dependencies to match what@tiptap/pm@3.29.2declares (without whichprosemirror-modelandprosemirror-vieweach resolved to two copies and broke type-checking).getBlockFromNodeView, used byaddNodeView, which falls back to building the block from the node alone — correcttype/props/content, with a freshly generatedidthat belongs to no block, since this is only reached once a re-entrant dispatch has superseded the document.useNodeViewBlock, used bycreateReactBlockSpec, which falls back to the last block it resolved, seeded from the one core resolved at construction.Impact
No API change:
blockstays a plainBlockfor consumers ofcreateReactBlockSpec, neverundefined. Both fallbacks are silent and transient — ProseMirror rebuilds the node view against the current document immediately after, so a stale frame is invisible where a throw is not. Neither fallback is reached during normal editing (0 hits across the 828-test e2e suite), so there is no cost on the happy path.Testing
tests/src/unit/react/staleNodeViewPos.test.tsxreproduces the crash end-to-end by dispatching re-entrantly from a node view's mount effect. Verified as a real regression test: bypassing the fallback makes both recovery cases fail withNode should be a bnBlock, but is instead: doc, while the normal-case test still passes.internal.test.tsanduseNodeViewBlock.test.tsxcover each branch, including that the standalone block's synthetic id never collides with a real one.Checklist
Additional Notes
The reproduction only fires because it sets
isEditorContentInitialized = trueby hand. BlockNote never sets it, having replaced TipTap'sPureEditorContentwith its own mounting, so on@tiptap/react>= 3.22 it takes the deferred-microtask path and avoids the mount-timeflushSyncby accident rather than by design — worth revisiting alongsideEditorContent.tsx, which still notifies portal subscribers synchronously rather than adopting upstream's 3.28.0 batching.Summary by CodeRabbit
Bug Fixes
New Features
Chores
Tests