[WIKI-544] chore: smoother insert handles for tables #7398
Conversation
aaryan610
added
🌟enhancement
WalkthroughThe update refactors the drag-based logic for inserting and removing columns or rows in the table editor extension. It replaces incremental tracking with a system that recalculates the target number of columns or rows based on total drag displacement, synchronizing the table size directly to the drag distance. No changes to public APIs were made. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableEditor
User->>TableEditor: Mouse down on insert handle
TableEditor->>TableEditor: Record original column/row count and mouse position
loop While dragging
User->>TableEditor: Drag handle
TableEditor->>TableEditor: Calculate drag displacement
TableEditor->>TableEditor: Compute target count (based on drag distance and threshold)
TableEditor->>TableEditor: Add/remove columns/rows to match target count
end
User->>TableEditor: Mouse up
alt No drag occurred
TableEditor->>TableEditor: Insert single column/row
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/editor/src/core/extensions/table/plugins/insert-handlers/utils.ts (1)
205-219: Same performance consideration applies to row operations.Similar to the column logic, sequential row operations could benefit from batching for better performance with large displacements.
The same batching optimization suggested for columns (lines 91-105) would apply here for row operations.
🧹 Nitpick comments (4)
packages/editor/src/core/extensions/table/plugins/insert-handlers/utils.ts (4)
34-37: Consider the implications of persistent state variables.The variables
columnsAddedandoriginalColumnCountpersist across mouse operations and are only reset on new drag start. If the table is modified externally (e.g., through other editor commands), these counters could become out of sync with the actual table state.Consider refreshing these values more frequently or adding validation:
let columnsAdded = 0; let originalColumnCount = 0; // Track original column count at drag start +let lastKnownColumnCount = 0; // For validation
40-40: Add early return for non-left mouse button clicks.While the function is properly guarded later, adding an early return improves readability and performance.
- if (e.button !== 0) return; + if (e.button !== 0) return; // Only handle left mouse button
91-105: Consider performance implications of sequential operations.The while loops for adding/removing columns perform operations sequentially. For large displacement distances, this could result in many individual editor transactions.
Consider batching operations into a single transaction:
- // Add columns if needed - while (columnsAdded < targetColumns) { - insertColumnAfterLast(editor, tableInfo); - columnsAdded++; - } - - // Remove columns if needed - while (columnsAdded > targetColumns) { - const deleted = removeLastColumn(editor, tableInfo); - if (deleted) { - columnsAdded--; - } else { - break; - } - } + // Batch add/remove operations + const columnDiff = targetColumns - columnsAdded; + if (columnDiff > 0) { + // Add multiple columns in one transaction + for (let i = 0; i < columnDiff; i++) { + insertColumnAfterLast(editor, tableInfo); + } + columnsAdded = targetColumns; + } else if (columnDiff < 0) { + // Remove multiple columns, checking each one + let actuallyRemoved = 0; + for (let i = 0; i < Math.abs(columnDiff); i++) { + if (removeLastColumn(editor, tableInfo)) { + actuallyRemoved++; + } else { + break; + } + } + columnsAdded -= actuallyRemoved; + }
124-125: Document the rationale for persistent state variables.The comments explain that variables aren't reset on mouse up, but it would be helpful to explain why this design choice was made and how it improves the user experience.
Consider expanding the comment:
- // Don't reset columnsAdded and originalColumnCount here - they'll be reset on next drag + // Don't reset columnsAdded and originalColumnCount here - preserving state + // allows for smoother continuous operations and they'll be reset on next drag startAlso applies to: 238-239
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/src/core/extensions/table/plugins/insert-handlers/utils.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/editor/src/core/extensions/table/plugins/insert-handlers/utils.ts (3)
71-89: Well-implemented displacement-based calculation with proper hysteresis.The logic correctly calculates target columns using
ACTION_THRESHOLD / 2as an offset, providing smooth hysteresis behavior. The minimum column constraint is properly enforced.
148-151: Note the different ACTION_THRESHOLD values between columns and rows.Columns use
ACTION_THRESHOLD = 150while rows useACTION_THRESHOLD = 40. This significant difference should be intentional due to different UI spacing requirements.Verify that the different ACTION_THRESHOLD values (150 for columns, 40 for rows) are intentional and provide the desired user experience.
185-203: Consistent implementation with column logic.The row insertion logic follows the same well-designed pattern as the column logic, with proper hysteresis and minimum constraints.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/editor/src/core/extensions/table/plugins/insert-handlers/utils.ts (3)
221-240: Review the action threshold values for user experience.The configuration uses different action thresholds (150px for columns, 40px for rows). Consider whether these values provide optimal user experience across different screen sizes and input devices.
Consider making these thresholds configurable or responsive to the table size:
+const getActionThreshold = (direction: InsertDirection, tableSize: number) => { + const base = direction === "column" ? 150 : 40; + return Math.max(base, tableSize * 0.1); // Adaptive to table size +};
341-362: Mathematical logic is correct but could be simplified.The calculation correctly determines target items based on crossing threshold midpoints. However, the logic could be more readable.
Consider simplifying the calculation:
const calculateTargetItems = (delta: number, originalCount: number, actionThreshold: number): number => { - let targetItems = originalCount; - - if (delta > 0) { - // Moving in positive direction - add items - let itemsToAdd = 0; - while (itemsToAdd * actionThreshold + actionThreshold / 2 <= delta) { - itemsToAdd++; - } - targetItems = originalCount + itemsToAdd; - } else if (delta < 0) { - // Moving in negative direction - remove items - const distance = Math.abs(delta); - let itemsToRemove = 0; - while (itemsToRemove * actionThreshold + actionThreshold / 2 <= distance) { - itemsToRemove++; - } - targetItems = Math.max(1, originalCount - itemsToRemove); - } - - return targetItems; + // Calculate how many threshold segments we've crossed + const thresholdsCrossed = Math.floor((Math.abs(delta) + actionThreshold / 2) / actionThreshold); + + if (delta > 0) { + return originalCount + thresholdsCrossed; + } else if (delta < 0) { + return Math.max(1, originalCount - thresholdsCrossed); + } + + return originalCount; };
383-383: Apply static analysis suggestions for optional chaining.The static analysis correctly identifies opportunities to use optional chaining for cleaner, safer code.
Apply the suggested optional chaining improvements:
- while (domTable && domTable.parentNode && domTable.nodeType !== Node.ELEMENT_NODE) { + while (domTable?.parentNode && domTable.nodeType !== Node.ELEMENT_NODE) { domTable = domTable.parentNode; } - while (domTable && domTable.parentNode && (domTable as HTMLElement).tagName !== "TABLE") { + while (domTable?.parentNode && (domTable as HTMLElement).tagName !== "TABLE") { domTable = domTable.parentNode; }Also applies to: 387-387
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/src/core/extensions/table/plugins/insert-handlers/utils.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/editor/src/core/extensions/table/plugins/insert-handlers/utils.ts
[error] 383-383: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 387-387: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/editor/src/core/extensions/table/plugins/insert-handlers/utils.ts (3)
125-175: LGTM: Helper functions correctly reorganized.The helper functions
isCellEmpty,isColumnEmpty, andisRowEmptymaintain their original logic while being reorganized for better code structure. The implementation correctly checks for empty content in table cells, columns, and rows.
177-218: Well-designed type system for the refactored approach.The new interfaces and types provide a clean, generic foundation for the unified insert button implementation. The separation of concerns between configuration, state, and handlers is well-structured.
364-368: Clean delegation to the unified function.The export functions correctly delegate to the new generic
createInsertButtonfunction, maintaining backward compatibility while leveraging the refactored implementation.
| if (dragState.isDragging) { | ||
| const targetItems = calculateTargetItems(delta, dragState.originalItemCount, config.actionThreshold); | ||
|
|
||
| // Add items if needed | ||
| while (dragState.itemsAdded < targetItems) { | ||
| handlers.insertItem(editor, tableInfo); | ||
| dragState.itemsAdded++; | ||
| } | ||
|
|
||
| // Remove items if needed | ||
| while (dragState.itemsAdded > targetItems) { | ||
| const deleted = handlers.removeItem(editor, tableInfo); | ||
| if (deleted) { | ||
| dragState.itemsAdded--; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential performance concern with synchronous while loops.
The while loops that add/remove items execute synchronously during mouse movement, which could cause UI blocking for large operations. Consider debouncing or limiting the number of operations per frame.
Add throttling to prevent excessive operations:
+let lastUpdate = 0;
+const UPDATE_THROTTLE = 16; // ~60fps
if (dragState.isDragging) {
+ const now = Date.now();
+ if (now - lastUpdate < UPDATE_THROTTLE) return;
+ lastUpdate = now;
+
const targetItems = calculateTargetItems(delta, dragState.originalItemCount, config.actionThreshold);
// Add items if needed
- while (dragState.itemsAdded < targetItems) {
+ const maxOperationsPerFrame = 5;
+ let operations = 0;
+ while (dragState.itemsAdded < targetItems && operations < maxOperationsPerFrame) {
handlers.insertItem(editor, tableInfo);
dragState.itemsAdded++;
+ operations++;
}
// Remove items if needed
- while (dragState.itemsAdded > targetItems) {
+ while (dragState.itemsAdded > targetItems && operations < maxOperationsPerFrame) {
const deleted = handlers.removeItem(editor, tableInfo);
if (deleted) {
dragState.itemsAdded--;
+ operations++;
} else {
break;
}
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/editor/src/core/extensions/table/plugins/insert-handlers/utils.ts
around lines 296 to 314, the synchronous while loops for adding and removing
items during dragging can block the UI on large operations. To fix this,
implement throttling or debouncing to limit the number of insertItem and
removeItem calls per animation frame or time interval, ensuring the UI remains
responsive during drag events.
There was a problem hiding this comment.
can we also add touch event support so that this works in mobile as well? touchstart, touchmove,etc
Description
This PR improves the table insert handles by making the creation/removal flow smoother.
Type of Change
Summary by CodeRabbit