fix(table): fix merge row show error #435
Conversation
WalkthroughThe pull request introduces a new private method Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/table-module/src/module/menu/MergeCell.ts (1)
21-30: Consider extracting table structure validation logicThe
needKeepCellmethod currently handles both table structure validation and cell visibility decisions. Consider extracting the table structure validation into a separate service or utility class for better separation of concerns and easier testing.This would allow:
- Reuse of table structure validation logic across different operations
- Easier unit testing of validation rules
- Better maintainability as table structure rules evolve
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/table-module/src/module/menu/MergeCell.ts(2 hunks)
🔇 Additional comments (1)
packages/table-module/src/module/menu/MergeCell.ts (1)
125-128: Verify merge behavior with different table configurations
The implementation looks correct, but we should verify the merge behavior in various scenarios:
- Tables with different sizes
- Merging cells across multiple rows/columns
- Edge cases with already merged cells
Let's analyze the usage patterns:
✅ Verification successful
The previous search had issues with the file type. Let's try a more comprehensive search with specific file patterns and context to understand the merge cell implementation and its test coverage.
Let's analyze the implementation details of the merge functionality and related code to better understand the behavior and type safety.
We need to examine the complete implementation and type definitions to understand how the merge functionality handles different table configurations and cell states.
Implementation verified for table cell merging
The implementation is correct and handles various table configurations safely:
- The
needKeepCellmethod properly checks both row and column context:- Verifies sibling cells in the same row
- Checks for other rows in the parent table
- The merge operation is type-safe with proper
TableCellElementinterface - The hidden property is consistently used across the codebase:
- In merge operations
- In HTML parsing
- In test cases
- The implementation includes proper validation through
canMergemethod that prevents invalid merges:- Empty selections
- Cross-section merges (thead/tbody/tfoot)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases or example usage of the merge functionality
rg -l "MergeCell.*merge" --type ts
# Look for existing tests related to cell merging
rg -l "describe.*merge.*cell" --type test
Length of output: 109
Script:
#!/bin/bash
# Search for test files with different extensions
rg -l "describe.*merge.*cell" -g "*.test.*" -g "*.spec.*"
# Look for merge cell implementations and usages
rg "MergeCell" -A 5 --type ts
# Search for test scenarios or documentation mentioning merge cells
rg "merge.*cell" -i -A 3 --type md
Length of output: 3129
Script:
#!/bin/bash
# Let's examine the implementation details of the merge functionality
ast-grep --pattern 'class MergeCell {
$$$
needKeepCell($$$) {
$$$
}
$$$
}'
# Look for related split cell implementation for comparison
rg "class SplitCell" -A 10 --type ts
# Search for table cell element type definitions
ast-grep --pattern 'interface TableCellElement {
$$$
}'
Length of output: 986
Script:
#!/bin/bash
# Let's examine the full implementation of MergeCell class
rg "class MergeCell" -A 50 --type ts
# Look for table cell type definitions in different locations
rg "type.*TableCell.*=|interface.*TableCell" -A 5 --type ts
# Search for usage of hidden property in table cells
rg "hidden.*:.*true" -A 3 --type ts
Length of output: 5926
| private needKeepCell(editor: Editor, trPath: Path): boolean { | ||
| // 检查同行是否有其他单元格 | ||
| const [, rowSibling] = Node.children(editor, trPath) | ||
|
|
||
| // 检查同列是否有其他单元格 | ||
| const parentTable = Editor.parent(editor, trPath) | ||
| const hasOtherRows = parentTable[0].children.length > 1 | ||
|
|
||
| return !!rowSibling || hasOtherRows | ||
| } |
There was a problem hiding this comment.
Fix misleading comment and potential edge cases
Several issues need attention in this method:
- The comment on line 25 is incorrect - it's checking for other rows, not columns
- The row sibling check doesn't handle potential errors from
Node.children - The row count check includes the current row, which might lead to incorrect behavior
Consider this improved implementation:
private needKeepCell(editor: Editor, trPath: Path): boolean {
// 检查同行是否有其他单元格
- const [, rowSibling] = Node.children(editor, trPath)
+ let hasRowSibling = false
+ try {
+ const [, rowSibling] = Node.children(editor, trPath)
+ hasRowSibling = !!rowSibling
+ } catch (e) {
+ // Handle case where row has no children
+ }
- // 检查同列是否有其他单元格
+ // 检查是否有其他行
const parentTable = Editor.parent(editor, trPath)
- const hasOtherRows = parentTable[0].children.length > 1
+ const currentRowIndex = trPath[trPath.length - 1]
+ const hasOtherRows = parentTable[0].children.length > 1 &&
+ (currentRowIndex < parentTable[0].children.length - 1)
- return !!rowSibling || hasOtherRows
+ return hasRowSibling || hasOtherRows
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/strong-laws-walk.md (1)
1-6: Please provide a more detailed changeset description.The current description "fix table merge row show error" is too brief. Since this is a bug fix affecting multiple packages, please include:
- A clear description of the bug that was fixed
- How it was fixed (mention the new
needKeepCellmethod and changes to merge logic)- The relationship between the table-module and editor package changes
- Any potential migration steps or breaking changes
This will help users understand the impact of these changes when reading the changelog.
Example format:
--- '@wangeditor-next/table-module': patch '@wangeditor-next/editor': patch --- fix: table merge row display error - Fixed an issue where merged table rows were not displaying correctly - Added new cell retention logic via `needKeepCell` method - Refactored merge operation to improve reliability - No breaking changes or migration steps required
Changes Overview
Implementation Approach
Testing Done
Verification Steps
Additional Notes
Checklist
Related Issues
Summary by CodeRabbit