fix: multi-column trailing blocks, column hover borders & drop cursor left edge BLO-1226 - #2885
Conversation
…p cursor left edge Fixes #2820. - Trailing block widgets are now also rendered in columns, filling the empty space below a column's last block. Clicking one appends a block inside that column, instead of blocks silently landing below the multi-column area. - Hovering a column list shows light separators between its columns, and the resize border now actually appears when nearing a column boundary (it was suppressed by a side menu check that was almost always true). The side menu is no longer stretched next to headings (which blocked the resize handle) - it keeps its natural size and is positioned with a floating-ui offset instead. - The drop cursor only treats a drop as a "left edge" drop when the cursor is actually left of the block, instead of within a margin inside the block. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds trailing widgets for eligible containers, updates column hover and resize rendering, offsets the side menu by block type, and supports multi-block drag-and-drop in multi-column layouts. It also updates selectors and end-to-end coverage. ChangesMulti-column and editor behavior updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DropCursor
participant DropHandler
participant EditorDocument
User->>DropCursor: Drag blocks to a column edge
DropCursor->>DropHandler: Provide drop position
DropHandler->>EditorDocument: Convert, remove, and insert dragged blocks
EditorDocument-->>User: Render updated column structure
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
@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 (2)
tests/src/end-to-end/multicolumn/multicolumn.test.tsx (1)
76-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the column trailing-block selector to a shared constant.
".bn-block-column > .bn-trailing-block"is an inline selector, mirroring the pattern already extracted forDOC_TRAILING_BLOCK_SELECTOR. Extracting it (e.g.,COLUMN_TRAILING_BLOCK_SELECTOR) would keep selector definitions centralized and consistent with the rest ofconst.ts.🤖 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 `@tests/src/end-to-end/multicolumn/multicolumn.test.tsx` around lines 76 - 87, The column trailing-block selector is hardcoded in the multicolumn end-to-end test, which should be centralized like DOC_TRAILING_BLOCK_SELECTOR. Extract the ".bn-block-column > .bn-trailing-block" selector into a shared constant in const.ts (for example, COLUMN_TRAILING_BLOCK_SELECTOR) and update the test to use that constant so selector definitions stay consistent and easier to maintain.packages/react/src/components/SideMenu/SideMenuController.tsx (1)
13-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded pixel offsets lack traceability to their source.
The magic numbers (39, 27, 18.5, 12, 4, 15) aren't tied to any shared constant or the actual heading/line-height CSS values they're meant to compensate for. If those styles change later, these offsets can silently drift out of sync with no compile-time signal.
Consider deriving the offset from the block's actual rendered first-line height (e.g. via a ref/
getBoundingClientRectmeasurement) instead of hardcoding per-block-type pixel values, or at least add a comment linking each number to the corresponding CSS rule/font-size it compensates for.🤖 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/react/src/components/SideMenu/SideMenuController.tsx` around lines 13 - 55, The hardcoded offsets in getBlockOffset are magic numbers that can drift from the rendered styles, so replace them with values derived from the block’s actual first-line height using a DOM measurement approach in SideMenuController, or otherwise centralize them into named constants with a clear mapping to the relevant CSS/typography rules. Keep the existing block-type branching in getBlockOffset, but make the heading, file, audio, and table offsets traceable to their rendered dimensions so style changes don’t silently break alignment.
🤖 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 `@tests/src/end-to-end/multicolumn/multicolumn.test.tsx`:
- Around line 64-75: The hover-state check in the multicolumn end-to-end test is
non-retrying and can flake; after moving the mouse onto .bn-side-menu, replace
the immediate document.querySelector assertion with a retrying wait/assertion
pattern like the earlier waitForSelector usage so the test waits for
.bn-column-list-hovered to remain present. Keep the fix localized to the
hover-border test and use the existing moveMouseOverElement and waitForSelector
helpers.
---
Nitpick comments:
In `@packages/react/src/components/SideMenu/SideMenuController.tsx`:
- Around line 13-55: The hardcoded offsets in getBlockOffset are magic numbers
that can drift from the rendered styles, so replace them with values derived
from the block’s actual first-line height using a DOM measurement approach in
SideMenuController, or otherwise centralize them into named constants with a
clear mapping to the relevant CSS/typography rules. Keep the existing block-type
branching in getBlockOffset, but make the heading, file, audio, and table
offsets traceable to their rendered dimensions so style changes don’t silently
break alignment.
In `@tests/src/end-to-end/multicolumn/multicolumn.test.tsx`:
- Around line 76-87: The column trailing-block selector is hardcoded in the
multicolumn end-to-end test, which should be centralized like
DOC_TRAILING_BLOCK_SELECTOR. Extract the ".bn-block-column > .bn-trailing-block"
selector into a shared constant in const.ts (for example,
COLUMN_TRAILING_BLOCK_SELECTOR) and update the test to use that constant so
selector definitions stay consistent and easier to maintain.
🪄 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: bc8d6cab-1700-41b5-a23c-3c6fd235d817
⛔ Files ignored due to path filters (1)
tests/src/end-to-end/multicolumn/__snapshots__/trailingBlockInColumn.jsonis excluded by!**/__snapshots__/**
📒 Files selected for processing (13)
packages/core/src/editor/Block.csspackages/core/src/extensions/SideMenu/SideMenu.tspackages/core/src/extensions/TrailingNode/TrailingNode.tspackages/react/src/components/SideMenu/SideMenu.tsxpackages/react/src/components/SideMenu/SideMenuController.tsxpackages/react/src/editor/styles.csspackages/xl-multi-column/src/extensions/ColumnResize/ColumnResizeExtension.tspackages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.tstests/src/end-to-end/copypaste/copypaste.test.tsxtests/src/end-to-end/dragdrop/dragdrop.test.tsxtests/src/end-to-end/multicolumn/multicolumn.test.tsxtests/src/utils/const.tstests/src/utils/copypaste.ts
…, offset traceability comment Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-block selections Fixes #2534. - Dragging the only block of a column to create a new column on the other side threw: the block was removed from the document before the column list was updated, dissolving the (then single-column) column list that the update targeted. Blocks already in the column list are now removed via the children update instead of removeBlocks. - Dragging a multi-block selection to a column edge only moved the first block of the selection (#2534), or threw when the drag fragment nested the blocks in a blockGroup node (e.g. selections of list items). The slice is now converted with fragmentToBlocks, which handles both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 (1)
packages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.ts (1)
83-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecompute the insertion index after filtering empty columns.
indexis taken fromcolumnList.children, but insertion uses the filtered array. If the dragged blocks empty a column before the drop target, the filtered indices shift and the new column lands one slot too far to the right (for example,[A, B, C]→ dragBto the left edge ofCbecomes[A, C, new]). Recompute the target position from the filtered columns and handle the case where the target column itself was removed. Add a test for the preceding-source-column case.🤖 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/multiColumnHandleDropPlugin.ts` around lines 83 - 116, Recompute the drop insertion position after the column list is filtered, because the current `index` from `columnList.children` can be wrong once empty columns are removed in `multiColumnHandleDropPlugin`. Update the logic around the `newChildren` construction so the target position is derived from the filtered columns array, and handle the case where the drop target column itself was removed before insertion. Keep the existing `blocksAlreadyInColumnList`/`draggedBlocks` flow intact, and add coverage for the preceding-source-column scenario where removing the dragged blocks shifts the insertion point.
🧹 Nitpick comments (1)
tests/src/end-to-end/multicolumn/multicolumnDrop.test.tsx (1)
88-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCoverage looks good and the intent comments are helpful. Consider adding a case that drops onto the left edge of the surviving column when the source column empties out — that path is what exposes the stale-index issue flagged in
multiColumnHandleDropPlugin.tsand is currently untested.🤖 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 `@tests/src/end-to-end/multicolumn/multicolumnDrop.test.tsx` around lines 88 - 137, Add an end-to-end test in multicolumnDrop.test.tsx for the empty-source-column case by dragging the only block out of a column and dropping it onto the left edge of the surviving column. Reuse the existing helpers like focusOnOutsideParagraph, dragAndDropBlock, and compareDocToSnapshot, and make the drop target explicit on the left side so it exercises the stale-index path in multiColumnHandleDropPlugin.
🤖 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/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.ts`:
- Around line 83-116: Recompute the drop insertion position after the column
list is filtered, because the current `index` from `columnList.children` can be
wrong once empty columns are removed in `multiColumnHandleDropPlugin`. Update
the logic around the `newChildren` construction so the target position is
derived from the filtered columns array, and handle the case where the drop
target column itself was removed before insertion. Keep the existing
`blocksAlreadyInColumnList`/`draggedBlocks` flow intact, and add coverage for
the preceding-source-column scenario where removing the dragged blocks shifts
the insertion point.
---
Nitpick comments:
In `@tests/src/end-to-end/multicolumn/multicolumnDrop.test.tsx`:
- Around line 88-137: Add an end-to-end test in multicolumnDrop.test.tsx for the
empty-source-column case by dragging the only block out of a column and dropping
it onto the left edge of the surviving column. Reuse the existing helpers like
focusOnOutsideParagraph, dragAndDropBlock, and compareDocToSnapshot, and make
the drop target explicit on the left side so it exercises the stale-index path
in multiColumnHandleDropPlugin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10966b32-0d27-4884-a405-ae9877713000
⛔ Files ignored due to path filters (3)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltests/src/end-to-end/multicolumn/__snapshots__/dragMultipleBlocksToNewColumn.jsonis excluded by!**/__snapshots__/**tests/src/end-to-end/multicolumn/__snapshots__/dragOnlyBlockToOtherSide.jsonis excluded by!**/__snapshots__/**
📒 Files selected for processing (4)
packages/core/src/index.tspackages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.tstests/package.jsontests/src/end-to-end/multicolumn/multicolumnDrop.test.tsx
matthewlipski
left a comment
matthewlipski
left a comment
There was a problem hiding this comment.
Overall good changes but left a few comments, and also I don't think I'm totally happy with the UX still - having the resize handle on top of the side menu looks quite wonky to me. I think it's worth experimenting with the column padding such that we can display the resize bar and side menu side-by-side, see below.
| const lastBlock = rootGroup?.lastChild; | ||
| // Skip the widget when the container already ends with an empty paragraph | ||
| // block (since the user can just type into it). | ||
| function containerNeedsTrailingWidget(container: PMNode): boolean { |
There was a problem hiding this comment.
isEditable check not needed anymore?
There was a problem hiding this comment.
moved down (to call site)
| selector: (state) => state?.block, | ||
| }); | ||
|
|
||
| const dataAttributes = useMemo(() => { |
There was a problem hiding this comment.
We originally went with this approach to make it easier for people to adjust the side menu height for custom blocks they add, since it's just CSS. If we want to go back to FloatingUI for this, we should be certain that we're ok with this being more difficult for consumers to do.
| // rendered next to the block, e.g. the column resize borders. The values are | ||
| // ported from the per-block-type CSS height rules that used to stretch the | ||
| // menu: (first line height - 30px menu height) / 2. | ||
| function getBlockOffset( |
There was a problem hiding this comment.
Related to previous comment - we should at least make sure that the side menu offset is configurable in some way for custom blocks.
There was a problem hiding this comment.
we could allow a prop to customize this in SideMenuController. I'd also be ok with skipping this for now until we actually see someone needing this
Agree, but don't see a great fix for this yet. It would require quite some padding right in order for them not to overlap? This then comes at the cost of space for content cc @robin-lecomte do you have ideas on how to resolve the overlapping? 
|
|
In notion, if you hover the directly over the side menu, the column resize will hide. That fulfills my user expectation. I would hide the resize handle whether the target is not just the column container. Also hovering over one column should only show a single resize handle, not two if you are in a three column layout. |
YousefED
changed the title
|
Congrats @YousefED this is much much better! It solved all the hickups I had identified on my end.
I studied this a bit closer. In Notion you actually display both if your mouse is on the resize column and get near a block side menu.
Agreed. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…e border UX - Compute the new column's insertion index on the filtered column list, so drops that empty a column no longer land one slot too far to the right - Treat dropping a column's entire contents on its own edge as a no-op - Hide the resize border while hovering a side menu button (keeping the lighter separators), matching Notion's behaviour - Asymmetric column padding so the side menu has room next to the resize border, which now fades in instead of appearing instantly
|
fixed some small ux items based on the feedback: positioning / overlapping: 
re. side-menu hovering: I've opted to show the "light" version when hovering those menu items. Making them hide completely makes it easy to get into a flickering user experience. We can also test / decide on this live at some later point if needed
I'm keeping this as-is for now; the idea is the borders show the "layout" of all columns, so you get an idea of the "skeleton" of the area. Not showing all edges / borders would not reach that goal imo |
Vercel infers pnpm 9 from the lockfile version and ignores the packageManager pin during install, so it can't read the catalog entries pnpm 11 writes to the lockfile and fails with ERR_PNPM_OUTDATED_LOCKFILE. The buildCommand already enables corepack, but that runs after install.
corepack enable alone doesn't help during the install phase: the shim it creates is shadowed by Vercel's own pnpm (selected from the lockfile version) earlier in PATH. Invoking pnpm through corepack directly always resolves the packageManager pin.
Vercel provides pnpm 10, which can't reconcile the catalog entries in a pnpm 11 lockfile and fails with ERR_PNPM_OUTDATED_LOCKFILE. corepack shims proved unreliable in the install phase, so fetch the pinned pnpm directly. Keep the version in sync with packageManager in the root package.json.
|
LGTM this PR improves things by a lot |
4 participants
Fixes #2820. Fixes #2534. Fix #2943
Five multi-column UX fixes, plus the side menu positioning change they required.
1. Trailing block support for columns (#2820)
Clicking the empty space below a short column's last block used to do nothing useful, and clicking the strip below the whole multi-column area added a block outside the columns — which looked like it was added to a column until another column grew, leaving a "gap" (see the issue video).
The
TrailingNodeextension now renders its clickable trailing-block widget not just at the end of the document, but also at the end of every column, filling the column's leftover height (columns stretch to their tallest sibling, so short columns have dead space). Clicking it appends a paragraph inside that column and focuses it. As with the doc-level widget, it's a decoration: hidden in read-only mode, absent from exported/collaborative content, and skipped when the column already ends with an empty paragraph. Schemas without columns skip the doc traversal entirely.2. Column hover borders & resize affordance
It was hard to tell where columns begin and end (part of the confusion behind #2820), and the resize border now has two improvements:
menuFrozengetter onSideMenuExtension(i.e. drag-handle menu open), which is what the original check appears to have intended.offset({ crossAxis })inSideMenuControllerinstead. Button positions are pixel-identical; thedata-block-type/data-level/data-urlattributes that only existed for those CSS rules are removed.New behavior (UX improvement): hovering anywhere over a column list fades in light
#eeeseparators between all its columns (showing each column's full extent, including its clickable empty space). Nearing a boundary instantly switches that boundary to the familiar#cccresize border with acol-resizecursor. Both are driven by plugin state (hover-column-list/hover-column/resize) and class-based decorations (bn-column-list-hovered,bn-column-resize-border), so they survive the pointer crossing the side menu.This last fix should make it easier for users to understand where columns are in the document
3. Drop cursor left-edge detection
multiColumnDropCursortreated a drop as a "left edge" (column-creating) drop when the pointer was within a margin inside the block. It now only does so when the pointer is actually left of the block.This caused users to create column-based layouts accidentally (while the intention was to re-order blocks vertically)
4. Dragging a column's last block to create a column on the other side threw
In a 2-column layout, dragging the only block of a column to the other side of the column list (to create a new column there) threw
Block with ID ... not foundand did nothing. The drop handler removed the dragged block from the document before updating the column list - emptying its column dissolved the (then single-column) column list that the update was targeting. Blocks that are already in the column list are now moved purely via the children update instead ofremoveBlocks.5. Multi-block drag to a column edge only moved the first block (#2534)
The drop handler only converted the first node of the drag slice, so dragging a multi-block selection to a column edge moved one block and left the rest behind. Worse: multi-block selections of list items produce drag fragments where the blocks are nested inside a
blockGroupnode, which made the handler throw (Node should be a bnBlock, but is instead: blockGroup) - the drop then did nothing at all. The slice is now converted with core'sfragmentToBlocks(the same helper the drag/clipboard serialization already uses, now exported from@blocknote/core), and all dragged blocks are placed into the new column together.How to test
All in the Multi-Column Blocks playground example (
/basic/multi-column):Trailing blocks (the #2820 repro):
Hover borders / resize:
col-resize; dragging resizes as before (border stays visible during the drag).Drop cursor:
Drag & drop into columns:
Covered by two new e2e tests (
multicolumn.test.tsx: clicking a column's trailing block, and hover borders persisting over the side menu) plus a new doc snapshot; existing e2e selectors were updated to target the doc-level trailing block specifically. The newmulticolumnDrop.test.tsxcovers the two drag fixes with snapshot tests, both verified to fail without the fixes. Full e2e suite, unit tests, andvp checkpass.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
UI/Style
Tests