fix(TableHandles): guard stale block state after doc updates#2821
fix(TableHandles): guard stale block state after doc updates#2821satoren wants to merge 1 commit into
Conversation
Optional chaining on `this.state?.block.id` did not protect when `block` was undefined after `update()` refreshed a removed table block. Clear internal PluginView state instead of only hiding handles, and allow emitUpdate to sync undefined state to the extension store. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@satoren is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe TableHandlesView plugin in BlockNote is updated to safely handle undefined state throughout its lifecycle. The constructor callback type is widened to accept undefined, state access is guarded with optional chaining in comparisons and initialization, and the update logic now fully clears internal tracking when a table becomes disconnected or invalid. ChangesTable Handles Robustness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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 (2)
packages/core/src/extensions/TableHandles/TableHandles.ts (2)
303-316:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPre-existing issue: Non-null assertion could crash on first wrapper hover.
Line 304 uses
...this.state!which assumesthis.stateis always defined when hovering the table wrapper. However, if a user hovers the wrapper before hovering any cell,this.statewill beundefined(initialized at line 146), causing a runtime error.This is inconsistent with the defensive approach taken in this PR (optional chaining at line 297, nullable type at line 166, clearing state at lines 542-544).
🛡️ Proposed fix to use optional spread
this.state = { - ...this.state!, + ...(this.state ?? {}), show: true, showAddOrRemoveRowsButton: belowTable,This ensures the spread operation safely handles undefined state by defaulting to an empty object, aligning with the defensive state handling throughout the rest of this PR.
🤖 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` around lines 303 - 316, The assignment currently uses a non-null assertion "...this.state!" which can throw if this.state is undefined; change the spread to safely handle undefined by using a default object (e.g. "...(this.state ?? {})") when building the new state so components like showAddOrRemoveRowsButton, showAddOrRemoveColumnsButton, referencePosTable, block, widgetContainer, colIndex, rowIndex and referencePosCell are merged without assuming this.state exists.
534-534: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPre-existing issue: Non-null assertion contradicts subsequent check.
Line 534 uses a non-null assertion (
!) ongetBlock(), telling TypeScript the result is never undefined. However, line 536 immediately checksif (!this.state.block ...), indicating the code expects it might be undefined.This creates a disconnect between the type system and runtime behavior. While not a runtime bug (the check at line 536 handles it), it reduces type safety and is inconsistent with the defensive approach of this PR.
♻️ Proposed fix to remove misleading assertion
- this.state.block = this.editor.getBlock(this.state.block.id)!; + this.state.block = this.editor.getBlock(this.state.block.id);This aligns the type signature with the runtime check and improves consistency with the defensive state handling added elsewhere in this PR.
🤖 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 534, Remove the misleading non-null assertion on the getBlock call in TableHandles so the assignment reflects that the result may be undefined: change the assignment using this.editor.getBlock(this.state.block.id)! to one without the "!" (this.editor.getBlock(this.state.block.id)) and keep the existing defensive check if (!this.state.block ...) that follows; ensure any downstream usage of this.state.block handles the undefined case or narrows the type after the check so TypeScript no longer believes the value is always non-null.
🤖 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/extensions/TableHandles/TableHandles.ts`:
- Around line 303-316: The assignment currently uses a non-null assertion
"...this.state!" which can throw if this.state is undefined; change the spread
to safely handle undefined by using a default object (e.g. "...(this.state ??
{})") when building the new state so components like showAddOrRemoveRowsButton,
showAddOrRemoveColumnsButton, referencePosTable, block, widgetContainer,
colIndex, rowIndex and referencePosCell are merged without assuming this.state
exists.
- Line 534: Remove the misleading non-null assertion on the getBlock call in
TableHandles so the assignment reflects that the result may be undefined: change
the assignment using this.editor.getBlock(this.state.block.id)! to one without
the "!" (this.editor.getBlock(this.state.block.id)) and keep the existing
defensive check if (!this.state.block ...) that follows; ensure any downstream
usage of this.state.block handles the undefined case or narrows the type after
the check so TypeScript no longer believes the value is always non-null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5563fdc9-4a45-4a15-b182-a8577b2c99d0
📒 Files selected for processing (1)
packages/core/src/extensions/TableHandles/TableHandles.ts
1 participant
Summary
Fixes a
TypeError: Cannot read properties of undefined (reading 'id')inTableHandlesView.mouseMoveHandlerwhen hovering a table wrapper after the underlying table block was removed or became invalid (e.g. collaborative edits / Yjs DOM replacement).Root cause
update()refreshesthis.state.blockviagetBlock(id). When the block is gone,blockbecomesundefined, but the PluginView kept thestateobject (onlyshowwas set tofalse).On a later
mousemoveover the table wrapper, this expression ran before new state was assigned:Because optional chaining stops at
block,(this.state?.block).idthrows whenblockisundefined.Fix
this.state?.block?.idin the wrapper hover branch.update(), clear internal PluginView state (this.state,tableId,tableElement) instead of leaving a stale object.emitUpdateto passundefinedto the extension store (matching existingstate.block ? … : undefinedlogic).Reproduction (consumer apps)
Reported in
@blocknote/core0.51, staging,mouseMoveHandler)Made with Cursor
Summary by CodeRabbit