fix: insert table outside nested list context#14840
Conversation
📝 WalkthroughWalkthroughModifies table insertion command to walk up the parent chain of list blocks before inserting, ensuring that tables are inserted at the outermost list ancestor rather than at the nested list level. This addresses a bug where secondary numbered lists couldn't accept table insertion. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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)
blocksuite/affine/blocks/table/src/commands.ts (1)
30-30: Consider schema-driven validation instead of hardcoding'affine:list'.Hardcoding the flavour couples this command to one specific invalid-parent case. The schema already provides
safeValidate()to check parent-child relationships. Replace the hardcoded check withwhile (parent && !std.store.schema.safeValidate('affine:table', parent.flavour))to walk up until a valid parent is found. This generalizes the fix, automatically handles new container flavours, and keeps the logic in sync with schema changes.Not a blocker for this PR — flagging as a follow-up refactor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocksuite/affine/blocks/table/src/commands.ts` at line 30, Replace the hardcoded parent.flavour check in the loop that walks up container parents (the while (parent && parent.flavour === 'affine:list') block) with a schema-driven validation using std.store.schema.safeValidate('affine:table', parent.flavour); specifically change the condition to continue while parent exists and safeValidate returns false so the loop climbs until a schema-validated parent for 'affine:table' is found, keeping the logic in sync with the schema and avoiding coupling to a single flavour string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocksuite/affine/blocks/table/src/commands.ts`:
- Around line 23-33: The code mutates targetModel during the parent-walk (using
std.store.getParent) which causes the later empty-line cleanup (the
removeEmptyLine check and std.store.deleteBlock call) to inspect the wrong
block; preserve the originally selected leaf model by saving it (e.g.,
originalTargetModel = targetModel or leafModel = selectedModels[0 or last])
before the while loop and continue to use the walked targetModel for insertion
logic while using the preserved originalTargetModel when evaluating
removeEmptyLine and calling std.store.deleteBlock; update references in the
cleanup branch to check originalTargetModel.text?.length and delete that block
if empty.
---
Nitpick comments:
In `@blocksuite/affine/blocks/table/src/commands.ts`:
- Line 30: Replace the hardcoded parent.flavour check in the loop that walks up
container parents (the while (parent && parent.flavour === 'affine:list') block)
with a schema-driven validation using
std.store.schema.safeValidate('affine:table', parent.flavour); specifically
change the condition to continue while parent exists and safeValidate returns
false so the loop climbs until a schema-validated parent for 'affine:table' is
found, keeping the logic in sync with the schema and avoiding coupling to a
single flavour string.
🪄 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: 65c1059a-cab7-4147-8dce-a7c425b78865
📒 Files selected for processing (1)
blocksuite/affine/blocks/table/src/commands.ts
| let targetModel = | ||
| place === 'before' | ||
| ? selectedModels[0] | ||
| : selectedModels[selectedModels.length - 1]; | ||
|
|
||
| if (!targetModel) return; | ||
|
|
||
| let parent = std.store.getParent(targetModel); | ||
| while (parent && parent.flavour === 'affine:list') { | ||
| targetModel = parent; | ||
| parent = std.store.getParent(targetModel); | ||
| } |
There was a problem hiding this comment.
Reassigning targetModel breaks the empty-line cleanup at lines 52–54.
targetModel is now mutated to point at the outermost affine:list ancestor, but the cleanup branch below still uses it to decide whether to remove "the empty line":
if (removeEmptyLine && targetModel.text?.length === 0) {
std.store.deleteBlock(targetModel);
}In the exact scenario this PR targets (slash menu invoked on an empty nested list item), targetModel after the walk is the top-level list item, not the empty leaf the user typed / on. That means either:
- the top-level list item happens to have empty text → the entire outer list item (and all its nested children) gets deleted, or
- it has non-empty text → the originally empty line the user invoked from is left behind, regressing the existing "remove empty slash-command line" UX.
Preserve the originally selected model for the cleanup decision.
🐛 Proposed fix
- let targetModel =
+ const originModel =
place === 'before'
? selectedModels[0]
: selectedModels[selectedModels.length - 1];
- if (!targetModel) return;
+ if (!originModel) return;
- let parent = std.store.getParent(targetModel);
+ let targetModel: BlockModel = originModel;
+ let parent = std.store.getParent(targetModel);
while (parent && parent.flavour === 'affine:list') {
targetModel = parent;
parent = std.store.getParent(targetModel);
}
@@
- if (removeEmptyLine && targetModel.text?.length === 0) {
- std.store.deleteBlock(targetModel);
+ if (removeEmptyLine && originModel.text?.length === 0) {
+ std.store.deleteBlock(originModel);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@blocksuite/affine/blocks/table/src/commands.ts` around lines 23 - 33, The
code mutates targetModel during the parent-walk (using std.store.getParent)
which causes the later empty-line cleanup (the removeEmptyLine check and
std.store.deleteBlock call) to inspect the wrong block; preserve the originally
selected leaf model by saving it (e.g., originalTargetModel = targetModel or
leafModel = selectedModels[0 or last]) before the while loop and continue to use
the walked targetModel for insertion logic while using the preserved
originalTargetModel when evaluating removeEmptyLine and calling
std.store.deleteBlock; update references in the cleanup branch to check
originalTargetModel.text?.length and delete that block if empty.
darkskygit
force-pushed
the
canary
branch
5 times, most recently
from
3403237 to
78a9942
Compare
1 participant
Fixes #14741
Issue
When attempting to insert a table inside a nested numbered list using
the slash menu, the table would fail to insert and nothing would happen.
Fix
insertTableBlockCommandusesaddSiblingBlockswhich tries to addthe table as a child of the
affine:listparent block — invalid fora table block
affine:list, so the table is inserted as a sibling of the outermostlist block instead
Summary by CodeRabbit
Release Notes