codeant-ai-for-open-source[bot] commented on PR #36315: URL: https://github.com/apache/superset/pull/36315#issuecomment-3615251583
## 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/36315/files#diff-359940b6f6470257ed49fa39b82361ccb50a826407072043843a36a0ed6ce4e9R802-R805'><strong>Possible Negative/Incorrect Range</strong></a><br>Subtracting one day from `end` when `domain == "month"` can make `end` earlier than `start`. Using `relativedelta` and then computing the months from a potentially inverted interval may produce a negative or otherwise incorrect `range_`. This can lead to off-by-one or zero/negative ranges in edge cases (e.g., `start == end == first-of-month`), causing unexpected rendering behavior.<br> - [ ] <a href='https://github.com/apache/superset/pull/36315/files#diff-359940b6f6470257ed49fa39b82361ccb50a826407072043843a36a0ed6ce4e9R789-R812'><strong>Lack of defensive checks / tests</strong></a><br>The change alters boundary logic for month ranges but there are no guards or tests in the same change. Recommend adding unit tests for edge cases (e.g., end is first day of month, start == end, timezone-aware datetimes) and defensive code to prevent negative ranges.<br> - [ ] <a href='https://github.com/apache/superset/pull/36315/files#diff-a63b892b1855dbd88154b9d9780e12ab68a2568e20ef7c3eb7e25cf5779f2b26R2722-R2730'><strong>Timezone Inconsistency</strong></a><br>The new getMonthDomain uses UTC-based Date construction (Date.UTC / getUTCFullYear / getUTCMonth) to build month boundaries. Much of the rest of the code (e.g., month extraction/comparison) uses local getters/constructors (getFullYear/getMonth / new Date(...)). This mixing of UTC and local date methods can produce off-by-one-month behaviour (especially around midnight UTC in timezones behind/ahead of UTC). Verify all month-related code paths (domain keys, extractUnit, comparisons, parseDatas, getDomainKeys) are consistent with UTC handling or explicitly normalized to local timezone.<br> - [ ] <a href='https://github.com/apache/superset/pull/36315/files#diff-a63b892b1855dbd88154b9d9780e12ab68a2568e20ef7c3eb7e25cf5779f2b26R2722-R2730'><strong>Range Ordering / Edge Cases</strong></a><br>getMonthDomain constructs a start and a stopExclusive but does not ensure start <= stopExclusive when range is a Date that might be before the provided `d`. d3.time.months expects a proper [start, stop) ordering — failing to normalize order may produce unexpected results for backward ranges or negative ranges. Also check numeric `range` negative values and ensure setUTCMonth handles them correctly.<br> - [ ] <a href='https://github.com/apache/superset/pull/36315/files#diff-ef67d090c5529588ee01a5eef9e2d93d6de4d9406e63e0c8ed67b5c1134bd3c2R37-R47'><strong>Flaky / Timezone-sensitive test</strong></a><br>The tests verify month boundaries using UTC-based Dates and string containment of ISO strings. There is risk of flakiness if some environments construct Dates in local time or if implementations of getMonthDomain return Date objects with non-zero time components. Add assertions that are timezone-robust and add coverage for local-time Date inputs to ensure consistent behavior across environments.<br> - [ ] <a href='https://github.com/apache/superset/pull/36315/files#diff-ef67d090c5529588ee01a5eef9e2d93d6de4d9406e63e0c8ed67b5c1134bd3c2R43-R45'><strong>Weak assertions</strong></a><br>Several assertions use toContain('YYYY-MM-DD') which is permissive and may hide off-by-hour bugs (e.g., '2025-03-01T23:00:00.000Z' still contains '2025-03-01'). Tests should assert full ISO strings or explicit UTC year/month/day equality to catch subtle timezone shifts.<br> - [ ] <a href='https://github.com/apache/superset/pull/36315/files#diff-ef67d090c5529588ee01a5eef9e2d93d6de4d9406e63e0c8ed67b5c1134bd3c2R71-R76'><strong>Missing local Date coverage</strong></a><br>The test that exercises the Date-object-range case constructs Dates via Date.UTC. It would be valuable to add a test that passes Dates created with the local constructor (new Date(year, monthIndex, day)) to ensure the plugin is robust against local timezone offsets.<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]
