SCAL-270176 : don't pre render if already present on screen #300
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @sastaachar, 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 significant optimization to the embedding mechanism by preventing unnecessary pre-rendering of content that is already displayed or initialized. It enhances performance and user experience by avoiding redundant operations and provides a new option to explicitly control when existing pre-rendered content should be replaced.
Highlights
- Optimized Pre-rendering Logic: The core
preRendermethod now intelligently checks if content is already rendered. If it is, and thereplaceExistingPreRenderflag is not set to true, the method will return early, preventing redundant pre-rendering and improving performance. - New
replaceExistingPreRenderOption: A new boolean parameter,replaceExistingPreRender, has been added to thepreRendermethod. This allows callers to explicitly force a re-render even if content is already present, providing more control over the embedding process. - Initial Hidden State for Pre-rendered Components: React components that are pre-rendered will now initially have their display style set to
none. This ensures a smoother visual experience by preventing incomplete or flickering content from being briefly visible before the full embed is ready. - Enhanced Test Coverage: New test cases have been added to
liveboard.spec.tsto cover the newreplaceExistingPreRenderfunctionality and to properly mock session information and authentication events, ensuring the robustness of the changes.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces an optimization to avoid re-rendering components that are already pre-rendered, and adds a mechanism to explicitly replace them if needed. The core logic changes in ts-embed.ts are sound and well-implemented. The accompanying test updates in liveboard.spec.ts are thorough, including a new test case for the replacement functionality. I've added a couple of suggestions to improve the test suite's maintainability and reliability by reducing code duplication and replacing fixed timeouts with fake timers.
src/embed/liveboard.spec.ts
Outdated
| // mock getSessionInfo | ||
| jest.spyOn(SessionInfoService, 'getSessionInfo').mockResolvedValue({ | ||
| releaseVersion: '1.0.0', | ||
| userGUID: '1234567890', | ||
| currentOrgId: 1, | ||
| privileges: [], | ||
| mixpanelToken: '1234567890', | ||
| }); |
There was a problem hiding this comment.
This mock setup for SessionInfoService.getSessionInfo is duplicated in several tests within this file. To improve maintainability and reduce boilerplate code, consider extracting this mock into a reusable helper function.
There was a problem hiding this comment.
Please take care of this
| executeAfterWait(() => { | ||
| liveboardEmbed.navigateToLiveboard('lb1', 'viz1'); | ||
| expect(onSpy).toHaveBeenCalledWith(HostEvent.Navigate, 'embed/viz/lb1/viz1'); | ||
| done(); | ||
| }); | ||
| }, 1002); |
There was a problem hiding this comment.
Using a fixed timeout like 1002 can make tests slow and potentially flaky, as they depend on real-time execution. It's better to use fake timers. You can call jest.useFakeTimers() at the top of your describe block and then use jest.advanceTimersByTime() to control the passage of time deterministically. This makes tests faster and more reliable.
There was a problem hiding this comment.
this would require wider rework , i tried using faketimers at this point we are using the execute after wait
commit: |
|
SonarQube Quality Gate
|
3 participants
No description provided.