codeant-ai-for-open-source[bot] commented on code in PR #35459:
URL: https://github.com/apache/superset/pull/35459#discussion_r2678595996
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/controlPanel.tsx:
##########
@@ -118,6 +119,36 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'start_angle',
+ config: {
+ type: 'NumberControl',
+ min: 0,
+ max: 360,
+ step: 10,
+ label: t('Start angle'),
+ description: t('Angle at which to start progress axis'),
+ renderTrigger: true,
+ default: DEFAULT_FORM_DATA.startAngle,
+ },
+ },
+ ],
+ [
+ {
+ name: 'swept_angle',
Review Comment:
**Suggestion:** Control keys were added as snake_case (`start_angle`,
`swept_angle`) while the codebase (and DEFAULT_FORM_DATA) use camelCase
properties (e.g., `startAngle`, `sweptAngle`); this will store user input under
different formData keys than the visualization expects and changes will have no
effect. Rename the control `name` fields to camelCase to match formData
property names. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
name: 'startAngle',
config: {
type: 'NumberControl',
min: 0,
max: 360,
step: 10,
label: t('Start angle'),
description: t('Angle at which to start progress axis'),
renderTrigger: true,
default: DEFAULT_FORM_DATA.startAngle,
},
},
],
[
{
name: 'sweptAngle',
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The controls will populate formData keys named `start_angle` and
`swept_angle`, while the rest of the code (and DEFAULT_FORM_DATA) expects
camelCase `startAngle`/`sweptAngle`. That mismatch means user changes won't be
read by visualization code. Renaming control `name` to camelCase fixes the
mapping issue.
</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/Pie/controlPanel.tsx
**Line:** 124:139
**Comment:**
*Logic Error: Control keys were added as snake_case (`start_angle`,
`swept_angle`) while the codebase (and DEFAULT_FORM_DATA) use camelCase
properties (e.g., `startAngle`, `sweptAngle`); this will store user input under
different formData keys than the visualization expects and changes will have no
effect. Rename the control `name` fields to camelCase to match formData
property names.
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/Pie/controlPanel.tsx:
##########
@@ -41,6 +41,7 @@ const {
numberFormat,
showLabels,
roseType,
+ half,
Review Comment:
**Suggestion:** Unused destructured property `half` is introduced but never
used; this will trigger linter warnings and is confusing (likely the author
intended to add a control or remove the destructure). Remove `half` from the
DEFAULT_FORM_DATA destructuring if it's not needed, or add the corresponding
control UI where `half` is consumed. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The destructured `half` variable is not referenced anywhere in the final
file — it will trigger an unused-variable lint warning and adds noise. Removing
it is safe and a clean-up: no runtime or logic change.
</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/Pie/controlPanel.tsx
**Line:** 44:44
**Comment:**
*Possible Bug: Unused destructured property `half` is introduced but
never used; this will trigger linter warnings and is confusing (likely the
author intended to add a control or remove the destructure). Remove `half` from
the DEFAULT_FORM_DATA destructuring if it's not needed, or add the
corresponding control UI where `half` is consumed.
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/Pie/transformProps.ts:
##########
@@ -95,26 +97,36 @@ function getTotalValuePadding({
top: donut ? 'middle' : '0',
left: 'center',
};
+ // When half, the donut is centered at 70% - 1% so that the total does not
go beyond the limits
+ const HALF_DONUT_BASE_POSITION = 69;
+ // When halfway, the donut is centered at 70% - 1% to keep the total from
going over the limit - 0.5% legend offset
+ const HALF_DONUT_TOP_OFFSET = 68.5;
+ // When donut, the donut is centered at 50%
+ const DONUT_OFFSET = 50;
+ const getDonutBase = () => (half ? HALF_DONUT_TOP_OFFSET : DONUT_OFFSET);
+ if (half) {
Review Comment:
**Suggestion:** Division by zero when computing percentage offsets: the code
divides chartPadding by `height` and `width` without guarding against zero or
negative dimensions, which can produce Infinity/NaN in the computed `%` strings
and break layout; return early or use safe defaults when dimensions are
invalid. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
// Guard against zero or invalid dimensions to avoid division by zero
producing Infinity/NaN
if (height <= 0 || width <= 0) {
return padding;
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The code performs arithmetic dividing by height/width which can produce
Infinity/NaN if those dimensions are 0. Guarding early (return default padding)
is a small, defensive change that prevents layout breakage in edge cases and is
safe here because the default padding is already a sensible fallback.
</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/Pie/transformProps.ts
**Line:** 107:107
**Comment:**
*Possible Bug: Division by zero when computing percentage offsets: the
code divides chartPadding by `height` and `width` without guarding against zero
or negative dimensions, which can produce Infinity/NaN in the computed `%`
strings and break layout; return early or use safe defaults when dimensions are
invalid.
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]
