SCAL-262671 : Refactor fullheight code #268

sastaachar · 2025-07-21T12:33:50Z

No description provided.

mnk-blr · 2025-07-21T12:34:06Z

🎉 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)

ashubham · 2025-07-24T04:17:58Z

/gemini review

gemini-code-assist

Code Review

This pull request is a well-executed refactoring of the "full height" functionality. By extracting this logic into a dedicated FullHeight class, you've improved code organization, separation of concerns, and maintainability. The changes are consistently applied across both AppEmbed and LiveboardEmbed, and the addition of a new test suite for the FullHeight class is a great touch that ensures the refactored code is well-tested.

I have a few suggestions to further enhance the code quality, mainly around removing some leftover code from the refactoring and improving type safety.

Overall, great work on this refactoring!

gemini-code-assist · 2025-07-24T04:19:17Z

src/embed/app.ts

+    private sendFullHeightLazyLoadData = () => {
+        const data = calculateVisibleElementData(this.iFrame);
+        this.trigger(HostEvent.VisibleEmbedCoordinates, data);
+    }
+
+    /**
+     * This is a handler for the RequestVisibleEmbedCoordinates event.
+     * It is used to send the visible coordinates data to the host application.
+     * @param data The event payload
+     * @param responder The responder function
+     */
+    private requestVisibleEmbedCoordinatesHandler = (data: MessagePayload, responder: any) => {
+        logger.info('Sending RequestVisibleEmbedCoordinates', data);
+        const visibleCoordinatesData = calculateVisibleElementData(this.iFrame);
+        responder({ type: EmbedEvent.RequestVisibleEmbedCoordinates, data: visibleCoordinatesData });
+    }


These methods, sendFullHeightLazyLoadData and requestVisibleEmbedCoordinatesHandler, appear to be duplicates of logic that now resides in the new FullHeight class. Since AppEmbed delegates full-height handling to fullHeightClient, these methods seem to be unused within AppEmbed. Removing them would eliminate code duplication and prevent potential confusion.

gemini-code-assist · 2025-07-24T04:19:17Z

src/embed/app.ts

        } = this.viewConfig;

-        let params = {};
+        let params: any = {};


Using any for the params variable bypasses TypeScript's type checking. To improve type safety and code clarity, consider using a more specific type. Record<string, any> would be more explicit about the object's shape, making it clearer that this is a dictionary-like object.

Suggested change

let params: any = {};

let params: Record<string, any> = {};

gemini-code-assist · 2025-07-24T04:19:17Z

src/embed/liveboard.ts

     */
    protected getEmbedParams() {
-        let params = {};
+        let params: any = {};


Similar to other parts of the codebase, the params variable is typed as any. Using a more explicit type like Record<string, any> would improve code clarity and type safety, making it clearer that this is a dictionary-like object.

Suggested change

let params: any = {};

let params: Record<string, any> = {};

pkg-pr-new · 2025-07-24T04:55:53Z

Open in StackBlitz

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

commit: aa4e328

sastaachar · 2025-07-24T05:05:10Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the fullHeight functionality by extracting the logic into a new FullHeight class. This is a good improvement for code organization and separation of concerns. The tests have also been moved to a new spec file, which is appropriate.

However, I've found a few critical issues in the new FullHeight class that could lead to incorrect behavior or runtime errors, particularly related to how fallback heights are calculated. There are also some minor documentation and encapsulation improvements suggested. Please review the comments for details.

gemini-code-assist · 2025-07-24T05:06:55Z

src/full-height.ts

+  private updateIFrameHeight = (data: MessagePayload) => {
+    const viewConfig = this.getViewConfig();
+    this.setIFrameHeight(Math.max(data.data, viewConfig.defaultHeight));
+    this.sendFullHeightLazyLoadData();
+  };


This implementation has a couple of critical issues:

Potential NaN height: If viewConfig.defaultHeight is not provided (e.g., for AppEmbed), it will be undefined. Math.max(number, undefined) results in NaN, which will set an invalid height on the iframe.

Regression for AppEmbed: The original logic for AppEmbed used iframe.scrollHeight as a fallback height. This logic has been lost in the refactoring, which can lead to incorrect height calculations for AppEmbed.

Here's a suggestion that addresses both issues by checking the component type and providing safe fallbacks.

const viewConfig = this.getViewConfig(); const isAppEmbed = viewConfig.embedComponentType === 'AppEmbed'; const fallbackHeight = isAppEmbed ? (this.getIframe()?.scrollHeight || 0) : (viewConfig.defaultHeight || 500); this.setIFrameHeight(Math.max(data.data, fallbackHeight)); this.sendFullHeightLazyLoadData();

gemini-code-assist · 2025-07-24T05:06:55Z

src/full-height.ts

+    }
+    this.setIFrameHeight(frameHeight || viewConfig.defaultHeight);


This may result in setIFrameHeight being called with undefined if frameHeight and viewConfig.defaultHeight are both not provided. This can cause issues with the iframe's height.

You should use this.defaultHeight as a fallback, which is correctly initialized in the constructor to handle this case.

this.setIFrameHeight(frameHeight || this.defaultHeight);

gemini-code-assist · 2025-07-24T05:06:56Z

src/full-height.ts

+  * Sets the height of the iframe
+  * @param height The height in pixels
+  */
+  protected setIFrameHeight(height: number | string): void {


Since this method is only used within the FullHeight class, it should be marked as private to properly encapsulate the class's internal implementation.

Suggested change

protected setIFrameHeight(height: number | string): void {

private setIFrameHeight(height: number | string): void {

gemini-code-assist · 2025-07-24T05:06:56Z

src/types.ts

+     * This flag is used to enable the full height lazy load data.
+     * 
+     * @example
+     * ```js


The fullHeight option is also supported by AppEmbed. Please update the documentation to reflect this for clarity and consistency.

Suggested change

* Supported embed types: `LiveboardEmbed`

* Supported embed types: `LiveboardEmbed`, `AppEmbed`

sastaachar · 2025-07-28T12:18:06Z

/gemini review

gemini-code-assist

Code Review

An excellent refactoring effort! Abstracting the fullHeight logic into a dedicated FullHeight class significantly improves code reuse and maintainability by removing duplicated code from AppEmbed and LiveboardEmbed. The use of dependency injection to provide the FullHeight class with the necessary context from the embed components is well-executed.

I've identified a critical issue regarding how the iframe height is calculated, which could lead to errors. I've also noted a regression where LiveboardEmbed loses its default height, and a few minor improvements for maintainability. Please see my detailed comments below.

Once these issues are addressed, this will be a solid improvement to the codebase.

gemini-code-assist · 2025-07-28T12:19:48Z

src/full-height.ts

+  private updateIFrameHeight = (data: MessagePayload) => {
+    const viewConfig = this.getViewConfig();
+    this.setIFrameHeight(Math.max(data.data, viewConfig.defaultHeight));
+    this.sendFullHeightLazyLoadData();
+  };


There's a potential issue in the updateIFrameHeight method. The viewConfig.defaultHeight can be undefined if it's not provided by the consumer. When this happens, Math.max(number, undefined) will result in NaN, which is then passed to setIFrameHeight. This will set an invalid height on the iframe and break the full-height functionality.

You should provide a fallback value to prevent this. A simple fallback is 0, but note that the previous implementation for AppEmbed used iframe.scrollHeight, which might be a more desirable behavior to preserve.

private updateIFrameHeight = (data: MessagePayload) => { const viewConfig = this.getViewConfig(); // Use a fallback for defaultHeight to prevent NaN. const fallbackHeight = viewConfig.defaultHeight || 0; this.setIFrameHeight(Math.max(data.data, fallbackHeight)); this.sendFullHeightLazyLoadData(); };

gemini-code-assist · 2025-07-28T12:19:48Z

src/full-height.ts

+// Communicate with the embed 
+// 


The file-level comment is a bit vague. To improve clarity for future maintainers, it would be beneficial to provide a more descriptive comment that explains the purpose of the FullHeight class.

Suggested change

// Communicate with the embed

//

// This file contains the FullHeight class, which encapsulates the logic for

// handling full-height embeds. It is designed to be reused by different embed

// components like AppEmbed and LiveboardEmbed to avoid code duplication.

gemini-code-assist · 2025-07-28T12:19:48Z

src/full-height.ts

+  * Sets the height of the iframe
+  * @param height The height in pixels
+  */
+  protected setIFrameHeight(height: number | string): void {


The setIFrameHeight method is declared as protected, but it appears to be used only within the FullHeight class. To improve encapsulation and clearly define the class's public API, it's better to make it private.

Suggested change

protected setIFrameHeight(height: number | string): void {

private setIFrameHeight(height: number | string): void {

sonar-prod-ts · 2025-07-30T13:14:06Z

SonarQube Quality Gate

10.2% Duplicated Lines (%) on New Code (is greater than 3%)

See analysis details on SonarQube

ruchI9897 · 2025-07-30T14:12:00Z

src/embed/liveboard.ts

-     * ```
-     */
-    defaultHeight?: number;
+export interface LiveboardViewConfig extends BaseViewConfig, LiveboardOtherViewConfig, LiveboardAppEmbedViewConfig, FullHeightViewConfig {


This full height configs can be added in LiveboardAppEmbedViewConfig - it is common for liveboard and full app configs.

we should not have LiveboardAppEmbedViewConfig itself, we should have feature configs and liveboard should extend those

Feature configs are also there, but single common configs of app embed and liveboard, i have created this config.
We can keep the feature config and add in the LiveboardAppEmbedViewConfig for common or AllEmbedConfig for app embed.

yes what i meant it is , we should not keep full stuff there cause this could be supported in future as well to other embeds.

jbhanu-thoughtspot · 2025-08-14T21:45:06Z

src/embed/liveboard.ts

-            this.on(EmbedEvent.EmbedIframeCenter, this.embedIframeCenter);
-            this.on(EmbedEvent.RequestVisibleEmbedCoordinates, this.requestVisibleEmbedCoordinatesHandler);
+            this.fullHeightClient = new FullHeight({
+                getIframe: () => this.iFrame,


Maybe just pass the instance of liveboardEmbed here, and create these constructs inside the constructor? These constructs are very specific to the embed instance anyway, and this parts seems a little duplicated.

i did that before but that just creates a circular reference
FullHeight -> embed -> FullHeight

sonar-prod-ts · 2025-09-03T12:33:01Z

SonarQube Quality Gate

10.2% Duplicated Lines (%) on New Code (is greater than 3%)

See analysis details on SonarQube

sastaachar requested a review from a team as a code owner July 21, 2025 12:33

sastaachar changed the base branch from main to SCAL-258312 July 21, 2025 12:37

sastaachar changed the base branch from SCAL-258312 to main July 23, 2025 05:56

gemini-code-assist bot reviewed Jul 24, 2025

View reviewed changes

sastaachar added 5 commits July 24, 2025 10:22

resovle conflict

677a185

SCAL-258312 : lint

8ef2acd

SCAL-262671 : refactor fullheight

76de2f7

SCAL-262671 : rename

adbd3d4

SCAL-262671 : fix test

c801b9c

sastaachar force-pushed the SCAL-262671 branch from 6528508 to c801b9c Compare July 24, 2025 04:54

gemini-code-assist bot reviewed Jul 24, 2025

View reviewed changes

SCAL-262671 : review comments

bca762a

gemini-code-assist bot reviewed Jul 28, 2025

View reviewed changes

sastaachar added 2 commits July 30, 2025 18:35

SCAL-262671 : full height

57aa11a

SCAL-262671 : refactor

fed1642

ruchI9897 reviewed Jul 30, 2025

View reviewed changes

jbhanu-thoughtspot reviewed Aug 14, 2025

View reviewed changes

sastaachar added 2 commits September 3, 2025 18:01

SCAL-262671 : test

aa4e328

SCAL-262671 : test

434eb34

		}
		this.setIFrameHeight(frameHeight \|\| viewConfig.defaultHeight);

	protected setIFrameHeight(height: number \| string): void {
	private setIFrameHeight(height: number \| string): void {

	* Supported embed types: `LiveboardEmbed`
	* Supported embed types: `LiveboardEmbed`, `AppEmbed`

-// Communicate with the embed
-//
+// This file contains the FullHeight class, which encapsulates the logic for
+// handling full-height embeds. It is designed to be reused by different embed
+// components like AppEmbed and LiveboardEmbed to avoid code duplication.

SCAL-262671 : Refactor fullheight code #268

Are you sure you want to change the base?

SCAL-262671 : Refactor fullheight code #268

Uh oh!

Conversation

sastaachar commented Jul 21, 2025

Uh oh!

mnk-blr commented Jul 21, 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!

ashubham commented Jul 24, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

sastaachar commented Jul 24, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

sastaachar commented Jul 28, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

sonar-prod-ts bot commented Jul 30, 2025

Uh oh!

ruchI9897 Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

sastaachar Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

ruchI9897 Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

sastaachar Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

jbhanu-thoughtspot Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

sastaachar Aug 21, 2025

mnk-blr commented Jul 21, 2025 •

edited

Loading

pkg-pr-new bot commented Jul 24, 2025 •

edited

Loading