fix xl-multi-column - #2944
Conversation
|
@fulcanellee is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe multi-column drop plugin now delegates to reusable handlers. A shared utility computes column-list children, removes emptied columns, and preserves drop order. Tests cover column moves, block wrapping, insertion order, and source-column handling. ChangesMulti-column drop behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant multiColumnHandleDropPlugin
participant dropOntoColumn
participant computeColumnListChildrenAfterDrop
participant Editor
User->>multiColumnHandleDropPlugin: Drop block beside column or block
multiColumnHandleDropPlugin->>dropOntoColumn: Pass column drop data
dropOntoColumn->>computeColumnListChildrenAfterDrop: Compute updated children
computeColumnListChildrenAfterDrop-->>dropOntoColumn: Return reordered columns
dropOntoColumn->>Editor: Apply transaction
Editor-->>User: Show updated block placement
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/xl-multi-column/src/test/dropCursor/dropHandlers.test.ts (1)
36-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the resulting structure, not only the absence of a throw.
The fixed bug causes both a console error and a misplaced block. These two tests assert
not.toThrow()and a snapshot. A snapshot change is easy to accept without review. Add an explicit assertion for the resulting column order and column contents, in the style of Lines 152-156.♻️ Example assertion for the left-edge case
).not.toThrow(); + const updated = getEditor().getBlock("column-list-solo")!; + expect(updated.children.map((c) => c.id)).toEqual([ + expect.any(String), + "column-sibling", + ]); + expect(updated.children[0].children.map((b) => b.id)).toEqual(["solo"]); expect(getEditor().document).toMatchSnapshot();🤖 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/xl-multi-column/src/test/dropCursor/dropHandlers.test.ts` around lines 36 - 66, Strengthen the left- and right-edge tests around dropOntoColumn by explicitly asserting the resulting column order and each column’s contents, following the assertion style used around lines 152-156. Keep the existing no-throw checks and snapshots, but ensure both cases verify the dragged block is placed in the correct target position rather than relying on snapshot updates alone.
🤖 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/xl-multi-column/src/extensions/DropCursor/dropHandlers.ts`:
- Around line 75-79: In dropOntoBlock, update the first removeAndInsertBlocks
call that removes draggedBlock.id to pass fixColumns: false, preventing column
cleanup before the subsequent operation wraps targetBlock.id. Leave the later
insertion behavior unchanged.
In
`@packages/xl-multi-column/src/extensions/DropCursor/util/computeColumnListChildren.ts`:
- Around line 39-55: Handle a missing target column in the list-building logic
around targetIndex and the return from computeColumnListChildren: when findIndex
returns -1 because removal emptied the target column, use an explicit fallback
insertion position that preserves the intended drop location instead of passing
-1 or 0 to toSpliced. Keep the existing left/right positioning behavior when
targetIndex is found.
---
Nitpick comments:
In `@packages/xl-multi-column/src/test/dropCursor/dropHandlers.test.ts`:
- Around line 36-66: Strengthen the left- and right-edge tests around
dropOntoColumn by explicitly asserting the resulting column order and each
column’s contents, following the assertion style used around lines 152-156. Keep
the existing no-throw checks and snapshots, but ensure both cases verify the
dragged block is placed in the correct target position rather than relying on
snapshot updates alone.
🪄 Autofix
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: dd985aef-cc65-47d5-b447-d916ddd207b5
⛔ Files ignored due to path filters (1)
packages/xl-multi-column/src/test/dropCursor/__snapshots__/dropHandlers.test.ts.snapis excluded by!**/*.snap,!**/__snapshots__/**
📒 Files selected for processing (5)
packages/xl-multi-column/src/extensions/DropCursor/dropHandlers.tspackages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.tspackages/xl-multi-column/src/extensions/DropCursor/util/computeColumnListChildren.test.tspackages/xl-multi-column/src/extensions/DropCursor/util/computeColumnListChildren.tspackages/xl-multi-column/src/test/dropCursor/dropHandlers.test.ts
| editor.transact((tr) => { | ||
| if (getNodeById(draggedBlock.id, tr.doc)) { | ||
| removeAndInsertBlocks(tr, [draggedBlock.id], []); | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect removeAndInsertBlocks and the fixColumns option semantics.
fd -t f 'removeAndInsertBlocks*' -E node_modules | while IFS= read -r f; do
echo "== $f"; ast-grep outline "$f" --items all
done
rg -n --type=ts -C 6 'fixColumns' -g '!**/node_modules/**'
rg -n --type=ts -C 4 'function (fixColumnList|fixColumns)' -g '!**/node_modules/**'Repository: TypeCellOS/BlockNote
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files matching dropHandlers / multi-column =="
fd -i 'dropHandlers\.ts|multi.*column|column' packages | head -100
echo "== line count for target =="
fd -t f 'dropHandlers\.ts' packages -x sh -c 'echo "--- $1"; wc -l "$1"' sh {}
echo "== exact target excerpt =="
target="$(fd -t f 'dropHandlers\.ts' packages | head -1)"
if [ -n "${target:-}" ]; then
sed -n '1,140p' "$target" | cat -n
fi
echo "== removeAndInsertBlocks source =="
rg -n --type=ts -C 8 'removeAndInsertBlocks|getNodeById|getNodes' packages | head -200Repository: TypeCellOS/BlockNote
Length of output: 24577
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== core replaceBlocks util =="
fd -t f 'replaceBlocks.*|fixColumnList.*|blockManipulation' packages/core/src packages -x sh -c 'echo "--- $1"; wc -l "$1"' sh {} | rg -n 'replaceBlocks|fixColumnList|blockManipulation' || true
echo "== removeAndInsertBlocks source candidates =="
fd -t f . packages/core packages/xl-multi-column | rg 'replaceBlocks|fixColumn|manipulation' | xargs -r sed -n '1,220p' | nl -ba | sed -n '1,360p'
echo "== core export/import locations for removeAndInsertBlocks =="
rg -n --type=ts -C 3 'removeAndInsertBlocks|export \{|export \*|blockManipulation' packages/core/src packages/xl-multi-column/src | head -240Repository: TypeCellOS/BlockNote
Length of output: 3419
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packages/core/src/api/blockManipulation/commands/replaceBlocks/replaceBlocks.ts =="
sed -n '1,180p' packages/core/src/api/blockManipulation/commands/replaceBlocks/replaceBlocks.ts | cat -n
echo "== packages/core/src/api/blockManipulation/commands/replaceBlocks/util/fixColumnList.ts =="
sed -n '1,220p' packages/core/src/api/blockManipulation/commands/replaceBlocks/util/fixColumnList.ts | cat -n
echo "== relevant tests mentioning column collapsing removal order =="
rg -n --type=ts -C 8 'fixColumns|last.*column|columnList|removeAndInsertBlocks' packages/core/src/api/blockManipulation/commands/replaceBlocks packages/xl-multi-column/src/test/commands packages/xl-multi-column/src/test/commands/util | sed -n '1,260p'Repository: TypeCellOS/BlockNote
Length of output: 43034
Disable column collapsing before wrapping the target block.
removeAndInsertBlocks default-enables fixColumns; removing draggedBlock.id can collapse its empty columnList before the second call tries to wrap targetBlock.id. Pass fixColumns: false to the first removal in dropOntoBlock so the target block remains present for the subsequent insert.
🤖 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/xl-multi-column/src/extensions/DropCursor/dropHandlers.ts` around
lines 75 - 79, In dropOntoBlock, update the first removeAndInsertBlocks call
that removes draggedBlock.id to pass fixColumns: false, preventing column
cleanup before the subsequent operation wraps targetBlock.id. Leave the later
insertion behavior unchanged.
| const targetIndex = withDraggedRemoved.findIndex( | ||
| (column) => column.id === targetColumnId, | ||
| ); | ||
|
|
||
| const newColumn = { | ||
| type: "column" as const, | ||
| children: draggedIsColumn ? draggedBlock.children : [draggedBlock], | ||
| props: {}, | ||
| content: undefined, | ||
| id: UniqueID.options.generateID(), | ||
| }; | ||
|
|
||
| return withDraggedRemoved.toSpliced( | ||
| position === "left" ? targetIndex : targetIndex + 1, | ||
| 0, | ||
| newColumn, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle targetIndex === -1 when the target column is emptied by the removal.
If the dragged block is the only child of the target column, the filter at Line 37 removes the target column. findIndex then returns -1. toSpliced(-1, 0, ...) inserts before the last element, and toSpliced(0, 0, ...) for "right" inserts at the head. Both place the new column at a wrong index. dropOntoColumn only guards targetColumnId === draggedBlock.id, so a block drag onto its own single-block column reaches this path.
Add an explicit fallback.
🐛 Proposed fallback for a missing target column
const targetIndex = withDraggedRemoved.findIndex(
(column) => column.id === targetColumnId,
);
const newColumn = {
type: "column" as const,
children: draggedIsColumn ? draggedBlock.children : [draggedBlock],
props: {},
content: undefined,
id: UniqueID.options.generateID(),
};
+ if (targetIndex === -1) {
+ // The target column was emptied by the removal, so append the new column.
+ return [...withDraggedRemoved, newColumn];
+ }
+
return withDraggedRemoved.toSpliced(
position === "left" ? targetIndex : targetIndex + 1,
0,
newColumn,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const targetIndex = withDraggedRemoved.findIndex( | |
| (column) => column.id === targetColumnId, | |
| ); | |
| const newColumn = { | |
| type: "column" as const, | |
| children: draggedIsColumn ? draggedBlock.children : [draggedBlock], | |
| props: {}, | |
| content: undefined, | |
| id: UniqueID.options.generateID(), | |
| }; | |
| return withDraggedRemoved.toSpliced( | |
| position === "left" ? targetIndex : targetIndex + 1, | |
| 0, | |
| newColumn, | |
| ); | |
| const targetIndex = withDraggedRemoved.findIndex( | |
| (column) => column.id === targetColumnId, | |
| ); | |
| const newColumn = { | |
| type: "column" as const, | |
| children: draggedIsColumn ? draggedBlock.children : [draggedBlock], | |
| props: {}, | |
| content: undefined, | |
| id: UniqueID.options.generateID(), | |
| }; | |
| if (targetIndex === -1) { | |
| // The target column was emptied by the removal, so append the new column. | |
| return [...withDraggedRemoved, newColumn]; | |
| } | |
| return withDraggedRemoved.toSpliced( | |
| position === "left" ? targetIndex : targetIndex + 1, | |
| 0, | |
| newColumn, | |
| ); |
🤖 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/xl-multi-column/src/extensions/DropCursor/util/computeColumnListChildren.ts`
around lines 39 - 55, Handle a missing target column in the list-building logic
around targetIndex and the return from computeColumnListChildren: when findIndex
returns -1 because removal emptied the target column, use an explicit fallback
insertion position that preserves the intended drop location instead of passing
-1 or 0 to toSpliced. Keep the existing left/right positioning behavior when
targetIndex is found.
1 participant
Fix #2943
Summary by CodeRabbit
New Features
Bug Fixes
Tests