optimize timezone dropdown with virtualization and memoization #9896
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR implements performance optimizations for the timezone dropdown to prevent page freezing issues by adding virtualization and memoization.
- Added new
VirtualizedSelectcomponent in/packages/twenty-front/src/modules/ui/input/components/VirtualizedSelect.tsxusing@tanstack/react-virtualfor efficient rendering - Implemented memoized timezone options with pre-calculated offsets in
/packages/twenty-front/src/modules/settings/accounts/constants/AvailableTimezoneOptions.ts - Replaced standard
SelectwithVirtualizedSelectin timezone selection components with 40px item height and 300px max height - Added scroll position reset and cleanup on dropdown open/close in
VirtualizedSelect - Optimized timezone filtering by removing GMT/UTC/UCT duplicates and caching filtered results
5 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| // <Select | ||
| // dropdownId="settings-accounts-calendar-time-zone" | ||
| // dropdownWidth={416} | ||
| // label="Time zone" | ||
| // fullWidth | ||
| // value={findAvailableTimeZoneOption(value)?.value} | ||
| // options={AVAILABLE_TIMEZONE_OPTIONS} | ||
| // onChange={onChange} | ||
| // withSearchInput | ||
| // /> |
There was a problem hiding this comment.
style: Remove commented out code since it's in version control
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| const difference = | ||
| getTimezoneOffset(optionA.value) - getTimezoneOffset(optionB.value); | ||
| export const createTimeZoneOptions = memoize(() => { |
There was a problem hiding this comment.
logic: memoizing this function won't help with performance since it's only called once at module initialization - the AVAILABLE_TIMEZONE_OPTIONS constant already serves as a cache
| const timeZoneOptionsMap = IANA_TIME_ZONES.reduce<Record<string, TimezoneSelectOption>>( | ||
| (result, ianaTimeZone) => { | ||
| const timeZoneLabel = formatTimeZoneLabel(ianaTimeZone); | ||
| const timeZoneName = timeZoneLabel.slice(11); |
There was a problem hiding this comment.
style: slicing at index 11 is fragile - consider using a regex or split() to extract the timezone name more reliably
| const rowVirtualizer = useVirtualizer({ | ||
| count: filteredOptions.length, | ||
| getScrollElement: () => parentRef.current, | ||
| estimateSize: () => itemHeight, | ||
| overscan: 5, | ||
| }); |
There was a problem hiding this comment.
style: overscan value of 5 may be too low for smooth scrolling with variable height items. Consider increasing to 10-20 for better performance
| @@ -13,27 +55,41 @@ export const DateTimeSettingsTimeZoneSelect = ({ | |||
| value = detectTimeZone(), | |||
| onChange, | |||
| }: DateTimeSettingsTimeZoneSelectProps) => { | |||
| const [searchQuery, setSearchQuery] = useState(''); | |||
There was a problem hiding this comment.
logic: searchQuery state is defined but setSearchQuery is never used - this may prevent the search functionality from working
| const allOptions = createTimeZoneOptions(); | ||
| if (!searchQuery) return allOptions; |
There was a problem hiding this comment.
style: createTimeZoneOptions() is called on every search - should be memoized separately to avoid regenerating the full list repeatedly
|
Thanks you @mdrazak2001 :) The virtualizer approach is interesting but I'm surprised this is needed for a dropdown. I'll take a look tomorrow |
|
@charlesBochet, your PR fixes this issue, isn't it ? |
Fixes #9663
28.01.2025_23.27.23_REC.mp4