codeant-ai-for-open-source[bot] commented on PR #36702: URL: https://github.com/apache/superset/pull/36702#issuecomment-3664491098
## 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/36702/files#diff-f066be5f3a8b8b807b00714836b4d6f868233ed60bb2423e7bf9ed489bcfde24R48-R51'><strong>Possible Missing Named Exports</strong></a><br>The file re-exports `RELATIVE_DAY_ID`, `createRelativeDayFormatter` and their "no time" counterparts from two modules. If those modules do not export these identifiers as named exports (for example they export a default or different names), this re-export will cause build-time errors. Validate that the formatter modules actually export those names and that their TypeScript declarations are present.<br> - [ ] <a href='https://github.com/apache/superset/pull/36702/files#diff-91f9fc1f75fd47cde0bc108906162087ab1d789eef99d6e02c6b329667d10d8eR19-R26'><strong>Possible missing export</strong></a><br>This change imports `RELATIVE_DAY_ID` and `RELATIVE_DAY_NO_TIME_ID` from `@superset-ui/core`. If those constants are not exported from that package (or have different names), the build will fail with a TypeScript/ES module error. Verify the constants are exported and stable across package versions, and update the import source or add a fallback if necessary.<br> - [ ] <a href='https://github.com/apache/superset/pull/36702/files#diff-131da5e05f5855060a3cf29eec69773ce4203953f5e4d2976b8ccbf523b26decR39-R42'><strong>Day boundary logic</strong></a><br>The day calculation uses `Math.floor(msDiff / MS_PER_DAY)` and then maps non-negative values to `daysDiff + 1`. This works for many cases but is subtle around negative fractions (times before Day 1 but within the same UTC day) and might be easier to reason about if extracted to a small helper with tests. Ensure this behavior (no Day 0, Day 1 = 2000-01-01T00:00Z) is deliberate and covered by unit tests.<br> - [ ] <a href='https://github.com/apache/superset/pull/36702/files#diff-f066be5f3a8b8b807b00714836b4d6f868233ed60bb2423e7bf9ed489bcfde24R52-R55'><strong>Registry / UI integration</strong></a><br>Adding the exports is only part of the change — confirm the new formatter IDs are registered with the formatter registry and included in the chart time-format dropdown (with proper labels and i18n). Also check that switching to these formatters preserves datetime chart behaviors (zoom/aggregation).<br> - [ ] <a href='https://github.com/apache/superset/pull/36702/files#diff-8fa09dddb58067c61e8a8ca18ad456fb0979f40204cc86f4c4b7d669b0b590e0R22-R26'><strong>API inconsistency</strong></a><br>The first test asserts that the returned formatter has a `format` method (object API) but all other tests call the formatter as a function (callable API). This mismatch can cause fragile tests or false failures depending on whether the implementation returns a function, an object with `format()`, or a callable function-object hybrid. The test should assert the concrete API the implementation provides.<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]
