feat: Argument to select moved block in moveBlocksUp/Down (BLO-907)#2723
Open
matthewlipski wants to merge 4 commits intomainfrom
Open
feat: Argument to select moved block in moveBlocksUp/Down (BLO-907)#2723matthewlipski wants to merge 4 commits intomainfrom
moveBlocksUp/Down (BLO-907)#2723matthewlipski wants to merge 4 commits intomainfrom
Conversation
…en editor is read-only
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds optional ChangesBlock Movement with Optional Identifier
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
…block when editor is read-only" This reverts commit 086beeb.
@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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/api/blockManipulation/commands/moveBlocks/moveBlocks.ts (1)
312-354: 💤 Low valueDuplicated
if (blockIdentifier)branch inmoveBlocksUp/moveBlocksDownBoth functions check
if (blockIdentifier)twice: once to resolvesourceBlock(lines 318 / 362) and again to decide betweenmoveBlocksvs.moveSelectedBlocksAndSelection(lines 339 / 384). SincesourceBlockis always defined by the time the second check runs (early return on!sourceBlock), a boolean local variable or a single refactored branch would make the intent clearer and eliminate the duplication.♻️ Suggested simplification (excerpt for `moveBlocksUp`)
export function moveBlocksUp( editor: BlockNoteEditor<any, any, any>, blockIdentifier?: BlockIdentifier, ) { editor.transact(() => { - let sourceBlock: Block<any, any, any> | undefined; - if (blockIdentifier) { - sourceBlock = editor.getBlock(blockIdentifier); - if (!sourceBlock) { - return; - } - } else { - const selection = editor.getSelection(); - sourceBlock = - selection?.blocks[0] || editor.getTextCursorPosition().block; - } + const isExplicit = blockIdentifier !== undefined; + let sourceBlock: Block<any, any, any>; + if (isExplicit) { + const found = editor.getBlock(blockIdentifier!); + if (!found) return; + sourceBlock = found; + } else { + const selection = editor.getSelection(); + sourceBlock = + selection?.blocks[0] || editor.getTextCursorPosition().block; + } const moveUpPlacement = getMoveUpPlacement( editor, editor.getPrevBlock(sourceBlock), editor.getParentBlock(sourceBlock), ); if (!moveUpPlacement) { return; } - if (blockIdentifier) { + if (isExplicit) { moveBlocks(editor, [sourceBlock], moveUpPlacement.referenceBlock, moveUpPlacement.placement); } else { moveSelectedBlocksAndSelection(editor, moveUpPlacement.referenceBlock, moveUpPlacement.placement); } }); }Also applies to: 356-399
🤖 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/api/blockManipulation/commands/moveBlocks/moveBlocks.ts` around lines 312 - 354, The duplicated if (blockIdentifier) branches in moveBlocksUp (and likewise in moveBlocksDown) should be consolidated: after resolving sourceBlock (using editor.getBlock(blockIdentifier) or selection logic) capture a boolean like isExplicit = Boolean(blockIdentifier) (or rename to hadIdentifier) right after you ensure sourceBlock exists, then use that boolean for the later branch to call moveBlocks(...) vs moveSelectedBlocksAndSelection(...); this removes the second repeated blockIdentifier check while preserving the early-return behavior when sourceBlock is undefined and keeps intent clear (reference symbols: moveBlocksUp, moveBlocksDown, sourceBlock, editor.getBlock, moveBlocks, moveSelectedBlocksAndSelection).
🤖 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/api/blockManipulation/commands/moveBlocks/moveBlocks.ts`:
- Around line 160-170: The moveBlocks function currently always calls
flattenColumns, which destroys a moved columnList node when callers (e.g.,
moveBlocksUp/moveBlocksDown) supply an explicit single block identifier; fix by
only flattening when appropriate: detect if blocks.length === 1 and the sole
block is a columnList (check block.type or Block.kind used in your model) and in
that case call editor.insertBlocks with the original block (preserving the
columnList) instead of flattenColumns; alternatively add an optional parameter
(e.g., preserveStructure: boolean) to moveBlocks and thread it through callers
(moveBlocksUp/moveBlocksDown) so callers can opt-in to preserving columnList,
and add a unit test asserting that moving a single columnList preserves its
structure.
---
Nitpick comments:
In `@packages/core/src/api/blockManipulation/commands/moveBlocks/moveBlocks.ts`:
- Around line 312-354: The duplicated if (blockIdentifier) branches in
moveBlocksUp (and likewise in moveBlocksDown) should be consolidated: after
resolving sourceBlock (using editor.getBlock(blockIdentifier) or selection
logic) capture a boolean like isExplicit = Boolean(blockIdentifier) (or rename
to hadIdentifier) right after you ensure sourceBlock exists, then use that
boolean for the later branch to call moveBlocks(...) vs
moveSelectedBlocksAndSelection(...); this removes the second repeated
blockIdentifier check while preserving the early-return behavior when
sourceBlock is undefined and keeps intent clear (reference symbols:
moveBlocksUp, moveBlocksDown, sourceBlock, editor.getBlock, moveBlocks,
moveSelectedBlocksAndSelection).
🪄 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
Run ID: 5e58c310-3426-4e0a-b5e0-0777f550cf98
⛔ Files ignored due to path filters (1)
packages/core/src/api/blockManipulation/commands/moveBlocks/__snapshots__/moveBlocks.test.ts.snapis excluded by!**/*.snap,!**/__snapshots__/**
📒 Files selected for processing (5)
docs/content/docs/reference/editor/manipulating-content.mdxpackages/core/src/api/blockManipulation/commands/moveBlocks/moveBlocks.test.tspackages/core/src/api/blockManipulation/commands/moveBlocks/moveBlocks.tspackages/core/src/editor/BlockNoteEditor.tspackages/core/src/editor/managers/BlockManager.ts
Comment on lines
+160
to
+170
| export function moveBlocks( | ||
| editor: BlockNoteEditor<any, any, any>, | ||
| blocks: Block<any, any, any>[], | ||
| referenceBlock: BlockIdentifier, | ||
| placement: "before" | "after", | ||
| ) { | ||
| editor.transact(() => { | ||
| editor.removeBlocks(blocks); | ||
| editor.insertBlocks(flattenColumns(blocks), referenceBlock, placement); | ||
| }); | ||
| } |
There was a problem hiding this comment.
moveBlocks always applies flattenColumns, which destructures columnList blocks moved by explicit identifier
When moveBlocksUp/moveBlocksDown is called with a blockIdentifier that points to a columnList block, moveBlocks will:
- Remove the entire
columnListnode. - Insert only the inner content (
blockContainernodes extracted from each column) viaflattenColumns.
The columnList structure is permanently destroyed rather than moved. This is the same behavior as selection-based movement (the pre-existing logic), but it is more surprising in the explicit-identifier path since callers directly name the block they intend to move. There is no test or documentation covering this case.
If moving a columnList as a unit is a valid use case, consider skipping flattenColumns when blocks contains only a single explicit block (or expose a flag). At a minimum, adding a test that asserts this behavior makes the implicit contract explicit.
🤖 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/api/blockManipulation/commands/moveBlocks/moveBlocks.ts`
around lines 160 - 170, The moveBlocks function currently always calls
flattenColumns, which destroys a moved columnList node when callers (e.g.,
moveBlocksUp/moveBlocksDown) supply an explicit single block identifier; fix by
only flattening when appropriate: detect if blocks.length === 1 and the sole
block is a columnList (check block.type or Block.kind used in your model) and in
that case call editor.insertBlocks with the original block (preserving the
columnList) instead of flattenColumns; alternatively add an optional parameter
(e.g., preserveStructure: boolean) to moveBlocks and thread it through callers
(moveBlocksUp/moveBlocksDown) so callers can opt-in to preserving columnList,
and add a unit test asserting that moving a single columnList preserves its
structure.
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
This PR adds a
blockIdentifieroptional argument tomoveBlocksUpandmoveBlocksDown. If specified, the provided block is moved up/down. Nothing happens if the block could not be found. In not provided, the selected blocks are moved, i.e. the same behaviour as before is used.Closes #1414
Rationale
This makes the functions more useful for consumers.
Changes
moveBlockshelper function.blockIdentifier: BlockIdentifierargument tomoveBlocksUpandmoveBlocksDown.Impact
N/A
Testing
Added unit tests.
Screenshots/Video
N/A
Checklist
Additional Notes
N/A
Summary by CodeRabbit
New Features
moveBlocksUp()andmoveBlocksDown()methods now accept an optional block identifier parameter, enabling precise block movement while preserving current selection.Documentation
Tests