codeant-ai-for-open-source[bot] commented on PR #36012: URL: https://github.com/apache/superset/pull/36012#issuecomment-3726328260
## 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/36012/files#diff-083c5fc50d02a2932f886bfeefccf397814c3cef72139e259f4ed81dc74a72edR312-R318'><strong>Edge case handling</strong></a><br>Ensure `shouldShowTimeRangePicker` robustly handles dataset shapes that are partially loaded or missing expected fields (e.g., `columns`, `is_dttm`). If the util assumes a particular dataset structure, guard against runtime issues when the dataset object is undefined or incomplete.<br> - [ ] <a href='https://github.com/apache/superset/pull/36012/files#diff-402b8509601c7a6fe0341169cc9a04d4178196a3610daf98a53f88bffb314bc8R81-R85'><strong>Behavioral assumption</strong></a><br>`shouldShowTimeRangePicker` returns true when `currentDataset` is undefined and `hasTemporalColumns` treats "empty column_types" as temporal (returns true for empty arrays). Confirm this default behaviour is intended in all callers — it causes the time-range picker to be shown when dataset info is missing or column types are empty, which might be surprising or lead to incorrect UI decisions in some flows.<br> - [ ] <a href='https://github.com/apache/superset/pull/36012/files#diff-083c5fc50d02a2932f886bfeefccf397814c3cef72139e259f4ed81dc74a72edR320-R325'><strong>Possible Behavior Change</strong></a><br>The inline ternary used to compute `showTimeRangePicker` previously returned `true` when there was no `currentDataset`. The new call delegates the decision to `shouldShowTimeRangePicker(currentDataset)`. Verify that the extracted util preserves the old fallback (returning true when `currentDataset` is undefined) to avoid changing UI behavior for filters with no dataset selected.<br> - [ ] <a href='https://github.com/apache/superset/pull/36012/files#diff-402b8509601c7a6fe0341169cc9a04d4178196a3610daf98a53f88bffb314bc8R72-R85'><strong>Pre-filter default semantics</strong></a><br>`hasTemporalColumns` treats empty/undefined `column_types` as "no columns reported" and returns true (show picker). Confirm this is the intended UX across callers: returning true on unknown/empty metadata may be surprising in some flows.<br> - [ ] <a href='https://github.com/apache/superset/pull/36012/files#diff-402b8509601c7a6fe0341169cc9a04d4178196a3610daf98a53f88bffb314bc8R94-R117'><strong>Tie-breaking behaviour</strong></a><br>The algorithm in `mostUsedDataset` only updates the winner when a dataset's count is strictly greater than the current max. If two datasets tie in usage, the function implicitly keeps the first encountered dataset. This is a behaviour worth verifying (either make it explicit with a test or change to deterministic tie handling).<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]
