-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: tooltip when shown and next not enable, hide tooltip #7462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @XiaoMing-xmg666, 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 addresses a bug within the tooltip plugin where tooltips could remain visible even after the conditions for their display were no longer met. The update introduces a check within the tooltip's 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses an issue where the tooltip would remain visible even when it should be disabled. The change correctly hides the tooltip when the isEnable condition is not met. However, I've identified a critical issue where the new logic could cause a runtime crash. This can happen when the tooltip is shown programmatically (e.g., via showById) because the event object would lack a client property, which the hide method expects. I've provided a code suggestion to prevent this potential crash.
| if (!this.tooltipElement) return; | ||
| // if shown, when is not enable, hide | ||
| if (!this.isEnable(event, items)) { | ||
| this.hide(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling this.hide(event) here can cause a runtime error. The event object might not have a client property, for example when show() is called via showById(). The hide() method unconditionally destructures event.client, which will throw an exception if client is undefined.
To fix this, we can conditionally pass the event to hide() only when event.client exists. If it doesn't, calling hide() with undefined will trigger the safe path within the hide method that does not access the client property.
| this.hide(event); | |
| this.hide(event.client ? event : undefined); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v5 #7462 +/- ##
==========================================
+ Coverage 95.57% 95.60% +0.03%
==========================================
Files 188 188
Lines 9871 9874 +3
Branches 2082 2131 +49
==========================================
+ Hits 9434 9440 +6
- Misses 402 434 +32
+ Partials 35 0 -35
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Fix #7362
