codeant-ai-for-open-source[bot] commented on PR #36993: URL: https://github.com/apache/superset/pull/36993#issuecomment-3727126201
## 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/36993/files#diff-26ee6b02af1970f54d4dfaffabd065e966b064b85132577816b08c8ddaabeb6fR26-R85'><strong>UX / backward-compatibility</strong></a><br>Changing the default margin from a non-zero value to 0 changes the initial layout for existing and new charts. Confirm this is intentional for all affected chart types and that tests or snapshots are updated. Also confirm automation that sets margin to 30 when a title is added still triggers reliably when starting from 0.<br> - [ ] <a href='https://github.com/apache/superset/pull/36993/files#diff-88405b689ea6064a26842b88ad09e219f3f2e22d9bd031303c4d7b2a518e68f0R562-R583'><strong>Numeric comparison risk</strong></a><br>The new code compares margin values using strict equality against 0, but margin control values may be strings (coming from form controls). This can cause the branch that sets margins to 30 or 0 to be skipped unexpectedly. Convert or coerce margin values to numbers before comparisons.<br> - [ ] <a href='https://github.com/apache/superset/pull/36993/files#diff-d73c2cf4fc7d8cbaf9d0aee4ba2ecc465568d353e29f70dc244af1a4f464a647R245-R256'><strong>Missing automatic margin update</strong></a><br>The PR description promises automatically setting margin to 30 when an axis title is added (and resetting to 0 when removed). The new lines only compute whether to add title offsets to padding but do not compute or apply an "effective" margin value (or mutate formData). Verify where/how the automatic margin change is implemented — if it's not, consider computing an effective margin here (without mutating formData) or implementing the automation at the form layer.<br> - [ ] <a href='https://github.com/apache/superset/pull/36993/files#diff-2672bc046f1a56f67dd661b1ace7c639da51ad78c4e442b96445c60924621c4eR553-R557'><strong>Inconsistent effective margin handling</strong></a><br>The new booleans `addYAxisTitleOffset` / `addXAxisTitleOffset` gate adding extra padding based on whether the configured title margin is non-zero. However, axis `nameGap` and the `chartPadding` call still use the raw converted margins (via `convertInteger(...)`) rather than an "effective" margin that could be auto-set (e.g., to 30) when a title is added and the margin is 0. Ensure the code that "automatically set[s] margin to 30 when an axis title is added and margin is 0" is applied consistently to both determining padding and the axis `nameGap`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36993/files#diff-5f5c22612f270fe5718a3d200d291ec22bc1ee0bd576ef16f36e76d286cddb44R646-R661'><strong>Offset applied when title missing</strong></a><br>The new ternary still applies `yAxisOffset` in the fallback branch when `yAxisTitlePosition` is falsy. Per the PR description, offsets should be applied only when an axis title exists and margin is non-zero — verify whether the code should avoid adding `yAxisOffset` when `yAxisTitlePosition` is undefined/empty.<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]
