SCAL-313306: refactor declarative embedParams building in V1Embed#547
SCAL-313306: refactor declarative embedParams building in V1Embed#547yinstardev wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
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.
| export function buildEmbedParams( | ||
| contributions: Partial<EmbedParamsContribution>[] | ||
| ): Partial<EmbedParamsContribution> | undefined { | ||
| if (contributions.length === 0) return undefined; | ||
|
|
||
| return contributions.reduce((acc, contrib) => { | ||
| if (!contrib || Object.keys(contrib).length === 0) return acc; | ||
| return { ...acc, ...contrib }; | ||
| }, {} as Partial<EmbedParamsContribution>); | ||
| } |
There was a problem hiding this comment.
If all contributions are empty, buildEmbedParams currently returns an empty object {}. Since {} is truthy, this results in an empty embedParams object being added to the payload (e.g., embedParams: {}), which unnecessarily bloats the payload. Returning undefined when the merged object is empty prevents this.
| export function buildEmbedParams( | |
| contributions: Partial<EmbedParamsContribution>[] | |
| ): Partial<EmbedParamsContribution> | undefined { | |
| if (contributions.length === 0) return undefined; | |
| return contributions.reduce((acc, contrib) => { | |
| if (!contrib || Object.keys(contrib).length === 0) return acc; | |
| return { ...acc, ...contrib }; | |
| }, {} as Partial<EmbedParamsContribution>); | |
| } | |
| export function buildEmbedParams( | |
| contributions: Partial<EmbedParamsContribution>[] | |
| ): Partial<EmbedParamsContribution> | undefined { | |
| const merged = contributions.reduce((acc, contrib) => { | |
| if (!contrib || Object.keys(contrib).length === 0) return acc; | |
| return { ...acc, ...contrib }; | |
| }, {} as Partial<EmbedParamsContribution>); | |
| return Object.keys(merged).length > 0 ? merged : undefined; | |
| } |
References
- Prefer omitting empty or unset parameters entirely from the payload/URL to avoid bloating. Returning undefined when the merged embedParams object is empty ensures it is omitted. (link)
- When a utility function processes optional configuration and its output is forwarded without immediate narrowing, it is acceptable to keep the return type optional to match the input's optionality, especially if a tighter type wouldn't benefit downstream consumers.
| protected async getEmbedParamsContributions(): Promise<Partial<EmbedParamsContribution>[]> { | ||
| const contributions: Partial<EmbedParamsContribution>[] = []; | ||
|
|
||
| const sidebarContribution = buildSpotterSidebarEmbedParamsContribution( | ||
| this.viewConfig, | ||
| this.handleError.bind(this), | ||
| ); | ||
| return buildSpotterVizAppInitData(sidebarInitData, this.viewConfig); | ||
| if (Object.keys(sidebarContribution).length > 0) { | ||
| contributions.push(sidebarContribution); | ||
| } | ||
|
|
||
| const vizContribution = buildSpotterVizEmbedParamsContribution(this.viewConfig); | ||
| if (Object.keys(vizContribution).length > 0) { | ||
| contributions.push(vizContribution); | ||
| } | ||
|
|
||
| return contributions; | ||
| } |
There was a problem hiding this comment.
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),
),
buildSpotterVizEmbedParamsContribution(this.viewConfig),
];
}| 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; | ||
| } |
There was a problem hiding this comment.
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),
),
];
}| protected async getEmbedParamsContributions(): Promise<Partial<EmbedParamsContribution>[]> { | ||
| const contributions: Partial<EmbedParamsContribution>[] = []; | ||
|
|
||
| const vizContribution = buildSpotterVizEmbedParamsContribution(this.viewConfig); | ||
| if (Object.keys(vizContribution).length > 0) { | ||
| contributions.push(vizContribution); | ||
| } | ||
|
|
||
| return contributions; | ||
| } |
There was a problem hiding this comment.
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 [buildSpotterVizEmbedParamsContribution(this.viewConfig)];
}| protected async getEmbedParamsContributions(): Promise<Partial<EmbedParamsContribution>[]> { | ||
| const contributions: Partial<EmbedParamsContribution>[] = []; | ||
|
|
||
| if (this.viewConfig.visualOverrides) { | ||
| contributions.push({ visualOverridesParams: this.viewConfig.visualOverrides }); | ||
| } | ||
|
|
||
| return contributions; | ||
| } |
There was a problem hiding this comment.
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 [
this.viewConfig.visualOverrides
? { visualOverridesParams: this.viewConfig.visualOverrides }
: {},
];
}
commit: |
Replace imperative embedParams construction with a declarative pattern to eliminate spread-based mutation chains and remove per-subclass wiring. Changes: - Add embedParams-builder.ts as the single facade that maps viewConfig fields to embedParams payloads (collectEmbedParamsPayloads + buildEmbedParams). - TsEmbed.getDefaultAppInitData() calls collectEmbedParamsPayloads() once and spreads the merged result; the base class depends only on the facade, not on the individual feature modules. - Add contribution builders buildSpotterSidebarEmbedParamsContribution and buildSpotterVizEmbedParamsContribution; delete the old spread-based buildSpotterSidebarAppInitData / buildSpotterVizAppInitData. - Remove now-redundant getAppInitData overrides from AppEmbed, LiveboardEmbed, and SpotterEmbed; SearchEmbed keeps only its top-level searchOptions merge. - Rewrite spotter-utils and spotter-viz-utils specs for the new builders. Adding a new payload type now touches only embedParams-builder.ts. tsc clean; full SDK suite passes (1102 passed, 4 skipped).
a1f7dc1 to
8e0ac2b
Compare
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:
Benefits:
Tests: all 1103 tests pass