[BUGFIX] (dashboards): apply dashboard.spec.timezone to TimeZoneProvider - #298
Conversation
230a098 to
24635a3
Compare
| * Hierarchy (highest wins): | ||
| * 1. dashboard.spec.timezone (when non-empty) | ||
| * 2. user preference timezone (when non-empty) | ||
| * 3. browser local (`'local'`) |
There was a problem hiding this comment.
This hierarchy means a dashboard defining a timezone will "lock it" disallowing the user to use a custom one?
There was a problem hiding this comment.
Yes for the default view, by design (see docs/concepts/timezone.md):
spec.timezone > user preference > local, so ops dashboards can pin UTC for everyone.
Users can still override at runtime via the time-range timezone control / ?tz= (useTimeZoneParams), which takes precedence over the resolved initial value.
| setRefreshInterval={setRefreshInterval} | ||
| > | ||
| <TimeZoneProvider timeZone={timeZone}>{children}</TimeZoneProvider> | ||
| {children} |
There was a problem hiding this comment.
This is a breaking change for consumers of this provider as it no longer uses the timezone provider, this creates the duplication under other components. Instead of removing this, we can add another provider in the case an override (the dashboard spec timezone) is needed. As the hook will take the value from the closest provider
There was a problem hiding this comment.
Good point, restored TimeZoneProvider inside TimeRangeProviderWithQueryParams so existing consumers keep working.
ViewDashboard nests an additional provider with spec.timezone || user pref, so useTimeZone() picks the closest override without a breaking API change.
24635a3 to
1f5610d
Compare
cfb033a to
af07b2b
Compare
|
|
| const fromDashboard = dashboardTimezone?.trim(); | ||
| if (fromDashboard) { | ||
| return fromDashboard; | ||
| } | ||
| const fromUser = userPreferenceTimezone?.trim(); | ||
| if (fromUser) { | ||
| return fromUser; | ||
| } | ||
| return 'local'; |
There was a problem hiding this comment.
Suggestion
Could it be simplified?
return (dashboardTimezone?.trim() || userPreferenceTimezone?.trim() || "local");There was a problem hiding this comment.
I am not sure why the second TimeZoneProvider has been added. In the hierarchy, the TimeRangeProviderWithQueryParams already provides the tz provider.
Apart from that, in my opinion, an ideal implementation would take the tz from
- try to fetch tz from the query params, does not exist?
- try to fetch it from spec.tz , does not exist?
- try to fetch it from the user preferences, does not exits
- set it to local
- Update tz to the last read value, establish a loop of retrieve/update. (at this point the tz could not be technically undefined anymore. It is showing either spec, user prefrences, or local)
Whenever the time zone is edited by the user the tz query param should be updated. Subsequently, the updated tz should be retrieved and used accordingly. Let's take a look at the fllowing code:
/* initialTimeZone is set by spec || user preference || local */
export function useTimeZoneParams(initialTimeZone?: string): { timeZone: string; setTimeZone: (tz: string) => void } {
const [query, setQuery] = useQueryParams(timeZoneQueryConfig, { updateType: 'replaceIn' });
const { tz } = query;
/* >>>>> interesting line <<<<< */
const timeZone = (tz as string | undefined) ?? initialTimeZone ?? 'local';
const setTimeZone = useCallback(
(newTz: string) => {
setQuery({ tz: newTz });
},
[setQuery],
);
return { timeZone, setTimeZone };
}af07b2b to
3d08b55
Compare
Pass resolveDashboardTimeZone(spec, userPref) as initialTimeZone into TimeRangeProviderWithQueryParams (single TimeZoneProvider). Runtime order via useTimeZoneParams: 1. URL ?tz= 2. dashboard.spec.timezone || user preference || local No second nested TimeZoneProvider. Sticky toolbar seeds from context. Signed-off-by: colivi <charles.olivi@gmail.com>
3d08b55 to
a578a87
Compare

Description
"dashboard.spec.timezone" was applied only to the toolbar time picker ("DashboardApp" → "toolBarTimezone"), while charts used "TimeRangeProviderWithQueryParams" → "useTimeZoneParams('local')".
Result: the picker could show GMT+0 / UTC while panels still formatted in browser local.
Approach
Keep time range and timezone as separate concerns (credits to Celian):
TimeRangeProviderWithQueryParams
└── TimeZoneProvider ← ViewDashboard / ViewExplore
└── charts + toolbars (useTimeZone)
Fix
Hierarchy (charts + toolbar)
dashboard.spec.timezone > user preference > 'local' (URL ?tz= still overrides via useTimeZoneParams)
Checklist