korbit-ai[bot] commented on code in PR #34876:
URL: https://github.com/apache/superset/pull/34876#discussion_r2305784557
##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -231,10 +232,24 @@ function Echart(
return echartsTheme;
};
- const themedEchartOptions = merge(
+ // Custom replacer function to replace arrays instead of merging them
+ const replaceArrays = (objValue: any, srcValue: any) => {
+ if (Array.isArray(srcValue)) {
+ return srcValue; // Replace arrays entirely
+ }
+ // Let lodash handle object merging for non-arrays
+ return undefined;
+ };
Review Comment:
### Incomplete Array Replacement Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The replaceArrays customizer function doesn't handle nested arrays within
objects, which could lead to unexpected behavior when merging complex ECharts
configurations.
###### Why this matters
When ECharts configurations contain nested objects with array properties,
the current implementation will only replace top-level arrays, potentially
causing data visualization issues or style inconsistencies in complex charts.
###### Suggested change ∙ *Feature Preview*
Implement a recursive array replacement strategy:
```typescript
const replaceArrays = (objValue: any, srcValue: any) => {
if (Array.isArray(srcValue)) {
return srcValue;
}
if (srcValue && typeof srcValue === 'object') {
const result = {};
Object.keys(srcValue).forEach(key => {
if (Array.isArray(srcValue[key])) {
result[key] = srcValue[key];
}
});
if (Object.keys(result).length > 0) {
return { ...objValue, ...result };
}
}
return undefined;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0373b18-b006-4f62-bc8f-6df4461ace6d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0373b18-b006-4f62-bc8f-6df4461ace6d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0373b18-b006-4f62-bc8f-6df4461ace6d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0373b18-b006-4f62-bc8f-6df4461ace6d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0373b18-b006-4f62-bc8f-6df4461ace6d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ef4231fd-1f65-46bd-b6f1-10cc5a6eedfa -->
[](ef4231fd-1f65-46bd-b6f1-10cc5a6eedfa)
##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -231,10 +232,24 @@
return echartsTheme;
};
- const themedEchartOptions = merge(
+ // Custom replacer function to replace arrays instead of merging them
+ const replaceArrays = (objValue: any, srcValue: any) => {
+ if (Array.isArray(srcValue)) {
+ return srcValue; // Replace arrays entirely
+ }
+ // Let lodash handle object merging for non-arrays
+ return undefined;
+ };
+
+ const themedEchartOptions = mergeWith(
{},
getEchartsTheme(echartOptions),
echartOptions,
+ theme.echartsOptionsOverrides || {},
+ vizType
+ ? theme.echartsOptionsOverridesByChartType?.[vizType] || {}
+ : {},
+ replaceArrays,
);
Review Comment:
### Redundant Theme Merging on Renders <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple merge operations with theme options are performed on every render,
potentially causing unnecessary object creation and computation overhead.
###### Why this matters
Creating new merged objects on every render can impact performance,
especially with complex chart configurations. This could lead to increased
memory usage and potential garbage collection pauses.
###### Suggested change ∙ *Feature Preview*
Memoize the merged theme options using useMemo to prevent unnecessary
recomputation:
```typescript
const themedEchartOptions = useMemo(() =>
mergeWith(
{},
getEchartsTheme(echartOptions),
echartOptions,
theme.echartsOptionsOverrides || {},
vizType
? theme.echartsOptionsOverridesByChartType?.[vizType] || {}
: {},
replaceArrays,
),
[echartOptions, theme, vizType]
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/830ff730-a2f3-4c4e-8f49-9ee2468a34f7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/830ff730-a2f3-4c4e-8f49-9ee2468a34f7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/830ff730-a2f3-4c4e-8f49-9ee2468a34f7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/830ff730-a2f3-4c4e-8f49-9ee2468a34f7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/830ff730-a2f3-4c4e-8f49-9ee2468a34f7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d781f0b2-1fd6-4a62-accf-131a3d391312 -->
[](d781f0b2-1fd6-4a62-accf-131a3d391312)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
