codeant-ai-for-open-source[bot] commented on code in PR #39012:
URL: https://github.com/apache/superset/pull/39012#discussion_r3023838034
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -682,9 +685,21 @@ export default function transformProps(
// For horizontal bar charts, set max/min from calculated data bounds
if (shouldCalculateDataBounds) {
- // Set max to actual data max to avoid gaps and ensure labels are visible
- if (dataMax !== undefined && yAxisMax === undefined) {
- yAxisMax = dataMax;
+ // For stacked charts, use the max stacked total to avoid clipping bars;
+ // for non-stacked charts, use the individual series max.
+ const effectiveDataMax = stack
+ ? Math.max(
+ ...sortedTotalValues.filter(
+ (v): v is number => typeof v === 'number' && !Number.isNaN(v),
+ ),
+ )
Review Comment:
**Suggestion:** The new stacked-axis max uses `sortedTotalValues` directly,
but those totals are algebraic sums per row. In mixed-sign stacks (positive +
negative values), positives and negatives cancel out, so the computed max can
become much smaller than the actual positive bar extent and clip stacked bars.
Compute the stacked max, then clamp it against `dataMax` so cancellation cannot
shrink the axis range below the largest visible segment. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Horizontal stacked bar with mixed signs mis-scales axis.
- ❌ Positive segments may render clipped or truncated.
- ⚠️ Affects `echarts_timeseries_bar` with truncate Y-axis.
```
</details>
```suggestion
const stackedTotalMax = Math.max(
...sortedTotalValues.filter(
(v): v is number => typeof v === 'number' && !Number.isNaN(v),
),
);
const effectiveDataMax = stack
? Math.max(dataMax ?? Number.NEGATIVE_INFINITY, stackedTotalMax)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render an `echarts_timeseries_bar` chart with horizontal orientation and
stacking
enabled so that `orientation === OrientationType.Horizontal`, `seriesType ===
EchartsTimeseriesSeriesType.Bar`, `stack === StackControlsValue.Stack`, and
`truncateYAxis
=== true` in `transformProps`
(`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts`,
around
lines 122–127 and 156–160), e.g. using the same `ChartProps` pattern as in
`test/Timeseries/Bar/transformProps.test.ts:51–58` but with `orientation:
'horizontal'`,
`stack: StackControlsValue.Stack`, and `truncateYAxis: true`.
2. Ensure the query result (`queriesData[0].data`) contains rows with
mixed-sign stacked
values, e.g. fields `A = 100` and `B = -80` in the same row;
`transformProps` receives
this data and calls `extractDataTotalValues` at
`superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:17–46`,
which computes
`totalStackedValues` by summing all non-x-axis fields algebraically (`value
= datum[curr]
|| 0; return prev + (value as number);`), so a row like `A=100, B=-80`
yields a total of
`20`.
3. `transformProps` then calls `extractSeries` at `utils/series.ts:61–99`
with
`totalStackedValues`, and the returned `sortedTotalValues` (the per-row
totals) are passed
back into `transformProps` (`Timeseries/transformProps.ts:60–75`). In the
same function,
`dataMax` is computed from individual series values (not totals) in the loop
at
`Timeseries/transformProps.ts:130–173`, so `dataMax` correctly reflects the
largest
positive segment (e.g. `100` from `A`).
4. When `shouldCalculateDataBounds` is true, the axis bounds logic at
`Timeseries/transformProps.ts:168–185` computes `effectiveDataMax` for
stacked charts as
`Math.max(...sortedTotalValues.filter(...))`, ignoring `dataMax`. For a
mixed-sign row
where the algebraic total is small (e.g. `20`) but one positive segment is
large (e.g.
`100`), `effectiveDataMax` becomes `20`, and because `yAxisMax` is set to
this value, the
resulting swapped horizontal axis cannot accommodate the full positive bar.
ECharts
receives an axis max below the largest positive segment, and the positive
portion of the
stacked bar is visibly clipped in the rendered horizontal stacked bar chart.
```
</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/Timeseries/transformProps.ts
**Line:** 690:695
**Comment:**
*Logic Error: The new stacked-axis max uses `sortedTotalValues`
directly, but those totals are algebraic sums per row. In mixed-sign stacks
(positive + negative values), positives and negatives cancel out, so the
computed max can become much smaller than the actual positive bar extent and
clip stacked bars. Compute the stacked max, then clamp it against `dataMax` so
cancellation cannot shrink the axis range below the largest visible segment.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39012&comment_hash=b95888701321110283ed0e482fb0b1baed38b355f33ec8f43019dad1e4957d81&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39012&comment_hash=b95888701321110283ed0e482fb0b1baed38b355f33ec8f43019dad1e4957d81&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]
