codeant-ai-for-open-source[bot] commented on code in PR #39972:
URL: https://github.com/apache/superset/pull/39972#discussion_r3210610581
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts:
##########
@@ -39,23 +39,20 @@ export const getSmartDateDetailedFormatter = () =>
export const getSmartDateFormatter = (timeGrain?: string) => {
const baseFormatter = getTimeFormatter(SMART_DATE_ID);
- // If no time grain provided, use the standard smart date formatter
- if (!timeGrain) {
- return baseFormatter;
- }
-
- // Create a wrapper that normalizes dates based on time grain
return new TimeFormatter({
id: SMART_DATE_ID,
label: baseFormatter.label,
formatFunc: (date: Date) => {
- // Create a normalized date based on time grain to ensure consistent
smart formatting
const normalizedDate = new Date(date);
-
- // Always remove milliseconds to prevent .XXXms format
normalizedDate.setMilliseconds(0);
- // For all time grains, normalize using UTC methods to avoid timezone
issues
+ if (!timeGrain) {
+ if (normalizedDate.getUTCSeconds() !== 0) {
+ return '';
+ }
+ return baseFormatter(normalizedDate);
Review Comment:
**Suggestion:** This new no-grain branch drops every tick whose timestamp
has non-zero seconds, which suppresses legitimate labels for real second-level
data (not just phantom edge ticks). `getXAxisFormatter` is also used by charts
like Gantt/BigNumber that do not pass a time grain, so valid axis labels can
become blank across the chart. Restrict this suppression to proven synthetic
edge ticks (or to contexts where minute alignment is guaranteed) instead of
applying it to all no-grain temporal axes. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Gantt chart x-axis hides legitimate second-level time labels.
- ⚠️ BigNumber trendline loses labels for second-resolution timestamps.
- ⚠️ Timeseries charts without time grain suppress valid axis ticks.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Gantt chart using the ECharts Gantt plugin
(`superset-frontend/plugins/plugin-chart-echarts/src/Gantt/index.ts`) on a
dataset whose
`startTime` column is a TIMESTAMP with non-zero seconds (e.g.
`2025-04-15T12:34:56Z`); the
chart's transform entrypoint is `transformProps` in
`src/Gantt/transformProps.ts:111`.
2. At `src/Gantt/transformProps.ts:50–53`, the form data is merged with
`DEFAULT_FORM_DATA` from `src/constants.ts:116`, which re-exports
`Timeseries/constants.ts:37–75` where `xAxisTimeFormat` defaults to
`'smart_date'` (the
`SMART_DATE_ID`), and at `src/Gantt/transformProps.ts:123` the code builds
`const
xAxisFormatter = getXAxisFormatter(xAxisTimeFormat);`.
3. Later in the same file, the x-axis configuration sets
`axisLabel.formatter:
xAxisFormatter` (`src/Gantt/transformProps.ts:455–462`), so ECharts calls
`getXAxisFormatter` from `src/utils/formatters.ts:172–177` with
`format='smart_date'` and
`timeGrain` omitted (`undefined`), which returns
`getSmartDateFormatter(timeGrain)`.
4. Inside `getSmartDateFormatter` (`src/utils/formatters.ts:39–55`), the new
no-grain
branch at lines 49–53 (`if (!timeGrain) { if (normalizedDate.getUTCSeconds()
!== 0) return
''; ... }`) runs for all of these charts: for any axis tick at a timestamp
with non-zero
seconds, `formatFunc` returns an empty string, so legitimate second-level
Gantt x-axis
labels render blank; the same formatter is also used in
BigNumber-with-trendline charts
via `getXAxisFormatter(timeFormat)` at
`src/BigNumber/BigNumberWithTrendline/transformProps.ts:245–247, 36–37`,
causing the same
blanking of valid second-resolution timestamps there.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fplugins%2Fplugin-chart-echarts%2Fsrc%2Futils%2Fformatters.ts%0A%2A%2ALine%3A%2A%2A%2049%3A53%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20This%20new%20no-grain%20branch%20drops%20every%20tick%20whose%20timestamp%20has%20non-zero%20seconds%2C%20which%20suppresses%20legitimate%20labels%20for%20real%20second-level%20data%20%28not%20just%20phantom%20edge%20ticks%29.%20%60getXAxisFormatter%60%20is%20also%20used%20by%20charts%20like%20Gantt%2FBigNumber%20that%20do%20not%20pass%20a%20time%20grain%2C%20so%20valid%20axis%20labels%20can%20become%20blank%20across%20the%20chart.%20Restrict%20this%20suppression%20to%20proven%20synthetic%20edge%20ticks%20%28or%20to%20contexts%20where%20minute%20alignment%20is%20guaranteed%29%20instead%20of%20applying%20it%20to%20all%20no-grain%20temporal%20axes.%0A%0AValidate%20th
e%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fplugins%2Fplugin-chart-echarts%2Fsrc%2Futils%2Fformatters.ts%0A%2A%2ALine%3A%2A%2A%2049%3A53%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20This%20new%20no-grain%20branch%20drops%20every%20tick%20whose%20timestamp%20has%20non-zero%20seconds%2C%20which%20suppresses%20legitimate%2
0labels%20for%20real%20second-level%20data%20%28not%20just%20phantom%20edge%20ticks%29.%20%60getXAxisFormatter%60%20is%20also%20used%20by%20charts%20like%20Gantt%2FBigNumber%20that%20do%20not%20pass%20a%20time%20grain%2C%20so%20valid%20axis%20labels%20can%20become%20blank%20across%20the%20chart.%20Restrict%20this%20suppression%20to%20proven%20synthetic%20edge%20ticks%20%28or%20to%20contexts%20where%20minute%20alignment%20is%20guaranteed%29%20instead%20of%20applying%20it%20to%20all%20no-grain%20temporal%20axes.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the
%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts
**Line:** 49:53
**Comment:**
*Logic Error: This new no-grain branch drops every tick whose timestamp
has non-zero seconds, which suppresses legitimate labels for real second-level
data (not just phantom edge ticks). `getXAxisFormatter` is also used by charts
like Gantt/BigNumber that do not pass a time grain, so valid axis labels can
become blank across the chart. Restrict this suppression to proven synthetic
edge ticks (or to contexts where minute alignment is guaranteed) instead of
applying it to all no-grain temporal axes.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39972&comment_hash=75390f58a805062e1bd6f278a854ddb5740601e0a8abf46b15da6f9721eeebde&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39972&comment_hash=75390f58a805062e1bd6f278a854ddb5740601e0a8abf46b15da6f9721eeebde&reaction=dislike'>👎</a>
--
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]
