fix(core): fix unnesting blocks with siblings (BLO-1017)#2601
Open
fix(core): fix unnesting blocks with siblings (BLO-1017)#2601
Conversation
…(BLO-1017) ProseMirror's upstream liftListItem doesn't work with BlockNote's strict blockContainer schema (blockContent blockGroup?), producing RangeError when unnesting blocks that have siblings after them. This adds a minimal adaptation of liftToOuterList (mirroring the existing sinkItem adaptation) that correctly handles merging siblings into existing child blockGroups. Fixes BLO-835, BLO-899, BLO-953, BLO-844, BLO-847. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors nesting/unnesting to explicit transaction-based handlers, adds exported Changes
Sequence DiagramsequenceDiagram
actor User
participant KeyboardShortcuts as "KeyboardShortcuts"
participant Editor as "BlockNoteEditor"
participant Transaction as "Transaction (tr)"
participant Schema as "Schema / NodeTypes"
User->>KeyboardShortcuts: press Shift-Tab
KeyboardShortcuts->>Editor: unnestBlock(editor)
Editor->>Editor: editor.transact(fn)
Editor->>Transaction: fn(tr) -> liftItem(tr, itemType, groupType)
Transaction->>Schema: compute blockRange (predicate on node.type)
alt selection inside itemType
Transaction->>Transaction: liftToOuterList(tr, itemType, groupType, range)
Transaction->>Transaction: apply ReplaceAroundStep / slice (groupType.create(...))
Transaction->>Transaction: tr.lift and optional tr.join
else not inside itemType
Transaction->>Transaction: return false (no-op)
end
Transaction->>Editor: scrollIntoView / commit
Editor->>User: UI updates (block un-nested)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.ts (1)
98-98: Useconstinstead ofletsinceendis never reassigned.🔧 Proposed fix
- let end = range.end; + const end = range.end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.ts` at line 98, Change the variable declaration for end from a mutable binding to an immutable one because it is never reassigned; in nestBlock.ts (inside the nestBlock function / scope where range is used) replace "let end = range.end" with a const declaration so the identifier end is declared with const for clarity and safety.packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.test.ts (1)
1-6: Remove unused importbeforeEach.The
beforeEachhook is imported but never used in this test file.🧹 Proposed fix
import { describe, expect, it } from "vitest"; import { BlockNoteEditor } from "../../../../editor/BlockNoteEditor.js"; import { PartialBlock } from "../../../../blocks/defaultBlocks.js"; -import { afterAll, beforeAll, beforeEach } from "vitest"; +import { afterAll, beforeAll } from "vitest";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.test.ts` around lines 1 - 6, The import list at the top of nestBlock.test.ts includes an unused symbol beforeEach; remove beforeEach from the import statement (leaving describe, expect, it, beforeAll, afterAll as needed) so the file no longer imports an unused identifier, then run tests/lint to confirm no references to beforeEach remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.test.ts`:
- Around line 1-6: The import list at the top of nestBlock.test.ts includes an
unused symbol beforeEach; remove beforeEach from the import statement (leaving
describe, expect, it, beforeAll, afterAll as needed) so the file no longer
imports an unused identifier, then run tests/lint to confirm no references to
beforeEach remain.
In `@packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.ts`:
- Line 98: Change the variable declaration for end from a mutable binding to an
immutable one because it is never reassigned; in nestBlock.ts (inside the
nestBlock function / scope where range is used) replace "let end = range.end"
with a const declaration so the identifier end is declared with const for
clarity and safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41f813a5-79b4-4d59-b40b-63b6c523e838
⛔ Files ignored due to path filters (1)
packages/core/src/api/blockManipulation/commands/nestBlock/__snapshots__/nestBlock.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.test.tspackages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.tspackages/core/src/extensions/tiptap-extensions/KeyboardShortcuts/KeyboardShortcutsExtension.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.test.ts (2)
1-5: Consider consolidating vitest imports.The imports from
vitestare split across two statements. Consolidating them improves readability.Suggested consolidation
-import { describe, expect, it } from "vitest"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { BlockNoteEditor } from "../../../../editor/BlockNoteEditor.js"; import { PartialBlock } from "../../../../blocks/defaultBlocks.js"; -import { afterAll, beforeAll } from "vitest";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.test.ts` around lines 1 - 5, Consolidate the two separate vitest import statements into a single import that brings in describe, expect, it, beforeAll, and afterAll; update the top of the test file where vitest is imported so the symbols (describe, expect, it, beforeAll, afterAll) come from one import statement rather than two separate ones.
594-618: Consider adding a structural verification for the round-trip test.The current assertions verify content preservation, but don't explicitly confirm that
block1.childrenis empty after the round-trip. Adding this check would strengthen the test against edge cases where children arrays might unexpectedly retain entries.Optional enhancement
// Content should be preserved (IDs may differ but structure/content same) expect(editor.document.length).toBe(originalDoc.length); expect(editor.document[0].content).toEqual(originalDoc[0].content); expect(editor.document[1].content).toEqual(originalDoc[1].content); + expect(editor.document[0].children).toEqual([]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.test.ts` around lines 594 - 618, The test "nest then unnest should be a round trip" currently checks content but not structure; update the assertions after editor.unnestBlock() to also verify that the first block's children array is empty (e.g., assert that editor.document[0].children is undefined or has length 0) to ensure the nest/unnest cycle fully restores structure; locate this in the test using the withEditor setup and the editor.nestBlock()/editor.unnestBlock() calls and add a check referencing editor.document[0].children (or block1.children) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.test.ts`:
- Around line 1-5: Consolidate the two separate vitest import statements into a
single import that brings in describe, expect, it, beforeAll, and afterAll;
update the top of the test file where vitest is imported so the symbols
(describe, expect, it, beforeAll, afterAll) come from one import statement
rather than two separate ones.
- Around line 594-618: The test "nest then unnest should be a round trip"
currently checks content but not structure; update the assertions after
editor.unnestBlock() to also verify that the first block's children array is
empty (e.g., assert that editor.document[0].children is undefined or has length
0) to ensure the nest/unnest cycle fully restores structure; locate this in the
test using the withEditor setup and the editor.nestBlock()/editor.unnestBlock()
calls and add a check referencing editor.document[0].children (or
block1.children) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 889f6867-99f8-4119-8be1-ff06a0cdeb51
📒 Files selected for processing (2)
packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.test.tspackages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.ts
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
Adds a custom
liftItem/liftToOuterListimplementation — a minimal adaptation of ProseMirror's upstreamliftListItem/liftToOuterList— to fix crashes when unnesting blocks that have siblings after them.Rationale
ProseMirror's upstream
liftListItemassumes a flexible list schema (listItemcontent =paragraph block*), but BlockNote uses a strictblockContainerschema (blockContent blockGroup?). When unnesting a block that has siblings after it, the upstream code creates invalidblockContainer(blockGroup(...))nodes (missing the requiredblockContent), causingRangeError: Invalid content for node blockContainer.The existing
sinkListItem(nesting) was already adapted for BlockNote's schema, but the symmetricliftListItem(unnesting) was not — it used TipTap's unmodified upstream directly.Changes
nestBlock.ts: AddedliftToOuterListandliftItemas minimal adaptations of the upstream PM functions, following the same pattern as the existingsinkItemadaptation. Key changes from upstream:node.typeforblockGroup/columninstead offirstChild.typenestedAfterdetection adjustsReplaceAroundStepdepth to merge siblings into existing child blockGroupsgroupType.create()instead ofrange.parent.copy()liftOutOfListpath (root-level blocks can't be unnested)KeyboardShortcutsExtension.ts: Replacedcommands.liftListItem("blockContainer")with directliftItem(tr, ...)calls in Backspace/Enter handlers, andunnestBlock(editor)in Shift-Tab handlersinkListItem→sinkItemandliftListItem→liftItem(no lists in BlockNote)unnestBlocknow returns the boolean result (matchingnestBlock)Impact
Closes #73
Closes #1338
Closes #2066
Closes #481
Closes #598
Testing
nestBlock.test.tscovering all 5 bug scenarios plus edge cases (root-level blocks, different block types, existing children + siblings, nest/unnest round-trip)Checklist
Additional Notes
The implementation mirrors the existing
sinkItempattern exactly: a 1-1 copy of the upstream PM source with clearly numbered minimal changes documented in the docblock.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Refactor