fix: edge editor text invalid when configuring the arrowhead #4801
Conversation
Summary of ChangesHello @lxfu1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a bug where text entered into the edge editor might not be correctly saved or could become invalid under certain conditions, such as when interacting with arrowheads. The changes ensure that any modifications made within the cell editor are reliably captured and the editor is properly dismissed, whether the user clicks outside of it or simply moves their mouse away. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where text in the edge editor could be lost when configuring an arrowhead. The fix introduces a mouseleave event handler on the editor, ensuring that content is saved when the mouse moves away. This change is well-implemented and includes a good refactoring that extracts the cell update logic into a new updateCell method. This new method is used by both the new mouseleave handler and the existing mouseup handler, which improves code clarity and reduces duplication. My review includes a suggestion to add a null check in the new updateCell method to make it more robust and self-contained.
| updateCell() { | ||
| const value = this.editor.innerText.replace(/\n$/, '') || '' | ||
| // set value, when value is null, we will remove label in edge | ||
| this.setCellText(value !== '' ? value : null) | ||
| // remove tool | ||
| this.removeElement() | ||
| } |
There was a problem hiding this comment.
For better robustness and to make updateCell self-contained, it's a good practice to add a null check for this.editor at the beginning of the method. Although the current callers of updateCell already perform this check, adding it here will prevent potential null reference errors if the method is called from other places in the future without a check.
updateCell() {
if (!this.editor) {
return
}
const value = this.editor.innerText.replace(/\n$/, '') || ''
// set value, when value is null, we will remove label in edge
this.setCellText(value !== '' ? value : null)
// remove tool
this.removeElement()
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4801 +/- ##
==========================================
- Coverage 82.05% 82.04% -0.01%
==========================================
Files 327 327
Lines 37658 37665 +7
Branches 7597 7597
==========================================
+ Hits 30900 30902 +2
- Misses 6734 6739 +5
Partials 24 24
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Description
Motivation and Context
Types of changes
Self Check before Merge