codeant-ai-for-open-source[bot] commented on PR #35459: URL: https://github.com/apache/superset/pull/35459#issuecomment-3732506920
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/35459/files#diff-1ea47929dd1778650cf647bd12fc99a357fc322d563ab8fd49da382c107a149bR75-R132'><strong>Conflicting assignment</strong></a><br>The function computing `padding.top` assigns the value in multiple places (an early `if (half)` block and later `if (chartPadding.top)` / `if (chartPadding.bottom)` blocks). When both conditions are true the earlier value is immediately overwritten causing possible off-by values and inconsistent behavior between half and full donuts. Consolidate the logic so only one code-path computes the final top padding.<br> - [ ] <a href='https://github.com/apache/superset/pull/35459/files#diff-1d2467213a55fb07a26a38ea928dc498f200cdd61a561e2104d634cafbf79904R109-R151'><strong>Naming mismatch (snake_case vs camelCase)</strong></a><br>New controls are named `start_angle` and `swept_angle` while their defaults come from `DEFAULT_FORM_DATA.startAngle` and `DEFAULT_FORM_DATA.sweptAngle` (camelCase). If form data keys rely on control `name`, the defaults may not apply as expected and runtime formData keys will be inconsistent. Align control `name` with the `DEFAULT_FORM_DATA` keys or adjust the default sources.<br> - [ ] <a href='https://github.com/apache/superset/pull/35459/files#diff-5a537c73b95a675756f9adc28f6c012e7739ad0ae4e77bc83abab3f4a5ae6c87R38-R39'><strong>Required fields risk</strong></a><br>`startAngle` and `sweptAngle` were added as required `number` properties on `EchartsPieFormData`. Making them required at the type level can break callers that construct form data objects without these properties (even if DEFAULT_FORM_DATA provides defaults). Verify all places that create/merge form data (including tests and consumers) and consider making these fields optional in the type or ensuring defaults are applied earlier in the pipeline.<br> - [ ] <a href='https://github.com/apache/superset/pull/35459/files#diff-5a537c73b95a675756f9adc28f6c012e7739ad0ae4e77bc83abab3f4a5ae6c87R88-R89'><strong>Angle value validation</strong></a><br>There is no validation or normalization for `startAngle`/`sweptAngle`. Invalid values (negative, >360, or a sweptAngle <= 0) could produce unexpected rendering or runtime issues in ECharts. Confirm expected ranges and add normalization/clamping where the values are handed off to the chart renderer or transformation layer.<br> - [ ] <a href='https://github.com/apache/superset/pull/35459/files#diff-5a537c73b95a675756f9adc28f6c012e7739ad0ae4e77bc83abab3f4a5ae6c87R38-R89'><strong>Semantic constraints missing</strong></a><br>The semantics of `sweptAngle` (full circle vs half donut vs >360) aren't documented or constrained. Review UX/requirements: should `sweptAngle` be limited to (0, 360] and should `startAngle` allow negative values? Ensure transformation logic enforces the intended semantics.<br> </td></tr> </table> -- 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]
