Skip to content

prerender poc#508

Open
ruchI9897 wants to merge 4 commits intomainfrom
prerender-bug
Open

prerender poc#508
ruchI9897 wants to merge 4 commits intomainfrom
prerender-bug

Conversation

@ruchI9897
Copy link
Copy Markdown
Contributor

No description provided.

@ruchI9897 ruchI9897 requested a review from a team as a code owner April 16, 2026 11:06
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a fullHeight property to the viewConfig and updates the setIFrameHeight method to adjust the host container and pre-render wrapper heights when this mode is active. A critical regression was identified where the standard iframe height assignment was commented out, which would break resizing in default scenarios. Other feedback includes optimizing the code by caching dimension calculations, removing redundant type casting, and cleaning up trailing whitespace.

Comment thread src/embed/ts-embed.ts Outdated
Comment on lines +1125 to +1135
if (this.viewConfig.fullHeight && this.isPreRendered) {
if (this.preRenderWrapper) {
this.preRenderWrapper.style.height = getCssDimension(height);
}
// Also grow the host container (this.el) so in-flow siblings are
// pushed down correctly and the page layout reflects the real height.
if (this.el) {
(this.el as HTMLElement).style.height = getCssDimension(height);
}
}
// this.iFrame.style.height = getCssDimension(height);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The implementation of setIFrameHeight contains a regression and several minor issues:

  1. Regression: The default logic for setting the iframe height (this.iFrame.style.height) has been commented out. This will prevent the iframe from resizing correctly in all standard (non-prerendered) scenarios or when fullHeight is disabled.
  2. Efficiency: getCssDimension(height) is called three times. It should be calculated once and stored in a variable.
  3. Redundancy: The cast (this.el as HTMLElement) is unnecessary as this.el is already declared as an HTMLElement.
  4. Style: There is significant trailing whitespace on several lines (e.g., 1125, 1127, 1130).
        const cssHeight = getCssDimension(height);
        if (this.viewConfig.fullHeight && this.isPreRendered) {
            if (this.preRenderWrapper) {
                this.preRenderWrapper.style.height = cssHeight;
            }
            // Also grow the host container (this.el) so in-flow siblings are
            // pushed down correctly and the page layout reflects the real height.
            if (this.el) {
                this.el.style.height = cssHeight;
            }
        }
        this.iFrame.style.height = cssHeight;

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Apr 16, 2026

Open in StackBlitz

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

commit: 3c745e6

@sonar-prod-ts
Copy link
Copy Markdown

sonar-prod-ts Bot commented Apr 16, 2026

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant