[WEB-2884] chore: Update timezone list, add new endpoint, and update timezone dropdowns #6231
Conversation
WalkthroughThis pull request introduces a comprehensive timezone management system across the Plane application. The changes span multiple components, including backend API endpoints, frontend services, and UI components. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant TimezoneService
participant TimezoneEndpoint
Client->>TimezoneService: Request timezones
TimezoneService->>TimezoneEndpoint: Fetch timezone data
TimezoneEndpoint-->>TimezoneService: Return timezone list
TimezoneService-->>Client: Provide timezone options
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
web/core/hooks/use-timezone.tsx (1)
59-66: Double-check overlap between “UTC” and “Universal.”
Exporting both “UTC” and “Universal” might confuse users, as they both refer to Coordinated Universal Time. If you do intend to differentiate them, consider clarifying the distinction in user-facing labels.web/core/services/timezone.service.ts (1)
12-18: Consider enhancing error handling and simplifying the promise chain.The current implementation can be improved for better error handling and code simplicity:
async fetch(): Promise<TTimezones> { - return this.get(`/api/timezones/`) - .then((response) => response?.data) - .catch((error) => { - throw error?.response?.data; - }); + try { + const response = await this.get('/api/timezones/'); + return response?.data; + } catch (error) { + if (error?.response?.data) { + throw error.response.data; + } + throw new Error('Failed to fetch timezones'); + } }This refactor:
- Uses async/await for better readability
- Provides a fallback error message
- Maintains type safety with the TTimezones return type
web/core/components/global/timezone-select.tsx (1)
21-53: Consider performance optimization and accessibility improvements.While the implementation is functionally correct, consider these enhancements:
- Memoize the component to prevent unnecessary re-renders
- Add aria-label for better accessibility
-export const TimezoneSelect: FC<TTimezoneSelect> = observer((props) => { +export const TimezoneSelect: FC<TTimezoneSelect> = observer(memo((props) => { // ... return ( <div> <CustomSearchSelect + aria-label="Timezone selector" value={value} label={selectedValue ? selectedValue(value) : label} // ... /> </div> ); -}); +}));apiserver/plane/app/views/timezone/base.py (1)
204-246: Optimize timezone processing and enhance error handling.The timezone processing logic could be improved in several ways:
- Consider caching the processed timezone list instead of raw mapping
- Add more specific error handling for edge cases
- Make cache duration configurable
+from django.conf import settings + +TIMEZONE_CACHE_DURATION = getattr(settings, 'TIMEZONE_CACHE_DURATION', 60 * 60 * 24) + class TimezoneEndpoint(APIView): permission_classes = [AllowAny] throttle_classes = [AuthenticationThrottle] - @method_decorator(cache_page(60 * 60 * 24)) + @method_decorator(cache_page(TIMEZONE_CACHE_DURATION)) def get(self, request): try: timezone_list = [] now = datetime.now() # ... rest of the processing + except ValueError as e: + return Response( + {"error": f"Invalid timezone data: {str(e)}"}, + status=status.HTTP_500_INTERNAL_SERVER_ERROR + ) except pytz.exceptions.UnknownTimeZoneError: - continue + return Response( + {"error": "Invalid timezone identifier"}, + status=status.HTTP_400_BAD_REQUEST + )web/core/components/project/form.tsx (2)
382-391: Enhance timezone selection UX with helper text.The timezone selection implementation is good, but could benefit from additional user guidance.
<> <TimezoneSelect value={value} onChange={(value: string) => { onChange(value); }} error={Boolean(errors.timezone)} buttonClassName="border-none" + label="Select project timezone (affects due dates and notifications)" /> </>
393-393: Make timezone error message more descriptive.Consider providing a more informative error message to guide users.
-{errors.timezone && <span className="text-xs text-red-500">{errors.timezone.message}</span>} +{errors.timezone && ( + <span className="text-xs text-red-500"> + {errors.timezone.message || "Please select a timezone to ensure accurate scheduling and notifications"} + </span> +)}web/app/profile/page.tsx (1)
Line range hint
39-39: Consider improving default timezone handlingInstead of hardcoding "Asia/Kolkata" as the default timezone, consider:
- Using the browser's timezone:
Intl.DateTimeFormat().resolvedOptions().timeZone- Or defaulting to "UTC" as a neutral choice
- user_timezone: "Asia/Kolkata", + user_timezone: Intl.DateTimeFormat().resolvedOptions().timeZone,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apiserver/plane/app/urls/__init__.py(2 hunks)apiserver/plane/app/urls/timezone.py(1 hunks)apiserver/plane/app/views/__init__.py(1 hunks)apiserver/plane/app/views/timezone/base.py(1 hunks)packages/types/src/index.d.ts(1 hunks)packages/types/src/timezone.d.ts(1 hunks)web/app/profile/page.tsx(2 hunks)web/core/components/global/index.ts(1 hunks)web/core/components/global/timezone-select.tsx(1 hunks)web/core/components/project/form.tsx(3 hunks)web/core/hooks/use-timezone.tsx(1 hunks)web/core/services/timezone.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apiserver/plane/app/views/init.py
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/hooks/use-timezone.tsx
[error] 52-52: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
🔇 Additional comments (9)
web/core/hooks/use-timezone.tsx (1)
7-27: Ensure grouping function merges data consistently.
This grouping logic appears correct for handling duplicates by merging labels for the same timezone value. However, please confirm that merging labels into a comma-separated list is the intended behavior. In some cases, separate entries might be more appropriate if they differ in subtle ways.
If you need confirmation of usage patterns throughout the codebase, run the following script to locate calls to the grouped data and ensure that all references expect merged labels:
✅ Verification successful
Let me gather more context about the timezone usage and data structure.
Let me search for the timezone component usage and implementation of getTimeZoneLabel.
Based on the gathered context, I can now generate the final response as we have sufficient information about the timezone implementation and its usage.
Timezone grouping implementation is correct and consistent.
The grouping logic that merges labels for the same timezone value is the intended behavior. The implementation is used in a timezone selector component where the grouped labels are properly rendered through getTimeZoneLabel. This is a common pattern for timezone selectors where multiple locations share the same UTC offset, making it more user-friendly by showing all applicable locations in a single option.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to 'groupTimezones' usage
rg -A 3 'groupTimezones'
Length of output: 714
Script:
#!/bin/bash
# Check the type definition and structure of TTimezoneObject
rg -A 5 "type TTimezoneObject"
# Look for getTimeZoneLabel implementation
ast-grep --pattern 'function getTimeZoneLabel($_) { $$$ }'
# Search for any timezone-related tests
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts | xargs rg "timezone"
Length of output: 91816
Script:
#!/bin/bash
# Look for timezone component usage
rg -A 3 "useTimezone"
# Search for getTimeZoneLabel implementation
rg -A 3 "getTimeZoneLabel"
# Look for any timezone selector component
fd -e tsx -e ts | xargs rg "timezone.*select"
Length of output: 1662
web/core/components/global/index.ts (1)
3-3: Exporting the new “timezone-select” is consistent with existing patterns.
This export neatly integrates the new TimezoneSelect component into the global public API. Looks good!
packages/types/src/timezone.d.ts (1)
1-8: Well-defined timezone types.
These definitions facilitate consistent handling of timezones across the application. No issues found.
apiserver/plane/app/urls/timezone.py (1)
5-8: New endpoint configuration is straightforward.
The path mapping to “timezones/” is clearly named “timezone-list.” This addition aligns well with Django’s urlpatterns.
packages/types/src/index.d.ts (1)
36-36: LGTM! Export follows established pattern.
The timezone type export follows the existing pattern and is properly placed.
apiserver/plane/app/urls/__init__.py (1)
20-20: Verify the timezone URL configuration.
The URL configuration follows the established pattern. Let's verify the corresponding timezone URL patterns are properly defined.
Also applies to: 42-42
✅ Verification successful
Let me verify the view implementation to ensure it's properly defined.
Let me try to find the view implementation using ripgrep instead.
Timezone URL configuration is properly implemented
The verification confirms that:
- The timezone URL patterns are correctly defined in the URLs file, exposing a single endpoint at
/timezones/ - The corresponding view
TimezoneEndpointis properly implemented as an API view with appropriate permissions and caching - The endpoint provides timezone mapping data with a cached response
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if timezone URL patterns are properly defined
fd timezone.py -t f | xargs cat
Length of output: 224
Script:
#!/bin/bash
# Check the TimezoneEndpoint view implementation
ast-grep --pattern 'class TimezoneEndpoint'
Length of output: 45
Script:
#!/bin/bash
# Search for TimezoneEndpoint view implementation
rg "class TimezoneEndpoint" -A 10
Length of output: 865
web/core/components/global/timezone-select.tsx (1)
10-19: LGTM! Well-structured props interface.
The props interface is comprehensive and properly typed with optional properties.
web/app/profile/page.tsx (2)
23-23: LGTM: Clean import of TimezoneSelect component
The import follows the project's organization pattern and is correctly placed with other component imports.
366-371: Verify timezone persistence and validation
The TimezoneSelect implementation looks good with proper error handling and validation. However, let's verify the timezone persistence flow.
✅ Verification successful
Let me generate additional scripts to verify the timezone persistence flow.
Let me try one more search to find the timezone handling in the user profile components and API.
Based on the search results, I can now generate a final response since we have found evidence of timezone handling in the codebase:
Timezone persistence implementation is properly handled
The timezone persistence flow is correctly implemented:
- The timezone value is included in the user profile update payload (
user_timezonefield) - The backend has proper migration support for timezone data (seen in
apiserver/plane/db/migrations/0042_alter_analyticview_created_by_and_more.py) - The form validation and error handling is properly wired up through the
TimezoneSelectcomponent - The update is processed through the
updateCurrentUserfunction which handles the persistence
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the timezone persistence flow and related components
# Check for timezone validation in other components
rg -A 5 "user_timezone.*required"
# Check for timezone persistence handling
rg -A 10 "updateCurrentUser.*user_timezone"
# Look for any timezone-related API calls
ast-grep --pattern 'fetch($_, $_).then($_)'
Length of output: 65719
Script:
#!/bin/bash
# Look for timezone-related form validation and persistence
# Check for timezone validation in form schemas
rg -A 5 "user_timezone.*schema"
# Look for timezone-related API endpoints
rg -A 5 "user_timezone.*api"
# Check for timezone update mutations/actions
rg -A 10 "updateUser.*timezone"
# Look for timezone select component implementation
ast-grep --pattern 'const TimezoneSelect = $_'
Length of output: 833
Script:
#!/bin/bash
# Look for timezone handling in user profile and API
# Check for timezone field in user profile components
rg -A 10 "timezone.*profile"
# Look for timezone in API calls or mutations
rg -A 10 "timezone.*mutation"
# Find timezone validation or form handling
rg -A 10 "timezone.*form"
# Check for timezone state management
rg -A 10 "timezone.*state"
Length of output: 67269
| ); | ||
| }; | ||
| const options = [ | ||
| ...groupTimezones(timezones?.timezones || [])?.map((timezone) => ({ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary optional chaining to avoid potential short-circuit issues.
Although “groupTimezones” always returns an array, using optional chaining with “?.map” can lead to TypeErrors if the returned value is unexpectedly not an array in the future.
Proposed fix:
- ...groupTimezones(timezones?.timezones || [])?.map((timezone) => ({
+ ...groupTimezones(timezones?.timezones || []).map((timezone) => ({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ...groupTimezones(timezones?.timezones || [])?.map((timezone) => ({ | |
| ...groupTimezones(timezones?.timezones || []).map((timezone) => ({ |
🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
| ("Tunis", "Africa/Tunis"), | ||
| ], | ||
| "+0200": [ | ||
| ("Eastern European Time (Cairo, Helsinki, Kyiv)", "Europe/Kiev"), | ||
| ("Athens", "Europe/Athens"), | ||
| ("Jerusalem", "Asia/Jerusalem"), | ||
| ("Johannesburg", "Africa/Johannesburg"), | ||
| ("Harare, Pretoria", "Africa/Harare"), | ||
| ], | ||
| "+0300": [ | ||
| ("Moscow Time", "Europe/Moscow"), | ||
| ("Baghdad", "Asia/Baghdad"), | ||
| ("Nairobi", "Africa/Nairobi"), | ||
| ("Kuwait, Riyadh", "Asia/Riyadh"), | ||
| ], | ||
| "+0330": [("Tehran", "Asia/Tehran")], | ||
| "+0400": [ | ||
| ("Abu Dhabi", "Asia/Dubai"), | ||
| ("Baku", "Asia/Baku"), | ||
| ("Yerevan", "Asia/Yerevan"), | ||
| ("Astrakhan", "Europe/Astrakhan"), | ||
| ("Tbilisi", "Asia/Tbilisi"), | ||
| ("Mauritius", "Indian/Mauritius"), | ||
| ], | ||
| "+0500": [ | ||
| ("Islamabad", "Asia/Karachi"), | ||
| ("Karachi", "Asia/Karachi"), | ||
| ("Tashkent", "Asia/Tashkent"), | ||
| ("Yekaterinburg", "Asia/Yekaterinburg"), | ||
| ("Maldives", "Indian/Maldives"), | ||
| ("Chagos", "Indian/Chagos"), | ||
| ], | ||
| "+0530": [ | ||
| ("Chennai", "Asia/Kolkata"), | ||
| ("Kolkata", "Asia/Kolkata"), | ||
| ("Mumbai", "Asia/Kolkata"), | ||
| ("New Delhi", "Asia/Kolkata"), | ||
| ("Sri Jayawardenepura", "Asia/Colombo"), | ||
| ], | ||
| "+0545": [("Kathmandu", "Asia/Kathmandu")], | ||
| "+0600": [ | ||
| ("Dhaka", "Asia/Dhaka"), | ||
| ("Almaty", "Asia/Almaty"), | ||
| ("Bishkek", "Asia/Bishkek"), | ||
| ("Thimphu", "Asia/Thimphu"), | ||
| ], | ||
| "+0630": [ | ||
| ("Yangon (Rangoon)", "Asia/Yangon"), | ||
| ("Cocos Islands", "Indian/Cocos"), | ||
| ], | ||
| "+0700": [ | ||
| ("Bangkok", "Asia/Bangkok"), | ||
| ("Hanoi", "Asia/Ho_Chi_Minh"), | ||
| ("Jakarta", "Asia/Jakarta"), | ||
| ("Novosibirsk", "Asia/Novosibirsk"), | ||
| ("Krasnoyarsk", "Asia/Krasnoyarsk"), | ||
| ], | ||
| "+0800": [ | ||
| ("Beijing", "Asia/Shanghai"), | ||
| ("Singapore", "Asia/Singapore"), | ||
| ("Perth", "Australia/Perth"), | ||
| ("Hong Kong", "Asia/Hong_Kong"), | ||
| ("Ulaanbaatar", "Asia/Ulaanbaatar"), | ||
| ("Palau", "Pacific/Palau"), | ||
| ], | ||
| "+0845": [("Eucla", "Australia/Eucla")], | ||
| "+0900": [ | ||
| ("Tokyo", "Asia/Tokyo"), | ||
| ("Seoul", "Asia/Seoul"), | ||
| ("Yakutsk", "Asia/Yakutsk"), | ||
| ], | ||
| "+0930": [ | ||
| ("Adelaide", "Australia/Adelaide"), | ||
| ("Darwin", "Australia/Darwin"), | ||
| ], | ||
| "+1000": [ | ||
| ("Sydney", "Australia/Sydney"), | ||
| ("Brisbane", "Australia/Brisbane"), | ||
| ("Guam", "Pacific/Guam"), | ||
| ("Vladivostok", "Asia/Vladivostok"), | ||
| ("Tahiti", "Pacific/Tahiti"), | ||
| ], | ||
| "+1030": [("Lord Howe Island", "Australia/Lord_Howe")], | ||
| "+1100": [ | ||
| ("Solomon Islands", "Pacific/Guadalcanal"), | ||
| ("Magadan", "Asia/Magadan"), | ||
| ("Norfolk Island", "Pacific/Norfolk"), | ||
| ("Bougainville Island", "Pacific/Bougainville"), | ||
| ("Chokurdakh", "Asia/Srednekolymsk"), | ||
| ], | ||
| "+1200": [ | ||
| ("Auckland", "Pacific/Auckland"), | ||
| ("Wellington", "Pacific/Auckland"), | ||
| ("Fiji Islands", "Pacific/Fiji"), | ||
| ("Anadyr", "Asia/Anadyr"), | ||
| ], | ||
| "+1245": [("Chatham Islands", "Pacific/Chatham")], | ||
| "+1300": [("Nuku'alofa", "Pacific/Tongatapu"), ("Samoa", "Pacific/Apia")], | ||
| "+1400": [("Kiritimati Island", "Pacific/Kiritimati")], | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving timezone mapping to a configuration file.
The large timezone mapping dictionary should be moved to a separate configuration file for better maintainability and readability.
Create a new file timezone_config.py and move the mapping there:
# timezone_config.py
TIMEZONE_MAPPING = {
"-1100": [
("Midway Island", "Pacific/Midway"),
# ... rest of the mapping
],
}
Description
Type of Change
Test Scenarios
References
[WEB-2884]
Summary by CodeRabbit
New Features
TimezoneSelectcomponent for selecting timezones in profile and project settings.useTimezone, for managing timezone data fetching and state.Bug Fixes
Documentation
Chores