Skip to content

SCAL-337722 Add preRenderConfig.inFlow to mount the pre-render in-flow - #664

Open
sastaachar wants to merge 1 commit into
mainfrom
SCAL-337722
Open

sastaachar wants to merge 1 commit into
mainfrom
SCAL-337722

Conversation

@sastaachar

@sastaachar sastaachar commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

What

Adds preRenderConfig.inFlow. When set, showPreRender() moves the pre-rendered frame into the host element with Element.moveBefore() instead of overlaying it, so the browser lays it out like a normal embed.

SCAL-337722

Why

Today the wrapper is parked on document.body, absolutely positioned, and aligned to an in-flow placeholder by syncPreRenderStyle() + a ResizeObserver + a container scroll listener. Because it sits outside the host's layout flow, the SDK emulates what the browser does for free — position, size, stacking, clipping, scrolling.

That emulation is the source of a recurring class of defects: SCAL-235696, SCAL-270176 (P0), SCAL-287671, SCAL-307017, SCAL-325118, SCAL-331221, and SCAL-265112 which is still open — six sizing/stacking bugs in 21 months, each from a different host layout, none predicted by the last.

moveBefore() is a state-preserving atomic move: the frame keeps its browsing context, so it can be reparented without reloading. That removes the reason the wrapper had to live out of flow.

How

  • showPreRender() — move the wrapper into hostElement, clear position/top/left, size it from frameParams. The placeholder, syncPreRenderStyle(), the ResizeObserver and the scroll listener are all skipped.
  • hidePreRender() — move it back to the parking container before restoring the hidden styles.
  • syncPreRenderStyle() is a no-op while in-flow, so a host calling it directly cannot fight the browser.

Guarded end to end: off unless opted in, feature-detected on Element.prototype.moveBefore, and try/catch around the move. Any of those failing takes the existing overlay path, so it is safe to set unconditionally.

Not in scope

  • Reuse defects — filter leak, config bleed, residual screen state, memory retention on navigateToLiveboard. moveBefore preserves state by design, so these are unaffected.
  • Background-load behaviour — the instance still loads and runs while parked (SCAL-337430 unchanged).
  • fullHeight — the height negotiation over postMessage is untouched. This removes the wrapper sync, not the protocol.

Browser support

Chrome 133+, Firefox 144+. Safari has not shipped moveBefore, so WebKit keeps the overlay path and both placement models remain supported.

Testing

tsc --noEmit clean. Full ts-embed.spec.ts green: 1257 passed, nothing existing changed.

Five new cases under preRenderConfig.inFlow:

Case Expected
inFlow + moveBefore available wrapper is a child of the host, no placeholder, position/top/left cleared
inFlow, moveBefore missing overlay path, placeholder present
inFlow, moveBefore throws overlay path, placeholder present
inFlow not set overlay path
hide after an in-flow show wrapper back in the parking container, hidden styles restored

Worth noting there is no e2e harness that can park and show a pre-render separately (see SCAL-336321's notes on the embed test bed), so real-browser verification across host layouts — ancestor transform, clipping overflow: hidden, inner scroll container, sticky header, high z-index sibling, two co-located embeds — is still manual.

showPreRender() parks the wrapper outside the host's layout flow and keeps it
aligned in JS, so position, size, stacking, clipping and scrolling are emulated
rather than owned by the browser.

With inFlow set, Element.moveBefore() reparents the frame into the host element
without reloading it, and the placeholder, syncPreRenderStyle, the ResizeObserver
and the container scroll listener are all skipped. hidePreRender() moves it back
to the parking container.

Feature-detected and guarded: where moveBefore is missing (Safari) or the move is
rejected, it falls back to the existing overlay path.
@sastaachar
sastaachar requested a review from a team as a code owner September 10, 2026 10:52

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

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.

Code Review

This pull request introduces an inFlow option for pre-rendering, allowing the pre-rendered frame to be reparented into the host element using Element.moveBefore() for native browser layout and scrolling. Feedback includes removing a redundant nullish coalescing operator in hidePreRender and correcting a spelling typo ('behaviour' to 'behavior') in the JSDoc to adhere to the en-US style guide.

Comment thread src/embed/ts-embed.ts
return;
}
if (this.isInFlow) {
this.movePreRenderWrapperTo(this.preRenderContainerEl ?? document.body);

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.

medium

The preRenderContainerEl property is initialized with document.body and is always an HTMLElement, so it will never be null or undefined. The nullish coalescing operator ?? document.body is redundant and can be removed for simplicity.

Suggested change
this.movePreRenderWrapperTo(this.preRenderContainerEl ?? document.body);
this.movePreRenderWrapperTo(this.preRenderContainerEl);

Comment thread src/types.ts
*
* Requires `Element.moveBefore()`, which preserves the frame's state across the
* move. Where it is unavailable (notably Safari) or the move is rejected, this
* silently falls back to the overlay behaviour, so it is always safe to set.

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.

medium

Per the repository style guide (Rule 9), please use American English (en-US) spelling. The word behaviour should be behavior.

Suggested change
* silently falls back to the overlay behaviour, so it is always safe to set.
* silently falls back to the overlay behavior, so it is always safe to set.
References
  1. The style guide (Rule 9) mandates the use of American English (en-US) format for all written work, including code comments. This includes using 'behavior' instead of 'behaviour'. (link)

@pkg-pr-new

pkg-pr-new Bot commented Sep 10, 2026

Copy link
Copy Markdown

Open in StackBlitz

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

commit: 94a8236

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