SCAL-255099 Enhance event listener management in TsEmbed #315

shivam-kumar-ts · 2025-09-22T09:11:52Z

Enhance event listener management in TsEmbed:

separate subscription for network and message events, ensuring network events are always active regardless of authentication status.
Update tests to verify correct listener registration and behavior after authentication failures and successes.
Solution of getting Not logged in message

gemini-code-assist · 2025-09-22T09:12:06Z

Summary of Changes

Hello @shivam-kumar-ts, I'm Gemini Code Assist¹! 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 enhances the application's resilience to network fluctuations by implementing a robust internet connectivity check. This new utility ensures that actions dependent on network availability, such as reloading the embed on an 'online' event, are only performed when a stable connection is verified, improving the user experience by preventing premature or failed operations.

Highlights

New Utility Function: Introduced a checkInternetConnection utility function that verifies internet connectivity by attempting to fetch a reliable endpoint, incorporating exponential backoff for retries and a timeout mechanism.
Enhanced Online Event Handling: Integrated the new checkInternetConnection utility into the TsEmbed class's onlineEventListener to conditionally trigger a reload only when actual internet connectivity is confirmed, preventing unnecessary reloads.
Comprehensive Testing: Added dedicated test cases for the checkInternetConnection function in src/utils.spec.ts to validate its behavior under both online and offline scenarios.

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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description

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.

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. ↩

mnk-blr · 2025-09-22T09:12:07Z

🎉 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

Code Review

This pull request introduces a utility function, checkInternetConnection, to verify internet connectivity with an exponential backoff mechanism and integrates it into the onlineEventListener. While the intention is good, I've found a few critical issues. The implementation of checkInternetConnection has a bug that prevents the retry logic from executing on non-2xx HTTP responses. Additionally, the integration in ts-embed.ts is missing an await for the asynchronous call, which means it won't work as intended. The tests for the new utility are also insufficient as they don't cover the retry scenarios. I've provided detailed comments and suggestions to address these points.

gemini-code-assist · 2025-09-22T09:13:45Z

src/embed/ts-embed.ts

        const onlineEventListener = (e: Event) => {
-            this.trigger(HostEvent.Reload);
+            if(checkInternetConnection(5 , 1000 , this.thoughtSpotHost)) {
+                this.trigger(HostEvent.Reload);
+            }


The checkInternetConnection function is async and returns a Promise. In an if condition, a Promise is always truthy, so this.trigger(HostEvent.Reload) will always be called when the online event fires, regardless of the actual connectivity. You need to await the result of checkInternetConnection and make the onlineEventListener callback async.

Suggested change

const onlineEventListener = (e: Event) => {

this.trigger(HostEvent.Reload);

if(checkInternetConnection(5 , 1000 , this.thoughtSpotHost)) {

this.trigger(HostEvent.Reload);

}

const onlineEventListener = async (e: Event) => {

if(await checkInternetConnection(5 , 1000 , this.thoughtSpotHost)) {

this.trigger(HostEvent.Reload);

}

};

gemini-code-assist · 2025-09-22T09:13:45Z

src/utils.ts

+            logger.debug(`Pinging ${pingUrl} failed with HTTP ${response.status}: ${response.statusText}`);
+            return false;


There's a critical issue here. The function returns false immediately upon receiving a non-2xx HTTP response, which prevents the retry mechanism from being triggered. The retry logic with exponential backoff is only in the catch block, which is not executed for resolved promises with error statuses. To fix this, you should throw an error to ensure that non-2xx responses are handled by the retry logic in the catch block.

Suggested change

logger.debug(`Pinging ${pingUrl} failed with HTTP ${response.status}: ${response.statusText}`);

return false;

logger.debug(`Pinging ${pingUrl} failed with HTTP ${response.status}: ${response.statusText}`);

throw new Error(`Server responded with status ${response.status}`);

gemini-code-assist · 2025-09-22T09:13:46Z

src/embed/ts-embed.ts


        const onlineEventListener = (e: Event) => {
-            this.trigger(HostEvent.Reload);
+            if(checkInternetConnection(5 , 1000 , this.thoughtSpotHost)) {


The values 5 and 1000 are magic numbers. It would be better to define them as named constants at the top of the file or in a shared constants file. This improves readability and makes it easier to change these values in the future.

gemini-code-assist · 2025-09-22T09:13:46Z

src/utils.spec.ts

+describe('checkInternetConnection', () => {
+    const thoughtSpotHost = 'http://localhost:3000';
+
+    it('should return true when internet connection is available', async () => {
+        jest.spyOn(window, 'fetch').mockImplementation(() => Promise.resolve({
+            ok: true,
+            status: 200,
+            statusText: 'OK',
+        }));
+        expect(await checkInternetConnection(5 , 1000 , thoughtSpotHost)).toBe(true);
+    });
+
+    it('should return false when internet connection is not available', async () => {
+        jest.spyOn(window, 'fetch').mockImplementation(() => Promise.resolve({
+            ok: false,
+            status: 404,
+            statusText: 'Not Found',
+        }));
+        expect(await checkInternetConnection(5 , 1000 , thoughtSpotHost)).toBe(false);
+    });
+});


The current tests for checkInternetConnection are insufficient as they do not cover the retry logic, which is a core part of this utility. The tests only check for immediate success or immediate failure.

Please add tests to cover the following scenarios:

The connection check fails once but succeeds on a retry.

The connection check fails for all retry attempts.

This will ensure the exponential backoff and retry mechanism works as expected. You might find jest.useFakeTimers() useful to avoid long-running tests.

gemini-code-assist · 2025-09-22T09:13:46Z

src/utils.ts

+                return false;
+            }
+
+            logger.debug(`Connectivity check failed (attempt ${attempt + 1}/${maxRetries + 1}), retrying in ${delayMs}ms`);


For better observability and easier debugging, it's a good practice to include the actual error in the log message when a connectivity check fails.

Suggested change

logger.debug(`Connectivity check failed (attempt ${attempt + 1}/${maxRetries + 1}), retrying in ${delayMs}ms`);

logger.debug(`Connectivity check failed (attempt ${attempt + 1}/${maxRetries + 1}), retrying in ${delayMs}ms. Error: ${error}`);

pkg-pr-new · 2025-09-22T09:14:00Z

Open in StackBlitz

npm i https://pkg.pr.new/thoughtspot/visual-embed-sdk/@thoughtspot/visual-embed-sdk@315

commit: cd4b519

adityamittal3107 · 2025-10-03T02:52:29Z

src/embed/ts-embed.ts

+     * embed instance through an identifier contained in the payload,
+     * and executes the registered callbacks accordingly.
+     */
+    private subscribeToEvents() {


why do we need this if we are calling network events function and message events function seperately?

Calling From showPreRender function

adityamittal3107 · 2025-10-03T02:53:58Z

src/embed/ts-embed.ts

+        window.addEventListener('online', onlineEventListener);
+
+        const offlineEventListener = (e: Event) => {
+            const offlineWarning =


Move error message to error file

sonar-prod-ts · 2025-10-03T03:25:25Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

shivam-kumar-ts requested a review from a team as a code owner September 22, 2025 09:11

gemini-code-assist bot reviewed Sep 22, 2025

View reviewed changes

shivam-kumar-ts force-pushed the SCAL-255099 branch 17 times, most recently from ad82e21 to 2e75792 Compare September 27, 2025 11:28

shivam-kumar-ts changed the title ~~SCAL-255099 Add checkInternetConnection utility and integrate into onlineEventListener~~ SCAL-255099 Enhance error handling for authentication token Sep 27, 2025

shivam-kumar-ts force-pushed the SCAL-255099 branch from 2e75792 to c7906cf Compare September 27, 2025 11:36

shivam-kumar-ts requested review from adityamittal3107 and sastaachar September 27, 2025 11:39

shivam-kumar-ts force-pushed the SCAL-255099 branch 4 times, most recently from 5798e12 to 2b29dbe Compare September 30, 2025 06:18

SCAL-255099 Enhance event listener management in TsEmbed

57ff86b

shivam-kumar-ts force-pushed the SCAL-255099 branch from 2b29dbe to 57ff86b Compare September 30, 2025 06:22

shivam-kumar-ts changed the title ~~SCAL-255099 Enhance error handling for authentication token~~ SCAL-255099 Enhance event listener management in TsEmbed Sep 30, 2025

adityamittal3107 reviewed Oct 3, 2025

View reviewed changes

SCAL-255099 Add offline warning message to error handling in TsEmbed

cd4b519

sastaachar approved these changes Oct 6, 2025

View reviewed changes

shivam-kumar-ts merged commit f2780d2 into main Oct 6, 2025
9 checks passed

		logger.debug(`Pinging ${pingUrl} failed with HTTP ${response.status}: ${response.statusText}`);
		return false;

	logger.debug(`Connectivity check failed (attempt ${attempt + 1}/${maxRetries + 1}), retrying in ${delayMs}ms`);
	logger.debug(`Connectivity check failed (attempt ${attempt + 1}/${maxRetries + 1}), retrying in ${delayMs}ms. Error: ${error}`);

SCAL-255099 Enhance event listener management in TsEmbed #315

SCAL-255099 Enhance event listener management in TsEmbed #315

Uh oh!

Conversation

shivam-kumar-ts commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Sep 22, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

mnk-blr commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎉 Snyk checks have passed. No issues have been found so far.

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

pkg-pr-new bot commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adityamittal3107 Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

shivam-kumar-ts Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

adityamittal3107 Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

shivam-kumar-ts Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

sonar-prod-ts bot commented Oct 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shivam-kumar-ts commented Sep 22, 2025 •

edited

Loading

mnk-blr commented Sep 22, 2025 •

edited

Loading

pkg-pr-new bot commented Sep 22, 2025 •

edited

Loading