Skip to content

SCAL-313306: refactor declarative embedParams building in V1Embed#547

Open
yinstardev wants to merge 1 commit into
mainfrom
SCAL-313306-embedParams-declarative
Open

SCAL-313306: refactor declarative embedParams building in V1Embed#547
yinstardev wants to merge 1 commit into
mainfrom
SCAL-313306-embedParams-declarative

Conversation

@yinstardev

Copy link
Copy Markdown
Contributor

Replace imperative embedParams construction with declarative pattern to eliminate mutation chains and improve maintainability. Each embed type now declares contributions separately, merged once in base class.

Changes:

  • Create new embedParams-builder.ts with EmbedParamsContribution interface
  • Add getEmbedParamsContributions() method to TsEmbed (returns empty by default)
  • Update getDefaultAppInitData() to collect and merge contributions declaratively
  • Refactor AppEmbed: replaces getAppInitData() with getEmbedParamsContributions()
  • Refactor LiveboardEmbed: replaces getAppInitData() with getEmbedParamsContributions()
  • Refactor SearchEmbed: adds getEmbedParamsContributions(), preserves searchOptions in getAppInitData()
  • Refactor SpotterEmbed: replaces getAppInitData() with getEmbedParamsContributions()
  • Add contribution builders to spotter-utils and spotter-viz-utils
  • Keep old builder functions for backward compatibility

Benefits:

  • Eliminates spread operator chains causing hidden mutations
  • Type-safe embedParams with explicit interface
  • Easier to test contributions in isolation
  • Cleaner code with single responsibility
  • Maintains 100% backward compatibility

Tests: all 1103 tests pass

@yinstardev yinstardev requested a review from a team as a code owner June 2, 2026 04:43

@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 a centralized, declarative embedParams builder system (embedParams-builder.ts) to aggregate configuration contributions across various embed classes (AppEmbed, SpotterEmbed, LiveboardEmbed, and SearchEmbed), replacing the previous imperative getAppInitData overrides. The review feedback recommends updating buildEmbedParams to return undefined instead of an empty object when no contributions exist, which avoids bloating the payload. Following this change, the subclass implementations of getEmbedParamsContributions can be simplified to be fully declarative, eliminating redundant boilerplate checks.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/embed/embedParams-builder.ts Outdated
Comment thread src/embed/app.ts Outdated
Comment thread src/embed/conversation.ts Outdated
Comment on lines 409 to 421
protected async getEmbedParamsContributions(): Promise<Partial<EmbedParamsContribution>[]> {
const contributions: Partial<EmbedParamsContribution>[] = [];

const sidebarContribution = buildSpotterSidebarEmbedParamsContribution(
this.viewConfig,
this.handleError.bind(this),
);
if (Object.keys(sidebarContribution).length > 0) {
contributions.push(sidebarContribution);
}

return contributions;
}

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

With buildEmbedParams updated to safely filter out empty contributions and return undefined when empty, we can simplify this method to be completely declarative and eliminate the imperative boilerplate of checking Object.keys().length and pushing to an array.

    protected async getEmbedParamsContributions(): Promise<Partial<EmbedParamsContribution>[]> {
        return [
            buildSpotterSidebarEmbedParamsContribution(
                this.viewConfig,
                this.handleError.bind(this),
            ),
        ];
    }

Comment thread src/embed/liveboard.ts Outdated
Comment thread src/embed/search.ts Outdated
@pkg-pr-new

pkg-pr-new Bot commented Jun 2, 2026

Copy link
Copy Markdown

Open in StackBlitz

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

commit: a5f203f

@yinstardev yinstardev force-pushed the SCAL-313306-embedParams-declarative branch from a1f7dc1 to 8e0ac2b Compare June 2, 2026 07:17
Replace the imperative, spread-chain embedParams construction with a single
declarative mapping, removing the per-subclass overrides and the mutation
chains they relied on.

Before: each embed (App/Liveboard/Search/Spotter) overrode getAppInitData and
hand-wired builders that took the whole payload, reached into the previous
builder's embedParams via `as any`, and re-spread it. This was order-dependent,
duplicated across subclasses, and handled visualOverrides in two separate paths.

After:
- New embedParams-builder.ts owns the single viewConfig -> embedParams mapping
  as a declarative table (EMBED_PARAM_FIELDS). Trivial fields are pass-throughs;
  spotterSidebarConfig (legacy-flag merge + URL validation) is the one resolver
  with real logic. buildEmbedParamsPayload() runs the table and merges results.
- TsEmbed.getDefaultAppInitData() calls buildEmbedParamsPayload() once, so every
  embed type gets embedParams with zero per-subclass wiring.
- Deleted getAppInitData overrides from AppEmbed, LiveboardEmbed, SpotterEmbed;
  SearchEmbed keeps only its top-level searchOptions merge.
- visualOverrides is now one row in the table instead of two divergent paths.

Adding a new embedParams field is now a one-row change in embedParams-builder.ts.
No change to the emitted embedParams shape. tsc + eslint clean; full SDK suite
passes (1103 passed, 4 skipped).
@yinstardev yinstardev force-pushed the SCAL-313306-embedParams-declarative branch from 8e0ac2b to a5f203f Compare June 11, 2026 01:15
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