fix: Multi-column errors when moving the only block out of a column#2678
fix: Multi-column errors when moving the only block out of a column#2678Tokonigeorge wants to merge 1 commit intoTypeCellOS:mainfrom
Conversation
|
@Tokonigeorge is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughRefactors the block move operation to resolve destination positions upfront, remap insertion positions through transaction steps following block removal, and use direct ProseMirror Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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/xl-multi-column/src/test/commands/moveBlocks.test.ts (2)
63-139: Add parallelmoveBlocksDownregression coverage.The PR description and the production fix apply equally to
moveBlocksDown, but the new describe block only exercisesmoveBlocksUp. The "after" placement branch (targetPos = posBeforeNode + nodeSize) is not covered by these regression tests; a parallelmoveBlocksDowncase for moving the only block in the last column out of the columnList would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xl-multi-column/src/test/commands/moveBlocks.test.ts` around lines 63 - 139, Add a parallel test case exercising moveBlocksDown to hit the "after" placement branch (targetPos = posBeforeNode + nodeSize): duplicate the existing single-block-column tests that call editor.moveBlocksUp but instead set the text cursor in the last column's only paragraph (e.g., "only-paragraph-b" / "only-paragraph-d") and invoke editor.moveBlocksDown(), asserting it does not throw; ensure one test covers the columnList preceded by a sibling (same setup as "column-list-single-2" block) so the branch for moving the only block out of the columnList into the following position is covered.
99-99: Strengthen regression assertions beyond "does not throw".The fix's risk surface is mostly silent: an off-by-one in the position remap, or a future change to
fixColumnList's step shape, would not throw but would land the moved block in the wrong place (e.g., afteronly-paragraph-brather than before the collapsedcolumnList). Consider asserting the resulting document viatoMatchSnapshot()or an explicit shape check so a positional regression is caught.🧪 Suggested addition
- expect(() => editor.moveBlocksUp()).not.toThrow(); + expect(() => editor.moveBlocksUp()).not.toThrow(); + expect(editor.document).toMatchSnapshot();Also applies to: 137-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xl-multi-column/src/test/commands/moveBlocks.test.ts` at line 99, The test currently only asserts editor.moveBlocksUp() does not throw, which misses positional regressions; after invoking editor.moveBlocksUp() (and the related call at the other occurrence), capture the editor document/state (e.g., via the test helper you use to inspect the doc, or a serializer like getDocument()/toJSON()/state.doc) and add an assertion that checks the resulting structure — either expect(serializedDoc).toMatchSnapshot() or explicit expectations that the moved block now appears before the collapsed columnList (and not after only-paragraph-b). Also update both occurrences (the expect(...not.toThrow()) at the two spots noted) to include the new structural assertion instead of relying solely on not.toThrow().
🤖 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/xl-multi-column/src/test/commands/moveBlocks.test.ts`:
- Around line 63-139: Add a parallel test case exercising moveBlocksDown to hit
the "after" placement branch (targetPos = posBeforeNode + nodeSize): duplicate
the existing single-block-column tests that call editor.moveBlocksUp but instead
set the text cursor in the last column's only paragraph (e.g.,
"only-paragraph-b" / "only-paragraph-d") and invoke editor.moveBlocksDown(),
asserting it does not throw; ensure one test covers the columnList preceded by a
sibling (same setup as "column-list-single-2" block) so the branch for moving
the only block out of the columnList into the following position is covered.
- Line 99: The test currently only asserts editor.moveBlocksUp() does not throw,
which misses positional regressions; after invoking editor.moveBlocksUp() (and
the related call at the other occurrence), capture the editor document/state
(e.g., via the test helper you use to inspect the doc, or a serializer like
getDocument()/toJSON()/state.doc) and add an assertion that checks the resulting
structure — either expect(serializedDoc).toMatchSnapshot() or explicit
expectations that the moved block now appears before the collapsed columnList
(and not after only-paragraph-b). Also update both occurrences (the
expect(...not.toThrow()) at the two spots noted) to include the new structural
assertion instead of relying solely on not.toThrow().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4d1ee46-a720-40c7-872b-fa8c26d6de2b
📒 Files selected for processing (2)
packages/core/src/api/blockManipulation/commands/moveBlocks/moveBlocks.tspackages/xl-multi-column/src/test/commands/moveBlocks.test.ts
1 participant
Summary
Fixes the
Uncaught Error: Block with ID … not foundthrown bymoveBlocksUp/moveBlocksDownwhen the block being moved is the only block in its column. The throw happens insideeditor.transact, so the transaction is rolled back and the block does not move.Closes #2594
Rationale
When a column contains only a single block, removing that block from the column makes the column empty.
removeAndInsertBlocksthen runsfixColumnList, which removes the empty column. ProseMirror's schema requires at least two columns, so it auto-fills an empty one, andfixColumnListthen replaces the entirecolumnListwith the contents of the remaining non-empty column. ThecolumnList's ID no longer exists in the document.moveSelectedBlocksAndSelectionwas cachingreferenceBlock(thecolumnListitself, in the move-out-of-column case) and callingeditor.insertBlocks(..., referenceBlock, placement)afterremoveBlocks. The subsequentgetNodeByIdlookup failed and threw.Changes
packages/core/src/api/blockManipulation/commands/moveBlocks/moveBlocks.ts: inmoveSelectedBlocksAndSelection, resolve the destination to a ProseMirror position beforeremoveBlocks, map it through any steps the removal adds, and insert at the mapped position via a directReplaceStepinstead of re-resolving thereferenceBlockID.packages/xl-multi-column/src/test/commands/moveBlocks.test.ts: two regression tests that fail without the fix and pass with it. Covers both the case from the issue (columnList followed by a paragraph) and the variant where the columnList is preceded by a sibling.Impact
No behavioral change for existing scenarios. All existing
moveBlockssnapshot tests still pass. Removing the ID-based re-lookup also slightly hardens the function against any future side effect that might destroy the destination block.Testing
packages/xl-multi-column/src/test/commands/moveBlocks.test.ts./basic/multi-column/: add a multi-column block, type text under it,Cmd+Shift+Up. No console error; the block moves out cleanly.Screenshots/Video
column-console-bug.mov
Checklist
(No documentation change applicable; this is an internal bug fix.)
Additional Notes
N/A
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests