codeant-ai-for-open-source[bot] commented on PR #36689: URL: https://github.com/apache/superset/pull/36689#issuecomment-3663835420
## 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/36689/files#diff-88cfb8074eec39fa78997341a017c9b8251c54d19a2ab8e188b9e8bcfad7e00bR537-R551'><strong>Brush axis targeting</strong></a><br>The newly added `brush` configuration always targets `xAxisIndex: 0`. When the chart is rendered horizontal (the code swaps x/y axes earlier), the time axis may no longer be the chart's "x" axis. As a result the brush selection may target the wrong axis or not behave as expected. Verify the brush targets the time axis in both orientations.<br> - [ ] <a href='https://github.com/apache/superset/pull/36689/files#diff-a4dc411a5a3aaba0b7ee7ab948a8ff7a3c77bf9b45d882e074751c1cbf33788eR168-R227'><strong>Inconsistent date types in filters</strong></a><br>The brush handler uses coordRange values directly to build filters. Depending on the axis, these values may be Date objects or numbers. Other code paths (e.g. table filters) normalize Date -> timestamp before sending to backend. Verify and normalize brush values to the expected type (e.g., epoch ms) to avoid backend mismatches.<br> - [ ] <a href='https://github.com/apache/superset/pull/36689/files#diff-c1ab60bb6e27889acf1f596823778f0c303760da7f9fd9bfffe0d03235055808R107-R111'><strong>Optional property may be undefined</strong></a><br>`isHorizontal` is declared optional in the transformed props. Consumers of TimeseriesChartTransformedProps may assume a boolean and not guard against `undefined`, causing conditional rendering bugs at runtime. Consider making it non-optional in the transformed props or ensure it is always set by the transform function with a sensible default.<br> - [ ] <a href='https://github.com/apache/superset/pull/36689/files#diff-c1ab60bb6e27889acf1f596823778f0c303760da7f9fd9bfffe0d03235055808R107-R111'><strong>Naming / duplication mismatch</strong></a><br>Form input uses `orientation?: OrientationType` while transformed props add `isHorizontal?: boolean`. This duplication of layout intent can lead to inconsistent usage and confusion. Verify a single source of truth and ensure the transform reliably maps `orientation` -> `isHorizontal` (or rename for consistency).<br> - [ ] <a href='https://github.com/apache/superset/pull/36689/files#diff-870721794278f9bd3c4e9c5708151dc8e2a62735f26cd731f79a463e73a3b0faR157-R196'><strong>Missing edge case: inverted coordRange</strong></a><br>There is no test covering coordRange where start > end or where start === end. The handler assumes coordRange[0] contains [start, end] in order. Add tests to validate handling of inverted ranges or degenerate (zero-length) selections.<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]
