Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 34 additions & 13 deletions web-common/src/features/chat/core/messages/chart/ChartBlock.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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;
Expand All @@ -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
Comment on lines +44 to +48

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.

?.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
Expand Down Expand Up @@ -113,16 +132,18 @@
/>

<div class="chart-container">
<ChartContainer
chartType={block.chartType}
{spec}
{timeAndFilterStore}
{project}
theme={$themeQuery?.data}
showExploreLink
{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.

<ChartContainer
chartType={block.chartType}
{spec}
{timeAndFilterStore}
{project}
theme={$themeQuery?.data}
showExploreLink
{organization}
themeMode="light"
/>
{/if}
</div>
</div>

Expand Down
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");
});
});
24 changes: 23 additions & 1 deletion web-common/src/features/chat/core/messages/chart/chart-block.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import type { ChartType } from "@rilldata/web-common/features/components/charts";
import type { V1Message } from "@rilldata/web-common/runtime-client";
import { getDefaultTimeZone } from "@rilldata/web-common/features/dashboards/stores/get-rill-default-explore-state";
import { getUTCIANA } from "@rilldata/web-common/lib/time/timezone";
import type {
V1ExploreSpec,
V1Message,
} from "@rilldata/web-common/runtime-client";
import { MessageContentType } from "../../types";

// =============================================================================
Expand Down Expand Up @@ -53,6 +58,23 @@ export function createChartBlock(
};
}

/**
* Resolves the timezone used to bin and label a chat chart.
*
* Explicit `time_range.time_zone` in the spec is authoritative.
* Otherwise inherit the explore default (`defaultPreset.timezone`, else
* `timeZones[0]`, else UTC). Missing explore context (embed, still loading,
* no dashboard for the metrics view) degrades to UTC.
*/
export function resolveChartTimeZone(
explicitTimeZone: string | undefined,
explore: V1ExploreSpec | undefined,
): string {
if (explicitTimeZone) return explicitTimeZone;
if (!explore) return getUTCIANA();
return getDefaultTimeZone(explore);
}

// =============================================================================
// HELPERS
// =============================================================================
Expand Down
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
Expand Up @@ -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 ||

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.

explore.timeZones?.[0] ||
DEFAULT_TIMEZONES[0];

if (preference === "Local") {
return getLocalIANA();
Expand Down