Copilot commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2620411039
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,117 +63,160 @@ const offsetsToName = {
'060': ['GMT Standard Time - London', 'British Summer Time'],
};
-export type TimezoneSelectorProps = {
- onTimezoneChange: (value: string) => void;
- timezone?: string | null;
- minWidth?: string;
-};
+let cachedTimezoneOptions: TimezoneOption[] | null = null;
+let computePromise: Promise<TimezoneOption[]> | null = null;
+
+function getOffsetKey(timezoneName: string): string {
+ return (
+ JANUARY_REF.tz(timezoneName).utcOffset().toString() +
+ JULY_REF.tz(timezoneName).utcOffset().toString()
+ );
+}
+
+function getTimezoneDisplayName(
+ timezoneName: string,
+ currentDate: ReturnType<typeof extendedDayjs>,
+): string {
+ const offsetKey = getOffsetKey(timezoneName);
+ const dateInZone = currentDate.tz(timezoneName);
+ const isDSTActive = isDST(dateInZone, timezoneName);
+ const namePair = offsetsToName[offsetKey];
+ return namePair ? (isDSTActive ? namePair[1] : namePair[0]) : timezoneName;
+}
+
+function computeTimezoneOptions(): TimezoneOption[] {
+ const currentDate = extendedDayjs(new Date());
+ const allZones = Intl.supportedValuesOf('timeZone');
+ const seenLabels = new Set<string>();
+ const options: TimezoneOption[] = [];
+
+ for (const zone of allZones) {
+ const offsetKey = getOffsetKey(zone);
+ const displayName = getTimezoneDisplayName(zone, currentDate);
+ const offset = extendedDayjs.tz(currentDate, zone).format('Z');
+ const label = `GMT ${offset} (${displayName})`;
+
+ if (!seenLabels.has(label)) {
+ seenLabels.add(label);
+ options.push({
+ label,
+ value: zone,
+ offsets: offsetKey,
+ timezoneName: zone,
+ });
+ }
+ }
+
+ // Pre-sort timezone options by time offset
+ options.sort(
+ (a, b) =>
+ extendedDayjs.tz(currentDate, a.timezoneName).utcOffset() -
+ extendedDayjs.tz(currentDate, b.timezoneName).utcOffset(),
+ );
+
+ cachedTimezoneOptions = options;
+ return options;
+}
+
+function getTimezoneOptionsAsync(): Promise<TimezoneOption[]> {
+ if (cachedTimezoneOptions) {
+ return Promise.resolve(cachedTimezoneOptions);
+ }
+
+ if (computePromise) {
+ return computePromise;
+ }
+
+ // Use queueMicrotask for better performance than setTimeout(0)
+ // Falls back to setTimeout for older browsers
+ computePromise = new Promise<TimezoneOption[]>(resolve => {
+ if (typeof queueMicrotask === 'function') {
+ queueMicrotask(() => {
+ resolve(computeTimezoneOptions());
+ });
+ } else {
+ setTimeout(() => {
+ resolve(computeTimezoneOptions());
+ }, 0);
+ }
+ });
+
+ return computePromise;
+}
+
+function findMatchingTimezone(
+ timezone: string | null | undefined,
+ options: TimezoneOption[],
+): string {
+ const targetTimezone = timezone || extendedDayjs.tz.guess();
+ const targetOffsetKey = getOffsetKey(targetTimezone);
+ let fallbackValue: string | undefined;
+
+ for (const option of options) {
+ if (
+ option.offsets === targetOffsetKey &&
+ option.timezoneName === targetTimezone
+ ) {
+ return option.value;
+ }
+ if (!fallbackValue && option.offsets === targetOffsetKey) {
+ fallbackValue = option.value;
+ }
+ }
+
+ return fallbackValue || DEFAULT_TIMEZONE.value;
+}
export default function TimezoneSelector({
onTimezoneChange,
timezone,
- minWidth = MIN_SELECT_WIDTH, // smallest size for current values
+ minWidth = MIN_SELECT_WIDTH,
+ placeholder,
...rest
}: TimezoneSelectorProps) {
- const { TIMEZONE_OPTIONS, TIMEZONE_OPTIONS_SORT_COMPARATOR, validTimezone } =
- useMemo(() => {
- const currentDate = extendedDayjs();
- const JANUARY = extendedDayjs.tz('2021-01-01');
- const JULY = extendedDayjs.tz('2021-07-01');
-
- const getOffsetKey = (name: string) =>
- JANUARY.tz(name).utcOffset().toString() +
- JULY.tz(name).utcOffset().toString();
-
- const getTimezoneName = (name: string) => {
- const offsets = getOffsetKey(name);
- return (
- (isDST(currentDate.tz(name), name)
- ? offsetsToName[offsets as keyof typeof offsetsToName]?.[1]
- : offsetsToName[offsets as keyof typeof offsetsToName]?.[0]) ||
name
- );
- };
-
- // TODO: remove this ts-ignore when typescript is upgraded to 5.1
- // @ts-ignore
- const ALL_ZONES: string[] = Intl.supportedValuesOf('timeZone');
-
- const labels = new Set<string>();
- const TIMEZONE_OPTIONS = ALL_ZONES.map(zone => {
- const label = `GMT ${extendedDayjs
- .tz(currentDate, zone)
- .format('Z')} (${getTimezoneName(zone)})`;
-
- if (labels.has(label)) {
- return null; // Skip duplicates
- }
- labels.add(label);
- return {
- label,
- value: zone,
- offsets: getOffsetKey(zone),
- timezoneName: zone,
- };
- }).filter(Boolean) as {
- label: string;
- value: string;
- offsets: string;
- timezoneName: string;
- }[];
-
- const TIMEZONE_OPTIONS_SORT_COMPARATOR = (
- a: (typeof TIMEZONE_OPTIONS)[number],
- b: (typeof TIMEZONE_OPTIONS)[number],
- ) =>
- extendedDayjs.tz(currentDate, a.timezoneName).utcOffset() -
- extendedDayjs.tz(currentDate, b.timezoneName).utcOffset();
-
- // pre-sort timezone options by time offset
- TIMEZONE_OPTIONS.sort(TIMEZONE_OPTIONS_SORT_COMPARATOR);
-
- const matchTimezoneToOptions = (timezone: string) => {
- const offsetKey = getOffsetKey(timezone);
- let fallbackValue: string | undefined;
-
- for (const option of TIMEZONE_OPTIONS) {
- if (
- option.offsets === offsetKey &&
- option.timezoneName === timezone
- ) {
- return option.value;
- }
- if (!fallbackValue && option.offsets === offsetKey) {
- fallbackValue = option.value;
- }
- }
- return fallbackValue || DEFAULT_TIMEZONE.value;
- };
-
- const validTimezone = matchTimezoneToOptions(
- timezone || extendedDayjs.tz.guess(),
- );
-
- return {
- TIMEZONE_OPTIONS,
- TIMEZONE_OPTIONS_SORT_COMPARATOR,
- validTimezone,
- };
- }, [timezone]);
-
- // force trigger a timezone update if provided `timezone` is not invalid
- useEffect(() => {
- if (validTimezone && timezone !== validTimezone) {
- onTimezoneChange(validTimezone);
+ const [timezoneOptions, setTimezoneOptions] = useState<
+ TimezoneOption[] | null
+ >(cachedTimezoneOptions);
+ const [isLoadingOptions, setIsLoadingOptions] = useState(false);
+
+ const handleOpenChange = useCallback(
+ (isOpen: boolean) => {
+ if (isOpen && !timezoneOptions && !isLoadingOptions) {
+ setIsLoadingOptions(true);
+ getTimezoneOptionsAsync()
+ .then(setTimezoneOptions)
+ .finally(() => setIsLoadingOptions(false));
+ }
+ },
+ [timezoneOptions, isLoadingOptions],
+ );
+
+ const sortComparator = useMemo(() => {
+ if (!timezoneOptions) return undefined;
+ const currentDate = extendedDayjs();
+ return (a: TimezoneOption, b: TimezoneOption) =>
+ extendedDayjs.tz(currentDate, a.timezoneName).utcOffset() -
+ extendedDayjs.tz(currentDate, b.timezoneName).utcOffset();
+ }, [timezoneOptions]);
Review Comment:
The `sortComparator` computes `extendedDayjs.tz()` for each comparison
during sort operations, which is expensive and unnecessary since options are
already pre-sorted in `computeTimezoneOptions()`. Consider removing this
comparator entirely (pass `undefined` or omit the prop) to avoid redundant
computation, as the options array is already sorted before being set in state.
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/TimezoneSelector.DaylightSavingTime.test.tsx:
##########
@@ -37,25 +37,55 @@ afterEach(() => {
});
test('render timezones in correct order for daylight saving time', async () =>
{
+ // Set system time BEFORE loading component to ensure cache uses correct date
+ jest.useFakeTimers();
+ jest.setSystemTime(new Date('2022-07-01'));
+
const TimezoneSelector = await loadComponent('2022-07-01');
const onTimezoneChange = jest.fn();
- render(
+ const { container } = render(
<TimezoneSelector
onTimezoneChange={onTimezoneChange}
timezone="America/Nassau"
/>,
);
+ // Trigger data loading by clicking the select
const searchInput = screen.getByRole('combobox');
await userEvent.click(searchInput);
+ // Run timers to execute queueMicrotask/setTimeout callback
+ jest.runAllTimers();
+
+ // Wait for timezone data to load
+ await waitFor(() => {
+ expect(screen.queryByText('Loading...')).not.toBeInTheDocument();
+ });
+
+ // Verify the selected timezone is displayed correctly (in DST)
+ const selectionItem = container.querySelector('.ant-select-selection-item');
+ expect(selectionItem).toHaveTextContent('GMT -04:00 (Eastern Daylight
Time)');
+
+ // Verify options are sorted by UTC offset (lowest/most negative first)
const options = await waitFor(() =>
document.querySelectorAll('.ant-select-item-option-content'),
);
- // first option is always current timezone
- expect(options[0]).toHaveTextContent('GMT -04:00 (Eastern Daylight Time)');
- expect(options[1]).toHaveTextContent('GMT -11:00 (Pacific/Midway)');
- expect(options[2]).toHaveTextContent('GMT -11:00 (Pacific/Niue)');
- expect(options[3]).toHaveTextContent('GMT -11:00 (Pacific/Pago_Pago)');
+ // Options should be sorted by offset: -11:00 comes before -04:00
+ expect(options[0]).toHaveTextContent('GMT -11:00 (Pacific/Midway)');
+ expect(options[1]).toHaveTextContent('GMT -11:00 (Pacific/Niue)');
+ expect(options[2]).toHaveTextContent('GMT -11:00 (Pacific/Pago_Pago)');
+
+ // Find the Eastern Daylight Time option
+ // Virtual list only renders visible items, so we search the DOM for the
option element
+ // by its aria-label attribute which should be available even if not rendered
Review Comment:
The comment states that the aria-label attribute 'should be available even
if not rendered,' but virtual list items that are not rendered typically do not
exist in the DOM at all. This comment may be misleading. Consider clarifying
that the test waits for the option to be rendered when scrolled into view, or
adjust the test approach if the option is not guaranteed to be in the DOM.
```suggestion
// Virtual list only renders visible items, so we wait for the option to
be rendered
// and then search the DOM for the option element by its aria-label
attribute
```
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,117 +63,160 @@ const offsetsToName = {
'060': ['GMT Standard Time - London', 'British Summer Time'],
};
-export type TimezoneSelectorProps = {
- onTimezoneChange: (value: string) => void;
- timezone?: string | null;
- minWidth?: string;
-};
+let cachedTimezoneOptions: TimezoneOption[] | null = null;
+let computePromise: Promise<TimezoneOption[]> | null = null;
+
+function getOffsetKey(timezoneName: string): string {
+ return (
+ JANUARY_REF.tz(timezoneName).utcOffset().toString() +
+ JULY_REF.tz(timezoneName).utcOffset().toString()
+ );
+}
+
+function getTimezoneDisplayName(
+ timezoneName: string,
+ currentDate: ReturnType<typeof extendedDayjs>,
+): string {
+ const offsetKey = getOffsetKey(timezoneName);
+ const dateInZone = currentDate.tz(timezoneName);
+ const isDSTActive = isDST(dateInZone, timezoneName);
+ const namePair = offsetsToName[offsetKey];
+ return namePair ? (isDSTActive ? namePair[1] : namePair[0]) : timezoneName;
+}
+
+function computeTimezoneOptions(): TimezoneOption[] {
+ const currentDate = extendedDayjs(new Date());
+ const allZones = Intl.supportedValuesOf('timeZone');
+ const seenLabels = new Set<string>();
+ const options: TimezoneOption[] = [];
+
+ for (const zone of allZones) {
+ const offsetKey = getOffsetKey(zone);
+ const displayName = getTimezoneDisplayName(zone, currentDate);
+ const offset = extendedDayjs.tz(currentDate, zone).format('Z');
+ const label = `GMT ${offset} (${displayName})`;
+
+ if (!seenLabels.has(label)) {
+ seenLabels.add(label);
+ options.push({
+ label,
+ value: zone,
+ offsets: offsetKey,
+ timezoneName: zone,
+ });
+ }
+ }
+
+ // Pre-sort timezone options by time offset
+ options.sort(
+ (a, b) =>
+ extendedDayjs.tz(currentDate, a.timezoneName).utcOffset() -
+ extendedDayjs.tz(currentDate, b.timezoneName).utcOffset(),
+ );
Review Comment:
The sort order is reversed. The comment states 'Pre-sort timezone options by
time offset' but the subtraction order produces descending order (most positive
offset first). Based on the test expectations (e.g., GMT -11:00 before GMT
-04:00) and typical timezone selector UX, the sort should be ascending. Swap
the operands: `b.timezoneName` minus `a.timezoneName`.
--
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]
