[lexical-history][lexical-selection][lexical-react] Fix: #6409 TextNode change detection #26
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
Add delay option to HistoryPlugin Add test for #6409 Fix lint issues Fix $cloneWithProperties and test expectations Targeted fix for TextNode leaf change
|
Clone of the PR facebook/lexical#6420 |
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update enhances the Lexical codebase by streamlining node cloning processes, improving type definitions, and refining history management functionalities. Key additions include the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant History
User->>Editor: Make changes to text node
Editor->>History: Log changes
History-->>Editor: Update history state
User->>Editor: Undo changes
Editor->>History: Fetch previous state
History-->>Editor: Restore text node
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
| return ( | ||
| prevNode.__type === nextNode.__type && | ||
| prevNode.__text === nextNode.__text && | ||
| prevNode.__mode === nextNode.__mode && | ||
| prevNode.__detail === nextNode.__detail && | ||
| prevNode.__style === nextNode.__style && | ||
| prevNode.__format === nextNode.__format && | ||
| prevNode.__parent === nextNode.__parent | ||
| JSON.stringify(prevEditorState.read(() => prevNode.exportJSON())) === | ||
| JSON.stringify(nextEditorState.read(() => nextNode.exportJSON())) | ||
| ); |
There was a problem hiding this comment.
The current implementation of isTextNodeUnchanged uses JSON.stringify and exportJSON to compare text nodes. This approach might miss some subtle changes or introduce unnecessary comparisons. Consider implementing a more direct comparison of the relevant properties of the text nodes, such as __text, __mode, __format, and __style. This will ensure more accurate change detection and potentially improve performance.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
| export function HistoryPlugin({ | ||
| delay, | ||
| externalHistoryState, | ||
| }: { | ||
| delay?: number; | ||
| externalHistoryState?: HistoryState; | ||
| }): null { | ||
| const [editor] = useLexicalComposerContext(); | ||
|
|
||
| useHistory(editor, externalHistoryState); | ||
| useHistory(editor, externalHistoryState, delay); | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
The new delay parameter has been added to the HistoryPlugin function and passed to the useHistory hook. However, there's no documentation or explanation about what this parameter does or how it affects the plugin's behavior. Could you please add a comment or JSDoc explaining the purpose of the delay parameter and how it's used in the history functionality? This will help other developers understand how to use this new option correctly.
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 cc5b270)
|
|
Persistent review updated to latest commit cc5b270 |
|
@coderabbitai full review |
codiumai-pr-agent-free
bot
added
enhancement
Actions performedFull review triggered. |
|
Persistent review updated to latest commit cc5b270 |
PR Code Suggestions ✨
|
![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
Outside diff range, codebase verification and nitpick comments (2)
packages/lexical-history/src/__tests__/unit/LexicalHistory.test.tsx (1)
382-439: Consider adding comments to the test case.Adding comments to the test case for
Changes to TextNode leaf are detected properly #6409would improve readability and understanding of the test logic.+ // Initialize the editor with CustomTextNode editor = createTestEditor({ nodes: [CustomTextNode], });packages/lexical-clipboard/src/clipboard.ts (1)
270-274: Acknowledgment: Grammatical correction in comments.The correction from "it's" to "its" improves the clarity of the documentation.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- packages/lexical-clipboard/src/clipboard.ts (2 hunks)
- packages/lexical-history/src/tests/unit/LexicalHistory.test.tsx (4 hunks)
- packages/lexical-history/src/index.ts (1 hunks)
- packages/lexical-html/src/index.ts (1 hunks)
- packages/lexical-react/src/LexicalContentEditable.tsx (2 hunks)
- packages/lexical-react/src/LexicalHistoryPlugin.ts (1 hunks)
- packages/lexical-selection/src/lexical-node.ts (2 hunks)
- packages/lexical-website/src/components/Gallery/utils.tsx (1 hunks)
Additional comments not posted (11)
packages/lexical-react/src/LexicalHistoryPlugin.ts (1)
20-28: LGTM!The addition of the
delayparameter to theHistoryPluginfunction is well-integrated with the existinguseHistoryhook.packages/lexical-website/src/components/Gallery/utils.tsx (1)
54-54: LGTM!The inclusion of
tagsin the dependency array ofuseFilteredExamplesenhances the reactivity of the filtering logic.packages/lexical-react/src/LexicalContentEditable.tsx (1)
Line range hint
16-41: LGTM!The removal of the deprecated
editor__DEPRECATEDproperty and ESLint disable comments enhances the maintainability and clarity of the code.packages/lexical-html/src/index.ts (1)
106-106: LGTM!The removal of the explicit type parameter
<LexicalNode>from the$cloneWithPropertiescall enhances type inference and flexibility.packages/lexical-history/src/__tests__/unit/LexicalHistory.test.tsx (1)
46-104: LGTM!The introduction of the
CustomTextNodeclass and related utility functions enhances the flexibility and usability of text node management.packages/lexical-history/src/index.ts (1)
213-216: Consider direct property comparison for performance.The existing comment suggests using a more direct comparison of relevant properties for performance improvements.
packages/lexical-clipboard/src/clipboard.ts (1)
259-259: Verify the impact of removing the generic type parameter.The removal of the generic type parameter from
$cloneWithPropertiesmay increase flexibility but could affect type safety. Ensure this change aligns with the intended use and doesn't introduce issues.Run the following script to verify the usage of
$cloneWithPropertiesand ensure type safety is maintained:packages/lexical-selection/src/lexical-node.ts (4)
39-39: LGTM: Return type change tovoid.The change to a
voidreturn type emphasizes in-place updates, aligning with the procedural nature of the function.
51-51: LGTM: Return type change tovoid.The change to a
voidreturn type emphasizes in-place updates, aligning with the procedural nature of the function.
61-61: LGTM: Return type change tovoid.The change to a
voidreturn type emphasizes in-place updates, aligning with the procedural nature of the function.
80-85: LGTM: Streamlined logic in$cloneWithProperties.The streamlined logic enhances clarity by directly applying updates and returning the clone, aligning with the intended procedural updates.
|
Zero comments from coderabbit |
User description
Description
TextNodechange detection when only one text leaf is dirty but unchanged$cloneWithProperties- the previous implementation did not correctly handle__textFormatfromParagraphNode. Also updated the very incorrect doc string.$cloneWithPropertiesto the lexical package because the correct implementation was already inlined inLexicalNode.getWritable()A more general fix may be possible, it's unclear whether this hack is really necessary for MaxLengthPlugin but other text node transforms may have similar issues so removing the hack could be backwards incompatible.
Closes #6409
Test plan
Before
History's TextNode change detection did not work correctly for subclasses where only properties that did not exist on TextNode changed.
After
The tests show that the behavior is consistent with expectations
Description by Korbit AI
Note
This feature is in early access. You can enable or disable it in the Korbit Console.
What change is being made?
Fix text node change detection by updating the
isTextNodeUnchangedfunction, adding a newCustomTextNodeclass, and modifying related test cases and utility functions.Why are these changes being made?
These changes address issue #6409 by ensuring that changes to text nodes, including text content and class modifications, are properly detected and reflected in the undo stack. This improves the accuracy of the editor's history tracking and ensures consistent behavior when text nodes are updated.
PR Type
Bug fix, Enhancement, Tests
Description
isTextNodeUnchangedfunction.CustomTextNodeclass and related tests to ensure proper change detection.HistoryPluginwith a delay option for better control.Changes walkthrough 📝
clipboard.ts
Fix typos and update node cloning logicpackages/lexical-clipboard/src/clipboard.ts
index.ts
Refine text node unchanged detection logicpackages/lexical-history/src/index.ts
isTextNodeUnchangedfunction logic.index.ts
Update node cloning logicpackages/lexical-html/src/index.ts
utils.tsx
Add tags dependency to useFilteredExamplespackages/lexical-website/src/components/Gallery/utils.tsx
tagsdependency touseFilteredExamples.LexicalHistory.test.tsx
Add tests for CustomTextNode change detectionpackages/lexical-history/src/tests/unit/LexicalHistory.test.tsx
CustomTextNodeclass for testing.CustomTextNode.LexicalContentEditable.tsx
Remove unnecessary TypeScript lint commentspackages/lexical-react/src/LexicalContentEditable.tsx
LexicalHistoryPlugin.ts
Add delay option to HistoryPluginpackages/lexical-react/src/LexicalHistoryPlugin.ts
HistoryPlugin.lexical-node.ts
Improve node cloning and property update logicpackages/lexical-selection/src/lexical-node.ts
$cloneWithPropertiesto improve node cloning.Summary by CodeRabbit
New Features
CustomTextNodefor enhanced text management with custom CSS classes.delayparameter to theHistoryPluginfor improved control over history state updates.Improvements
useFilteredExamplesfunction to react to changes intags, improving filtering accuracy in the Gallery component.Bug Fixes
Refactor