Skip to content
Open
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,7 @@ export default function transformProps(
: String;
const xAxisFormatter =
xAxisDataType === GenericDataType.Temporal
? getXAxisFormatter(xAxisTimeFormat)
? getXAxisFormatter(xAxisTimeFormat, timeGrainSqla)
: xAxisDataType === GenericDataType.Numeric
? getNumberFormatter(xAxisNumberFormat)
: String;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,99 @@ import {
SMART_DATE_ID,
SMART_DATE_VERBOSE_ID,
TimeFormatter,
TimeGranularity,
ValueFormatter,
} from '@superset-ui/core';

export const getSmartDateDetailedFormatter = () =>
getTimeFormatter(SMART_DATE_DETAILED_ID);

export const getSmartDateFormatter = () => getTimeFormatter(SMART_DATE_ID);
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 === TimeGranularity.YEAR) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So quick question here. I see that the TimeGranularity has more pre-defined values like WEEK_STARTING_SUNDAY, WEEK_STARTING_MONDAY, WEEK_ENDING_SATURDAY, WEEK_ENDING_SUNDAY.

So I'm not sure if we should handle all these variations here to avoid displaying raw timestamps with milliseconds

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a nice catch. Yeah we should

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be handled now

// Set to January 1st at midnight UTC - smart formatter will show year
const year = normalizedDate.getUTCFullYear();
const cleanDate = new Date(Date.UTC(year, 0, 1, 0, 0, 0, 0));
return baseFormatter(cleanDate);
} else if (timeGrain === TimeGranularity.QUARTER) {
// Set to first month of quarter, first day, midnight UTC
const year = normalizedDate.getUTCFullYear();
const month = normalizedDate.getUTCMonth();
const quarterStartMonth = Math.floor(month / 3) * 3;
const cleanDate = new Date(
Date.UTC(year, quarterStartMonth, 1, 0, 0, 0, 0),
);
return baseFormatter(cleanDate);
} else if (timeGrain === TimeGranularity.MONTH) {
// Set to first of month at midnight UTC - smart formatter will show month name or year
const year = normalizedDate.getUTCFullYear();
const month = normalizedDate.getUTCMonth();
const cleanDate = new Date(Date.UTC(year, month, 1, 0, 0, 0, 0));
return baseFormatter(cleanDate);
} else if (
timeGrain === TimeGranularity.WEEK ||
timeGrain === TimeGranularity.WEEK_STARTING_SUNDAY ||
timeGrain === TimeGranularity.WEEK_STARTING_MONDAY ||
timeGrain === TimeGranularity.WEEK_ENDING_SATURDAY ||
timeGrain === TimeGranularity.WEEK_ENDING_SUNDAY
) {
// Set to midnight UTC, keep the day
const year = normalizedDate.getUTCFullYear();
const month = normalizedDate.getUTCMonth();
const day = normalizedDate.getUTCDate();
const cleanDate = new Date(Date.UTC(year, month, day, 0, 0, 0, 0));
return baseFormatter(cleanDate);
} else if (
timeGrain === TimeGranularity.DAY ||
timeGrain === TimeGranularity.DATE
) {
// Set to midnight UTC
const year = normalizedDate.getUTCFullYear();
const month = normalizedDate.getUTCMonth();
const day = normalizedDate.getUTCDate();
const cleanDate = new Date(Date.UTC(year, month, day, 0, 0, 0, 0));
return baseFormatter(cleanDate);
} else if (
timeGrain === TimeGranularity.HOUR ||
timeGrain === TimeGranularity.THIRTY_MINUTES ||
timeGrain === TimeGranularity.FIFTEEN_MINUTES ||
timeGrain === TimeGranularity.TEN_MINUTES ||
timeGrain === TimeGranularity.FIVE_MINUTES ||
timeGrain === TimeGranularity.MINUTE ||
timeGrain === TimeGranularity.SECOND
) {
Comment on lines +102 to +110
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Grouping all sub-hour time grains (e.g., 30/15/5 minutes, seconds) into the same branch as the hourly grain and normalizing them to the top of the hour zeros out their minute/second components, so distinct time buckets within the same hour produce identical formatted labels and misrepresent the actual bucket boundaries. Restrict this normalization to the pure hourly grain so that finer-grained buckets preserve their minute/second resolution while still having milliseconds stripped earlier in the function. [logic error]

Severity Level: Major ⚠️
- ⚠️ Timeseries chart x-axis mislabels sub-hour time buckets.
- ⚠️ Users may misinterpret 30/15/5-minute trend boundaries.
- ⚠️ Time-based analyses lose clarity at finer granularity.
Suggested change
} else if (
timeGrain === TimeGranularity.HOUR ||
timeGrain === TimeGranularity.THIRTY_MINUTES ||
timeGrain === TimeGranularity.FIFTEEN_MINUTES ||
timeGrain === TimeGranularity.TEN_MINUTES ||
timeGrain === TimeGranularity.FIVE_MINUTES ||
timeGrain === TimeGranularity.MINUTE ||
timeGrain === TimeGranularity.SECOND
) {
} else if (timeGrain === TimeGranularity.HOUR) {
Steps of Reproduction ✅
1. In Superset, create or open a Timeseries ECharts chart (handled by
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:117-211`),
using a temporal column on the x-axis so `xAxisDataType === GenericDataType.Temporal` (see
lines 250-252).

2. In the chart controls, set **Time Grain** to a sub-hour grain such as `30 minutes` so
that `timeGrainSqla` is a sub-hour `TimeGranularity` value like
`TimeGranularity.THIRTY_MINUTES` (`transformProps.ts:181`), and set **X-Axis Time Format**
to "Adaptive formatting" (maps to `SMART_DATE_ID`, passed as `xAxisTimeFormat` at
`transformProps.ts:186,199`).

3. On render, `transformProps` computes the x-axis formatter for temporal data as
`getXAxisFormatter(xAxisTimeFormat, timeGrainSqla)` at
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:559-566`.
With `xAxisTimeFormat = SMART_DATE_ID`, `getXAxisFormatter` returns
`getSmartDateFormatter(timeGrainSqla)` at
`superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts:172-178`.

4. Inside `getSmartDateFormatter`'s `formatFunc` (`formatters.ts:51-118`), when
`timeGrainSqla` is any of `TimeGranularity.THIRTY_MINUTES`, `FIFTEEN_MINUTES`,
`TEN_MINUTES`, `FIVE_MINUTES`, `MINUTE`, or `SECOND`, the current branch at
`formatters.ts:99-115` executes, normalizing each bucket's `Date` to the *top of the hour*
(minutes and seconds set to `0`) before passing it to the base SMART_DATE formatter. As a
result, distinct 30-minute (or finer) buckets within the same hour (e.g., `10:00` and
`10:30`) produce identical axis labels like `"2025-03-15 10:00"`, causing
duplicate/misleading x-axis labels in the rendered ECharts Timeseries chart.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts
**Line:** 99:107
**Comment:**
	*Logic Error: Grouping all sub-hour time grains (e.g., 30/15/5 minutes, seconds) into the same branch as the hourly grain and normalizing them to the top of the hour zeros out their minute/second components, so distinct time buckets within the same hour produce identical formatted labels and misrepresent the actual bucket boundaries. Restrict this normalization to the pure hourly grain so that finer-grained buckets preserve their minute/second resolution while still having milliseconds stripped earlier in the function.

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.
👍 | 👎

// Set to top of hour UTC
const year = normalizedDate.getUTCFullYear();
const month = normalizedDate.getUTCMonth();
const day = normalizedDate.getUTCDate();
const hour = normalizedDate.getUTCHours();
const cleanDate = new Date(Date.UTC(year, month, day, hour, 0, 0, 0));
return baseFormatter(cleanDate);
}

// Use the base formatter on the normalized date
return baseFormatter(normalizedDate);
},
});
};

export const getSmartDateVerboseFormatter = () =>
getTimeFormatter(SMART_DATE_VERBOSE_ID);
Expand Down Expand Up @@ -88,9 +174,10 @@ export function getTooltipTimeFormatter(

export function getXAxisFormatter(
format?: string,
timeGrain?: string,
): TimeFormatter | StringConstructor | undefined {
if (format === SMART_DATE_ID || !format) {
return undefined;
return getSmartDateFormatter(timeGrain);
}
if (format) {
return getTimeFormatter(format);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ describe('Scatter Chart X-axis Time Formatting', () => {
expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel');
const xAxis = transformedProps.echartOptions.xAxis as any;
expect(xAxis.axisLabel).toHaveProperty('formatter');
expect(xAxis.axisLabel.formatter).toBeUndefined();
expect(typeof xAxis.axisLabel.formatter).toBe('function');
});

test.each(D3_TIME_FORMAT_OPTIONS.map(([id]) => id))(
Expand All @@ -96,10 +96,8 @@ describe('Scatter Chart X-axis Time Formatting', () => {

const xAxis = transformedProps.echartOptions.xAxis as any;
expect(xAxis.axisLabel).toHaveProperty('formatter');
if (format === SMART_DATE_ID) {
expect(xAxis.axisLabel.formatter).toBeUndefined();
} else {
expect(typeof xAxis.axisLabel.formatter).toBe('function');
expect(typeof xAxis.axisLabel.formatter).toBe('function');
if (format !== SMART_DATE_ID) {
expect(xAxis.axisLabel.formatter.id).toBe(format);
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,166 @@
* specific language governing permissions and limitations
* under the License.
*/
import { NumberFormats } from '@superset-ui/core';
import { getPercentFormatter } from '../../src/utils/formatters';
import {
NumberFormats,
SMART_DATE_ID,
TimeFormatter,
TimeGranularity,
} from '@superset-ui/core';
import {
getPercentFormatter,
getXAxisFormatter,
} from '../../src/utils/formatters';

describe('getPercentFormatter', () => {
test('getPercentFormatter should format as percent if no format is specified', () => {
const value = 0.6;
test('should format as percent if no format is specified', () => {
expect(getPercentFormatter().format(value)).toEqual('60%');
});
test('should format as percent if SMART_NUMBER is specified', () => {
expect(
getPercentFormatter(NumberFormats.SMART_NUMBER).format(value),
).toEqual('60%');
});
test('should format using a provided format', () => {
expect(
getPercentFormatter(NumberFormats.PERCENT_2_POINT).format(value),
).toEqual('60.00%');
expect(getPercentFormatter().format(value)).toEqual('60%');
});

test('getPercentFormatter should format as percent if SMART_NUMBER is specified', () => {
const value = 0.6;
expect(getPercentFormatter(NumberFormats.SMART_NUMBER).format(value)).toEqual(
'60%',
);
});

test('getPercentFormatter should format using a provided format', () => {
const value = 0.6;
expect(
getPercentFormatter(NumberFormats.PERCENT_2_POINT).format(value),
).toEqual('60.00%');
});

test('getXAxisFormatter should return smart date formatter for SMART_DATE_ID format', () => {
const formatter = getXAxisFormatter(SMART_DATE_ID);
expect(formatter).toBeDefined();
expect(formatter).toBeInstanceOf(TimeFormatter);
expect((formatter as TimeFormatter).id).toBe(SMART_DATE_ID);
});

test('getXAxisFormatter should return smart date formatter for undefined format', () => {
const formatter = getXAxisFormatter();
expect(formatter).toBeDefined();
expect(formatter).toBeInstanceOf(TimeFormatter);
expect((formatter as TimeFormatter).id).toBe(SMART_DATE_ID);
});

test('getXAxisFormatter should return custom time formatter for custom format', () => {
const customFormat = '%Y-%m-%d';
const formatter = getXAxisFormatter(customFormat);
expect(formatter).toBeDefined();
expect(formatter).toBeInstanceOf(TimeFormatter);
expect((formatter as TimeFormatter).id).toBe(customFormat);
});

test('getXAxisFormatter smart date formatter should be returned and not undefined', () => {
const formatter = getXAxisFormatter(SMART_DATE_ID);
expect(formatter).toBeDefined();
expect(formatter).toBeInstanceOf(TimeFormatter);
expect((formatter as TimeFormatter).id).toBe(SMART_DATE_ID);

const undefinedFormatter = getXAxisFormatter(undefined);
expect(undefinedFormatter).toBeDefined();
expect(undefinedFormatter).toBeInstanceOf(TimeFormatter);
expect((undefinedFormatter as TimeFormatter).id).toBe(SMART_DATE_ID);

const emptyFormatter = getXAxisFormatter();
expect(emptyFormatter).toBeDefined();
expect(emptyFormatter).toBeInstanceOf(TimeFormatter);
expect((emptyFormatter as TimeFormatter).id).toBe(SMART_DATE_ID);
});

test('getXAxisFormatter time grain aware formatter should prevent millisecond and timestamp formats', () => {
const formatter = getXAxisFormatter(SMART_DATE_ID, TimeGranularity.MONTH);

// Test that dates with milliseconds don't show millisecond format
const dateWithMs = new Date('2025-03-15T21:13:32.389Z');
const result = (formatter as TimeFormatter).format(dateWithMs);
expect(result).not.toContain('.389ms');
expect(result).not.toMatch(/\.\d+ms/);
expect(result).not.toContain('PM');
expect(result).not.toContain('AM');
expect(result).not.toMatch(/\d{1,2}:\d{2}/); // No time format
});

test('getXAxisFormatter time grain aware formatting should prevent problematic formats', () => {
// Test that time grain aware formatter prevents the specific issues we solved
const monthFormatter = getXAxisFormatter(
SMART_DATE_ID,
TimeGranularity.MONTH,
);
const yearFormatter = getXAxisFormatter(SMART_DATE_ID, TimeGranularity.YEAR);
const dayFormatter = getXAxisFormatter(SMART_DATE_ID, TimeGranularity.DAY);

// Test dates that previously caused issues
const problematicDates = [
new Date('2025-03-15T21:13:32.389Z'), // Had .389ms issue
new Date('2025-04-01T02:30:00.000Z'), // Timezone edge case
new Date('2025-07-01T00:00:00.000Z'), // Month boundary
];

problematicDates.forEach(date => {
// Month formatter should not show milliseconds or PM/AM
const monthResult = (monthFormatter as TimeFormatter).format(date);
expect(monthResult).not.toMatch(/\.\d+ms/);
expect(monthResult).not.toMatch(/PM|AM/);
expect(monthResult).not.toMatch(/\d{1,2}:\d{2}:\d{2}/);

// Year formatter should not show milliseconds or PM/AM
const yearResult = (yearFormatter as TimeFormatter).format(date);
expect(yearResult).not.toMatch(/\.\d+ms/);
expect(yearResult).not.toMatch(/PM|AM/);
expect(yearResult).not.toMatch(/\d{1,2}:\d{2}:\d{2}/);

// Day formatter should not show milliseconds or seconds
const dayResult = (dayFormatter as TimeFormatter).format(date);
expect(dayResult).not.toMatch(/\.\d+ms/);
expect(dayResult).not.toMatch(/:\d{2}:\d{2}/); // No seconds
});
});

test('getXAxisFormatter time grain parameter should be passed correctly', () => {
// Test that formatter with time grain is different from formatter without
const formatterWithGrain = getXAxisFormatter(
SMART_DATE_ID,
TimeGranularity.MONTH,
);
const formatterWithoutGrain = getXAxisFormatter(SMART_DATE_ID);

expect(formatterWithGrain).toBeDefined();
expect(formatterWithoutGrain).toBeDefined();
expect(formatterWithGrain).toBeInstanceOf(TimeFormatter);
expect(formatterWithoutGrain).toBeInstanceOf(TimeFormatter);

// Both should be valid formatters
const testDate = new Date('2025-04-15T12:30:45.789Z');
const resultWithGrain = (formatterWithGrain as TimeFormatter).format(
testDate,
);
const resultWithoutGrain = (formatterWithoutGrain as TimeFormatter).format(
testDate,
);

expect(typeof resultWithGrain).toBe('string');
expect(typeof resultWithoutGrain).toBe('string');
expect(resultWithGrain.length).toBeGreaterThan(0);
expect(resultWithoutGrain.length).toBeGreaterThan(0);
});

test('getXAxisFormatter without time grain should use standard smart date behavior', () => {
const standardFormatter = getXAxisFormatter(SMART_DATE_ID);
const timeGrainFormatter = getXAxisFormatter(SMART_DATE_ID, undefined);

// Both should be equivalent when no time grain is provided
expect(standardFormatter).toBeDefined();
expect(timeGrainFormatter).toBeDefined();

// Test with a date that has time components
const testDate = new Date('2025-01-01T00:00:00.000Z');
const standardResult = (standardFormatter as TimeFormatter).format(testDate);
const timeGrainResult = (timeGrainFormatter as TimeFormatter).format(
testDate,
);

expect(standardResult).toBe(timeGrainResult);
});
Loading