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-level handlers: adds exported Changes
Sequence DiagramsequenceDiagram
actor User
participant Keyboard as "KeyboardShortcuts"
participant Editor as "BlockNoteEditor"
participant Tr as "Transaction (tr)"
participant Schema as "Schema / NodeTypes"
User->>Keyboard: press Shift-Tab / invoke shortcut
Keyboard->>Editor: call unnestBlock(editor)
Editor->>Editor: editor.transact(fn)
Editor->>Tr: fn(tr) -> liftItem(tr, itemType, groupType)
Tr->>Schema: compute blockRange (predicate on node.type)
alt selection inside itemType
Tr->>Tr: liftToOuterList(tr, itemType, groupType, range)
Tr->>Tr: apply ReplaceAroundStep / create Slice
Tr->>Tr: tr.lift and optional tr.join
else not inside itemType
Tr->>Tr: return false (no-op)
end
Tr->>Editor: scrollIntoView / commit
Editor->>User: UI updates (block un-nested)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.test.ts (3)
621-645: Round-trip test could verify block IDs are preserved.The round-trip test verifies document length and content equality, but doesn't verify that block IDs are preserved. Since block IDs may differ (as noted in the comment on line 641), consider either:
- Explicitly verifying IDs are preserved (if that's the expected behavior), or
- Using a more comprehensive structural comparison that ignores IDs.
✨ Suggested enhancement for more comprehensive verification
// 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); + // Verify IDs are preserved (if expected) + expect(editor.document[0].id).toBe(originalDoc[0].id); + expect(editor.document[1].id).toBe(originalDoc[1].id);🤖 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 621 - 645, The test currently checks content/length after calling nestBlock() and unnestBlock() but doesn't assert block IDs; update the test around withEditor / editor.document to also assert that IDs are preserved across the round trip by comparing originalDoc[0].id and originalDoc[1].id to editor.document[0].id and editor.document[1].id (or, if the intended behavior is that IDs can change, replace the simple content checks with a structural comparison that ignores the id fields when comparing originalDoc and editor.document). Ensure you reference the same originalDoc variable and the editor.document array when adding these assertions.
1-3: Consider consolidating vitest imports.The vitest imports are split across two lines. Consolidating them improves readability.
✨ Suggested consolidation
-import { describe, expect, it } from "vitest"; - -import { afterAll, beforeAll } from "vitest"; +import { afterAll, beforeAll, describe, expect, it } 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 - 3, The test file currently imports from vitest on two separate statements; consolidate them into a single import by merging the named imports (describe, expect, it, beforeAll, afterAll) into one import from "vitest", removing the duplicate import line so the test uses a single consolidated vitest import statement.
635-635: Consider usingstructuredClonefor deep copying.
structuredCloneis a cleaner alternative toJSON.parse(JSON.stringify())for deep cloning objects.✨ Suggested change
- const originalDoc = JSON.parse(JSON.stringify(editor.document)); + const originalDoc = structuredClone(editor.document);🤖 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` at line 635, Replace the manual deep-copy using JSON.parse(JSON.stringify(editor.document)) with structuredClone(editor.document) by changing the assignment to const originalDoc = structuredClone(editor.document); if your test runtime may not support structuredClone, add a small fallback (e.g., globalThis.structuredClone ?? ((obj)=>require('lodash/cloneDeep')(obj)) or import the v8 structuredClone polyfill) so tests remain portable; update the reference in nestBlock.test.ts where originalDoc is created.
🤖 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 621-645: The test currently checks content/length after calling
nestBlock() and unnestBlock() but doesn't assert block IDs; update the test
around withEditor / editor.document to also assert that IDs are preserved across
the round trip by comparing originalDoc[0].id and originalDoc[1].id to
editor.document[0].id and editor.document[1].id (or, if the intended behavior is
that IDs can change, replace the simple content checks with a structural
comparison that ignores the id fields when comparing originalDoc and
editor.document). Ensure you reference the same originalDoc variable and the
editor.document array when adding these assertions.
- Around line 1-3: The test file currently imports from vitest on two separate
statements; consolidate them into a single import by merging the named imports
(describe, expect, it, beforeAll, afterAll) into one import from "vitest",
removing the duplicate import line so the test uses a single consolidated vitest
import statement.
- Line 635: Replace the manual deep-copy using
JSON.parse(JSON.stringify(editor.document)) with
structuredClone(editor.document) by changing the assignment to const originalDoc
= structuredClone(editor.document); if your test runtime may not support
structuredClone, add a small fallback (e.g., globalThis.structuredClone ??
((obj)=>require('lodash/cloneDeep')(obj)) or import the v8 structuredClone
polyfill) so tests remain portable; update the reference in nestBlock.test.ts
where originalDoc is created.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37d032a5-d4bf-4909-8a12-59a1800a2dd4
📒 Files selected for processing (1)
packages/core/src/api/blockManipulation/commands/nestBlock/nestBlock.test.ts
@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: |
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
2 participants
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