[Lexical][Tables] Follow up on #6267 handle multi column delete crash on forward selection at end of the table #39
Conversation
![local-dev-korbit-ai-mentor[bot]](https://avatars.githubusercontent.com/in/318591?s=60&v=4){kind=small}
…ion at end of the table
![[local-dev] Korbit - AI Mentor](https://avatars.githubusercontent.com/in/318591?s=40&u=4153787aed90096cc5d05c9c38a439007e524086&v=4){kind=avatar}
|
Clone of the PR facebook/lexical#6279 |
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
WalkthroughThis update enhances the table manipulation capabilities within the editor by introducing a new test case for deleting columns from a table. Additionally, it modifies the logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant TableUtils
User->>Editor: Selects table columns to delete
Editor->>TableUtils: Calls deleteTableColumns()
TableUtils->>TableUtils: Determine previousRow based on focus and anchor columns
TableUtils-->>Editor: Remove selected columns
Editor-->>User: Update table display
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
| const previousRow = | ||
| focusStartColumn < anchorStartColumn | ||
| ? focusRowMap[focusStartColumn - 1] | ||
| : focusRowMap[anchorStartColumn - 1]; |
There was a problem hiding this comment.
The current implementation for moving the selection after deleting columns only handles the case where the focus column is less than the anchor column. To fix this, we should update the logic to handle both cases: when the focus column is less than the anchor column and when it's greater or equal. This will ensure that the selection is moved correctly regardless of the deletion direction.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
| const previousRow = | ||
| focusStartColumn < anchorStartColumn | ||
| ? focusRowMap[focusStartColumn - 1] | ||
| : focusRowMap[anchorStartColumn - 1]; |
There was a problem hiding this comment.
The changes in the $deleteTableColumn__EXPERIMENTAL function do not include error handling for the case when focusRowMap[focusStartColumn - 1] or focusRowMap[anchorStartColumn - 1] is accessed with an out-of-bounds index. This could lead to a runtime error if focusStartColumn or anchorStartColumn is 0. Consider adding a check to ensure the indices are valid before accessing the array, and handle the case appropriately if they are out of bounds.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
|
/review |
PR Reviewer Guide 🔍(Review updated until commit 44f6e45)
|
github-actions
bot
added
Review effort [1-5]: 3
and removed
Review effort [1-5]: 2
labels
|
Persistent review updated to latest commit 44f6e45 |
|
@coderabbitai review |
Actions performedReview triggered.
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/lexical-playground/tests/e2e/Tables.spec.mjs (1 hunks)
- packages/lexical-table/src/LexicalTableUtils.ts (1 hunks)
Additional comments not posted (2)
packages/lexical-table/src/LexicalTableUtils.ts (1)
639-642: Enhance selection logic for column deletion.The current implementation only handles the case where
focusStartColumnis less thananchorStartColumn. Consider updating the logic to handle cases wherefocusStartColumnis greater than or equal toanchorStartColumnto ensure correct selection movement.Additionally, ensure that accessing
focusRowMap[focusStartColumn - 1]orfocusRowMap[anchorStartColumn - 1]does not result in an out-of-bounds error. Consider adding checks to validate indices before accessing the array.packages/lexical-playground/__tests__/e2e/Tables.spec.mjs (1)
2623-2670: New test case for forward column deletion looks good.The test case effectively sets up the scenario, performs the column deletion, and verifies the expected HTML structure. This addition enhances test coverage for table manipulation features.
|
Zero comments from coderabbit |
What
fix crash on forward column deletion at the end of the table
Why
currently backward multiple column deletion at the end of the table works, forward multiple column deletion crashes, add handling for the same.
Before
Screen.Recording.2024-06-09.at.1.47.39.PM.mov
After
Screen.Recording.2024-06-09.at.1.47.20.PM.mov
Description by Korbit AI
What change is being made?
Add a test case to handle multi-column deletion at the end of a table and update the
$deleteTableColumn__EXPERIMENTALfunction to correctly move the selection when deleting columns.Why are these changes being made?
These changes address a crash that occurs when deleting multiple columns with a forward selection at the end of a table. The test ensures the issue is fixed, and the code update corrects the selection movement logic to prevent the crash.
Summary by CodeRabbit
New Features
Improvements