codeant-ai-for-open-source[bot] commented on PR #36753: URL: https://github.com/apache/superset/pull/36753#issuecomment-3672825584
## 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/36753/files#diff-fd855b910f6e482f1edd136d6c77cb64626f04fbfcef4c487a72aa0e75779ff0R524-R529'><strong>Page size fallback logic</strong></a><br>The page size passed to the Pagination component was simplified to `serverPaginationData?.pageSize ?? serverPageLength`. Previously there was logic using `hasServerPageLengthChanged` and falling back to a default (10). This change might reintroduce cases where the pageSize becomes undefined or does not respect the explore/editor patching behavior. Validate expected behavior when both values are undefined and when `hasServerPageLengthChanged` is true.<br> - [ ] <a href='https://github.com/apache/superset/pull/36753/files#diff-d987d4addee4ccb4cccae5bb4376bdf7915776d6e8631f9a777abfa98c08c9a0R788-R1260'><strong>Type assumptions / casting</strong></a><br>The code casts `columns` to a type that includes `columnLabel`. Ensure the returned column objects always include this field and the type definitions reflect it; otherwise the narrow cast may hide type issues. Consider extending the column return type or the ColumnWithLooseAccessor type instead of casting via `unknown`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36753/files#diff-d987d4addee4ccb4cccae5bb4376bdf7915776d6e8631f9a777abfa98c08c9a0R794-R906'><strong>Column label selection</strong></a><br>The new `columnLabel` property is set from the local `label` variable, but the component computes a separate `displayLabel` that better represents what end-users see (especially for comparison columns). Verify that the value used for search dropdowns should be `displayLabel` rather than `label` so the "Search by" UI shows the intended human-facing strings.<br> - [ ] <a href='https://github.com/apache/superset/pull/36753/files#diff-bd919fa26ebe6f116bb11bdeb0fd42d9044597bf0e7066e6a2528ea8ba8c3f4eR121-R121'><strong>Translation staleness</strong></a><br>Calling `t()` in the function parameter default (and using React.memo via typedMemo) can prevent the component from picking up runtime locale changes because memoization may stop re-renders. Verify that translations update correctly when the app locale changes and consider computing translations inside the render with proper dependencies.<br> - [ ] <a href='https://github.com/apache/superset/pull/36753/files#diff-d987d4addee4ccb4cccae5bb4376bdf7915776d6e8631f9a777abfa98c08c9a0R1236-R1260'><strong>Derived state effect dependency</strong></a><br>`searchOptions` is derived from `columns`, but the effect that updates `searchOptions` lists `searchOptions` itself in the dependency array. That can cause unnecessary additional runs. Consider deriving options from `columns` only (useMemo) or update the effect dependency list to avoid redundant updates.<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]
