codeant-ai-for-open-source[bot] commented on code in PR #35459:
URL: https://github.com/apache/superset/pull/35459#discussion_r3605815780
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -73,52 +76,120 @@ export function parseParams({
return [name, formattedValue, formattedPercent];
}
-function getTotalValuePadding({
+const HALF_DONUT_SWEEP_LIMIT = 180;
+
+/**
+ * Geometric configuration for each type of semi-circular layout.
+ *
+ * - `centerOffset` — offset of the chart center from the baseline 50% on the
X and Y axes.
+ * Resulting position: `50% + offset`.
+ * - `totalBase` — base position of the "Total" text as a percentage on the
X and Y axes.
+ *
+ * The values are selected so that the "Total" text visually remains
+ * at the geometric center of the arc after the chart is re-centered.
+ */
+
+const HALF_DONUT_LAYOUT: Record<
+ HalfDonut,
+ {
+ centerOffset: { x: number; y: number };
+ totalBase: { left: number; top: number };
+ }
+> = {
+ top: { centerOffset: { x: 0, y: 20 }, totalBase: { left: 50, top: 68.5 } },
+ bottom: { centerOffset: { x: 0, y: -20 }, totalBase: { left: 50, top: 30 } },
+ left: { centerOffset: { x: 5, y: 0 }, totalBase: { left: 55, top: 50 } },
+ right: { centerOffset: { x: -5, y: 0 }, totalBase: { left: 30, top: 50 } },
+ none: { centerOffset: { x: 0, y: 0 }, totalBase: { left: 50, top: 50 } },
+};
+
+/**
+ * Determines the type of semicircular layout based on the start angle and
swept angle.
+ *
+ * All four semicircle orientations are supported:
+ * - `'top'` — arc at the top, center shifted down (center Y = 70%).
+ * - `'bottom'` — arc at the bottom, center shifted up (center Y = 30%).
+ * - `'left'` — arc on the left, center shifted right (center X = 70%).
+ * - `'right'` — arc on the right, center shifted left (center X = 30%).
+ *
+ * @param startAngle - The start angle of the arc in degrees (0–360).
+ * @param sweptAngle - The swept angle of the arc in degrees (0–360).
+ * @returns The type of semicircular layout.
+ */
+
+export const getHalfDonut = (
+ startAngle: number,
+ sweptAngle: number,
+): HalfDonut => {
+ if (sweptAngle > HALF_DONUT_SWEEP_LIMIT) return 'none';
+
+ const normalized = startAngle % 360;
+
+ if (normalized === 180) return 'top';
+ if (normalized === 0) return 'bottom';
+ if (normalized === 270) return 'left';
+ if (normalized === 90) return 'right';
Review Comment:
**Suggestion:** `getHalfDonut` currently treats `startAngle` 90° and 270° as
half-donut orientations, which triggers automatic center and total-label
recentering for side layouts. That conflicts with the intended behavior (and
new tests) where side orientations should not be recentered, so charts at
90°/270° render shifted unexpectedly. Return `none` for those angles (or
otherwise gate side recentering) to keep side layouts stable. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Side half-donut pies display mis-centered arcs and totals.
⚠️ Jest half-donut tests fail, breaking Pie chart suite.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-echarts/src/Pie/controlPanel.tsx` and
see
the Pie control config for `startAngle` at lines 16–27, whose description
states that
half-donut layouts use 180° (top) or 0°/360° (bottom) and that side
orientations
(90°/270°) are not recentered automatically.
2. Inspect
`superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts`
around lines 120–137 where `getHalfDonut(startAngle, sweptAngle)` is defined
and note the
logic `if (normalized === 270) return 'left';` and `if (normalized === 90)
return
'right';`, which causes `HalfDonut` to be `'left'` or `'right'` whenever
`sweptAngle` ≤
180 and `startAngle` is 90°/270°.
3. Follow the usage of `getHalfDonut` in the same file: `getHalfDonutLayout`
at lines
136–137 and the series definition at lines 221–233 where `center` is
computed as `['${50 +
getHalfDonutLayout(startAngle, sweptAngle).centerOffset.x}%', '${50 +
getHalfDonutLayout(startAngle, sweptAngle).centerOffset.y}%']`, so a Pie
chart rendered
with `donut: true`, `startAngle: 90` or `270`, and `sweptAngle` ≤ 180 is
automatically
re-centered horizontally as if it were a half-donut side layout.
4. Run the Jest tests in
`superset-frontend/plugins/plugin-chart-echarts/test/Pie/transformProps.test.ts`
and
observe the parametrized test `test.each([...])('startAngle=%i,
sweptAngle=%i → %s', ...)`
near lines 193–205, which expects `getHalfDonut(90, 180)` and
`getHalfDonut(270, 180)` to
return `'none'`; with the current implementation they return `'right'` and
`'left'`,
causing test failures and confirming that side orientations are
misclassified and
recentered contrary to both test expectations and the control panel
documentation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=50276bc3ea9c46df8a12404b3ad2d240&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=50276bc3ea9c46df8a12404b3ad2d240&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/Pie/transformProps.ts
**Line:** 130:131
**Comment:**
*Logic Error: `getHalfDonut` currently treats `startAngle` 90° and 270°
as half-donut orientations, which triggers automatic center and total-label
recentering for side layouts. That conflicts with the intended behavior (and
new tests) where side orientations should not be recentered, so charts at
90°/270° render shifted unexpectedly. Return `none` for those angles (or
otherwise gate side recentering) to keep side layouts stable.
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%2F35459&comment_hash=8c78df3f049c8fbec679dd65490a3f1d62fed133ff869c414f85922940c5f135&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35459&comment_hash=8c78df3f049c8fbec679dd65490a3f1d62fed133ff869c414f85922940c5f135&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -73,52 +76,120 @@ export function parseParams({
return [name, formattedValue, formattedPercent];
}
-function getTotalValuePadding({
+const HALF_DONUT_SWEEP_LIMIT = 180;
+
+/**
+ * Geometric configuration for each type of semi-circular layout.
+ *
+ * - `centerOffset` — offset of the chart center from the baseline 50% on the
X and Y axes.
+ * Resulting position: `50% + offset`.
+ * - `totalBase` — base position of the "Total" text as a percentage on the
X and Y axes.
+ *
+ * The values are selected so that the "Total" text visually remains
+ * at the geometric center of the arc after the chart is re-centered.
+ */
+
+const HALF_DONUT_LAYOUT: Record<
+ HalfDonut,
+ {
+ centerOffset: { x: number; y: number };
+ totalBase: { left: number; top: number };
+ }
+> = {
+ top: { centerOffset: { x: 0, y: 20 }, totalBase: { left: 50, top: 68.5 } },
+ bottom: { centerOffset: { x: 0, y: -20 }, totalBase: { left: 50, top: 30 } },
+ left: { centerOffset: { x: 5, y: 0 }, totalBase: { left: 55, top: 50 } },
+ right: { centerOffset: { x: -5, y: 0 }, totalBase: { left: 30, top: 50 } },
+ none: { centerOffset: { x: 0, y: 0 }, totalBase: { left: 50, top: 50 } },
+};
+
+/**
+ * Determines the type of semicircular layout based on the start angle and
swept angle.
+ *
+ * All four semicircle orientations are supported:
+ * - `'top'` — arc at the top, center shifted down (center Y = 70%).
+ * - `'bottom'` — arc at the bottom, center shifted up (center Y = 30%).
+ * - `'left'` — arc on the left, center shifted right (center X = 70%).
+ * - `'right'` — arc on the right, center shifted left (center X = 30%).
Review Comment:
**Suggestion:** The inline documentation claims left/right half-donut
recentering uses `center X = 70%` and `center X = 30%`, but the actual layout
offsets are ±5 (i.e., 55%/45%). This mismatch is misleading for maintainers and
makes the geometry contract incorrect; update the comment to match the
implemented coordinates or adjust constants to match the documented values.
[comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
⚠️ Comment misleads maintainers about half-donut side center positions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts` and
inspect the documentation block for `getHalfDonut` around lines 106–118,
noting the bullet
points that state the left half-donut has center X = 70% and the right
half-donut has
center X = 30%.
2. In the same file, examine the `HALF_DONUT_LAYOUT` constant at lines
92–104, where
`left` uses `centerOffset: { x: 5, y: 0 }` and `right` uses `centerOffset: {
x: -5, y: 0
}`, yielding actual centers of 55% and 45% respectively when applied as `50%
+ offset`.
3. Follow the usage in the series configuration at lines 221–233 where
`center` is
computed based on `getHalfDonutLayout(startAngle, sweptAngle).centerOffset`,
confirming
that these layout offsets are what ECharts actually uses to position the pie
center.
4. Compare the documented 70%/30% positions with the implemented 55%/45%
centers to see
that the comment no longer reflects the real geometry contract; this
mismatch does not
affect runtime behavior but can mislead future maintainers or reviewers who
rely on the
docstring instead of reading `HALF_DONUT_LAYOUT`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cfc24fd772a14dd7971e054f1314ddf7&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=cfc24fd772a14dd7971e054f1314ddf7&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/Pie/transformProps.ts
**Line:** 112:113
**Comment:**
*Comment Mismatch: The inline documentation claims left/right
half-donut recentering uses `center X = 70%` and `center X = 30%`, but the
actual layout offsets are ±5 (i.e., 55%/45%). This mismatch is misleading for
maintainers and makes the geometry contract incorrect; update the comment to
match the implemented coordinates or adjust constants to match the documented
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.
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%2F35459&comment_hash=3c32caaaa5f9bb1b11a28d8ba1cb4d0d456f9621943551b638c1720e003b350b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35459&comment_hash=3c32caaaa5f9bb1b11a28d8ba1cb4d0d456f9621943551b638c1720e003b350b&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]
