Pmm-14550: preferred timezone when displaying dates#878
Pmm-14550: preferred timezone when displaying dates#878fabio-silva wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new application event intended to notify the UI when a user’s preferred timezone changes, so other components can react to preference updates.
Changes:
- Introduces a new
TimeZoneUpdatedEventon the Grafana runtime app event bus. - Publishes the new event from the Shared Preferences “Save” flow when a timezone value is present.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| public/app/core/components/SharedPreferences/SharedPreferences.tsx | Publishes a new timezone-updated app event after saving preferences. |
| packages/grafana-runtime/src/services/appEvents.ts | Adds the TimeZoneUpdatedEvent event definition to the runtime event bus API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| window.location.reload(); | ||
|
|
||
| // @PERCONA: Publish TimeZoneUpdatedEvent to allow other components to listen to timezone changes | ||
| if (timezone) { | ||
| getAppEvents().publish(new TimeZoneUpdatedEvent(timezone)); | ||
| } |
There was a problem hiding this comment.
TimeZoneUpdatedEvent is published after window.location.reload(), which will typically stop JS execution and drop any in-memory subscribers. Also this path calls getAppEvents().publish(...) without checking the event bus exists (elsewhere in this component you guard before subscribing). Publish the event (and optionally verify the timezone actually changed) before triggering the reload, and guard against getAppEvents() being undefined/without publish to avoid runtime/test failures.
| window.location.reload(); | |
| // @PERCONA: Publish TimeZoneUpdatedEvent to allow other components to listen to timezone changes | |
| if (timezone) { | |
| getAppEvents().publish(new TimeZoneUpdatedEvent(timezone)); | |
| } | |
| // @PERCONA: Publish TimeZoneUpdatedEvent to allow other components to listen to timezone changes | |
| if (timezone) { | |
| const eventBus = getAppEvents(); | |
| if (eventBus && typeof eventBus.publish === 'function') { | |
| eventBus.publish(new TimeZoneUpdatedEvent(timezone)); | |
| } | |
| } | |
| window.location.reload(); |
| // @PERCONA: Publish TimeZoneUpdatedEvent to allow other components to listen to timezone changes | ||
| if (timezone) { | ||
| getAppEvents().publish(new TimeZoneUpdatedEvent(timezone)); | ||
| } |
There was a problem hiding this comment.
This publishes TimeZoneUpdatedEvent whenever preferences are saved and timezone is truthy, even if the user didn’t actually change the timezone. Since this event is intended to represent a change, consider comparing against the originally loaded timezone and only publishing when the value differs to avoid unnecessary downstream recalculation/work.
| await this.service.update({ homeDashboardUID, theme, timezone, weekStart, language, queryHistory, navbar }); | ||
| window.location.reload(); | ||
|
|
||
| // @PERCONA: Publish TimeZoneUpdatedEvent to allow other components to listen to timezone changes | ||
| if (timezone) { |
There was a problem hiding this comment.
New TimeZoneUpdatedEvent behavior isn’t covered by the existing SharedPreferences tests (this component already has good test coverage). Add/adjust a test to assert the timezone-updated event is published (and not published when unchanged) when the user saves preferences, so regressions around the save flow/reload ordering are caught.
| await this.service.update({ homeDashboardUID, theme, timezone, weekStart, language, queryHistory, navbar }); | |
| window.location.reload(); | |
| // @PERCONA: Publish TimeZoneUpdatedEvent to allow other components to listen to timezone changes | |
| if (timezone) { | |
| const timezoneChanged = timezone && timezone !== config.bootData.user?.timezone; | |
| await this.service.update({ homeDashboardUID, theme, timezone, weekStart, language, queryHistory, navbar }); | |
| window.location.reload(); | |
| // @PERCONA: Publish TimeZoneUpdatedEvent to allow other components to listen to timezone changes | |
| if (timezoneChanged) { |
PMM-14550
With this PR, we send an event to notify the user preferred timezone has changed