SCAL-255929 - Added cleanup event on destroy #338
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 @jbhanu-thoughtspot, 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 refines the embed destruction process by introducing more control over how embedded components are cleaned up. It allows developers to configure whether the system should wait for a cleanup event to complete before removing the embed's DOM element, providing a more robust and asynchronous cleanup mechanism. This ensures that all necessary internal resources can be properly released, improving the stability and lifecycle management of embedded applications. 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 introduces a cleanup mechanism on embed destruction. I've found a critical issue in the timeout implementation for the cleanup, which acts as a delay instead and can lead to resource leaks. I've also identified a couple of medium-severity issues: a discrepancy in the default timeout value between the implementation and documentation, and an inconsistent naming for the new DestroyEmbed host event. I've provided suggestions to address these points.
| } else { | ||
| const cleanupTimeout = getEmbedConfig().cleanupTimeout; | ||
| setTimeout(() => { | ||
| this.trigger(HostEvent.DestroyEmbed).then(() => { | ||
| this.insertedDomEl?.parentNode?.removeChild(this.insertedDomEl); | ||
| }).catch((e) => { | ||
| logger.log('Error destroying TS Embed', e); | ||
| }); | ||
| }, cleanupTimeout); | ||
| } |
There was a problem hiding this comment.
The current implementation for handling waitForCleanupOnDestroy has a couple of issues:
setTimeoutis used to introduce a delay before triggering the cleanup, rather than acting as a timeout for the cleanup operation. The namecleanupTimeoutsuggests it should be a timeout.- The DOM element
this.insertedDomElis only removed if thethis.trigger(HostEvent.DestroyEmbed)promise resolves successfully. If it rejects or never resolves, the DOM element will leak.
I suggest refactoring this to use Promise.race to correctly implement the timeout and ensure the DOM element is always removed using .finally().
} else {
const timeout = getEmbedConfig().cleanupTimeout;
const cleanupWithTimeout = Promise.race([
this.trigger(HostEvent.DestroyEmbed),
new Promise(resolve => setTimeout(() => {
logger.warn(`Cleanup on destroy timed out after ${timeout}ms.`);
resolve(undefined);
}, timeout)),
]);
cleanupWithTimeout
.catch((e) => {
logger.log('Error during destroy cleanup', e);
})
.finally(() => {
this.insertedDomEl?.parentNode?.removeChild(this.insertedDomEl);
});
}| authType: AuthType.None, | ||
| logLevel: LogLevel.ERROR, | ||
| waitForCleanupOnDestroy: false, | ||
| cleanupTimeout: 5000, |
There was a problem hiding this comment.
The default value for cleanupTimeout is set to 5000 here, but the JSDoc for this property in src/types.ts on line 705 specifies a default of 10000. To ensure consistency between the code and documentation, please update the default value here to match the documentation.
| cleanupTimeout: 5000, | |
| cleanupTimeout: 10000, |
| * ``` | ||
| * @version SDK: 1.41.0 | ThoughtSpot: 10.12.0.cl | ||
| */ | ||
| DestroyEmbed = 'EmbedDestroyed', |
There was a problem hiding this comment.
The string value for HostEvent.DestroyEmbed is 'EmbedDestroyed'. This is a bit confusing as it sounds like an event coming from the embed, but HostEvents are sent to the embed. For consistency with other HostEvent members like AskSpotter = 'AskSpotter', consider changing the value to 'DestroyEmbed'.
| DestroyEmbed = 'EmbedDestroyed', | |
| DestroyEmbed = 'DestroyEmbed', |
jbhanu-thoughtspot
force-pushed
the
SCAL-255929
branch
from
66169c7 to
cd0775c
Compare
commit: |
|
SonarQube Quality Gate
|
No description provided.