codeant-ai-for-open-source[bot] commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2674760341
##########
superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts:
##########
@@ -242,8 +242,10 @@ export default function transformProps(
// @ts-ignore
...outlierData,
];
- const addYAxisTitleOffset = !!yAxisTitle;
- const addXAxisTitleOffset = !!xAxisTitle;
+ const addYAxisTitleOffset =
+ !!yAxisTitle && convertInteger(yAxisTitleMargin) !== 0;
+ const addXAxisTitleOffset =
+ !!xAxisTitle && convertInteger(xAxisTitleMargin) !== 0;
Review Comment:
**Suggestion:** `convertInteger` is invoked multiple times (here and later
when building padding), which is redundant and can cause inconsistent results
if the conversion is non-deterministic; call `convertInteger` once per margin
and reuse the parsed integer values. [performance]
**Severity Level:** Minor ⚠️
```suggestion
const yAxisTitleMarginInt = convertInteger(yAxisTitleMargin);
const xAxisTitleMarginInt = convertInteger(xAxisTitleMargin);
const addYAxisTitleOffset =
!!yAxisTitle && yAxisTitleMarginInt !== 0;
const addXAxisTitleOffset =
!!xAxisTitle && xAxisTitleMarginInt !== 0;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid, practical improvement. convertInteger is called again later
when building chartPadding; parsing once into local
yAxisTitleMarginInt/xAxisTitleMarginInt and reusing those values avoids
duplicate work and ensures consistent behavior if convertInteger ever changes.
It's low risk and improves clarity.
</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/BoxPlot/transformProps.ts
**Line:** 245:248
**Comment:**
*Performance: `convertInteger` is invoked multiple times (here and
later when building padding), which is redundant and can cause inconsistent
results if the conversion is non-deterministic; call `convertInteger` once per
margin and reuse the parsed integer values.
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/MixedTimeseries/transformProps.ts:
##########
@@ -550,8 +550,11 @@ export default function transformProps(
? getXAxisFormatter(xAxisTimeFormat)
: String;
- const addYAxisTitleOffset = !!(yAxisTitle || yAxisTitleSecondary);
- const addXAxisTitleOffset = !!xAxisTitle;
+ const addYAxisTitleOffset =
+ !!(yAxisTitle || yAxisTitleSecondary) &&
+ convertInteger(yAxisTitleMargin) !== 0;
+ const addXAxisTitleOffset =
+ !!xAxisTitle && convertInteger(xAxisTitleMargin) !== 0;
Review Comment:
**Suggestion:** `convertInteger` is called multiple times for the same
margin values (here and later); compute the integer margins once and reuse them
to avoid repeated parsing and improve clarity. [performance]
**Severity Level:** Minor ⚠️
```suggestion
const yAxisTitleMarginInt = convertInteger(yAxisTitleMargin ?? 0);
const xAxisTitleMarginInt = convertInteger(xAxisTitleMargin ?? 0);
const addYAxisTitleOffset =
!!(yAxisTitle || yAxisTitleSecondary) && yAxisTitleMarginInt !== 0;
const addXAxisTitleOffset = !!xAxisTitle && xAxisTitleMarginInt !== 0;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Good suggestion — convertInteger is called multiple times for the same
inputs later
(chartPadding and axis nameGap). Converting once and reusing the integer
improves
clarity and avoids repeated parseInt calls. It's a small, safe optimization
with real benefit.
</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/MixedTimeseries/transformProps.ts
**Line:** 553:557
**Comment:**
*Performance: `convertInteger` is called multiple times for the same
margin values (here and later); compute the integer margins once and reuse them
to avoid repeated parsing and improve clarity.
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/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -559,6 +559,29 @@ function ExploreViewContainer(props) {
}
}
+ // Automatically set axis title margins when titles are added or removed
+ if (changedControlKeys.includes('x_axis_title')) {
+ const xAxisTitle = props.controls.x_axis_title?.value || '';
+ const currentMargin = props.controls.x_axis_title_margin?.value ?? 0;
Review Comment:
**Suggestion:** Type mismatch in numeric comparison: `currentMargin` may be
a string (e.g. "0" or "30") or another non-numeric value, so strict comparisons
`currentMargin === 0` and `currentMargin !== 0` can behave incorrectly. Convert
the control value to a number (with a safe fallback to 0) before comparing.
[type error]
**Severity Level:** Minor ⚠️
```suggestion
const xAxisTitleRaw = props.controls.x_axis_title?.value || '';
const xAxisTitle =
typeof xAxisTitleRaw === 'string' ? xAxisTitleRaw :
String(xAxisTitleRaw);
const currentMarginRaw = props.controls.x_axis_title_margin?.value;
const currentMargin = Number.isFinite(Number(currentMarginRaw))
? Number(currentMarginRaw)
: 0;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing comparisons (=== 0 / !== 0) can misbehave if the control stores
margin values as strings (e.g. "0" or "30"). Coercing/normalizing to Number
before comparing is a legitimate fix for a real logic bug that could leave
margins stuck in the wrong state.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
**Line:** 564:565
**Comment:**
*Type Error: Type mismatch in numeric comparison: `currentMargin` may
be a string (e.g. "0" or "30") or another non-numeric value, so strict
comparisons `currentMargin === 0` and `currentMargin !== 0` can behave
incorrectly. Convert the control value to a number (with a safe fallback to 0)
before comparing.
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/Timeseries/transformers.ts:
##########
@@ -656,7 +656,9 @@ export function getPadding(
top:
yAxisTitlePosition && yAxisTitlePosition === 'Top'
? TIMESERIES_CONSTANTS.gridOffsetTop + (Number(yAxisTitleMargin) ||
0)
- : TIMESERIES_CONSTANTS.gridOffsetTop + yAxisOffset,
+ : yAxisTitlePosition === 'Left'
+ ? TIMESERIES_CONSTANTS.gridOffsetTop
Review Comment:
**Suggestion:** Logic error: when the Y-axis title is positioned on the left
the top padding branch returns just `TIMESERIES_CONSTANTS.gridOffsetTop` and
ignores `yAxisOffset`, which means the Y-axis label top offset (when
`addYAxisTitleOffset` is true) will not be applied and can cause overlap of the
chart content with the axis title; include `yAxisOffset` in the 'Left' branch
so the top padding accounts for the offset. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
? TIMESERIES_CONSTANTS.gridOffsetTop + yAxisOffset
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The left-branch currently returns TIMESERIES_CONSTANTS.gridOffsetTop without
adding yAxisOffset,
so when addYAxisTitleOffset is true the top padding will be too small for
the Left case.
The improved code restores consistent behavior with the other branch
(including the yAxisOffset)
and fixes a real layout bug. This is a localized logic fix.
</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/transformers.ts
**Line:** 660:660
**Comment:**
*Logic Error: Logic error: when the Y-axis title is positioned on the
left the top padding branch returns just `TIMESERIES_CONSTANTS.gridOffsetTop`
and ignores `yAxisOffset`, which means the Y-axis label top offset (when
`addYAxisTitleOffset` is true) will not be applied and can cause overlap of the
chart content with the axis title; include `yAxisOffset` in the 'Left' branch
so the top padding accounts for the offset.
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/transformers.test.ts:
##########
@@ -190,3 +194,149 @@ describe('transformNegativeLabelsPosition', () => {
expect((result as any)[4].label).toBe(undefined);
});
});
+
+describe('getPadding', () => {
+ let getChartPaddingSpy: jest.SpyInstance;
+
+ beforeEach(() => {
+ // Mock getChartPadding to return the padding object as-is for easier
testing
+ getChartPaddingSpy = jest.spyOn(seriesUtils, 'getChartPadding');
+ getChartPaddingSpy.mockImplementation(
+ (
+ show: boolean,
+ orientation: LegendOrientation,
+ margin: string | number | null | undefined,
+ padding:
+ | {
+ bottom?: number;
+ left?: number;
+ right?: number;
+ top?: number;
+ }
+ | undefined,
+ ) => {
+ return {
+ bottom: padding?.bottom ?? 0,
+ left: padding?.left ?? 0,
+ right: padding?.right ?? 0,
+ top: padding?.top ?? 0,
+ };
+ },
+ );
+ });
+
+ afterEach(() => {
+ getChartPaddingSpy.mockRestore();
Review Comment:
**Suggestion:** `afterEach` calls `getChartPaddingSpy.mockRestore()`
unguarded; if the spy wasn't created (e.g., beforeEach failed), calling
`mockRestore` will throw. Guard the restore call to run only when the spy is
defined to make the teardown robust. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
if (getChartPaddingSpy) {
getChartPaddingSpy.mockRestore();
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a pragmatic, defensive fix for the test teardown. If beforeEach
throws and the spy isn't assigned,
calling mockRestore on undefined will throw and obscure the original
failure. Guarding the restore makes
the teardown robust and avoids noisy errors in CI. This addresses a real
risk in test execution, not just a
stylistic preference.
</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/transformers.test.ts
**Line:** 229:229
**Comment:**
*Possible Bug: `afterEach` calls `getChartPaddingSpy.mockRestore()`
unguarded; if the spy wasn't created (e.g., beforeEach failed), calling
`mockRestore` will throw. Guard the restore call to run only when the spy is
defined to make the teardown robust.
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]
