korbit-ai[bot] commented on code in PR #31941:
URL: https://github.com/apache/superset/pull/31941#discussion_r1923668031
##########
superset-frontend/src/components/TimezoneSelector/index.tsx:
##########
@@ -112,10 +112,21 @@ export default function TimezoneSelector({
// pre-sort timezone options by time offset
TIMEZONE_OPTIONS.sort(TIMEZONE_OPTIONS_SORT_COMPARATOR);
- const matchTimezoneToOptions = (timezone: string) =>
- TIMEZONE_OPTIONS.find(
- option => option.offsets === getOffsetKey(timezone),
- )?.value || DEFAULT_TIMEZONE.value;
+ const matchTimezoneToOptions = (timezone: string) => {
+ const matchedOption = TIMEZONE_OPTIONS.find(
+ option =>
+ option.offsets === getOffsetKey(timezone) &&
+ option.timezoneName === timezone,
+ );
+
+ return (
+ matchedOption?.value ||
+ TIMEZONE_OPTIONS.find(
+ option => option.offsets === getOffsetKey(timezone),
+ )?.value ||
+ DEFAULT_TIMEZONE.value
+ );
+ };
Review Comment:
### Inefficient Multiple Array Traversals <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple array traversals with find() operations are performed sequentially,
computing getOffsetKey() twice for the same input.
###### Why this matters
Redundant array traversals and repeated offset calculations impact
performance, especially with large timezone arrays.
###### Suggested change ∙ *Feature Preview*
Cache the offset calculation and use a single array traversal:
```typescript
const matchTimezoneToOptions = (timezone: string) => {
const offsetKey = getOffsetKey(timezone);
const option = TIMEZONE_OPTIONS.find(
opt => (opt.timezoneName === timezone && opt.offsets === offsetKey) ||
opt.offsets === offsetKey
);
return option?.value || DEFAULT_TIMEZONE.value;
}
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/96268c3c-e475-417b-8289-1e07bff9c93a?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:280af07f-abc7-4143-8e7b-af8320174a1a -->
##########
superset-frontend/src/components/TimezoneSelector/index.tsx:
##########
@@ -112,10 +112,21 @@
// pre-sort timezone options by time offset
TIMEZONE_OPTIONS.sort(TIMEZONE_OPTIONS_SORT_COMPARATOR);
- const matchTimezoneToOptions = (timezone: string) =>
- TIMEZONE_OPTIONS.find(
- option => option.offsets === getOffsetKey(timezone),
- )?.value || DEFAULT_TIMEZONE.value;
+ const matchTimezoneToOptions = (timezone: string) => {
+ const matchedOption = TIMEZONE_OPTIONS.find(
+ option =>
+ option.offsets === getOffsetKey(timezone) &&
+ option.timezoneName === timezone,
+ );
+
+ return (
+ matchedOption?.value ||
+ TIMEZONE_OPTIONS.find(
+ option => option.offsets === getOffsetKey(timezone),
+ )?.value ||
+ DEFAULT_TIMEZONE.value
+ );
Review Comment:
### Imprecise Timezone Fallback Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The fallback logic may select an incorrect timezone when multiple timezones
share the same offset but have different names.
###### Why this matters
When the exact timezone match fails, falling back to offset-only matching
could select any timezone with the same offset, potentially choosing the wrong
one due to the array's sort order rather than the best semantic match.
###### Suggested change ∙ *Feature Preview*
Implement a more sophisticated fallback mechanism that considers partial
name matches or geographical proximity before falling back to offset-only
matching:
```typescript
const matchTimezoneToOptions = (timezone: string) => {
// Exact match (name and offset)
const exactMatch = TIMEZONE_OPTIONS.find(
option =>
option.offsets === getOffsetKey(timezone) &&
option.timezoneName === timezone,
);
if (exactMatch) return exactMatch.value;
// Partial name match with same offset
const partialMatch = TIMEZONE_OPTIONS.find(
option =>
option.offsets === getOffsetKey(timezone) &&
(option.timezoneName.includes(timezone) ||
timezone.includes(option.timezoneName))
);
if (partialMatch) return partialMatch.value;
// Offset-only match as last resort
const offsetMatch = TIMEZONE_OPTIONS.find(
option => option.offsets === getOffsetKey(timezone)
);
return offsetMatch?.value || DEFAULT_TIMEZONE.value;
};
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/43bc4298-de59-4b11-a0d5-820ebe672cc6?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:0877d194-521a-495a-9349-5041a694d903 -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
