codeant-ai-for-open-source[bot] commented on code in PR #36847:
URL: https://github.com/apache/superset/pull/36847#discussion_r2649233375
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -163,10 +165,24 @@ export default function transformProps(
const metricLabel = getMetricLabel(metric);
const groupbyLabels = groupby.map(getColumnLabel);
const treeData = treeBuilder(data, groupbyLabels, metricLabel);
+ // this will actually prepare sequential color mapping when metric values
are numeric
+ const metricValues = (data || [])
+ .map(row => {
+ const v = row[metricLabel as string];
+ return typeof v === 'number' ? v : Number(v);
Review Comment:
**Suggestion:** Null/undefined metric values are converted to 0 by using
Number(null) which yields 0, so missing values are treated as valid numeric
zeros and will skew min/max and enable sequential coloring incorrectly; exclude
null/undefined before converting to Number so only real numeric-like values are
considered. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
// treat null/undefined as missing values (NaN), so they are filtered
out
if (v === null || v === undefined) {
return NaN;
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid logic bug in the PR: Number(null) === 0, so null/undefined
metric cells get treated as numeric zeros and will skew min/max and
enableSequential detection.
The improved code correctly treats null/undefined as missing (NaN) which the
subsequent Number.isFinite filter removes. This fixes the statistics used to
decide sequential coloring.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts
**Line:** 172:172
**Comment:**
*Logic Error: Null/undefined metric values are converted to 0 by using
Number(null) which yields 0, so missing values are treated as valid numeric
zeros and will skew min/max and enable sequential coloring incorrectly; exclude
null/undefined before converting to Number so only real numeric-like values are
considered.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -163,10 +165,24 @@ export default function transformProps(
const metricLabel = getMetricLabel(metric);
const groupbyLabels = groupby.map(getColumnLabel);
const treeData = treeBuilder(data, groupbyLabels, metricLabel);
+ // this will actually prepare sequential color mapping when metric values
are numeric
+ const metricValues = (data || [])
+ .map(row => {
+ const v = row[metricLabel as string];
+ return typeof v === 'number' ? v : Number(v);
+ })
+ .filter(v => Number.isFinite(v));
+ const minMetricValue = metricValues.length ? Math.min(...metricValues) : 0;
+ const maxMetricValue = metricValues.length ? Math.max(...metricValues) : 0;
Review Comment:
**Suggestion:** Using Math.min(...metricValues) / Math.max(...metricValues)
with the spread operator can blow the call stack or consume excessive memory
for large datasets; compute min/max with an iterative reduce to avoid spreading
a large array into a function call. [performance]
**Severity Level:** Minor ⚠️
```suggestion
const minMetricValue =
metricValues.length > 0
? metricValues.reduce((acc, val) => (val < acc ? val : acc),
metricValues[0])
: 0;
const maxMetricValue =
metricValues.length > 0
? metricValues.reduce((acc, val) => (val > acc ? val : acc),
metricValues[0])
: 0;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Using reduce to compute min/max avoids creating huge argument lists for
Math.min/Math.max, which can blow the call stack for very large datasets. The
suggested reduce-based calculation is safer and more memory-friendly.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts
**Line:** 175:176
**Comment:**
*Performance: Using Math.min(...metricValues) /
Math.max(...metricValues) with the spread operator can blow the call stack or
consume excessive memory for large datasets; compute min/max with an iterative
reduce to avoid spreading a large array into a function call.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -300,6 +315,29 @@ export default function transformProps(
},
series,
};
+
+ // this will add visualMap for sequential coloring when appropriate
+ if (useSequential) {
+ const startColor = colorFn('min', sliceId);
+ const endColor = colorFn('max', sliceId);
+ // try to pick a proper sequential palette from registry using the current
colorScheme as a hint
+ const seqScheme = sequentialRegistry.get(
+ (formData as any)?.linearColorScheme || (colorScheme as string),
+ );
+ const inRangeColors: string[] =
+ seqScheme?.colors && seqScheme.colors.length
+ ? seqScheme.colors
+ : [startColor, endColor];
Review Comment:
**Suggestion:** Calling `sequentialRegistry.get(...)` without guarding that
`sequentialRegistry` (or its `get` method) exists can throw at runtime;
additionally, the fallback `inRange` colors may include undefined entries (e.g.
if categorical colorFn returned undefined), which can break ECharts — guard the
registry access with optional chaining and filter/ensure the resulting color
array is non-empty (fallback to a safe theme color). [possible bug]
**Severity Level:** Critical 🚨
```suggestion
const seqScheme = sequentialRegistry?.get?.(
(formData as any)?.linearColorScheme || (colorScheme as string),
);
const inRangeColors: string[] = (
seqScheme?.colors && seqScheme.colors.length
? seqScheme.colors
: [startColor, endColor]
).filter((c): c is string => !!c);
if (inRangeColors.length === 0) {
// Fall back to a safe theme color if no valid sequential colors are
available
inRangeColors.push(theme?.colorPrimary ?? '#000000');
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Guarding registry access with optional chaining and filtering falsy colors
is a sensible defensive improvement.
getSequentialSchemeRegistry likely returns a registry in normal builds, but
optional chaining prevents a potential runtime crash in environments where the
registry might be unavailable. Filtering and falling back to a theme color
ensures ECharts doesn't receive invalid color entries.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts
**Line:** 324:330
**Comment:**
*Possible Bug: Calling `sequentialRegistry.get(...)` without guarding
that `sequentialRegistry` (or its `get` method) exists can throw at runtime;
additionally, the fallback `inRange` colors may include undefined entries (e.g.
if categorical colorFn returned undefined), which can break ECharts — guard the
registry access with optional chaining and filter/ensure the resulting color
array is non-empty (fallback to a safe theme color).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Treemap/transformProps.test.ts:
##########
@@ -74,4 +74,34 @@ describe('Treemap transformProps', () => {
}),
);
});
+
+ it('auto mode: should add visualMap for numeric varying metric', () => {
+ const result = transformProps(chartProps as EchartsTreemapChartProps);
+ expect(result.echartOptions).toHaveProperty('visualMap');
+ // @ts-ignore
+ expect(result.echartOptions.visualMap.min).toBeCloseTo(2.5);
+ // @ts-ignore
+ expect(result.echartOptions.visualMap.max).toBeCloseTo(10);
+ });
+
+ it('auto mode: should NOT add visualMap for constant metric', () => {
+ const formDataConst = { ...formData } as any;
+ const chartPropsConst = new ChartProps({
+ formData: formDataConst,
+ width: 800,
+ height: 600,
+ queriesData: [
+ {
+ data: [
+ { foo: 'A', bar: 'b1', sum__num: 5 },
+ { foo: 'B', bar: 'b2', sum__num: 5 },
+ ],
+ },
+ ],
+ theme: supersetTheme,
+ });
+ const result = transformProps(chartPropsConst as EchartsTreemapChartProps);
+ // @ts-ignore
+ expect(result.echartOptions.visualMap).toBeUndefined();
Review Comment:
**Suggestion:** Shared/shallow copy of `formData` can produce test coupling
or flakiness if `formData` is mutated elsewhere; also the test used `//
@ts-ignore` when asserting `visualMap` undefined. Build an explicit
`formDataConst` object with the fields the transform expects (instead of
shallow spread) and use a typed cast when checking `visualMap` to avoid
ts-ignore and accidental runtime access on unexpected shapes. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
const formDataConst = {
colorScheme: 'bnbColors',
datasource: '3__table',
granularity_sqla: 'ds',
metric: 'sum__num',
groupby: ['foo', 'bar'],
} as any;
const chartPropsConst = new ChartProps({
formData: formDataConst,
width: 800,
height: 600,
queriesData: [
{
data: [
{ foo: 'A', bar: 'b1', sum__num: 5 },
{ foo: 'B', bar: 'b2', sum__num: 5 },
],
},
],
theme: supersetTheme,
});
const result = transformProps(chartPropsConst as
EchartsTreemapChartProps);
expect((result.echartOptions as any).visualMap).toBeUndefined();
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Reasonable improvement — constructing an explicit formDataConst removes
ambiguity about what's being sent to transformProps and avoids relying on
shared mutable fixture state. Also switching to a proper typed access like
(result.echartOptions as any).visualMap removes the ts-ignore. It's a small
clarity and safety win for tests.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/test/Treemap/transformProps.test.ts
**Line:** 88:105
**Comment:**
*Possible Bug: Shared/shallow copy of `formData` can produce test
coupling or flakiness if `formData` is mutated elsewhere; also the test used
`// @ts-ignore` when asserting `visualMap` undefined. Build an explicit
`formDataConst` object with the fields the transform expects (instead of
shallow spread) and use a typed cast when checking `visualMap` to avoid
ts-ignore and accidental runtime access on unexpected shapes.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
