-
Notifications
You must be signed in to change notification settings - Fork 7
Reload global function #345
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
base: main
Are you sure you want to change the base?
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello @adityamittal3107, 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 new global function, 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 introduces a new global function, reloadIframe, which wraps an internal reload utility to allow consumers of the SDK to reload a ThoughtSpot iframe. The change is straightforward and useful. My feedback includes a suggestion to make the new function more robust by handling cases where an invalid iframe element is passed, and a recommendation to add unit tests to ensure the feature is reliable and maintainable.
| /** | ||
| * Reloads the ThoughtSpot iframe. | ||
| * @param iFrame | ||
| * @group Global methods | ||
| * @version SDK: 1.43.1 | ||
| */ | ||
| export const reloadIframe = (iFrame: HTMLIFrameElement) => { | ||
| reload(iFrame); | ||
| }; |
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.
The new reloadIframe function is a valuable addition to the public API. However, it's crucial to add unit tests for it in src/embed/base.spec.ts to ensure its functionality is verified and to prevent future regressions. Please add tests that cover the successful reload scenario and also how it behaves when an invalid iFrame is passed.
| export const reloadIframe = (iFrame: HTMLIFrameElement) => { | ||
| reload(iFrame); | ||
| }; |
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.
For robustness in this public-facing SDK function, it's good practice to add a check to ensure the iFrame argument is not null or undefined before attempting to use it. This prevents potential runtime errors if the function is called with an invalid argument, especially from JavaScript code.
| export const reloadIframe = (iFrame: HTMLIFrameElement) => { | |
| reload(iFrame); | |
| }; | |
| export const reloadIframe = (iFrame: HTMLIFrameElement) => { | |
| if (!iFrame) { | |
| logger.warn('reloadIframe called with no iFrame element.'); | |
| return; | |
| } | |
| reload(iFrame); | |
| }; |
1965c9c to
2488dd0
Compare
commit: |
77e4ad5 to
720da9d
Compare
720da9d to
b18a6e2
Compare
|
SonarQube Quality Gate
|








No description provided.