codeant-ai-for-open-source[bot] commented on PR #36881: URL: https://github.com/apache/superset/pull/36881#issuecomment-3702597950
## 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/36881/files#diff-fd855b910f6e482f1edd136d6c77cb64626f04fbfcef4c487a72aa0e75779ff0R528-R528'><strong>Pagination pageSize fallback</strong></a><br>The pageSize passed to <Pagination> now uses `serverPaginationData?.pageSize ?? serverPageLength`. If both values are undefined this can pass `undefined` to the Pagination component (previous logic guaranteed a numeric default of 10). Also removing the previous conditional that used `hasServerPageLengthChanged` may re-introduce edge-cases where explore/editor changes aren't applied as before. Verify intended behavior across all cases (initial render, editor updates, and resize).<br> - [ ] <a href='https://github.com/apache/superset/pull/36881/files#diff-d987d4addee4ccb4cccae5bb4376bdf7915776d6e8631f9a777abfa98c08c9a0R794-R920'><strong>Label vs originalLabel</strong></a><br>The PR introduces a `columnLabel` (display label) derived from `config.customColumnName || originalLabel`, but many comparisons and grouping decisions rely on `column.label` or `column.originalLabel`. This mix of "display label" vs "original label" may cause misclassification of "comparison" columns, incorrect grouping, or inconsistent aria/ID mapping. Verify all places that compare labels (e.g., `comparisonLabels.includes(...)`, header grouping logic, and className logic) use the intended source consistently.<br> - [ ] <a href='https://github.com/apache/superset/pull/36881/files#diff-bd919fa26ebe6f116bb11bdeb0fd42d9044597bf0e7066e6a2528ea8ba8c3f4eR564-R569'><strong>Accessibility</strong></a><br>A visible label "Search by" was added as a plain <span>. Ensure the label is programmatically associated with the SearchSelectDropdown (via aria-labelledby / aria-label or a <label htmlFor>) so screen readers can correctly identify the control.<br> - [ ] <a href='https://github.com/apache/superset/pull/36881/files#diff-d987d4addee4ccb4cccae5bb4376bdf7915776d6e8631f9a777abfa98c08c9a0R1235-R1261'><strong>Search options effect</strong></a><br>The effect that builds `searchOptions` depends on both `columns` and `searchOptions`. While it guards with `isEqual`, having the external state in the dependency list risks extra runs or subtle update loops. Consider deriving and setting `searchOptions` only when `columns` change, or using a ref to compare previous value.<br> - [ ] <a href='https://github.com/apache/superset/pull/36881/files#diff-fd855b910f6e482f1edd136d6c77cb64626f04fbfcef4c487a72aa0e75779ff0R390-R390'><strong>Translation formatting</strong></a><br>The added translated label includes surrounding whitespace and a colon outside the translation call. This can cause inconsistent punctuation in some languages and double space in the UI. Decide whether the colon should be part of the translation or appended, and remove the extra leading space before the translated string for consistent rendering.<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]
