You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2025/01/21 12:44:52 UTC
[PR] fix(timezoneselector): Correct the order to match names first [superset]
msyavuz opened a new pull request, #31941:
URL: https://github.com/apache/superset/pull/31941
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
While selecting a timezone try to match both names and offsets and default to just offsets if the first match isn't found #31839
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Run the testing suite
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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] fix(timezoneselector): Correct the order to match names first [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido merged PR #31941:
URL: https://github.com/apache/superset/pull/31941
--
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] fix(timezoneselector): Correct the order to match names first [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
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: 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