codeant-ai-for-open-source[bot] commented on PR #31765: URL: https://github.com/apache/superset/pull/31765#issuecomment-3625667411
## 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/31765/files#diff-88cfb8074eec39fa78997341a017c9b8251c54d19a2ab8e188b9e8bcfad7e00bR481-R496'><strong>Possible runtime error</strong></a><br>The new calls pass `formData.extraFormData.time_grain_sqla` directly. If `formData.extraFormData` is undefined at runtime, this will throw. Use safe optional chaining or ensure `extraFormData` exists before accessing its properties.<br> - [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-210e76eb52ab194e19d0ef3bc8485c5dd0fda48a6af1c72a4376c5e448dfeb8fR78-R96'><strong>Format precedence</strong></a><br>The new timeGrain early-return in both tooltip and x-axis formatters takes precedence over any explicit `format` argument. If a caller specifies an explicit time format string, it will be ignored when `timeGrain` is QUARTER/MONTH/YEAR. Confirm whether timeGrain should override an explicit format or only apply when `format` is absent or equal to SMART_DATE_ID.<br> - [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-210e76eb52ab194e19d0ef3bc8485c5dd0fda48a6af1c72a4376c5e448dfeb8fR98-R116'><strong>X axis behavior change</strong></a><br>`getXAxisFormatter` used to return `undefined` for `!format` so the charting lib could apply defaults. With the new timeGrain branch it now returns a concrete formatter for QUARTER/MONTH/YEAR even when `format` is absent, changing the default rendering behaviour. Ensure this is intentional.<br> - [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-210e76eb52ab194e19d0ef3bc8485c5dd0fda48a6af1c72a4376c5e448dfeb8fR41-R42'><strong>SMART_DATE_VERBOSE usage</strong></a><br>`getSmartDateVerboseFormatter` now accepts a `timeGrain` and forwards it to `getTimeFormatter`. Verify that `getTimeFormatter(SMART_DATE_VERBOSE_ID, timeGrain)` produces the intended verbose smart-date output for all supported time grains and that `@superset-ui/core` supports that combination.<br> - [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-2ada607a5c3e8ab7bee07307fefb6ab7236948f5bed812126793074bdc4d89c6R332-R481'><strong>Timezone-dependent / flaky tests</strong></a><br>The new tests call the time formatter with hardcoded epoch milliseconds (e.g. 1610000000000) and assert exact formatted strings. Date/time formatting can be influenced by the environment timezone or locale, which can make these expectations flaky on CI or developer machines. Verify tests are deterministic across environments (use UTC-based timestamps, Date.UTC, or mock the timezone) and consider asserting patterns rather than exact strings where appropriate.<br> - [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-788715ca89a2182b9d9f2edec52fbb5f3e8e5cb70c49bf166992193a81eb87f6R631-R758'><strong>Timezone-dependent test</strong></a><br>The new tests assert formatted date strings using a raw epoch number (1610000000000). Formatting can vary by environment timezone/locale and lead to flaky CI tests. Use UTC-based timestamps or construct dates with Date.UTC / moment.utc to make expectations deterministic.<br> - [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-788715ca89a2182b9d9f2edec52fbb5f3e8e5cb70c49bf166992193a81eb87f6R652-R659'><strong>Fragile xAxis access</strong></a><br>The test assumes echartOptions.xAxis is an object with axisLabel.formatter. In some chart configs xAxis can be an array or undefined — this will throw during test runtime. Add a defensive guard or normalize the value before accessing formatter.<br> - [ ] <a href='https://github.com/apache/superset/pull/31765/files#diff-2ada607a5c3e8ab7bee07307fefb6ab7236948f5bed812126793074bdc4d89c6R332-R481'><strong>Duplication in test setup</strong></a><br>The tests for xAxis and tooltip formatters repeat very similar setup: constructing an updatedChartPropsConfig with modified formData and queriesData (colnames/coltypes). This duplication makes maintenance harder and increases the chance of inconsistent changes. Extract a small helper to build chart props for a given time grain/format to avoid divergence and reduce copy/paste.<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]
