-
Notifications
You must be signed in to change notification settings - Fork 202
fix: inherit explore default timezone in chat charts without time_zone
#9839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,10 +5,12 @@ | |
| <script lang="ts"> | ||
| import { page } from "$app/stores"; | ||
| import { ChartContainer } from "@rilldata/web-common/features/components/charts"; | ||
| import { useGetExploresForMetricsView } from "@rilldata/web-common/features/dashboards/selectors"; | ||
| import { | ||
| ResourceKind, | ||
| useResource, | ||
| } from "@rilldata/web-common/features/entity-management/resource-selectors"; | ||
| import { selectBestDashboard } from "@rilldata/web-common/features/explore-mappers/explore-validation"; | ||
| import { mapResolverExpressionToV1Expression } from "@rilldata/web-common/features/explore-mappers/map-metrics-resolver-query-to-dashboard"; | ||
| import { Theme } from "@rilldata/web-common/features/themes/theme"; | ||
| import { queryClient } from "@rilldata/web-common/lib/svelte-query/globalQueryClient"; | ||
|
|
@@ -17,7 +19,7 @@ | |
| import { readable } from "svelte/store"; | ||
| import type { V1Tool } from "../../../../../runtime-client"; | ||
| import ToolCall from "../tools/ToolCall.svelte"; | ||
| import type { ChartBlock } from "./chart-block"; | ||
| import { resolveChartTimeZone, type ChartBlock } from "./chart-block"; | ||
|
|
||
| export let block: ChartBlock; | ||
| export let tools: V1Tool[] | undefined = undefined; | ||
|
|
@@ -33,17 +35,34 @@ | |
|
|
||
| $: spec = readable(chartSpec); | ||
|
|
||
| $: explicitTimeZone = | ||
| typeof chartSpec.time_range?.time_zone === "string" && | ||
| chartSpec.time_range.time_zone | ||
| ? chartSpec.time_range.time_zone | ||
| : undefined; | ||
|
|
||
| $: exploresQuery = useGetExploresForMetricsView( | ||
| runtimeClient, | ||
| chartSpec.metrics_view ?? "", | ||
| ); | ||
| $: exploreSpec = selectBestDashboard($exploresQuery.data ?? [])?.explore | ||
| ?.state?.validSpec; | ||
| $: timeZone = resolveChartTimeZone(explicitTimeZone, exploreSpec); | ||
| // Wait for explore lookup when the spec omits time_zone so we don't | ||
| // briefly query UTC and then refetch after inheritance resolves. | ||
| $: timezoneReady = !!explicitTimeZone || !$exploresQuery.isLoading; | ||
|
|
||
| // Extract time range from the chart spec or use defaults | ||
| $: timeRange = chartSpec.time_range | ||
| ? { | ||
| start: chartSpec.time_range.start, | ||
| end: chartSpec.time_range.end, | ||
| timeZone: chartSpec.time_range.time_zone || "UTC", | ||
| timeZone, | ||
| } | ||
| : { | ||
| start: new Date(Date.now() - 7 * 24 * 60 * 60 * 1000).toISOString(), | ||
| end: new Date().toISOString(), | ||
| timeZone: "UTC", | ||
| timeZone, | ||
| }; | ||
|
|
||
| $: comparisonTimeRange = chartSpec.comparison_time_range | ||
|
|
@@ -113,16 +132,18 @@ | |
| /> | ||
|
|
||
| <div class="chart-container"> | ||
| <ChartContainer | ||
| chartType={block.chartType} | ||
| {spec} | ||
| {timeAndFilterStore} | ||
| {project} | ||
| theme={$themeQuery?.data} | ||
| showExploreLink | ||
| {organization} | ||
| themeMode="light" | ||
| /> | ||
| {#if timezoneReady} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While the explores query is loading for a spec without |
||
| <ChartContainer | ||
| chartType={block.chartType} | ||
| {spec} | ||
| {timeAndFilterStore} | ||
| {project} | ||
| theme={$themeQuery?.data} | ||
| showExploreLink | ||
| {organization} | ||
| themeMode="light" | ||
| /> | ||
| {/if} | ||
| </div> | ||
| </div> | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import { resolveChartTimeZone } from "@rilldata/web-common/features/chat/core/messages/chart/chart-block"; | ||
| import type { V1ExploreSpec } from "@rilldata/web-common/runtime-client"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| describe("resolveChartTimeZone", () => { | ||
| const shanghaiExplore: V1ExploreSpec = { | ||
| timeZones: ["Asia/Shanghai", "UTC"], | ||
| }; | ||
|
|
||
| it("keeps an explicit spec time_zone authoritative", () => { | ||
| expect(resolveChartTimeZone("America/New_York", shanghaiExplore)).toBe( | ||
| "America/New_York", | ||
| ); | ||
| }); | ||
|
|
||
| it("inherits the explore default when the spec omits time_zone", () => { | ||
| expect(resolveChartTimeZone(undefined, shanghaiExplore)).toBe( | ||
| "Asia/Shanghai", | ||
| ); | ||
| }); | ||
|
|
||
| it("inherits defaultPreset.timezone when that is the explore default", () => { | ||
| const explore: V1ExploreSpec = { | ||
| defaultPreset: { timezone: "Europe/Paris" }, | ||
| timeZones: ["UTC"], | ||
| }; | ||
| expect(resolveChartTimeZone(undefined, explore)).toBe("Europe/Paris"); | ||
| }); | ||
|
|
||
| it("falls back to UTC when the explore lists no time zones", () => { | ||
| expect(resolveChartTimeZone(undefined, {})).toBe("UTC"); | ||
| }); | ||
|
|
||
| it("falls back to UTC when explore context is absent", () => { | ||
| expect(resolveChartTimeZone(undefined, undefined)).toBe("UTC"); | ||
| }); | ||
|
|
||
| it("does not treat an empty spec time_zone as explicit", () => { | ||
| expect(resolveChartTimeZone("", shanghaiExplore)).toBe("Asia/Shanghai"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import { getDefaultTimeZone } from "@rilldata/web-common/features/dashboards/stores/get-rill-default-explore-state"; | ||
| import { DEFAULT_TIMEZONES } from "@rilldata/web-common/lib/time/config"; | ||
| import { getLocalIANA } from "@rilldata/web-common/lib/time/timezone"; | ||
| import type { V1ExploreSpec } from "@rilldata/web-common/runtime-client"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| describe("getDefaultTimeZone", () => { | ||
| it("uses defaultPreset.timezone over timeZones[0]", () => { | ||
| const explore: V1ExploreSpec = { | ||
| defaultPreset: { timezone: "Asia/Shanghai" }, | ||
| timeZones: ["UTC", "Asia/Shanghai"], | ||
| }; | ||
| expect(getDefaultTimeZone(explore)).toBe("Asia/Shanghai"); | ||
| }); | ||
|
|
||
| it("uses the first explore time zone when no preset timezone is set", () => { | ||
| const explore: V1ExploreSpec = { | ||
| timeZones: ["America/Los_Angeles", "UTC"], | ||
| }; | ||
| expect(getDefaultTimeZone(explore)).toBe("America/Los_Angeles"); | ||
| }); | ||
|
|
||
| it("falls back to DEFAULT_TIMEZONES[0] when the explore lists none", () => { | ||
| expect(getDefaultTimeZone({})).toBe(DEFAULT_TIMEZONES[0]); | ||
| expect(DEFAULT_TIMEZONES[0]).toBe("UTC"); | ||
| }); | ||
|
|
||
| it("resolves Local to the browser IANA zone", () => { | ||
| const explore: V1ExploreSpec = { timeZones: ["Local"] }; | ||
| expect(getDefaultTimeZone(explore)).toBe(getLocalIANA()); | ||
| }); | ||
|
|
||
| it("falls back to UTC for an invalid IANA zone", () => { | ||
| const explore: V1ExploreSpec = { timeZones: ["Not/A_Timezone"] }; | ||
| expect(getDefaultTimeZone(explore)).toBe("UTC"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -246,7 +246,10 @@ export function getGrainForRange( | |
| } | ||
|
|
||
| export function getDefaultTimeZone(explore: V1ExploreSpec) { | ||
| const preference = explore.timeZones?.[0] ?? DEFAULT_TIMEZONES[0]; | ||
| const preference = | ||
| explore.defaultPreset?.timezone || | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| explore.timeZones?.[0] || | ||
| DEFAULT_TIMEZONES[0]; | ||
|
|
||
| if (preference === "Local") { | ||
| return getLocalIANA(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ChartContainer.svelte:117already runs this same explores query andselectBestDashboardthroughuseExploreAvailabilityfor 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 selectedvalidSpecfromuseExploreAvailability, or a shared selector, would make that coupling explicit.