Skip to content

SCAL-315058: isReadyForRenderPromise gate never resolves + null-safety in init path#545

Merged
mouryabalabhadra merged 3 commits into
mainfrom
SCAL-315058
May 29, 2026
Merged

SCAL-315058: isReadyForRenderPromise gate never resolves + null-safety in init path#545
mouryabalabhadra merged 3 commits into
mainfrom
SCAL-315058

Conversation

@mouryabalabhadra
Copy link
Copy Markdown
Collaborator

No description provided.

@mouryabalabhadra mouryabalabhadra requested a review from a team as a code owner May 28, 2026 21:39
@mouryabalabhadra
Copy link
Copy Markdown
Collaborator Author

@copilot review the pr

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 28, 2026

Open in StackBlitz

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

commit: e0cd2ae

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 refactors the initialization and pre-rendering lifecycle of the ThoughtSpot embed SDK, ensuring that iframe replacement waits for initialization to complete and that initialization status is correctly tracked as a boolean. It also adds comprehensive unit tests for these lifecycle states. The review feedback highlights two critical issues in src/embed/ts-embed.ts where initialization failures are handled unsafely: first, an unavailable embedConfig in afterInit is silently ignored rather than throwing an error, which leads to downstream crashes; second, the initialization promise chain swallows errors in its .catch block, allowing downstream processes to proceed with a broken state instead of properly rejecting.

Comment thread src/embed/ts-embed.ts
Comment thread src/embed/ts-embed.ts
Copy link
Copy Markdown

Copilot AI commented May 28, 2026

@copilot review the pr

Reviewed the current PR changes and I don’t see any actionable issues in the patch. I also ran targeted validation on the touched area: npx jest -c jest.config.sdk.js --runInBand --coverage=false src/embed/base.spec.ts src/embed/ts-embed.spec.ts passed, and npx eslint on the touched files reported only warning-level comment-length findings.

@mouryabalabhadra mouryabalabhadra merged commit cbea591 into main May 29, 2026
8 of 10 checks passed
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.

3 participants