You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "LuisSanchez (via GitHub)" <gi...@apache.org> on 2025/12/10 02:35:36 UTC
[PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
LuisSanchez opened a new pull request, #36486:
URL: https://github.com/apache/superset/pull/36486
### SUMMARY
Optimized `TimezoneSelector` to fix a ~550ms delay when opening the Schedule panel in the Alert creation modal. Timezone data now loads on demand when the user opens the dropdown, with a loading indicator.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
## Before
https://github.com/user-attachments/assets/712abe50-973a-403e-8141-9096554fefd7
## After
https://github.com/user-attachments/assets/b22d86bf-efea-42ce-afcf-fccd9a432c0a
### TESTING INSTRUCTIONS
1. Create an alert or report.
2. Click to open the Schedule section of the modal.
### Details on the refactor
### Performance
- Lazy loading: timezone options load only when the dropdown opens (via `onOpenChange`)
- Async computation: uses `queueMicrotask`/`setTimeout(0)` to avoid blocking the main thread
- Global caching: computed options are cached for subsequent renders
### UX
- Loading indicator: shows spinner and "Loading timezones..." while data loads
- Dropdown stability: added `minHeight: 265px` to prevent repositioning after data loads
- Conditional value: only sets the selected value after options are loaded
### Code quality
- Extracted helper functions (`getOffsetKey`, `getTimezoneDisplayName`, `findMatchingTimezone`)
- Reorganized file structure: types β constants β helpers β component
- Removed code duplication and improved readability
### Testing
- Updated tests to handle async loading behavior
- Fixed DST test to correctly verify timezone formatting
## Impact
- Schedule panel opens instantly (0ms delay)
- Timezone dropdown shows loading state during computation (~150ms, only on first open)
- Subsequent opens are instant (cached)
- No functionality changes, only performance improvements
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3688001097
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3635112629
CodeAnt AI finished reviewing your PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3715986119
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2631150798
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +65,183 @@ 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 = currentDate.tz(zone).format('Z');
+ const label = `GMT ${offset} (${displayName})`;
+
+ if (!seenLabels.has(label)) {
+ seenLabels.add(label);
+ options.push({
+ label,
+ value: zone,
+ offsets: offsetKey,
+ timezoneName: zone,
+ });
+ }
+ }
+
+ 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, reject) => {
+ const run = () => {
+ try {
+ const result = computeTimezoneOptions();
+ resolve(result);
+ } catch (err) {
+ reject(err);
+ } finally {
+ computePromise = null;
+ }
+ };
+ if (typeof queueMicrotask === 'function') {
+ queueMicrotask(run);
+ } else {
+ setTimeout(run, 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
+ const [timezoneOptions, setTimezoneOptions] = useState<
+ TimezoneOption[] | null
+ >(cachedTimezoneOptions);
+ const [isLoadingOptions, setIsLoadingOptions] = useState(false);
+ const hasSetDefaultRef = useRef(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) =>
+ currentDate.tz(a.timezoneName).utcOffset() -
+ currentDate.tz(b.timezoneName).utcOffset();
+ }, [timezoneOptions]);
+
+ const validTimezone = useMemo(() => {
+ if (!timezoneOptions) {
+ return timezone || extendedDayjs.tz.guess();
+ }
+ return findMatchingTimezone(timezone, timezoneOptions);
+ }, [timezone, timezoneOptions]);
+
+ // Preload timezone options on mount and set default value
useEffect(() => {
- if (validTimezone && timezone !== validTimezone) {
- onTimezoneChange(validTimezone);
+ if (!timezoneOptions && !isLoadingOptions) {
+ setIsLoadingOptions(true);
+ getTimezoneOptionsAsync()
+ .then(options => {
+ setTimezoneOptions(options);
+ // Set default value if no timezone is provided and we haven't set it yet
+ if (!timezone && !hasSetDefaultRef.current) {
+ const defaultTz = findMatchingTimezone(null, options);
+ onTimezoneChange(defaultTz);
+ hasSetDefaultRef.current = true;
+ }
+ })
+ .finally(() => setIsLoadingOptions(false));
+ } else if (timezoneOptions && !timezone && !hasSetDefaultRef.current) {
+ // If options are already available (cached), set default immediately
+ const defaultTz = findMatchingTimezone(null, timezoneOptions);
+ onTimezoneChange(defaultTz);
+ hasSetDefaultRef.current = true;
}
- }, [validTimezone, onTimezoneChange, timezone]);
+ }, [timezoneOptions, isLoadingOptions, timezone, onTimezoneChange]);
return (
<Select
ariaLabel={t('Timezone selector')}
onChange={tz => onTimezoneChange(tz as string)}
- value={validTimezone}
- options={TIMEZONE_OPTIONS}
- sortComparator={TIMEZONE_OPTIONS_SORT_COMPARATOR}
- {...rest}
+ onOpenChange={handleOpenChange}
+ value={timezoneOptions ? validTimezone : undefined}
+ options={timezoneOptions || []}
+ sortComparator={sortComparator}
+ loading={isLoadingOptions}
+ placeholder={isLoadingOptions ? t('Loading timezones...') : placeholder}
+ {...({ placement: 'topLeft', ...rest } as any)}
Review Comment:
Can we avoid `any`?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2664980458
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +60,142 @@ const offsetsToName = {
'060': ['GMT Standard Time - London', 'British Summer Time'],
};
-export type TimezoneSelectorProps = {
- onTimezoneChange: (value: string) => void;
- timezone?: string | null;
- minWidth?: string;
-};
+function getOffsetKey(timezoneName: string): string {
+ return (
+ JANUARY_REF.tz(timezoneName).utcOffset().toString() +
+ JULY_REF.tz(timezoneName).utcOffset().toString()
+ );
+}
+
+const timezoneCache = new TimezoneOptionsCache(getOffsetKey, offsetsToName);
+
+// Export function to check if options are cached (for parent components)
+export function areTimezoneOptionsCached(): boolean {
+ return timezoneCache.isCached();
+}
+
+function getTimezoneOptionsAsync(): Promise<TimezoneOption[]> {
+ return timezoneCache.getOptionsAsync();
+}
Review Comment:
Should we export the object instead? These two functions doesn't seem that useful now
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #36486:
URL: https://github.com/apache/superset/pull/36486
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3756243529
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/21041641101) for [afdb112](https://github.com/apache/superset/commit/afdb112)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3756313111
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/21041641101) for [afdb112](https://github.com/apache/superset/commit/afdb112)
β’ **Environment:** http://35.93.145.144:8080 (admin/admin)
β’ **Lifetime:** 48h auto-cleanup
β’ **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3751683726
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2625201461
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -17,19 +17,37 @@
* under the License.
*/
-import { useEffect, useMemo } from 'react';
+import { useCallback, useMemo, useState } from 'react';
import { t } from '@superset-ui/core';
import { Select } from '@superset-ui/core/components';
import { isDST, extendedDayjs } from '../../utils/dates';
+export type TimezoneSelectorProps = {
+ onTimezoneChange: (value: string) => void;
+ timezone?: string | null;
+ minWidth?: string;
+ placeholder?: string;
+};
+
+type TimezoneOption = {
+ label: string;
+ value: string;
+ offsets: string;
+ timezoneName: string;
+};
+
const DEFAULT_TIMEZONE = {
name: 'GMT Standard Time',
value: 'Africa/Abidjan', // timezones are deduped by the first alphabetical value
};
const MIN_SELECT_WIDTH = '400px';
+const DROPDOWN_MIN_HEIGHT = 265;
Review Comment:
`placement: 'topLeft'` did the trick!, it is done!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2665411080
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +60,142 @@ const offsetsToName = {
'060': ['GMT Standard Time - London', 'British Summer Time'],
};
-export type TimezoneSelectorProps = {
- onTimezoneChange: (value: string) => void;
- timezone?: string | null;
- minWidth?: string;
-};
+function getOffsetKey(timezoneName: string): string {
+ return (
+ JANUARY_REF.tz(timezoneName).utcOffset().toString() +
+ JULY_REF.tz(timezoneName).utcOffset().toString()
+ );
+}
+
+const timezoneCache = new TimezoneOptionsCache(getOffsetKey, offsetsToName);
+
+// Export function to check if options are cached (for parent components)
+export function areTimezoneOptionsCached(): boolean {
+ return timezoneCache.isCached();
+}
+
+function getTimezoneOptionsAsync(): Promise<TimezoneOption[]> {
+ return timezoneCache.getOptionsAsync();
+}
Review Comment:
#### TL;DR;
The `areTimezoneOptionsCached()` function makes sense because it's exported and provides a public API. `getTimezoneOptionsAsync()` is internal-only and doesn't need a wrapper.
#### areTimezoneOptionsCached()
Basically it provides a focused, stable API that matches the current needs. If more functionality is needed later, more functions is better rather than exposing the class. This keeps the API simple and maintainable.
With the function instead of the class we gain encapsulation, it hides the `TimezoneOptionsCache` instance and its dependencies. Consumers don't need to know about `getOffsetKey` or `offsetsToName`.
Also principle of least privilege, it exposes only what's needed (a boolean check) with the current usage only needs `isCached()`.
`areTimezoneOptionsCached()` is straightforward, no need to understand the class structure.
#### getTimezoneOptionsAsync
We can certainly remove it since it is used internally and is a pass-through.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3716176999
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2644298754
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/TimezoneSelector.DaylightSavingTime.test.tsx:
##########
@@ -46,12 +47,15 @@ test('render timezones in correct order for daylight saving time', async () => {
/>,
);
+ // Wait for loading to complete by waiting for expected timezone text
+ await screen.findByText('GMT -04:00 (Eastern Daylight Time)');
+
const searchInput = screen.getByRole('combobox');
await userEvent.click(searchInput);
- const options = await waitFor(() =>
- document.querySelectorAll('.ant-select-item-option-content'),
- );
+ // Wait for options to appear by finding one of the expected timezone texts
+ await screen.findByText('GMT -11:00 (Pacific/Midway)');
+ const options = document.querySelectorAll('.ant-select-item-option-content');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Fix async test waiting</b></div>
<div id="fix">
The old waitFor(() => document.querySelectorAll(...)) was incorrectβit didn't assert or throw on failure, so it wouldn't actually wait for options to load, potentially making the test flaky. The new approach uses findByText to properly wait for async DOM updates, aligning with React Testing Library best practices.
</div>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a href="https://github.com/apache/superset/blob/a9ff796/.cursor/rules/dev-standard.mdc#L38">dev-standard.mdc:38</a>
</li>
<li>
Rule Violated: <a href="https://github.com/apache/superset/blob/a9ff796/AGENTS.md#L21">AGENTS.md:21</a>
</li>
</ul>
</details>
<small><i>Code Review Run #420f57</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2691735150
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -17,146 +17,149 @@
* under the License.
*/
-import { useEffect, useMemo } from 'react';
+import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
import { t } from '@superset-ui/core';
import { Select } from '@superset-ui/core/components';
-import { isDST, extendedDayjs } from '../../utils/dates';
+import { extendedDayjs } from '../../utils/dates';
+import {
+ timezoneOptionsCache,
+ getOffsetKey,
+ DEFAULT_TIMEZONE,
+} from './TimezoneOptionsCache';
+import type { TimezoneOption } from './types';
-const DEFAULT_TIMEZONE = {
- name: 'GMT Standard Time',
- value: 'Africa/Abidjan', // timezones are deduped by the first alphabetical value
-};
-
-const MIN_SELECT_WIDTH = '400px';
-
-const offsetsToName = {
- '-300-240': ['Eastern Standard Time', 'Eastern Daylight Time'],
- '-360-300': ['Central Standard Time', 'Central Daylight Time'],
- '-420-360': ['Mountain Standard Time', 'Mountain Daylight Time'],
- '-420-420': [
- 'Mountain Standard Time - Phoenix',
- 'Mountain Standard Time - Phoenix',
- ],
- '-480-420': ['Pacific Standard Time', 'Pacific Daylight Time'],
- '-540-480': ['Alaska Standard Time', 'Alaska Daylight Time'],
- '-600-600': ['Hawaii Standard Time', 'Hawaii Daylight Time'],
- '60120': ['Central European Time', 'Central European Daylight Time'],
- '00': [DEFAULT_TIMEZONE.name, DEFAULT_TIMEZONE.name],
- '060': ['GMT Standard Time - London', 'British Summer Time'],
-};
+// Import dayjs plugin types for TypeScript support
+import 'dayjs/plugin/timezone';
export type TimezoneSelectorProps = {
onTimezoneChange: (value: string) => void;
timezone?: string | null;
minWidth?: string;
+ placeholder?: string;
};
+const MIN_SELECT_WIDTH = '400px';
+
+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 [timezoneOptions, setTimezoneOptions] = useState<
+ TimezoneOption[] | null
+ >(timezoneOptionsCache.getOptions());
+ const [isLoadingOptions, setIsLoadingOptions] = useState(false);
+ const hasSetDefaultRef = useRef(false);
+
+ const handleOpenChange = useCallback(
+ (isOpen: boolean) => {
+ if (isOpen && !timezoneOptions && !isLoadingOptions) {
+ setIsLoadingOptions(true);
+ timezoneOptionsCache
+ .getOptionsAsync()
+ .then(options => {
+ setTimezoneOptions(options);
+ })
+ .finally(() => setIsLoadingOptions(false));
+ }
+ },
+ [timezoneOptions, isLoadingOptions],
+ );
+
+ const sortComparator = useMemo(() => {
+ if (!timezoneOptions) return undefined;
+ const currentDate = extendedDayjs();
+ const comparator = (a: TimezoneOption, b: TimezoneOption) =>
+ currentDate.tz(a.timezoneName).utcOffset() -
+ currentDate.tz(b.timezoneName).utcOffset();
+ return comparator;
+ }, [timezoneOptions]);
+
+ const validTimezone = useMemo(() => {
+ let result: string | undefined;
+ if (!timezoneOptions) {
+ // Don't call tz.guess() synchronously to avoid blocking render
+ // Return timezone if provided, otherwise undefined (will be set after options load)
+ result = timezone || undefined;
+ } else {
+ result = findMatchingTimezone(timezone, timezoneOptions);
+ }
+ return result;
+ }, [timezone, timezoneOptions]);
+
+ // Load timezone options asynchronously when component mounts
+ // Parent component (AlertReportModal) already delays mounting until panel opens
+ useEffect(() => {
+ if (timezoneOptions || isLoadingOptions) return;
+
+ setIsLoadingOptions(true);
- const validTimezone = matchTimezoneToOptions(
- timezone || extendedDayjs.tz.guess(),
- );
+ timezoneOptionsCache
+ .getOptionsAsync()
+ .then(options => {
+ setTimezoneOptions(options);
- return {
- TIMEZONE_OPTIONS,
- TIMEZONE_OPTIONS_SORT_COMPARATOR,
- validTimezone,
- };
- }, [timezone]);
+ // Set default value if no timezone is provided and we haven't set it yet
+ if (!timezone && !hasSetDefaultRef.current) {
+ const defaultTz = findMatchingTimezone(null, options);
+ onTimezoneChange(defaultTz);
+ hasSetDefaultRef.current = true;
+ }
+ })
+ .finally(() => {
+ setIsLoadingOptions(false);
+ });
+ }, [timezoneOptions, isLoadingOptions, timezone, onTimezoneChange]);
- // force trigger a timezone update if provided `timezone` is not invalid
+ // Set default value when cached options are available on mount
useEffect(() => {
- if (validTimezone && timezone !== validTimezone) {
- onTimezoneChange(validTimezone);
- }
- }, [validTimezone, onTimezoneChange, timezone]);
+ if (!timezoneOptions || timezone || hasSetDefaultRef.current) return;
+
+ const defaultTz = findMatchingTimezone(null, timezoneOptions);
+ onTimezoneChange(defaultTz);
+ hasSetDefaultRef.current = true;
+ }, [timezoneOptions, timezone, onTimezoneChange]);
+
+ const selectValue = timezoneOptions ? validTimezone : undefined;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing invalid timezone prop update</b></div>
<div id="fix">
The refactored async loading removes the original logic that forces an update to the parent component when an invalid timezone prop is provided. This can cause the select to display a corrected timezone while the parent's state remains invalid, leading to potential inconsistencies. To restore the behavior, add a useEffect that calls onTimezoneChange if validTimezone differs from the provided timezone after options are loaded.
</div>
</div>
<small><i>Code Review Run #bba88c</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2668164625
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/TimezoneOptionsCache.ts:
##########
Review Comment:
Indeed, file extension updated!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3718780686
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2610658049
##########
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)');
Review Comment:
Highlighting this one to check if current timezone showing first is intended behaviour and a regression or not
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3642079814
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20135260185) for [24a9771](https://github.com/apache/superset/commit/24a9771)
β’ **Environment:** http://54.184.58.196:8080 (admin/admin)
β’ **Lifetime:** 48h auto-cleanup
β’ **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3712274442
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3756264505
β οΈ **DEPRECATED WORKFLOW** β οΈ
@sadpandajoe This workflow is deprecated! Please use the new **Superset Showtime** system instead:
- Replace "testenv-up" label with "πͺ trigger-start"
- Better reliability and easier management
- See https://github.com/mistercrunch/superset-showtime for details
Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/21041816975). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3635100932
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2604983820
##########
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'),
);
Review Comment:
**Suggestion:** The call to `waitFor(() => document.querySelectorAll(...))` will resolve immediately because an empty NodeList is truthy, causing the test to proceed before options are rendered and making `options[0]`/`options[1]`/`options[2]` undefined; change the wait condition to assert on the NodeList length (e.g., wait until length > 2) so the test only continues once the expected items exist. [possible bug]
**Severity Level:** Critical π¨
```suggestion
const options = await waitFor(() => {
const opts = document.querySelectorAll('.ant-select-item-option-content');
expect(opts.length).toBeGreaterThan(2);
return opts;
});
```
<details>
<summary><b>Why it matters? β </b></summary>
Correct. waitFor will resolve as soon as the callback returns (a NodeList is truthy even when empty),
so the test can proceed with an empty NodeList and lead to undefined indexing. The proposed change
asserts on opts.length inside the waitFor, which creates a proper synchronization point.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/TimezoneSelector.DaylightSavingTime.test.tsx
**Line:** 70:72
**Comment:**
*Possible Bug: The call to `waitFor(() => document.querySelectorAll(...))` will resolve immediately because an empty NodeList is truthy, causing the test to proceed before options are rendered and making `options[0]`/`options[1]`/`options[2]` undefined; change the wait condition to assert on the NodeList length (e.g., wait until length > 2) so the test only continues once the expected items exist.
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.
```
</details>
##########
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);
Review Comment:
**Suggestion:** `getTimezoneOptionsAsync` assigns `computePromise` but never clears it after resolution or rejection; if the promise rejects or future recomputations are needed, callers may receive a rejected or stale promise β clear `computePromise` once the async work completes (in finally) to allow retries and avoid stuck state. [possible bug]
**Severity Level:** Critical π¨
```suggestion
computePromise = new Promise<TimezoneOption[]>((resolve, reject) => {
const run = () => {
try {
const result = computeTimezoneOptions();
resolve(result);
} catch (err) {
reject(err);
} finally {
computePromise = null;
}
};
if (typeof queueMicrotask === 'function') {
queueMicrotask(run);
} else {
setTimeout(run, 0);
```
<details>
<summary><b>Why it matters? β </b></summary>
If computeTimezoneOptions throws in some environment, computePromise would remain a rejected promise and future calls would keep returning that rejected promise, preventing retries. Clearing computePromise in a finally (or after resolution/rejection) improves robustness. The improved snippet handles errors and resets computePromise so callers can retry.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx
**Line:** 132:140
**Comment:**
*Possible Bug: `getTimezoneOptionsAsync` assigns `computePromise` but never clears it after resolution or rejection; if the promise rejects or future recomputations are needed, callers may receive a rejected or stale promise β clear `computePromise` once the async work completes (in finally) to allow retries and avoid stuck state.
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.
```
</details>
##########
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:
**Suggestion:** `computeTimezoneOptions` uses `extendedDayjs.tz(currentDate, zone)` where `currentDate` is a Dayjs instance; the dayjs timezone API expects either `date.tz(zone)` or `dayjs.tz(dateString, zone)`, so calling `extendedDayjs.tz` with a Dayjs object can be unsupported and cause runtime errors; use `currentDate.tz(zone)` and `currentDate.tz(...).format(...)` consistently. [possible bug]
**Severity Level:** Critical π¨
```suggestion
const offset = currentDate.tz(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) =>
currentDate.tz(a.timezoneName).utcOffset() -
currentDate.tz(b.timezoneName).utcOffset(),
```
<details>
<summary><b>Why it matters? β </b></summary>
The current code mixes extendedDayjs.tz(currentDate, zone) with other usages like currentDate.tz(zone). The safe, consistent API is to call .tz on a Dayjs instance (currentDate.tz(zone)). The improved code correctly normalizes these uses and avoids potential runtime errors depending on the dayjs-timezone API surface.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx
**Line:** 96:114
**Comment:**
*Possible Bug: `computeTimezoneOptions` uses `extendedDayjs.tz(currentDate, zone)` where `currentDate` is a Dayjs instance; the dayjs timezone API expects either `date.tz(zone)` or `dayjs.tz(dateString, zone)`, so calling `extendedDayjs.tz` with a Dayjs object can be unsupported and cause runtime errors; use `currentDate.tz(zone)` and `currentDate.tz(...).format(...)` consistently.
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.
```
</details>
##########
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();
Review Comment:
**Suggestion:** The comparator in `useMemo` uses `extendedDayjs.tz(currentDate, ...)` with `currentDate` as a Dayjs instance, which is inconsistent with the dayjs API used elsewhere and can cause runtime errors; use `currentDate.tz(...)` instead. [possible bug]
**Severity Level:** Critical π¨
```suggestion
currentDate.tz(a.timezoneName).utcOffset() -
currentDate.tz(b.timezoneName).utcOffset();
```
<details>
<summary><b>Why it matters? β </b></summary>
This is the same inconsistency as in computeTimezoneOptions: calling extendedDayjs.tz with a Dayjs instance is inconsistent and risky. Replacing with currentDate.tz(...) is correct, consistent with other code in the file, and avoids potential dayjs-timezone API misuse.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx
**Line:** 198:199
**Comment:**
*Possible Bug: The comparator in `useMemo` uses `extendedDayjs.tz(currentDate, ...)` with `currentDate` as a Dayjs instance, which is inconsistent with the dayjs API used elsewhere and can cause runtime errors; use `currentDate.tz(...)` instead.
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.
```
</details>
##########
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)');
Review Comment:
**Suggestion:** `container.querySelector(...)` can return null; calling `toHaveTextContent` on a null value will throw β assert the element exists (or guard) before checking its text content to avoid a null dereference. [null pointer]
**Severity Level:** Minor β οΈ
```suggestion
expect(selectionItem).not.toBeNull();
if (selectionItem) {
expect(selectionItem).toHaveTextContent('GMT -04:00 (Eastern Daylight Time)');
}
```
<details>
<summary><b>Why it matters? β </b></summary>
Also correct. querySelector can return null and calling toHaveTextContent on null will throw.
Guarding with not.toBeNull() (or using a get* query) avoids a TypeError and makes the test failure
clearer if the element is missing.
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/TimezoneSelector.DaylightSavingTime.test.tsx
**Line:** 67:67
**Comment:**
*Null Pointer: `container.querySelector(...)` can return null; calling `toHaveTextContent` on a null value will throw β assert the element exists (or guard) before checking its text content to avoid a null dereference.
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.
```
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2625204729
##########
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:
Here the assignment of `offset` was updated, pre-sorted was removed since the `Select` component always sort, so this was not needed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2625205211
##########
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)');
Review Comment:
Suggestion taken into consideration on latest commit
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2620405138
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -17,19 +17,37 @@
* under the License.
*/
-import { useEffect, useMemo } from 'react';
+import { useCallback, useMemo, useState } from 'react';
import { t } from '@superset-ui/core';
import { Select } from '@superset-ui/core/components';
import { isDST, extendedDayjs } from '../../utils/dates';
+export type TimezoneSelectorProps = {
+ onTimezoneChange: (value: string) => void;
+ timezone?: string | null;
+ minWidth?: string;
+ placeholder?: string;
+};
+
+type TimezoneOption = {
+ label: string;
+ value: string;
+ offsets: string;
+ timezoneName: string;
+};
+
const DEFAULT_TIMEZONE = {
name: 'GMT Standard Time',
value: 'Africa/Abidjan', // timezones are deduped by the first alphabetical value
};
const MIN_SELECT_WIDTH = '400px';
+const DROPDOWN_MIN_HEIGHT = 265;
Review Comment:
Is there any way around needing this? These always seem "icky"
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2662964476
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +62,213 @@ 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;
+
+// Export function to check if options are cached (for parent components)
+export function areTimezoneOptionsCached(): boolean {
+ return cachedTimezoneOptions !== 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 = currentDate.tz(zone).format('Z');
+ const label = `GMT ${offset} (${displayName})`;
+
+ if (!seenLabels.has(label)) {
+ seenLabels.add(label);
+ options.push({
+ label,
+ value: zone,
+ offsets: offsetKey,
+ timezoneName: zone,
+ });
+ }
+ }
+
+ cachedTimezoneOptions = options;
Review Comment:
As discussed, the refactor was done
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2665059252
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +60,142 @@ const offsetsToName = {
'060': ['GMT Standard Time - London', 'British Summer Time'],
};
-export type TimezoneSelectorProps = {
- onTimezoneChange: (value: string) => void;
- timezone?: string | null;
- minWidth?: string;
-};
+function getOffsetKey(timezoneName: string): string {
+ return (
+ JANUARY_REF.tz(timezoneName).utcOffset().toString() +
+ JULY_REF.tz(timezoneName).utcOffset().toString()
+ );
+}
+
+const timezoneCache = new TimezoneOptionsCache(getOffsetKey, offsetsToName);
+
+// Export function to check if options are cached (for parent components)
+export function areTimezoneOptionsCached(): boolean {
+ return timezoneCache.isCached();
+}
+
+function getTimezoneOptionsAsync(): Promise<TimezoneOption[]> {
+ return timezoneCache.getOptionsAsync();
+}
Review Comment:
`areTimezoneOptionsCached()` is used on `superset-frontend/src/features/alerts/AlertReportModal.tsx` and since the new object `TimezoneOptionsCache` should work as a service and it is instantiated in `superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx`, seems like prefer flow.
Related to `getTimezoneOptionsAsync()` seems like a more clearer wait to use, since it is called in various places, it also is easier to test.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2665326075
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +60,142 @@ const offsetsToName = {
'060': ['GMT Standard Time - London', 'British Summer Time'],
};
-export type TimezoneSelectorProps = {
- onTimezoneChange: (value: string) => void;
- timezone?: string | null;
- minWidth?: string;
-};
+function getOffsetKey(timezoneName: string): string {
+ return (
+ JANUARY_REF.tz(timezoneName).utcOffset().toString() +
+ JULY_REF.tz(timezoneName).utcOffset().toString()
+ );
+}
+
+const timezoneCache = new TimezoneOptionsCache(getOffsetKey, offsetsToName);
+
+// Export function to check if options are cached (for parent components)
+export function areTimezoneOptionsCached(): boolean {
+ return timezoneCache.isCached();
+}
+
+function getTimezoneOptionsAsync(): Promise<TimezoneOption[]> {
+ return timezoneCache.getOptionsAsync();
+}
Review Comment:
I'm not clear on what you mean. These two functions (areTimezoneOptionsCached() and getTimezoneOptionsAsync()) are just thin wrappers that call and return the result of the object methods. Using the object methods directly would achieve the same result. Could you clarify why you prefer exporting the functions over the object, and how it makes testing easier? I'm happy to adjust based on your reasoning.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3642019792
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20135260185) for [24a9771](https://github.com/apache/superset/commit/24a9771)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3718574029
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3723582862
β οΈ **DEPRECATED WORKFLOW** β οΈ
@geido This workflow is deprecated! Please use the new **Superset Showtime** system instead:
- Replace "testenv-up" label with "πͺ trigger-start"
- Better reliability and easier management
- See https://github.com/mistercrunch/superset-showtime for details
Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/20816310923). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2644219057
##########
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)');
Review Comment:
I went back to the original test, since I did a different approach to avoid changing the UX.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3751681445
> Sorry, rebase needed again :/
Oh sorry for checking this message late, rebase done! πͺ
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3635100680
CodeAnt AI is reviewing your PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2665619818
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +60,142 @@ const offsetsToName = {
'060': ['GMT Standard Time - London', 'British Summer Time'],
};
-export type TimezoneSelectorProps = {
- onTimezoneChange: (value: string) => void;
- timezone?: string | null;
- minWidth?: string;
-};
+function getOffsetKey(timezoneName: string): string {
+ return (
+ JANUARY_REF.tz(timezoneName).utcOffset().toString() +
+ JULY_REF.tz(timezoneName).utcOffset().toString()
+ );
+}
+
+const timezoneCache = new TimezoneOptionsCache(getOffsetKey, offsetsToName);
+
+// Export function to check if options are cached (for parent components)
+export function areTimezoneOptionsCached(): boolean {
+ return timezoneCache.isCached();
+}
+
+function getTimezoneOptionsAsync(): Promise<TimezoneOption[]> {
+ return timezoneCache.getOptionsAsync();
+}
Review Comment:
There is already a singleton instance:
`const timezoneCache = new TimezoneOptionsCache(getOffsetKey, offsetsToName);`
on
`superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx::70`
If I'm not mistaken, exporting the class would require consumers to create instances, which I think isn't needed.
Also there's a problem: `TimezoneOptionsCache` requires `getOffsetKey` and `offsetsToName` as constructor parameters, which are defined in **`index.tsx`**, and we cannot be used on **`AlertReportModal.tsx`**.
Maybe what we can do is:
- Move singleton creation to `TimezoneOptionsCache.tsx`.
- Move `getOffsetKey`, `offsetsToName`, and related constants to `TimezoneOptionsCache.tsx`.
- Create and export the `singleton` there.
- Import it in `index.tsx`.
I think it will solve keep the cache and its dependencies together, makes the singleton the single source of truth and simplifies the API by just importing the singleton.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2665623309
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +60,142 @@ const offsetsToName = {
'060': ['GMT Standard Time - London', 'British Summer Time'],
};
-export type TimezoneSelectorProps = {
- onTimezoneChange: (value: string) => void;
- timezone?: string | null;
- minWidth?: string;
-};
+function getOffsetKey(timezoneName: string): string {
+ return (
+ JANUARY_REF.tz(timezoneName).utcOffset().toString() +
+ JULY_REF.tz(timezoneName).utcOffset().toString()
+ );
+}
+
+const timezoneCache = new TimezoneOptionsCache(getOffsetKey, offsetsToName);
+
+// Export function to check if options are cached (for parent components)
+export function areTimezoneOptionsCached(): boolean {
+ return timezoneCache.isCached();
+}
+
+function getTimezoneOptionsAsync(): Promise<TimezoneOption[]> {
+ return timezoneCache.getOptionsAsync();
+}
Review Comment:
Yep, that's the way to do it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3723611139
@geido Ephemeral environment spinning up at http://35.163.235.96:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2625168757
##########
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:
This does not work as suggested, it will sort the array from -01:00 to -11:00
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3663083591
Solved nitpicks and possible future issues. CodeAnt gave good suggestions, so I took them into consideration.
Also solved @geido and @rusackas suggestions, thanks for that!
Rebased from master as well
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3660437146
CodeAnt AI is running Incremental review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3670353303
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20338912077) for [209facd](https://github.com/apache/superset/commit/209facd)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3670426681
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20338912077) for [209facd](https://github.com/apache/superset/commit/209facd)
β’ **Environment:** http://35.86.134.37:8080 (admin/admin)
β’ **Lifetime:** 48h auto-cleanup
β’ **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3687873171
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3688404637
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2623348829
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -17,19 +17,37 @@
* under the License.
*/
-import { useEffect, useMemo } from 'react';
+import { useCallback, useMemo, useState } from 'react';
import { t } from '@superset-ui/core';
import { Select } from '@superset-ui/core/components';
import { isDST, extendedDayjs } from '../../utils/dates';
+export type TimezoneSelectorProps = {
+ onTimezoneChange: (value: string) => void;
+ timezone?: string | null;
+ minWidth?: string;
+ placeholder?: string;
+};
+
+type TimezoneOption = {
+ label: string;
+ value: string;
+ offsets: string;
+ timezoneName: string;
+};
+
const DEFAULT_TIMEZONE = {
name: 'GMT Standard Time',
value: 'Africa/Abidjan', // timezones are deduped by the first alphabetical value
};
const MIN_SELECT_WIDTH = '400px';
+const DROPDOWN_MIN_HEIGHT = 265;
Review Comment:
Yeah, I know what you mean, gonna double check!
For more context, this is what is happening without the min_height hack:
https://github.com/user-attachments/assets/3368fab5-4de0-4c77-91f1-fdd62286e10c
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2653060833
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +62,213 @@ 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;
+
+// Export function to check if options are cached (for parent components)
+export function areTimezoneOptionsCached(): boolean {
+ return cachedTimezoneOptions !== 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 = currentDate.tz(zone).format('Z');
+ const label = `GMT ${offset} (${displayName})`;
+
+ if (!seenLabels.has(label)) {
+ seenLabels.add(label);
+ options.push({
+ label,
+ value: zone,
+ offsets: offsetKey,
+ timezoneName: zone,
+ });
+ }
+ }
+
+ cachedTimezoneOptions = options;
Review Comment:
Can we either refactor this into a class to couple the variable and function tighter or take this variable as an argument?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3714548162
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3712111077
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3751180872
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3750949498
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2625202909
##########
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();
Review Comment:
Suggestion taken into consideration and updated on latest commit
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2623217709
##########
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();
Review Comment:
Sure, done!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3635110197
## 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/36486/files#diff-b14ca673750ade06cbc648f5dcd0c700b97241b0265bb6a639b483e8783fa939R121-R145'><strong>Promise error handling</strong></a><br>`getTimezoneOptionsAsync()` stores a global `computePromise` but does not handle rejection. If the async computation throws/rejects, `computePromise` will remain set to a rejected promise and subsequent callers may never recover. Consider clearing or replacing `computePromise` on rejection.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-b14ca673750ade06cbc648f5dcd0c700b97241b0265bb6a639b483e8783fa939R182-R190'><strong>setState after unmount</strong></a><br>The async flow started in `handleOpenChange` calls `getTimezoneOptionsAsync().then(setTimezoneOptions).finally(() => setIsLoadingOptions(false))`. If the component unmounts before the promise settles, the resolved callbacks will call state setters on an unmounted component and can cause React warnings or memory leaks. Add a mounted check or cleanup to avoid calling setState after unmount.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-b14ca673750ade06cbc648f5dcd0c700b97241b0265bb6a639b483e8783fa939R69-R74'><strong>Offset key ambiguity</strong></a><br>`getOffsetKey` concats January and July offsets without a separator (e.g. `utcOffset().toString() + utcOffset().toString()`), producing keys like `60120` or `00`. This relies on matching keys in `offsetsToName`, but the concatenation may be ambiguous for certain offsets. Consider a clear delimiter to avoid accidental collisions.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-b14ca673750ade06cbc648f5dcd0c700b97241b0265bb6a639b483e8783fa939R173-R175'><strong>Unused prop</strong></a><br>The `minWidth` prop has a default value but is never applied to the rendered <Select>. This is misleading and means the default has no effect β callers relying on that default will not get the expected behavior.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-65e4188666ca6e3d65c21462b0f1713161d0d60ecc5bd0abb9af7eabf751c730R41-R59'><strong>Timer / microtask flakiness</strong></a><br>The test uses fake timers and calls `jest.runAllTimers()` to trigger async work that in implementation is scheduled via a combination of `queueMicrotask` and `setTimeout(0)`. Microtasks (queueMicrotask / Promise microtasks) are not always flushed by timer APIs and this can lead to intermittent flakes where the timezone options never materialize in time. Consider ensuring microtasks are flushed after advancing timers (e.g. awaiting a resolved promise or using React's act).<br>
- [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-7dc3633607d66f62b7c5a2c6703ba56acfe6643cb320a582edc24f9a54f132b1R56-R66'><strong>Flaky timers / microtasks</strong></a><br>The tests rely on calling `jest.runAllTimers()` after triggering the dropdown open to flush async work. The component under test uses a mix of `queueMicrotask` and `setTimeout(0)` (per PR description). Jest's fake timer APIs may not run microtasks the same way as real runtime which can lead to intermittent failures or tests that hang. Verify the tests reliably flush both microtasks and timers (use `act`, a dedicated flush helper, or adjust the scheduling in the component/tests).<br>
- [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-7dc3633607d66f62b7c5a2c6703ba56acfe6643cb320a582edc24f9a54f132b1R74-R122'><strong>Repeated test setup / duplication</strong></a><br>The new tests add repeated steps to trigger timezone data loading (click combobox, run timers, wait for loading to finish) across many tests. This increases maintenance burden and risk of inconsistent behavior between tests. Consider extracting a small helper (e.g. `triggerTimezoneLoad`) to centralize this flow and ensure consistent ordering of microtask/timer flushing.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-65e4188666ca6e3d65c21462b0f1713161d0d60ecc5bd0abb9af7eabf751c730R60-R63'><strong>Brittle loading text assertion</strong></a><br>The test waits for "Loading..." to disappear. The component's UX mentions "Loading timezones..." and labeling could change; asserting exact text may make the test brittle. Prefer a more robust selector (regex, role, aria-label, or a spinner class) so translations/wording changes don't break the test.<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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2610648120
##########
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();
Review Comment:
Nit: can we use `findByText` instead of `waitFor`?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2665442738
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +60,142 @@ const offsetsToName = {
'060': ['GMT Standard Time - London', 'British Summer Time'],
};
-export type TimezoneSelectorProps = {
- onTimezoneChange: (value: string) => void;
- timezone?: string | null;
- minWidth?: string;
-};
+function getOffsetKey(timezoneName: string): string {
+ return (
+ JANUARY_REF.tz(timezoneName).utcOffset().toString() +
+ JULY_REF.tz(timezoneName).utcOffset().toString()
+ );
+}
+
+const timezoneCache = new TimezoneOptionsCache(getOffsetKey, offsetsToName);
+
+// Export function to check if options are cached (for parent components)
+export function areTimezoneOptionsCached(): boolean {
+ return timezoneCache.isCached();
+}
+
+function getTimezoneOptionsAsync(): Promise<TimezoneOption[]> {
+ return timezoneCache.getOptionsAsync();
+}
Review Comment:
- Encapsulation is already broken - If you're exporting areTimezoneOptionsCached(), you're already coupling external code to the existence of a cache. The "encapsulation" benefit is minimal.
- Principle of least privilege is misapplied here as it doesn't relate to API design but security.
I think the best course of action here is to just create and export a singleton from the cache file and use it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2667689880
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/TimezoneOptionsCache.tsx:
##########
Review Comment:
Just realized this file could just be .ts right?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3718575099
Rebased
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2665059252
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +60,142 @@ const offsetsToName = {
'060': ['GMT Standard Time - London', 'British Summer Time'],
};
-export type TimezoneSelectorProps = {
- onTimezoneChange: (value: string) => void;
- timezone?: string | null;
- minWidth?: string;
-};
+function getOffsetKey(timezoneName: string): string {
+ return (
+ JANUARY_REF.tz(timezoneName).utcOffset().toString() +
+ JULY_REF.tz(timezoneName).utcOffset().toString()
+ );
+}
+
+const timezoneCache = new TimezoneOptionsCache(getOffsetKey, offsetsToName);
+
+// Export function to check if options are cached (for parent components)
+export function areTimezoneOptionsCached(): boolean {
+ return timezoneCache.isCached();
+}
+
+function getTimezoneOptionsAsync(): Promise<TimezoneOption[]> {
+ return timezoneCache.getOptionsAsync();
+}
Review Comment:
`areTimezoneOptionsCached()` is used on `superset-frontend/src/features/alerts/AlertReportModal.tsx` and since the new object `TimezoneOptionsCache` should work as a service and it is instantiated in `superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx`, seems like prefer flow.
Related to `getTimezoneOptionsAsync()` seems like a more clearer way to use, since it is called in various places, it also is easier to test.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3750951583
CodeAnt AI Incremental review completed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3688318048
Applied minor suggestions of bito.
Rebased.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3740037026
Sorry, rebase needed again :/
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "dirivasm (via GitHub)" <gi...@apache.org>.
dirivasm commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3757372660
**Test Summary**
I've tested the performance of the Schedule option under Alerts and Reports modal and the click is taking aprox 550ms to open which is decent time (And its an enhancement of previous behavior)
Also I tested that alerts and reports can be created with schedule! β
This is QA approved β
@LuisSanchez
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2644217514
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +65,183 @@ 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 = currentDate.tz(zone).format('Z');
+ const label = `GMT ${offset} (${displayName})`;
+
+ if (!seenLabels.has(label)) {
+ seenLabels.add(label);
+ options.push({
+ label,
+ value: zone,
+ offsets: offsetKey,
+ timezoneName: zone,
+ });
+ }
+ }
+
+ 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, reject) => {
+ const run = () => {
+ try {
+ const result = computeTimezoneOptions();
+ resolve(result);
+ } catch (err) {
+ reject(err);
+ } finally {
+ computePromise = null;
+ }
+ };
+ if (typeof queueMicrotask === 'function') {
+ queueMicrotask(run);
+ } else {
+ setTimeout(run, 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
+ const [timezoneOptions, setTimezoneOptions] = useState<
+ TimezoneOption[] | null
+ >(cachedTimezoneOptions);
+ const [isLoadingOptions, setIsLoadingOptions] = useState(false);
+ const hasSetDefaultRef = useRef(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) =>
+ currentDate.tz(a.timezoneName).utcOffset() -
+ currentDate.tz(b.timezoneName).utcOffset();
+ }, [timezoneOptions]);
+
+ const validTimezone = useMemo(() => {
+ if (!timezoneOptions) {
+ return timezone || extendedDayjs.tz.guess();
+ }
+ return findMatchingTimezone(timezone, timezoneOptions);
+ }, [timezone, timezoneOptions]);
+
+ // Preload timezone options on mount and set default value
useEffect(() => {
- if (validTimezone && timezone !== validTimezone) {
- onTimezoneChange(validTimezone);
+ if (!timezoneOptions && !isLoadingOptions) {
+ setIsLoadingOptions(true);
+ getTimezoneOptionsAsync()
+ .then(options => {
+ setTimezoneOptions(options);
+ // Set default value if no timezone is provided and we haven't set it yet
+ if (!timezone && !hasSetDefaultRef.current) {
+ const defaultTz = findMatchingTimezone(null, options);
+ onTimezoneChange(defaultTz);
+ hasSetDefaultRef.current = true;
+ }
+ })
+ .finally(() => setIsLoadingOptions(false));
+ } else if (timezoneOptions && !timezone && !hasSetDefaultRef.current) {
+ // If options are already available (cached), set default immediately
+ const defaultTz = findMatchingTimezone(null, timezoneOptions);
+ onTimezoneChange(defaultTz);
+ hasSetDefaultRef.current = true;
}
- }, [validTimezone, onTimezoneChange, timezone]);
+ }, [timezoneOptions, isLoadingOptions, timezone, onTimezoneChange]);
return (
<Select
ariaLabel={t('Timezone selector')}
onChange={tz => onTimezoneChange(tz as string)}
- value={validTimezone}
- options={TIMEZONE_OPTIONS}
- sortComparator={TIMEZONE_OPTIONS_SORT_COMPARATOR}
- {...rest}
+ onOpenChange={handleOpenChange}
+ value={timezoneOptions ? validTimezone : undefined}
+ options={timezoneOptions || []}
+ sortComparator={sortComparator}
+ loading={isLoadingOptions}
+ placeholder={isLoadingOptions ? t('Loading timezones...') : placeholder}
+ {...({ placement: 'topLeft', ...rest } as any)}
Review Comment:
Done!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2666709783
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -45,118 +60,142 @@ const offsetsToName = {
'060': ['GMT Standard Time - London', 'British Summer Time'],
};
-export type TimezoneSelectorProps = {
- onTimezoneChange: (value: string) => void;
- timezone?: string | null;
- minWidth?: string;
-};
+function getOffsetKey(timezoneName: string): string {
+ return (
+ JANUARY_REF.tz(timezoneName).utcOffset().toString() +
+ JULY_REF.tz(timezoneName).utcOffset().toString()
+ );
+}
+
+const timezoneCache = new TimezoneOptionsCache(getOffsetKey, offsetsToName);
+
+// Export function to check if options are cached (for parent components)
+export function areTimezoneOptionsCached(): boolean {
+ return timezoneCache.isCached();
+}
+
+function getTimezoneOptionsAsync(): Promise<TimezoneOption[]> {
+ return timezoneCache.getOptionsAsync();
+}
Review Comment:
Refactor is done!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3712287589
Rebased
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2623222786
##########
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'),
);
Review Comment:
Updated code in favor of `findByText`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2625182044
##########
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:
This is needed for the Selector component, does not apply.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36486:
URL: https://github.com/apache/superset/pull/36486#discussion_r2625203387
##########
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);
Review Comment:
This is a good, catch, updated in latest commit
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3663058710
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3713200163
### <span aria-hidden="true">π·</span> Deploy Preview for *superset-docs-preview* processing.
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 53abd8f574f92281fff732fce457bf72ec8669f9 |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/695ca1a8e881280007ac95ef |
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(TimezoneSelector): Enhance timezone selection logic and improve performance [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36486:
URL: https://github.com/apache/superset/pull/36486#issuecomment-3756302805
@sadpandajoe Ephemeral environment spinning up at http://34.213.180.209:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org