codeant-ai-for-open-source[bot] commented on PR #36989: URL: https://github.com/apache/superset/pull/36989#issuecomment-3726111843
## 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/36989/files#diff-88cfb8074eec39fa78997341a017c9b8251c54d19a2ab8e188b9e8bcfad7e00bR506-R514'><strong>All-negative series edge case</strong></a><br>When all values are negative, the code sets only `yAxisMin` to `dataMin` (if negative) but leaves `yAxisMax` undefined. Depending on downstream defaults, this may still produce undesirable bounds or label clipping. Verify behavior when all data is negative and when users expect symmetric/diverging bounds.<br> - [ ] <a href='https://github.com/apache/superset/pull/36989/files#diff-bbf4598c738256137a39995b5cbbf3cf4e6612a339cb8a03f11be5493236f9b5R48-R49'><strong>Hardcoded padding</strong></a><br>A fixed numeric padding (70px) may not correctly accommodate value labels of varying lengths, fonts, or locales. This can still cause clipping for very large labels or be excessive for short labels. Consider measuring label widths at render time and deriving padding or making the value configurable and bounded.<br> - [ ] <a href='https://github.com/apache/superset/pull/36989/files#diff-788715ca89a2182b9d9f2edec52fbb5f3e8e5cb70c49bf166992193a81eb87f6R769-R771'><strong>Axis shape assumption</strong></a><br>The new tests assume `echartOptions.xAxis` is an object with `.max`/`.min`. In some transform implementations the axis can be an array (multiple axes) or an object. If `xAxis` is an array the tests will falsely succeed/fail. Normalize or assert both shapes in tests to avoid flakiness.<br> - [ ] <a href='https://github.com/apache/superset/pull/36989/files#diff-bbf4598c738256137a39995b5cbbf3cf4e6612a339cb8a03f11be5493236f9b5R48-R49'><strong>Naming / Scope</strong></a><br>The new property is added to TIMESERIES_CONSTANTS though it is specific to horizontal bar charts. This may cause discoverability issues and unintended coupling. Consider moving it to a bar-chart-specific constants object or renaming to make its intent clearer.<br> - [ ] <a href='https://github.com/apache/superset/pull/36989/files#diff-788715ca89a2182b9d9f2edec52fbb5f3e8e5cb70c49bf166992193a81eb87f6R731-R743'><strong>Missing padding assertion</strong></a><br>The PR description mentions increasing right padding when value labels are shown, but the new tests only validate axis bounds. Add tests to confirm grid/right padding is increased when `showValue` is enabled so the change is covered and can't regress.<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]
