feat: select table cell to set attributes #440
Conversation
🦋 Changeset detectedLatest commit: ccc359d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
Poem
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 (2)
packages/table-module/src/module/menu/TableProperty.ts (2)
9-9: Consider importing types for better type safetyThe import of
EDITOR_TO_SELECTIONsuggests it's a WeakMap. Consider importing its type for better type safety.-import { Editor, Transforms } from 'slate' +import { Editor, Transforms, NodeEntry } from 'slate' // ... other imports ... -import { EDITOR_TO_SELECTION } from '../weak-maps' +import type { EditorSelectionMap } from '../weak-maps' +import { EDITOR_TO_SELECTION } from '../weak-maps'Also applies to: 21-21
189-193: Optimize performance for bulk operationsThe nested forEach loops for applying properties to selected cells could be optimized.
Consider using a more efficient approach:
- selection.forEach(row => { - row.forEach(cell => { - Transforms.setNodes(editor, props, { at: cell[0][1] }) - }) - }) + // Batch update all selected cells at once + const cellPaths = selection.flat().map(cell => cell[0][1]) + Transforms.setNodes(editor, props, { at: cellPaths })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/table-module/src/module/menu/CellProperty.ts(1 hunks)packages/table-module/src/module/menu/TableProperty.ts(4 hunks)
🔇 Additional comments (3)
packages/table-module/src/module/menu/CellProperty.ts (1)
19-20: LGTM: Clean implementation of menu identifier
The addition of the menu property with value 'cell' is well-integrated with the parent TableProperty class's selection handling mechanism.
packages/table-module/src/module/menu/TableProperty.ts (2)
34-35: LGTM: Clear menu identifier implementation
The menu property addition provides a clear identifier for distinguishing between table and cell property operations.
186-196: Verify the selection state persistence
The code relies on EDITOR_TO_SELECTION to maintain selection state. We should verify its lifecycle management.
✅ Verification successful
Based on the search results, I can now generate the final response:
Selection state is properly managed through WeakMap lifecycle
The codebase shows proper lifecycle management of the EDITOR_TO_SELECTION WeakMap:
- Selection state is set in
withSelectionplugin when operations are applied - Selection is cleaned up via
delete()inTableCursor.clearSelection() - WeakMap usage ensures automatic cleanup when editor is garbage collected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for WeakMap usage and cleanup
rg -A 5 "EDITOR_TO_SELECTION"
# Look for selection cleanup on editor state changes
ast-grep --pattern 'EDITOR_TO_SELECTION.delete($_)'
Length of output: 12068
| const selection = EDITOR_TO_SELECTION.get(editor) | ||
|
|
||
| if (this.menu === 'cell' && !!selection?.length) { | ||
| selection.forEach(row => { | ||
| row.forEach(cell => { | ||
| Transforms.setNodes(editor, props, { at: cell[0][1] }) | ||
| }) | ||
| }) | ||
| } else { | ||
| Transforms.setNodes(editor, props, { at: path }) | ||
| } |
There was a problem hiding this comment.
Add error handling and type safety for selection operations
The selection handling code needs additional safety checks:
- Missing null/undefined check for EDITOR_TO_SELECTION.get(editor)
- No type validation for selection data structure
- No error handling for failed transformations
Consider implementing these safety measures:
- const selection = EDITOR_TO_SELECTION.get(editor)
+ const selection = EDITOR_TO_SELECTION.get(editor) ?? null
+
+ try {
if (this.menu === 'cell' && !!selection?.length) {
+ // Validate selection structure
+ if (!Array.isArray(selection) || !selection.every(row => Array.isArray(row))) {
+ throw new Error('Invalid selection structure')
+ }
+
selection.forEach(row => {
row.forEach(cell => {
+ if (!Array.isArray(cell?.[0]?.[1])) {
+ throw new Error('Invalid cell path')
+ }
Transforms.setNodes(editor, props, { at: cell[0][1] })
})
})
} else {
Transforms.setNodes(editor, props, { at: path })
}
+ } catch (error) {
+ console.error('Failed to apply table properties:', error)
+ // Consider showing user feedback
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const selection = EDITOR_TO_SELECTION.get(editor) | |
| if (this.menu === 'cell' && !!selection?.length) { | |
| selection.forEach(row => { | |
| row.forEach(cell => { | |
| Transforms.setNodes(editor, props, { at: cell[0][1] }) | |
| }) | |
| }) | |
| } else { | |
| Transforms.setNodes(editor, props, { at: path }) | |
| } | |
| const selection = EDITOR_TO_SELECTION.get(editor) ?? null | |
| try { | |
| if (this.menu === 'cell' && !!selection?.length) { | |
| // Validate selection structure | |
| if (!Array.isArray(selection) || !selection.every(row => Array.isArray(row))) { | |
| throw new Error('Invalid selection structure') | |
| } | |
| selection.forEach(row => { | |
| row.forEach(cell => { | |
| if (!Array.isArray(cell?.[0]?.[1])) { | |
| throw new Error('Invalid cell path') | |
| } | |
| Transforms.setNodes(editor, props, { at: cell[0][1] }) | |
| }) | |
| }) | |
| } else { | |
| Transforms.setNodes(editor, props, { at: path }) | |
| } | |
| } catch (error) { | |
| console.error('Failed to apply table properties:', error) | |
| // Consider showing user feedback | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.changeset/warm-garlics-wave.md (1)
5-5: Enhance the changeset descriptionThe current description is too brief. Consider adding:
- Detailed explanation of the new feature
- Usage examples
- Any changes to existing APIs
- Migration steps if applicable
Here's a suggested template:
feat: select table cell to set attributes This change introduces the ability to select table cells and modify their attributes: - Feature: [describe the main functionality] - Usage: [provide example code] - API Changes: [list any API modifications] - Migration: [if applicable]
| --- | ||
| "@wangeditor-next/table-module": patch | ||
| --- |
There was a problem hiding this comment.
Consider using "minor" instead of "patch" version bump
According to semantic versioning (semver), new features should increment the minor version, while patch versions are reserved for backwards-compatible bug fixes. Since this change introduces a new feature for table cell attribute selection, it should use a "minor" bump instead.
Apply this diff to fix the version bump:
---
-"@wangeditor-next/table-module": patch
+"@wangeditor-next/table-module": minor
---📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --- | |
| "@wangeditor-next/table-module": patch | |
| --- | |
| --- | |
| "@wangeditor-next/table-module": minor | |
| --- |
Changes Overview
Implementation Approach
Testing Done
Verification Steps
Additional Notes
Checklist
Related Issues
Close #438
Summary by CodeRabbit
New Features
menuproperty to theCellPropertyclass, enhancing its functionality with a specific identifier.menuproperty to theTablePropertyclass, providing a distinct identifier for table-related actions.Bug Fixes
isDisabledmethod for better performance regarding table node selection.Improvements
getModalContentElemmethod to improve how properties are set based on selected cells in the editor.