Skip to content

fix: inherit explore default timezone in chat charts without time_zone - #9839

Open
eminemead wants to merge 1 commit into
rilldata:mainfrom
eminemead:xiaofei.yin/fix-chart-timezone-inherit
Open

eminemead wants to merge 1 commit into
rilldata:mainfrom
eminemead:xiaofei.yin/fix-chart-timezone-inherit

Conversation

@eminemead

Copy link
Copy Markdown
  • Chat charts whose spec omits time_zone were always binned in UTC, so day grains did not match the explore default timezone.
  • ChartBlock now resolves timezone the same way dashboards do: explicit spec time_zone, else the explore defaultPreset.timezone / first timeZones entry, else UTC.
  • getDefaultTimeZone honors defaultPreset.timezone first. An explicit spec zone stays authoritative.

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

Developed in collaboration with Claude Code

Explain charts omit time_zone and were binned in UTC. Resolve the same explore default as dashboards, keeping an explicit spec zone authoritative and falling back to UTC when no explore is available.
@nishantmonu51 nishantmonu51 added Type:Bug Something isn't working Area:Time Size:M Medium change: 100-499 lines labels Sep 1, 2026
@nishantmonu51

Copy link
Copy Markdown
Collaborator

1. The getDefaultTimeZone change breaks explore URL state — 20 existing tests fail.

On this branch, npx vitest run src/features/dashboards/url-state fails 20 tests in url-state-variations.spec.ts and convertURLSearchParamsToExploreState.spec.ts, all with the same shape:

Expected: "tr=P7D&tz=Asia%2FKathmandu&compare_tr=rill-PP&..."
Received: "tr=P7D&compare_tr=rill-PP&..."

These pass on main. getRillDefaultExploreState feeds getRillDefaultExploreUrlParams, which getCleanedUrlParamsForGoto uses to strip params that match the defaults. Folding defaultPreset.timezone into getDefaultTimeZone means tz is now dropped from the URL of every dashboard that sets defaults: timezone:, so shared and bookmarked links no longer carry the timezone explicitly.

The layering here is deliberate: DashboardStateDataLoader keeps rillDefaultExploreState and exploreStateFromYAMLConfig as separate layers, and the YAML layer already applies the preset timezone (get-explore-state-from-yaml-config.ts:126). getDefaultExplorePreset also spreads ...explore.defaultPreset after timezone: getDefaultTimeZone(explore), so the preset already wins there. In other words, the change to getDefaultTimeZone is not needed for dashboards — it only shifts the pre-YAML baseline. Resolving the preset inside resolveChartTimeZone instead keeps the fix scoped to the chat chart:

export function resolveChartTimeZone(explicitTimeZone, explore) {
  if (explicitTimeZone) return explicitTimeZone;
  if (!explore) return getUTCIANA();
  return getDefaultTimeZone({
    ...explore,
    timeZones: explore.defaultPreset?.timezone
      ? [explore.defaultPreset.timezone, ...(explore.timeZones ?? [])]
      : explore.timeZones,
  });
}

(or simply read defaultPreset?.timezone first and pass it through the same Local/IANA normalization).

2. The explore that supplies the timezone is chosen arbitrarily, and the selection is duplicated.

selectBestDashboard with the default first_available criteria returns validDashboards[0] — whichever explore ListResources happens to return first. When a metrics view backs several explores with different timeZones, the chart's binning becomes dependent on resource ordering rather than on any user-visible dashboard. ChartContainer already runs the same query and the same selection through useExploreAvailability for the explore link, so the component now performs that selection twice with no guarantee, other than identical inputs, that the timezone comes from the dashboard the link points at. Extending useExploreAvailability to also return the selected validSpec (or adding a shared selector both call) would make the coupling explicit and remove the duplicate.

3. The chart is blank while the explores query is in flight.

{#if timezoneReady} hides ChartContainer entirely, with nothing in its place, so the chart area collapses and then pops back in once ListResources resolves — a visible layout shift in the chat transcript, and one that happens for every chart whose spec omits time_zone. Rendering the container with its existing loading state, or reserving the height, avoids the jump while still preventing the UTC-then-refetch problem the comment describes.

4. Minor: the UTC branch in resolveChartTimeZone is redundant.

getDefaultTimeZone({}) already returns DEFAULT_TIMEZONES[0], which the new spec asserts is "UTC", so if (!explore) return getUTCIANA() can be getDefaultTimeZone(explore ?? {}). Similarly, resolveChartTimeZone already treats "" as absent (there is a test for it), so the typeof … === "string" && … narrowing in ChartBlock.svelte only exists to keep timezoneReady honest — passing chartSpec.time_range?.time_zone straight through and computing readiness from the same value would read more directly.

@nishantmonu51 nishantmonu51 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The create_chart tool prompt is not in this diff but now contradicts the renderer: runtime/ai/create_chart.go:431 documents time_zone as "Optional time zone (defaults to "UTC")" and every example uses Z timestamps, so the model plans UTC-midnight start/end bounds and narrates in UTC while the chart bins in the explore's zone (data-provider.ts:119 passes timeRange.timeZone to the query). For an explore defaulting to Asia/Shanghai, start: 2024-01-01T00:00:00Z is 08:00 on 2024-01-01 local, so the first and last day buckets are partial and the day labels no longer line up with the model's text. Either state the inheritance rule in the prompt or instruct the model to always pass an explicit time_zone.

The branch is 56 commits behind main (merge-base a21215b6), but none of the touched files changed there, so there is no conflict.

export function getDefaultTimeZone(explore: V1ExploreSpec) {
const preference = explore.timeZones?.[0] ?? DEFAULT_TIMEZONES[0];
const preference =
explore.defaultPreset?.timezone ||

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getDefaultTimeZone is the pre-YAML baseline that getRillDefaultExploreState feeds into getRillDefaultExploreUrlParams, and cleanUrlParams/cleanUrlParamsForGoto subtract that baseline from every generated explore URL. Folding defaultPreset.timezone in here means an explore whose YAML sets defaults: timezone: Asia/Kathmandu has tz stripped from its URLs: running the suites against this head gives 44 failures across url-state-variations.spec.ts (10), convertURLSearchParamsToExploreState.spec.ts (10), DashboardStateManager.spec.ts (14) and explore-web-view-store.spec.ts (10), all of the shape Expected: "tr=P7D&tz=Asia%2FKathmandu&..." Received: "tr=P7D&...", and the same suites pass on main. The preset timezone is deliberately applied one layer up, in get-explore-state-from-yaml-config.ts:126 and in getDefaultExplorePreset.ts:70, where ...explore.defaultPreset is spread after timezone: getDefaultTimeZone(explore) so the preset already wins. The chat chart only needs the preset-first rule inside resolveChartTimeZone (read explore.defaultPreset?.timezone first and put it through the same Local/IANA normalization), which lets this hunk and the first case of its new spec be dropped.

Comment on lines +44 to +48
$: exploresQuery = useGetExploresForMetricsView(
runtimeClient,
chartSpec.metrics_view ?? "",
);
$: exploreSpec = selectBestDashboard($exploresQuery.data ?? [])?.explore

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ChartContainer.svelte:117 already runs this same explores query and selectBestDashboard through useExploreAvailability for the "open in explore" link, so this duplicates it, and the inherited zone agrees with the link target only because both inputs happen to be identical. Exposing the selected validSpec from useExploreAvailability, or a shared selector, would make that coupling explicit.

{organization}
themeMode="light"
/>
{#if timezoneReady}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the explores query is loading for a spec without time_zone, this gate renders an empty 400px .chart-container instead of ChartContainer's own loading state, so the chart pops in only once ListResources resolves. Gating the query rather than the component would keep the skeleton.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area:Time Size:M Medium change: 100-499 lines Type:Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants