codeant-ai-for-open-source[bot] commented on code in PR #36989:
URL: https://github.com/apache/superset/pull/36989#discussion_r2674130728
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -479,6 +480,39 @@ export default function transformProps(
yAxisMin = calculateLowerLogTick(minPositiveValue);
}
+ // For horizontal bar charts, calculate max from data to avoid cutting off
labels
+ if (
+ isHorizontal &&
+ seriesType === EchartsTimeseriesSeriesType.Bar &&
+ truncateYAxis
+ ) {
+ let dataMax: number | undefined;
+ let dataMin: number | undefined;
+ rawSeries.forEach(s => {
+ if (s.data && Array.isArray(s.data)) {
+ (s.data as [number, any][]).forEach((datum: [number, any]) => {
+ const value = datum[0];
Review Comment:
**Suggestion:** Logic error: the code reads the numeric value from
`datum[0]` but the numeric value for timeseries points is at index 1 (the
second element) - using index 0 will pick the x/category value instead of the
numeric y value and lead to incorrect min/max computation. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
(s.data as [any, any][]).forEach((datum: [any, any]) => {
const value = datum[1];
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a real logic bug: elsewhere in the file series rows are treated as
[x, y] (see the stream handling that references row[1] as the numeric value).
Using datum[0] reads the x/category/time value, not the numeric y, so min/max
will be wrong for horizontal bars. Changing to read the numeric element
(datum[1]) fixes the bug. The suggested type loosen (to any) in the improved
snippet is reasonable for this isolated hunk.
</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:** 493:494
**Comment:**
*Logic Error: Logic error: the code reads the numeric value from
`datum[0]` but the numeric value for timeseries points is at index 1 (the
second element) - using index 0 will pick the x/category value instead of the
numeric y value and lead to incorrect min/max computation.
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/constants.ts:
##########
@@ -45,6 +45,8 @@ export const TIMESERIES_CONSTANTS = {
dataZoomEnd: 100,
yAxisLabelTopOffset: 20,
extraControlsOffset: 22,
+ // Minimum right padding for horizontal bar charts to ensure value labels
are fully visible
Review Comment:
**Suggestion:** The new property lives on a TIMESERIES-specific constants
object but documents horizontal bar chart behavior; this mixes concerns and can
confuse maintainers—annotate intent clearly so future changes don't
accidentally remove or misuse it. [maintainability]
**Severity Level:** Minor ⚠️
```suggestion
/** Minimum right padding (in pixels) used by horizontal bar chart
renderers to ensure value labels are fully visible.
* NOTE: This is specific to horizontal bar charts; keep here only if
timeseries consumers intentionally reuse it.
*/
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Adding a JSDoc that explains this constant is specific to horizontal bar
charts and its units is a helpful, non-breaking maintainability improvement.
It protects future maintainers from accidentally removing or reusing the
constant inappropriately.
</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/constants.ts
**Line:** 48:48
**Comment:**
*Maintainability: The new property lives on a TIMESERIES-specific
constants object but documents horizontal bar chart behavior; this mixes
concerns and can confuse maintainers—annotate intent clearly so future changes
don't accidentally remove or misuse it.
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/Timeseries/transformProps.test.ts:
##########
@@ -723,3 +727,171 @@ describe('legend sorting', () => {
]);
});
});
+
+describe('Horizontal bar chart axis bounds', () => {
+ const baseFormData: SqlaFormData = {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ granularity_sqla: '__timestamp',
+ metric: 'sum__num',
+ groupby: [],
+ viz_type: 'echarts_timeseries',
+ seriesType: EchartsTimeseriesSeriesType.Bar,
+ orientation: OrientationType.Horizontal,
+ truncateYAxis: true,
+ yAxisBounds: [null, null],
+ };
+
+ it('should set yAxis max to actual data max for horizontal bar charts', ()
=> {
+ const queriesData = [
+ {
+ data: [
+ { 'Series A': 15000, __timestamp: 599616000000 },
+ { 'Series A': 20000, __timestamp: 599916000000 },
+ { 'Series A': 18000, __timestamp: 600216000000 },
+ ],
+ },
+ ];
+
+ const chartProps = new ChartProps({
+ formData: baseFormData,
+ width: 800,
+ height: 600,
+ queriesData,
+ theme: supersetTheme,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+
+ // In horizontal orientation, axes are swapped, so yAxis becomes xAxis
+ const xAxis = transformedProps.echartOptions.xAxis as any;
+ expect(xAxis.max).toBe(20000); // Should be the actual max value, not
rounded
+ });
+
+ it('should set yAxis min and max for diverging horizontal bar charts', () =>
{
+ const queriesData = [
+ {
+ data: [
+ { 'Series A': -21000, __timestamp: 599616000000 },
+ { 'Series A': 20000, __timestamp: 599916000000 },
+ { 'Series A': 18000, __timestamp: 600216000000 },
+ ],
+ },
+ ];
+
+ const chartProps = new ChartProps({
+ formData: baseFormData,
+ width: 800,
+ height: 600,
+ queriesData,
+ theme: supersetTheme,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+
+ // In horizontal orientation, axes are swapped, so yAxis becomes xAxis
+ const xAxis = transformedProps.echartOptions.xAxis as any;
+ expect(xAxis.max).toBe(20000); // Should be the actual max value
+ expect(xAxis.min).toBe(-21000); // Should be the actual min value for
diverging bars
+ });
+
+ it('should not override explicit yAxisBounds', () => {
+ const queriesData = [
+ {
+ data: [
+ { 'Series A': 15000, __timestamp: 599616000000 },
+ { 'Series A': 20000, __timestamp: 599916000000 },
+ { 'Series A': 18000, __timestamp: 600216000000 },
+ ],
+ },
+ ];
+
+ const chartProps = new ChartProps({
+ formData: {
+ ...baseFormData,
+ yAxisBounds: [0, 25000], // Explicit bounds
+ },
+ width: 800,
+ height: 600,
+ queriesData,
+ theme: supersetTheme,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+
+ // In horizontal orientation, axes are swapped, so yAxis becomes xAxis
+ const xAxis = transformedProps.echartOptions.xAxis as any;
Review Comment:
**Suggestion:** A third test in this new describe block also reads
`echartOptions.xAxis` directly; make the same defensive change here (extract
first element if `xAxis` is an array). This prevents intermittent failures when
transformProps produces an axis array. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const xAxisRaw = transformedProps.echartOptions.xAxis as any;
const xAxis = Array.isArray(xAxisRaw) ? xAxisRaw[0] : xAxisRaw;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Repeating the same defensive change for the third occurrence is valid. The
proposed Array.isArray guard is correct and prevents assertions from
accidentally reading properties off the array instead of the axis object.
</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/Timeseries/transformProps.test.ts
**Line:** 829:829
**Comment:**
*Logic Error: A third test in this new describe block also reads
`echartOptions.xAxis` directly; make the same defensive change here (extract
first element if `xAxis` is an array). This prevents intermittent failures when
transformProps produces an axis array.
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/Timeseries/transformProps.test.ts:
##########
@@ -723,3 +727,171 @@ describe('legend sorting', () => {
]);
});
});
+
+describe('Horizontal bar chart axis bounds', () => {
+ const baseFormData: SqlaFormData = {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ granularity_sqla: '__timestamp',
+ metric: 'sum__num',
+ groupby: [],
+ viz_type: 'echarts_timeseries',
+ seriesType: EchartsTimeseriesSeriesType.Bar,
+ orientation: OrientationType.Horizontal,
+ truncateYAxis: true,
+ yAxisBounds: [null, null],
+ };
+
+ it('should set yAxis max to actual data max for horizontal bar charts', ()
=> {
+ const queriesData = [
+ {
+ data: [
+ { 'Series A': 15000, __timestamp: 599616000000 },
+ { 'Series A': 20000, __timestamp: 599916000000 },
+ { 'Series A': 18000, __timestamp: 600216000000 },
+ ],
+ },
+ ];
+
+ const chartProps = new ChartProps({
+ formData: baseFormData,
+ width: 800,
+ height: 600,
+ queriesData,
+ theme: supersetTheme,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+
+ // In horizontal orientation, axes are swapped, so yAxis becomes xAxis
+ const xAxis = transformedProps.echartOptions.xAxis as any;
Review Comment:
**Suggestion:** The test assumes `echartOptions.xAxis` is an object and
reads `xAxis.max` directly, but ECharts can return `xAxis` as an array (or as a
single object). If `xAxis` is an array the test will access properties on the
array instead of the axis object. Safely extract the first axis when `xAxis` is
an array before asserting `max`. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const xAxisRaw = transformedProps.echartOptions.xAxis as any;
const xAxis = Array.isArray(xAxisRaw) ? xAxisRaw[0] : xAxisRaw;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
ECharts API can represent axes as an array or a single object. The test
currently assumes a single object and will break if transformProps returns an
axis array. The improved code defensively extracts the first axis when
necessary and prevents flaky failures — this fixes a real, verifiable risk
visible in the test.
</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/Timeseries/transformProps.test.ts
**Line:** 769:769
**Comment:**
*Logic Error: The test assumes `echartOptions.xAxis` is an object and
reads `xAxis.max` directly, but ECharts can return `xAxis` as an array (or as a
single object). If `xAxis` is an array the test will access properties on the
array instead of the axis object. Safely extract the first axis when `xAxis` is
an array before asserting `max`.
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/Timeseries/transformProps.test.ts:
##########
@@ -723,3 +727,171 @@ describe('legend sorting', () => {
]);
});
});
+
+describe('Horizontal bar chart axis bounds', () => {
+ const baseFormData: SqlaFormData = {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ granularity_sqla: '__timestamp',
+ metric: 'sum__num',
+ groupby: [],
+ viz_type: 'echarts_timeseries',
+ seriesType: EchartsTimeseriesSeriesType.Bar,
+ orientation: OrientationType.Horizontal,
+ truncateYAxis: true,
+ yAxisBounds: [null, null],
+ };
+
+ it('should set yAxis max to actual data max for horizontal bar charts', ()
=> {
+ const queriesData = [
+ {
+ data: [
+ { 'Series A': 15000, __timestamp: 599616000000 },
+ { 'Series A': 20000, __timestamp: 599916000000 },
+ { 'Series A': 18000, __timestamp: 600216000000 },
+ ],
+ },
+ ];
+
+ const chartProps = new ChartProps({
+ formData: baseFormData,
+ width: 800,
+ height: 600,
+ queriesData,
+ theme: supersetTheme,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+
+ // In horizontal orientation, axes are swapped, so yAxis becomes xAxis
+ const xAxis = transformedProps.echartOptions.xAxis as any;
+ expect(xAxis.max).toBe(20000); // Should be the actual max value, not
rounded
+ });
+
+ it('should set yAxis min and max for diverging horizontal bar charts', () =>
{
+ const queriesData = [
+ {
+ data: [
+ { 'Series A': -21000, __timestamp: 599616000000 },
+ { 'Series A': 20000, __timestamp: 599916000000 },
+ { 'Series A': 18000, __timestamp: 600216000000 },
+ ],
+ },
+ ];
+
+ const chartProps = new ChartProps({
+ formData: baseFormData,
+ width: 800,
+ height: 600,
+ queriesData,
+ theme: supersetTheme,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+
+ // In horizontal orientation, axes are swapped, so yAxis becomes xAxis
+ const xAxis = transformedProps.echartOptions.xAxis as any;
Review Comment:
**Suggestion:** Another test block reuses the same unsafe assumption when
reading `echartOptions.xAxis`; replace the direct access with a safe extraction
that handles either an array or an object so assertions on `min`/`max` are
valid. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const xAxisRaw = transformedProps.echartOptions.xAxis as any;
const xAxis = Array.isArray(xAxisRaw) ? xAxisRaw[0] : xAxisRaw;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same as the previous: multiple tests in the new block read
echartOptions.xAxis directly. Making the access robust to either an array or
object addresses a likely source of intermittent test failures and is
straightforward to apply.
</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/Timeseries/transformProps.test.ts
**Line:** 797:797
**Comment:**
*Logic Error: Another test block reuses the same unsafe assumption when
reading `echartOptions.xAxis`; replace the direct access with a safe extraction
that handles either an array or an object so assertions on `min`/`max` are
valid.
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]
