fix: Selecting multi-column slash menu items within a column (BLO-905) - #2914
fix: Selecting multi-column slash menu items within a column (BLO-905)#2914matthewlipski wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe slash menu now builds column lists through shared helpers, detects nested ChangesNested column insertion
Estimated code review effort: 3 (Moderate) | ~15 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/xl-multi-column/src/extensions/SuggestionMenu/getMultiColumnSlashMenuItems.tsx (1)
98-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd a regression test for nested insertion.
Cover both two- and three-column actions from inside a column, asserting that insertion succeeds, the new
columnListis a sibling after the ancestor, the cursor lands in the inserted content, and the slash trigger is removed.🤖 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/SuggestionMenu/getMultiColumnSlashMenuItems.tsx` around lines 98 - 105, Add regression coverage for the two- and three-column actions in getMultiColumnSlashMenuItems, invoking each from inside an existing column. Assert insertion succeeds, the new columnList is inserted as a sibling after the ancestor, the cursor is positioned in the inserted content, and the slash trigger is removed.
🤖 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/SuggestionMenu/getMultiColumnSlashMenuItems.tsx`:
- Around line 73-81: Update the ancestorColumnList branch in
getMultiColumnSlashMenuItems so a slash-only trigger paragraph is cleared before
editor.insertBlocks runs. Mirror the cleanup behavior provided by
insertOrUpdateBlockForSlashMenu, while preserving the existing nested insertion
and cursor positioning flow.
---
Nitpick comments:
In
`@packages/xl-multi-column/src/extensions/SuggestionMenu/getMultiColumnSlashMenuItems.tsx`:
- Around line 98-105: Add regression coverage for the two- and three-column
actions in getMultiColumnSlashMenuItems, invoking each from inside an existing
column. Assert insertion succeeds, the new columnList is inserted as a sibling
after the ancestor, the cursor is positioned in the inserted content, and the
slash trigger is removed.
🪄 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 Plus
Run ID: 5f3b86de-0ccb-4bc8-8af1-c64086ce9c07
📒 Files selected for processing (1)
packages/xl-multi-column/src/extensions/SuggestionMenu/getMultiColumnSlashMenuItems.tsx
| if (ancestorColumnList) { | ||
| const newBlock = editor.insertBlocks( | ||
| [columnList], | ||
| ancestorColumnList, | ||
| "after", | ||
| )[0]; | ||
| editor.setTextCursorPosition(newBlock, "start"); | ||
| } else { | ||
| insertOrUpdateBlockForSlashMenu(editor, columnList); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear the slash-trigger block before inserting outside the ancestor.
When the menu is opened from an empty paragraph containing only /, this branch bypasses insertOrUpdateBlockForSlashMenu, which normally replaces that block. insertBlocks leaves the original paragraph unchanged, so nested insertion can leave a visible / in the existing column. Mirror the core helper’s slash-only cleanup before inserting the new list.
🤖 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/SuggestionMenu/getMultiColumnSlashMenuItems.tsx`
around lines 73 - 81, Update the ancestorColumnList branch in
getMultiColumnSlashMenuItems so a slash-only trigger paragraph is cleared before
editor.insertBlocks runs. Mirror the cleanup behavior provided by
insertOrUpdateBlockForSlashMenu, while preserving the existing nested insertion
and cursor positioning flow.
@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: |
|
|
Great, can you add a test? |
2 participants
Summary
This PR fixes an error thrown when selecting one of the multi-column slash menu items from within a column. Because having a
columnListnode inside acolumnviolates the schema, this would throw an error. The fix is that when the item is selected, it checks if there is an ancestor column list from the text cursor position. If there is, a the item inserts the new column list after it, rather than after the current block.Closes #1380
Rationale
This is a bug.
Changes
See above.
Impact
N/A
Testing
Added unit tests.
Screenshots/Video
N/A
Checklist
Additional Notes
N/A
Summary by CodeRabbit
New Features
Bug Fixes
Tests