codeant-ai-for-open-source[bot] commented on code in PR #42300:
URL: https://github.com/apache/superset/pull/42300#discussion_r3680950064
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -724,6 +724,57 @@ export default function transformProps(
}
}
+ // Whenever a Y axis bound is defined, whether explicitly configured or
+ // derived above from the data, clamp series values to those bounds
+ // instead of leaving raw out-of-range values in place. ECharts axis
+ // clipping can otherwise drop an out-of-bounds point (and the line
+ // segments around it) entirely rather than truncating it at the
+ // boundary (see https://github.com/apache/superset/issues/27449).
+ if (yAxisMin !== undefined || yAxisMax !== undefined) {
+ const valueIndex = isHorizontal ? 0 : 1;
+ type AxisValue = string | number | null | undefined;
+ type AxisPoint = AxisValue[];
+ const clampAxisValue = (value: AxisValue): AxisValue => {
+ if (typeof value !== 'number' || Number.isNaN(value)) return value;
+ let clamped = value;
+ if (yAxisMin !== undefined) clamped = Math.max(clamped, yAxisMin);
+ if (yAxisMax !== undefined) clamped = Math.min(clamped, yAxisMax);
Review Comment:
**Suggestion:** Stacked series are clamped independently before ECharts
applies stacking. For example, two segments valued at 8 with a maximum of 10
remain 8 each, but their cumulative stack reaches 16 and is still outside the
axis; conversely, clamping individual segments can alter stack totals and
percentage calculations. The bounds need to be applied to the rendered
cumulative stack geometry or this rewrite must be limited to unstacked series.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Stacked area and bar geometry remains incorrectly clipped.
- ⚠️ Segment totals and percentages can reflect altered values.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=824838fa4c2b4e778b337123658ed842&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=824838fa4c2b4e778b337123658ed842&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:** 740:741
**Comment:**
*Logic Error: Stacked series are clamped independently before ECharts
applies stacking. For example, two segments valued at 8 with a maximum of 10
remain 8 each, but their cumulative stack reaches 16 and is still outside the
axis; conversely, clamping individual segments can alter stack totals and
percentage calculations. The bounds need to be applied to the rendered
cumulative stack geometry or this rewrite must be limited to unstacked series.
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%2F42300&comment_hash=b02505244211ad97156745a677996241d43156070e292e2588dc80c9a9dc8a6e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42300&comment_hash=b02505244211ad97156745a677996241d43156070e292e2588dc80c9a9dc8a6e&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -724,6 +724,57 @@ export default function transformProps(
}
}
+ // Whenever a Y axis bound is defined, whether explicitly configured or
+ // derived above from the data, clamp series values to those bounds
+ // instead of leaving raw out-of-range values in place. ECharts axis
+ // clipping can otherwise drop an out-of-bounds point (and the line
+ // segments around it) entirely rather than truncating it at the
+ // boundary (see https://github.com/apache/superset/issues/27449).
+ if (yAxisMin !== undefined || yAxisMax !== undefined) {
+ const valueIndex = isHorizontal ? 0 : 1;
+ type AxisValue = string | number | null | undefined;
+ type AxisPoint = AxisValue[];
+ const clampAxisValue = (value: AxisValue): AxisValue => {
+ if (typeof value !== 'number' || Number.isNaN(value)) return value;
+ let clamped = value;
+ if (yAxisMin !== undefined) clamped = Math.max(clamped, yAxisMin);
+ if (yAxisMax !== undefined) clamped = Math.min(clamped, yAxisMax);
+ return clamped;
+ };
+ const clampPoint = (point: AxisPoint): AxisPoint => {
+ const newPoint = [...point];
+ newPoint[valueIndex] = clampAxisValue(newPoint[valueIndex]);
+ return newPoint;
+ };
+ series.forEach(s => {
+ if (!Array.isArray(s.data)) return;
Review Comment:
**Suggestion:** The loop includes timeseries annotation series, whose
ordinary `[x, y]` points are appended to `series` before this block. An
annotation value outside the chart bounds is therefore permanently rewritten to
the bound, changing the annotation's location instead of merely clipping the
observation series. Restrict clamping to query-derived observation series and
leave annotation series unchanged. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Formula annotations can appear at incorrect Y coordinates.
- ⚠️ Timeseries annotation locations and tooltips become inaccurate.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6cb4480938ff4f5b86258a693dfb1710&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6cb4480938ff4f5b86258a693dfb1710&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:** 749:750
**Comment:**
*Logic Error: The loop includes timeseries annotation series, whose
ordinary `[x, y]` points are appended to `series` before this block. An
annotation value outside the chart bounds is therefore permanently rewritten to
the bound, changing the annotation's location instead of merely clipping the
observation series. Restrict clamping to query-derived observation series and
leave annotation series unchanged.
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%2F42300&comment_hash=0a3920dc1e4c689c929e434f51d992a7939566a4ca37b8f060d1a8821b7e8d1a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42300&comment_hash=0a3920dc1e4c689c929e434f51d992a7939566a4ca37b8f060d1a8821b7e8d1a&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -724,6 +724,57 @@ export default function transformProps(
}
}
+ // Whenever a Y axis bound is defined, whether explicitly configured or
+ // derived above from the data, clamp series values to those bounds
+ // instead of leaving raw out-of-range values in place. ECharts axis
+ // clipping can otherwise drop an out-of-bounds point (and the line
+ // segments around it) entirely rather than truncating it at the
+ // boundary (see https://github.com/apache/superset/issues/27449).
+ if (yAxisMin !== undefined || yAxisMax !== undefined) {
+ const valueIndex = isHorizontal ? 0 : 1;
+ type AxisValue = string | number | null | undefined;
+ type AxisPoint = AxisValue[];
+ const clampAxisValue = (value: AxisValue): AxisValue => {
+ if (typeof value !== 'number' || Number.isNaN(value)) return value;
+ let clamped = value;
+ if (yAxisMin !== undefined) clamped = Math.max(clamped, yAxisMin);
+ if (yAxisMax !== undefined) clamped = Math.min(clamped, yAxisMax);
+ return clamped;
+ };
+ const clampPoint = (point: AxisPoint): AxisPoint => {
+ const newPoint = [...point];
+ newPoint[valueIndex] = clampAxisValue(newPoint[valueIndex]);
+ return newPoint;
+ };
+ series.forEach(s => {
+ if (!Array.isArray(s.data)) return;
+ const clampedData = (
+ s.data as (AxisPoint | Record<string, unknown>)[]
+ ).map(point => {
+ if (Array.isArray(point)) {
+ return clampPoint(point);
+ }
+ // Some series paths (e.g. colorByPrimaryAxis, or negative bar
+ // label positioning) wrap the tuple in an object of the shape
+ // `{ value: [x, y], ... }` instead of passing the tuple
+ // directly; clamp the wrapped tuple in place so those points
+ // aren't skipped and left to be dropped by ECharts axis clipping.
+ if (
+ point &&
+ typeof point === 'object' &&
+ Array.isArray((point as { value?: unknown }).value)
+ ) {
+ return {
+ ...point,
+ value: clampPoint((point as { value: AxisPoint }).value),
+ };
+ }
+ return point;
+ });
+ s.data = clampedData as typeof s.data;
Review Comment:
**Suggestion:** This mutates the values used by ECharts tooltips and labels,
so an original observation such as `1000` is exposed to the formatter as `10`
when the configured maximum is `10`. The chart should preserve the source value
for tooltip and label content while using a separate clipped value for
rendering, or explicitly retain the original value in the data item. [api
mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Tooltips report truncated values as source observations.
- ⚠️ Data labels and stacked totals can become misleading.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2d955762601043b1877b23194f838b35&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2d955762601043b1877b23194f838b35&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:** 773:774
**Comment:**
*Api Mismatch: This mutates the values used by ECharts tooltips and
labels, so an original observation such as `1000` is exposed to the formatter
as `10` when the configured maximum is `10`. The chart should preserve the
source value for tooltip and label content while using a separate clipped value
for rendering, or explicitly retain the original value in the data item.
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%2F42300&comment_hash=cb51c398f177fc688f55c4ca73d395d161044cbced2cfc1256317a604a100b6d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42300&comment_hash=cb51c398f177fc688f55c4ca73d395d161044cbced2cfc1256317a604a100b6d&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]
