codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3400625645
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -337,6 +342,89 @@ export default function transformProps(
xAxisType,
},
);
+
+ // Dot size by metric (scatter): the size metric's series are excluded from
+ // rendering and instead provide per-point values that scale each marker's
+ // area between minMarkerSize and maxMarkerSize.
+ const sizeMetricLabel =
+ seriesType === EchartsTimeseriesSeriesType.Scatter && size
+ ? getMetricLabel(size)
+ : undefined;
+ const sizeSeriesLabel = isDefined(sizeMetricLabel)
+ ? (verboseMap[sizeMetricLabel!] ?? sizeMetricLabel)
+ : undefined;
+ const valueMetricLabels = ensureIsArray(metrics)
+ .map(getMetricLabel)
+ .map(label => verboseMap[label] ?? label);
+ // When the size metric is also a value metric, the query dedupes them into a
+ // single column, so each point's own value doubles as its size value.
+ const sizeIsValueMetric = isDefined(sizeSeriesLabel)
+ ? valueMetricLabels.includes(sizeSeriesLabel!)
+ : false;
+ const isSizeSeries = (name: string) =>
+ isDefined(sizeSeriesLabel) &&
+ !sizeIsValueMetric &&
+ (name === sizeSeriesLabel || name.startsWith(`${sizeSeriesLabel}, `));
+ const rawSeries = sizeSeriesLabel
+ ? allRawSeries.filter(entry => !isSizeSeries(String(entry.name ?? '')))
+ : allRawSeries;
+ // Maps each value series' dimension key to a lookup from primary-axis value
+ // to size value.
+ let sizeLookups: Map<string, Map<DataRecordValue, number>> | undefined;
+ let sizeExtent: [number, number] | undefined;
+ if (sizeSeriesLabel) {
+ let sizeMin = Infinity;
+ let sizeMax = -Infinity;
+ if (sizeIsValueMetric) {
+ rawSeries.forEach(entry => {
+ (entry.data as DataRecordValue[][]).forEach(datum => {
+ const sizeValue = isHorizontal ? datum[0] : datum[1];
+ if (typeof sizeValue === 'number' && Number.isFinite(sizeValue)) {
+ sizeMin = Math.min(sizeMin, sizeValue);
+ sizeMax = Math.max(sizeMax, sizeValue);
+ }
+ });
+ });
+ } else {
+ sizeLookups = new Map();
+ allRawSeries
+ .filter(entry => isSizeSeries(String(entry.name ?? '')))
+ .forEach(entry => {
+ const name = String(entry.name ?? '');
+ const dimsKey =
+ name === sizeSeriesLabel
+ ? ''
+ : name.slice(sizeSeriesLabel.length + 2);
+ const lookup = new Map<DataRecordValue, number>();
+ (entry.data as DataRecordValue[][]).forEach(datum => {
+ const axisValue = isHorizontal ? datum[1] : datum[0];
+ const sizeValue = isHorizontal ? datum[0] : datum[1];
+ if (typeof sizeValue === 'number' && Number.isFinite(sizeValue)) {
+ lookup.set(axisValue, sizeValue);
+ sizeMin = Math.min(sizeMin, sizeValue);
+ sizeMax = Math.max(sizeMax, sizeValue);
+ }
+ });
+ sizeLookups!.set(dimsKey, lookup);
+ });
+ }
+ if (sizeMin <= sizeMax) {
+ sizeExtent = [sizeMin, sizeMax];
+ }
+ }
+ // Strips the metric label off a series name, leaving the dimension key used
+ // to match a value series with its size series.
+ const getSeriesDimsKey = (name: string): string => {
+ const matchedLabel = valueMetricLabels.find(
+ label => name === label || name.startsWith(`${label}, `),
+ );
+ if (matchedLabel === undefined) {
+ return name;
+ }
+ return name === matchedLabel ? '' : name.slice(matchedLabel.length + 2);
+ };
+ const markerSizeRange: [number, number] = [minMarkerSize, maxMarkerSize];
Review Comment:
**Suggestion:** The code passes `minMarkerSize` and `maxMarkerSize` through
without normalizing or validating their order, so if users set minimum larger
than maximum the scaling inverts (larger metric values render smaller dots).
Normalize the range before use (or enforce min <= max in controls) to preserve
the documented semantics. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Higher size metrics can render with smaller dots.
- ⚠️ Dot-size controls allow contradictory min/max configuration.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the "Generic Chart" Scatter variant, set a "Dot size metric" and then
use the
"Minimum dot size" and "Maximum dot size" sliders configured in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:59-89`.
The UI allows choosing a minimum larger than the maximum because the two
sliders are
independent and have no mutual validation.
2. Save or run the chart so that form data flows into `transformProps` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:20-82`,
where `minMarkerSize` and `maxMarkerSize` are destructured from
`EchartsTimeseriesFormData` without adjustment (defaults at
`constants.ts:64-68` but
overridden by the user's selections).
3. `transformProps.ts:419-427` constructs `markerSizeRange: [number, number]
=
[minMarkerSize, maxMarkerSize];` (diff line 426) and later builds
`symbolSizeFn` at
`transformProps.ts:145-159`, which calls `getAreaScaledSymbolSize(sizeValue,
extent,
markerSizeRange)` for each data point when `sizeExtent` is defined.
4. `getAreaScaledSymbolSize` in
`superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:6-23`
interprets
`sizeRange` as `[minSize, maxSize]` and computes `sqrt(minSize ** 2 + ratio
* (maxSize **
2 - minSize ** 2))`. When `minMarkerSize > maxMarkerSize`, `maxSize ** 2 -
minSize ** 2`
is negative, so as `ratio` (and the metric value) increases, the returned
size decreases,
causing larger metric values to render with smaller dots, inverting the
intended sizing
semantics.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e356e02b3a274dcf935e7c20a2f6a766&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e356e02b3a274dcf935e7c20a2f6a766&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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/Timeseries/transformProps.ts
**Line:** 426:426
**Comment:**
*Incorrect Condition Logic: The code passes `minMarkerSize` and
`maxMarkerSize` through without normalizing or validating their order, so if
users set minimum larger than maximum the scaling inverts (larger metric values
render smaller dots). Normalize the range before use (or enforce min <= max in
controls) to preserve the documented semantics.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=4917579fa5e0ffbb675b4e7a2ebadcf6df92232bfb8dda9471109588dae57134&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=4917579fa5e0ffbb675b4e7a2ebadcf6df92232bfb8dda9471109588dae57134&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -472,6 +560,24 @@ export default function transformProps(
}
}
+ let symbolSizeFn:
+ | ((value: (number | string | null)[]) => number)
+ | undefined;
+ if (sizeExtent) {
+ const extent = sizeExtent;
+ const sizeLookup = sizeLookups?.get(getSeriesDimsKey(entryName));
+ if (sizeIsValueMetric || sizeLookup) {
+ symbolSizeFn = value => {
+ const sizeValue = sizeIsValueMetric
+ ? value[isHorizontal ? 0 : 1]
+ : sizeLookup!.get(value[isHorizontal ? 1 : 0]);
+ return typeof sizeValue === 'number'
+ ? getAreaScaledSymbolSize(sizeValue, extent, markerSizeRange)
+ : markerSize;
Review Comment:
**Suggestion:** Points without a valid size metric currently fall back to
`markerSize`, but that control is hidden when a dot-size metric is selected.
This makes fallback sizing depend on a stale/hidden value instead of the
configured dot-size range and produces inconsistent marker sizes for
null/missing size values. Use `minMarkerSize` (or the lower bound of the
configured range) as the fallback so missing values follow the visible dot-size
controls. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Scatter chart sized dots render inconsistent for null sizes.
- ⚠️ Hidden markerSize overrides visible dot-size configuration.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or edit a "Generic Chart" using the ECharts timeseries plugin,
select the
Scatter series type, and configure a "Dot size metric" in the control panel
defined in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:24-37`
(size metric row) and `:55-89` (minMarkerSize/maxMarkerSize sliders).
2. Ensure your query returns some points where the dot-size metric is null
or missing
(e.g., some dimension rows lack that metric). These values flow into
`extractSeries()` and
then into the size-mapping logic in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:147-215`,
where `sizeLookups` and `sizeExtent` are computed only for finite numeric
size values.
3. When `rawSeries.forEach` runs in `transformProps.ts:64-161`, the code
constructs
`symbolSizeFn` at `transformProps.ts:145-159`. For a point whose size metric
is
null/undefined or absent from `sizeLookup`, `sizeValue` is not a number, so
the ternary at
`transformProps.ts:156-159` returns `markerSize` instead of a value derived
from
`[minMarkerSize, maxMarkerSize]`.
4. `transformSeries()` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:197-201,239-243,159`
receives both `markerSize` and `symbolSizeFn`; it assigns `symbolSize:
symbolSizeFn ??
markerSize`, so such points are rendered using `markerSize`. However, when a
dot-size
metric is set, the `markerSize` slider is hidden (Scatter control panel
visibility logic
at `Regular/Scatter/controlPanel.tsx:39-54`), while only
`minMarkerSize`/`maxMarkerSize`
are visible (`:59-89`), causing points with missing size values to use a
hidden/stale
`markerSize` that can fall outside the configured dot-size range.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3f4459092f1b449ca6e8aceaff338daa&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3f4459092f1b449ca6e8aceaff338daa&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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/Timeseries/transformProps.ts
**Line:** 574:576
**Comment:**
*Logic Error: Points without a valid size metric currently fall back to
`markerSize`, but that control is hidden when a dot-size metric is selected.
This makes fallback sizing depend on a stale/hidden value instead of the
configured dot-size range and produces inconsistent marker sizes for
null/missing size values. Use `minMarkerSize` (or the lower bound of the
configured range) as the fallback so missing values follow the visible dot-size
controls.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=1419211f6171cf5b113e61c44f741e6d7e16ca3be3781bd4f8e66c974fa18042&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=1419211f6171cf5b113e61c44f741e6d7e16ca3be3781bd4f8e66c974fa18042&reaction=dislike'>👎</a>
--
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]
