#39401 make closeContextMenu optional#104
#39401 make closeContextMenu optional#104daniela-mateescu wants to merge 3 commits intomaster-flower-platformfrom
closeContextMenu optional#104Conversation
Summary of ChangesHello @daniela-mateescu, 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 introduces a minor but significant change to the 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.
Code Review
This pull request makes the closeContextMenu function optional on the IActionParamForRun interface. This is a significant change as it introduces a breaking change for any consumers of the library who have implemented actions that rely on this function. The function was previously guaranteed to exist, but now it may be undefined, which could lead to runtime errors in existing code. It's crucial that this breaking change is clearly communicated and that consumers are guided on how to adapt their code.
|
|
||
| export interface IActionParamForRun extends IActionParam { | ||
| closeContextMenu: () => void, | ||
| closeContextMenu?: () => void, |
There was a problem hiding this comment.
Making closeContextMenu optional is a breaking change for consumers of this library. Any action implementation that calls params.closeContextMenu() will now fail if the function is not provided. Previously, its presence was guaranteed by the type definition.
All call sites within consumer code will need to be updated to check for the existence of closeContextMenu before calling it, for example:
if (params.closeContextMenu) {
params.closeContextMenu();
}or using optional chaining:
params.closeContextMenu?.();If this breaking change is intentional, it should be clearly documented in the pull request description and reflected in the package's versioning (e.g., a major version bump).
An alternative to making it optional, which would not be a breaking change, is to ensure it's always provided, even if it's a no-op function (() => {}) in contexts where there's no menu to close. This seems to align with the suggestion in the TODO comment on line 5 of this file.
There was a problem hiding this comment.
This is always populated internally in ContextMenu before passed to handlers. I have added a comment on top of this property
Proposed Change:
What is your change
Change type
Put an
xin the boxes that applyChecklist