SCAL-236431 Add embed initialization state management #325
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) |
Summary of ChangesHello @shivam-kumar-ts, 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 significantly enhances the embed's initialization process by introducing a comprehensive state management system. It allows embed instances to clearly communicate their readiness through defined states and events, enabling more robust and responsive integrations. This change ensures that embed components are only rendered when fully prepared, leading to a more stable and predictable user experience. 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 state machine for managing the initialization of embedded components, which is a great improvement for robustness and developer experience. It adds InitState, getInitState(), waitForInit(), and the InitStateChange event. The implementation correctly transitions from Initializing to Ready upon successful global SDK initialization. However, there are a few critical areas for improvement. The current implementation does not handle initialization failures, which can lead to unhandled promise rejections and cause the application to hang. The InitState enum is also missing a failure state. Additionally, a new test suite has some confusing and unused variables. Addressing these points will make the new state management system more complete and robust.
src/embed/ts-embed.ts
Outdated
| this.initPromise = new Promise((resolve) => { | ||
| this.initPromiseResolve = resolve; | ||
| }); | ||
| this.setInitState(InitState.Initializing); | ||
|
|
||
| this.isReadyForRenderPromise = getInitPromise().then(async () => { | ||
| if (!embedConfig.authTriggerContainer && !embedConfig.useEventForSAMLPopup) { | ||
| this.embedConfig.authTriggerContainer = domSelector; | ||
| } | ||
| this.thoughtSpotHost = getThoughtSpotHost(embedConfig); | ||
| this.thoughtSpotV2Base = getV2BasePath(embedConfig); | ||
| this.shouldEncodeUrlQueryParams = embedConfig.shouldEncodeUrlQueryParams; | ||
| this.setInitState(InitState.Ready); | ||
| }); |
There was a problem hiding this comment.
The promise returned by getInitPromise() can potentially reject if the global init() fails. This rejection is not handled here, which will result in an "unhandled promise rejection" error in the consumer's application. Furthermore, the embed instance's initPromise will never resolve or reject, causing any code awaiting it (like in renderIFrame) to hang indefinitely.
You should handle this rejection by rejecting the instance's initPromise. This will also make the try/catch block in renderIFrame functional.
To do this, you'll need to:
- Add a
private initPromiseReject: (reason?: any) => void;property to the class (nearinitPromiseResolve). - Update the constructor logic as suggested below to capture the
rejectfunction and add a.catch()block.
this.initPromise = new Promise((resolve, reject) => {
this.initPromiseResolve = resolve;
this.initPromiseReject = reject;
});
this.setInitState(InitState.Initializing);
this.isReadyForRenderPromise = getInitPromise().then(async () => {
if (!embedConfig.authTriggerContainer && !embedConfig.useEventForSAMLPopup) {
this.embedConfig.authTriggerContainer = domSelector;
}
this.thoughtSpotHost = getThoughtSpotHost(embedConfig);
this.thoughtSpotV2Base = getV2BasePath(embedConfig);
this.shouldEncodeUrlQueryParams = embedConfig.shouldEncodeUrlQueryParams;
this.setInitState(InitState.Ready);
}).catch((error) => {
this.initPromiseReject(error);
});| export enum InitState { | ||
| /** | ||
| * Embed instance created but initialization not started | ||
| */ | ||
| NotStarted = 'not-started', | ||
| /** | ||
| * SDK initialization in progress | ||
| */ | ||
| Initializing = 'initializing', | ||
| /** | ||
| * SDK initialization completed successfully | ||
| */ | ||
| Ready = 'ready', | ||
| } |
There was a problem hiding this comment.
The InitState enum currently only accounts for success states (NotStarted, Initializing, Ready). It's missing a state to represent a failure during initialization. Without a failure state, if initialization fails, the component will be stuck in the Initializing state forever, which can lead to hangs and makes it difficult for consumers of the SDK to handle errors gracefully. Please add a state for failures, for example Error.
export enum InitState {
/**
* Embed instance created but initialization not started
*/
NotStarted = 'not-started',
/**
* SDK initialization in progress
*/
Initializing = 'initializing',
/**
* SDK initialization completed successfully
*/
Ready = 'ready',
/**
* SDK initialization failed
*/
Error = 'error',
}
src/embed/ts-embed.ts
Outdated
| try { | ||
| await this.ensureInitialized(); | ||
| } catch (error) { | ||
| this.handleError('Cannot render: initialization failed'); |
There was a problem hiding this comment.
You've added a new constant EMBED_INITIALIZATION_FAILED in src/errors.ts, which is great for maintainability. However, this hardcoded string is used here instead of the constant. Please use the constant to ensure consistency and make future changes easier.
this.handleError(ERROR_MESSAGE.EMBED_INITIALIZATION_FAILED);
src/embed/ts-embed.spec.ts
Outdated
| mockInitPromise = new Promise<void>((resolve) => { | ||
| mockResolve = () => resolve(); | ||
| }); |
There was a problem hiding this comment.
In the new 'Initialization State Management' test suite, the mockInitPromise and mockResolve variables are defined in beforeEach but are not actually connected to the TsEmbed instance under test. The test passes due to the side effect of the global init() call in beforeEach. This can be confusing and makes the test less explicit about what it's testing. To make the test more robust and clear, consider mocking getInitPromise from base.ts to directly control the initialization flow for the embed instance. This would allow you to test both success and failure scenarios more reliably.
commit: |
shivam-kumar-ts
force-pushed
the
SCAL-236431
branch
4 times, most recently
from
f15ed82 to
d1ca382
Compare
shivam-kumar-ts
force-pushed
the
SCAL-236431
branch
from
d1ca382 to
9ae8462
Compare
|
SonarQube Quality Gate
|
Add embed initialization state management and error handling
InitStateChangeevent to track changes in embed initialization state.InitStateenum to represent various initialization states.TsEmbedclass to manage initialization state and handle errors more gracefully during rendering.ALREADY REMOVED THIS BUG IN TIS PR #152