Pre render + fullHeight

[sastaachar]

No description provided.

[gemini-code-assist]

Code Review

This pull request refactors pre-rendering logic by introducing a placeholder element for better size tracking and renaming the primary element to hostElement. Review feedback identifies a critical bug where an uninitialized element is appended to the DOM, and an incorrect style update within the ResizeObserver that could break layout consistency. Other requested changes include removing debugging logs and unprofessional comments, as well as improving TSDoc documentation to meet style guide standards.

I am having trouble creating individual review comments. Click here to see my feedback.

src/embed/ts-embed.ts (1107-1109)

This block of code attempts to append this.insertedDomEl to this.hostElement. However, at this point in the execution flow, this.insertedDomEl has not been initialized yet; it gets initialized later within the showPreRender() method. This will cause a TypeError because appendChild will be called with undefined.

The placeholder element (insertedDomEl) is correctly created and appended within showPreRender(), so this block seems to be both buggy and redundant. It should be removed.

src/embed/ts-embed.ts (1775-1778)

The ResizeObserver callback is updating the iFrame's style directly. However, the iFrame's height is already set to 100% to fill its container (preRenderWrapper). The goal of the observer is to keep the preRenderWrapper's size in sync with the placeholder element. The previous implementation correctly updated this.preRenderWrapper.

This change seems incorrect and may break the fullHeight functionality for pre-rendered components. The test file ts-embed.spec.ts also seems to expect the preRenderWrapper to be updated, not the iFrame.

Please revert this to update this.preRenderWrapper to maintain consistency with existing patterns.

                            setStyleProperties(this.preRenderWrapper, {
                                width: entry.contentRect.width + 'px',
                                height: entry.contentRect.height + 'px',
                            });

References

1. When adding new code, prioritize maintaining local consistency with existing patterns in the file over introducing a new, more concise pattern if it breaks from the established convention.

src/embed/ts-embed.ts (917)

This console.log statement appears to be for debugging purposes. Please remove it before merging.

src/embed/ts-embed.ts (894)

This new method is missing a TSDoc comment. According to the style guide, documentation is important for maintainability. Please add a comment explaining what this method does and what it returns.

For example:

/**
 * Checks if the current embed is configured for pre-rendering.
 * @returns {boolean} True if a `preRenderId` is present in the view configuration.
 */

src/embed/ts-embed.ts (1031-1035)

The JSDoc for this method is informal and contains typos. According to the repository style guide (rules 71, 83, 84), documentation should be clear, grammatically correct, and use proper spelling.

I suggest improving it for clarity and professionalism. For example:

/**
 * Checks for an existing pre-rendered component and connects to it.
 *
 * If a matching pre-rendered component is found in the DOM, this method
 * sets the internal properties of the embed object to reference it.
 *
 * @returns {boolean} True if a connection was successfully established, false otherwise.
 */

    /**
     * Checks for an existing pre-rendered component and connects to it.
     * 
     * If a matching pre-rendered component is found in the DOM, this method
     * sets the internal properties of the embed object to reference it.
     * 
     * @returns True if a connection was successfully established, false otherwise.
     */

References

1. Documentation should be clear, grammatically correct, and use proper spelling. ^(link)

src/embed/ts-embed.ts (1091)

This comment is unprofessional and should be removed.

[sonar-prod-ts]

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.0% Duplication

[pkg-pr-new]

Open in StackBlitz

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

commit: ade94c2

[ruchI9897]

lgtm