fix(core): guard getBlock() calls to prevent TypeError on stale blocks - #2941
Merged
Conversation
When a block is removed from the document (via undo, collaboration, or replaceBlocks), closed-over references become stale and getBlock() returns undefined. The non-null assertion (!) let undefined through, causing uncaught TypeErrors in DOM event handlers and React callbacks. Replace all getBlock()! assertions with proper undefined guards across toggle wrappers, side menu, table handles, file panel tabs, file insertion handler, and AI rebase tool. Fixes #2907
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe changes add missing-block guards across clipboard insertion, toggle interactions, side-menu and table-handle updates, React block components, file panels, and Markdown rebasing. Non-null assertions are replaced with early returns, nullable handling, or explicit errors. ChangesStale block guards
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/src/extensions/TableHandles/TableHandles.ts (1)
536-536: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid assigning the nullable lookup through
as any.
as anysuppresses TypeScript checks at the stale-block boundary. Store the lookup in a local nullable variable, run the existing absence and type checks on that variable, and assignthis.state.blockonly after those checks. Use a specific table-block type if a cast is still required.Suggested type-safe shape
- this.state.block = this.editor.getBlock(this.state.block.id) as any; + const currentBlock = this.editor.getBlock(this.state.block.id); if ( - !this.state.block || - this.state.block.type !== "table" || + !currentBlock || + currentBlock.type !== "table" || !this.tableElement?.isConnected ) { // existing cleanup } + this.state.block = currentBlock;As per coding guidelines, use
vp run lintfor linting and type-checking.🤖 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/extensions/TableHandles/TableHandles.ts` at line 536, Update the block refresh logic in the relevant TableHandles method to store getBlock(this.state.block.id) in a nullable local variable instead of assigning through as any. Apply the existing missing-block and type checks to that variable, then assign this.state.block only after validation, using the specific table-block type if a cast remains necessary; verify with vp run lint.Source: Coding guidelines
🤖 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/core/src/extensions/SideMenu/SideMenu.ts`:
- Around line 244-249: Update the missing-block branch in the side-menu hover
handling around getBlock to hide the menu and emit the existing state before
returning. Clear or disable the stale visible state and remove the old
state.block reference so later same-block handling and side-menu actions cannot
reuse the removed block.
In `@packages/react/src/components/FilePanel/DefaultTabs/EmbedTab.tsx`:
- Around line 44-66: The update paths in EmbedTab must re-check block existence
immediately before each editor.updateBlock call using
editor.getBlock(props.blockId), and skip the update when the target was removed.
Apply the same guard in UploadTab.tsx at lines 79-82, ensuring a missing block
does not mark the upload as failed when the continuation is only ignoring a
removed target.
In `@packages/react/src/components/FilePanel/DefaultTabs/UploadTab.tsx`:
- Around line 79-82: Update the asynchronous upload continuation around
editor.uploadFile to re-check the block ID after the upload resolves, and call
editor.updateBlock only when the block still exists. Preserve the existing
render-time !block guard and finally block that clears loading state.
---
Nitpick comments:
In `@packages/core/src/extensions/TableHandles/TableHandles.ts`:
- Line 536: Update the block refresh logic in the relevant TableHandles method
to store getBlock(this.state.block.id) in a nullable local variable instead of
assigning through as any. Apply the existing missing-block and type checks to
that variable, then assign this.state.block only after validation, using the
specific table-block type if a cast remains necessary; verify with vp run lint.
🪄 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 Plus
Run ID: 9489547f-1d05-4b9b-850b-9ba85167dad5
📒 Files selected for processing (8)
packages/core/src/api/clipboard/fromClipboard/handleFileInsertion.tspackages/core/src/blocks/ToggleWrapper/createToggleWrapper.tspackages/core/src/extensions/SideMenu/SideMenu.tspackages/core/src/extensions/TableHandles/TableHandles.tspackages/react/src/blocks/ToggleWrapper/ToggleWrapper.tsxpackages/react/src/components/FilePanel/DefaultTabs/EmbedTab.tsxpackages/react/src/components/FilePanel/DefaultTabs/UploadTab.tsxpackages/xl-ai/src/api/formats/markdown-blocks/tools/rebaseTool.ts
4 tasks
…fety - SideMenu: hide menu and clear hoveredBlock when getBlock returns undefined, preventing stale visible state - EmbedTab: re-check block existence before each updateBlock call - UploadTab: re-check block existence after async upload resolves - TableHandles: use typed local variable instead of as any cast
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Guard all unguarded
editor.getBlock()!non-null assertions across the codebase to prevent uncaughtTypeErrors when blocks are removed from the document while closures still reference them.Rationale
When a block is removed (via undo, collaborative editing,
replaceBlocks, or menu actions that recreate the block), closed-over block references become stale andgetBlock()returnsundefined. The non-null assertion (!) letsundefinedpass through, causing uncaughtTypeError: Cannot read properties of undefined (reading 'id')in DOM event handlers and React callbacks. These errors escape React error boundaries and land inwindow.onerror, making them indistinguishable from real crashes in error monitoring. This is the same class of bug previously fixed in TableHandles (#2821/#2847).Changes
createToggleWrapper.ts: Guard 4getBlock(block)!calls — early-return in click handler, conditional guard inonChangecallback.ToggleWrapper.tsx(React): Guard 3getBlock(block)!calls — early-return inhandleToggle, return 0 inuseEditorStateselector, passblockdirectly to click handler.SideMenu.ts: ExtractgetBlock()into a variable and guard before state assignment.TableHandles.ts: Remove misleading!assertion (line already has a null check on the next line).handleFileInsertion.ts: GuardgetBlock(id)!and downstreaminsertedBlockIdusage.rebaseTool.ts(markdown): Add guard matching the HTML counterpart's existing pattern.EmbedTab.tsx/UploadTab.tsx: Remove!, add null guard after hooks (respecting React rules of hooks).Impact
No functional changes for the happy path. Stale block references now silently bail out instead of throwing uncaught errors. This matches the existing guarded patterns already used elsewhere in the codebase (optional chaining on the same
getBlock()calls, the #2821 TableHandles fix).Testing
vp run lintpasses with 0 errors.vp run buildsucceeds across all packages.vp run testpasses all 695+ unit tests across all packages.Checklist
Additional Notes
No unit tests added — the bug requires a stale block reference from a removed DOM node, which is difficult to reproduce deterministically (the issue reporter also could not find reliable manual repro steps). The fix is a defensive guard pattern consistent with existing code.
Fixes #2907
Summary by CodeRabbit